Allow VPA admission controller to reload the caBundle certificate and patch the webhook #7454
Conversation
k8s-ci-robot
added
the
kind/feature
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @maxcao13! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: maxcao13 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 |
|
Hi @maxcao13. 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. |
k8s-ci-robot
added
needs-ok-to-test
k8s-ci-robot
added
size/M
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
maxcao13
force-pushed
the
reload-ca
branch
2 times, most recently
from
cb06d72
to
5681a01
Compare
| @@ -92,6 +119,36 @@ func (cr *certReloader) load() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (cr *certReloader) reloadWebhookCA() error { | |||
There was a problem hiding this comment.
Could you try add a test for this?
There was a problem hiding this comment.
k8s-ci-robot
added
size/L
| _, err2 := client.Patch(context.TODO(), webhookConfigName, types.StrategicMergePatchType, patch, metav1.PatchOptions{}) | ||
| if err2 != nil { | ||
| return err2 |
There was a problem hiding this comment.
Any reason to use err2?
| _, err2 := client.Patch(context.TODO(), webhookConfigName, types.StrategicMergePatchType, patch, metav1.PatchOptions{}) | |
| if err2 != nil { | |
| return err2 | |
| _, err := client.Patch(context.TODO(), webhookConfigName, types.StrategicMergePatchType, patch, metav1.PatchOptions{}) | |
| if err != nil { | |
| return err |
…anged. Also changes the way the certReloader watches the certs/keys by watchign the files individually instead of the mounted directory. Signed-off-by: Max Cao <[email protected]>
Signed-off-by: Max Cao <[email protected]>
|
This seems good to me. Just two changes: |
|
Oh, and the release note needs changing:
This PR extends the reload-cert flag to include the CA cert, it doesn't add it. |
| if event.Has(fsnotify.Remove) || event.Has(fsnotify.Create) || event.Has(fsnotify.Write) { | ||
| if event.Name == cr.tlsCertPath || event.Name == cr.tlsKeyPath { | ||
| klog.V(2).InfoS("New certificate found, reloading") | ||
| if err := cr.load(); err != nil { | ||
| klog.ErrorS(err, "Failed to reload certificate") | ||
| } | ||
| } else if event.Name == cr.clientCaPath { | ||
| if err := cr.reloadWebhookCA(); err != nil { | ||
| klog.ErrorS(err, "Failed to reload client CA") | ||
| } | ||
| } else { | ||
| continue | ||
| } | ||
| // watches get removed along with the symlinks, so we need to add them back | ||
| if event.Has(fsnotify.Remove) { | ||
| if err = watcher.Add(event.Name); err != nil { | ||
| klog.ErrorS(err, "Failed to add watcher for file", "filename", event.Name) | ||
| } |
There was a problem hiding this comment.
Can we replace all those if statements with a switch? The current code is hard to follow, and using a switch could improve readability. For example:
if !event.Has(fsnotify.Remove) && !event.Has(fsnotify.Create) && !event.Has(fsnotify.Write) {
return
}
switch event.Name {
case cr.tlsCertPath, cr.tlsKeyPath:
klog.V(2).InfoS("New certificate found, reloading")
if err := cr.load(); err != nil {
klog.ErrorS(err, "Failed to reload certificate")
}
case cr.clientCaPath:
if err := cr.reloadWebhookCA(); err != nil {
klog.ErrorS(err, "Failed to reload client CA")
}
default:
return
}
if event.Has(fsnotify.Remove) {
if err := watcher.Add(event.Name); err != nil {
klog.ErrorS(err, "Failed to add watcher for file", "filename", event.Name)
}
}
| if webhook == nil { | ||
| return fmt.Errorf("webhook not found") | ||
| } | ||
| if len(webhook.Webhooks) > 0 { |
There was a problem hiding this comment.
How about we rearrange the statements?
if len(webhook.Webhooks) == 0 {
return fmt.Errorf("webhook configuration has no webhooks")
}
currentBundle := webhook.Webhooks[0].ClientConfig.CABundle[:]
...
What type of PR is this?
/kind feature
What this PR does / why we need it:
There is currently no functionality to reload the caBundle in the vpa webhook if it was changed. Currently only the server cert and keys are reloaded.
Additionallity, this PR changes the way the files are watched. Previously, the watcher was watching the entire directory (i.e.
/etc/tls-certswhich resulted in threefsnotify.Createevents on 1 change because of the way Kubernetes updates mounted Secret/ConfigMaps. Here's a blog that explains it more thoroughly, but the mounted files are actually symbolic links that are linked to another symbolic link.This PR changes the implementation to watch the files directly, which only results in
fsnotify.Removeevents. This was changed so that we can figure out the names of the watched files being changed. From the blog:Which issue(s) this PR fixes:
Related to: #6665
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Probably needs an update to the
reload-certflag e.g.