fix: Addresses bug #4979 #5153
fix: Addresses bug #4979 #5153
Conversation
MihailoPlavsic34
requested review from
ThisGuyCodes,
denis256 and
yhakbar
as code owners
|
Someone is attempting to deploy a commit to the Gruntwork Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded@MihailoPlavsic34 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMove the early environment-credentials short-circuit in CreateAwsConfig so IAM role assumption and web-identity credential assembly occur before returning env-based credentials; env creds are only returned immediately when no RoleARN is present and env creds exist. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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
🧹 Nitpick comments (1)
internal/awshelper/config_test.go (1)
153-182: Test coverage gap: bug #4979 scenario not directly tested.Bug #4979 involves the backend's
assume_role.role_arn(passed viaawsCfg.RoleArninAwsSessionConfig) not being used when env credentials exist. This test setsopts.IAMRoleOptions.RoleARN, which tests a different code path ingetMergedIAMRoleOptions.Consider adding a complementary test that passes the RoleARN via
AwsSessionConfig.RoleArnto validate the exact scenario from the bug report.func TestAwsConfigBackendRoleArnTakesPrecedenceOverEnvCredentials(t *testing.T) { t.Parallel() l := logger.CreateLogger() ctx := context.Background() opts := &options.TerragruntOptions{ Env: map[string]string{ "AWS_ACCESS_KEY_ID": "test-access-key", "AWS_SECRET_ACCESS_KEY": "test-secret-key", }, } // Simulate backend assume_role config (bug #4979 scenario) awsCfg := &awshelper.AwsSessionConfig{ RoleArn: "arn:aws:iam::123456789012:role/backend-role", } cfg, err := awshelper.CreateAwsConfig(ctx, l, awsCfg, opts) require.NoError(t, err) assert.NotNil(t, cfg.Credentials) _, err = cfg.Credentials.Retrieve(ctx) assert.Error(t, err) // Role doesn't exist, proving assumption was attempted }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/awshelper/config.go(1 hunks)internal/awshelper/config_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/awshelper/config_test.gointernal/awshelper/config.go
🧬 Code graph analysis (1)
internal/awshelper/config_test.go (2)
test/helpers/logger/logger.go (1)
CreateLogger(9-14)internal/awshelper/config.go (1)
CreateAwsConfig(86-148)
🔇 Additional comments (1)
internal/awshelper/config_test.go (1)
186-210: LGTM!The test correctly validates that environment credentials are used directly when no RoleARN is specified, and verifies the exact credential values match the environment input.
MihailoPlavsic34
force-pushed
the
bug/credential-return-order-aws-helper-4979
branch
from
9602547 to
5507d83
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/awshelper/config.go (1)
145-147: Dead code: conditional check has no effect.This is the same issue flagged in the previous review. Both branches return
cfg, nilwith identical STS credentials already set at line 142. ThecreateCredentialsFromEnv(opts)check does not alter the returned configuration.As suggested in the previous review, remove the no-op conditional:
l.Debugf("Assuming role %s", iamRoleOptions.RoleARN) cfg.Credentials = getSTSCredentialsFromIAMRoleOptions(cfg, iamRoleOptions, getExternalID(awsCfg)) - if createCredentialsFromEnv(opts) != nil { - return cfg, nil - } - return cfg, nil
🧹 Nitpick comments (1)
internal/awshelper/config.go (1)
140-143: Remove redundant condition.The check
if iamRoleOptions.RoleARN != ""at line 140 is redundant because lines 129-131 already return early whenRoleARNis empty. By the time execution reaches line 140,RoleARNis guaranteed to be non-empty.Apply this diff to remove the unnecessary nesting:
- if iamRoleOptions.RoleARN != "" { - l.Debugf("Assuming role %s", iamRoleOptions.RoleARN) - cfg.Credentials = getSTSCredentialsFromIAMRoleOptions(cfg, iamRoleOptions, getExternalID(awsCfg)) - } + l.Debugf("Assuming role %s", iamRoleOptions.RoleARN) + cfg.Credentials = getSTSCredentialsFromIAMRoleOptions(cfg, iamRoleOptions, getExternalID(awsCfg))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/awshelper/config.go(1 hunks)internal/awshelper/config_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/awshelper/config_test.gointernal/awshelper/config.go
🧬 Code graph analysis (2)
internal/awshelper/config_test.go (2)
internal/awshelper/config.go (1)
CreateAwsConfig(86-150)internal/runner/run/creds/providers/provider.go (1)
Credentials(16-19)
internal/awshelper/config.go (2)
pkg/log/log.go (1)
Debugf(72-74)internal/runner/run/creds/providers/provider.go (1)
Credentials(16-19)
🔇 Additional comments (2)
internal/awshelper/config_test.go (2)
153-182: Test appropriately validates RoleARN precedence.This test correctly validates the fix for issue #4979 by ensuring that when both environment credentials and a RoleARN are present, the RoleARN configuration takes precedence. The expectation of an error from
Retrieve(because the test role doesn't exist) demonstrates that role assumption was attempted rather than falling back to environment credentials.
186-210: Test correctly validates environment credential fallback.This test appropriately verifies that environment credentials are used directly when no RoleARN is specified, confirming the expected fallback behavior.
MihailoPlavsic34
force-pushed
the
bug/credential-return-order-aws-helper-4979
branch
from
5507d83 to
e099148
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/awshelper/config.go (1)
130-132: Redundant conditional - both branches return the same result.Lines 130-132 check for env credentials, but both the
ifbranch (line 131) and the fallthrough (line 133) returncfg, nilwith identical state. At this point,cfgalready contains env credentials from line 123 (loaded viaconfig.LoadDefaultConfigwith env credentials added toconfigOptionsat line 99). The check has no side effects and doesn't alter behavior.Apply this diff to simplify:
iamRoleOptions := getMergedIAMRoleOptions(awsCfg, opts) if iamRoleOptions.RoleARN == "" { - if createCredentialsFromEnv(opts) != nil { - return cfg, nil - } return cfg, nil }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/awshelper/config.go(1 hunks)internal/awshelper/config_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/awshelper/config_test.gointernal/awshelper/config.go
🧬 Code graph analysis (1)
internal/awshelper/config_test.go (3)
test/helpers/logger/logger.go (1)
CreateLogger(9-14)options/options.go (2)
TerragruntOptions(99-329)IAMRoleOptions(349-354)internal/awshelper/config.go (1)
CreateAwsConfig(86-147)
🔇 Additional comments (3)
internal/awshelper/config.go (1)
129-147: Fix correctly addresses issue #4979 - RoleARN now takes precedence over env credentials.The repositioning of the env-credentials check inside the
RoleARN == ""block ensures that when a backend role is configured (e.g., ROLE_B in the issue), it takes precedence and is used for state fetching, even when environment credentials are present. Previously, env credentials would short-circuit before role assumption logic, causing the backend role to be ignored.The flow is now:
- RoleARN set: skip lines 130-133, proceed to STS/WebIdentity role assumption (lines 136-146) using env creds as initial credentials
- RoleARN empty: return
cfgwith env creds (already configured at line 123)This restores the expected behavior where backend role configuration is honored.
internal/awshelper/config_test.go (2)
151-182: Test correctly validates RoleARN precedence over env credentials.The test verifies that when both env credentials and a RoleARN are present, the role assumption path is taken (not static env credentials). The error from
Retrieve()confirms an STS AssumeRole call was attempted, which would fail for the non-existent test role. If env credentials were used directly, no error would occur.Note: This test makes a real AWS STS API call (due to the
//go:build awstag), which is consistent with the integration test pattern in this file but may require AWS connectivity in CI.
184-210: Test correctly validates env credentials are used when no RoleARN is specified.The test confirms that when only env credentials are present (no RoleARN), the static credentials provider is used and returns the expected access key and secret key values without making external API calls. This validates the complementary behavior to the precedence test.
MihailoPlavsic34
force-pushed
the
bug/credential-return-order-aws-helper-4979
branch
from
e099148 to
1c54901
Compare
1 participant
Description
Fixes #4979.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Updated aws-creds-helper logic to return pre v0.85.1 behavior.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.