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/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*
  • Loading branch information
sumupitchayan authored Oct 3, 2024
1 parent 0c753c3 commit be1207b
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 36 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
7 changes: 4 additions & 3 deletions packages/aws-cdk/lib/api/deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import { StackActivityMonitor, StackActivityProgress } from './util/cloudformati
import { StackEventPoller } from './util/cloudformation/stack-event-poller';
import { RollbackChoice } from './util/cloudformation/stack-status';
import { replaceEnvPlaceholders } from './util/placeholders';
import { makeBodyParameterAndUpload } from './util/template-body-parameter';
import { makeBodyParameter } from './util/template-body-parameter';
import { AssetManifestBuilder } from '../util/asset-manifest-builder';
import { buildAssets, publishAssets, BuildAssetsOptions, PublishAssetsOptions, PublishingAws, EVENT_TO_LOGGER } from '../util/asset-publishing';

const BOOTSTRAP_STACK_VERSION_FOR_ROLLBACK = 23;
Expand Down Expand Up @@ -426,11 +427,11 @@ export class Deployments {
const cfn = stackSdk.cloudFormation();

// Upload the template, if necessary, before passing it to CFN
const cfnParam = await makeBodyParameterAndUpload(
const cfnParam = await makeBodyParameter(
stackArtifact,
resolvedEnvironment,
new AssetManifestBuilder(),
envResources,
this.sdkProvider,
stackSdk);

const response = await cfn.getTemplateSummary(cfnParam).promise();
Expand Down
48 changes: 45 additions & 3 deletions packages/aws-cdk/lib/api/util/cloudformation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import { DescribeChangeSetOutput } from '@aws-cdk/cloudformation-diff';
import { SSMPARAM_NO_INVALIDATE } from '@aws-cdk/cx-api';
import * as cxapi from '@aws-cdk/cx-api';
import { CloudFormation } from 'aws-sdk';
import { AssetManifest, FileManifestEntry } from 'cdk-assets';
import { StackStatus } from './cloudformation/stack-status';
import { makeBodyParameterAndUpload, TemplateBodyParameter } from './template-body-parameter';
import { makeBodyParameter, TemplateBodyParameter } from './template-body-parameter';
import { debug } from '../../logging';
import { deserializeStructure } from '../../serialize';
import { AssetManifestBuilder } from '../../util/asset-manifest-builder';
import { SdkProvider } from '../aws-auth';
import { Deployments } from '../deployments';

Expand Down Expand Up @@ -338,14 +340,54 @@ export async function createDiffChangeSet(options: PrepareChangeSetOptions): Pro
return uploadBodyParameterAndCreateChangeSet(options);
}

/**
* Returns all file entries from an AssetManifestArtifact. This is used in the
* `uploadBodyParameterAndCreateChangeSet` function to find all template asset files to build and publish.
*
* Returns a tuple of [AssetManifest, FileManifestEntry[]]
*/
function fileEntriesFromAssetManifestArtifact(artifact: cxapi.AssetManifestArtifact): [AssetManifest, FileManifestEntry[]] {
const assets: (FileManifestEntry)[] = [];
const fileName = artifact.file;
const assetManifest = AssetManifest.fromFile(fileName);

assetManifest.entries.forEach(entry => {
if (entry.type === 'file') {
const source = (entry as FileManifestEntry).source;
if (source.path && (source.path.endsWith('.template.json'))) {
assets.push(entry as FileManifestEntry);
}
}
});
return [assetManifest, assets];
}

async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOptions): Promise<DescribeChangeSetOutput | undefined> {
try {
for (const artifact of options.stack.dependencies) {
// Skip artifact if it is not an Asset Manifest Artifact
if (!cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) {
continue;
}

// Build and publish each file entry of the Asset Manifest Artifact:
const [assetManifest, file_entries] = fileEntriesFromAssetManifestArtifact(artifact);
for (const entry of file_entries) {
await options.deployments.buildSingleAsset(artifact, assetManifest, entry, {
stack: options.stack,
});
await options.deployments.publishSingleAsset(assetManifest, entry, {
stack: options.stack,
});
}
}
const preparedSdk = (await options.deployments.prepareSdkWithDeployRole(options.stack));
const bodyParameter = await makeBodyParameterAndUpload(

const bodyParameter = await makeBodyParameter(
options.stack,
preparedSdk.resolvedEnvironment,
new AssetManifestBuilder(),
preparedSdk.envResources,
options.sdkProvider,
preparedSdk.stackSdk,
);
const cfn = preparedSdk.stackSdk.cloudFormation();
Expand Down
31 changes: 1 addition & 30 deletions packages/aws-cdk/lib/api/util/template-body-parameter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@ import * as fs from 'fs-extra';
import { debug, error } from '../../logging';
import { toYAML } from '../../serialize';
import { AssetManifestBuilder } from '../../util/asset-manifest-builder';
import { publishAssets } from '../../util/asset-publishing';
import { contentHash } from '../../util/content-hash';
import { ISDK, SdkProvider } from '../aws-auth';
import { ISDK } from '../aws-auth';
import { EnvironmentResources } from '../environment-resources';

export type TemplateBodyParameter = {
Expand Down Expand Up @@ -85,34 +84,6 @@ export async function makeBodyParameter(
return { TemplateURL: templateURL };
}

/**
* Prepare a body parameter for CFN, performing the upload
*
* Return it as-is if it is small enough to pass in the API call,
* upload to S3 and return the coordinates if it is not.
*/
export async function makeBodyParameterAndUpload(
stack: cxapi.CloudFormationStackArtifact,
resolvedEnvironment: cxapi.Environment,
resources: EnvironmentResources,
sdkProvider: SdkProvider,
sdk: ISDK,
overrideTemplate?: any): Promise<TemplateBodyParameter> {

// We don't have access to the actual asset manifest here, so pretend that the
// stack doesn't have a pre-published URL.
const forceUploadStack = Object.create(stack, {
stackTemplateAssetObjectUrl: { value: undefined },
});

const builder = new AssetManifestBuilder();
const bodyparam = await makeBodyParameter(forceUploadStack, resolvedEnvironment, builder, resources, sdk, overrideTemplate);
const manifest = builder.toManifest(stack.assembly.directory);
await publishAssets(manifest, sdkProvider, resolvedEnvironment, { quiet: true });

return bodyparam;
}

/**
* Format an S3 URL in the manifest for use with CloudFormation
*
Expand Down

0 comments on commit be1207b

Please sign in to comment.