ROSAENG-60638 | fix: reduce STS pressure and improve retry for transient InvalidClientTokenId#3342
ROSAENG-60638 | fix: reduce STS pressure and improve retry for transient InvalidClientTokenId#3342jerichokeyne wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR modifies pkg/aws/client.go and pkg/aws/helpers.go to add a custom retry backoff function used when building AWS sessions, introduce caching of STS GetCallerIdentity results on the awsClient struct, and route GetCreator and getClientDetails through this cached call. Session retry configuration for both static-credential and profile-based builders is updated to use the new backoff function with a configured max backoff. Error handling in getClientDetails now returns a dedicated credential error with a help link when InvalidClientTokenId occurs, and debug logging in ValidateAccessKeys switches to formatted string logging. Changes
Sequence Diagram(s)See caller identity retrieval flow in the hidden review stack artifact. Estimated code review effort: High Related issues: None found in provided data. Related PRs: None found in provided data. Suggested labels: aws, retry-logic, refactor Suggested reviewers: None specified. Poem: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jerichokeyne 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 |
openshift-ci
Bot
added
the
approved
jerichokeyne
force-pushed
the
ROSAENG-60638
branch
from
ef1269e to
7ad7d67
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 54 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/aws/helpers.go (1)
162-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
awserr.IsInvalidTokenExceptionhere and wrap the error with%w.This duplicates the existing
InvalidClientTokenIdhandling inpkg/aws/client.go; using the shared helper keeps the check consistent, and%wpreserves the error chain for callers.🤖 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 `@pkg/aws/helpers.go` around lines 162 - 169, The AWS caller identity error handling in awsClient.GetCallerIdentity() should reuse the shared invalid-token check instead of string matching on InvalidClientTokenId. Update the error branch in helpers.go to use awserr.IsInvalidTokenException like the existing logic in client.go, and change the returned formatted error to wrap the original error with %w so callers can inspect the chain. Keep the same credential-help context and locate the change around GetCallerIdentity and the invalid credentials return path.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@pkg/aws/helpers.go`:
- Around line 162-169: The AWS caller identity error handling in
awsClient.GetCallerIdentity() should reuse the shared invalid-token check
instead of string matching on InvalidClientTokenId. Update the error branch in
helpers.go to use awserr.IsInvalidTokenException like the existing logic in
client.go, and change the returned formatted error to wrap the original error
with %w so callers can inspect the chain. Keep the same credential-help context
and locate the change around GetCallerIdentity and the invalid credentials
return path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 32b37547-0418-4b08-ae6a-562b79881cbb
📒 Files selected for processing (2)
pkg/aws/client.gopkg/aws/helpers.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3342 +/- ##
==========================================
+ Coverage 28.38% 28.40% +0.02%
==========================================
Files 334 333 -1
Lines 36730 36710 -20
==========================================
+ Hits 10424 10427 +3
+ Misses 25549 25530 -19
+ Partials 757 753 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jerichokeyne
force-pushed
the
ROSAENG-60638
branch
from
7ad7d67 to
69180d6
Compare
…ent InvalidClientTokenId Eliminate redundant GetCallerIdentity calls during AWS client initialization and wire in the previously unused retry delay constants to extend the retry window for transient InvalidClientTokenId errors. - Cache GetCallerIdentity result in awsClient so Build(), GetCreator(), and GetCallerIdentity() share a single STS call instead of making three. - Consolidate getClientDetails to call GetCallerIdentity once instead of calling ValidateCredentials (which calls it) then calling it again. - Replace the 1-second max backoff with a custom backoff that enforces minRetryDelay (1s) as a floor for normal errors and minThrottleDelay (5s) as a floor for InvalidClientTokenId, with maxThrottleDelay (5s) as the ceiling. This extends the retry window from ~10s to ~60s. - Fix fmt.Printf misuse in ValidateAccessKeys debug logging (should be fmt.Sprintf). Co-authored-by: Cursor <cursoragent@cursor.com>
jerichokeyne
force-pushed
the
ROSAENG-60638
branch
from
375a01e to
32f3f61
Compare
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
|
Out of curiosity: what if the InvalidClientTokenId is a valid error? The user will wait 55-60 seconds to find out? |
| retryer = retry.AddWithMaxBackoffDelay(retryer, time.Second) | ||
| retryer := retry.AddWithMaxAttempts(retry.NewStandard(func(o *retry.StandardOptions) { | ||
| o.MaxBackoff = maxThrottleDelay | ||
| o.Backoff = newBackoffWithMinDelay(minRetryDelay, minThrottleDelay, maxThrottleDelay) |
There was a problem hiding this comment.
With minThrottleDelay == maxThrottleDelay == 5s and 12 attempts, a genuinely invalid token now takes ~60s to fail instead of ~12s. For an interactive CLI, that's a noticeable hang. Could we show a warning on first retry, or cap the token-specific retry window shorter than the full 12 attempts?
| user, err := awsClient.stsClient.GetCallerIdentity(context.Background(), &sts.GetCallerIdentityInput{}) | ||
| user, err := awsClient.GetCallerIdentity() | ||
| if err != nil { | ||
| if awserr.IsInvalidTokenException(err) { |
There was a problem hiding this comment.
ValidateCredentials still uses strings.Contains for InvalidClientTokenId detection while the rest of this PR uses the typed awserr.IsInvalidTokenException helper. Worth aligning for consistency and robustness, or at minimum noting as a follow-up.
| retryer := retry.AddWithMaxAttempts(retry.NewStandard(), numMaxRetries) | ||
| retryer = retry.AddWithMaxBackoffDelay(retryer, time.Second) | ||
| retryer := retry.AddWithMaxAttempts(retry.NewStandard(func(o *retry.StandardOptions) { | ||
| o.MaxBackoff = maxThrottleDelay |
There was a problem hiding this comment.
Nit: o.MaxBackoff is unused by the SDK when o.Backoff is explicitly provided. Not harmful, but the max is already enforced by the inner ExponentialJitterBackoff(maxDelay).
|
@amandahla This is a real trade-off. With 12 attempts at a 5s floor, permanently invalid credentials now take ~60s to fail vs ~12s before. A few options to mitigate: (1) emit a reporter warning on the first |
|
Yeah, that's fair. I'm gonna do a little bit more investigation into this issue I think. I tried re-running the tests that I ran against the master branch and wasn't able to trigger the issue with the tests I ran for this, so I'm not fully confident that this is going to be enough to improve the situation anyways. I think part of the real issue is that in the e2e tests we're basically creating a lot of AWS clients that are short lived (both as part of the test code and from running a bunch of |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
3 participants
PR Summary
Fixes intermittent
InvalidClientTokenIdfailures during AWS client initialization by eliminating redundant STS calls and extending the retry window from ~10 seconds to ~60 seconds.Detailed Description of the Issue
ROSA CLI commands and CI e2e tests intermittently fail with
InvalidClientTokenIdwhen calling STSGetCallerIdentity. The credentials are valid — the error is transient and resolves after ~30 seconds. Two root causes were identified:Redundant STS calls: A single
WithAWS()runtime initialization made 3 separateGetCallerIdentitycalls (ValidateCredentials→GetCallerIdentity, thengetClientDetails→GetCallerIdentityagain, thenGetCreator→GetCallerIdentitya third time), tripling STS pressure and increasing the likelihood of hitting transient failures.Insufficient retry window: The SDK retryer was configured with a max backoff of 1 second across 12 attempts (~10s total), but the transient issue needs ~30 seconds to resolve. The constants
minRetryDelay,minThrottleDelay, andmaxThrottleDelaywere defined but never wired into the retryer configuration.Related Issues and PRs
Type of Change
Previous Behavior
getClientDetails()calledValidateCredentials()(which callsGetCallerIdentity) and then calledGetCallerIdentityagain directly — result from validation was discarded.GetCreator()calledGetCallerIdentitya third time independently.minRetryDelay(1s),minThrottleDelay(5s), andmaxThrottleDelay(5s) were defined but unused.ValidateAccessKeyshadfmt.Printfcalls wherefmt.Sprintfwas intended (debug logging wrote to stdout instead of the logger).Behavior After This Change
GetCallerIdentityresult is cached inawsClient.callerIdentity.Build(),GetCallerIdentity(), andGetCreator()all share a single STS call.getClientDetailscallsGetCallerIdentityonce and handlesInvalidClientTokenIdinline.minRetryDelay(1s) as a floor for normal errors,minThrottleDelay(5s) as a floor forInvalidClientTokenIderrors, andmaxThrottleDelay(5s) as the ceiling. With 12 attempts at 5s floor for token errors, the retry window covers ~60 seconds.ValidateAccessKeysdebug logging now correctly usesfmt.Sprintf.How to Test (Step-by-Step)
Preconditions
Test Steps
make rosamake test— verify all pass.rosa list clusters) and confirm it works.Expected Results
Proof of the Fix
Breaking Changes
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Made with Cursor
Summary by CodeRabbit