[OCM-24570] feat: add external ID support when assuming customer support roles#937
Conversation
openshift-ci
Bot
added
the
tide/merge-method-squash
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds per-step ExternalID support for AWS STS role assumption: response parsing includes ChangesExternal ID Support in AWS Role Assumption Chain
Sequence DiagramsequenceDiagram
participant API as API Response
participant Handler as getIsolatedCredentials
participant Chain as AssumeRoleSequence
participant STS as AWS STS
API->>Handler: return assumeChainResponse (externalId)
Handler->>Handler: parse externalId
Handler->>Handler: set RoleArnSession.ExternalID for Org & Target roles
Handler->>Chain: pass role-assumption chain (with ExternalID fields)
Chain->>STS: AssumeRole(step N, ExternalID?)
STS-->>Chain: credentials
Chain->>STS: AssumeRole(next step, ExternalID?)
STS-->>Chain: credentials
Chain-->>Handler: final chained credentials
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (1)
pkg/awsutil/sts_test.go (1)
820-860: ⚡ Quick winGood targeted tests; consider one
AssumeRoleSequence-level ExternalID test to close the wiring gap.
TestAssumeRole_externalIDandTestAssumeRole_emptyExternalIDcorrectly exercise the low-levelAssumeRolefunction. However, the propagation pathRoleArnSession.ExternalID → AssumeRole(..., roleArnSession.ExternalID)insideAssumeRoleSequenceis untested. If that argument were accidentally dropped from either call site in the loop, no test would fail. Since ExternalID protects against confused-deputy attacks, this wiring is worth a direct test.The
stsAssumeRoleCapturehelper you've added is already perfectly suited for this; it just needs to be wired into aTestAssumeRoleSequencecase:✅ Suggested additional test case for `TestAssumeRoleSequence`
// Add to the TestAssumeRoleSequence tests slice, or as a standalone test: func TestAssumeRoleSequence_ExternalID(t *testing.T) { capture := &stsAssumeRoleCapture{} _, err := AssumeRoleSequence( capture, // seed client — captures the first AssumeRole call []RoleArnSession{ {RoleArn: "arn:aws:iam::123:role/r", RoleSessionName: "sess", ExternalID: "my-ext-id"}, }, nil, func(optFns ...func(*config.LoadOptions) error) (stscreds.AssumeRoleAPIClient, error) { return capture, nil }, ) if err != nil { t.Fatal(err) } if capture.lastInput == nil || capture.lastInput.ExternalId == nil || *capture.lastInput.ExternalId != "my-ext-id" { t.Fatalf("AssumeRoleSequence did not propagate ExternalID to STS: %+v", capture.lastInput) } }🤖 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/awsutil/sts_test.go` around lines 820 - 860, Add a test that verifies RoleArnSession.ExternalID is passed through AssumeRoleSequence into the STS AssumeRole call: use the existing stsAssumeRoleCapture as the seed client, call AssumeRoleSequence with a single RoleArnSession{RoleArn: "...", RoleSessionName: "sess", ExternalID: "my-ext-id"}, and provide a loader function that returns the capture; after the call assert capture.lastInput is non-nil and that capture.lastInput.ExternalId != nil and equals "my-ext-id". This will exercise the AssumeRoleSequence -> AssumeRole propagation without changing production code.
🤖 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/awsutil/sts_test.go`:
- Around line 820-860: Add a test that verifies RoleArnSession.ExternalID is
passed through AssumeRoleSequence into the STS AssumeRole call: use the existing
stsAssumeRoleCapture as the seed client, call AssumeRoleSequence with a single
RoleArnSession{RoleArn: "...", RoleSessionName: "sess", ExternalID:
"my-ext-id"}, and provide a loader function that returns the capture; after the
call assert capture.lastInput is non-nil and that capture.lastInput.ExternalId
!= nil and equals "my-ext-id". This will exercise the AssumeRoleSequence ->
AssumeRole propagation without changing production code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fc9630be-b4f6-426c-ace2-13ad73b414d6
⛔ Files ignored due to path filters (1)
README.mdis excluded by!**/*.md
📒 Files selected for processing (4)
cmd/ocm-backplane/cloud/common.gocmd/ocm-backplane/cloud/common_test.gopkg/awsutil/sts.gopkg/awsutil/sts_test.go
michaelryanmcneill
force-pushed
the
OCM-24570
branch
from
d4efcd9 to
1331e95
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #937 +/- ##
==========================================
+ Coverage 53.96% 53.99% +0.02%
==========================================
Files 82 82
Lines 6319 6323 +4
==========================================
+ Hits 3410 3414 +4
Misses 2463 2463
Partials 446 446
🚀 New features to boost your workflow:
|
michaelryanmcneill
force-pushed
the
OCM-24570
branch
from
1331e95 to
cfd41ff
Compare
…ort roles Signed-off-by: michaelryanmcneill <michael@michaelryanmcneill.com>
michaelryanmcneill
force-pushed
the
OCM-24570
branch
from
cfd41ff to
c054bda
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@michaelryanmcneill: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelryanmcneill, smarthall 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
3 participants
What type of PR is this?
What this PR does / Why we need it?
For isolated AWS cloud credentials (assume-role sequence from Backplane API), the CLI now:
externalIdfrom the assume-role-sequence JSON.stscredsAssumeRoleOptions.ExternalIDfor Org and Target roles in the chain (not the jump-account SRE user role).Non-isolated
cloud credentialsstill rely on the API only; no user-facing flag changes. README documents both paths.Which Jira/Github issue(s) does this PR fix?
Special notes for your reviewer
externalIdfor clusters that use it; missing field keeps previous behavior (emptyExternalIDonRoleArnSession). API changes are included in OCM-24568.Unit Test Coverage
Test coverage checks
Pre-checks (if applicable)
/label tide/merge-method-squash
Summary by CodeRabbit
New Features
Tests