search

kubernetes-sigs / controller-runtime

Repo for the controller-runtime subproject of kubebuilder (sig-apimachinery)
Apache License 2.0
2.56k stars 1.15k forks source link

Suspect code in certwatcher.go #2182

Closed justinsb closed 8 months ago

justinsb commented 1 year ago

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: https://github.com/kubernetes-sigs/controller-runtime/blob/f127c11669de63e97a0e568b53b09362c8735154/pkg/certwatcher/certwatcher.go#L129

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 https://github.com/kubernetes-sigs/controller-runtime/blob/f127c11669de63e97a0e568b53b09362c8735154/pkg/certwatcher/certwatcher.go#L118

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

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 commented 1 year ago

@vincepri

A CertificateProvider might make sense in this case

I think this pr https://github.com/kubernetes-sigs/controller-runtime/pull/1897 has already make webhook TLSConfig configurable. So that a CertificateProvider is an interface which can provide a func like: https://github.com/kubernetes-sigs/controller-runtime/blob/f127c11669de63e97a0e568b53b09362c8735154/pkg/certwatcher/certwatcher.go#L72

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 https://github.com/fluent/fluent-operator/pull/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 commented 1 year ago

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:

k8s-triage-robot commented 10 months ago

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

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

You can:

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

/lifecycle stale

k8s-triage-robot commented 9 months ago

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:

You can:

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

/lifecycle rotten

k8s-triage-robot commented 8 months ago

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:

You can:

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

/close not-planned

k8s-ci-robot commented 8 months ago

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

In response to [this](https://github.com/kubernetes-sigs/controller-runtime/issues/2182#issuecomment-2006559521): >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][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.