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

Contains the following changes:

- Print table to STDOUT instead of STDERR. This ensures the diff output and the "confirm (y/n)" prompt that follows it don't interleave on the terminal, degrading readability.
- Control table width to not exceed the terminal width, so that it doesn't wrap and lead to a mess of lines.
- Switch back to the new version of the `table` library (instead of `cli-table`) which now has in-cell newline support, to get rid of the rendering bugs in `cli-table` that would occasionally eat newlines.
- Render resource `replace` impact as red instead of yellow, to indicate that data loss will be happening here (fixes #1458).
- Change replacement diffing in order to not trigger IAM changes dialog (fixes #1495).
- Print a message instead of an empty table if 'cdk context' has no information in it (fixes #1549).

Explanation: 

We used to modify the new target template in place, causing changes to
the logical ids of replaced resources, which would trigger downstream
changes, which would further trigger potential downstream replacements, etc. 

This would properly calculate the set of impacted resources, but would also lead to the modified logical ID popping up in the IAM diff, which was not desirable.

In this change, we do the same processing but we throw away the
template after all changes have been propagated, and only copy
the resultant property *statuses* onto the diff object that gets
returned to the user. This leads to the same displayed result
without the changes to the template actually propagating.

In the course of this modification, the diff classes have been changed
to also have objects in places of resources and properties that
didn't actually change (so that they could be modified in-place),
and diff objects have a boolean telling whether they are actual
changes or not.

Author: rix0rrr

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;