🐛 Refactor certificate watcher to use polling, instead of fsnotify

[m-messiah]

Summary

In the current controller-runtime/certwatcher behaviour if any of the files (cert or key) are deleted before the replacement, the code removes the watcher. It gets stuck with watching nothing, so when the replacement arrives - no goroutine will receive that event and all the future certificate updates (including this) are missed until the code is restarted.

Details

The added test in the PR shows the example scenario of updating the certificate in two steps:

1. rm /certs/mtls.pem /certs/mtls.key
2. tar xvf -C /certs new_mtls.tar.gz (or similar).

Expected behaviour:
Return the previous certificate until the new files appear, reestablish the watch and reread the certs when ready.

Current behaviour:
The watch is removed and new files do not trigger anything.

Possible options to consider

1. The ensureAllFilesAreWatched could be called in select/default branch, but it increases the load drastically. 1 second seems reasonable enough.
2. The ticker could be increased to 10 seconds to reduce load (or be customizable), but don't see a big issue here as it checks the filesystem and its internal locks only.
3. Instead of reestablishing watches we could stop the watcher and trigger controller code to be reinitialised from the beginning - it is a nice behaviour we use for Kube caches and watchers, but does not seem good for certificates to reinitialise the whole context after such small change.

[alvaroaleman]

Permanently polling once per second seems pretty aggressive. Can we not watch the dir intead if the file gets removed and re-add the filewatch once it exists again?

[vincepri]

sidenote: wondering at this point if we should generally avoid relying of fs events, and rather say poll every N seconds and reload the certificates if the hash changed or something?

[vincepri]

We could also make the polling configurable, and default it to something reasonable where both old (loaded) and new certificates are supposed to be valid

[m-messiah]

Can we not watch the dir intead if the file gets removed and re-add the filewatch once it exists again?

It does not watch the dir, it just tries to reinstall the notify watch that fails if the file is not present.
So technically, it is the same as "waiting until the file exists again" as the first step of addWatch is os.Lstat(name).

The other option is to watch the directory events instead of file events and parse all events for the file names but it:

1. Increases the number of events to parse and handle
2. Does not solve the case when the directory would be removed and recreated again (or have mount issues, whatever).

[m-messiah]

sidenote: wondering at this point if we should generally avoid relying of fs events, and rather say poll every N seconds and reload the certificates if the hash changed or something?

It makes sense, and in some of our internal projects, we rely on this behaviour instead. The downside is that you need to find a balance between polling interval, certificate expiration time, and number of file reads in this case; so ideally this param should also be configurable across all usages of certwatcher (webhook server and metrics server at least)

[alvaroaleman]

I guess the main point is with the aggressive 1 second polling. I like vinces idea of just re-reading the file every 10 seconds and caching in between, that is also what client-go does for the SA token

[m-messiah]

Thank you for your suggestions.
@alvaroaleman @vincepri I updated the PR with full removal of fsnotify from certwatcher and having a simple read/cache/repeat ticker loop. Please check and leave your opinions.

[k8s-ci-robot]

@m-messiah: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff	cbb3859	link	false	/test pull-controller-runtime-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[alvaroaleman]

thanks!
/hold

@vincepri does this look good to you?

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 3ab463016c247d6ff9787de7b6df1faffe0d50d0

[m-messiah]

Do you know what I should do with apidiff check?

The method Watch does not look to be important outside of the Start, but it was Exported. Shall I keep it there and do nothing?

sigs.k8s.io/controller-runtime/pkg/certwatcher
  Incompatible changes:
  - (*CertWatcher).Watch: removed
  Compatible changes:
  - (*CertWatcher).WithWatchInterval: added

[alvaroaleman]

Do you know what I should do with apidiff check?

It is just informative and tells us that there was a breaking change to exported interfaces. It won't block merge

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, m-messiah, vincepri

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:

• OWNERS [alvaroaleman,vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vincepri]

/retitle 🐛 Refactor certificate watcher to use polling, instead of fsnotify

[vincepri]

/hold cancel

[vincepri]

/cherry-pick release-0.19

[k8s-infra-cherrypick-robot]

@vincepri: new pull request created: #3023

In response to this:

/cherry-pick release-0.19

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.

[skhalash]

@m-messiah After this change our tests showed the following problem - our operator starts, a certificate is generated on startup and saved in the cert dir, and only after 10 seconds it is actually getting picked up. Which was not the case with the old fsnotify approach. We can fix it by decreasing the timeout, but is still feels wrong to me.

[m-messiah]

@m-messiah After this change our tests showed the following problem - our operator starts, a certificate is generated on startup and saved in the cert dir, and only after 10 seconds it is actually getting picked up. Which was not the case with the old fsnotify approach. We can fix it by decreasing the timeout, but is still feels wrong to me.

The expectation of certwatcher is that files are already present, or be present in the next loop after interval.
I understand the problem, but it looks like your operator is trying to start the watcher before the certs are generated and to solve this you should be able to fix the order of prerequisites and the actual start.

[skhalash]

@m-messiah Hmm, we are not doing anything fancy there and as far as I understand the initial cert read and the timer creation happen here

controller-runtime/pkg/webhook/server.go

Line 207 in aea2e32

certWatcher, err := certwatcher.New(certPath, keyPath)

and at that point our cert is already there.

[skhalash]

I found the root cause in our codebase, but I’m still questioning this change. Our operator manages its own webhook certificates (rotation, etc.) instead of using an external service like cert-manager. The issue is that we assume updating the cert file makes it immediately usable by the HTTPS server. This worked before but doesn’t anymore after this change.