Suspect code in certwatcher.go

[justinsb]

While investigating a bug around certificate rotation in one of our controllers, I had a look at the logic in certwatcher.go. I found a few things that gave me pause:

1. It looks like an overflow error is logged but otherwise unhandled:

   controller-runtime/pkg/certwatcher/certwatcher.go

   Line 129 in f127c11

   log.Error(err, "certificate watch error")

2. Our initial read of the certificate takes place before starting the watch, which suggests a potential race condition if fsnotify doesn't send an initial (fake) "create" message

3. In general we don't surface errors from fsnotify e.g. if the channel is closed

   controller-runtime/pkg/certwatcher/certwatcher.go

   Line 118 in f127c11

   return

I'm wondering if we should have a polling fallback; a simple os.Stat every 60 seconds or similar. In my particular scenario I'm watching the certificate in a k8s secret directly, so writing to disk and then relying on an fsnotify is just extra complexity.

Would we be open to a PR that accepts a CertificateProvider instead of requiring CertDir? Then I could prove out some of these other options and submit them upstream...

[vincepri]

A CertificateProvider might make sense in this case, I think the original thinking around the certwatcher was that file systems are generic enough that folks can plug into.

Could we maybe start by improving the current implementation to improve our current implementation, and potentially adding a polling mode?

[sbueringer]

I like the idea of being able to watch a secret directly

We are thinking about potentially getting rid of cert-manager for webhook certificates and a Certificate provider could help with that

[vincepri]

I like the idea of being able to watch a secret directly

Wouldn't that work just fine with the current implementation without setting up a watch/controller/reconciler? The secret can be volume mounted in the container itself

[sbueringer]

Don't want to divert to much from the original topic, but I was thinking about something like this:

• The controller itself generates the certificate, stores it in a Secret, sets it in Validating/MutatingWebhookConfigurations, etc. and then uses the certificate also to serve the webhook
• The certificate has to be stored in a Secret so it can be used by other replicas of the controller

I don't know if when I use an optional Secret the secret is mounted into the pod as soon as it exists.

But just early thoughts. Need more research / exploration if something like this would work

[itomsawyer]

@vincepri

A CertificateProvider might make sense in this case

I think this pr #1897 has already make webhook TLSConfig configurable. So that a CertificateProvider is an interface which can provide a func like:

controller-runtime/pkg/certwatcher/certwatcher.go

Line 72 in f127c11

func (cw *CertWatcher) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error) {

CertificateProvider might make sense to be a convenience wrapper of TLSConfig

I think the original thinking around the certwatcher was that file systems are generic enough that folks can plug into.

I'm agreed with that certwatcher should be generic enough for most folks, and it should be worked in most environments.
And that's why we need a polling mode, it's more simple to understand and do not depends on fsnotify.

fsnotify is not works within sandboxed or restricted environment contexts. Please refers to:
https://github.com/fsnotify/fsnotify#why-dont-notifications-work-with-nfs-smb-fuse-proc-or-sys

Also , Other projects is also use filenotify (polling mode) to replace fsnotify
fluent/fluent-operator#126

I'm willing to submit a PR with support polling mode of certwatcher which make it be more generic for kinds of filesystem implements if you agreed with that.

[sbueringer]

Would we be open to a PR that accepts a CertificateProvider instead of requiring CertDir? Then I could prove out some of these other options and submit them upstream...

Related:

• Optimize webhook.Server start procedure when tls.Config.GetCertificate is configured via Server.TLSOpts #2269
• 🌱 Handle TLSOpts.GetCertificate in webhook #2291

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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/test-infra repository.