This repository has been archived by the owner on Aug 25, 2021. It is now read-only.
Provide deployment info to webhook cert manager pod #987
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
2 tasks
thisisnotashwin
force-pushed
the
webhook-secrets-owner
branch
from
db0ebc9 to
7b72269
Compare
lkysow
reviewed
Jun 10, 2021
Comment on lines
41
to
44
| - get | ||
| - list | ||
| - watch | ||
| - patch |
There was a problem hiding this comment.
yeah..was a copypasta slipup
thisisnotashwin
force-pushed
the
webhook-secrets-owner
branch
from
7b72269 to
8078622
Compare
lkysow
approved these changes
Jun 10, 2021
kschoche
approved these changes
Jun 11, 2021
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
8078622 to
adec53c
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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:
-deployment-nameand-deployment-namespaceto webhook cert manager. This allows the cert-manager to set the deployment as the owner of the secrets it creates.How I've tested this PR: Manually with a helm install and uninstall. The secrets get deleted post install. Additionally, performing a helm upgrade on a previous helm install adds an OwnerReference to the secret which should allow people upgrade to this version of the chart to have their secrets owned by the deployment.
How I expect reviewers to test this PR: Repeat above steps.
Checklist: