Require leader election for CertRotator and webhook cache#490
Require leader election for CertRotator and webhook cache#490
Conversation
There was a problem hiding this comment.
Pull request overview
Restores a safer default for certificate rotation by ensuring the CertRotator, its webhook reconciler/controller, and the namespaced cache only run when the manager is leader-elected (preventing concurrent rotations across replicas when leader election is enabled).
Changes:
- Force
ReconcileWH.needLeaderElectiontotruewhen wiring the rotator (AddRotator). - Register the namespaced cache wrapper with
needLeaderElection: trueso its startup is gated by leader election. - Deprecate/ignore
CertRotator.RequireLeaderElectionand makeCertRotator.NeedLeaderElection()always returntrue.
Show a summary per file
| File | Description |
|---|---|
pkg/rotator/rotator.go |
Forces leader-election participation for the rotator, webhook controller, and namespaced cache to avoid multi-replica certificate rotation/injection churn. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 3
| // Wrapping the cache so the manager can gate startup on leader election. | ||
| if err := mgr.Add(&cacheWrapper{Cache: c, needLeaderElection: true}); err != nil { | ||
| return nil, fmt.Errorf("registering namespaced cache: %w", err) |
There was a problem hiding this comment.
After hardcoding needLeaderElection: true, the cr *CertRotator parameter is no longer used in addNamespacedCache, which will cause a Go compile error (cr declared but not used). Consider removing the cr parameter from addNamespacedCache (and updating its call sites) or renaming it to _ if you need to keep the signature temporarily.
| ExtKeyUsages *[]x509.ExtKeyUsage | ||
| // RequireLeaderElection should be set to true if the CertRotator needs to | ||
| // be run in the leader election mode. | ||
| // RequireLeaderElection is deprecated and ignored. |
There was a problem hiding this comment.
The field is described as deprecated, but the doc comment won’t be recognized by Go tooling unless it uses the canonical Deprecated: form. Consider changing the comment to start with // Deprecated: so go doc and linters surface the deprecation correctly.
| // RequireLeaderElection is deprecated and ignored. | |
| // Deprecated: RequireLeaderElection is ignored. |
| func (cr *CertRotator) NeedLeaderElection() bool { | ||
| return cr.RequireLeaderElection | ||
| return true | ||
| } |
There was a problem hiding this comment.
Leader-election behavior is being changed here (the rotator now always returns true). Given pkg/rotator already has a substantial test suite, it would be good to add a unit test that asserts NeedLeaderElection() is true regardless of RequireLeaderElection’s value, to prevent regressions back to the unsafe default.
Motivation
CertRotatorand its webhook reconciler defaulted to opt-in leader election via the zero-valueRequireLeaderElection=false, which allowed multiple replicas to concurrently rotate certificates and inject CA bundles causing webhook TLS churn and availability risk.Description
ReconcileWH.needLeaderElectiontotruewhen constructing the reconciler inAddRotatorso the webhook reconciler always requires leader election.cacheWrapperwithneedLeaderElection: trueinaddNamespacedCacheso the cache startup is gated by leader election.CertRotator.RequireLeaderElectionfield as deprecated/ignored and makeCertRotator.NeedLeaderElection()returntrueunconditionally to force leader-election participation.RequireLeaderElectionfield for API/backwards-compatibility but prevents the unsafe zero-value behavior.Testing
go test ./pkg/rotator/...with a 25s timeout in the environment which timed out and returnedEXIT:124, so tests could not be completed successfully in this environment.Codex Task