Set owner reference to secrets created by webhook cert manager #530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lkysow
reviewed
Jun 10, 2021
| @@ -59,6 +62,10 @@ func (c *Command) init() { | |||
| c.flagSet = flag.NewFlagSet("", flag.ContinueOnError) | |||
| c.flagSet.StringVar(&c.flagConfigFile, "config-file", "", | |||
| "Path to a config file to read webhook configs from. This file must be in JSON format.") | |||
| c.flagSet.StringVar(&c.flagDeploymentName, "deployment-name", "", | |||
| "Name of deployment that is the owner of the secret") | |||
There was a problem hiding this comment.
Not clear what "the secret" is. I think we can just say name of the deployment this pod is running in. What we do with that is an impl detail
| @@ -251,6 +281,14 @@ func (c *Command) reconcileCertificates(ctx context.Context, clientset kubernete | |||
|
|
|||
| certSecret.Data[corev1.TLSCertKey] = bundle.Cert | |||
| certSecret.Data[corev1.TLSPrivateKeyKey] = bundle.Key | |||
| certSecret.OwnerReferences = []metav1.OwnerReference{ | |||
There was a problem hiding this comment.
add a comment that this is here to update existing secrets that were created before we added ownerReference?
thisisnotashwin
force-pushed
the
webhook-secrets-owner
branch
from
56e6430
to
1161412
Compare
lkysow
approved these changes
Jun 10, 2021
oh oh I did!! i forgot to put it in here. you were right. the pods just keep them in memory and continue to work. it only becomes problematic if the cert expires or the pod has to restart. |
When the certificate secret is created or updated, set an OwnerReference on the secret as the webhook-cert-manager deployment. This ensures that deletion of the deployment will also delete the secrets. This addresses the race condition bug that we sometimes see when re-installing consul on a cluster that had a consul deleted from it. This was because the helm delete would not delete the existing secrets with certificates. When the controller would get created with a new installation, it would mount the existing secret (which was stale) and the secret on disk would get rotated before the cert watcher started which would lead to the controller using certificates signed by a CA different from the CA bundle on the MWC which would lead to x509 errors. This change would ensure the secrets get deleted every single time and hence, a new secret would always get created during a helm install. This also ensure an existing secret, when updated is updated with the owner ref ensuring helm upgrades or installs to a cluster with an existing secret give people the desired behavior as well.
thisisnotashwin
force-pushed
the
webhook-secrets-owner
branch
from
1161412
to
7c57bdb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this PR:
When the certificate secret is created or updated, set an OwnerReference on the secret as the
webhook-cert-managerdeployment. This ensures that deletion of the deployment will also delete the secrets. This addresses the race condition bug that we sometimes see when re-installing consul on a cluster that had a consul deleted from it. This was because the helm delete would not delete the existing secrets with certificates. When the controller would get created with a new installation, it would mount the existing secret (which was stale) and the secret on disk would get rotated before the cert watcher started which would lead to the controller using certificates signed by a CA different from the CA bundle on the MWC which would lead to x509 errors.This change would ensure the secrets get deleted every single time and hence, a new secret would always get created during a helm install. This also ensure an existing secret, when updated is updated with the owner ref ensuring helm upgrades or installs to a cluster with an existing secret give people the desired behavior as well.
How I've tested this PR: hashicorp/consul-helm#987
How I expect reviewers to test this PR: Code review
Checklist: