fix(integ-runner): ignoring asset changes doesn't work with new style…

… assets (#21638) The integ-runner has the ability to ignore changes to assets since asset changes can lead to a lot of snapshot failures and we don't always need to re-deploy the integ test when this occurs. The `canonicalizeTemplate` function only worked with the v1 (`@aws-cdk/core:newStyleStackSynthesis:false`) assets which were stored in CFN parameters. This PR updates the logic to also account for the new style assets. Because we now have a list of the actual assets in the asset manifest the logic simply reads the list of assets from the manifest and replaces substitutes those values. I also refactored some things: - The `canonicalizeTemplate` function was moved into a private method of `IntegSnapshotRunner` since that is the only place it is used. - The CloudAssemblyManifest asset methods were refactored so that we could either return the asset location or the asset id ---- ### All Submissions: * [ ] 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 * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*
aws · Aug 17, 2022 · 7857f55 · 7857f55
1 parent 96ae997
commit 7857f55
Show file tree

Hide file tree

Showing 38 changed files with 1,205 additions and 95 deletions.
diff --git a/packages/@aws-cdk/integ-runner/lib/runner/private/canonicalize-assets.ts b/packages/@aws-cdk/integ-runner/lib/runner/private/canonicalize-assets.ts
diff --git a/packages/@aws-cdk/integ-runner/lib/runner/private/cloud-assembly.ts b/packages/@aws-cdk/integ-runner/lib/runner/private/cloud-assembly.ts
@@ -90,49 +90,76 @@ export class AssemblyManifestReader {
   }
 
   /**
-   * For a given stackId return a list of assets that belong to the stack
+   * Return a list of assets for a given stack
    */
-  public getAssetsForStack(stackId: string): string[] {
+  public getAssetIdsForStack(stackId: string): string[] {
     const assets: string[] = [];
     for (const artifact of Object.values(this.manifest.artifacts ?? {})) {
       if (artifact.type === ArtifactType.ASSET_MANIFEST && (artifact.properties as AssetManifestProperties)?.file === `${stackId}.assets.json`) {
-        assets.push(...this.assetsFromAssetManifest(artifact));
+        assets.push(...this.assetsFromAssetManifest(artifact).map(asset => asset.id.assetId));
       } else if (artifact.type === ArtifactType.AWS_CLOUDFORMATION_STACK) {
-        assets.push(...this.assetsFromAssemblyManifest(artifact));
+        assets.push(...this.assetsFromAssemblyManifest(artifact).map(asset => asset.id));
       }
     }
     return assets;
   }
 
-  private assetsFromAssemblyManifest(artifact: ArtifactManifest): string[] {
+  /**
+   * For a given stackId return a list of assets that belong to the stack
+   */
+  public getAssetLocationsForStack(stackId: string): string[] {
     const assets: string[] = [];
+    for (const artifact of Object.values(this.manifest.artifacts ?? {})) {
+      if (artifact.type === ArtifactType.ASSET_MANIFEST && (artifact.properties as AssetManifestProperties)?.file === `${stackId}.assets.json`) {
+        assets.push(...this.assetsFromAssetManifest(artifact).map(asset => {
+          if (asset.type === 'file') {
+            return asset.source.path!;
+          } else {
+            return asset.source.directory!;
+          }
+        }));
+      } else if (artifact.type === ArtifactType.AWS_CLOUDFORMATION_STACK) {
+        assets.push(...this.assetsFromAssemblyManifest(artifact).map(asset => asset.path));
+      }
+    }
+    return assets;
+  }
+
+  /**
+   * Get a list of assets from the assembly manifest
+   */
+  private assetsFromAssemblyManifest(artifact: ArtifactManifest): (ContainerImageAssetMetadataEntry | FileAssetMetadataEntry)[] {
+    const assets: (ContainerImageAssetMetadataEntry | FileAssetMetadataEntry)[] = [];
     for (const metadata of Object.values(artifact.metadata ?? {})) {
       metadata.forEach(data => {
         if (data.type === ArtifactMetadataEntryType.ASSET) {
-          const assetPath = (data.data as ContainerImageAssetMetadataEntry | FileAssetMetadataEntry).path;
-          if (assetPath.startsWith('asset.')) {
-            assets.push(assetPath);
+          const asset = (data.data as ContainerImageAssetMetadataEntry | FileAssetMetadataEntry);
+          if (asset.path.startsWith('asset.')) {
+            assets.push(asset);
           }
         }
       });
     }
     return assets;
   }
 
-  private assetsFromAssetManifest(artifact: ArtifactManifest): string[] {
-    const assets: string[] = [];
+  /**
+   * Get a list of assets from the asset manifest
+   */
+  private assetsFromAssetManifest(artifact: ArtifactManifest): (FileManifestEntry | DockerImageManifestEntry)[] {
+    const assets: (FileManifestEntry | DockerImageManifestEntry)[] = [];
     const fileName = (artifact.properties as AssetManifestProperties).file;
     const assetManifest = AssetManifest.fromFile(path.join(this.directory, fileName));
     assetManifest.entries.forEach(entry => {
       if (entry.type === 'file') {
         const source = (entry as FileManifestEntry).source;
         if (source.path && source.path.startsWith('asset.')) {
-          assets.push((entry as FileManifestEntry).source.path!);
+          assets.push(entry as FileManifestEntry);
         }
       } else if (entry.type === 'docker-image') {
         const source = (entry as DockerImageManifestEntry).source;
         if (source.directory && source.directory.startsWith('asset.')) {
-          assets.push((entry as DockerImageManifestEntry).source.directory!);
+          assets.push(entry as DockerImageManifestEntry);
         }
       }
     });

diff --git a/packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts b/packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts
@@ -268,7 +268,7 @@ export abstract class IntegRunner {
     const stacks = this.actualTestSuite.getStacksWithoutUpdateWorkflow() ?? [];
     const manifest = AssemblyManifestReader.fromPath(this.snapshotDir);
     const assets = flatten(stacks.map(stack => {
-      return manifest.getAssetsForStack(stack) ?? [];
+      return manifest.getAssetLocationsForStack(stack) ?? [];
     }));
 
     assets.forEach(asset => {

diff --git a/packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts b/packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts
@@ -3,7 +3,6 @@ import { Writable, WritableOptions } from 'stream';
 import { StringDecoder } from 'string_decoder';
 import { diffTemplate, formatDifferences, ResourceDifference, ResourceImpact } from '@aws-cdk/cloudformation-diff';
 import { Diagnostic, DiagnosticReason, DestructiveChange, SnapshotVerificationOptions } from '../workers/common';
-import { canonicalizeTemplate } from './private/canonicalize-assets';
 import { AssemblyManifestReader } from './private/cloud-assembly';
 import { IntegRunnerOptions, IntegRunner, DEFAULT_SYNTH_OPTIONS } from './runner-base';
 
@@ -166,8 +165,8 @@ export class IntegSnapshotRunner extends IntegRunner {
         // asset hashes from the templates so they are not part of the diff
         // comparison
         if (!this.actualTestSuite.getOptionsForStack(templateId)?.diffAssets) {
-          actualTemplate = canonicalizeTemplate(actualTemplate);
-          expectedTemplate = canonicalizeTemplate(expectedTemplate);
+          actualTemplate = this.canonicalizeTemplate(actualTemplate, templateId, this.cdkOutDir);
+          expectedTemplate = this.canonicalizeTemplate(expectedTemplate, templateId, this.snapshotDir);
         }
         const templateDiff = diffTemplate(expectedTemplate, actualTemplate);
         if (!templateDiff.isEmpty) {
@@ -225,6 +224,87 @@ export class IntegSnapshotRunner extends IntegRunner {
 
     return stacks;
   }
+
+  /**
+  * Reduce template to a normal form where asset references have been normalized
+  *
+  * 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 {
+    const assetsSeen = new Set<string>();
+    const stringSubstitutions = new Array<[RegExp, string]>();
+
+    // Find assets via parameters (for LegacyStackSynthesizer)
+    const paramRe = /^AssetParameters([a-zA-Z0-9]{64})(S3Bucket|S3VersionKey|ArtifactHash)([a-zA-Z0-9]{8})$/;
+    for (const paramName of Object.keys(template?.Parameters || {})) {
+      const m = paramRe.exec(paramName);
+      if (!m) { continue; }
+      if (assetsSeen.has(m[1])) { continue; }
+
+      assetsSeen.add(m[1]);
+      const ix = assetsSeen.size;
+
+      // Full parameter reference
+      stringSubstitutions.push([
+        new RegExp(`AssetParameters${m[1]}(S3Bucket|S3VersionKey|ArtifactHash)([a-zA-Z0-9]{8})`),
+        `Asset${ix}$1`,
+      ]);
+      // Substring asset hash reference
+      stringSubstitutions.push([
+        new RegExp(`${m[1]}`),
+        `Asset${ix}Hash`,
+      ]);
+    }
+
+    // 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);
+          const ix = assetsSeen.size;
+          stringSubstitutions.push([
+            new RegExp(asset),
+            `Asset${ix}$1`,
+          ]);
+        }
+      });
+    } catch {
+      // if there is no asset manifest that is fine.
+    }
+
+    // Substitute them out
+    return substitute(template);
+
+    function substitute(what: any): any {
+      if (Array.isArray(what)) {
+        return what.map(substitute);
+      }
+
+      if (typeof what === 'object' && what !== null) {
+        const ret: any = {};
+        for (const [k, v] of Object.entries(what)) {
+          ret[stringSub(k)] = substitute(v);
+        }
+        return ret;
+      }
+
+      if (typeof what === 'string') {
+        return stringSub(what);
+      }
+
+      return what;
+    }
+
+    function stringSub(x: string) {
+      for (const [re, replacement] of stringSubstitutions) {
+        x = x.replace(re, replacement);
+      }
+      return x;
+    }
+  }
 }
 
 class StringWritable extends Writable {

diff --git a/packages/@aws-cdk/integ-runner/test/runner/private/cloud-assembly.test.ts b/packages/@aws-cdk/integ-runner/test/runner/private/cloud-assembly.test.ts
@@ -287,7 +287,7 @@ describe('cloud assembly manifest reader', () => {
   test('can get assets from assembly manifest', () => {
     // WHEN
     const manifest = AssemblyManifestReader.fromFile(manifestFile);
-    const assets = manifest.getAssetsForStack('test-stack2');
+    const assets = manifest.getAssetLocationsForStack('test-stack2');
 
     // THEN
     expect(assets).toEqual([

diff --git a/packages/@aws-cdk/integ-runner/test/runner/snapshot-test-runner.test.ts b/packages/@aws-cdk/integ-runner/test/runner/snapshot-test-runner.test.ts
@@ -136,6 +136,51 @@ describe('IntegTest runSnapshotTests', () => {
     }]);
   });
 
+  test('dont diff new asset hashes', () => {
+    // WHEN
+    const integTest = new IntegSnapshotRunner({
+      cdk: cdkMock.cdk,
+      test: new IntegTest({
+        fileName: path.join(__dirname, '../test-data/xxxxx.test-with-new-assets-diff.js'),
+        discoveryRoot: 'test/test-data',
+      }),
+      integOutDir: 'test/test-data/cdk-integ.out.test-with-new-assets',
+    });
+    const results = integTest.testSnapshot();
+    expect(results.diagnostics).toEqual([]);
+
+    // THEN
+    expect(synthFastMock).toHaveBeenCalledTimes(2);
+    expect(synthFastMock).toHaveBeenCalledWith({
+      execCmd: ['node', 'xxxxx.test-with-new-assets-diff.js'],
+      env: expect.objectContaining({
+        CDK_INTEG_ACCOUNT: '12345678',
+        CDK_INTEG_REGION: 'test-region',
+      }),
+      output: 'cdk-integ.out.test-with-new-assets',
+    });
+  });
+
+  test('diff new asset hashes', () => {
+    // WHEN
+    const integTest = new IntegSnapshotRunner({
+      cdk: cdkMock.cdk,
+      test: new IntegTest({
+        fileName: path.join(__dirname, '../test-data/xxxxx.test-with-new-assets.js'),
+        discoveryRoot: 'test/test-data',
+      }),
+      integOutDir: 'test/test-data/cdk-integ.out.test-with-new-assets-diff',
+    });
+    const results = integTest.testSnapshot();
+
+    // THEN
+    expect(results.diagnostics).toEqual(expect.arrayContaining([expect.objectContaining({
+      reason: DiagnosticReason.SNAPSHOT_FAILED,
+      testName: integTest.testName,
+      message: expect.stringContaining('S3Key'),
+    })]));
+  });
+
   test('dont diff asset hashes', () => {
     // WHEN
     const integTest = new IntegSnapshotRunner({
@@ -146,9 +191,8 @@ describe('IntegTest runSnapshotTests', () => {
       }),
       integOutDir: 'test/test-data/test-with-snapshot-assets.integ.snapshot',
     });
-    expect(() => {
-      integTest.testSnapshot();
-    }).not.toThrow();
+    const results = integTest.testSnapshot();
+    expect(results.diagnostics).toEqual([]);
 
     // THEN
     expect(synthFastMock).toHaveBeenCalledTimes(2);

diff --git a/...ages/@aws-cdk/integ-runner/test/test-data/cdk-integ.out.test-with-new-assets-diff/cdk.out b/...ages/@aws-cdk/integ-runner/test/test-data/cdk-integ.out.test-with-new-assets-diff/cdk.out
@@ -0,0 +1 @@
+{"version":"18.0.0"}
diff --git a/...s/@aws-cdk/integ-runner/test/test-data/cdk-integ.out.test-with-new-assets-diff/integ.json b/...s/@aws-cdk/integ-runner/test/test-data/cdk-integ.out.test-with-new-assets-diff/integ.json
@@ -0,0 +1,13 @@
+{
+  "version": "v1.0.0",
+  "testCases": {
+    "xxxxx.test-with-new-assets": {
+      "stacks": ["test-stack"],
+      "stackUpdateWorkflow": false,
+      "diffAssets": true,
+      "allowDestroy": [
+        "AWS::IAM::Role"
+      ]
+    }
+  }
+}