From b06a38ffca7faeebd994a31bd36e079478b67da5 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Tue, 29 Aug 2023 15:59:51 +0200 Subject: [PATCH] fix(cli): asset not uploaded with different synthesizer configs (#26910) If the same asset is used in 2 stacks that use different synthesizer configurations for publishing (for example, by using a different prefix) the asset will only be uploaded once instead of twice. We used to make the assumption that it was okay to use the destination ID as token of uniqueness. This is true inside a single manifest, but does not hold when there is more than stack that each have a manifest: both may have the destination ID `current_account:current_region`, but have different parameters for each destination. Instead, we calculate a content hash over the destination definition itself. That way, if the definitions are different we will create different nodes for each of them. Fixes #25927. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/aws-cdk/lib/util/content-hash.ts | 39 +++++++ .../aws-cdk/lib/util/work-graph-builder.ts | 53 +++++---- packages/aws-cdk/lib/util/work-graph-types.ts | 2 + packages/aws-cdk/lib/util/work-graph.ts | 19 +++- .../aws-cdk/test/work-graph-builder.test.ts | 105 +++++++++++++++--- 5 files changed, 169 insertions(+), 49 deletions(-) diff --git a/packages/aws-cdk/lib/util/content-hash.ts b/packages/aws-cdk/lib/util/content-hash.ts index aa78d55dd688d..fefe2dfb2ee1c 100644 --- a/packages/aws-cdk/lib/util/content-hash.ts +++ b/packages/aws-cdk/lib/util/content-hash.ts @@ -2,4 +2,43 @@ import * as crypto from 'crypto'; export function contentHash(data: string | Buffer | DataView) { return crypto.createHash('sha256').update(data).digest('hex'); +} + +/** + * A stably sorted hash of an arbitrary JS object + */ +export function contentHashAny(value: unknown) { + const ret = crypto.createHash('sha256'); + recurse(value); + return ret.digest('hex'); + + function recurse(x: unknown) { + if (typeof x === 'string') { + ret.update(x); + return; + } + + if (Array.isArray(x)) { + ret.update('['); + for (const e of x) { + recurse(e); + ret.update('||'); + } + ret.update(']'); + return; + } + + if (x && typeof x === 'object') { + ret.update('{'); + for (const key of Object.keys(x).sort()) { + ret.update(key); + ret.update(':'); + recurse((x as any)[key]); + } + ret.update('}'); + return; + } + + ret.update(`${x}${typeof x}`); // typeof to make sure hash('123') !== hash(123) + } } \ No newline at end of file diff --git a/packages/aws-cdk/lib/util/work-graph-builder.ts b/packages/aws-cdk/lib/util/work-graph-builder.ts index 4da85242d4c20..1c61d307014a8 100644 --- a/packages/aws-cdk/lib/util/work-graph-builder.ts +++ b/packages/aws-cdk/lib/util/work-graph-builder.ts @@ -1,5 +1,6 @@ import * as cxapi from '@aws-cdk/cx-api'; import { AssetManifest, IManifestEntry } from 'cdk-assets'; +import { contentHashAny } from './content-hash'; import { WorkGraph } from './work-graph'; import { DeploymentState, AssetBuildNode, WorkNode } from './work-graph-types'; @@ -26,7 +27,7 @@ export class WorkGraphBuilder { this.graph.addNodes({ type: 'stack', id: `${this.idPrefix}${artifact.id}`, - dependencies: new Set(this.getDepIds(onlyStacks(artifact.dependencies))), + dependencies: new Set(this.stackArtifactIds(onlyStacks(artifact.dependencies))), stack: artifact, deploymentState: DeploymentState.PENDING, priority: WorkGraphBuilder.PRIORITIES.stack, @@ -37,27 +38,26 @@ export class WorkGraphBuilder { * Oof, see this parameter list */ // eslint-disable-next-line max-len - private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) { + private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetManifestArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) { // Just the artifact identifier const assetId = asset.id.assetId; - // Unique per destination where the artifact needs to go - const assetDestinationId = `${asset.id}`; - const buildId = `${this.idPrefix}${assetId}-build`; - const publishNodeId = `${this.idPrefix}${assetDestinationId}-publish`; + const buildId = `build-${assetId}-${contentHashAny([assetId, asset.genericSource]).substring(0, 10)}`; + const publishId = `publish-${assetId}-${contentHashAny([assetId, asset.genericDestination]).substring(0, 10)}`; // 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, + note: assetId, dependencies: new Set([ - ...this.getDepIds(assetArtifact.dependencies), + ...this.stackArtifactIds(assetManifestArtifact.dependencies), // If we disable prebuild, then assets inherit (stack) dependencies from their parent stack - ...!this.prebuildAssets ? this.getDepIds(onlyStacks(parentStack.dependencies)) : [], + ...!this.prebuildAssets ? this.stackArtifactIds(onlyStacks(parentStack.dependencies)) : [], ]), - parentStack, - assetManifestArtifact: assetArtifact, + parentStack: parentStack, + assetManifestArtifact, assetManifest, asset, deploymentState: DeploymentState.PENDING, @@ -66,16 +66,17 @@ export class WorkGraphBuilder { this.graph.addNodes(node); } - const publishNode = this.graph.tryGetNode(publishNodeId); + const publishNode = this.graph.tryGetNode(publishId); if (!publishNode) { this.graph.addNodes({ type: 'asset-publish', - id: publishNodeId, + id: publishId, + note: `${asset.id}`, dependencies: new Set([ buildId, ]), parentStack, - assetManifestArtifact: assetArtifact, + assetManifestArtifact, assetManifest, asset, deploymentState: DeploymentState.PENDING, @@ -83,7 +84,7 @@ export class WorkGraphBuilder { }); } - for (const inheritedDep of this.getDepIds(onlyStacks(parentStack.dependencies))) { + for (const inheritedDep of this.stackArtifactIds(onlyStacks(parentStack.dependencies))) { // The asset publish step also depends on the stacks that the parent depends on. // This is purely cosmetic: if we don't do this, the progress printing of asset publishing // is going to interfere with the progress bar of the stack deployment. We could remove this @@ -91,11 +92,11 @@ export class WorkGraphBuilder { // Note: this may introduce a cycle if one of the parent's dependencies is another stack that // depends on this asset. To workaround this we remove these cycles once all nodes have // been added to the graph. - this.graph.addDependency(publishNodeId, inheritedDep); + this.graph.addDependency(publishId, inheritedDep); } // This will work whether the stack node has been added yet or not - this.graph.addDependency(`${this.idPrefix}${parentStack.id}`, publishNodeId); + this.graph.addDependency(`${this.idPrefix}${parentStack.id}`, publishId); } public build(artifacts: cxapi.CloudArtifact[]): WorkGraph { @@ -131,17 +132,15 @@ export class WorkGraphBuilder { return this.graph; } - private getDepIds(deps: cxapi.CloudArtifact[]): string[] { - const ids = []; - for (const artifact of deps) { - if (cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) { - // Depend on only the publish step. The publish step will depend on the build step on its own. - ids.push(`${this.idPrefix}${artifact.id}-publish`); - } else { - ids.push(`${this.idPrefix}${artifact.id}`); - } + private stackArtifactIds(deps: cxapi.CloudArtifact[]): string[] { + return deps.flatMap((d) => cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact(d) ? [this.stackArtifactId(d)] : []); + } + + private stackArtifactId(artifact: cxapi.CloudArtifact): string { + if (!cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact(artifact)) { + throw new Error(`Can only call this on CloudFormationStackArtifact, got: ${artifact.constructor.name}`); } - return ids; + return `${this.idPrefix}${artifact.id}`; } /** @@ -174,4 +173,4 @@ function stacksFromAssets(artifacts: cxapi.CloudArtifact[]) { function onlyStacks(artifacts: cxapi.CloudArtifact[]) { return artifacts.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact); -} \ No newline at end of file +} diff --git a/packages/aws-cdk/lib/util/work-graph-types.ts b/packages/aws-cdk/lib/util/work-graph-types.ts index 9da5c59d85ec0..4a20295bf5cc1 100644 --- a/packages/aws-cdk/lib/util/work-graph-types.ts +++ b/packages/aws-cdk/lib/util/work-graph-types.ts @@ -16,6 +16,8 @@ export interface WorkNodeCommon { readonly id: string; readonly dependencies: Set; deploymentState: DeploymentState; + /** Some readable information to attach to the id, which may be unreadable */ + readonly note?: string; } export interface StackNode extends WorkNodeCommon { diff --git a/packages/aws-cdk/lib/util/work-graph.ts b/packages/aws-cdk/lib/util/work-graph.ts index a06a969d5f0dd..e0e3336f34d25 100644 --- a/packages/aws-cdk/lib/util/work-graph.ts +++ b/packages/aws-cdk/lib/util/work-graph.ts @@ -217,19 +217,16 @@ export class WorkGraph { function renderNode(id: string, node: WorkNode): string[] { const ret = []; if (node.deploymentState === DeploymentState.COMPLETED) { - ret.push(` "${simplifyId(id)}" [style=filled,fillcolor=yellow];`); + ret.push(` ${gv(id, { style: 'filled', fillcolor: 'yellow', comment: node.note })};`); } else { - ret.push(` "${simplifyId(id)}";`); + ret.push(` ${gv(id, { comment: node.note })};`); } for (const dep of node.dependencies) { - ret.push(` "${simplifyId(id)}" -> "${simplifyId(dep)}";`); + ret.push(` ${gv(id)} -> ${gv(dep)};`); } return ret; } - function simplifyId(id: string) { - return id.replace(/([0-9a-f]{6})[0-9a-f]{6,}/g, '$1'); - } } /** @@ -392,3 +389,13 @@ function sum(xs: number[]) { function retainOnly(xs: A[], pred: (x: A) => boolean) { xs.splice(0, xs.length, ...xs.filter(pred)); } + +function gv(id: string, attrs?: Record) { + const attrString = Object.entries(attrs ?? {}).flatMap(([k, v]) => v !== undefined ? [`${k}="${v}"`] : []).join(','); + + return attrString ? `"${simplifyId(id)}" [${attrString}]` : `"${simplifyId(id)}"`; +} + +function simplifyId(id: string) { + return id.replace(/([0-9a-f]{6})[0-9a-f]{6,}/g, '$1'); +} \ No newline at end of file diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index 686affae6bfb6..a8457bc48f639 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/packages/aws-cdk/test/work-graph-builder.test.ts @@ -3,6 +3,8 @@ 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'; +// eslint-disable-next-line import/no-extraneous-dependencies +import { expect } from '@jest/globals'; 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'; @@ -16,6 +18,25 @@ afterEach(() => { rootBuilder.delete(); }); +function superset(xs: A[]): Set { + const ret = new Set(xs); + (ret as any).isSuperset = true; + return ret; +} + +expect.addEqualityTesters([ + function(exp: unknown, act: unknown): boolean | undefined { + if (exp instanceof Set && isIterable(act)) { + if ((exp as any).isSuperset) { + const actSet = new Set(act); + return Array.from(exp as any).every((x) => actSet.has(x)); + } + return this.equals(Array.from(exp).sort(), Array.from(act).sort()); + } + return undefined; + }, +]); + describe('with some stacks and assets', () => { let assembly: cxapi.CloudAssembly; beforeEach(() => { @@ -28,34 +49,34 @@ describe('with some stacks and assets', () => { expect(assertableNode(graph.node('stack2'))).toEqual(expect.objectContaining({ type: 'stack', - dependencies: expect.arrayContaining(['F1:D1-publish']), - } as StackNode)); + dependencies: superset(['publish-F1-add54bdbcb']), + } as Partial)); }); test('asset publishing step depends on asset building step', () => { const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - expect(graph.node('F1:D1-publish')).toEqual(expect.objectContaining({ + expect(graph.node('publish-F1-add54bdbcb')).toEqual(expect.objectContaining({ type: 'asset-publish', - dependencies: new Set(['F1-build']), - } as Partial)); + dependencies: superset(['build-F1-a533139934']), + } satisfies 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-build')).toEqual(expect.objectContaining({ + expect(graph.node('build-F1-a533139934')).toEqual(expect.objectContaining({ type: 'asset-build', - dependencies: new Set(['stack0', 'stack1']), + dependencies: superset(['stack0', 'stack1']), } as Partial)); }); test('with prebuild on, assets only have their own dependencies', () => { const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - expect(graph.node('F1-build')).toEqual(expect.objectContaining({ + expect(graph.node('build-F1-a533139934')).toEqual(expect.objectContaining({ type: 'asset-build', - dependencies: new Set(['stack0']), + dependencies: superset(['stack0']), } as Partial)); }); }); @@ -84,13 +105,16 @@ test('can handle nested assemblies', async () => { let workDone = 0; const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + await graph.doParallel(10, { deployStack: async () => { workDone += 1; }, buildAsset: async () => { }, publishAsset: async () => { workDone += 1; }, }); - expect(workDone).toEqual(8); + // The asset is shared between parent assembly and nested assembly, but the stacks will be deployed + // 3 stacks + 1 asset + 3 stacks (1 reused asset) + expect(workDone).toEqual(7); }); test('dependencies on unselected artifacts are silently ignored', async () => { @@ -143,8 +167,8 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'work-graph-builder.test.js-build', - 'work-graph-builder.test.js:D1-publish', + expect.stringMatching(/^build-work-graph-builder.test.js-.*$/), + expect.stringMatching(/^publish-work-graph-builder.test.js-.*$/), 'StackA', 'StackB', ]); @@ -205,11 +229,56 @@ describe('tests that use assets', () => { const traversal = await traverseAndRecord(graph); expect(traversal).toEqual([ - 'abcdef-build', - 'abcdef:D1-publish', - 'abcdef:D2-publish', + expect.stringMatching(/^build-abcdef-.*$/), + expect.stringMatching(/^publish-abcdef-.*$/), + expect.stringMatching(/^publish-abcdef-.*$/), + 'StackA', + expect.stringMatching(/^publish-abcdef-.*$/), + 'StackB', + ]); + }); + + test('different parameters for the same named definition are both published', async () => { + addStack(rootBuilder, 'StackA', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackA.assets'], + }); + addAssets(rootBuilder, 'StackA.assets', { + files: { + abcdef: { + source: { path: __dirname }, + destinations: { + D: { bucketName: 'bucket1', 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: { + D: { bucketName: 'bucket2', objectKey: 'key' }, + }, + }, + }, + }); + + const assembly = rootBuilder.buildAssembly(); + + const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + const traversal = await traverseAndRecord(graph); + + expect(traversal).toEqual([ + expect.stringMatching(/^build-abcdef-.*$/), + expect.stringMatching(/^publish-abcdef-.*$/), 'StackA', - 'abcdef:D3-publish', + expect.stringMatching(/^publish-abcdef-.*$/), 'StackB', ]); }); @@ -302,4 +371,8 @@ async function traverseAndRecord(graph: WorkGraph) { publishAsset: async (node) => { ret.push(node.id); }, }); return ret; +} + +function isIterable(x: unknown): x is Iterable { + return x && typeof x === 'object' && (x as any)[Symbol.iterator]; } \ No newline at end of file