Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cdk diff: in 2.134.0 diff fails if deploy cannot be assumed #29650

Closed
Smeb opened this issue Mar 29, 2024 · 8 comments · Fixed by #29718 · 4 remaining pull requests
Closed

cdk diff: in 2.134.0 diff fails if deploy cannot be assumed #29650

Smeb opened this issue Mar 29, 2024 · 8 comments · Fixed by #29718 · 4 remaining pull requests
Labels
bug This issue is a bug. documentation This is a problem with documentation. effort/medium Medium work item – several days of effort p1 package/tools Related to AWS CDK Tools or CLI

Comments

@Smeb
Copy link

Smeb commented Mar 29, 2024

Describe the bug

In our CI we have a readonly role with access to the lookup role, but not the deploy role for non protected branches.

In version 2.133.0 running diff works - in 2.134.0 we get a crash.

For now we fixed it by pinning the previous version.

Expected Behavior

Diffing with a role which can assume the lookup role works

Current Behavior

Diffing with a role which cannot assume deploy fails.

current credentials could not be used to assume '<deploy role>' but are for the right account. Proceeding anyway.
User: <ci_role> is not authorized to perform: cloudformation:DescribeStacks on <stack> because no identity-based policy allows the cloudformation:DescribeStacks action

Not clear what's happening here - I guess after failing to assume the deploy role the current role is the ci role which attempted to assume deploy, and then after DescribeStacks is called.

Reproduction Steps

I don't think I can easily provide this, since you would need to set up the same roles, stacks, etc. This does work with a higher privilege role, which indicates it's to do with the role assumption.

  • Bootstrapped CDK
  • CDK setup (we use Go, but it seems very clearly to be a permission issue).
  • Run cdk diff --all --ci

Possible Solution

Ensure that the lookup role is still used after trying to assume the deploy role and failing.

Additional Information/Context

No response

CDK CLI Version

v2.134.0

Framework Version

No response

Node.js Version

20

OS

Darwin

Language

Go

Language Version

1.22

Other information

No response

@Smeb Smeb added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 29, 2024
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Mar 29, 2024
@Smeb
Copy link
Author

Smeb commented Mar 29, 2024

Also: running with the top level role seems like an oversight - in theory that role could have more permissions than intended. I also tried running with --no-changeset, which didn't help.

@Smeb
Copy link
Author

Smeb commented Mar 29, 2024

On the above, when I run with --no-changeset then my params for the diff have the following:

changeset: false
changeSet: true
'chage-set': true

-> because it should be --change-set=false. There are a couple of places in the codebase where --no-changeset is mentioned which I can update PR

@tim-finnigan tim-finnigan self-assigned this Apr 1, 2024
@tim-finnigan tim-finnigan added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 1, 2024
@scanlonp
Copy link
Contributor

scanlonp commented Apr 1, 2024

Hey @Smeb, once you caught the typo in our docs, did running diff with the correct spelling of the flag solve your issue? I believe in v2.134 that we put logic in exactly for this case, so I want to make sure you are not still running into issues.

@tim-finnigan tim-finnigan added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Apr 1, 2024
@Smeb
Copy link
Author

Smeb commented Apr 2, 2024

Yes, using the correct flag did work around the issue - I would still expect to not fall back to the parent role though (that, or the change in behaviour would be worth documentation)

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 2, 2024
@scanlonp
Copy link
Contributor

scanlonp commented Apr 2, 2024

@Smeb, good to hear. Yes, this should be documented.

Thanks for your PR! We are also adding --changeset as an alias.

@tim-finnigan tim-finnigan added p2 effort/medium Medium work item – several days of effort documentation This is a problem with documentation. labels Apr 2, 2024
@tim-finnigan tim-finnigan removed their assignment Apr 2, 2024
@scanlonp
Copy link
Contributor

scanlonp commented Apr 4, 2024

@Smeb, after some digging, there is a deeper problem here.

Before, if the deploy role could not be assumed, we caught the error in the changeset creation and defaulted back to the classic diff behavior. Now, if the describeStacks call fails, we hard fail.

I have made a PR that both handles the error if describeStacks fails and attempts to use the lookup role for the call in the first place.

Thanks again for reporting this.

@scanlonp scanlonp added p1 and removed p2 labels Apr 4, 2024
@mergify mergify bot closed this as completed in #29718 Apr 5, 2024
mergify bot pushed a commit that referenced this issue Apr 5, 2024
…29718)

Closes #29650

### Description of changes

This addresses the issue in two ways:

1. If the describeStacks call errors out, we now catch it and default to classic diff behavior.
2. The describeStacks call now tries to use the lookup role rather than the deploy role.

### Description of how you validated changes

Manual testing with a user that could only assume lookup roles.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

github-actions bot commented Apr 5, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@aws-cdk-automation
Copy link
Collaborator

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.