Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(assertions): throw error or warn when synth is called multiple times on mutated construct tree #31865

Merged
merged 25 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c4e1795
WIP - add unit test to synthesis.test.ts
sumupitchayan Oct 23, 2024
fd13c07
remove assembly caching from stage.ts synth() function
sumupitchayan Oct 23, 2024
f780751
fix import order in synthesis.test.ts
sumupitchayan Oct 23, 2024
299733d
WIP - try different approach, comparing construct path sets
sumupitchayan Oct 25, 2024
31afea1
fix linter errors
sumupitchayan Oct 25, 2024
e9cbc48
WIP - cache after running synth
sumupitchayan Oct 28, 2024
1c5cff1
fix pipelines test (environment variables specified in multiple place…
sumupitchayan Oct 28, 2024
c111479
WIP - update unit test for this issue
sumupitchayan Oct 28, 2024
f7e70bb
WIP - improve error/warning messages
sumupitchayan Oct 28, 2024
8e6a194
typo
sumupitchayan Oct 28, 2024
8798232
Merge branch 'main' into sumughan/fix-assertions-unable-to-find-artifact
sumupitchayan Oct 28, 2024
de57d44
Update packages/aws-cdk-lib/core/lib/stage.ts
sumupitchayan Oct 28, 2024
af20ae7
rename warnInsteadOfError to errorOnDuplicateSynth
sumupitchayan Oct 28, 2024
9a2e089
improve error message
sumupitchayan Oct 28, 2024
a78ab0e
make 2 helper functions top-level
sumupitchayan Oct 28, 2024
c7430d8
eslint comment to allow console.error statement
sumupitchayan Oct 28, 2024
ac524e1
Update packages/aws-cdk-lib/core/lib/stage.ts
sumupitchayan Oct 29, 2024
280e2e2
rename parameter in recurse funciton
sumupitchayan Oct 29, 2024
39cc2fd
WIP Momo feedback: improve err/warning messages
sumupitchayan Oct 29, 2024
c133601
edit test err msg assertion
sumupitchayan Oct 29, 2024
02f405c
WIP err msgs again
sumupitchayan Oct 29, 2024
b68519a
WIP err msgs again x2
sumupitchayan Oct 29, 2024
e8b866b
WIP err msgs 3x
sumupitchayan Oct 29, 2024
6f098be
remove force option from error throwing in synth
sumupitchayan Oct 29, 2024
4fec523
Merge branch 'main' into sumughan/fix-assertions-unable-to-find-artifact
sumupitchayan Oct 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/core/lib/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export class App extends Stage {
if (autoSynth) {
// synth() guarantees it will only execute once, so a default of 'true'
// doesn't bite manual calling of the function.
process.once('beforeExit', () => this.synth());
process.once('beforeExit', () => this.synth({ errorOnDuplicateSynth: false }));
}

this._treeMetadata = props.treeMetadata ?? true;
Expand Down
61 changes: 60 additions & 1 deletion packages/aws-cdk-lib/core/lib/stage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,11 @@ export class Stage extends Construct {
*/
private assembly?: cxapi.CloudAssembly;

/**
* The cached set of construct paths. Empty if assembly was not yet built.
*/
private constructPathsCache: Set<string>;

/**
* Validation plugins to run during synthesis. If any plugin reports any violation,
* synthesis will be interrupted and the report displayed to the user.
Expand All @@ -163,6 +168,7 @@ export class Stage extends Construct {

Object.defineProperty(this, STAGE_SYMBOL, { value: true });

this.constructPathsCache = new Set<string>();
this.parentStage = Stage.of(this);

this.region = props.env?.region ?? this.parentStage?.region;
Expand Down Expand Up @@ -210,16 +216,62 @@ export class Stage extends Construct {
* calls will return the same assembly.
*/
public synth(options: StageSynthesisOptions = { }): cxapi.CloudAssembly {
if (!this.assembly || options.force) {

let newConstructPaths = this.listAllConstructPaths(this);

// If the assembly cache is uninitiazed, run synthesize and reset construct paths cache
if (this.constructPathsCache.size == 0 || !this.assembly || options.force) {
this.assembly = synthesize(this, {
skipValidation: options.skipValidation,
validateOnSynthesis: options.validateOnSynthesis,
});
newConstructPaths = this.listAllConstructPaths(this);
this.constructPathsCache = newConstructPaths;
}

// If the construct paths set has changed
if (!this.constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) {
const errorMessage = 'The construct tree has been modified after synthesis. Only the results of the first synth() call are written to disk, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called.';
sumupitchayan marked this conversation as resolved.
Show resolved Hide resolved
if (options.errorOnDuplicateSynth ?? true) {
throw new Error(errorMessage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we can really use the exact same message. the call to action is different when there is an error vs a warning. In my head, the error scenario is this:

  • Hey we have detected that you called synth() multiple times and changed the construct tree after the first synth()
  • You cannot do this, we are failing the execution.
  • If you really know what you are doing, you can set this flag to keep going:

} else {
// eslint-disable-next-line no-console
console.error(errorMessage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we print this as a warning. And now what? The message text implies that we are in the warning situation and is trying to say the following:

  • Hey we have detected that you called synth() multiple and changed the construct tree after the first synth()
  • We are going to ignoring all changes, but are not failing the execution.
  • You should stop doing this, unless you really know what you do.

}
}

// Reset construct paths cache
this.constructPathsCache = newConstructPaths;

return this.assembly;
}

// Function that lists all construct paths and returns them as a set
private listAllConstructPaths(construct: IConstruct): Set<string> {
const paths = new Set<string>();
function recurse(construct: IConstruct) {
paths.add(construct.node.path);
for (const child of construct.node.children) {
if (!Stage.isStage(child)) {
recurse(child);
}
}
}
recurse(construct);
return paths;
}

// Checks if sets of construct paths are equal
private constructPathSetsAreEqual(set1: Set<string>, set2: Set<string>): boolean {
if (set1.size !== set2.size) return false;
for (const id of set1) {
if (!set2.has(id)) {
return false;
}
}
return true;
}

private createBuilder(outdir?: string) {
// cannot specify "outdir" if we are a nested stage
if (this.parentStage && outdir) {
Expand Down Expand Up @@ -259,4 +311,11 @@ export interface StageSynthesisOptions {
* @default false
*/
readonly force?: boolean;

/**
* Whether or not to throw a warning instead of an error if the construct tree has
* been mutated since the last synth.
* @default true
*/
readonly errorOnDuplicateSynth?: boolean;
}
25 changes: 25 additions & 0 deletions packages/aws-cdk-lib/core/test/synthesis.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as os from 'os';
import * as path from 'path';
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
import { Construct } from 'constructs';
import { Template } from '../../assertions';
import * as cxschema from '../../cloud-assembly-schema';
import * as cxapi from '../../cx-api';
import * as cdk from '../lib';
Expand Down Expand Up @@ -362,6 +363,30 @@ describe('synthesis', () => {

});

test('calling synth multiple times errors if construct tree is mutated', () => {
const app = new cdk.App();

const stages = [
{
stage: 'PROD',
},
{
stage: 'BETA',
},
];

// THEN - no error the first time synth is called
let stack = new cdk.Stack(app, `${stages[0].stage}-Stack`, {});
expect(() => {
Template.fromStack(stack);
}).not.toThrow();

// THEN - error is thrown since synth was called with mutated stack name
stack = new cdk.Stack(app, `${stages[1].stage}-Stack`, {});
expect(() => {
Template.fromStack(stack);
}).toThrow('The construct tree has been modified after synthesis.');
});
});

function list(outdir: string) {
Expand Down
42 changes: 21 additions & 21 deletions packages/aws-cdk-lib/pipelines/test/compliance/synths.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,27 @@ test('CodeBuild: environment variables specified in multiple places are correctl
}),
});

new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk-2', {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This existing pipelines test had to be changed to not throw an error - all I did was move the stack mutations before the Template.fromStack() calls

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's perfect!

synth: new cdkp.CodeBuildStep('Synth', {
input: cdkp.CodePipelineSource.gitHub('test/test', 'main'),
primaryOutputDirectory: '.',
env: {
SOME_ENV_VAR: 'SomeValue',
},
installCommands: [
'install1',
'install2',
],
commands: ['synth'],
buildEnvironment: {
environmentVariables: {
INNER_VAR: { value: 'InnerValue' },
},
privileged: true,
},
}),
});

// THEN
Template.fromStack(pipelineStack).hasResourceProperties('AWS::CodeBuild::Project', {
Environment: Match.objectLike({
Expand Down Expand Up @@ -217,27 +238,6 @@ test('CodeBuild: environment variables specified in multiple places are correctl
},
});

new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk-2', {
synth: new cdkp.CodeBuildStep('Synth', {
input: cdkp.CodePipelineSource.gitHub('test/test', 'main'),
primaryOutputDirectory: '.',
env: {
SOME_ENV_VAR: 'SomeValue',
},
installCommands: [
'install1',
'install2',
],
commands: ['synth'],
buildEnvironment: {
environmentVariables: {
INNER_VAR: { value: 'InnerValue' },
},
privileged: true,
},
}),
});

// THEN
Template.fromStack(pipelineStack).hasResourceProperties('AWS::CodeBuild::Project', {
Environment: Match.objectLike({
Expand Down