From 68ed8caeb7e8e17d82f77f9a618723e0af367e5a Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Thu, 8 Jun 2023 22:01:57 +0200 Subject: [PATCH] fix(cli): assets shared between stages lead to an error (#25907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The problem would manifest as this error message: ``` ❌ Deployment failed: Error: Duplicate use of node id: 07a6878c7a2ec9b49ef3c0ece94cef1c2dd20fba34ca9650dfa6e7e00f2b9961:current_account-current_region-build ``` The problem was that we were using the full asset "destination identifier" for both the build and publish steps, but then were trying to use the `source` object to deduplicate build steps. A more robust solution is to only use the asset identifier (excluding the destination identifier) for the build step, which includes all data necessary to deduplicate the asset. No need to look at the source at all anymore. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-cdk/lib/util/work-graph-builder.ts | 19 +++-- .../aws-cdk/test/work-graph-builder.test.ts | 74 ++++++++++++++++--- packages/cdk-assets/lib/asset-manifest.ts | 13 +++- 3 files changed, 83 insertions(+), 23 deletions(-) diff --git a/packages/aws-cdk/lib/util/work-graph-builder.ts b/packages/aws-cdk/lib/util/work-graph-builder.ts index 64dcb0d91e60a..4da85242d4c20 100644 --- a/packages/aws-cdk/lib/util/work-graph-builder.ts +++ b/packages/aws-cdk/lib/util/work-graph-builder.ts @@ -19,7 +19,6 @@ export class WorkGraphBuilder { 'stack': 5, }; private readonly graph = new WorkGraph(); - private readonly assetBuildNodes = new Map; constructor(private readonly prebuildAssets: boolean, private readonly idPrefix = '') { } @@ -39,12 +38,16 @@ export class WorkGraphBuilder { */ // eslint-disable-next-line max-len private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) { - const buildId = `${this.idPrefix}${asset.id}-build`; + // Just the artifact identifier + const assetId = asset.id.assetId; + // Unique per destination where the artifact needs to go + const assetDestinationId = `${asset.id}`; - // Add the build node, but only one per "source" - // The genericSource includes a relative path we could make absolute to do more effective deduplication of build steps. Not doing that right now. - const assetBuildNodeKey = JSON.stringify(asset.genericSource); - if (!this.assetBuildNodes.has(assetBuildNodeKey)) { + const buildId = `${this.idPrefix}${assetId}-build`; + const publishNodeId = `${this.idPrefix}${assetDestinationId}-publish`; + + // Build node only gets added once because they are all the same + if (!this.graph.tryGetNode(buildId)) { const node: AssetBuildNode = { type: 'asset-build', id: buildId, @@ -60,13 +63,9 @@ export class WorkGraphBuilder { deploymentState: DeploymentState.PENDING, priority: WorkGraphBuilder.PRIORITIES['asset-build'], }; - this.assetBuildNodes.set(assetBuildNodeKey, node); this.graph.addNodes(node); } - // Always add the publish - const publishNodeId = `${this.idPrefix}${asset.id}-publish`; - const publishNode = this.graph.tryGetNode(publishNodeId); if (!publishNode) { this.graph.addNodes({ diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index e89d8e35e911d..686affae6bfb6 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/packages/aws-cdk/test/work-graph-builder.test.ts @@ -3,6 +3,7 @@ import * as path from 'path'; import * as cxschema from '@aws-cdk/cloud-assembly-schema'; import * as cxapi from '@aws-cdk/cx-api'; import { CloudAssemblyBuilder } from '@aws-cdk/cx-api'; +import { WorkGraph } from '../lib/util/work-graph'; import { WorkGraphBuilder } from '../lib/util/work-graph-builder'; import { AssetBuildNode, AssetPublishNode, StackNode, WorkNode } from '../lib/util/work-graph-types'; @@ -36,14 +37,14 @@ describe('with some stacks and assets', () => { expect(graph.node('F1:D1-publish')).toEqual(expect.objectContaining({ type: 'asset-publish', - dependencies: new Set(['F1:D1-build']), + dependencies: new Set(['F1-build']), } as Partial)); }); test('with prebuild off, asset building inherits dependencies from their parent stack', () => { const graph = new WorkGraphBuilder(false).build(assembly.artifacts); - expect(graph.node('F1:D1-build')).toEqual(expect.objectContaining({ + expect(graph.node('F1-build')).toEqual(expect.objectContaining({ type: 'asset-build', dependencies: new Set(['stack0', 'stack1']), } as Partial)); @@ -52,7 +53,7 @@ describe('with some stacks and assets', () => { test('with prebuild on, assets only have their own dependencies', () => { const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - expect(graph.node('F1:D1-build')).toEqual(expect.objectContaining({ + expect(graph.node('F1-build')).toEqual(expect.objectContaining({ type: 'asset-build', dependencies: new Set(['stack0']), } as Partial)); @@ -138,17 +139,11 @@ describe('tests that use assets', () => { const assembly = rootBuilder.buildAssembly(); - const traversal: string[] = []; const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - await graph.doParallel(1, { - deployStack: async (node) => { traversal.push(node.id); }, - buildAsset: async (node) => { traversal.push(node.id); }, - publishAsset: async (node) => { traversal.push(node.id); }, - }); + const traversal = await traverseAndRecord(graph); - expect(traversal).toHaveLength(4); // 1 asset build, 1 asset publish, 2 stacks expect(traversal).toEqual([ - 'work-graph-builder.test.js:D1-build', + 'work-graph-builder.test.js-build', 'work-graph-builder.test.js:D1-publish', 'StackA', 'StackB', @@ -171,6 +166,53 @@ describe('tests that use assets', () => { // THEN expect(graph.findCycle()).toBeUndefined(); }); + + test('the same asset to different destinations is only built once', async () => { + addStack(rootBuilder, 'StackA', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackA.assets'], + }); + addAssets(rootBuilder, 'StackA.assets', { + files: { + abcdef: { + source: { path: __dirname }, + destinations: { + D1: { bucketName: 'bucket1', objectKey: 'key' }, + D2: { bucketName: 'bucket2', objectKey: 'key' }, + }, + }, + }, + }); + + addStack(rootBuilder, 'StackB', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackB.assets', 'StackA'], + }); + addAssets(rootBuilder, 'StackB.assets', { + files: { + abcdef: { + source: { path: __dirname }, + destinations: { + D3: { bucketName: 'bucket3', objectKey: 'key' }, + }, + }, + }, + }); + + const assembly = rootBuilder.buildAssembly(); + + const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + const traversal = await traverseAndRecord(graph); + + expect(traversal).toEqual([ + 'abcdef-build', + 'abcdef:D1-publish', + 'abcdef:D2-publish', + 'StackA', + 'abcdef:D3-publish', + 'StackB', + ]); + }); }); /** @@ -251,3 +293,13 @@ function assertableNode(x: A) { dependencies: Array.from(x.dependencies), }; } + +async function traverseAndRecord(graph: WorkGraph) { + const ret: string[] = []; + await graph.doParallel(1, { + deployStack: async (node) => { ret.push(node.id); }, + buildAsset: async (node) => { ret.push(node.id); }, + publishAsset: async (node) => { ret.push(node.id); }, + }); + return ret; +} \ No newline at end of file diff --git a/packages/cdk-assets/lib/asset-manifest.ts b/packages/cdk-assets/lib/asset-manifest.ts index 961200e024a18..857bbbc8b0144 100644 --- a/packages/cdk-assets/lib/asset-manifest.ts +++ b/packages/cdk-assets/lib/asset-manifest.ts @@ -106,7 +106,10 @@ export class AssetManifest { } /** - * List of assets, splat out to destinations + * List of assets per destination + * + * Returns one asset for every publishable destination. Multiple asset + * destinations may share the same asset source. */ public get entries(): IManifestEntry[] { return [ @@ -145,7 +148,7 @@ const ASSET_TYPES: AssetType[] = ['files', 'dockerImages']; */ export interface IManifestEntry { /** - * The identifier of the asset + * The identifier of the asset and its destination */ readonly id: DestinationIdentifier; @@ -209,10 +212,16 @@ export class DockerImageManifestEntry implements IManifestEntry { /** * Identify an asset destination in an asset manifest + * + * When stringified, this will be a combination of the source + * and destination IDs. */ export class DestinationIdentifier { /** * Identifies the asset, by source. + * + * The assetId will be the same between assets that represent + * the same physical file or image. */ public readonly assetId: string;