feat: create root ca. #510
Conversation
Updates the controller to create the root ca used in all the certificates used by the Kubewarden stack. Signed-off-by: José Guilherme Vanz <[email protected]>
| // secret change. Therefore, the certificate can be | ||
| // recreated and the webhooks updated |
There was a problem hiding this comment.
I'm still going through the whole PR, hence I might be wrong...
the certificate can be recreated
AFAIK the certificate is inside of the secret, hence the only thing to recreate (actually restart) would be the Deployment running Policy Server (so that it picks the new certificate).
Moreover:
the webhooks updated
The webhooks should not be updated when the certificate of the Policy Server is changed. That's because they are registered with the root CA certificate specified, not with the Policy Server certificate.
The only exception to this "rule" is when the root CA is updated, leading to a change of the Policy Server certificate. Only in this case the webhooks should be updated.
I haven't read the code yet, maybe all these things are already done. Nevertheless, the comment should be changed
There was a problem hiding this comment.
Yes, this is what is done in the controller. The comment is wrong. I'm still walkingthrouhg your comments. But this is the goal. The webhooks always use the root CA to validate the server certificate and the policy server deployment will rollout a new version when the secret version change (indicating that policy server certificate change)
| secret, err := r.buildPolicyServerCASecret(policyServerName, caSecret, generateCert) | ||
| if err != nil { | ||
| return secret, fmt.Errorf("cannot fetch or initialize Policy Server CA secret: %w", err) | ||
| func ReconcileSecret(ctx context.Context, clusterClient client.Client, secret *corev1.Secret) error { |
There was a problem hiding this comment.
Sorry, not related to this line of code but to line 22: I did a quick search and the private method reconcileCASecret is never used inside of this file. I think we should remove it
There was a problem hiding this comment.
Actually, it's used here. But maybe we can merge both functions in to one. I'll double check that out
| // When the controller is run for the first time, there | ||
| // is no root CA. Therefore, we need to restart the pod | ||
| // to ensure that secret data is available for the | ||
| // controller before start running the manager. Here we | ||
| // just exit the controller. Thus, the control plane | ||
| // will launch another container with access to the | ||
| // root CA data initialized in the first run. |
There was a problem hiding this comment.
Why do we have to restart? The HTTP endpoint of the webhook server is not yet running
| // validatingWebhooks are the ValidatingWebhookConfiguration names which should | ||
| // be updated when the root CA is initialized | ||
| // | ||
| // mutatingWebhooks are the MutatingWebhookConfiguration names which should | ||
| // be updated when the root CA is initialized |
There was a problem hiding this comment.
Aren't these names hard coded by us?
| return []byte{}, []byte{}, false, fmt.Errorf("failed to initialize root CA certificate") | ||
| } | ||
|
|
||
| controllerSecret, initialized, err := admission.FetchOrInitializeCertificate(ctx, clusterClient, controllerWebhookService, deploymentsNamespace, constants.ControllerCertificateSecretName, caSecret, admissionregistration.GenerateCert) |
There was a problem hiding this comment.
I think the code below this line should be moved to another function. The name of the this function is setupCA, hence it should only ensure the root CA is properly created.
| // If the controller is NOT running in development mode, the | ||
| // webhooks are created by Helm chart and missing the | ||
| // `caBundle` field with the root CA certificate. Therefore, | ||
| // the controller needs to patch these resources |
There was a problem hiding this comment.
That means that when the controller is deployed via helm our helm charts take care of creating the webhook configurations? Can't we have the controller always create them? I mean, we already have the code somewhere to make the development environment work... This would make everything more consistent
There was a problem hiding this comment.
Moreover, the name of this function is confusing, it should be setupCAWebhooksInProductionMode, but again, I hope we can get rid of this code and have the webhook configurations managed by the controller in any kind of deployment environment
There was a problem hiding this comment.
Having a main_test is not good, please move all the functions being tested to a different file and then rename this file accordingly.
Description
Updates the controller to create the root ca used in all the certificates used by the Kubewarden stack.
Related to #493