fix(aws-cdk): Improvements to IAM diff rendering

packages/@aws-cdk/assert/lib/assertions/match-template.ts

@@ -55,7 +55,7 @@ class StackMatchesTemplateAssertion extends Assertion<StackInspector> {
   private isDiffAcceptable(diff: cfnDiff.TemplateDiff): boolean {
     switch (this.matchStyle) {
     case MatchStyle.EXACT:
-      return diff.count === 0;
+      return diff.differenceCount === 0;
     case MatchStyle.NO_REPLACES:
       for (const key of Object.keys(diff.resources.changes)) {
         const change = diff.resources.changes[key]!;

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

@@ -39,42 +39,77 @@ const DIFF_HANDLERS: HandlerRegistry = {
  *      the template +newTemplate+.
  */
 export function diffTemplate(currentTemplate: { [key: string]: any }, newTemplate: { [key: string]: any }): types.TemplateDiff {
+  // Base diff
+  const theDiff = calculateTemplateDiff(currentTemplate, newTemplate);
+
   // We're going to modify this in-place
-  newTemplate = deepCopy(newTemplate);
-
-  while (true) {
-    const differences: types.ITemplateDiff = {};
-    const unknown: { [key: string]: types.Difference<any> } = {};
-    for (const key of unionOf(Object.keys(currentTemplate), Object.keys(newTemplate)).sort()) {
-      const oldValue = currentTemplate[key];
-      const newValue = newTemplate[key];
-      if (deepEqual(oldValue, newValue)) { continue; }
-      const handler: DiffHandler = DIFF_HANDLERS[key]
-                    || ((_diff, oldV, newV) => unknown[key] = impl.diffUnknown(oldV, newV));
-      handler(differences, oldValue, newValue);
+  const newTemplateCopy = deepCopy(newTemplate);
 
-    }
-    if (Object.keys(unknown).length > 0) { differences.unknown = new types.DifferenceCollection(unknown); }
+  let didPropagateReferenceChanges;
+  let diffWithReplacements;
+  do {
+    diffWithReplacements = calculateTemplateDiff(currentTemplate, newTemplateCopy);
 
     // Propagate replacements for replaced resources
-    let didPropagateReferenceChanges = false;
-    if (differences.resources) {
-      differences.resources.forEach((logicalId, change) => {
+    didPropagateReferenceChanges = false;
+    if (diffWithReplacements.resources) {
+      diffWithReplacements.resources.forEachDifference((logicalId, change) => {
         if (change.changeImpact === types.ResourceImpact.WILL_REPLACE) {
-          if (propagateReplacedReferences(newTemplate, logicalId)) {
+          if (propagateReplacedReferences(newTemplateCopy, logicalId)) {
             didPropagateReferenceChanges = true;
           }
         }
       });
     }
+  } while (didPropagateReferenceChanges);
+
+  // Copy "replaced" states from `diffWithReplacements` to `theDiff`.
+  diffWithReplacements.resources
+      .filter(r => isReplacement(r!.changeImpact))
+      .forEachDifference((logicalId, downstreamReplacement) => {
+    const resource = theDiff.resources.get(logicalId);
+
+    if (resource.changeImpact !== downstreamReplacement.changeImpact) {
+      propagatePropertyReplacement(downstreamReplacement, resource);
+    }
+  });
+
+  return theDiff;
+}
+
+function isReplacement(impact: types.ResourceImpact) {
+  return impact === types.ResourceImpact.MAY_REPLACE || impact === types.ResourceImpact.WILL_REPLACE;
+}
 
-    // We're done only if we didn't have to propagate any more replacements.
-    if (!didPropagateReferenceChanges) {
-      return new types.TemplateDiff(differences);
+/**
+ * For all properties in 'source' that have a "replacement" impact, propagate that impact to "dest"
+ */
+function propagatePropertyReplacement(source: types.ResourceDifference, dest: types.ResourceDifference) {
+  for (const [propertyName, diff] of Object.entries(source.propertyUpdates)) {
+    if (diff.changeImpact && isReplacement(diff.changeImpact)) {
+      // Use the propertydiff of source in target. The result of this happens to be clear enough.
+      dest.setPropertyChange(propertyName, diff);
     }
   }
 }
 
+function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTemplate: { [key: string]: any }): types.TemplateDiff {
+  const differences: types.ITemplateDiff = {};
+  const unknown: { [key: string]: types.Difference<any> } = {};
+  for (const key of unionOf(Object.keys(currentTemplate), Object.keys(newTemplate)).sort()) {
+    const oldValue = currentTemplate[key];
+    const newValue = newTemplate[key];
+    if (deepEqual(oldValue, newValue)) { continue; }
+    const handler: DiffHandler = DIFF_HANDLERS[key]
+                  || ((_diff, oldV, newV) => unknown[key] = impl.diffUnknown(oldV, newV));
+    handler(differences, oldValue, newValue);
+
+  }
+  if (Object.keys(unknown).length > 0) { differences.unknown = new types.DifferenceCollection(unknown); }
+
+  return new types.TemplateDiff(differences);
+}
+
 /**
  * Compare two CloudFormation resources and return semantic differences between them
  */
@@ -109,7 +144,7 @@ function propagateReplacedReferences(template: object, logicalId: string): boole
 
     if (key === 'Ref') {
       if (obj.Ref === logicalId) {
-        obj.Ref = logicalId + '(replaced)';
+        obj.Ref = logicalId + ' (replaced)';
         ret = true;
       }
       return true;

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

@@ -1,6 +1,6 @@
 import cfnspec = require('@aws-cdk/cfnspec');
 import types = require('./types');
-import { diffKeyedEntities } from './util';
+import { deepEqual, diffKeyedEntities } from './util';
 
 export function diffAttribute(oldValue: any, newValue: any): types.Difference<string> {
   return new types.Difference<string>(_asString(oldValue), _asString(newValue));
@@ -31,31 +31,29 @@ export function diffResource(oldValue?: types.Resource, newValue?: types.Resourc
     oldType: oldValue && oldValue.Type,
     newType: newValue && newValue.Type
   };
-  let propertyUpdates: { [key: string]: types.PropertyDifference<any> } = {};
-  let otherChanges: { [key: string]: types.Difference<any> } = {};
+  let propertyDiffs: { [key: string]: types.PropertyDifference<any> } = {};
+  let otherDiffs: { [key: string]: types.Difference<any> } = {};
 
   if (resourceType.oldType !== undefined && resourceType.oldType === resourceType.newType) {
     // Only makes sense to inspect deeper if the types stayed the same
     const typeSpec = cfnspec.filteredSpecification(resourceType.oldType);
     const impl = typeSpec.ResourceTypes[resourceType.oldType];
-    propertyUpdates = diffKeyedEntities(oldValue!.Properties,
+    propertyDiffs = diffKeyedEntities(oldValue!.Properties,
                       newValue!.Properties,
                       (oldVal, newVal, key) => _diffProperty(oldVal, newVal, key, impl));
-    otherChanges = diffKeyedEntities(oldValue, newValue, _diffOther);
-    delete otherChanges.Properties;
+    otherDiffs = diffKeyedEntities(oldValue, newValue, _diffOther);
+    delete otherDiffs.Properties;
   }
 
   return new types.ResourceDifference(oldValue, newValue, {
-    resourceType, propertyUpdates, otherChanges,
-    oldProperties: oldValue && oldValue.Properties,
-    newProperties: newValue && newValue.Properties,
+    resourceType, propertyDiffs, otherDiffs,
   });
 
   function _diffProperty(oldV: any, newV: any, key: string, resourceSpec?: cfnspec.schema.ResourceType) {
-    let changeImpact;
+    let changeImpact = types.ResourceImpact.NO_CHANGE;
 
     const spec = resourceSpec && resourceSpec.Properties && resourceSpec.Properties[key];
-    if (spec) {
+    if (spec && !deepEqual(oldV, newV)) {
       switch (spec.UpdateType) {
         case 'Immutable':
           changeImpact = types.ResourceImpact.WILL_REPLACE;