NO-JIRA: Fall back to AWS SDK default credential chain when no explicit credentials provided#8889
NO-JIRA: Fall back to AWS SDK default credential chain when no explicit credentials provided#8889ironcladlou wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
📝 WalkthroughWalkthroughThis PR updates AWS credential handling to allow default AWS SDK credential-chain fallback when explicit credentials are absent, and adds role-assumption support through Sequence Diagram(s)sequenceDiagram
participant Command
participant AWSCredentialsOptions
participant supportawsutil
participant AWS SDK
Command->>AWSCredentialsOptions: Validate() / ValidateProduct()
AWSCredentialsOptions-->>Command: validation result
Command->>AWSCredentialsOptions: GetSession()
AWSCredentialsOptions->>AWS SDK: build default-chain session
AWSCredentialsOptions->>supportawsutil: AssumeRole(RoleArn)
supportawsutil-->>AWSCredentialsOptions: assumed credentials
AWSCredentialsOptions-->>Command: awsSession
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
openshift-ci
Bot
added
area/cli
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
cmd/infra/aws/create_cli_role.go (1)
174-174: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "(required)" in flag help text.
--aws-credsis no longer enforced as required (Line 178'sMarkFlagRequiredcall was removed), but the flag description still states "(required)", which will mislead users.📝 Proposed fix
- cmd.Flags().StringVar(&opts.AWSCredentialsFile, "aws-creds", opts.AWSCredentialsFile, "Path to an AWS credentials file (required)") + cmd.Flags().StringVar(&opts.AWSCredentialsFile, "aws-creds", opts.AWSCredentialsFile, "Path to an AWS credentials file (optional; falls back to the AWS SDK default credential chain if not set)")🤖 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 `@cmd/infra/aws/create_cli_role.go` at line 174, Update the help text for the AWS credentials flag in the create CLI role setup so it no longer says “(required)”; the requirement was removed, so the description on the StringVar for opts.AWSCredentialsFile should reflect that the flag is optional. Keep the behavior in the command setup consistent with the current validation by adjusting the flag registration in the create CLI role command.
🧹 Nitpick comments (1)
cmd/infra/aws/util/util_test.go (1)
219-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the default credential chain in this test. In
cmd/infra/aws/util/util_test.go:219-223, the no-credentials case only checks thatGetSessionreturns a config; setAWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEYand callcfg.Credentials.Retrieve(ctx)so it proves the fallback returns usable credentials.🤖 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 `@cmd/infra/aws/util/util_test.go` around lines 219 - 223, The no-credentials test case for GetSession only verifies that a config is returned, but it does not exercise the SDK default credential chain. Update the test case in util_test.go around GetSession so that after obtaining cfg you set AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY in the environment and call cfg.Credentials.Retrieve(ctx), using the GetSession and config credential symbols to confirm the fallback produces usable credentials.
🤖 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 `@cmd/infra/aws/create_cli_role.go`:
- Line 174: Update the help text for the AWS credentials flag in the create CLI
role setup so it no longer says “(required)”; the requirement was removed, so
the description on the StringVar for opts.AWSCredentialsFile should reflect that
the flag is optional. Keep the behavior in the command setup consistent with the
current validation by adjusting the flag registration in the create CLI role
command.
---
Nitpick comments:
In `@cmd/infra/aws/util/util_test.go`:
- Around line 219-223: The no-credentials test case for GetSession only verifies
that a config is returned, but it does not exercise the SDK default credential
chain. Update the test case in util_test.go around GetSession so that after
obtaining cfg you set AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY in the
environment and call cfg.Credentials.Retrieve(ctx), using the GetSession and
config credential symbols to confirm the fallback produces usable credentials.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5040903c-c966-429b-b30f-71879400dd01
📒 Files selected for processing (3)
cmd/infra/aws/create_cli_role.gocmd/infra/aws/util/util.gocmd/infra/aws/util/util_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8889 +/- ##
==========================================
+ Coverage 43.28% 43.34% +0.06%
==========================================
Files 771 771
Lines 95503 95532 +29
==========================================
+ Hits 41335 41409 +74
+ Misses 51284 51239 -45
Partials 2884 2884
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ironcladlou
force-pushed
the
aws-auth-refactor
branch
from
116e7b0 to
0e0d7fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/cluster/aws/destroy_test.go (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required “When ... it should ...” test-case format.
As per coding guidelines,
**/*_test.go: Always use"When ... it should ..."format for describing test cases when creating unit tests.Proposed fix
- "when CredentialSecretName is blank and aws-creds is also blank it should fall back to SDK default chain": { + "When CredentialSecretName is blank and aws-creds is also blank, it should fall back to SDK default chain": {🤖 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 `@cmd/cluster/aws/destroy_test.go` at line 17, The test case name in destroy_test.go does not follow the required “When ... it should ...” format. Update the descriptive key used in the test table near the aws destroy tests so it starts with “When” and includes “it should”, matching the existing unit test naming convention for *_test.go files.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.
Inline comments:
In `@cmd/infra/aws/destroy.go`:
- Around line 199-205: The destroy flow leaves vpcOwnerEC2Client nil whenever
useDelegatedCredentials is true, but later VPC cleanup helpers still receive it.
Update the initialization logic in destroy.go so the delegated branch also
creates the VPC-owner EC2 client, mirroring the non-delegated default, and
verify the value passed from the destroy path into the VPC/IGW/EIP/DHCP cleanup
calls is never nil for non-shared-VPC destroys.
---
Nitpick comments:
In `@cmd/cluster/aws/destroy_test.go`:
- Line 17: The test case name in destroy_test.go does not follow the required
“When ... it should ...” format. Update the descriptive key used in the test
table near the aws destroy tests so it starts with “When” and includes “it
should”, matching the existing unit test naming convention for *_test.go files.
🪄 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: Enterprise
Run ID: 92e70707-327c-4408-aaf0-8ea62d65c133
📒 Files selected for processing (6)
cmd/cluster/aws/destroy_test.gocmd/infra/aws/create_cli_role.gocmd/infra/aws/create_operator_roles.gocmd/infra/aws/destroy.gocmd/infra/aws/util/util.gocmd/infra/aws/util/util_test.go
💤 Files with no reviewable changes (1)
- cmd/infra/aws/create_cli_role.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/infra/aws/util/util_test.go
- cmd/infra/aws/util/util.go
ironcladlou
force-pushed
the
aws-auth-refactor
branch
2 times, most recently
from
0a32eab to
ff40a2f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/cluster/aws/destroy.go (1)
163-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment slightly overstates the enforced guarantee.
The comment states
ValidateProductCredentialInfo"requires explicit --sts-creds and --role-arn flags rather than allowing SDK default chain fallback," but that stricter check (opts.ValidateProduct, requiring bothsts-credsandrole-arn) is only exercised whencredentialSecretNameis empty (line 171). When a secret is supplied, the shared helper (lines 177-181) only requiresrole-arn, notsts-creds, for bothValidateCredentialInfoandValidateProductCredentialInfo. Functionally this is likely fine (the secret path never uses SDK default-chain fallback inGetSession), but the doc comment could mislead a future reader into assumingsts-credsis always required for product callers.Consider clarifying the comment to note the distinction only applies to the no-secret path.
🤖 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 `@cmd/cluster/aws/destroy.go` around lines 163 - 167, The doc comment on ValidateProductCredentialInfo overstates its behavior by implying --sts-creds and --role-arn are always required. Update the comment to match the actual logic in ValidateProductCredentialInfo and validateCredentialInfo: the stricter ValidateProduct check only applies when credentialSecretName is empty, while the secret-backed path only requires role-arn. Keep the wording precise so future readers understand the distinction without changing the function behavior.
🤖 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 `@cmd/cluster/aws/destroy.go`:
- Around line 163-167: The doc comment on ValidateProductCredentialInfo
overstates its behavior by implying --sts-creds and --role-arn are always
required. Update the comment to match the actual logic in
ValidateProductCredentialInfo and validateCredentialInfo: the stricter
ValidateProduct check only applies when credentialSecretName is empty, while the
secret-backed path only requires role-arn. Keep the wording precise so future
readers understand the distinction without changing the function behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cdfd9336-54c7-4c2f-9966-612304cf8ef1
📒 Files selected for processing (9)
cmd/cluster/aws/destroy.gocmd/cluster/aws/destroy_test.gocmd/infra/aws/create_cli_role.gocmd/infra/aws/create_operator_roles.gocmd/infra/aws/destroy.gocmd/infra/aws/util/util.gocmd/infra/aws/util/util_test.goproduct-cli/cmd/cluster/aws/create.goproduct-cli/cmd/cluster/aws/destroy.go
💤 Files with no reviewable changes (1)
- cmd/infra/aws/create_cli_role.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/cluster/aws/destroy_test.go
- cmd/infra/aws/create_operator_roles.go
- cmd/infra/aws/destroy.go
- cmd/infra/aws/util/util.go
ironcladlou
force-pushed
the
aws-auth-refactor
branch
2 times, most recently
from
05380e3 to
27e5a81
Compare
|
I now have all three root causes clearly identified. The Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll three failures stem from the same PR change in Root CauseAll three failures share a single root cause: improperly formatted imports in
Recommendations
Evidence
|
ironcladlou
force-pushed
the
aws-auth-refactor
branch
from
27e5a81 to
40d1f98
Compare
…cit credentials provided The CLI previously required either --aws-creds or --sts-creds with --role-arn for all AWS operations. This prevented using standard SDK authentication mechanisms like AWS_PROFILE, environment variables, or SAML/SSO sessions. Make all credential flags optional and fall back to the SDK default chain, with optional --role-arn for role assumption. The product CLI behavior remains unchanged (--sts-creds and --role-arn are still required).
ironcladlou
force-pushed
the
aws-auth-refactor
branch
from
40d1f98 to
5951a54
Compare
ironcladlou
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ironcladlou: This pull request explicitly references no jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@ironcladlou: 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. |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Clean refactor that adds SDK default credential chain fallback for the dev CLI while preserving strict validation for the product CLI. Well-tested across all validation paths. LGTM.
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, ironcladlou 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
|
/verified by test coverage |
openshift-ci-robot
added
the
verified
|
@ironcladlou: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
3 participants
The CLI previously required either --aws-creds or --sts-creds with
--role-arn for all AWS operations. This prevented using standard SDK
authentication mechanisms like AWS_PROFILE, environment variables,
or SAML/SSO sessions. Make all credential flags optional and fall
back to the SDK default chain, with optional --role-arn for role
assumption.
The product CLI behavior remains unchanged (--sts-creds and --role-arn
are still required).
Summary by CodeRabbit
--aws-creds,--sts-creds, and--role-arnto prevent invalid flag combinations and enforce required options.