feat(integ-runner): support snapshot diff on nested stacks (#22881)

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*

Author: mrgrain

packages/@aws-cdk/integ-runner/README.md

@@ -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.

packages/@aws-cdk/integ-runner/lib/runner/private/cloud-assembly.ts

@@ -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
    */
@@ -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
    */
@@ -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') {

packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts

@@ -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
@@ -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
@@ -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
@@ -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;
         }
@@ -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
@@ -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,
+            });
+          }
         }
       }
     }
@@ -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);
   }
 
   /**
@@ -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]>();
 
@@ -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);

packages/@aws-cdk/integ-runner/lib/workers/common.ts

@@ -221,6 +221,11 @@ export interface Diagnostic {
    */
   readonly testName: string;
 
+  /**
+   * The name of the stack
+   */
+  readonly stackName: string;
+
   /**
    * The diagnostic message
    */
@@ -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 {