Skip to content

Commit b06a38f

Browse files
authored
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*
1 parent 7f31da8 commit b06a38f

File tree

5 files changed

+169
-49
lines changed

5 files changed

+169
-49
lines changed

packages/aws-cdk/lib/util/content-hash.ts

+39
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,43 @@ import * as crypto from 'crypto';
22

33
export function contentHash(data: string | Buffer | DataView) {
44
return crypto.createHash('sha256').update(data).digest('hex');
5+
}
6+
7+
/**
8+
* A stably sorted hash of an arbitrary JS object
9+
*/
10+
export function contentHashAny(value: unknown) {
11+
const ret = crypto.createHash('sha256');
12+
recurse(value);
13+
return ret.digest('hex');
14+
15+
function recurse(x: unknown) {
16+
if (typeof x === 'string') {
17+
ret.update(x);
18+
return;
19+
}
20+
21+
if (Array.isArray(x)) {
22+
ret.update('[');
23+
for (const e of x) {
24+
recurse(e);
25+
ret.update('||');
26+
}
27+
ret.update(']');
28+
return;
29+
}
30+
31+
if (x && typeof x === 'object') {
32+
ret.update('{');
33+
for (const key of Object.keys(x).sort()) {
34+
ret.update(key);
35+
ret.update(':');
36+
recurse((x as any)[key]);
37+
}
38+
ret.update('}');
39+
return;
40+
}
41+
42+
ret.update(`${x}${typeof x}`); // typeof to make sure hash('123') !== hash(123)
43+
}
544
}

packages/aws-cdk/lib/util/work-graph-builder.ts

+26-27
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import * as cxapi from '@aws-cdk/cx-api';
22
import { AssetManifest, IManifestEntry } from 'cdk-assets';
3+
import { contentHashAny } from './content-hash';
34
import { WorkGraph } from './work-graph';
45
import { DeploymentState, AssetBuildNode, WorkNode } from './work-graph-types';
56

@@ -26,7 +27,7 @@ export class WorkGraphBuilder {
2627
this.graph.addNodes({
2728
type: 'stack',
2829
id: `${this.idPrefix}${artifact.id}`,
29-
dependencies: new Set(this.getDepIds(onlyStacks(artifact.dependencies))),
30+
dependencies: new Set(this.stackArtifactIds(onlyStacks(artifact.dependencies))),
3031
stack: artifact,
3132
deploymentState: DeploymentState.PENDING,
3233
priority: WorkGraphBuilder.PRIORITIES.stack,
@@ -37,27 +38,26 @@ export class WorkGraphBuilder {
3738
* Oof, see this parameter list
3839
*/
3940
// eslint-disable-next-line max-len
40-
private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) {
41+
private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetManifestArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) {
4142
// Just the artifact identifier
4243
const assetId = asset.id.assetId;
43-
// Unique per destination where the artifact needs to go
44-
const assetDestinationId = `${asset.id}`;
4544

46-
const buildId = `${this.idPrefix}${assetId}-build`;
47-
const publishNodeId = `${this.idPrefix}${assetDestinationId}-publish`;
45+
const buildId = `build-${assetId}-${contentHashAny([assetId, asset.genericSource]).substring(0, 10)}`;
46+
const publishId = `publish-${assetId}-${contentHashAny([assetId, asset.genericDestination]).substring(0, 10)}`;
4847

4948
// Build node only gets added once because they are all the same
5049
if (!this.graph.tryGetNode(buildId)) {
5150
const node: AssetBuildNode = {
5251
type: 'asset-build',
5352
id: buildId,
53+
note: assetId,
5454
dependencies: new Set([
55-
...this.getDepIds(assetArtifact.dependencies),
55+
...this.stackArtifactIds(assetManifestArtifact.dependencies),
5656
// If we disable prebuild, then assets inherit (stack) dependencies from their parent stack
57-
...!this.prebuildAssets ? this.getDepIds(onlyStacks(parentStack.dependencies)) : [],
57+
...!this.prebuildAssets ? this.stackArtifactIds(onlyStacks(parentStack.dependencies)) : [],
5858
]),
59-
parentStack,
60-
assetManifestArtifact: assetArtifact,
59+
parentStack: parentStack,
60+
assetManifestArtifact,
6161
assetManifest,
6262
asset,
6363
deploymentState: DeploymentState.PENDING,
@@ -66,36 +66,37 @@ export class WorkGraphBuilder {
6666
this.graph.addNodes(node);
6767
}
6868

69-
const publishNode = this.graph.tryGetNode(publishNodeId);
69+
const publishNode = this.graph.tryGetNode(publishId);
7070
if (!publishNode) {
7171
this.graph.addNodes({
7272
type: 'asset-publish',
73-
id: publishNodeId,
73+
id: publishId,
74+
note: `${asset.id}`,
7475
dependencies: new Set([
7576
buildId,
7677
]),
7778
parentStack,
78-
assetManifestArtifact: assetArtifact,
79+
assetManifestArtifact,
7980
assetManifest,
8081
asset,
8182
deploymentState: DeploymentState.PENDING,
8283
priority: WorkGraphBuilder.PRIORITIES['asset-publish'],
8384
});
8485
}
8586

86-
for (const inheritedDep of this.getDepIds(onlyStacks(parentStack.dependencies))) {
87+
for (const inheritedDep of this.stackArtifactIds(onlyStacks(parentStack.dependencies))) {
8788
// The asset publish step also depends on the stacks that the parent depends on.
8889
// This is purely cosmetic: if we don't do this, the progress printing of asset publishing
8990
// is going to interfere with the progress bar of the stack deployment. We could remove this
9091
// for overall faster deployments if we ever have a better method of progress displaying.
9192
// Note: this may introduce a cycle if one of the parent's dependencies is another stack that
9293
// depends on this asset. To workaround this we remove these cycles once all nodes have
9394
// been added to the graph.
94-
this.graph.addDependency(publishNodeId, inheritedDep);
95+
this.graph.addDependency(publishId, inheritedDep);
9596
}
9697

9798
// This will work whether the stack node has been added yet or not
98-
this.graph.addDependency(`${this.idPrefix}${parentStack.id}`, publishNodeId);
99+
this.graph.addDependency(`${this.idPrefix}${parentStack.id}`, publishId);
99100
}
100101

101102
public build(artifacts: cxapi.CloudArtifact[]): WorkGraph {
@@ -131,17 +132,15 @@ export class WorkGraphBuilder {
131132
return this.graph;
132133
}
133134

134-
private getDepIds(deps: cxapi.CloudArtifact[]): string[] {
135-
const ids = [];
136-
for (const artifact of deps) {
137-
if (cxapi.AssetManifestArtifact.isAssetManifestArtifact(artifact)) {
138-
// Depend on only the publish step. The publish step will depend on the build step on its own.
139-
ids.push(`${this.idPrefix}${artifact.id}-publish`);
140-
} else {
141-
ids.push(`${this.idPrefix}${artifact.id}`);
142-
}
135+
private stackArtifactIds(deps: cxapi.CloudArtifact[]): string[] {
136+
return deps.flatMap((d) => cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact(d) ? [this.stackArtifactId(d)] : []);
137+
}
138+
139+
private stackArtifactId(artifact: cxapi.CloudArtifact): string {
140+
if (!cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact(artifact)) {
141+
throw new Error(`Can only call this on CloudFormationStackArtifact, got: ${artifact.constructor.name}`);
143142
}
144-
return ids;
143+
return `${this.idPrefix}${artifact.id}`;
145144
}
146145

147146
/**
@@ -174,4 +173,4 @@ function stacksFromAssets(artifacts: cxapi.CloudArtifact[]) {
174173

175174
function onlyStacks(artifacts: cxapi.CloudArtifact[]) {
176175
return artifacts.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact);
177-
}
176+
}

packages/aws-cdk/lib/util/work-graph-types.ts

+2
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ export interface WorkNodeCommon {
1616
readonly id: string;
1717
readonly dependencies: Set<string>;
1818
deploymentState: DeploymentState;
19+
/** Some readable information to attach to the id, which may be unreadable */
20+
readonly note?: string;
1921
}
2022

2123
export interface StackNode extends WorkNodeCommon {

packages/aws-cdk/lib/util/work-graph.ts

+13-6
Original file line numberDiff line numberDiff line change
@@ -217,19 +217,16 @@ export class WorkGraph {
217217
function renderNode(id: string, node: WorkNode): string[] {
218218
const ret = [];
219219
if (node.deploymentState === DeploymentState.COMPLETED) {
220-
ret.push(` "${simplifyId(id)}" [style=filled,fillcolor=yellow];`);
220+
ret.push(` ${gv(id, { style: 'filled', fillcolor: 'yellow', comment: node.note })};`);
221221
} else {
222-
ret.push(` "${simplifyId(id)}";`);
222+
ret.push(` ${gv(id, { comment: node.note })};`);
223223
}
224224
for (const dep of node.dependencies) {
225-
ret.push(` "${simplifyId(id)}" -> "${simplifyId(dep)}";`);
225+
ret.push(` ${gv(id)} -> ${gv(dep)};`);
226226
}
227227
return ret;
228228
}
229229

230-
function simplifyId(id: string) {
231-
return id.replace(/([0-9a-f]{6})[0-9a-f]{6,}/g, '$1');
232-
}
233230
}
234231

235232
/**
@@ -392,3 +389,13 @@ function sum(xs: number[]) {
392389
function retainOnly<A>(xs: A[], pred: (x: A) => boolean) {
393390
xs.splice(0, xs.length, ...xs.filter(pred));
394391
}
392+
393+
function gv(id: string, attrs?: Record<string, string | undefined>) {
394+
const attrString = Object.entries(attrs ?? {}).flatMap(([k, v]) => v !== undefined ? [`${k}="${v}"`] : []).join(',');
395+
396+
return attrString ? `"${simplifyId(id)}" [${attrString}]` : `"${simplifyId(id)}"`;
397+
}
398+
399+
function simplifyId(id: string) {
400+
return id.replace(/([0-9a-f]{6})[0-9a-f]{6,}/g, '$1');
401+
}

packages/aws-cdk/test/work-graph-builder.test.ts

+89-16
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ import * as path from 'path';
33
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
44
import * as cxapi from '@aws-cdk/cx-api';
55
import { CloudAssemblyBuilder } from '@aws-cdk/cx-api';
6+
// eslint-disable-next-line import/no-extraneous-dependencies
7+
import { expect } from '@jest/globals';
68
import { WorkGraph } from '../lib/util/work-graph';
79
import { WorkGraphBuilder } from '../lib/util/work-graph-builder';
810
import { AssetBuildNode, AssetPublishNode, StackNode, WorkNode } from '../lib/util/work-graph-types';
@@ -16,6 +18,25 @@ afterEach(() => {
1618
rootBuilder.delete();
1719
});
1820

21+
function superset<A>(xs: A[]): Set<A> {
22+
const ret = new Set(xs);
23+
(ret as any).isSuperset = true;
24+
return ret;
25+
}
26+
27+
expect.addEqualityTesters([
28+
function(exp: unknown, act: unknown): boolean | undefined {
29+
if (exp instanceof Set && isIterable(act)) {
30+
if ((exp as any).isSuperset) {
31+
const actSet = new Set(act);
32+
return Array.from(exp as any).every((x) => actSet.has(x));
33+
}
34+
return this.equals(Array.from(exp).sort(), Array.from(act).sort());
35+
}
36+
return undefined;
37+
},
38+
]);
39+
1940
describe('with some stacks and assets', () => {
2041
let assembly: cxapi.CloudAssembly;
2142
beforeEach(() => {
@@ -28,34 +49,34 @@ describe('with some stacks and assets', () => {
2849

2950
expect(assertableNode(graph.node('stack2'))).toEqual(expect.objectContaining({
3051
type: 'stack',
31-
dependencies: expect.arrayContaining(['F1:D1-publish']),
32-
} as StackNode));
52+
dependencies: superset(['publish-F1-add54bdbcb']),
53+
} as Partial<StackNode>));
3354
});
3455

3556
test('asset publishing step depends on asset building step', () => {
3657
const graph = new WorkGraphBuilder(true).build(assembly.artifacts);
3758

38-
expect(graph.node('F1:D1-publish')).toEqual(expect.objectContaining({
59+
expect(graph.node('publish-F1-add54bdbcb')).toEqual(expect.objectContaining({
3960
type: 'asset-publish',
40-
dependencies: new Set(['F1-build']),
41-
} as Partial<AssetPublishNode>));
61+
dependencies: superset(['build-F1-a533139934']),
62+
} satisfies Partial<AssetPublishNode>));
4263
});
4364

4465
test('with prebuild off, asset building inherits dependencies from their parent stack', () => {
4566
const graph = new WorkGraphBuilder(false).build(assembly.artifacts);
4667

47-
expect(graph.node('F1-build')).toEqual(expect.objectContaining({
68+
expect(graph.node('build-F1-a533139934')).toEqual(expect.objectContaining({
4869
type: 'asset-build',
49-
dependencies: new Set(['stack0', 'stack1']),
70+
dependencies: superset(['stack0', 'stack1']),
5071
} as Partial<AssetBuildNode>));
5172
});
5273

5374
test('with prebuild on, assets only have their own dependencies', () => {
5475
const graph = new WorkGraphBuilder(true).build(assembly.artifacts);
5576

56-
expect(graph.node('F1-build')).toEqual(expect.objectContaining({
77+
expect(graph.node('build-F1-a533139934')).toEqual(expect.objectContaining({
5778
type: 'asset-build',
58-
dependencies: new Set(['stack0']),
79+
dependencies: superset(['stack0']),
5980
} as Partial<AssetBuildNode>));
6081
});
6182
});
@@ -84,13 +105,16 @@ test('can handle nested assemblies', async () => {
84105

85106
let workDone = 0;
86107
const graph = new WorkGraphBuilder(true).build(assembly.artifacts);
108+
87109
await graph.doParallel(10, {
88110
deployStack: async () => { workDone += 1; },
89111
buildAsset: async () => { },
90112
publishAsset: async () => { workDone += 1; },
91113
});
92114

93-
expect(workDone).toEqual(8);
115+
// The asset is shared between parent assembly and nested assembly, but the stacks will be deployed
116+
// 3 stacks + 1 asset + 3 stacks (1 reused asset)
117+
expect(workDone).toEqual(7);
94118
});
95119

96120
test('dependencies on unselected artifacts are silently ignored', async () => {
@@ -143,8 +167,8 @@ describe('tests that use assets', () => {
143167
const traversal = await traverseAndRecord(graph);
144168

145169
expect(traversal).toEqual([
146-
'work-graph-builder.test.js-build',
147-
'work-graph-builder.test.js:D1-publish',
170+
expect.stringMatching(/^build-work-graph-builder.test.js-.*$/),
171+
expect.stringMatching(/^publish-work-graph-builder.test.js-.*$/),
148172
'StackA',
149173
'StackB',
150174
]);
@@ -205,11 +229,56 @@ describe('tests that use assets', () => {
205229
const traversal = await traverseAndRecord(graph);
206230

207231
expect(traversal).toEqual([
208-
'abcdef-build',
209-
'abcdef:D1-publish',
210-
'abcdef:D2-publish',
232+
expect.stringMatching(/^build-abcdef-.*$/),
233+
expect.stringMatching(/^publish-abcdef-.*$/),
234+
expect.stringMatching(/^publish-abcdef-.*$/),
235+
'StackA',
236+
expect.stringMatching(/^publish-abcdef-.*$/),
237+
'StackB',
238+
]);
239+
});
240+
241+
test('different parameters for the same named definition are both published', async () => {
242+
addStack(rootBuilder, 'StackA', {
243+
environment: 'aws://11111/us-east-1',
244+
dependencies: ['StackA.assets'],
245+
});
246+
addAssets(rootBuilder, 'StackA.assets', {
247+
files: {
248+
abcdef: {
249+
source: { path: __dirname },
250+
destinations: {
251+
D: { bucketName: 'bucket1', objectKey: 'key' },
252+
},
253+
},
254+
},
255+
});
256+
257+
addStack(rootBuilder, 'StackB', {
258+
environment: 'aws://11111/us-east-1',
259+
dependencies: ['StackB.assets', 'StackA'],
260+
});
261+
addAssets(rootBuilder, 'StackB.assets', {
262+
files: {
263+
abcdef: {
264+
source: { path: __dirname },
265+
destinations: {
266+
D: { bucketName: 'bucket2', objectKey: 'key' },
267+
},
268+
},
269+
},
270+
});
271+
272+
const assembly = rootBuilder.buildAssembly();
273+
274+
const graph = new WorkGraphBuilder(true).build(assembly.artifacts);
275+
const traversal = await traverseAndRecord(graph);
276+
277+
expect(traversal).toEqual([
278+
expect.stringMatching(/^build-abcdef-.*$/),
279+
expect.stringMatching(/^publish-abcdef-.*$/),
211280
'StackA',
212-
'abcdef:D3-publish',
281+
expect.stringMatching(/^publish-abcdef-.*$/),
213282
'StackB',
214283
]);
215284
});
@@ -302,4 +371,8 @@ async function traverseAndRecord(graph: WorkGraph) {
302371
publishAsset: async (node) => { ret.push(node.id); },
303372
});
304373
return ret;
374+
}
375+
376+
function isIterable(x: unknown): x is Iterable<any> {
377+
return x && typeof x === 'object' && (x as any)[Symbol.iterator];
305378
}

0 commit comments

Comments
 (0)