fix(service): check for the token Secret itself, not the ServiceAccount - #421
Open
sumanthd032 wants to merge 1 commit into
Open
fix(service): check for the token Secret itself, not the ServiceAccount#421sumanthd032 wants to merge 1 commit into
sumanthd032 wants to merge 1 commit into
Conversation
The worker ServiceAccount's token Secret is only ever created inside the branch that creates the ServiceAccount. That assumes the two are always absent together, which holds when this routine created both and fails as soon as one of them goes missing on its own. Delete a cluster's token Secret by hand and there is no way back. The ServiceAccount is still present, so the branch is skipped, no token is ever minted, and every subsequent reconcile of that cluster fails on the missing Secret. The requeue guard in ClusterService.ReconcileCluster does not catch it either, for a reason that is easy to miss: the ServiceAccount is built with its Secrets reference already populated, so it claims a Secret that does not exist and the check passes. Checking for the Secret's own existence is behaviour-neutral in the ordinary case, where the Secret is present whenever the account is, and restores the cluster in the case above. Verified against a live cluster by deleting the token Secret while leaving the ServiceAccount in place: before, 28 "could not find secret" reconcile errors in 45 seconds and no Secret; after, the Secret is recreated and populated and the errors stop. Re-running with both already present produces no creation events and leaves the Secret's UID unchanged. No unit test accompanies this because the service test package does not compile on master, independently of this change: undefined util.Client in three test files, plus an unknown ClusterName field in worker_slice_gateway_service_test.go. Filed separately. Signed-off-by: Sumanth D <sumanthd032@gmail.com>
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
Fixes cluster reconciliation to check for the token Secret's own existence, not the ServiceAccount's. Deleting the token Secret by hand previously left the cluster permanently broken, since the creation branch is skipped whenever the ServiceAccount is present. Unrelated to the HA work; a standalone fix on the cluster-registration path.
How Has This Been Tested?
Verified against a live cluster: deleted the token Secret while leaving the ServiceAccount in place. Before: reconcile errors every cycle, no Secret recreated. After: the Secret is recreated and the errors stop.
No unit test: the
servicetest package doesn't currently compile onmaster(undefinedutil.Clientin three test files, filed separately).Checklist:
Does this PR introduce a breaking change for other components like worker-operator?
No. Behavior-neutral except when the Secret was already missing, which was already a hard failure.