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/lib/integ-test.ts

@@ -79,3 +79,13 @@ export function randomString() {
   // Crazy
   return Math.random().toString(36).replace(/[^a-z0-9]+/g, '');
 }
+
+export function normalizeDiffOutput(s: string, removeFormatting: boolean = false): string {
+  if (removeFormatting) {
+    // remove all color and formatting (bolding, italic, etc)
+    s = s.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '');
+  }
+
+  return s.replace(/ /g, '') // remove all spaces
+    .replace(/\n/g, ''); // remove all new lines
+}

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

@@ -530,6 +530,45 @@ class DockerStackWithCustomFile extends cdk.Stack {
     });
   }
 }
+class SecurityDiffFromChangeSetStack extends Stack {
+  constructor(scope, id) {
+    super(scope, id);
+
+    const iamResourceName = ssm.StringParameter.valueForStringParameter(this, 'for-iam-role-defined-by-ssm-param');
+
+    new iam.Role(this, 'changeSetDiffIamRole', {
+      assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com'),
+      inlinePolicies: {
+        fun: new iam.PolicyDocument({
+          statements: [
+            new iam.PolicyStatement({
+              effect: iam.Effect.DENY,
+              actions: ['sqs:*'],
+              resources: [`arn:aws:sqs:us-east-1:444455556666:${iamResourceName}`],
+            }),
+          ],
+        }),
+      },
+    });
+
+  }
+}
+
+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 +724,9 @@ switch (stackSet) {
 
     const failed = new FailedStack(app, `${stackPrefix}-failed`)
 
+    new DiffFromChangeSetStack(app, `${stackPrefix}-queue-name-defined-by-ssm-param`)
+    new SecurityDiffFromChangeSetStack(app, `${stackPrefix}-iam-role-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, randomString, normalizeDiffOutput } from '../../lib';
 
 jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime
 
@@ -944,6 +944,107 @@ 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 security changes that are only in changeset', withDefaultFixture(async (fixture) => {
+  // GIVEN
+  const originalRoleName = randomString();
+  await fixture.aws.ssm('putParameter', {
+    Name: 'for-iam-role-defined-by-ssm-param',
+    Value: originalRoleName,
+    Type: 'String',
+    Overwrite: true,
+  });
+
+  try {
+    await fixture.cdkDeploy('iam-role-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 newRoleName = randomString();
+    await fixture.aws.ssm('putParameter', {
+      Name: 'for-iam-role-defined-by-ssm-param',
+      Value: newRoleName,
+      Type: 'String',
+      Overwrite: true,
+    });
+
+    const diff = await fixture.cdk(['diff', fixture.fullStackName('iam-role-defined-by-ssm-param')]);
+
+    // THEN
+    const normalizedPlainTextOutput = normalizeDiffOutput(diff, true);
+
+    const title = normalizeDiffOutput('IAM Statement Changes');
+    const header = normalizeDiffOutput('│   │ Resource                                                │ Effect │ Action │ Principal                    │ Condition │ ');
+    const remove = normalizeDiffOutput(`│ - │ arn:aws:sqs:us-east-1:444455556666:${originalRoleName}  │ Deny   │ sqs:*  │ AWS:\${changeSetDiffIamRole} │           │ `);
+    const add = normalizeDiffOutput(`   │ + │ arn:aws:sqs:us-east-1:444455556666:${newRoleName}       │ Deny   │ sqs:*  │ AWS:\${changeSetDiffIamRole} │           │ `);
+    const expectedDiff = normalizeDiffOutput(`
+      Resources
+      [~] AWS::IAM::Role changeSetDiffIamRole changeSetDiffIamRole9C803BA2
+      └─ [~] Policies
+          └─ @@ -6,7 +6,7 @@
+              [ ] "Statement": [
+              [ ]   {
+              [ ]     "Action": "sqs:*",
+              [-]     "Resource": "arn:aws:sqs:us-east-1:444455556666:${originalRoleName}",
+              [+]     "Resource": "arn:aws:sqs:us-east-1:444455556666:${newRoleName}",
+              [ ]     "Effect": "Deny"
+              [ ]   }
+              [ ] ]`);
+
+    expect(normalizedPlainTextOutput).toContain(expectedDiff);
+    expect(normalizedPlainTextOutput).toContain(title);
+    expect(normalizedPlainTextOutput).toContain(header);
+    expect(normalizedPlainTextOutput).toContain(remove);
+    expect(normalizedPlainTextOutput).toContain(add);
+  } finally {
+    await fixture.cdkDestroy('iam-role-defined-by-ssm-param');
+  }
+}));
+
+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 = normalizeDiffOutput(diff, true);
+
+    const normalizedExpectedOutput = normalizeDiffOutput(`
+      Resources
+      [~] AWS::SQS::Queue DiffFromChangeSetQueue DiffFromChangeSetQueue06622C07 replace
+       └─ [~] QueueName (requires replacement)
+           ├─ [-] ${originalQueueName}
+           └─ [+] ${newQueueName}
+      [~] AWS::SSM::Parameter DiffFromChangeSetSSMParam DiffFromChangeSetSSMParam92A9A723
+       └─ [~] Value
+           ├─ [-] ${originalQueueName}
+           └─ [+] ${newQueueName}`);
+
+    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); // I could remove this step and then run the integ tests and see what happens, assuming those tests use changeset
   if (changeSet) {
-    filterFalsePositives(theDiff, changeSet);
-    addImportInformation(theDiff, changeSet);
+    // These methods mutate the state of theDiff, using the changeSet.
+    const changeSetDiff = new TemplateAndChangeSetDiffMerger({ changeSet });
+    changeSetDiff.overrideDiffResourcesWithChangeSetResources(theDiff.resources);
+    theDiff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) =>
+      changeSetDiff.overrideDiffResourceChangeImpactWithChangeSetChangeImpact(logicalId, change),
+    );
+    changeSetDiff.addImportInformationFromChangeset(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 ?? {}))) {