Skip to content

Commit

Permalink
fix(cli): asset not uploaded with different synthesizer configs (#26910)
Browse files Browse the repository at this point in the history 
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*
  • Loading branch information
@rix0rrr
rix0rrr authored Aug 29, 2023
1 parent 7f31da8 commit b06a38f
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 49 deletions.
39 changes: 39 additions & 0 deletions packages/aws-cdk/lib/util/content-hash.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
53 changes: 26 additions & 27 deletions packages/aws-cdk/lib/util/work-graph-builder.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -66,36 +66,37 @@ 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,
priority: WorkGraphBuilder.PRIORITIES['asset-publish'],
});
}

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
// for overall faster deployments if we ever have a better method of progress displaying.
// 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 {
Expand Down Expand Up @@ -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}`;
}

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

function onlyStacks(artifacts: cxapi.CloudArtifact[]) {
return artifacts.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact);
}
}
2 changes: 2 additions & 0 deletions packages/aws-cdk/lib/util/work-graph-types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ export interface WorkNodeCommon {
readonly id: string;
readonly dependencies: Set<string>;
deploymentState: DeploymentState;
/** Some readable information to attach to the id, which may be unreadable */
readonly note?: string;
}

export interface StackNode extends WorkNodeCommon {
Expand Down
19 changes: 13 additions & 6 deletions packages/aws-cdk/lib/util/work-graph.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
}

/**
Expand Down Expand Up @@ -392,3 +389,13 @@ function sum(xs: number[]) {
function retainOnly<A>(xs: A[], pred: (x: A) => boolean) {
xs.splice(0, xs.length, ...xs.filter(pred));
}

function gv(id: string, attrs?: Record<string, string | undefined>) {
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');
}
105 changes: 89 additions & 16 deletions packages/aws-cdk/test/work-graph-builder.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -16,6 +18,25 @@ afterEach(() => {
rootBuilder.delete();
});

function superset<A>(xs: A[]): Set<A> {
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(() => {
Expand All @@ -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<StackNode>));
});

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<AssetPublishNode>));
dependencies: superset(['build-F1-a533139934']),
} satisfies Partial<AssetPublishNode>));
});

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<AssetBuildNode>));
});

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<AssetBuildNode>));
});
});
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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',
]);
Expand Down Expand Up @@ -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',
]);
});
Expand Down Expand Up @@ -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<any> {
return x && typeof x === 'object' && (x as any)[Symbol.iterator];
}

0 comments on commit b06a38f

Please sign in to comment.