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

fix(core): file asset publishing role not used in cdk diff to upload large templates #31597

Merged
merged 14 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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:
sumupitchayan marked this conversation as resolved.
Show resolved Hide resolved
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(),
sumupitchayan marked this conversation as resolved.
Show resolved Hide resolved
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
Loading