Skip to content

🐛 Shutdown manager if cert-watcher fails to start - #3625

Open
somtochiama wants to merge 15 commits into
kubernetes-sigs:mainfrom
somtochiama:somto/fix-cert-watcher
Open

🐛 Shutdown manager if cert-watcher fails to start#3625
somtochiama wants to merge 15 commits into
kubernetes-sigs:mainfrom
somtochiama:somto/fix-cert-watcher

Conversation

@somtochiama

@somtochiama somtochiama commented Oct 21, 2026

Copy link
Copy Markdown

This pull requests returns an error from the metrics and webhook server Start methods when the certificate watcher cannot register its file watches. The manager shuts down instead of serving a certificate that will never be rotated, which would later break TLS for clients.

Fixes: #3413

@somtochiama
return error in metrics/webhook servers if cert-watchers fails to start
370439e 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@somtochiama
discard log since we return error
18a3404 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@kubernetes-prow

kubernetes-prow Bot commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Welcome @somtochiama!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@kubernetes-prow kubernetes-prow Bot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2026
@kubernetes-prow

kubernetes-prow Bot commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Hi @somtochiama. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@kubernetes-prow kubernetes-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2026
@kubernetes-prow

kubernetes-prow Bot commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: somtochiama
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

sbueringer
sbueringer reviewed Oct 21, 2026
View reviewed changes
Comment thread pkg/metrics/server/server.go Resolved
@kubernetes-prow kubernetes-prow Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 22, 2026
@somtochiama
put error in cert watcher
e153d04 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
sbueringer
sbueringer reviewed Oct 22, 2026
View reviewed changes
Comment thread pkg/metrics/server/server.go Resolved
@somtochiama
add failure tests for metrics and webhook server
14015e1 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@somtochiama
move fifo into shared helper
0e45467 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@somtochiama
more refactoring
345cb91 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@kubernetes-prow kubernetes-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2026
somtochiama
somtochiama commented Oct 23, 2026
View reviewed changes
Comment thread pkg/internal/testing/fifo/fifo.go Resolved
sbueringer
sbueringer reviewed Oct 23, 2026
View reviewed changes
Comment thread pkg/metrics/server/server.go Resolved
@somtochiama
use error channel instead
0738c3b 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@sbueringer sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 3, 2026
@sbueringer

sbueringer commented Nov 3, 2026

Copy link
Copy Markdown
Member

/ok-to-test

@kubernetes-prow kubernetes-prow Bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 3, 2026
@kubernetes-prow kubernetes-prow Bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 3, 2026
sbueringer
sbueringer reviewed Nov 3, 2026
View reviewed changes
Comment thread pkg/metrics/server/server.go Resolved
Comment thread pkg/webhook/server.go Resolved
Comment thread pkg/metrics/server/server.go
@@ -322,6 +332,10 @@ func (s *defaultServer) createListener(ctx context.Context, log logr.Logger) (ne
go func() {
if err := certWatcher.Start(ctx); err != nil {

@sbueringer sbueringer Nov 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today cert watcher only fails if it cannot add watches in l.127 (certwatcher.go).

It does not fail if reading certificates continuously fails in l.142 (certwatcher.go).

I wonder if we should return an error in l.143 if reading certificates failed for a while

@alvaroaleman what do you think?

@alvaroaleman alvaroaleman Nov 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, it doesn't have to mean there is an issue since the already-loaded ones may still be valid? Potentially incorrectly failing seems subpar. Iff we do that, we could maybe check the existing certs NotAfter?

@sbueringer sbueringer Nov 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iff we do that, we could maybe check the existing certs NotAfter?

Good idea. I think let's look into that

@somtochiama somtochiama Nov 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in the last commit, an error is returned on three consecutive failures so temporary read errors don't cause shutdown.

Comment thread pkg/metrics/server/server_test.go Resolved
@somtochiama
better error msg
eea60b7
@somtochiama
use newCertWatcher interface for easy mocking in tests
8f4c5fa 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@somtochiama
return error from certwatcher on consecutive read failures
abb4d04 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
sbueringer
sbueringer reviewed Nov 5, 2026
View reviewed changes
Comment thread pkg/internal/testing/certwatcher/fake.go Resolved
Comment thread pkg/certwatcher/certwatcher.go Resolved
Comment thread pkg/certwatcher/certwatcher.go Resolved
Comment thread pkg/certwatcher/certwatcher.go Resolved
Comment thread pkg/certwatcher/certwatcher.go Resolved
Comment thread pkg/webhook/server_internal_test.go Resolved
@somtochiama
move webhook test to server_test.go
6179721
@somtochiama
store certificate NotAfter in struct
4b5d938 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@somtochiama
fix error
428e57c 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
sbueringer
sbueringer reviewed Nov 9, 2026
View reviewed changes
Comment thread pkg/certwatcher/certwatcher_test.go Resolved
sbueringer
sbueringer reviewed Nov 9, 2026
View reviewed changes
Comment thread pkg/certwatcher/certwatcher.go Resolved
@somtochiama
fix review comments
4441601 
Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alvaroaleman alvaroaleman Awaiting requested review from alvaroaleman
@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96
@sbueringer sbueringer Awaiting requested review from sbueringer

Assignees

No one assigned

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Shutdown manager if webhook & metrics server cert-watchers fail

3 participants

@somtochiama @sbueringer @alvaroaleman