fix(cli): diff with changeset fails if deploy role cannot be assumed#29718
fix(cli): diff with changeset fails if deploy role cannot be assumed#29718mergify[bot] merged 9 commits intomainfrom
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
We've already messed this up a few times so I'd like to see significantly more tests here.
|
@TheRealAmazonKendra, these tests should cover the cases for the stack exists call. I did not find a good way to test which permissions we grab for the stack exists call, but I did not find too much prior art around which role is assumed. Additionally, the new functionality in |
|
@Mergifyio update |
✅ Branch has been successfully updated |
| if (options.tryLookupRole) { | ||
| stackSdk = (await this.prepareSdkWithLookupOrDeployRole(options.stack)).stackSdk; | ||
| } else { | ||
| stackSdk = (await this.prepareSdkFor(options.stack, undefined, Mode.ForReading)).stackSdk; | ||
| } |
There was a problem hiding this comment.
prepareSdkWithLookupOrDeployRole defaults to ForReading. If we can't assume the lookup role, it will fall back to the deploy role.
Don't use prepareSdkFor directly, use the wrappers like prepareSdkWithLookupRole or whichever one you want to use; those methods have names that make it clear which role is being assumed.
There was a problem hiding this comment.
We should still try to fallback to the deploy role, in case somebody doesn't have permissions to the lookup role. Why don't we catch any errors we get when trying to assume the deploy role? If we can't assume it then we stop changeset creation.
Inside of prepareSdkFor, cachedSdkForEnvironment returns an object with didAssumeRole on it. didAssumeRole is not returned to the caller, but you could add it as a property to the returned object. Then you have a way of checking if the fallback was successful.
There was a problem hiding this comment.
discussed off(on?)line, we can investigate this in a follow up next week
| public async stackExists(options: StackExistsOptions): Promise<boolean> { | ||
| const { stackSdk } = await this.prepareSdkFor(options.stack, undefined, Mode.ForReading); | ||
| let stackSdk; | ||
| if (options.tryLookupRole) { |
There was a problem hiding this comment.
This option is confusing...if it's on, then try to assume the lookup role or the deploy role, but if it's off, we assume the lookup role only?
There was a problem hiding this comment.
we can follow this one up in another PR
|
LGTM; let's test out removing the option and only using |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
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. |
Closes #29650
Description of changes
This addresses the issue in two ways:
Description of how you validated changes
Manual testing with a user that could only assume lookup roles.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license