Watch cert files and reload on change #141
Conversation
The current webhook does not watch for cert renewal changes. When the cert is renewed, the pod must be restarted to reload the new cert. This commit watches the cert files and reloads the cert when a change is detected. See: kubernetes-sigs#135
k8s-ci-robot
added
the
cncf-cla: no
k8s-ci-robot
added
the
sig/windows
|
Welcome @ycheng-kareo! |
k8s-ci-robot
added
the
size/L
|
@ycheng-kareo thank you! can you sign the CLA so we can review and run the tests? |
|
Yes, certainly. I'm working on getting the CLA signed. Stay tuned.
…On Thu, Feb 8, 2024 at 8:22 AM James Sturtevant ***@***.***> wrote:
@ycheng-kareo <https://github.com/ycheng-kareo> thank you! can you sign
the CLA so we can review and run the tests?
—
Reply to this email directly, view it on GitHub
<#141 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AWLSYRLM3PM7HR5LSQUICXTYST3TLAVCNFSM6AAAAABC7J3DAWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZUGQ3TQNRTGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
YE CHENG
VP, SOFTWARE ENGINEERING
<https://www.tebra.com>
949.354.3886
TEBRA.COM
<https://www.tebra.com>
|
k8s-ci-robot
added
cncf-cla: yes
|
@jsturtevant not sure if you had a chance to review the changes yet. any feedback or changes you'd like to see? thank you! |
|
Thanks for the ping, I wasn't notified via the email notifications that the CLA was signed. I will take a look today |
There was a problem hiding this comment.
Thank you for your contribution! It would be nice to have an e2e tests for this as well. Could you either add one or open an issue so we can track that as a follow-up?
I was also wondering if this is something that should be put behind a cli-flag so it's an opt in feature? I am not sure what my thoughts are here. @marosset @aravindhp?
This seems like a new feature addition so my vote is to put this behind a cli-flag that defaults to off. With more testing we can consider making this feature GA and defaulting to on if needed. |
logging the failure silently may give a false sense that the cert watcher is watching the files when in fact it isn't. it's better to fail loudly so the user's aware and react accordingly
|
i'm going to work on adding the cli flag and the e2e test |
the cert reload is a new feature that could negatively impact the startup of the webhook. the -cert-reload CLI flag defaults to off to avoid surprises. once mature, we can default to on
|
still working on the e2e test. stay tuned |
|
@jsturtevant added e2e test. please provide feedback. thanks! |
| // apply the new cert & key pair | ||
| renderedTemplate := renderTemplate(t, testConfig, newSecretTemplate) | ||
| success, _, _ := applyManifest(t, renderedTemplate) | ||
| assert.True(t, success) |
There was a problem hiding this comment.
I think this ensures the secret got applied correctly, Do we also need to check that the webhook got the secret and is still running?
There was a problem hiding this comment.
i need some pointers for this. what's a good way to check the webhook got the secret and is still running?
There was a problem hiding this comment.
This is a bit involved since we need to:
- update the webhook pod to use the new flag
- make a request to create a pod to make sure it works (already done)
- get a blessed certificate from the API server (https://github.com/kubernetes-sigs/windows-gmsa/blob/141/admission-webhook/deploy/create-signed-cert.sh)
- update existing secret in place and wait for the pod to get new secrets which can take time (https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-files-from-a-pod) - similar to what you are doing here
- kubectl exec into the running pod to see that the secret changed (using utils like https://github.com/ycheng-kareo/windows-gmsa/blob/watch-reload-cert/admission-webhook/integration_tests/kube.go#L199)
- make a request to create a pod to verify that it still works (pod := waitForPodToComeUp(t, testConfig.Namespace, "app="+testName))
As a bonus (maybe seperate test), we could also verify that requests to the webhook always return during this process. I.e we don't drop requests during rotation.
This is a bit involved so if you want to skip it, we can resolve the remaining comments, merge the PR and I can do a follow up after but if you up for the challenge that works too
There was a problem hiding this comment.
i'm going to skip it in the interest of getting this PR merged so my colleague can move forward with his task. i'm a slow coder and don't want to hold everyone up. really appreciate the detailed pointers getting me this far and your willingness to do the follow-up 🙏
There was a problem hiding this comment.
could you add a TODO with the comments about for this test? Then I think we are good to go.
There was a problem hiding this comment.
just pushed up the comments. formatting is somehow screwed up. this also removed your lgtm
--cert-reload flag defaults to false. we can override by running helm install with --set certificates.certReload.enabled=true
|
/ok-to-test not sure if this will let GH actions run every time a new commit is pushed but worth a shot |
k8s-ci-robot
added
the
ok-to-test
if someone uses the --cert-reload arg with an older version, it will not work
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @aravindhp @marosset |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marosset, ycheng-kareo 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 |
k8s-ci-robot
added
the
approved
The current webhook does not watch for cert renewal changes. When the cert is renewed, the pod must be restarted to reload the new cert.
This commit watches the cert files and reloads the cert when a change is detected.
See: #135