-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add Helm-managed webhook with kube-webhook-certgen #8870
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Omer Aplatony <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omerap12 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cc @adrianmoisey |
iamzili
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Omer, I checked out your branch and I'm having trouble getting the admission controller to work. I would expect the following command to deploy everything properly:
helm upgrade vpa \
/home/zili/Repos/autoscaler/vertical-pod-autoscaler/charts/vertical-pod-autoscaler \
--install \
--version 0.6.0 \
--namespace vpa
What I have found so far is that the following settings seem to be incorrect:
- The
--webhook-servicevalue in the admission controller's Deployment. - The service name in the
MutatingWebhookConfigurationobject.
I'm also seeing an error in the admission controller when it attempts to perform actuation:
2025/12/04 13:37:32 http: TLS handshake error from 10.244.0.1:25304: remote error: tls: bad certificate
| metadata: | ||
| name: {{ include "vertical-pod-autoscaler.admissionController.certGen.fullname" . }} | ||
| annotations: | ||
| "helm.sh/hook": pre-install,pre-upgrade,post-install,post-upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a specific reason why this resource (and I see the same pattern in several others, such as ClusterRoleBinding, Role, RoleBinding) defines both pre and post hooks? I think pre-install,pre-upgrade would be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, you’re right - pre-install and pre-upgrade are enough, but it really depends on how we want to handle things going forward. As mentioned above, the hook currently creates a Secret containing all certificates, CA bundles, and the mutating webhook configuration. Because this Secret already exists during an upgrade, kube-webhook-certgen will not rotate the certificate values.
To address this, we may want to add post-install and post-upgrade hooks to delete the Secret, ensuring that on the next upgrade kube-webhook-certgen generates a fresh one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But using post-install and post-upgrade hooks to delete the Secret is still not a good idea, I assume because it would mean that after a Helm upgrade or install, there would be no Secret object in the cluster as Helm executes post-install and post-upgrade hooks after all non-hook resources have been deployed to the cluster.
If we want to delete the certificate before running a Helm upgrade or install, then we need to do it in pre-install and pre-upgrade hooks, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more thing: kube-webhook-certgen generates certificates that expire after 100 years, so I assume we don't need to rotate them. We could just ignore the "secret already exists" log when kube-webhook-certgen create runs.
|
Also I'm not sure if we need to bump the chart version every time at this stage of the chart's development here:
|
Thanks for review! I'll check that out |
Yeah, we already discussed it. if I remember correctly we have to do it because of the pre-commit. |
I can't remember, your memory may be correct, since cluster-autoscaler does it. However, I think that may be broken now? |
| webhooks: | ||
| - admissionReviewVersions: | ||
| - v1 | ||
| clientConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this config needs to be populated with the caCert
|
Let me add my thoughts regarding
|
I agree that we shouldn’t build our own mechanism for generating and renewing self-signed certificates. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Implements Helm-managed
MutatingWebhookConfigurationwith automatic TLS certificate generation usingkube-webhook-certgen. This replaces the application's self-registration logic.Which issue(s) this PR fixes:
Related to #8587
Special notes for your reviewer:
ingress-nginxis dead, so I’m not sure about the future ofkube-webhook-certgen, which is part of the old nginx stack (registry.k8s.io/ingress-nginx/kube-webhook-certgen). Does that mean we should use it?Right now, the hook simply creates a
Secretcontaining all the certificates, CA bundles, and the mutating webhook configuration. To rotate or update those certificates, we would need just to add a Job that deletes the Secret before.This is just an initial proposal, I’d like to hear other opinions on this setup.
Also, I have noticed some RBAC problems so I fixed those.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: