allow reference to existing secret as imagePullSecret #7657
Conversation
ozdanborne
added
release-note-required
charts/tigera-operator/values.yaml
Outdated
| @@ -1,8 +1,12 @@ | |||
| # imagePullSecrets specified here will result in helm attempting to create a secret resource. | |||
There was a problem hiding this comment.
I think it's important with this change that we give a bit more detail about the expected format of each of these fields, since IIUC they now differ pretty substantially.
one of them is a map of secret name to secret data, and one of them is a slice of pre-existing secret names. I think adding an example to the comment / docstring would help a lot.
There was a problem hiding this comment.
Updated to better explain the fields and their format. But to be clear - you say "they now differ pretty substantially" but this PR has not changed their format. Previously, you needed to use the toplevel field with helm and avoid using the field directly nested in the installation resource field. Any specification of the nested field would be discarded. Now, you can use both, and behavior for the nested field is now defined. Though the nested field was discarded before, it always mapped to a LocalObjectReference on the installation resource.
But anyways, now that both fields can be used, it makes sense to beef up the documentation of formatting on both. I'll update the PR description to cover this more clearly as well.
ozdanborne
force-pushed
the
helm-reference-image-pull-secret
branch
2 times, most recently
from
6046f28
to
9ca05e4
Compare
| */}} | ||
|
|
||
| {{- define "tigera-operator.imagePullSecrets" -}} | ||
| {{- $secrets := default list .Values.installation.imagePullSecrets -}} |
There was a problem hiding this comment.
I suppose if a secret is defined in both places we'll get an error back from k8s?
This is probably OK, although I can definitely imagine users trying to put a secret in both places.
charts/tigera-operator/values.yaml
Outdated
| @@ -1,8 +1,20 @@ | |||
| # imagePullSecrets are a special helm field which, when specified, creates a secret | |||
There was a problem hiding this comment.
"imagePullSecrets are" doesn't sound right to me (despite it being plural). It's just a single field.
charts/tigera-operator/values.yaml
Outdated
| @@ -1,8 +1,20 @@ | |||
| # imagePullSecrets are a special helm field which, when specified, creates a secret | |||
| # containing the pull secret and configures operator's serviceaccount to use it to pull the operator image | |||
There was a problem hiding this comment.
Maybe just "all images deployed by this helm chart and the resulting operator" is a more concise way of saying this?
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| var _ = Describe("Tigera Operator Helm Chart", func() { |
There was a problem hiding this comment.
You've already written it, so I won't make you change it, but I think we've said all new test suites should be written in standard go test without ginkgo / gomega, in case you're interested in tweaking these.
ozdanborne
force-pushed
the
helm-reference-image-pull-secret
branch
from
9ca05e4
to
2247bd4
Compare
allows reference to existing secrets for imagePullSecrets without passing the secret itself. this enables management of secrets by an external system like sealedsecrets and prevents the secret data from being stored in helm. it works by allowing use of the installation's imagePullSecret field directly instead of the toplevel imagePullSecrets field
ozdanborne
force-pushed
the
helm-reference-image-pull-secret
branch
from
2247bd4
to
cb93297
Compare
Description
new feature - allows reference to existing secrets for imagePullSecrets without passing the secret itself. this enables management of secrets by an external system like sealedsecrets and prevents the secret data from being stored in helm.
It works by allowing use of the installation's imagePullSecret field directly, whereas previously only the toplevel imagePullSecrets field was used.
also added basic unit tests for the feature via helm terratest which makes it easier to render the chart given certain values and assert based off the resulting k8s resources.
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.