-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
@@ -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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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; |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
.
Exemption Request |
…ff-changeset-role
hmm, the |
…ff-changeset-role
…ff-changeset-role
…ff-changeset-role
…ff-changeset-role
…ff-changeset-role
…aws-cdk into diff-changeset-role
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. |
…ff-changeset-role
The pull request linter fails with the following errors:
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 ✅ A exemption request has been requested. Please wait for a maintainer's review. |
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
2 similar comments
|
|
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 thedeploy-role
has strong privileges, such as deleting the cfn stack.Granting developers the ability to be able to assume the
deploy-role
to use thecdk 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 thecdk diff
command.Description of changes
Introducing a new role
Introduce a new
cfn-changeset-review-role
and give thecdk diff
command the permissions it needs to create changesets.With this change, the required roles for each operation are as follows
The
cfn-changeset-review-role
has the following permissions.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 existinglookup-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 newchangeset-role
.If for some reason it is not possible to assume to
changeset-role
, it will fall back to assume thedeploy-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 theapp-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