-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Added webhook-annotations flag to VPA admission-controller #7472
base: master
Are you sure you want to change the base?
Conversation
Hi @wcarlsen. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test Forgive me for not understanding, but if cert-manager is handling the CA bundle in the mutating webhook, doesn't the VPA also need to reload when the CA bundle changes? Or is this handling the case when the CA stays static, and only the certificates themselves are rotating? |
It already has this capability as far as I understand
|
At the moment it only reloads the cert and server key. If the CA bundle changes, my understanding is that it won't reload. If cert-manager is rotating the CA too (I don't know if it does), then this PR depends on #7454. If cert-manager is not rotating the CA, and is only rotating the server cert and key, then this doesn't depend on #7454 But, I'm not too familiar with this flow, so I could be wrong. |
Ok, I had a quick look at the code. The VPA uses clientCaFile in order to register the webhook. This PR will allow cert-manager to manage both CA and certificate/key. But cert-manager will be responsible for writing it into the webhook's configuration and VPA will be responsible for reloading the certificate/key for itself. That makes me think that this change will conflict with #7454, since setting |
At least now as soon as a certificate is renewed, by e.g. cert-manager, you get My take is that #7454 is the long term goal and the right solution in the end, but annotating the webhook offer an easier reviewable solution right now. But sure if one wanted split responsibility of updating the webhook it could make sense to introduce |
It seems like #7454 will solve this issue, since when cert-manager rotates the cert and ca, VPA will update the webhook and it will reload the certs. Any reason to not wait for that change? |
You are right and I'm totally cool with that. It is just hard for me to predict where we are at in terms of progress on that PR and a release. The experience is not great once you finally hit the edge case and it is kind of hard to debug. But it can be mitigated by manually adding the ca-injector annotation, since it won't be reconciled by the admission controller and then wait for an upstream fix. You are welcome to close the PR unless we can up with another argument on why it would be nice with an annotation flag. |
I'm unsure about a release, but I don't think this PR will shortcut the other one and be released first.
I agree that an annotation may be useful, I just don't see this use case as being one of them |
That is awesome news.
I will let that being up to you to decide and I'm totally fine with updating the release notes section too. No pressure and thanks 👍 |
@wcarlsen A side-note to unblock you with this use-case: Start up the admission-controller with the additional flag We're doing something similar in our platform and it works like a charm. For context: I consider this self-registration as some kind of odd feature that tries to improve the batteries-included experience for people who don't have a way to automatically generate certs (especially in combination with our |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wcarlsen The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Willi Carlsen <[email protected]>
Brilliant. Thanks blowing life into this idea! |
/remove-area balancer |
/remove-area provider/oci |
Since #7454 seems to be somewhat stuck in valid discussions, this offers a relatively easy to review alternative solution to the issue. Yes I can prevent the admission controller from registering the webhook with I would very much prefer #7454, but until then can we please have webhook annotations to mitigate this. |
Progress is happening over there, and should hopefully be concluded soon. |
What type of PR is this?
/kind bug
/kind feature
What this PR does / why we need it:
The VPA admission controller when deployed is bundled with a certificate. The responsibility of certificate renewal can be automatically handled with cert-manager, upon renewal the admission controller reloads the certificate, but the caBundle on the mutatingwebhook managed by the admission controller isn't updated.
Cert-manager comes with a ca-injector that exactly handles updating the CA certificates for mutating webhooks via an annotation
cert-manager.io/inject-ca-from: kube-system/vpa-cert-by-certmanager
see ca-injector docs. Thanks to @abstrask for coming up with the idea.Which issue(s) this PR fixes:
Related to PR #7454 and issue #6665
Special notes for your reviewer:
I still think the PR #7454 is in the end the right approach, but this at least offer a low hanging alternative solution.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: