fix(cli): diff with changeset fails if deploy role cannot be assumed #29718
Conversation
github-actions
bot
added
bug
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.
aws-cdk-automation
added
the
pr/needs-cli-test-run
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
|
@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
| @@ -430,7 +431,12 @@ export class Deployments { | |||
| } | |||
|
|
|||
| 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 |
scanlonp
added
pr-linter/cli-integ-tested
scanlonp
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ 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). |
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