Conversation
WalkthroughThe changes update the AWS provider initialization to improve how roles are assumed and how AWS regions are discovered. Optional fields for role assumption are now set conditionally, and a fallback mechanism is introduced for region discovery if initial attempts fail due to insufficient permissions. Changes
Sequence Diagram(s)sequenceDiagram
participant ProviderInit as AWS Provider Initialization
participant BaseSession as Base AWS Session
participant AssumeRole as Assume Role (if needed)
participant DescribeRegions as DescribeRegions Call
ProviderInit->>BaseSession: Create base session
BaseSession->>DescribeRegions: Call DescribeRegions
alt Success
DescribeRegions-->>ProviderInit: Return regions
else Failure & AssumeRoleName+AccountIds set
ProviderInit->>AssumeRole: Construct role ARN, set session name/external ID
AssumeRole->>BaseSession: Create temp session with assumed credentials
BaseSession->>DescribeRegions: Retry DescribeRegions
DescribeRegions-->>ProviderInit: Return regions or error
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/providers/aws/aws.go (2)
181-232: Review the fallback logic assumptions and consider refactoring.The fallback mechanism addresses the core issue well, but there are several considerations:
Error assumption: The code assumes that any
DescribeRegionsfailure withAssumeRoleNameconfigured is due to permission issues. Other errors (network, invalid regions, etc.) could trigger unnecessary role assumption attempts.Code duplication: The role assumption logic is duplicated between lines 140-153 and 191-203. Consider extracting this into a helper function.
Single account limitation: Only the first account ID is used for role ARN construction. This may be intentional but could be a limitation for multi-account scenarios.
Consider this refactoring to reduce duplication:
+func (p *Provider) createAssumeRoleInput(roleArn string, options *ProviderOptions) *sts.AssumeRoleInput { + roleInput := &sts.AssumeRoleInput{ + RoleArn: aws.String(roleArn), + } + + if options.AssumeRoleSessionName != "" { + roleInput.RoleSessionName = aws.String(options.AssumeRoleSessionName) + } else { + roleInput.RoleSessionName = aws.String("cloudlist-session") + } + + if options.ExternalId != "" { + roleInput.ExternalId = aws.String(options.ExternalId) + } + + return roleInput +}Then use it in both places:
- roleInput := &sts.AssumeRoleInput{ - RoleArn: aws.String(options.AssumeRoleArn), - } - - if options.AssumeRoleSessionName != "" { - roleInput.RoleSessionName = aws.String(options.AssumeRoleSessionName) - } else { - roleInput.RoleSessionName = aws.String("cloudlist-session") - } - - if options.ExternalId != "" { - roleInput.ExternalId = aws.String(options.ExternalId) - } + roleInput := p.createAssumeRoleInput(options.AssumeRoleArn, options)
186-189: Consider more specific error checking.The current logic assumes any error is due to permission issues, but other errors could trigger unnecessary role assumption attempts. Consider checking for specific AWS error codes related to permissions.
-if err != nil && options.AssumeRoleName != "" && len(options.AccountIds) > 0 { +if err != nil && options.AssumeRoleName != "" && len(options.AccountIds) > 0 { + // Check if error is specifically related to permissions + if awsErr, ok := err.(awserr.Error); ok { + if awsErr.Code() == "UnauthorizedOperation" || awsErr.Code() == "AccessDenied" { + // Permission-related error, try with assumed role + } else { + return nil, errors.Wrap(err, "could not get list of regions") + } + }Note: This would require importing
github.com/aws/aws-sdk-go/aws/awserr.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/providers/aws/aws.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: release-test
- GitHub Check: Lint Test
- GitHub Check: Test Builds (1.22.x, ubuntu-latest)
- GitHub Check: Test Builds (1.22.x, macOS-latest)
- GitHub Check: Test Builds (1.22.x, windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
pkg/providers/aws/aws.go (2)
5-5: LGTM: Required import addition.The
fmtimport is necessary for thefmt.Sprintfcall introduced in the fallback logic.
132-154: LGTM: Improved optional field handling.The refactoring to conditionally set optional fields (
RoleSessionNameandExternalId) is a good practice that avoids passing empty strings to the AWS API. The default session name "cloudlist-session" is appropriate.
3 participants
This PR handles the scenario when a user does not have permission to DescribeRegions unless they assume the role first.
Currently, cloudlist describes the regions first and then assumes the role which causes an issue in this case.
Summary by CodeRabbit
New Features
Bug Fixes