[rollout-operator] Replace default webhooks.selfSignedCertSecretName with fullname #3990
Conversation
…d use chart's fullname Signed-off-by: tanner <[email protected]>
kimxogus
force-pushed
the
feature/remove-default-secret-name
branch
from
5074f2a to
2cb15fb
Compare
dimitarvdimitrov
left a comment
dimitarvdimitrov
left a comment
There was a problem hiding this comment.
I left a couple of small comments
Perhaps my major question would be if this change would require manual upgrade path. If i understand correctly, the rollout operator would just ignore the existing secret and create a new one. There will be an orphaned secret in the namespace, but that shouldn't cause problems. Did i get this right? (it may be worth including these details in the PR description, since we don't have a changelog)
| enabled: true | ||
| # -- Validating and mutating webhook failure policy. `Ignore` or `Fail`. | ||
| failurePolicy: "Fail" | ||
| # -- Secret resource name for the TLS certificate to be used with the webhooks |
There was a problem hiding this comment.
can you clarify what the default is if this value is empty?
| {{- if .Values.webhooks.enabled }} | ||
| - -server-tls.enabled=true | ||
| - -server-tls.self-signed-cert.secret-name={{ .Values.webhooks.selfSignedCertSecretName }} | ||
| - -server-tls.self-signed-cert.secret-name={{ .Values.webhooks.selfSignedCertSecretName | default (include "rollout-operator.fullname" . ) }} |
There was a problem hiding this comment.
can {{ .Values.webhooks.selfSignedCertSecretName | default (include "rollout-operator.fullname" . ) }} maybe go in a named template in _helpers.tpl? that way we don't have to remember to update it in multiple places next time
|
I have double checked that we have the correct permissions for this certificate to be re-created. This change looks to be safe, but could we just update the values.yaml and/or README/md.gotmpl with additional documentation explaining that the operator can safely update this value if need be. Is there a specific issue that this generic name is causing? A conflict in the namespace perhaps? |
4 participants
Certificate secret's default name
certificateis too general name, so I made it chart's fullname to match with the other resources.Changing cert secret name doesn't break existing installations. It will recreate rollout-operator pod and cert secret will be recreated as well.