🐛 Stop recreating EKS access entries when username is unset - #6176
🐛 Stop recreating EKS access entries when username is unset#6176moko-poi wants to merge 1 commit into
Conversation
An access entry whose spec omits username was deleted and recreated on every reconcile: the spec value is "" while EKS generates a username server-side, so the two never compared equal. Each delete dropped the associated access policies, intermittently breaking authorization for the principal. UpdateAccessEntry accepts username, so only type actually requires a recreate. Move username to the update path and treat an empty spec value as unmanaged, leaving the EKS generated one in place. Gate the update call on a flag so a username-only change is no longer skipped. Signed-off-by: moko-poi <mokopoi44@gmail.com>
|
Hi @moko-poi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/assign @raykrueger |
|
@damdo: GitHub didn't allow me to assign the following users: raykrueger. Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
I should probably join the org, the bots keep telling me that :) |
|
@moko-poi I'm looking at cherry-picking your work into mine so you get credit. I also have an integration test to fix at the same time. |
|
Cherry-picked in, working on integration tests. |
|
/ok-to-test |
|
The work from this Branch/PR was merged into #6007 I think we can close this one. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
An access entry whose spec omits
usernameis deleted and recreated on every reconcile.updateAccessEntrycompares the spec username against the value reported by AWS. Whenusernameis omitted,accessEntry.Usernameis""(a plainstringwithomitemptyand no defaulting, andcreateAccessEntryskips setting it), while EKS generates a username server-side. The comparison therefore never holds and the entry is torn down and rebuilt on every cycle. EachDeleteAccessEntrydrops the associated access policies, so principals authorized viaaccessPoliciesintermittently lose access — this is how the bug becomes user-visible as sporadicForbiddenerrors.UpdateAccessEntryInputacceptsUsernameand has noTypefield, sotypeis the only genuinely immutable attribute. This PR:typechange, and corrects the comment claiming a recreate is required to change the username.usernameto the update path, treating an empty spec value as unmanaged so the EKS generated username is not seen as drift.UpdateAccessEntryon a flag. Previously only aKubernetesGroupsdifference triggered the call, so a username-only change would have been silently dropped.Beyond stopping the loop, this removes the delete/create window for username changes entirely.
Which issue(s) this PR fixes:
Fixes #6003
Special notes for your reviewer:
This overlaps with #6007, which fixes the same issue from the
Typeside by normalizing an emptyTypetoSTANDARD. That PR leavesaccessEntry.Username != existingUsernameuntouched, so entries that settypeexplicitly and omitusernamekeep looping. Both PRs modify the same condition and will conflict textually; I am happy to rebase whichever lands second, or to fold this into #6007 if the author prefers.Since this narrows the recreate condition to
typeonly, it also subsumes theTypehalf of #6007 for the recreate decision, though not the redundantAssociateAccessPolicycalls that #6007 also addresses.Three existing test cases changed expectations from delete+create to an in-place update. One of them,
username cleared requires recreate, asserted that emptyingusernamein the spec resets the entry. That behaviour is dropped here: EKS offers no way to clear a username and revert to a generated one, so the only mechanism was the recreate that causes this bug, and it is not documented anywhere. Losing an undocumented reset seemed clearly preferable to an endless loop that breaks authorization.On verification: I observed the delete/create loop and the resulting
Forbiddenerrors on a live EKS cluster, which is what led me here. The fix itself is verified by unit tests only — I have not run a patched controller against a real cluster.make lintand thepkg/cloud/services/eksandcontrolplane/eks/...suites pass.Not included:
test/e2e/data/eks/cluster-template-eks-control-plane-only-with-accessentries.yamlalready contains an entry that omitsusernameand carriesaccessPolicies, so the e2e suite exercises the affected configuration but has no assertion on entry stability. Adding one seems worthwhile but felt out of scope, and I cannot run the EKS e2e suite locally.AI Usage:
Claude Code (Claude Opus) was used throughout. I investigated the production symptom and directed the work; the model read the upstream source to identify the faulty comparison, confirmed via
go docthatUpdateAccessEntryInputcarriesUsernamebut notType(which is what shifted the fix from suppressing the comparison to moving username into the update path), and drafted the code, tests, and this description. I reviewed every change, and verified the tests fail without the fix and pass with it.Checklist: