fix: auto-cleanup closed AWS accounts and thread-safe STS retry handling - #1061
Merged
openshift-merge-bot[bot] merged 12 commits intoAug 4, 2026
Merged
Conversation
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe account controller now removes stale pool-owned zombie accounts, handles pending deletion from persisted ChangesAccount lifecycle safeguards
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/account/account_controller.go (1)
674-674: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClean up the finalizer-only STS retry entry after deletion.
handleAWSClientErrormust preserve retry counts through pending deletion, but the finalizer-only non-STS path removes the Account finalizer without aGet()before returning. IfGet()returnsk8serr.IsNotFound, also deleter.stsRetryCount[currentAcctInstance.Name]there so a reused Account name cannot inherit an exhausted count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/account/account_controller.go` at line 674, Update handleAWSClientError’s finalizer-only non-STS deletion path to remove r.stsRetryCount[currentAcctInstance.Name] when the Account Get() returns k8serr.IsNotFound, before returning. Preserve retry counts during pending deletion while ensuring a later Account with the same name starts without the stale exhausted count.
🧹 Nitpick comments (1)
controllers/account/account_controller_test.go (1)
2868-2876: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the finalizer-removal path through
Reconcile.This test calls
handleAWSClientErrordirectly and checks only the retained retry count. It does not verify the behavior added at Lines 326-334: skipping the STS role-assumption call and removingawsv1alpha1.AccountFinalizerfor a pending-deletion Account. Add a focusedReconciletest with an exhausted retry count and assertions for both outcomes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/account/account_controller_test.go` around lines 2868 - 2876, Extend the account controller tests with a focused Reconcile scenario for a pending-deletion Account whose stsRetryCount is already maxSTSClientErrorRetries. Assert that Reconcile skips the STS role-assumption call and removes awsv1alpha1.AccountFinalizer, covering the finalizer-removal behavior rather than only the direct handleAWSClientError retry-count check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/account/account_controller.go`:
- Around line 326-334: Ensure the account controller enforces max concurrent
reconciles of 1 before any access to r.stsRetryCount, preventing operator
configuration from enabling concurrent reconciles; alternatively, protect every
read and write of r.stsRetryCount with a mutex. Update the controller setup
symbol that applies maxReconciles while preserving the existing retry and
finalizer behavior.
---
Outside diff comments:
In `@controllers/account/account_controller.go`:
- Line 674: Update handleAWSClientError’s finalizer-only non-STS deletion path
to remove r.stsRetryCount[currentAcctInstance.Name] when the Account Get()
returns k8serr.IsNotFound, before returning. Preserve retry counts during
pending deletion while ensuring a later Account with the same name starts
without the stale exhausted count.
---
Nitpick comments:
In `@controllers/account/account_controller_test.go`:
- Around line 2868-2876: Extend the account controller tests with a focused
Reconcile scenario for a pending-deletion Account whose stsRetryCount is already
maxSTSClientErrorRetries. Assert that Reconcile skips the STS role-assumption
call and removes awsv1alpha1.AccountFinalizer, covering the finalizer-removal
behavior rather than only the direct handleAWSClientError retry-count check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d1830987-0a10-425c-828b-3f4e2ad0a3ea
📒 Files selected for processing (2)
controllers/account/account_controller.gocontrollers/account/account_controller_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1061 +/- ##
==========================================
+ Coverage 47.31% 47.85% +0.54%
==========================================
Files 46 46
Lines 7177 7217 +40
==========================================
+ Hits 3396 3454 +58
+ Misses 3429 3406 -23
- Partials 352 357 +5
🚀 New features to boost your workflow:
|
BATMAN-JD
marked this pull request as draft
August 3, 2026 14:09
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Three fixes for the STS retry counter bug that caused infinite queue congestion for Failed+PendingDeletion accounts: 1. Remove `delete(r.stsRetryCount, ...)` from handleAWSClientError that reset the counter after exhaustion, causing an infinite 3-retry loop 2. Add 6-hour requeue for exhausted PendingDeletion accounts instead of removing the finalizer, so AAO retains awareness and can clean up when the underlying issue (e.g. missing OrganizationAccountAccessRole) is resolved 3. Pre-fill retry counter from existing AccountClientError condition on pod restart to prevent queue storm from re-doing rapid retries on known-bad accounts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BATMAN-JD
force-pushed
the
BATMAN-JD/fix-sts-retry-loop
branch
from
August 3, 2026 14:37
0c38bed to
9956ce7
Compare
BATMAN-JD
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/account/account_controller_test.go`:
- Around line 2880-2939: Replace the three tests’ duplicated stsRetryCount map
logic with integration-style cases that construct a pending-deletion Account
containing AccountClientError and invoke AccountReconciler.Reconcile. Assert the
result requeues after six hours, stsRetryCount is maxSTSClientErrorRetries - 1,
and the STS AssumeRole mock is not called; retain coverage for the no-condition
and existing-count scenarios only where they are exercised through Reconcile.
In `@controllers/account/account_controller.go`:
- Around line 340-346: Remove the awsAccountID field from the retry log in
controllers/account/account_controller.go lines 340-346 within the STS
retries-exhausted logging block, and remove the same field from the retry log at
lines 689-695. Leave the remaining retry context and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 37dcac77-5c8d-446e-80d2-05fe74e9e8fc
📒 Files selected for processing (2)
controllers/account/account_controller.gocontrollers/account/account_controller_test.go
AlexSmithGH
reviewed
Aug 3, 2026
Contributor
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
When STS retries are exhausted for a pending-deletion account, call DescribeAccount to check if the AWS account is still active. If it's SUSPENDED or CLOSED, remove the finalizer since there are no AWS resources left to clean up. If still active, fall through to the 6-hour long-backoff retry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…logs - Remove pod-restart pre-fill logic per Alex's feedback: DescribeAccount auto-cleanup makes it unnecessary, and in-memory state hacks are not appropriate for controllers - Remove awsAccountID from STS exhaustion log lines (security concern) - Remove 3 associated pre-fill tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…STS check Replace stsRetryCount map lookup with AccountClientError condition check in the PendingDeletion path. The condition is already persisted on the CR by handleAWSClientError, so it survives pod restarts — no ephemeral state needed, no retry storm on restart, no external cache required. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BATMAN-JD
changed the title
Remove the 6h backoff for active-but-unreachable accounts. If DescribeAccount shows the account is active, fall through to the normal STS attempt — the role should work. If it doesn't, handleAWSClientError handles retries and failure as usual. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Widen the zombie GC to also catch NoState accounts (not just Failed) that are pool-owned and have no AWS account ID. These CRs were created during burst incidents but never provisioned — they sit on the limit requeue loop forever as dead weight. NoState accounts are only GC'd after createPendTime (25 min) to avoid racing with initial processing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ention Cover three defense layers that prevent the org from growing unchecked: - Pool controller: one account per reconcile, pending accounts satisfy pool, AWS limit blocking, rapid reconcile capping, failed account replacement - Account controller: zombie GC for pool-owned accounts with no AWS ID, PendingDeletion finalizer removal for SUSPENDED/PendingClosure accounts, condition persistence across pod restarts, account limit requeue - 13 new tests total, all using existing fake client and mock patterns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
controllers/account/account_controller_test.go (2)
2977-3001: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the tautological specs; the new
Reconciletests now cover this behavior. These specs assert on test-local fixtures and on constants compared to themselves, so they execute no controller code and cannot fail for a production reason. The specs at Lines 3004-3105 already drive the same pending-deletion branch throughReconcilewith a mockedDescribeAccount.
controllers/account/account_controller_test.go#L2977-L3001: delete the three specs, or replace them withReconcileinvocations.Expect(organizationstypes.AccountStatusActive).To(Equal(organizationstypes.AccountStatusActive))is always true, andGetConditionhere reads a condition the test itself just assigned.controllers/account/account_controller_test.go#L3107-L3133: delete this spec. It asserts that a map created bymake(map[string]int)on the previous line is empty, and that a condition assigned in the fixture is present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/account/account_controller_test.go` around lines 2977 - 3001, Remove the three tautological specs at controllers/account/account_controller_test.go:2977-3001, since they only inspect test-local fixtures; retain coverage through the existing Reconcile-based tests. Also delete the spec at controllers/account/account_controller_test.go:3107-3133, which only verifies an immediately created empty map and an explicitly assigned condition; make no production-code changes.
1758-1785: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive these offsets from
createPendTimeinstead of hardcoding minutes.
createPendTimeisutils.WaitTime * time.Minute, currently 25 minutes. The-30 * time.Minuteand-5 * time.Minuteoffsets assume that 25-minute boundary, so changingutils.WaitTimewould silently invert one assertion. Use fixed fractions ofcreatePendTime, such as-2 * createPendTimeand-createPendTime / 2.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/account/account_controller_test.go` around lines 1758 - 1785, Update the account age setup in the zombie-account tests around the relevant Reconcile cases to derive timestamps from createPendTime rather than hardcoded minute values. Replace the -30-minute and -5-minute offsets with fixed fractions such as -2*createPendTime and -createPendTime/2, preserving the intended older-than and newer-than boundary assertions when utils.WaitTime changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/account/account_controller_test.go`:
- Around line 1814-1838: Update the zombie BYOC test around
newTestAccountBuilder so the fixture sets Spec.AccountPool and includes the
corresponding AccountPool owner reference, ensuring IsOwnedByAccountPool() is
true; also update the controller condition handling the zombie branch to require
!currentAcctInstance.IsBYOC(). Keep the existing non-deletion assertion so the
test verifies BYOC protection for pool-owned accounts.
- Around line 3160-3191: Make the “account limit requeue” test explicitly
control and restore the process-wide watcher and FedRAMP state used by
AccountReconciler.Reconcile. Initialize the relevant TotalAccountWatcher and
config.isFedramp values in test setup, then restore their original values in
teardown so the test does not depend on or leak global state; preserve the
existing requeue and account-status assertions.
In `@controllers/account/account_controller.go`:
- Around line 331-337: Guard the DescribeAccount call in the AccountClientError
handling branch by checking that currentAcctInstance.Spec.AwsAccountID is
non-empty first. Skip the Organizations request and continue the existing
fallback flow when the AWS account ID is empty, while preserving the current
DescribeAccount behavior for valid IDs.
- Around line 133-151: Update the zombie-account deletion condition in the
reconciliation flow to require feature.validation_delete_account before removing
the finalizer or calling Delete, and require zombieAge > createPendTime for both
Failed and NoState cases. Preserve the existing pool-owned and no-AWS-account
checks, and use the existing feature flag symbol and age threshold.
---
Nitpick comments:
In `@controllers/account/account_controller_test.go`:
- Around line 2977-3001: Remove the three tautological specs at
controllers/account/account_controller_test.go:2977-3001, since they only
inspect test-local fixtures; retain coverage through the existing
Reconcile-based tests. Also delete the spec at
controllers/account/account_controller_test.go:3107-3133, which only verifies an
immediately created empty map and an explicitly assigned condition; make no
production-code changes.
- Around line 1758-1785: Update the account age setup in the zombie-account
tests around the relevant Reconcile cases to derive timestamps from
createPendTime rather than hardcoded minute values. Replace the -30-minute and
-5-minute offsets with fixed fractions such as -2*createPendTime and
-createPendTime/2, preserving the intended older-than and newer-than boundary
assertions when utils.WaitTime changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b2234552-8c9b-4007-931a-2cbe3a9c85bd
📒 Files selected for processing (3)
controllers/account/account_controller.gocontrollers/account/account_controller_test.gocontrollers/accountpool/accountpool_controller_test.go
- Add !IsBYOC() guard to zombie GC condition for defense-in-depth - Update BYOC test to set AccountPool so it tests BYOC protection specifically - Save/restore TotalAccountWatcher global in account limit test - Guard DescribeAccount call on non-empty AwsAccountID Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BATMAN-JD
marked this pull request as ready for review
August 3, 2026 19:45
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the in-memory stsRetryCount map with a CR annotation (aws.managed.openshift.io/aao-sts-retry-count) to eliminate shared mutable state across concurrent reconcile goroutines. The plain Go map could cause a runtime panic when accessed concurrently. Also adds a 1-hour requeue interval after retries are exhausted to prevent clogging the reconcile queue when many accounts are stuck. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
/retest |
Contributor
Author
|
Container test exited with code 137, reason OOMKilled for lint test. Very frustrating. Will try in a little bit |
Contributor
Author
|
/retest |
Contributor
Author
|
Container test exited with code 137, reason OOMKilled for lint test. Still seeing this. Will try in another hour or so |
BATMAN-JD
changed the title
Replace annotation-based approach with a mutex-protected in-memory map per Alex's feedback. The mutex prevents Go runtime panics from concurrent map access across reconcile goroutines while keeping the implementation simple — no new code paths or business logic for failed states. Lock/unlock is encapsulated in helper methods (incrementSTSRetry, getSTSRetry, clearSTSRetry) with defer-based unlock so callers never touch the mutex directly. After retries are exhausted the account is failed with no requeue, consistent with core controller design. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BATMAN-JD
force-pushed
the
BATMAN-JD/fix-sts-retry-loop
branch
from
August 4, 2026 20:05
ad44750 to
436fe25
Compare
Contributor
|
@BATMAN-JD: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexSmithGH, BATMAN-JD The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
7 tasks
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
3 participants
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.
Summary
Fixes the burst creation incident where the AWS org grew from 1232 to 1701 accounts in one day. PendingDeletion accounts with STS failures were looping forever, clogging the pool, and the pool controller kept creating replacements.
What it does
Unblocks PendingDeletion accounts stuck on STS failures:
AccountClientErrorcondition on the CR (persisted, survives pod restarts) to identify accounts that have exhausted STS retriesDescribeAccountto check if the AWS account still existsGarbage-collects zombie Account CRs:
Thread-safe STS retry with bounded retries:
sync.Mutexto prevent Go runtime panics from concurrent reconcile goroutinesAccountClientErrorcondition — no elaborate recovery logicPrevents burst creation from ever recurring:
Boilerplate sync:
9.8-1785339117→9.8-1785777232)Flow
Why is it safe to remove the finalizer for closed accounts?
finalizeAccountcleans up IAM users and roles inside the child AWS account. When the account is SUSPENDED/CLOSED, those resources no longer exist and API calls into the account are impossible. Skipping cleanup of resources that don't exist inside an account we can't reach.Test plan
go test ./...)🤖 Generated with Claude Code