fix(diff): properties from ChangeSet diff were ignored

packages/@aws-cdk-testing/cli-integ/lib/aws.ts

@@ -20,6 +20,7 @@ export class AwsClients {
   public readonly ecr: AwsCaller<AWS.ECR>;
   public readonly ecs: AwsCaller<AWS.ECS>;
   public readonly sso: AwsCaller<AWS.SSO>;
+  public readonly ssm: AwsCaller<AWS.SSM>;
   public readonly sns: AwsCaller<AWS.SNS>;
   public readonly iam: AwsCaller<AWS.IAM>;
   public readonly lambda: AwsCaller<AWS.Lambda>;
@@ -38,6 +39,7 @@ export class AwsClients {
     this.ecr = makeAwsCaller(AWS.ECR, this.config);
     this.ecs = makeAwsCaller(AWS.ECS, this.config);
     this.sso = makeAwsCaller(AWS.SSO, this.config);
+    this.ssm = makeAwsCaller(AWS.SSM, this.config);
     this.sns = makeAwsCaller(AWS.SNS, this.config);
     this.iam = makeAwsCaller(AWS.IAM, this.config);
     this.lambda = makeAwsCaller(AWS.Lambda, this.config);

packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js

@@ -531,6 +531,22 @@ class DockerStackWithCustomFile extends cdk.Stack {
   }
 }
 
+class DiffFromChangeSetStack extends Stack {
+  constructor(scope, id) {
+    super(scope, id);
+
+    const queueNameFromParameter = ssm.StringParameter.valueForStringParameter(this, 'for-queue-name-defined-by-ssm-param');
+    new sqs.Queue(this, "DiffFromChangeSetQueue", {
+      queueName: queueNameFromParameter,
+    })
+
+    new ssm.StringParameter(this, 'DiffFromChangeSetSSMParam', {
+      parameterName: 'DiffFromChangeSetSSMParamName',
+      stringValue: queueNameFromParameter,
+    });
+  }
+}
+
 /**
  * A stack that will never succeed deploying (done in a way that CDK cannot detect but CFN will complain about)
  */
@@ -685,6 +701,8 @@ switch (stackSet) {
 
     const failed = new FailedStack(app, `${stackPrefix}-failed`)
 
+    new DiffFromChangeSetStack(app, `${stackPrefix}-queue-name-defined-by-ssm-param`)
+
     // A stack that depends on the failed stack -- used to test that '-e' does not deploy the failing stack
     const dependsOnFailed = new OutputsStack(app, `${stackPrefix}-depends-on-failed`);
     dependsOnFailed.addDependency(failed);

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

@@ -944,6 +944,57 @@ integTest('cdk diff --quiet does not print \'There were no differences\' message
   expect(diff).not.toContain('There were no differences');
 }));
 
+integTest('cdk diff picks up changes that are only present in changeset', withDefaultFixture(async (fixture) => {
+  // GIVEN
+  const originalQueueName = randomString();
+  await fixture.aws.ssm('putParameter', {
+    Name: 'for-queue-name-defined-by-ssm-param',
+    Value: originalQueueName,
+    Type: 'String',
+    Overwrite: true,
+  });
+
+  try {
+    await fixture.cdkDeploy('queue-name-defined-by-ssm-param');
+
+    // WHEN
+    // We want to change the ssm value. Then the CFN changeset will detect that the queue will be changed upon deploy.
+    const newQueueName = randomString();
+    await fixture.aws.ssm('putParameter', {
+      Name: 'for-queue-name-defined-by-ssm-param',
+      Value: newQueueName,
+      Type: 'String',
+      Overwrite: true,
+    });
+
+    const diff = await fixture.cdk(['diff', fixture.fullStackName('queue-name-defined-by-ssm-param')]);
+
+    // THEN
+    const normalizedPlainTextOutput = diff.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '') // remove all color and formatting (bolding, italic, etc)
+      .replace(/ /g, '') // remove all spaces
+      .replace(/\n/g, '') // remove all new lines
+      .replace(/\d+/g, ''); // remove all digits
+
+    const normalizedExpectedOutput = `
+      Resources
+      [~] AWS::SQS::Queue DiffFromChangeSetQueue DiffFromChangeSetQueue06622C07 replace
+       └─ [~] QueueName (requires replacement)
+           ├─ [-] ${originalQueueName}
+           └─ [+] ${newQueueName}
+      [~] AWS::SSM::Parameter DiffFromChangeSetSSMParam DiffFromChangeSetSSMParam92A9A723
+       └─ [~] Value
+           ├─ [-] ${originalQueueName}
+           └─ [+] ${newQueueName}`
+      .replace(/ /g, '')
+      .replace(/\n/g, '')
+      .replace(/\d+/g, '');
+
+    expect(normalizedPlainTextOutput).toContain(normalizedExpectedOutput);
+  } finally {
+    await fixture.cdkDestroy('queue-name-defined-by-ssm-param');
+  }
+}));
+
 integTest('deploy stack with docker asset', withDefaultFixture(async (fixture) => {
   await fixture.cdkDeploy('docker');
 }));

packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts

@@ -2,6 +2,7 @@
 // The SDK should not make network calls here
 import type { DescribeChangeSetOutput as DescribeChangeSet } from '@aws-sdk/client-cloudformation';
 import * as impl from './diff';
+import { TemplateAndChangeSetDiffMerger } from './diff/template-and-changeset-diff-merger';
 import * as types from './diff/types';
 import { deepEqual, diffKeyedEntities, unionOf } from './diff/util';
 
@@ -53,10 +54,16 @@ export function fullDiff(
 
   normalize(currentTemplate);
   normalize(newTemplate);
-  const theDiff = diffTemplate(currentTemplate, newTemplate);
+  let theDiff = diffTemplate(currentTemplate, newTemplate);
   if (changeSet) {
-    filterFalsePositives(theDiff, changeSet);
-    addImportInformation(theDiff, changeSet);
+    // These methods mutate the state of theDiff, using the changeSet.
+    const changeSetDiff = new TemplateAndChangeSetDiffMerger({ changeSet: changeSet });
+    changeSetDiff.addChangeSetResourcesToDiffResources(theDiff.resources);
+    theDiff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) =>
+      changeSetDiff.hydrateChangeImpactFromChangeSet(logicalId, change),
+    );
+    changeSetDiff.addImportInformation(theDiff.resources);
+    theDiff = new types.TemplateDiff(theDiff); // do this to propagate security changes.
   } else if (isImport) {
     makeAllResourceChangesImports(theDiff);
   }
@@ -143,13 +150,6 @@ function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTempl
   return new types.TemplateDiff(differences);
 }
 
-/**
- * Compare two CloudFormation resources and return semantic differences between them
- */
-export function diffResource(oldValue: types.Resource, newValue: types.Resource): types.ResourceDifference {
-  return impl.diffResource(oldValue, newValue);
-}
-
 /**
  * Replace all references to the given logicalID on the given template, in-place
  *
@@ -214,100 +214,12 @@ function deepCopy(x: any): any {
   return x;
 }
 
-function addImportInformation(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput) {
-  const imports = findResourceImports(changeSet);
-  diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
-    if (imports.includes(logicalId)) {
-      change.isImport = true;
-    }
-  });
-}
-
 function makeAllResourceChangesImports(diff: types.TemplateDiff) {
   diff.resources.forEachDifference((_logicalId: string, change: types.ResourceDifference) => {
     change.isImport = true;
   });
 }
 
-function filterFalsePositives(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput) {
-  const replacements = findResourceReplacements(changeSet);
-  diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
-    if (change.resourceType.includes('AWS::Serverless')) {
-      // CFN applies the SAM transform before creating the changeset, so the changeset contains no information about SAM resources
-      return;
-    }
-    change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => {
-      if (type === 'Property') {
-        if (!replacements[logicalId]) {
-          (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
-          (value as types.PropertyDifference<any>).isDifferent = false;
-          return;
-        }
-        switch (replacements[logicalId].propertiesReplaced[name]) {
-          case 'Always':
-            (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_REPLACE;
-            break;
-          case 'Never':
-            (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_UPDATE;
-            break;
-          case 'Conditionally':
-            (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.MAY_REPLACE;
-            break;
-          case undefined:
-            (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
-            (value as types.PropertyDifference<any>).isDifferent = false;
-            break;
-          // otherwise, defer to the changeImpact from `diffTemplate`
-        }
-      } else if (type === 'Other') {
-        switch (name) {
-          case 'Metadata':
-            change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue));
-            break;
-        }
-      }
-    });
-  });
-}
-
-function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
-  const importedResourceLogicalIds = [];
-  for (const resourceChange of changeSet.Changes ?? []) {
-    if (resourceChange.ResourceChange?.Action === 'Import') {
-      importedResourceLogicalIds.push(resourceChange.ResourceChange.LogicalResourceId!);
-    }
-  }
-
-  return importedResourceLogicalIds;
-}
-
-function findResourceReplacements(changeSet: DescribeChangeSetOutput): types.ResourceReplacements {
-  const replacements: types.ResourceReplacements = {};
-  for (const resourceChange of changeSet.Changes ?? []) {
-    const propertiesReplaced: { [propName: string]: types.ChangeSetReplacement } = {};
-    for (const propertyChange of resourceChange.ResourceChange?.Details ?? []) {
-      if (propertyChange.Target?.Attribute === 'Properties') {
-        const requiresReplacement = propertyChange.Target.RequiresRecreation === 'Always';
-        if (requiresReplacement && propertyChange.Evaluation === 'Static') {
-          propertiesReplaced[propertyChange.Target.Name!] = 'Always';
-        } else if (requiresReplacement && propertyChange.Evaluation === 'Dynamic') {
-          // If Evaluation is 'Dynamic', then this may cause replacement, or it may not.
-          // see 'Replacement': https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ResourceChange.html
-          propertiesReplaced[propertyChange.Target.Name!] = 'Conditionally';
-        } else {
-          propertiesReplaced[propertyChange.Target.Name!] = propertyChange.Target.RequiresRecreation as types.ChangeSetReplacement;
-        }
-      }
-    }
-    replacements[resourceChange.ResourceChange?.LogicalResourceId!] = {
-      resourceReplaced: resourceChange.ResourceChange?.Replacement === 'True',
-      propertiesReplaced,
-    };
-  }
-
-  return replacements;
-}
-
 function normalize(template: any) {
   if (typeof template === 'object') {
     for (const key of (Object.keys(template ?? {}))) {