feat(cli): introduce the new cfn-changeset-review-role for the diff command by sakurai-ryo · Pull Request #30568 · aws/aws-cdk

sakurai-ryo · 2024-06-17T12:32:46Z

Issue # (if applicable)

Closes #29767
Closes #29936
Relates to aws/aws-cdk-cli#634

Reason for this change

The cdk diff command needs to assume the deploy-role to create the changeset, but the deploy-role has strong privileges, such as deleting the cfn stack.
Granting developers the ability to be able to assume the deploy-role to use the cdk diff command is a problem that some organizations should avoid.
To avoid this, creating a new role in the Bootstrapping process with only limited privileges is necessary to avoid using the deploy-role when using the cdk diff command.

Description of changes

Introducing a new role

Introduce a new cfn-changeset-review-role and give the cdk diff command the permissions it needs to create changesets.
With this change, the required roles for each operation are as follows

Operation	CDK Bootstrap Roles to Assume
Lookup	lookup-role
Diff	lookup-role cfn-changeset-review-role
Deploy	cfn-exec-role lookup-role deploy-role file-publishing-role image-publishing-role

The cfn-changeset-review-role has the following permissions.

Action	Description
cloudformation:CreateChangeSet	Creates a changeset.
cloudformation:DeleteChangeSet	Deletes a specified changeset.
cloudformation:DescribeChangeSet	Returns the description of the specified change set.
cloudformation:DescribeStacks	Check for the existence of the specified stack.
s3:PutObject*	Upload to S3 when a template exceeding 51,200 bytes is specified.
s3:GetObject*	Download a template uploaded to S3
kms:GenerateDataKey*	Upload files to the staging bucket in SSE-KMS encryption mode.

The cloud-assembly-schema is modified to allow users to use this role.
It will be added to manifest.json in the same format as the existing lookup-role.
Retaining the role's Arn and the requiresBootstrapStackVersion will help if permissions are changed in the future.

Change behavior when executing `cdk diff

Currently, we assume to deploy-role before creating the changeset, but changing it to assume to the new changeset-role.
If for some reason it is not possible to assume to changeset-role, it will fall back to assume the deploy-role.
For example, when changeset-role does not exist.
This allows the cdk diff to work in environments without the latest Bootstrap version.

app-staging-synthesizer module

With the introduction of the new changeset-role, I modified the app-staging-synthesizer module so that it can be used in the same way.

Description of how you validated changes

Added cli-integ test to verify successful upload of cfn template and creation of changeset using the new role, plus several other unit tests.

Others

I created a new PR because a previously created PR was deemed abandoned after an Exemption Request was submitted.
#30329

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

…ff-changeset-role

…aws-cdk into diff-changeset-role

…ff-changeset-role

aws-cdk-automation

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/30568/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

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.

sakurai-ryo · 2024-06-17T12:39:29Z

packages/@aws-cdk-testing/cli-integ/lib/with-cdk-app.ts

  // Now install that `package.json` using NPM7
-  await fixture.shell(['node', require.resolve('npm'), 'install']);
+  const isRepoMode = !!process.env.REPO_ROOT;
+  const npmPath = isRepoMode ? path.join(__dirname, 'package-sources/repo-tools/npm') : require.resolve('npm');


The require.resolve function searches under the node_modules, so we were not able to use fake npm to use local packages.

sakurai-ryo · 2024-06-17T12:40:02Z

packages/@aws-cdk/app-staging-synthesizer-alpha/test/integ.synth-default-resources.ts


 // IMAGE_COPIES env variable is used to test maximum number of ECR repositories allowed.
-const IMAGE_COPIES = Number(process.env.IMAGE_COPIES) ?? 1;
+const IMAGE_COPIES = Number(process.env.IMAGE_COPIES) || 1;


If we use ??, the right side will not be returned if Number(~) returns NaN when the IMAGE_COPIES is undefined.

sakurai-ryo · 2024-06-17T12:40:32Z

packages/aws-cdk/lib/api/util/cloudformation.ts

      bodyParameter,
      parameters: options.parameters,
      resourcesToImport: options.resourcesToImport,
-      role: executionRoleArn,


Because CloudFormation cannot assume the cfn-changeset-review-role.

aws-cdk-automation · 2024-07-09T00:10:01Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

sakurai-ryo · 2024-07-09T02:43:22Z

@Mergifyio update

mergify · 2024-07-09T02:43:30Z

update

✅ Branch has been successfully updated

@sakurai-ryo

…d large templates (#31597) Closes #29936 ### Reason for this change When running `cdk diff` on larger templates, the CDK needs to upload the diff template to S3 to create the ChangeSet. However, the CLI is currently not using the the [file asset publishing role](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk/lib/api/bootstrap/bootstrap-template.yaml#L275) to do so and is instead using the IAM user/role that is configured by the user in the CLI - this means that if the user/role lacks S3 permissions then the `AccessDenied` error is thrown and users cannot see a full diff. ### Description of changes This PR ensures that the `FileAssetPublishingRole` is used by `cdk diff` to upload assets to S3 before creating a ChangeSet by: - Deleting the `makeBodyParameterAndUpload` function which was using the deprecated `publishAssets` function from [deployments.ts](https://github.com/aws/aws-cdk/blob/4b00ffeb86b3ebb9a0190c2842bd36ebb4043f52/packages/aws-cdk/lib/api/deployments.ts#L605) - Building and Publishing the template file assets inside the `uploadBodyParameterAndCreateChangeSet` function within `cloudformation.ts` instead ### Description of how you validated changes Integ test that deploys a simple CDK app with a single IAM role, then runs `cdk diff` on a large template change adding 200 IAM roles. I asserted that the logs did not contain the S3 access denied permissions errors, and also contained a statement for assuming the file publishing role. Reused the CDK app for the integ test from this [PR](#30568) by @sakurai-ryo which tried fixing this issue by adding another Bootstrap role (which we decided against). ### 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*

…ff-changeset-role

…aws-cdk into diff-changeset-role

aws-cdk-automation · 2024-10-05T08:49:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: f7a90a8
Result: FAILED
Build Logs (available for 30 days)

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

@sakurai-ryo

…d large templates (#31597) Closes #29936 ### Reason for this change When running `cdk diff` on larger templates, the CDK needs to upload the diff template to S3 to create the ChangeSet. However, the CLI is currently not using the the [file asset publishing role](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk/lib/api/bootstrap/bootstrap-template.yaml#L275) to do so and is instead using the IAM user/role that is configured by the user in the CLI - this means that if the user/role lacks S3 permissions then the `AccessDenied` error is thrown and users cannot see a full diff. ### Description of changes This PR ensures that the `FileAssetPublishingRole` is used by `cdk diff` to upload assets to S3 before creating a ChangeSet by: - Deleting the `makeBodyParameterAndUpload` function which was using the deprecated `publishAssets` function from [deployments.ts](https://github.com/aws/aws-cdk/blob/f978155c40956440b80ca31695242d81f2f3af3a/packages/aws-cdk/lib/api/deployments.ts#L605) - Building and Publishing the template file assets inside the `uploadBodyParameterAndCreateChangeSet` function within `cloudformation.ts` instead ### Description of how you validated changes Integ test that deploys a simple CDK app with a single IAM role, then runs `cdk diff` on a large template change adding 200 IAM roles. I asserted that the logs did not contain the S3 access denied permissions errors, and also contained a statement for assuming the file publishing role. Reused the CDK app for the integ test from this [PR](aws/aws-cdk#30568) by @sakurai-ryo which tried fixing this issue by adding another Bootstrap role (which we decided against). ### 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*

kaizencc · 2025-02-17T17:27:42Z

Hi @sakurai-ryo, do you know what the state of this PR is? I currently see:

merge conflicts
build failing
maintainers have added labels to this PR but not reviewed

Before either of us do any work cleaning this up, @sakurai-ryo are you still interested in this PR? I can close it if not, but if so, it seems like this PR should get the needs-maintainer-review label (which forces us to review :)) if it passes the pr linter. Note that we've made some changes since you've opened the PR and will make more changes to aws-cdk this week, so additional migration will be needed.

If you're no longer interested, that is fine too, and I apologize for us not getting to this PR in quite some time.
If you are interested, I will take note and hopefully we can resume work on it in the coming weeks.

sakurai-ryo · 2025-02-18T12:52:24Z

Hi @kaizencc
Thank you for checking on the status.

For the safer use of CDK, I am still interested in this PR.
I also plan to resume work, but as you know, changes are needed for both the CLI and the cloud-assembly-schema.
In relation to this, could you please clarify the following two points?

Should I resume work after the aws-cdk code has been separated into a different repository?
The cloud-assembly-schema repository has been archived. Is it planned to be integrated into aws-cdk?
- https://github.com/cdklabs/cloud-assembly-schema

kaizencc · 2025-02-18T14:49:13Z

Hi @sakurai-ryo, thanks for your response. Yes, you should wait until the split is completed (probably this week). The new location will include the CLI code and the cloud assembly schema code. The repo is currently private but we will publicize when the split is done. Please sit tight until then! Thank you for your patience.

hoegertn · 2025-02-27T23:20:01Z

The CLI is now here: https://github.com/aws/aws-cdk-cli

Would love to see progress on a diff role

iliapolo · 2025-04-16T06:10:54Z

@sakurai-ryo Are you able to port this PR over to the new repository?

github-actions · 2025-04-18T08:06:50Z

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

iliapolo · 2025-04-23T09:03:27Z

Keeping this open for a little while longer

iliapolo · 2025-05-28T06:35:37Z

Closing due to no response from @sakurai-ryo. For anyone interested in picking this back up - please port this to the https://github.com/aws/aws-cdk-cli repository.

github-actions · 2025-05-28T06:35:49Z

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.

sakurai-ryo and others added 11 commits May 25, 2024 00:58

feat: introduce the cfn-changeset-role

7774e21

chore: add a new line

d0b32ef

Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into di…

6a6a37c

…ff-changeset-role

Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into di…

bd5626b

…ff-changeset-role

Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into di…

0b789a1

…ff-changeset-role

fix: syntax miss

172c353

Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into di…

12678ca

…ff-changeset-role

Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into di…

d26cc16

…ff-changeset-role

Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into di…

a5cf23f

…ff-changeset-role

Merge branch 'diff-changeset-role' of https://github.com/sakurai-ryo/…

a0b199c

…aws-cdk into diff-changeset-role

Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into di…

2b28865

…ff-changeset-role

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels Jun 17, 2024

aws-cdk-automation requested a review from a team June 17, 2024 12:32

mergify bot mentioned this pull request Jun 17, 2024

feat(cdk): introduce the new cfn-changeset-review-role for the diff command #30329

Closed

1 task

sakurai-ryo had a problem deploying to test-pipeline June 17, 2024 12:33 — with GitHub Actions Failure

aws-cdk-automation requested changes Jun 17, 2024

View reviewed changes

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

sakurai-ryo commented Jun 17, 2024

View reviewed changes

This was referenced Jun 24, 2024

Weekly PR metrics report #30637

Closed

Monthly PR metrics report #30715

Closed

Merge branch 'main' into diff-changeset-role

f66f0e8

mergify bot had a problem deploying to test-pipeline July 9, 2024 02:44 Failure

sakurai-ryo added 2 commits October 5, 2024 17:42

Merge branch 'main' of https://github.com/sakurai-ryo/aws-cdk into di…

85e4e49

…ff-changeset-role

Merge branch 'diff-changeset-role' of https://github.com/sakurai-ryo/…

f7a90a8

…aws-cdk into diff-changeset-role

sakurai-ryo had a problem deploying to test-pipeline October 5, 2024 08:44 — with GitHub Actions Failure

mrgrain changed the title ~~feat(cdk): introduce the new cfn-changeset-review-role for the diff command~~ feat(cli): introduce the new cfn-changeset-review-role for the diff command Jan 13, 2025

mrgrain mentioned this pull request Feb 28, 2025

(cdk): diff ignores --role-arn aws/aws-cdk-cli#634

Open

iliapolo self-assigned this Apr 16, 2025

iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 16, 2025

github-actions bot closed this Apr 23, 2025

iliapolo reopened this Apr 23, 2025

iliapolo requested a review from a team as a code owner April 23, 2025 09:03

iliapolo removed the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Apr 23, 2025

iliapolo had a problem deploying to test-pipeline April 23, 2025 09:04 — with GitHub Actions Failure

github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 23, 2025

iliapolo closed this May 28, 2025

github-actions bot locked as resolved and limited conversation to collaborators May 28, 2025

Conversation

sakurai-ryo commented Jun 17, 2024 • edited by mrgrain Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Introducing a new role

Change behavior when executing `cdk diff

app-staging-synthesizer module

Description of how you validated changes

Others

Checklist

Uh oh!

aws-cdk-automation left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakurai-ryo Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

sakurai-ryo Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

sakurai-ryo Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jul 9, 2024

Uh oh!

sakurai-ryo commented Jul 9, 2024

Uh oh!

mergify bot commented Jul 9, 2024

✅ Branch has been successfully updated

Uh oh!

aws-cdk-automation commented Oct 5, 2024

AWS CodeBuild CI Report

Uh oh!

kaizencc commented Feb 17, 2025

Uh oh!

sakurai-ryo commented Feb 18, 2025

Uh oh!

kaizencc commented Feb 18, 2025

Uh oh!

hoegertn commented Feb 27, 2025

Uh oh!

iliapolo commented Apr 16, 2025

Uh oh!

github-actions bot commented Apr 18, 2025

Uh oh!

iliapolo commented Apr 23, 2025

Uh oh!

iliapolo commented May 28, 2025

Uh oh!

github-actions bot commented May 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sakurai-ryo commented Jun 17, 2024 •

edited by mrgrain

Loading

aws-cdk-automation left a comment •

edited

Loading