Skip to content

Require leader election for CertRotator and webhook cache #490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sozercan wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Require leader election for CertRotator and webhook cache #490

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions pkg/rotator/rotator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func AddRotator(mgr manager.Manager, cr *CertRotator) error {
secretKey: cr.SecretKey,
wasCAInjected: cr.wasCAInjected,
webhooks: cr.Webhooks,
needLeaderElection: cr.RequireLeaderElection,
needLeaderElection: true,
refreshCertIfNeededDelegate: cr.refreshCertIfNeeded,
fieldOwner: cr.FieldOwner,
certsMounted: cr.certsMounted,
Expand Down Expand Up @@ -208,9 +208,8 @@ func addNamespacedCache(mgr manager.Manager, cr *CertRotator, namespace string)
if err != nil {
return nil, err
}
// Wrapping the cache to make sure it's also started when the manager
// hasn't been leader elected and CertRotator.RequireLeaderElection is false.
if err := mgr.Add(&cacheWrapper{Cache: c, needLeaderElection: cr.RequireLeaderElection}); err != nil {
// 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)
Comment on lines +211 to 213

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}
return c, nil
Expand Down Expand Up @@ -239,8 +238,8 @@ type CertRotator struct {
FieldOwner string
RestartOnSecretRefresh bool
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.

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
// RequireLeaderElection is deprecated and ignored.
// Deprecated: RequireLeaderElection is ignored.

Copilot uses AI. Check for mistakes.
// CertRotator always requires leader election.
RequireLeaderElection bool
// CaCertDuration sets how long a CA cert will be valid for.
CaCertDuration time.Duration
Expand Down Expand Up @@ -273,7 +272,7 @@ type CertRotator struct {
}

func (cr *CertRotator) NeedLeaderElection() bool {
return cr.RequireLeaderElection
return true
}
Comment on lines 274 to 276

Copilot AI Apr 9, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

// Start starts the CertRotator runnable to rotate certs and ensure the certs are ready.
Expand Down
Loading