Repository navigation
Conversation
A failed status write was only logged: unseal still returned nil, the key was forgotten and nothing requeued it, since status-only changes do not trigger the SealedSecret update handler. The SealedSecret then kept its previous condition (e.g. Synced=False) although the Secret had been unsealed, until the next spec change or controller restart. This is what happens on a conflict or on the UID precondition error from bitnami#898. The status was also written into the object owned by the informer cache, so a retry would compare against the condition that never made it to the API server and skip the write. Work on a copy and return the status error so the item is retried with the usual rate limiting. Signed-off-by: Lucas Barros <lucasbarros.kam@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the change
When the status write at the end of
unsealfails, the error is only logged.unsealstill returns nil, so the key is forgotten, and status-only changes never trigger the SealedSecret update handler (it only requeues on spec changes). The SealedSecret keeps its old condition, typicallySynced=Falsewith the previous error, even though the Secret was unsealed fine. It stays like that until the next spec change or a restart. A conflict is enough to hit this, and so is the UID precondition error from #898 when a SealedSecret is replaced.There is a second problem behind it: the condition was written into the object owned by the informer cache. After a failed write, the cache already holds the new condition, so a retry compares against it, decides nothing changed and skips the write anyway.
The fix works on a deep copy of the cached object and, when the unseal itself succeeded but the status write failed, returns that error so the item is retried with the normal rate limiting and
--max-unseal-retries.Benefits
The
Syncedcondition reflects reality after transient API errors, which removes staleSynced=Falsereports and false positives from alerts based on the status. The informer cache is no longer mutated outside the informer.Possible drawbacks
A failed status write now causes one more unseal attempt (idempotent: the Secret is unchanged, and an
Unsealedevent is recorded again).Applicable issues
Additional information
TestStatusUpdateFailureIsRetriedmakes the firstUpdateStatuscall return a conflict and expectsSynced=Trueto be stored eventually. It fails on main (only one status update is attempted). I also checked that it still fails with only the retry or only the deep copy, so both parts are needed.go test ./pkg/... ./cmd/...andgolangci-lint run ./pkg/controller/are clean.