rotator: stop exiting process on secret refresh#491
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the cert rotator to prevent the process from terminating when certificate Secrets are refreshed, eliminating a restart-loop/availability DoS risk while keeping the RestartOnSecretRefresh option accepted for compatibility.
Changes:
- Removed
os.Exit(0)fromrefreshCertIfNeededwhenRestartOnSecretRefreshis enabled. - Replaced forced-exit behavior with a deprecation/no-op informational log.
Show a summary per file
| File | Description |
|---|---|
pkg/rotator/rotator.go |
Stops exiting the process on Secret refresh and logs that RestartOnSecretRefresh is deprecated/ignored. |
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
| if cr.RestartOnSecretRefresh { | ||
| crLog.Info("Secrets have been updated; exiting so pod can be restarted (This behaviour can be changed with the option RestartOnSecretRefresh)") | ||
| os.Exit(0) | ||
| crLog.Info("Secrets have been updated; RestartOnSecretRefresh is deprecated and ignored to avoid restart loops") | ||
| } |
There was a problem hiding this comment.
RestartOnSecretRefresh is now effectively a no-op but the only signal to callers is a runtime log. Since this is an exported field, consider adding a GoDoc deprecation marker (e.g., a Deprecated: comment on the field or type) so API consumers and linters can detect the deprecation without relying on logs.
| @@ -353,8 +352,7 @@ func (cr *CertRotator) refreshCertIfNeeded() (bool, error) { | |||
| } | |||
| crLog.Info("server certs refreshed") | |||
| if cr.RestartOnSecretRefresh { | |||
| crLog.Info("Secrets have been updated; exiting so pod can be restarted (This behaviour can be changed with the option RestartOnSecretRefresh)") | |||
| os.Exit(0) | |||
| crLog.Info("Secrets have been updated; RestartOnSecretRefresh is deprecated and ignored to avoid restart loops") | |||
| } | |||
There was a problem hiding this comment.
The same deprecation log message is duplicated in both refresh paths. Consider extracting the message (const or small helper) to avoid future divergence and make it easier to update/translate consistently.
| rotatedCA = true | ||
| crLog.Info("server certs refreshed") | ||
| if cr.RestartOnSecretRefresh { | ||
| crLog.Info("Secrets have been updated; exiting so pod can be restarted (This behaviour can be changed with the option RestartOnSecretRefresh)") | ||
| os.Exit(0) | ||
| crLog.Info("Secrets have been updated; RestartOnSecretRefresh is deprecated and ignored to avoid restart loops") | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
There are existing rotator tests, but none cover the RestartOnSecretRefresh behavior. Adding a unit/integration test that sets RestartOnSecretRefresh: true and triggers a cert refresh would guard against regressions (e.g., reintroducing process termination) and validate the new no-op semantics.
JaydipGabani
left a comment
JaydipGabani
left a comment
There was a problem hiding this comment.
This is not something we want to do, this os.Exit is intentional and is dependent on the cert-controller setup.
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
sozercan
force-pushed
the
codex/fix-restart-loop-dos-vulnerability
branch
from
ed5ce6c to
a266146
Compare
Motivation
RestartOnSecretRefreshoption caused the process to callos.Exit(0)insiderefreshCertIfNeeded, enabling a Secret writer to repeatedly trigger restarts (availability DoS); the change prevents accidental restart loops while preserving API compatibility.Description
os.Exit(0)calls inrefreshCertIfNeededand replace them with a deprecation/no-op log so certificate refreshes no longer terminate the process while still accepting theRestartOnSecretRefreshfield for compatibility.Testing
go test ./pkg/rotatorandgo test -run TestDoesNotExist -count=1 ./pkg/rotator, but tests could not complete in this environment because kubebuilder test assets (for example/usr/local/kubebuilder/bin/etcd) are missing, so no automated test results are available.Codex Task