Skip to content

Commit

Permalink
fix(core): file asset publishing role not used in cdk diff to uploa…
Browse files Browse the repository at this point in the history
…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*
  • Loading branch information
sumupitchayan authored Oct 3, 2024
1 parent 6387657 commit c9cc174
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 0 deletions.
16 changes: 16 additions & 0 deletions packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,20 @@ class LambdaStack extends cdk.Stack {
}
}

class IamRolesStack extends cdk.Stack {
constructor(parent, id, props) {
super(parent, id, props);

// Environment variabile is used to create a bunch of roles to test
// that large diff templates are uploaded to S3 to create the changeset.
for(let i = 1; i <= Number(process.env.NUMBER_OF_ROLES) ; i++) {
new iam.Role(this, `Role${i}`, {
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
});
}
}
}

class SessionTagsStack extends cdk.Stack {
constructor(parent, id, props) {
super(parent, id, {
Expand Down Expand Up @@ -778,6 +792,8 @@ switch (stackSet) {

new LambdaStack(app, `${stackPrefix}-lambda`);

new IamRolesStack(app, `${stackPrefix}-iam-roles`);

if (process.env.ENABLE_VPC_TESTING == 'IMPORT') {
// this stack performs a VPC lookup so we gate synth
const env = { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1032,6 +1032,30 @@ integTest(
}),
);

integTest(
'cdk diff with large changeset does not fail',
withDefaultFixture(async (fixture) => {
// GIVEN - small initial stack with only ane IAM role
await fixture.cdkDeploy('iam-roles', {
modEnv: {
NUMBER_OF_ROLES: '1',
},
});

// WHEN - adding 200 roles to the same stack to create a large diff
const diff = await fixture.cdk(['diff', fixture.fullStackName('iam-roles')], {
verbose: true,
modEnv: {
NUMBER_OF_ROLES: '200',
},
});

// Assert that the CLI assumes the file publishing role:
expect(diff).toMatch(/Assuming role .*file-publishing-role/);
expect(diff).toContain('success: Published');
}),
);

integTest(
'cdk diff --security-only successfully outputs sso-permission-set-without-managed-policy information',
withDefaultFixture(async (fixture) => {
Expand Down

0 comments on commit c9cc174

Please sign in to comment.