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

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

Issue # (if applicable)

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 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.

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

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

@@ -620,7 +620,9 @@ export async function installNpmPackages(fixture: TestFixture, packages: Record<
  }, undefined, 2), { encoding: 'utf-8' });

  // 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

@@ -355,7 +354,6 @@ async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOp
      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

aws-cdk-automation · 2024-07-16T00:10:40Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@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:45:49Z

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.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

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

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jul 16, 2024

aws-cdk-automation closed this Jul 16, 2024

scanlonp added pr-linter/do-not-close The PR linter will not close this PR while this label is present and removed closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. labels Jul 25, 2024

scanlonp reopened this Jul 25, 2024

sumupitchayan mentioned this pull request Sep 30, 2024

fix(core): file asset publishing role not used in cdk diff to upload large templates #31597

Merged

1 task

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sakurai-ryo commented Jun 17, 2024 •

edited

Loading

aws-cdk-automation left a comment

sakurai-ryo Jun 17, 2024

sakurai-ryo Jun 17, 2024

sakurai-ryo Jun 17, 2024

aws-cdk-automation commented Jul 9, 2024

sakurai-ryo commented Jul 9, 2024

mergify bot commented Jul 9, 2024

aws-cdk-automation commented Jul 16, 2024

aws-cdk-automation commented Oct 5, 2024

aws-cdk-automation commented Oct 5, 2024

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

Are you sure you want to change the base?

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

Conversation

sakurai-ryo commented Jun 17, 2024 • edited Loading

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

aws-cdk-automation left a comment

Choose a reason for hiding this comment

sakurai-ryo Jun 17, 2024

Choose a reason for hiding this comment

sakurai-ryo Jun 17, 2024

Choose a reason for hiding this comment

sakurai-ryo Jun 17, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Jul 9, 2024

sakurai-ryo commented Jul 9, 2024

mergify bot commented Jul 9, 2024

✅ Branch has been successfully updated

aws-cdk-automation commented Jul 16, 2024

aws-cdk-automation commented Oct 5, 2024

aws-cdk-automation commented Oct 5, 2024

AWS CodeBuild CI Report

sakurai-ryo commented Jun 17, 2024 •

edited

Loading