revert: "fix(diff): properties from ChangeSet diff were ignored"

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

@@ -20,7 +20,6 @@ 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>;
@@ -40,7 +39,6 @@ export class AwsClients {
     this.ecs = makeAwsCaller(AWS.ECS, this.config);
     this.sso = makeAwsCaller(AWS.SSO, this.config);
     this.sns = makeAwsCaller(AWS.SNS, this.config);
-    this.ssm = makeAwsCaller(AWS.SSM, this.config);
     this.iam = makeAwsCaller(AWS.IAM, this.config);
     this.lambda = makeAwsCaller(AWS.Lambda, this.config);
     this.sts = makeAwsCaller(AWS.STS, this.config);

packages/@aws-cdk-testing/cli-integ/package.json

@@ -30,12 +30,12 @@
   "license": "Apache-2.0",
   "devDependencies": {
     "@aws-cdk/cdk-build-tools": "0.0.0",
-    "@aws-cdk/pkglint": "0.0.0",
+    "@types/semver": "^7.5.8",
+    "@types/yargs": "^15.0.19",
     "@types/fs-extra": "^9.0.13",
     "@types/glob": "^7.2.0",
     "@types/npm": "^7.19.3",
-    "@types/semver": "^7.5.8",
-    "@types/yargs": "^15.0.19"
+    "@aws-cdk/pkglint": "0.0.0"
   },
   "dependencies": {
     "@octokit/rest": "^18.12.0",
@@ -72,4 +72,4 @@
   "publishConfig": {
     "tag": "latest"
   }
-}
+}

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

@@ -126,22 +126,6 @@ class SsoInstanceAccessControlConfig extends 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,
-    });
-  }
-}
-
 class ListMultipleDependentStack extends Stack {
   constructor(scope, id) {
     super(scope, id);
@@ -701,8 +685,6 @@ 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

@@ -1,7 +1,7 @@
 import { promises as fs, existsSync } from 'fs';
 import * as os from 'os';
 import * as path from 'path';
-import { integTest, cloneDirectory, shell, withDefaultFixture, retry, sleep, randomInteger, withSamIntegrationFixture, RESOURCES_DIR, withCDKMigrateFixture, withExtendedTimeoutFixture, randomString } from '../../lib';
+import { integTest, cloneDirectory, shell, withDefaultFixture, retry, sleep, randomInteger, withSamIntegrationFixture, RESOURCES_DIR, withCDKMigrateFixture, withExtendedTimeoutFixture } from '../../lib';
 
 jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime
 
@@ -944,50 +944,6 @@ 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
-  await fixture.aws.ssm('putParameter', {
-    Name: 'for-queue-name-defined-by-ssm-param',
-    Value: randomString(),
-    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.
-    await fixture.aws.ssm('putParameter', {
-      Name: 'for-queue-name-defined-by-ssm-param',
-      Value: randomString(),
-      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)
-      [~] AWS::SSM::Parameter DiffFromChangeSetSSMParam DiffFromChangeSetSSMParam92A9A723
-       └─ [~] Value`
-      .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

@@ -55,7 +55,7 @@ export function fullDiff(
   normalize(newTemplate);
   const theDiff = diffTemplate(currentTemplate, newTemplate);
   if (changeSet) {
-    refineDiffWithChangeSet(theDiff, changeSet, newTemplate.Resources);
+    filterFalsePositives(theDiff, changeSet);
     addImportInformation(theDiff, changeSet);
   } else if (isImport) {
     makeAllResourceChangesImports(theDiff);
@@ -143,6 +143,13 @@ 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
  *
@@ -222,103 +229,45 @@ function makeAllResourceChangesImports(diff: types.TemplateDiff) {
   });
 }
 
-function refineDiffWithChangeSet(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput, newTemplateResources: {[logicalId: string]: any}) {
-  const replacements = _findResourceReplacements(changeSet);
-
-  _addChangeSetResourcesToDiff(replacements, newTemplateResources);
-  _enhanceChangeImpacts(replacements);
-  return;
-
-  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,
-      };
+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;
     }
-
-    return _replacements;
-  }
-
-  function _addChangeSetResourcesToDiff(_replacements: types.ResourceReplacements, _newTemplateResources: {[logicalId: string]: any}) {
-    const resourceDiffLogicalIds = diff.resources.logicalIds;
-    for (const logicalId of Object.keys(_replacements)) {
-      if (!(resourceDiffLogicalIds.includes(logicalId))) {
-        const noChangeResourceDiff = impl.diffResource(_newTemplateResources[logicalId], _newTemplateResources[logicalId]);
-        diff.resources.add(logicalId, noChangeResourceDiff);
-      }
-
-      for (const propertyName of Object.keys(_replacements[logicalId].propertiesReplaced)) {
-        if (propertyName in diff.resources.get(logicalId).propertyUpdates) {
-        // If the property is already marked to be updated, then we don't need to do anything.
-          continue;
+    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;
         }
-
-        const newProp = new types.PropertyDifference(
-        // these fields will be decided below
-          {}, {}, { changeImpact: undefined },
-        );
-        newProp.isDifferent = true;
-        diff.resources.get(logicalId).setPropertyChange(propertyName, newProp);
-      }
-    };
-  }
-
-  function _enhanceChangeImpacts(_replacements: types.ResourceReplacements) {
-    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]) {
+        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;
-            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;
+            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;
-          }
         }
-      });
+      } 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[] {
@@ -332,6 +281,33 @@ function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
   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 ?? {}))) {

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

@@ -349,10 +349,6 @@ export class PropertyDifference<ValueType> extends Difference<ValueType> {
 export class DifferenceCollection<V, T extends IDifference<V>> {
   constructor(private readonly diffs: { [logicalId: string]: T }) {}
 
-  public add(logicalId: string, diff: T): void {
-    this.diffs[logicalId] = diff;
-  }
-
   public get changes(): { [logicalId: string]: T } {
     return onlyChanges(this.diffs);
   }