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;