Skip to content

Commit be1207b

Browse files
fix(core): file asset publishing role not used in cdk diff to upload 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*
1 parent 0c753c3 commit be1207b

File tree

5 files changed

+90
-36
lines changed

5 files changed

+90
-36
lines changed

packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js

+16
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,20 @@ class LambdaStack extends cdk.Stack {
431431
}
432432
}
433433

434+
class IamRolesStack extends cdk.Stack {
435+
constructor(parent, id, props) {
436+
super(parent, id, props);
437+
438+
// Environment variabile is used to create a bunch of roles to test
439+
// that large diff templates are uploaded to S3 to create the changeset.
440+
for(let i = 1; i <= Number(process.env.NUMBER_OF_ROLES) ; i++) {
441+
new iam.Role(this, `Role${i}`, {
442+
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
443+
});
444+
}
445+
}
446+
}
447+
434448
class SessionTagsStack extends cdk.Stack {
435449
constructor(parent, id, props) {
436450
super(parent, id, {
@@ -778,6 +792,8 @@ switch (stackSet) {
778792

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

795+
new IamRolesStack(app, `${stackPrefix}-iam-roles`);
796+
781797
if (process.env.ENABLE_VPC_TESTING == 'IMPORT') {
782798
// this stack performs a VPC lookup so we gate synth
783799
const env = { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION };

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

+24
Original file line numberDiff line numberDiff line change
@@ -1032,6 +1032,30 @@ integTest(
10321032
}),
10331033
);
10341034

1035+
integTest(
1036+
'cdk diff with large changeset does not fail',
1037+
withDefaultFixture(async (fixture) => {
1038+
// GIVEN - small initial stack with only ane IAM role
1039+
await fixture.cdkDeploy('iam-roles', {
1040+
modEnv: {
1041+
NUMBER_OF_ROLES: '1',
1042+
},
1043+
});
1044+
1045+
// WHEN - adding 200 roles to the same stack to create a large diff
1046+
const diff = await fixture.cdk(['diff', fixture.fullStackName('iam-roles')], {
1047+
verbose: true,
1048+
modEnv: {
1049+
NUMBER_OF_ROLES: '200',
1050+
},
1051+
});
1052+
1053+
// Assert that the CLI assumes the file publishing role:
1054+
expect(diff).toMatch(/Assuming role .*file-publishing-role/);
1055+
expect(diff).toContain('success: Published');
1056+
}),
1057+
);
1058+
10351059
integTest(
10361060
'cdk diff --security-only successfully outputs sso-permission-set-without-managed-policy information',
10371061
withDefaultFixture(async (fixture) => {

packages/aws-cdk/lib/api/deployments.ts

+4-3
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ import { StackActivityMonitor, StackActivityProgress } from './util/cloudformati
1717
import { StackEventPoller } from './util/cloudformation/stack-event-poller';
1818
import { RollbackChoice } from './util/cloudformation/stack-status';
1919
import { replaceEnvPlaceholders } from './util/placeholders';
20-
import { makeBodyParameterAndUpload } from './util/template-body-parameter';
20+
import { makeBodyParameter } from './util/template-body-parameter';
21+
import { AssetManifestBuilder } from '../util/asset-manifest-builder';
2122
import { buildAssets, publishAssets, BuildAssetsOptions, PublishAssetsOptions, PublishingAws, EVENT_TO_LOGGER } from '../util/asset-publishing';
2223

2324
const BOOTSTRAP_STACK_VERSION_FOR_ROLLBACK = 23;
@@ -426,11 +427,11 @@ export class Deployments {
426427
const cfn = stackSdk.cloudFormation();
427428

428429
// Upload the template, if necessary, before passing it to CFN
429-
const cfnParam = await makeBodyParameterAndUpload(
430+
const cfnParam = await makeBodyParameter(
430431
stackArtifact,
431432
resolvedEnvironment,
433+
new AssetManifestBuilder(),
432434
envResources,
433-
this.sdkProvider,
434435
stackSdk);
435436

436437
const response = await cfn.getTemplateSummary(cfnParam).promise();

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

+45-3
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@ import { DescribeChangeSetOutput } from '@aws-cdk/cloudformation-diff';
33
import { SSMPARAM_NO_INVALIDATE } from '@aws-cdk/cx-api';
44
import * as cxapi from '@aws-cdk/cx-api';
55
import { CloudFormation } from 'aws-sdk';
6+
import { AssetManifest, FileManifestEntry } from 'cdk-assets';
67
import { StackStatus } from './cloudformation/stack-status';
7-
import { makeBodyParameterAndUpload, TemplateBodyParameter } from './template-body-parameter';
8+
import { makeBodyParameter, TemplateBodyParameter } from './template-body-parameter';
89
import { debug } from '../../logging';
910
import { deserializeStructure } from '../../serialize';
11+
import { AssetManifestBuilder } from '../../util/asset-manifest-builder';
1012
import { SdkProvider } from '../aws-auth';
1113
import { Deployments } from '../deployments';
1214

@@ -338,14 +340,54 @@ export async function createDiffChangeSet(options: PrepareChangeSetOptions): Pro
338340
return uploadBodyParameterAndCreateChangeSet(options);
339341
}
340342

343+
/**
344+
* Returns all file entries from an AssetManifestArtifact. This is used in the
345+
* `uploadBodyParameterAndCreateChangeSet` function to find all template asset files to build and publish.
346+
*
347+
* Returns a tuple of [AssetManifest, FileManifestEntry[]]
348+
*/
349+
function fileEntriesFromAssetManifestArtifact(artifact: cxapi.AssetManifestArtifact): [AssetManifest, FileManifestEntry[]] {
350+
const assets: (FileManifestEntry)[] = [];
351+
const fileName = artifact.file;
352+
const assetManifest = AssetManifest.fromFile(fileName);
353+
354+
assetManifest.entries.forEach(entry => {
355+
if (entry.type === 'file') {
356+
const source = (entry as FileManifestEntry).source;
357+
if (source.path && (source.path.endsWith('.template.json'))) {
358+
assets.push(entry as FileManifestEntry);
359+
}
360+
}
361+
});
362+
return [assetManifest, assets];
363+
}
364+
341365
async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOptions): Promise<DescribeChangeSetOutput | undefined> {
342366
try {
367+
for (const artifact of options.stack.dependencies) {
368+
// Skip artifact if it is not an Asset Manifest Artifact
369+
if (!cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) {
370+
continue;
371+
}
372+
373+
// Build and publish each file entry of the Asset Manifest Artifact:
374+
const [assetManifest, file_entries] = fileEntriesFromAssetManifestArtifact(artifact);
375+
for (const entry of file_entries) {
376+
await options.deployments.buildSingleAsset(artifact, assetManifest, entry, {
377+
stack: options.stack,
378+
});
379+
await options.deployments.publishSingleAsset(assetManifest, entry, {
380+
stack: options.stack,
381+
});
382+
}
383+
}
343384
const preparedSdk = (await options.deployments.prepareSdkWithDeployRole(options.stack));
344-
const bodyParameter = await makeBodyParameterAndUpload(
385+
386+
const bodyParameter = await makeBodyParameter(
345387
options.stack,
346388
preparedSdk.resolvedEnvironment,
389+
new AssetManifestBuilder(),
347390
preparedSdk.envResources,
348-
options.sdkProvider,
349391
preparedSdk.stackSdk,
350392
);
351393
const cfn = preparedSdk.stackSdk.cloudFormation();

packages/aws-cdk/lib/api/util/template-body-parameter.ts

+1-30
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,8 @@ import * as fs from 'fs-extra';
55
import { debug, error } from '../../logging';
66
import { toYAML } from '../../serialize';
77
import { AssetManifestBuilder } from '../../util/asset-manifest-builder';
8-
import { publishAssets } from '../../util/asset-publishing';
98
import { contentHash } from '../../util/content-hash';
10-
import { ISDK, SdkProvider } from '../aws-auth';
9+
import { ISDK } from '../aws-auth';
1110
import { EnvironmentResources } from '../environment-resources';
1211

1312
export type TemplateBodyParameter = {
@@ -85,34 +84,6 @@ export async function makeBodyParameter(
8584
return { TemplateURL: templateURL };
8685
}
8786

88-
/**
89-
* Prepare a body parameter for CFN, performing the upload
90-
*
91-
* Return it as-is if it is small enough to pass in the API call,
92-
* upload to S3 and return the coordinates if it is not.
93-
*/
94-
export async function makeBodyParameterAndUpload(
95-
stack: cxapi.CloudFormationStackArtifact,
96-
resolvedEnvironment: cxapi.Environment,
97-
resources: EnvironmentResources,
98-
sdkProvider: SdkProvider,
99-
sdk: ISDK,
100-
overrideTemplate?: any): Promise<TemplateBodyParameter> {
101-
102-
// We don't have access to the actual asset manifest here, so pretend that the
103-
// stack doesn't have a pre-published URL.
104-
const forceUploadStack = Object.create(stack, {
105-
stackTemplateAssetObjectUrl: { value: undefined },
106-
});
107-
108-
const builder = new AssetManifestBuilder();
109-
const bodyparam = await makeBodyParameter(forceUploadStack, resolvedEnvironment, builder, resources, sdk, overrideTemplate);
110-
const manifest = builder.toManifest(stack.assembly.directory);
111-
await publishAssets(manifest, sdkProvider, resolvedEnvironment, { quiet: true });
112-
113-
return bodyparam;
114-
}
115-
11687
/**
11788
* Format an S3 URL in the manifest for use with CloudFormation
11889
*

0 commit comments

Comments
 (0)