feat: use helm genCA to generate a certificate for the mutating web hook

[cristicalin]

This PR adds the capability to generate a temporary CA using the helm genCA function and install it for the mutating web hook.

[mumoshu]

Does this imply that there might be some admission webhook downtime each time you run helm upgrade, because it would take some time until all the ARC processes and ARC pods catches up the updated serving-cert secret, where the caBundle included in the admissionwebhookconfigs are updated without any delay?

[cristicalin]

Yes, that is what is going to happen (see helm/helm#10731), until the pods refresh the certificate, the kubernetes apiserver will get a TLS error calling the admissions web hook. It is not ideal but this is a solution for when you don't want to run cert-manager

An option would be to move this to a helm pre-start hook but I have an issue with the way code is laid out in the chart because you cannot share the cert or ca objects between different helm files so the hook would need to contain both the certificate config map as well as the mutating web hook configuration.

Other projects like kyverno use the CA secret to generate the web hook definitions from the application (see https://github.com/kyverno/kyverno/blob/42a2df56c17af311a7c006df1532d26f58347547/pkg/webhookconfig/registration.go#L241) but with the ARC doing it from the helm chart it is difficult to properly correlate this dependency between the certificate and the mutatingwebhookconfiguration.

[mumoshu]

Yes, that is what is going to happen

Gotcha!

I think you've already covered it in the new README section and given it's documented I think it's fine.

An option would be to move this to a helm pre-start hook

Looks very hard indeed! Thanks for sharing your insight here.

but with the ARC doing it from the helm chart it is difficult to properly correlate this dependency between the certificate and the mutatingwebhookconfiguration.

Understood. If we were to follow a path similar to kyverno, we should redesign ARC to create/update the mutatingwebhookconfigs by itself, not by helm.

kyverono does seem to have it's own certmanager (not cert-manager :) ) controller which probably depends on the user provided root CA to generate certs. Doing same on ARC would enable us to potentially drop the requirement on cert-manager, simplifying the deployment of ARC. Are you interested in it?

[cristicalin]

I could try to take a stab at it but it will take some time since I would be doing this in my spare time. Indeed moving the logic to ARC code instead of helm looks nicer as a design.

Could we do that in a separate PR though?

[mumoshu]

Thanks for confirming! Yeah, we should do it in another issue/PR. This one is definitely LGTM at this point- Let me merge this soon 👍

[mumoshu]

I realized that controller-runtime kindly watches and reloads the tls.crt and tls.key on update!

https://sourcegraph.com/github.com/kubernetes-sigs/controller-runtime/-/blob/pkg/webhook/server.go?L221#tab=references

Note though, regardless of whether we use cert-manager or this cert generation feature, helm-upgrade might result in some downtime due to K8s secret volume mount takes some time, a minutes or so, according to https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/ --sync-frequency , to reflect changes made in the corresponding k8s secret.

It would be even greater if you could make the chart to regenerate the ca, the cert and the key only on expiration. Is that possible?

[cristicalin]

Unfortunately the implementation of genCA does not cache the certificate between helm runs so we can be in either of two situations. We generate the CA in a helm hook so it is never updated by helm, this has the downside of having to also create the mutatingwebhookconfiguration as a helm hook due to helm scopes; or we can rotate the certificate each time causing a short outage to the web hook while ca rotation propagates.

The solution is intended to cover a specific corner case where you don't provide your own certs and you don't want to deploy cert-manager. My particular use-case is to have dedicated clusters for GitHub runners so an outage of the web hook wile we refresh the chart only causes minimal delays in spinning up workers, on clusters with high pod churn rates I can see this being a problem but if it is properly documented and called out I think it is a liveable tradeoff.

[mumoshu]

Got it! Thanks a lot for your clarification. I do think it's a liveable tradeoff as long as it's well documented as you've already one in this PR 👍 I'm going to merge this.

BTW, what if we enhanced ARC to embed it's own cert manager (not jetstack/cert-manager) as I've outlined in #1780 (comment) and enabled ARC to create/update the mutatingwebhookconfigs by itself? It would potentially simplify ARC deployments, in exchange of additional permissions required by ARC(CRUD permissions on cluster-wide mutatingwebhookconfig resources, which may be huge and not everyone could allow it).
Would you use it if we managed to implement it?

[cristicalin]

We already use kyverno that has this approach, thus why I gave it as an example above so for our use-case the extra permissions would not be an issue but we could implement conditional logic in ARC so that it can create the mutatingwebhook configuration when permissions are granted and still rely on user provided certs with helm created web hook when the user does not want to grant those extra permissions. It would seem to complicate the logic though.

[mumoshu]

LGTM. Thanks a lot for your contribution ☺️