fix(uat): refresh the EKS STS session mid-gate on AWS - #1872
Merged
Merged
Conversation
The STS session minted by configure-aws-credentials is a fixed ~1h (the role's default MaxSessionDuration) with no refresh token. The install phase's readiness gate has a 90m budget, so on a slow GB200 run the gate outlived the session minted at the install step and every EKS `aws eks get-token` then failed ExpiredToken -> Unauthorized, dropping the whole validate mid-phase. The shared readiness gate (tests/uat/lib/phases.sh) already calls a cloud_refresh_credentials hook every CLOUD_REFRESH_INTERVAL_SECONDS — Azure implements it (federated relogin); AWS left it a no-op on the assumption its session outlasts each phase. Implement it for AWS: re-mint the STS session in place via a fresh GitHub OIDC assertion exchanged with AssumeRoleWithWebIdentity (an unsigned call, so it works even after the current session expired), exporting the new credentials so the subsequent same-shell `aicr validate` / kubectl get-token sign with them. Interval 20m keeps every cached token comfortably inside the 1h session. This keeps the 1h session (no IAM MaxSessionDuration change, so no credential-lifetime increase and no terraform-apply rollout coupling) and mirrors the proven Azure pattern. The secret-handling section forces `set -x` off and ::add-mask::-registers the minted OIDC token and STS keys so they can never reach the public-repo job log. Fixes NVIDIA#1861 Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
njhensley
added
the
theme/ci-dx
Contributor
📝 WalkthroughWalkthroughThe AWS UAT runner adds periodic STS credential refresh during readiness-gate execution. In GitHub Actions, it obtains an OIDC token, exchanges it for AWS role credentials, masks sensitive values, and exports the refreshed credentials. The runner configures the shared cloud refresh interval and dispatches refresh calls through Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/uat/aws/run`:
- Around line 54-58: Update aws_sts_federated_refresh to distinguish non-GitHub
environments from misconfigured GitHub Actions runs using GITHUB_ACTIONS.
Preserve the successful no-op when outside CI, but when GITHUB_ACTIONS indicates
CI and any required OIDC or AWS configuration variable is missing, emit a
diagnostic and fail instead of returning success with no output.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 895fc44e-dd30-4969-8396-a217e04e4f50
📒 Files selected for processing (1)
tests/uat/aws/run
Address CodeRabbit review on NVIDIA#1872: the guard silently returned 0 when the OIDC/role identifiers were absent, so a CI misconfiguration (id-token: write dropped, AWS_ACCOUNT_ID unset) would degrade to the non-refreshing session and re-introduce NVIDIA#1861 with no diagnostic. Emit a ::warning:: in CI (GITHUB_ACTIONS set) while keeping the local-dev no-op quiet. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/uat/aws/run`:
- Around line 54-65: Update aws_sts_federated_refresh so missing OIDC or role
identifiers return a non-zero status when GITHUB_ACTIONS is set, allowing
phases.sh to retry instead of recording success. Preserve the current warning
and keep returning success for quiet local-development no-op behavior outside
GitHub Actions.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c3793de2-b053-4fa9-855b-8175f48170d6
📒 Files selected for processing (1)
tests/uat/aws/run
Follow-up to CodeRabbit on NVIDIA#1872: returning 0 when OIDC/role identifiers are missing in CI let the readiness gate record the missed refresh as a success (advancing last_cred_refresh, no retry). Return non-zero in GitHub Actions (warn + return 1) so the gate keeps warning and retrying; local-dev no-op stays a quiet return 0. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
mchmarny
approved these changes
Jul 23, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/uat/aws/run (2)
98-98: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winKeep the web identity token out of argv.
--web-identity-token "${oidc_token}"still exposes the bearer token to process inspection while the CLI runs. Use--web-identity-token file://...orweb_identity_token_filewith a0600temp file and cleanup trap instead.🤖 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 `@tests/uat/aws/run` at line 98, Update the command invocation around the web identity token option to avoid placing the bearer token directly in argv. Write the token to a temporary file with 0600 permissions, pass its path using the CLI’s file-based web identity token option, and add cleanup via a trap to remove the file on exit.Source: MCP tools
94-98: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winMake the STS call explicitly unsigned.
env -uonly drops the three environment variables here; the CLI can still pick up other configured credentials. Add--no-sign-requestso this refresh always relies on the web-identity token alone.Suggested fix
aws sts assume-role-with-web-identity \ + --no-sign-request \ --role-arn "${role_arn}" \🤖 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 `@tests/uat/aws/run` around lines 94 - 98, Update the aws sts assume-role-with-web-identity invocation in the credential refresh flow to include the --no-sign-request option, ensuring the call is explicitly unsigned and relies only on the web-identity token rather than other configured credentials.Source: MCP tools
🤖 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.
Outside diff comments:
In `@tests/uat/aws/run`:
- Line 98: Update the command invocation around the web identity token option to
avoid placing the bearer token directly in argv. Write the token to a temporary
file with 0600 permissions, pass its path using the CLI’s file-based web
identity token option, and add cleanup via a trap to remove the file on exit.
- Around line 94-98: Update the aws sts assume-role-with-web-identity invocation
in the credential refresh flow to include the --no-sign-request option, ensuring
the call is explicitly unsigned and relies only on the web-identity token rather
than other configured credentials.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3b54dbb7-ee1b-4842-b237-1a6d7981ba18
📒 Files selected for processing (1)
tests/uat/aws/run
njhensley
enabled auto-merge (squash)
July 23, 2026 01:49
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
2 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
Implement the
cloud_refresh_credentialshook for AWS so the EKS install phase's readiness gate re-mints its STS session mid-gate, instead of running past the 1h session and failingExpiredToken → Unauthorized.Motivation / Context
The STS session
configure-aws-credentialsmints is a fixed ~1h (the role's defaultMaxSessionDuration) with no refresh token. TheUAT - installstep (90m budget) runs the helmfile apply and the readiness-gate loop as a single step, so on a slow GB200 run the gate outlived the session minted at the install step and everyaws eks get-tokenthen failedExpiredToken → Unauthorized, dropping the whole validate mid-phase.The shared readiness gate (
tests/uat/lib/phases.sh) already calls acloud_refresh_credentialshook everyCLOUD_REFRESH_INTERVAL_SECONDS— Azure implements it (tests/uat/azure/run, federated relogin); AWS left it a no-op on the (now-disproven) assumption its session outlasts each phase.Fixes: #1861
Related: N/A
Type of Change
Component(s) Affected
tests/uat/aws/run)Implementation Notes
aws_sts_federated_refreshmints a fresh GitHub OIDC assertion and exchanges it viaaws sts assume-role-with-web-identity— an unsigned call (ambient credsenv -u'd), so it succeeds even after the current session has expired — thenexports the new credentials so the subsequent same-shellaicr validate/ kubectlget-tokensign with them. Wired into the shared hook withCLOUD_REFRESH_INTERVAL_SECONDS=1200(20m), which keeps every cached token comfortably inside the 1h session (the token is minted before helmfile apply, so the first mid-gate refresh lands ~40m in, ~20m before expiry).MaxSessionDurationchange, so no credential-lifetime increase and none of the terraform-apply rollout coordination an IAM bump would need. (An IAM-duration approach was prototyped and rejected in review for exactly those reasons.)az_federated_relogin); the gate treats a failed refresh as a warning and retries, so worst case it degrades to the pre-fix behavior with a diagnostic, never a new hard failure.set -xforced off (restored at a single exit) and::add-mask::-registers the OIDC token + STS keys, so they can never reach the public-repo job log even if xtrace is later enabled. (configure-aws-credentialsauto-masks its own creds; these are hand-minted, so we mask them ourselves.)--duration-seconds" finding was refuted (STSMaxSessionDurationcannot be set below 1h, so requesting the 3600 floor is always safe).Testing
shellcheckclean,bash -nOK.aws/curl, real bash): no-op on missing identifiers; mint→parse→export on full env; secret never appears in aset -xtrace; all four::add-mask::emitted; xtrace state correctly restored (on→on, off→off).Risk Assessment
Rollout notes: No IAM or infra change. Recommend a live validation: a GB200 EKS UAT run (from
test/uat) that previously failed at the install/readiness-gate phase — confirm the install completes withoutExpiredToken/Unauthorized, and an h100 run confirms no regression. Follow-up (not this PR): port the::add-mask::+ xtrace guard hardening to the Azure sibling for parity.Checklist
shellcheck+bash -n+ functional smoke tests)tests/uat/azure/run)git commit -S)