fix(cli): diff with changeset fails if deploy role cannot be assumed #29718

scanlonp · 2024-04-04T03:16:36Z

Closes #29650

Description of changes

This addresses the issue in two ways:

If the describeStacks call errors out, we now catch it and default to classic diff behavior.
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

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

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.

scanlonp · 2024-04-04T03:36:42Z

@Mergifyio update

mergify · 2024-04-04T03:36:46Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2024-04-04T03:40:45Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

TheRealAmazonKendra

We've already messed this up a few times so I'd like to see significantly more tests here.

…handle-error

scanlonp · 2024-04-04T23:02:39Z

@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 stackExists() is exclusive to the call made in our diff logic.

scanlonp · 2024-04-04T23:02:52Z

@Mergifyio update

mergify · 2024-04-04T23:03:08Z

update

✅ Branch has been successfully updated

comcalvi · 2024-04-05T00:17:12Z

packages/aws-cdk/lib/api/deployments.ts

+    if (options.tryLookupRole) {
+      stackSdk = (await this.prepareSdkWithLookupOrDeployRole(options.stack)).stackSdk;
+    } else {
+      stackSdk = (await this.prepareSdkFor(options.stack, undefined, Mode.ForReading)).stackSdk;
+    }


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.

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.

discussed off(on?)line, we can investigate this in a follow up next week

packages/aws-cdk/lib/cdk-toolkit.ts

comcalvi · 2024-04-05T00:31:09Z

packages/aws-cdk/lib/api/deployments.ts

@@ -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) {


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?

we can follow this one up in another PR

comcalvi · 2024-04-05T20:44:40Z

LGTM; let's test out removing the option and only using lookupOrDeployRole in a follow up PR

packages/aws-cdk/lib/cdk-toolkit.ts

Addressed review comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2024-04-05T23:28:45Z

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-cdk-automation · 2024-04-05T23:56:16Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 3559d44
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-04-05T23:56:35Z

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-cdk-automation · 2024-07-25T21:18:09Z

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.

scanlonp added 2 commits April 3, 2024 20:06

catch errors with descibeStacks call and revert to classic diff

d1190f8

add option to make stackExists function use lookup role

037af3f

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Apr 4, 2024

aws-cdk-automation requested a review from a team April 4, 2024 03:16

mergify bot added the contribution/core This is a PR that came from AWS. label Apr 4, 2024

scanlonp had a problem deploying to test-pipeline April 4, 2024 03:17 — with GitHub Actions Failure

aws-cdk-automation previously requested changes Apr 4, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Apr 4, 2024

Merge branch 'main' into diff-handle-error

cf0abab

mergify bot temporarily deployed to test-pipeline April 4, 2024 03:37 Inactive

TheRealAmazonKendra previously requested changes Apr 4, 2024

View reviewed changes

scanlonp added 2 commits April 4, 2024 15:56

unit tests

31c3c58

Merge branch 'diff-handle-error' of github.com:aws/aws-cdk into diff-…

7e508dc

…handle-error

scanlonp had a problem deploying to test-pipeline April 4, 2024 23:00 — with GitHub Actions Failure

Merge branch 'main' into diff-handle-error

04099d6

mergify bot had a problem deploying to test-pipeline April 4, 2024 23:03 Failure

comcalvi requested changes Apr 5, 2024

View reviewed changes

comcalvi approved these changes Apr 5, 2024

View reviewed changes

scanlonp commented Apr 5, 2024

View reviewed changes

packages/aws-cdk/lib/cdk-toolkit.ts Outdated Show resolved Hide resolved

Update packages/aws-cdk/lib/cdk-toolkit.ts

c2da394

scanlonp had a problem deploying to test-pipeline April 5, 2024 20:49 — with GitHub Actions Failure

scanlonp commented Apr 5, 2024

View reviewed changes

packages/aws-cdk/lib/cdk-toolkit.ts Outdated Show resolved Hide resolved

Update packages/aws-cdk/lib/cdk-toolkit.ts

c0ef951

comcalvi approved these changes Apr 5, 2024

View reviewed changes

scanlonp had a problem deploying to test-pipeline April 5, 2024 20:50 — with GitHub Actions Failure

scanlonp added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Apr 5, 2024

scanlonp added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Apr 5, 2024

Merge branch 'main' into diff-handle-error

3559d44

mergify bot had a problem deploying to test-pipeline April 5, 2024 23:29 Failure

mergify bot merged commit 21dba21 into main Apr 5, 2024
11 of 12 checks passed

mergify bot deleted the diff-handle-error branch April 5, 2024 23:56

github-actions bot mentioned this pull request Apr 8, 2024

Weekly PR metrics report #29758

Closed

github-actions bot mentioned this pull request May 1, 2024

Monthly PR metrics report #30026

Closed

This was referenced May 23, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#98

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#99

Open

NOUIY mentioned this pull request May 30, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#101

Open

NOUIY mentioned this pull request Jun 7, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#102

Open

niravparikh05 mentioned this pull request Jun 13, 2024

[Snyk] Upgrade aws-cdk from 2.65.0 to 2.142.1 paralus/eks-blueprints-addon#3

Open

pahud mentioned this pull request Jul 8, 2024

(core): file asset publishing role is not used while performing cdk diff with larger templates #29936

Closed

aws locked as resolved and limited conversation to collaborators Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #29718

scanlonp commented Apr 4, 2024

aws-cdk-automation left a comment

scanlonp commented Apr 4, 2024

mergify bot commented Apr 4, 2024

aws-cdk-automation commented Apr 4, 2024

TheRealAmazonKendra left a comment

scanlonp commented Apr 4, 2024

scanlonp commented Apr 4, 2024

mergify bot commented Apr 4, 2024

comcalvi Apr 5, 2024

comcalvi Apr 5, 2024

comcalvi Apr 5, 2024

comcalvi Apr 5, 2024

comcalvi Apr 5, 2024

comcalvi commented Apr 5, 2024 •

edited

Loading

mergify bot commented Apr 5, 2024

aws-cdk-automation commented Apr 5, 2024

mergify bot commented Apr 5, 2024

aws-cdk-automation commented Jul 25, 2024

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 #29718

Conversation

scanlonp commented Apr 4, 2024

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

scanlonp commented Apr 4, 2024

mergify bot commented Apr 4, 2024

✅ Branch has been successfully updated

aws-cdk-automation commented Apr 4, 2024

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

scanlonp commented Apr 4, 2024

scanlonp commented Apr 4, 2024

mergify bot commented Apr 4, 2024

✅ Branch has been successfully updated

comcalvi Apr 5, 2024

Choose a reason for hiding this comment

comcalvi Apr 5, 2024

Choose a reason for hiding this comment

comcalvi Apr 5, 2024

Choose a reason for hiding this comment

comcalvi Apr 5, 2024

Choose a reason for hiding this comment

comcalvi Apr 5, 2024

Choose a reason for hiding this comment

comcalvi commented Apr 5, 2024 • edited Loading

mergify bot commented Apr 5, 2024

aws-cdk-automation commented Apr 5, 2024

AWS CodeBuild CI Report

mergify bot commented Apr 5, 2024

aws-cdk-automation commented Jul 25, 2024

comcalvi commented Apr 5, 2024 •

edited

Loading