Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 11 commits into from

Conversation

sakurai-ryo
Copy link
Contributor

@sakurai-ryo sakurai-ryo commented May 24, 2024

Issue # (if applicable)

Closes #29767
Closes #29936

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.

Checklist


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

@github-actions github-actions bot added star-contributor [Pilot] contributed between 25-49 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels May 24, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team May 24, 2024 16:01
@@ -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');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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;
Copy link
Contributor Author

@sakurai-ryo sakurai-ryo May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -355,7 +354,6 @@ async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOp
bodyParameter,
parameters: options.parameters,
resourcesToImport: options.resourcesToImport,
role: executionRoleArn,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sakurai-ryo
Copy link
Contributor Author

Exemption Request
Wait for the maintainer to run the test.

@github-actions github-actions bot added bug This issue is a bug. and removed feature-request A feature should be added or improved. labels May 24, 2024
@sakurai-ryo
Copy link
Contributor Author

hmm, the pr-linter/exemption-requested label is not attached.

@sakurai-ryo sakurai-ryo changed the title feat(cdk): introduce the new cfn-changeset-role for the diff command feat(cdk): introduce the new cfn-changeset-review-role for the diff command May 24, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jun 7, 2024
@aws-cdk-automation
Copy link
Collaborator

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.

@aws-cdk-automation
Copy link
Collaborator

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

✅ A exemption request has been requested. Please wait for a maintainer's review.

Copy link
Contributor

mergify bot commented Jun 17, 2024

⚠️ The sha of the head commit of this PR conflicts with #30568. Mergify cannot evaluate rules on this PR. ⚠️

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 2b28865
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

mergify bot commented Jun 26, 2024

⚠️ The sha of the head commit of this PR conflicts with #30568. Mergify cannot evaluate rules on this PR. ⚠️

2 similar comments
Copy link
Contributor

mergify bot commented Jul 7, 2024

⚠️ The sha of the head commit of this PR conflicts with #30568. Mergify cannot evaluate rules on this PR. ⚠️

Copy link
Contributor

mergify bot commented Jul 14, 2024

⚠️ The sha of the head commit of this PR conflicts with #30568. Mergify cannot evaluate rules on this PR. ⚠️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort p1 pr/needs-cli-test-run This PR needs CLI tests run against it. star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
2 participants