Skip to content

Commit

Permalink
feat(integ-runner): support snapshot diff on nested stacks (#22881)
Browse files Browse the repository at this point in the history 
Previously we did not run diff Nested Stack templates separately. Any change would have been detected by the changed asset hash in the `AWS::CloudFormation::Stack` resource. With this change, nested templates are diff'ed as part of the integ test and any errors will be printed.

The change allows us to then ignore the asset hash in the s3 url for nested stacks if `diffAsset: false` (the default) is set. Previously this was the only way to detect changes in a nested stack. However the hash would also change if an asset inside the nested stack has changed (which should be ignored). With the change to diff nested stack templates, asset hashes in nested stack template are now transparently ignored. 

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@mrgrain
mrgrain authored Nov 16, 2022
1 parent fc4668d commit 5b3d06d
Show file tree
Hide file tree
Showing 53 changed files with 2,972 additions and 381 deletions.
2 changes: 2 additions & 0 deletions packages/@aws-cdk/integ-runner/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,8 @@ Test Results:
Tests: 1 passed, 1 total
```

Nested stack templates are also compared as part of the snapshot. However asset hashes are ignored by default. To enable diff for asset hashes, set `diffAssets: true` of `IntegTestProps`.

#### Update Workflow

By default, integration tests are run with the "update workflow" enabled. This can be disabled by using the `--disable-update-workflow` command line option.
Expand Down
34 changes: 33 additions & 1 deletion packages/@aws-cdk/integ-runner/lib/runner/private/cloud-assembly.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,25 @@ export class AssemblyManifestReader {
return stacks;
}

/**
* Get the nested stacks for a given stack
* returns a map of artifactId to CloudFormation template
*/
public getNestedStacksForStack(stackId: string): Record<string, any> {
const nestedTemplates: string[] = this.getAssetManifestsForStack(stackId).flatMap(
manifest => manifest.files
.filter(asset => asset.source.path?.endsWith('.nested.template.json'))
.map(asset => asset.source.path!),
);

const nestedStacks: Record<string, any> = Object.fromEntries(nestedTemplates.map(templateFile => ([
templateFile.split('.', 1)[0],
fs.readJSONSync(path.resolve(this.directory, templateFile)),
])));

return nestedStacks;
}

/**
* Write trace data to the assembly manifest metadata
*/
Expand Down Expand Up @@ -125,6 +144,19 @@ export class AssemblyManifestReader {
return assets;
}

/**
* Return a list of asset artifacts for a given stack
*/
public getAssetManifestsForStack(stackId: string): AssetManifest[] {
return Object.values(this.manifest.artifacts ?? {})
.filter(artifact =>
artifact.type === ArtifactType.ASSET_MANIFEST && (artifact.properties as AssetManifestProperties)?.file === `${stackId}.assets.json`)
.map(artifact => {
const fileName = (artifact.properties as AssetManifestProperties).file;
return AssetManifest.fromFile(path.join(this.directory, fileName));
});
}

/**
* Get a list of assets from the assembly manifest
*/
Expand Down Expand Up @@ -153,7 +185,7 @@ export class AssemblyManifestReader {
assetManifest.entries.forEach(entry => {
if (entry.type === 'file') {
const source = (entry as FileManifestEntry).source;
if (source.path && source.path.startsWith('asset.')) {
if (source.path && (source.path.startsWith('asset.') || source.path.endsWith('nested.template.json'))) {
assets.push(entry as FileManifestEntry);
}
} else if (entry.type === 'docker-image') {
Expand Down
226 changes: 133 additions & 93 deletions packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,25 @@ import { Diagnostic, DiagnosticReason, DestructiveChange, SnapshotVerificationOp
import { AssemblyManifestReader } from './private/cloud-assembly';
import { IntegRunnerOptions, IntegRunner, DEFAULT_SYNTH_OPTIONS } from './runner-base';

interface SnapshotAssembly {
/**
* Map of stacks that are part of this assembly
*/
[stackName: string]: {
/**
* All templates for this stack, including nested stacks
*/
templates: {
[templateId: string]: any
},

/**
* List of asset Ids that are used by this assembly
*/
assets: string[]
}
}

/**
* Runner for snapshot tests. This handles orchestrating
* the validation of the integration test snapshots
Expand All @@ -24,15 +43,7 @@ export class IntegSnapshotRunner extends IntegRunner {
public testSnapshot(options: SnapshotVerificationOptions = {}): { diagnostics: Diagnostic[], destructiveChanges: DestructiveChange[] } {
let doClean = true;
try {
// read the existing snapshot
const expectedStacks = this.readAssembly(this.snapshotDir);
// only diff stacks that are part of the test case
const expectedStacksToDiff: Record<string, any> = {};
for (const [stackName, template] of Object.entries(expectedStacks)) {
if (this.expectedTestSuite?.stacks.includes(stackName)) {
expectedStacksToDiff[stackName] = template;
}
}
const expectedSnapshotAssembly = this.getSnapshotAssembly(this.snapshotDir, this.expectedTestSuite?.stacks);

// synth the integration test
// FIXME: ideally we should not need to run this again if
Expand All @@ -52,18 +63,10 @@ export class IntegSnapshotRunner extends IntegRunner {
});

// read the "actual" snapshot
const actualDir = this.cdkOutDir;
const actualStacks = this.readAssembly(actualDir);
// only diff stacks that are part of the test case
const actualStacksToDiff: Record<string, any> = {};
for (const [stackName, template] of Object.entries(actualStacks)) {
if (this.actualTestSuite.stacks.includes(stackName)) {
actualStacksToDiff[stackName] = template;
}
}
const actualSnapshotAssembly = this.getSnapshotAssembly(this.cdkOutDir, this.actualTestSuite.stacks);

// diff the existing snapshot (expected) with the integration test (actual)
const diagnostics = this.diffAssembly(expectedStacksToDiff, actualStacksToDiff);
const diagnostics = this.diffAssembly(expectedSnapshotAssembly, actualSnapshotAssembly);

if (diagnostics.diagnostics.length) {
// Attach additional messages to the first diagnostic
Expand All @@ -72,7 +75,7 @@ export class IntegSnapshotRunner extends IntegRunner {
if (options.retain) {
additionalMessages.push(
`(Failure retained) Expected: ${path.relative(process.cwd(), this.snapshotDir)}`,
` Actual: ${path.relative(process.cwd(), actualDir)}`,
` Actual: ${path.relative(process.cwd(), this.cdkOutDir)}`,
),
doClean = false;
}
Expand Down Expand Up @@ -107,6 +110,36 @@ export class IntegSnapshotRunner extends IntegRunner {
}
}

/**
* For a given cloud assembly return a collection of all templates
* that should be part of the snapshot and any required meta data.
*
* @param cloudAssemblyDir The directory of the cloud assembly to look for snapshots
* @param pickStacks Pick only these stacks from the cloud assembly
* @returns A SnapshotAssembly, the collection of all templates in this snapshot and required meta data
*/
private getSnapshotAssembly(cloudAssemblyDir: string, pickStacks: string[] = []): SnapshotAssembly {
const assembly = this.readAssembly(cloudAssemblyDir);
const stacks = assembly.stacks;
const snapshots: SnapshotAssembly = {};
for (const [stackName, stackTemplate] of Object.entries(stacks)) {
if (pickStacks.includes(stackName)) {
const manifest = AssemblyManifestReader.fromPath(cloudAssemblyDir);
const assets = manifest.getAssetIdsForStack(stackName);

snapshots[stackName] = {
templates: {
[stackName]: stackTemplate,
...assembly.getNestedStacksForStack(stackName),
},
assets,
};
}
}

return snapshots;
}

/**
* For a given stack return all resource types that are allowed to be destroyed
* as part of a stack update
Expand All @@ -131,86 +164,98 @@ export class IntegSnapshotRunner extends IntegRunner {
* @returns any diagnostics and any destructive changes
*/
private diffAssembly(
expected: Record<string, any>,
actual: Record<string, any>,
expected: SnapshotAssembly,
actual: SnapshotAssembly,
): { diagnostics: Diagnostic[], destructiveChanges: DestructiveChange[] } {
const failures: Diagnostic[] = [];
const destructiveChanges: DestructiveChange[] = [];

// check if there is a CFN template in the current snapshot
// that does not exist in the "actual" snapshot
for (const templateId of Object.keys(expected)) {
if (!actual.hasOwnProperty(templateId)) {
failures.push({
testName: this.testName,
reason: DiagnosticReason.SNAPSHOT_FAILED,
message: `${templateId} exists in snapshot, but not in actual`,
});
for (const [stackId, stack] of Object.entries(expected)) {
for (const templateId of Object.keys(stack.templates)) {
if (!actual[stackId]?.templates[templateId]) {
failures.push({
testName: this.testName,
stackName: templateId,
reason: DiagnosticReason.SNAPSHOT_FAILED,
message: `${templateId} exists in snapshot, but not in actual`,
});
}
}
}

for (const templateId of Object.keys(actual)) {
for (const [stackId, stack] of Object.entries(actual)) {
for (const templateId of Object.keys(stack.templates)) {
// check if there is a CFN template in the "actual" snapshot
// that does not exist in the current snapshot
if (!expected.hasOwnProperty(templateId)) {
failures.push({
testName: this.testName,
reason: DiagnosticReason.SNAPSHOT_FAILED,
message: `${templateId} does not exist in snapshot, but does in actual`,
});
continue;
} else {
let actualTemplate = actual[templateId];
let expectedTemplate = expected[templateId];
const allowedDestroyTypes = this.getAllowedDestroyTypesForStack(templateId) ?? [];

// if we are not verifying asset hashes then remove the specific
// asset hashes from the templates so they are not part of the diff
// comparison
if (!this.actualTestSuite.getOptionsForStack(templateId)?.diffAssets) {
actualTemplate = this.canonicalizeTemplate(actualTemplate, templateId, this.cdkOutDir);
expectedTemplate = this.canonicalizeTemplate(expectedTemplate, templateId, this.snapshotDir);
}
const templateDiff = diffTemplate(expectedTemplate, actualTemplate);
if (!templateDiff.isEmpty) {
// go through all the resource differences and check for any
// "destructive" changes
templateDiff.resources.forEachDifference((logicalId: string, change: ResourceDifference) => {
if (!expected[stackId]?.templates[templateId]) {
failures.push({
testName: this.testName,
stackName: templateId,
reason: DiagnosticReason.SNAPSHOT_FAILED,
message: `${templateId} does not exist in snapshot, but does in actual`,
});
continue;
} else {
const config = {
diffAssets: this.actualTestSuite.getOptionsForStack(stackId)?.diffAssets,
};
let actualTemplate = actual[stackId].templates[templateId];
let expectedTemplate = expected[stackId].templates[templateId];

// if we are not verifying asset hashes then remove the specific
// asset hashes from the templates so they are not part of the diff
// comparison
if (!config.diffAssets) {
actualTemplate = this.canonicalizeTemplate(actualTemplate, actual[stackId].assets);
expectedTemplate = this.canonicalizeTemplate(expectedTemplate, expected[stackId].assets);
}
const templateDiff = diffTemplate(expectedTemplate, actualTemplate);
if (!templateDiff.isEmpty) {
const allowedDestroyTypes = this.getAllowedDestroyTypesForStack(stackId) ?? [];

// go through all the resource differences and check for any
// "destructive" changes
templateDiff.resources.forEachDifference((logicalId: string, change: ResourceDifference) => {
// if the change is a removal it will not show up as a 'changeImpact'
// so need to check for it separately, unless it is a resourceType that
// has been "allowed" to be destroyed
const resourceType = change.oldValue?.Type ?? change.newValue?.Type;
if (resourceType && allowedDestroyTypes.includes(resourceType)) {
return;
}
if (change.isRemoval) {
destructiveChanges.push({
impact: ResourceImpact.WILL_DESTROY,
logicalId,
stackName: templateId,
});
} else {
switch (change.changeImpact) {
case ResourceImpact.MAY_REPLACE:
case ResourceImpact.WILL_ORPHAN:
case ResourceImpact.WILL_DESTROY:
case ResourceImpact.WILL_REPLACE:
destructiveChanges.push({
impact: change.changeImpact,
logicalId,
stackName: templateId,
});
break;
const resourceType = change.oldValue?.Type ?? change.newValue?.Type;
if (resourceType && allowedDestroyTypes.includes(resourceType)) {
return;
}
}
});
const writable = new StringWritable({});
formatDifferences(writable, templateDiff);
failures.push({
reason: DiagnosticReason.SNAPSHOT_FAILED,
message: writable.data,
testName: this.testName,
});
if (change.isRemoval) {
destructiveChanges.push({
impact: ResourceImpact.WILL_DESTROY,
logicalId,
stackName: templateId,
});
} else {
switch (change.changeImpact) {
case ResourceImpact.MAY_REPLACE:
case ResourceImpact.WILL_ORPHAN:
case ResourceImpact.WILL_DESTROY:
case ResourceImpact.WILL_REPLACE:
destructiveChanges.push({
impact: change.changeImpact,
logicalId,
stackName: templateId,
});
break;
}
}
});
const writable = new StringWritable({});
formatDifferences(writable, templateDiff);
failures.push({
reason: DiagnosticReason.SNAPSHOT_FAILED,
message: writable.data,
stackName: templateId,
testName: this.testName,
config,
});
}
}
}
}
Expand All @@ -221,11 +266,8 @@ export class IntegSnapshotRunner extends IntegRunner {
};
}

private readAssembly(dir: string): Record<string, any> {
const assembly = AssemblyManifestReader.fromPath(dir);
const stacks = assembly.stacks;

return stacks;
private readAssembly(dir: string): AssemblyManifestReader {
return AssemblyManifestReader.fromPath(dir);
}

/**
Expand All @@ -234,7 +276,7 @@ export class IntegSnapshotRunner extends IntegRunner {
* This makes it possible to compare templates if all that's different between
* them is the hashes of the asset values.
*/
private canonicalizeTemplate(template: any, stackName: string, manifestDir: string): any {
private canonicalizeTemplate(template: any, assets: string[]): any {
const assetsSeen = new Set<string>();
const stringSubstitutions = new Array<[RegExp, string]>();

Expand Down Expand Up @@ -262,8 +304,6 @@ export class IntegSnapshotRunner extends IntegRunner {

// find assets defined in the asset manifest
try {
const manifest = AssemblyManifestReader.fromPath(manifestDir);
const assets = manifest.getAssetIdsForStack(stackName);
assets.forEach(asset => {
if (!assetsSeen.has(asset)) {
assetsSeen.add(asset);
Expand Down
10 changes: 10 additions & 0 deletions packages/@aws-cdk/integ-runner/lib/workers/common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,11 @@ export interface Diagnostic {
*/
readonly testName: string;

/**
* The name of the stack
*/
readonly stackName: string;

/**
* The diagnostic message
*/
Expand All @@ -240,6 +245,11 @@ export interface Diagnostic {
* Additional messages to print
*/
readonly additionalMessages?: string[];

/**
* Relevant config options that were used for the integ test
*/
readonly config?: Record<string, any>;
}

export function printSummary(total: number, failed: number): void {
Expand Down
Loading

0 comments on commit 5b3d06d

Please sign in to comment.