🐛 Rebuild AWS session per controller when principal credentials change - #6182
🐛 Rebuild AWS session per controller when principal credentials change#6182UgurTheG wants to merge 4 commits into
Conversation
…ange sessionForClusterWithRegion gated session reuse on a miss in the global providerCache, but sessionCache is keyed per controller. Whichever controller reconciled first repopulated providerCache, so every other controller took the 'nothing changed' path and kept returning a session built from credentials that no longer exist, until the process restarted. Record the principal provider hashes a session was built from and reuse the cached session only when they still match. Signed-off-by: Ugur Guenduez <ugur.guenduez@mercedes-benz.com>
|
|
|
Welcome @UgurTheG! |
|
Hi @UgurTheG. 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 Regular contributors should join the org to skip this step. 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 |
AWSRolePrincipalTypeProvider.Hash gob-encodes the provider and gob only encodes exported fields, so the unexported sourceProvider holding the credentials used to assume the role was not covered. Rotating those credentials while leaving the AWSClusterRoleIdentity untouched produced an identical hash, so providerCache returned the provider built from the previous credentials and every caller kept using them until the manager restarted. Mix the source provider hash into the role provider hash so rotated credentials invalidate the cached provider. Signed-off-by: Ugur Guenduez <ugur.guenduez@mercedes-benz.com>
|
/ok-to-test |
|
/retest |
What type of PR is this?
/kind bug
What this PR does / why we need it:
sessionForClusterWithRegiondecides whether it can reuse a cached AWS session by checking whether every principal provider was already present inproviderCache:providerCacheis global, butsessionCacheis keyed per controller:So when the credentials behind an identity change, the first controller to reconcile misses
providerCache, rebuilds its session correctly, and repopulatesproviderCache. Every other controller then hitsproviderCache, leavesisChangedfalse, and early-returns its ownsessionCacheentry, which was built from the previous credentials. ThesessionCache.Delete()recovery path further down is never reached, because the early return happens first, so the stale session is served until the process restarts.This PR records the provider hashes a session was built from in
sessionCacheEntryand reuses the cached session only when those hashes still match. The decision becomes local to each controller's own cache entry instead of depending on a global cache that another controller may have already populated.We hit this in production: a cluster was deleted and re-provisioned reusing the same name and namespace, with freshly minted static credentials (the previous IAM access key was removed).
AWSClusterreconciled first and came up fully healthy (VPC, subnets, NAT gateways, LB all ready) whileAWSMachinekept failing for ~10 hours with:Same identity, same cluster, different controller: the machine controller was pinned to the pre-rotation session. Restarting
capa-controller-managerwas the only remedy, and the problem returned on the next re-provisioning.Which issue(s) this PR fixes:
None filed. Happy to open one first if maintainers prefer that.
Special notes for your reviewer:
All tests pass on this branch (
go test ./pkg/cloud/scope/...is green).To confirm the new test is a genuine regression test rather than one that would pass either way, I reverted just the
session.gochange and re-ran it. With the fix reverted it fails as expected, because the second controller keeps serving the pre-rotation access key:With the fix in place it passes. The test drives
sessionForClusterWithRegionthrough twocloud.SessionMetadatastubs differing only inControllerName(), rotates the secret behind anAWSClusterStaticIdentity, and asserts both controllers observe the new access key. It uses a static identity so no STS or network calls are involved.This PR closes two distinct paths to the same symptom, because fixing only the first leaves the second reachable:
session.go). Covered above.identity.go).AWSRolePrincipalTypeProvider.Hash()gob-encodes the provider, and gob only encodes exported fields.sourceProvideris unexported, so the hash was determined solely by theAWSClusterRoleIdentityobject. Rotating the source secret while leaving that object untouched produced an identical hash,providerCachereturned the provider built from the previous credentials, and every caller kept using them. The source provider hash is now mixed in.Both are needed in practice. Our provisioning applies the identity objects and the credentials secret with create-or-update semantics, so a full teardown and recreate goes through path 1 (new object UIDs), while an in-place credential refresh goes through path 2 (object unchanged, secret rotated). Each has its own test that fails without the corresponding change.
Also worth noting separately: neither
sessionCachenorproviderCacheis ever evicted, andHash()computessha256.New()thenhash.Sum(gobBytes), which returnsgobBytes || sha256("")rather than a digest of the input, so cache keys are full gob payloads that accumulate per identity revision.AI Usage:
This PR was produced with AI assistance: GitHub Copilot in agent mode (Claude Sonnet 4.5) via the JetBrains IDE integration. The AI performed the production incident investigation, read the CAPA source to identify the cache interaction, wrote the fix and the regression test, and drafted this description. All of it was verified against a real cluster and by running the test against both patched and unpatched code. Reviewed by me before submission.
Checklist:
Release note: