Skip to content

fix: create assume role policy inline#439

Closed
tabrezm wants to merge 3 commits into
cdklabs:mainfrom
tabrezm:main
Closed

fix: create assume role policy inline#439
tabrezm wants to merge 3 commits into
cdklabs:mainfrom
tabrezm:main

Conversation

@tabrezm

@tabrezm tabrezm commented May 5, 2024

Copy link
Copy Markdown

Fixes #438

This PR moves the assume role policy inline to avoid a race condition of the stack set resource deployment before the policy is attached. As a consequence, an existing admin role won't be modified with a potentially duplicate policy.

@tabrezm tabrezm changed the title Create assume role policy inline fix: create assume role policy inline May 5, 2024

@comcalvi comcalvi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just please add or modify an integ test for this one.

@tabrezm

tabrezm commented Jun 26, 2024

Copy link
Copy Markdown
Author

Looks good, just please add or modify an integ test for this one.

I think the existing integration tests are sufficient but the snapshot needs to be updated because this PR removes the duplicate policy that gets added to the admin role. Unfortunately I'm a bit stumped on how to make this change on my end. I tried to run yarn integ:update but I'm not sure what values to set for INTEG_DEPLOYMENT_ACCOUNT or INTEG_TARGET_ACCOUNT. If I use a personal account, won't the checked-in snapshot point to it?

For context, this is the snapshot change that's causing the integration test to fail:

Verifying integration test snapshots...

  CHANGED    integ.stack-set 3.496s
      IAM Statement Changes
┌───┬───────────────────────────────────────────────────────────────────────┬────────┬────────────────┬──────────────────────────┬───────────┐
│   │ Resource                                                              │ Effect │ Action         │ Principal                │ Condition │
├───┼───────────────────────────────────────────────────────────────────────┼────────┼────────────────┼──────────────────────────┼───────────┤
│ - │ arn:aws:iam::*:role/AWSCloudFormationStackSetExecutionRole-integ-test │ Allow  │ sts:AssumeRole │ AWS:${AdminRole38563C57} │           │
└───┴───────────────────────────────────────────────────────────────────────┴────────┴────────────────┴──────────────────────────┴───────────┘
(NOTE: There may be security-related changes not in this list. See https://github.com/aws/aws-cdk/issues/1299)

Resources
[-] AWS::IAM::Policy AdminRoleDefaultPolicy1C2AB961 destroy



Snapshot Results: 

Tests:    1 failed, 1 total
Failed: /Users/tabrezm/Code/cdk-stacksets/test/integ.stack-set.ts
!!! This test contains destructive changes !!!
    Stack: integ-stackset-test - Resource: AdminRoleDefaultPolicy1C2AB961 - Impact: WILL_DESTROY
!!! If these destructive changes are necessary, please indicate this on the PR !!!

Please advise, thanks!

@robertjan-b

Copy link
Copy Markdown

I'm running into this issue as well. I want to have a pre-provisioned Admin role (due to the race condition) for the StackSet, but it tries to attach a duplicate inline permission to the pre-provisioned Admin role. This occurs when deploying multiple self managed stacksets with this same Admin role. Please fix the issue.

@go-to-k

go-to-k commented Nov 18, 2024

Copy link
Copy Markdown
Contributor

+1

I too hope this issue is resolved.

@kaizencc

Copy link
Copy Markdown
Contributor

#678 is the right thing to do here so im going to close this in favor of that.

@kaizencc kaizencc closed this Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition when using self managed deployment type

5 participants