-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Helm chart now allows to enable TLS and basic auth #302
Conversation
Signed-off-by: Vadym Fedorov <vfedorov@nvidia.com>
…es is not recommended for production environments. Signed-off-by: Vadym Fedorov <vfedorov@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor things, looking god otherwise.
deployment/templates/daemonset.yaml
Outdated
{{- if and .Values.tlsServerConfig.enabled }} | ||
- name: "tls" | ||
secret: | ||
secretName: {{ include "dcgm-exporter.tlsCertsSecretName" . }} | ||
defaultMode: 0664 | ||
{{- end }} | ||
{{- if or .Values.tlsServerConfig.enabled $.Values.basicAuth.users}} | ||
- name: "web-config-yaml" | ||
configMap: | ||
name: {{ include "dcgm-exporter.webConfigConfigMap" . }} | ||
defaultMode: 0664 | ||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohit-arora-dev, can you be more specific about where the issue is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at the file, it has indentation issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 81-92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohit-arora-dev Why do you think indentation is wrong when K8S accepts this yaml without issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If your question is about the if
- end
blocks, it is made intentionally to make it easy to see when the block starts and ends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a mix of styles, the file in general follow a style where if-end aligns with the indentation, and does not stand out. Even in other parts of the code where you have added new if-end blocks the indentation is correct, but for this specific block it is different.
My suggestion is to make it consistent across the file, either change all the if-end to follow this style or this block should follow the style consistent with the rest of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohit-arora-dev done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, definitely looking better.
shouldResolvePath() | ||
|
||
kubeClient = shouldCreateKubeClient(k8sConfig) | ||
|
||
helmClient = shouldCreateHelmClient(k8sConfig) | ||
}) | ||
kubeConfigShouldExists() | ||
|
||
AfterAll(func(ctx context.Context) { | ||
_, _ = fmt.Fprintln(GinkgoWriter, "Clean up: starting") | ||
|
||
shouldUninstallHelmChart(helmClient, helmReleaseName) | ||
shouldCleanupHelmClient(helmClient) | ||
k8sConfig := shouldCreateK8SConfig() | ||
|
||
shouldDeleteNamespace(ctx, kubeClient) | ||
kubeClient = shouldCreateKubeClient(k8sConfig) | ||
|
||
_, _ = fmt.Fprintln(GinkgoWriter, "Clean up: completed") | ||
}) | ||
helmClient = shouldCreateHelmClient(k8sConfig) | ||
|
||
It("should create namespace", func(ctx context.Context) { | ||
shouldCreateNamespace(ctx, kubeClient, labels) | ||
BeforeAll(func(ctx context.Context) { | ||
shouldCreateNamespace(ctx, kubeClient, testRunLabels) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity why are these not part of BeforeAll
anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohit-arora-dev Is it an old comment? BeforeAll now creates a namespace as part of the BeforeAll.
Global variables such as kubeClient, helmClient, and testRunLabels initialize once in the Context, which is the container (https://onsi.github.io/ginkgo/#organizing-specs-with-container-nodes. ) for e2e tests. After global variables are initialized, they are available for child specs, like BeforeAll, etc.
flag.BoolVar(&testContext.noCleanup, | ||
"no-cleanup", | ||
false, | ||
`Skip clean up after tests execution`) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what scenario we need this flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohit-arora-dev Troubleshooting. E2E destroys all resources after the end of the test or cancelation. The flag allows to keep created resources for the investigation of failures.
Signed-off-by: Vadym Fedorov <vfedorov@nvidia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/LGTM
This PR adds the ability to enable TLS and HTTP Basic authentication for dcgm-exporter.
The following configuration values were added:
Also, new E2E tests were added:
The E2E suite now allows to specify which tests to run:
make e2es-tests
- run all e2e tests;make e2e-basic-auth
- run e2e scenario when basic auth is enabled;make e2e-tls
- run e2e scenario when TLS is enabled;make e2e-default
- run e2e scenario when tests deploy the helm chart with the default configuration.