From 4b9ce0df46295ca2c1cc6c6973d5b21e3667fd89 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 20 Oct 2022 15:20:46 +0200 Subject: [PATCH] feat(cli): allow disabling parallel asset publishing Once again, any change to anything anywhere broke someone. In this particular case, parallel asset publishing (#19367, and in particular building) broke a use case where someone uses a tool to build assets that is not concurrency-safe. So -- now we need to make that configurable. Command line: ``` cdk deploy --no-asset-parallelism ``` cdk.json: ``` { "assetParallelism": false } ``` Environment variables: ``` export CDK_ASSET_PARALLELISM=false ``` --- .../lib/api/cloudformation-deployments.ts | 26 ++++++- packages/aws-cdk/lib/api/deploy-stack.ts | 11 ++- packages/aws-cdk/lib/cdk-toolkit.ts | 15 +++- packages/aws-cdk/lib/cli.ts | 4 +- packages/aws-cdk/lib/settings.ts | 1 + packages/aws-cdk/lib/util/asset-publishing.ts | 18 ++++- .../api/cloudformation-deployments.test.ts | 2 +- packages/aws-cdk/test/cdk-toolkit.test.ts | 71 +++++++++++++++++-- packages/aws-cdk/test/util.ts | 24 ++++++- 9 files changed, 158 insertions(+), 14 deletions(-) diff --git a/packages/aws-cdk/lib/api/cloudformation-deployments.ts b/packages/aws-cdk/lib/api/cloudformation-deployments.ts index e23c3be1e8747..c2477c56fbc3f 100644 --- a/packages/aws-cdk/lib/api/cloudformation-deployments.ts +++ b/packages/aws-cdk/lib/api/cloudformation-deployments.ts @@ -2,7 +2,7 @@ import * as cxapi from '@aws-cdk/cx-api'; import { AssetManifest } from 'cdk-assets'; import { Tag } from '../cdk-toolkit'; import { debug, warning } from '../logging'; -import { buildAssets, publishAssets } from '../util/asset-publishing'; +import { buildAssets, publishAssets, BuildAssetsOptions, PublishAssetsOptions } from '../util/asset-publishing'; import { Mode } from './aws-auth/credentials'; import { ISDK } from './aws-auth/sdk'; import { SdkProvider } from './aws-auth/sdk-provider'; @@ -253,6 +253,13 @@ export interface DeployStackOptions { * @default true To remain backward compatible. */ readonly buildAssets?: boolean; + + /** + * Whether to build/publish assets in parallel + * + * @default true To remain backward compatible. + */ + readonly assetParallelism?: boolean; } export interface BuildStackAssetsOptions { @@ -274,6 +281,11 @@ export interface BuildStackAssetsOptions { * @default - Current role */ readonly roleArn?: string; + + /** + * Options to pass on to `buildAsests()` function + */ + readonly buildOptions?: BuildAssetsOptions; } interface PublishStackAssetsOptions { @@ -283,6 +295,11 @@ interface PublishStackAssetsOptions { * @default true To remain backward compatible. */ readonly buildAssets?: boolean; + + /** + * Options to pass on to `publishAsests()` function + */ + readonly publishOptions?: Omit; } export interface DestroyStackOptions { @@ -401,6 +418,9 @@ export class CloudFormationDeployments { if (options.resourcesToImport === undefined) { await this.publishStackAssets(options.stack, toolkitInfo, { buildAssets: options.buildAssets ?? true, + publishOptions: { + parallel: options.assetParallelism, + }, }); } @@ -434,6 +454,7 @@ export class CloudFormationDeployments { extraUserAgent: options.extraUserAgent, resourcesToImport: options.resourcesToImport, overrideTemplate: options.overrideTemplate, + assetParallelism: options.assetParallelism, }); } @@ -529,7 +550,7 @@ export class CloudFormationDeployments { toolkitInfo); const manifest = AssetManifest.fromFile(assetArtifact.file); - await buildAssets(manifest, this.sdkProvider, stackEnv); + await buildAssets(manifest, this.sdkProvider, stackEnv, options.buildOptions); } } @@ -549,6 +570,7 @@ export class CloudFormationDeployments { const manifest = AssetManifest.fromFile(assetArtifact.file); await publishAssets(manifest, this.sdkProvider, stackEnv, { + ...options.publishOptions, buildAssets: options.buildAssets ?? true, }); } diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts index 23e16d14b989d..31dd7938b4cf2 100644 --- a/packages/aws-cdk/lib/api/deploy-stack.ts +++ b/packages/aws-cdk/lib/api/deploy-stack.ts @@ -198,6 +198,13 @@ export interface DeployStackOptions { * @default - Use the stored template */ readonly overrideTemplate?: any; + + /** + * Whether to build/publish assets in parallel + * + * @default true To remain backward compatible. + */ + readonly assetParallelism?: boolean; } export type DeploymentMethod = @@ -287,7 +294,9 @@ export async function deployStack(options: DeployStackOptions): Promise): Promise { + private async buildAllAssetsForSingleStack(stack: cxapi.CloudFormationStackArtifact, options: Pick): Promise { // Check whether the stack has an asset manifest before trying to build and publish. if (!stack.dependencies.some(cxapi.AssetManifestArtifact.isAssetManifestArtifact)) { return; @@ -775,6 +776,9 @@ export class CdkToolkit { stack, roleArn: options.roleArn, toolkitStackName: options.toolkitStackName, + buildOptions: { + parallel: options.assetParallelism, + }, }); print('\n%s: assets built\n', chalk.bold(stack.displayName)); } @@ -1029,6 +1033,15 @@ export interface DeployOptions extends CfnDeployOptions, WatchOptions { * @default 1 */ readonly concurrency?: number; + + /** + * Build/publish assets for a single stack in parallel + * + * Independent of whether stacks are being done in parallel or no. + * + * @default true + */ + readonly assetParallelism?: boolean; } export interface ImportOptions extends CfnDeployOptions { diff --git a/packages/aws-cdk/lib/cli.ts b/packages/aws-cdk/lib/cli.ts index 5716a29ed5e64..1f49d63430437 100644 --- a/packages/aws-cdk/lib/cli.ts +++ b/packages/aws-cdk/lib/cli.ts @@ -155,7 +155,8 @@ async function parseCommandLineArguments() { "'true' by default, use --no-logs to turn off. " + "Only in effect if specified alongside the '--watch' option", }) - .option('concurrency', { type: 'number', desc: 'Maximum number of simultaneous deployments (dependency permitting) to execute.', default: 1, requiresArg: true }), + .option('concurrency', { type: 'number', desc: 'Maximum number of simultaneous deployments (dependency permitting) to execute.', default: 1, requiresArg: true }) + .option('asset-parallelism', { type: 'boolean', desc: 'Whether to build/publish assets in parallel' }), ) .command('import [STACK]', 'Import existing resource(s) into the given STACK', (yargs: Argv) => yargs .option('execute', { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true }) @@ -514,6 +515,7 @@ async function initCommandLine() { watch: args.watch, traceLogs: args.logs, concurrency: args.concurrency, + assetParallelism: args.assetParallelism, }); case 'import': diff --git a/packages/aws-cdk/lib/settings.ts b/packages/aws-cdk/lib/settings.ts index b94f563eb8365..b4e3a2f4ebf7d 100644 --- a/packages/aws-cdk/lib/settings.ts +++ b/packages/aws-cdk/lib/settings.ts @@ -289,6 +289,7 @@ export class Settings { lookups: argv.lookups, rollback: argv.rollback, notices: argv.notices, + assetParallelism: argv['asset-parallelism'], }); } diff --git a/packages/aws-cdk/lib/util/asset-publishing.ts b/packages/aws-cdk/lib/util/asset-publishing.ts index b4dbe49ceae0d..af53828106069 100644 --- a/packages/aws-cdk/lib/util/asset-publishing.ts +++ b/packages/aws-cdk/lib/util/asset-publishing.ts @@ -18,6 +18,13 @@ export interface PublishAssetsOptions { * @default true To remain backward compatible. */ readonly buildAssets?: boolean; + + /** + * Whether to build/publish assets in parallel + * + * @default true To remain backward compatible. + */ + readonly parallel?: boolean; } /** @@ -44,7 +51,7 @@ export async function publishAssets( aws: new PublishingAws(sdk, targetEnv), progressListener: new PublishingProgressListener(options.quiet ?? false), throwOnError: false, - publishInParallel: true, + publishInParallel: options.parallel ?? true, buildAssets: options.buildAssets ?? true, publishAssets: true, }); @@ -59,6 +66,13 @@ export interface BuildAssetsOptions { * Print progress at 'debug' level */ readonly quiet?: boolean; + + /** + * Build assets in parallel + * + * @default true + */ + readonly parallel?: boolean; } /** @@ -85,7 +99,7 @@ export async function buildAssets( aws: new PublishingAws(sdk, targetEnv), progressListener: new PublishingProgressListener(options.quiet ?? false), throwOnError: false, - publishInParallel: true, + publishInParallel: options.parallel ?? true, buildAssets: true, publishAssets: false, }); diff --git a/packages/aws-cdk/test/api/cloudformation-deployments.test.ts b/packages/aws-cdk/test/api/cloudformation-deployments.test.ts index 26fb0f8bc5d7b..c2ce217ca1941 100644 --- a/packages/aws-cdk/test/api/cloudformation-deployments.test.ts +++ b/packages/aws-cdk/test/api/cloudformation-deployments.test.ts @@ -904,7 +904,7 @@ test('building assets', async () => { name: 'aws://account/region', region: 'region', }); - expect(buildAssets).toBeCalledWith(expectedAssetManifest, sdkProvider, expectedEnvironment); + expect(buildAssets).toBeCalledWith(expectedAssetManifest, sdkProvider, expectedEnvironment, undefined); }); function pushStackResourceSummaries(stackName: string, ...items: CloudFormation.StackResourceSummary[]) { diff --git a/packages/aws-cdk/test/cdk-toolkit.test.ts b/packages/aws-cdk/test/cdk-toolkit.test.ts index b0359fbb6dbc7..205eb8b28910a 100644 --- a/packages/aws-cdk/test/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cdk-toolkit.test.ts @@ -52,8 +52,11 @@ jest.mock('../lib/logging', () => ({ ...jest.requireActual('../lib/logging'), data: mockData, })); +jest.setTimeout(30_000); +import * as path from 'path'; import * as cxschema from '@aws-cdk/cloud-assembly-schema'; +import { Manifest } from '@aws-cdk/cloud-assembly-schema'; import * as cxapi from '@aws-cdk/cx-api'; import { Bootstrapper } from '../lib/api/bootstrap'; import { CloudFormationDeployments, DeployStackOptions, DestroyStackOptions } from '../lib/api/cloudformation-deployments'; @@ -62,7 +65,8 @@ import { Template } from '../lib/api/util/cloudformation'; import { CdkToolkit, Tag } from '../lib/cdk-toolkit'; import { RequireApproval } from '../lib/diff'; import { flatten } from '../lib/util'; -import { instanceMockFrom, MockCloudExecutable, TestStackArtifact } from './util'; +import { instanceMockFrom, MockCloudExecutable, TestStackArtifact, withMocked } from './util'; +import { MockSdkProvider } from './util/mock-sdk'; let cloudExecutable: MockCloudExecutable; let bootstrapper: jest.Mocked; @@ -555,6 +559,36 @@ describe('deploy', () => { expect(cloudExecutable.hasApp).toEqual(false); expect(mockSynthesize).not.toHaveBeenCalled(); }); + + test('can disable asset parallelism', async () => { + // GIVEN + cloudExecutable = new MockCloudExecutable({ + stacks: [MockStack.MOCK_STACK_WITH_ASSET], + }); + const fakeCloudFormation = new FakeCloudFormation({}); + + const toolkit = new CdkToolkit({ + cloudExecutable, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + cloudFormation: fakeCloudFormation, + }); + + // WHEN + // Not the best test but following this through to the asset publishing library fails + await withMocked(fakeCloudFormation, 'buildStackAssets', async (mockBuildStackAssets) => { + await toolkit.deploy({ + selector: { patterns: ['Test-Stack-Asset'] }, + assetParallelism: false, + }); + + expect(mockBuildStackAssets).toHaveBeenCalledWith(expect.objectContaining({ + buildOptions: expect.objectContaining({ + parallel: false, + }), + })); + }); + }); }); }); @@ -911,6 +945,23 @@ class MockStack { }, displayName: 'Test-Stack-A/witherrors', } + public static readonly MOCK_STACK_WITH_ASSET: TestStackArtifact = { + stackName: 'Test-Stack-Asset', + template: { Resources: { TemplateName: 'Test-Stack-Asset' } }, + env: 'aws://123456789012/bermuda-triangle-1', + assetManifest: { + version: Manifest.version(), + files: { + xyz: { + source: { + path: path.resolve(__dirname, '..', 'LICENSE'), + }, + destinations: { + }, + }, + }, + }, + } } class FakeCloudFormation extends CloudFormationDeployments { @@ -921,7 +972,7 @@ class FakeCloudFormation extends CloudFormationDeployments { expectedTags: { [stackName: string]: { [key: string]: string } } = {}, expectedNotificationArns?: string[], ) { - super({ sdkProvider: undefined as any }); + super({ sdkProvider: new MockSdkProvider() }); for (const [stackName, tags] of Object.entries(expectedTags)) { this.expectedTags[stackName] = @@ -934,9 +985,17 @@ class FakeCloudFormation extends CloudFormationDeployments { } public deployStack(options: DeployStackOptions): Promise { - expect([MockStack.MOCK_STACK_A.stackName, MockStack.MOCK_STACK_B.stackName, MockStack.MOCK_STACK_C.stackName]) - .toContain(options.stack.stackName); - expect(options.tags).toEqual(this.expectedTags[options.stack.stackName]); + expect([ + MockStack.MOCK_STACK_A.stackName, + MockStack.MOCK_STACK_B.stackName, + MockStack.MOCK_STACK_C.stackName, + MockStack.MOCK_STACK_WITH_ASSET.stackName, + ]).toContain(options.stack.stackName); + + if (this.expectedTags[options.stack.stackName]) { + expect(options.tags).toEqual(this.expectedTags[options.stack.stackName]); + } + expect(options.notificationArns).toEqual(this.expectedNotificationArns); return Promise.resolve({ stackArn: `arn:aws:cloudformation:::stack/${options.stack.stackName}/MockedOut`, @@ -959,6 +1018,8 @@ class FakeCloudFormation extends CloudFormationDeployments { return Promise.resolve({}); case MockStack.MOCK_STACK_C.stackName: return Promise.resolve({}); + case MockStack.MOCK_STACK_WITH_ASSET.stackName: + return Promise.resolve({}); default: return Promise.reject(`Not an expected mock stack: ${stack.stackName}`); } diff --git a/packages/aws-cdk/test/util.ts b/packages/aws-cdk/test/util.ts index 82447948b83cb..751c1c1ad6bba 100644 --- a/packages/aws-cdk/test/util.ts +++ b/packages/aws-cdk/test/util.ts @@ -1,6 +1,7 @@ import * as fs from 'fs'; import * as path from 'path'; import * as cxschema from '@aws-cdk/cloud-assembly-schema'; +import { AssetManifest } from '@aws-cdk/cloud-assembly-schema'; import * as cxapi from '@aws-cdk/cx-api'; import { CloudExecutable } from '../lib/api/cxapp/cloud-executable'; import { Configuration } from '../lib/settings'; @@ -14,10 +15,15 @@ export interface TestStackArtifact { env?: string, depends?: string[]; metadata?: cxapi.StackMetadata; + + /** Old-style assets */ assets?: cxschema.AssetMetadataEntry[]; properties?: Partial; terminationProtection?: boolean; displayName?: string; + + /** New-style assets */ + assetManifest?: AssetManifest; } export interface TestAssembly { @@ -69,11 +75,26 @@ function addAttributes(assembly: TestAssembly, builder: cxapi.CloudAssemblyBuild builder.addMissing(missing); } + const dependencies = [...stack.depends ?? []]; + + if (stack.assetManifest) { + const manifestFile = `${stack.stackName}.assets.json`; + fs.writeFileSync(path.join(builder.outdir, manifestFile), JSON.stringify(stack.assetManifest, undefined, 2)); + dependencies.push(`${stack.stackName}.assets`); + builder.addArtifact(`${stack.stackName}.assets`, { + type: cxschema.ArtifactType.ASSET_MANIFEST, + environment: stack.env || 'aws://123456789012/here', + properties: { + file: manifestFile, + }, + }); + } + builder.addArtifact(stack.stackName, { type: cxschema.ArtifactType.AWS_CLOUDFORMATION_STACK, environment: stack.env || 'aws://123456789012/here', - dependencies: stack.depends, + dependencies, metadata, properties: { ...stack.properties, @@ -82,6 +103,7 @@ function addAttributes(assembly: TestAssembly, builder: cxapi.CloudAssemblyBuild }, displayName: stack.displayName, }); + } }