fix(assert): Adjust assertion behavior to be stricter (#1289)

The SUPERSET matching mode would have aborted successfully upon finding
the first acceptable change (a change that is a CREATE), and not look at
subsequent changes, hence allowing any other type of update to pass as
long as it is detected later in the template. This change fixes that such that the
assertion will reject upon any change that is NOT a CREATE change.

Additionally, the default behavior of the `haveResource` matcher was changed
to not allow additional values in objects below the root of the expectation, which
makes it harder to write tests that inadvertently allow excessive IAM policies or
network security settings (amounts other things) to pass when the user intention
was for them to fail.

BREAKING CHANGE: the behavior change of `haveResource` can cause tests to
fail. If allowing extension of the expected values is the intended behavior, you can
switch to the `haveResourceLike` matcher instead, which exposes the previous
behavior.

Fixes awslabs/cdk-ops#186

packages/@aws-cdk/app-delivery/test/test.pipeline-deploy-stack-action.ts

@@ -153,6 +153,7 @@ export = nodeunit.testCase({
     });
     expect(pipelineStack).to(haveResource('AWS::IAM::Policy', {
       PolicyDocument: {
+        Version: '2012-10-17',
         Statement: [
           {
             Action: '*',
@@ -228,6 +229,7 @@ export = nodeunit.testCase({
     expect(pipelineStack).to(countResources('AWS::IAM::Policy', 3));
     expect(pipelineStack).to(haveResource('AWS::IAM::Policy', {
       PolicyDocument: {
+        Version: '2012-10-17',
         Statement: [
           {
             Action: [
@@ -324,7 +326,7 @@ function hasPipelineAction(expectedAction: any): (props: any) => boolean {
   return (props: any) => {
     for (const stage of props.Stages) {
       for (const action of stage.Actions) {
-        if (isSuperObject(action, expectedAction)) {
+        if (isSuperObject(action, expectedAction, [], true)) {
           return true;
         }
       }

packages/@aws-cdk/assert/lib/assertions/have-resource.ts

@@ -4,13 +4,28 @@ import { StackInspector } from "../inspector";
 /**
  * An assertion to check whether a resource of a given type and with the given properties exists, disregarding properties
  *
- * Properties can be:
- *
- * - An object, in which case its properties will be compared to those of the actual resource found
- * - A callable, in which case it will be treated as a predicate that is applied to the Properties of the found resources.
+ * @param resourceType the type of the resource that is expected to be present.
+ * @param properties   the properties that the resource is expected to have. A function may be provided, in which case
+ *                     it will be called with the properties of candidate resources and an ``InspectionFailure``
+ *                     instance on which errors should be appended, and should return a truthy value to denote a match.
+ * @param comparison   the entity that is being asserted against.
+ * @param allowValueExtension if properties is an object, tells whether values must match exactly, or if they are
+ *                     allowed to be supersets of the reference values. Meaningless if properties is a function.
+ */
+export function haveResource(resourceType: string,
+                             properties?: any,
+                             comparison?: ResourcePart,
+                             allowValueExtension: boolean = false): Assertion<StackInspector> {
+  return new HaveResourceAssertion(resourceType, properties, comparison, allowValueExtension);
+}
+
+/**
+ * Sugar for calling ``haveResources`` with ``allowValueExtension`` set to ``true``.
  */
-export function haveResource(resourceType: string, properties?: any, comparison?: ResourcePart): Assertion<StackInspector> {
-  return new HaveResourceAssertion(resourceType, properties, comparison);
+export function haveResourceLike(resourceType: string,
+                                 properties?: any,
+                                 comparison?: ResourcePart) {
+  return haveResource(resourceType, properties, comparison, true);
 }
 
 type PropertyPredicate = (props: any, inspection: InspectionFailure) => boolean;
@@ -22,10 +37,11 @@ class HaveResourceAssertion extends Assertion<StackInspector> {
 
   constructor(private readonly resourceType: string,
               private readonly properties?: any,
-              part?: ResourcePart) {
+              part?: ResourcePart,
+              allowValueExtension: boolean = false) {
     super();
 
-    this.predicate = typeof properties === 'function' ? properties : makeSuperObjectPredicate(properties);
+    this.predicate = typeof properties === 'function' ? properties : makeSuperObjectPredicate(properties, allowValueExtension);
     this.part = part !== undefined ? part : ResourcePart.Properties;
   }
 
@@ -78,10 +94,10 @@ function indent(n: number, s: string) {
 /**
  * Make a predicate that checks property superset
  */
-function makeSuperObjectPredicate(obj: any) {
+function makeSuperObjectPredicate(obj: any, allowValueExtension: boolean) {
   return (resourceProps: any, inspection: InspectionFailure) => {
     const errors: string[] = [];
-    const ret = isSuperObject(resourceProps, obj, errors);
+    const ret = isSuperObject(resourceProps, obj, errors, allowValueExtension);
     inspection.failureReason = errors.join(',');
     return ret;
   };
@@ -95,9 +111,9 @@ interface InspectionFailure {
 /**
  * Return whether `superObj` is a super-object of `obj`.
  *
- * A super-object has the same or more property values, recursing into nested objects.
+ * A super-object has the same or more property values, recursing into sub properties if ``allowValueExtension`` is true.
  */
-export function isSuperObject(superObj: any, obj: any, errors: string[] = []): boolean {
+export function isSuperObject(superObj: any, obj: any, errors: string[] = [], allowValueExtension: boolean = false): boolean {
   if (obj == null) { return true; }
   if (Array.isArray(superObj) !== Array.isArray(obj)) {
     errors.push('Array type mismatch');
@@ -111,7 +127,7 @@ export function isSuperObject(superObj: any, obj: any, errors: string[] = []): b
 
     // Do isSuperObject comparison for individual objects
     for (let i = 0; i < obj.length; i++) {
-      if (!isSuperObject(superObj[i], obj[i], [])) {
+      if (!isSuperObject(superObj[i], obj[i], [], allowValueExtension)) {
         errors.push(`Array element ${i} mismatch`);
       }
     }
@@ -128,7 +144,10 @@ export function isSuperObject(superObj: any, obj: any, errors: string[] = []): b
         continue;
       }
 
-      if (!isSuperObject(superObj[key], obj[key], [])) {
+      const valueMatches = allowValueExtension
+                         ? isSuperObject(superObj[key], obj[key], [], allowValueExtension)
+                         : isStrictlyEqual(superObj[key], obj[key]);
+      if (!valueMatches) {
         errors.push(`Field ${key} mismatch`);
       }
     }
@@ -139,6 +158,22 @@ export function isSuperObject(superObj: any, obj: any, errors: string[] = []): b
     errors.push('Different values');
   }
   return errors.length === 0;
+
+  function isStrictlyEqual(left: any, right: any): boolean {
+    if (left === right) { return true; }
+    if (typeof left !== typeof right) { return false; }
+    if (typeof left === 'object' && typeof right === 'object') {
+      if (Array.isArray(left) !== Array.isArray(right)) { return false; }
+      const allKeys = new Set<string>([...Object.keys(left), ...Object.keys(right)]);
+      for (const key of allKeys) {
+        if (!isStrictlyEqual(left[key], right[key])) {
+          return false;
+        }
+      }
+      return true;
+    }
+    return false;
+  }
 }
 
 /**

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

@@ -66,8 +66,9 @@ class StackMatchesTemplateAssertion extends Assertion<StackInspector> {
     case MatchStyle.SUPERSET:
       for (const key of Object.keys(diff.resources.changes)) {
         const change = diff.resources.changes[key]!;
-        return change.changeImpact === cfnDiff.ResourceImpact.WILL_CREATE;
+        if (change.changeImpact !== cfnDiff.ResourceImpact.WILL_CREATE) { return false; }
       }
+      return true;
     }
     throw new Error(`Unsupported match style: ${this.matchStyle}`);
   }

packages/@aws-cdk/assert/test/test.assertions.ts

@@ -48,6 +48,23 @@ passingExample('expect <stack> to match (no replaces) <template>', () => {
   const expected = {};
   expect(synthStack).to(matchTemplate(expected, MatchStyle.NO_REPLACES));
 });
+passingExample('expect <stack> to be a superset of <template>', () => {
+  const resourceType = 'Test::Resource';
+  const synthStack = synthesizedStack(stack => {
+    // Added
+    new TestResource(stack, 'NewResource', { type: 'AWS::S3::Bucket' });
+    // Expected
+    new TestResource(stack, 'TestResourceA', { type: resourceType });
+    new TestResource(stack, 'TestResourceB', { type: resourceType, properties: { Foo: 'Bar' } });
+  });
+  const expected = {
+    Resources: {
+      TestResourceA: { Type: 'Test::Resource' },
+      TestResourceB: { Type: 'Test::Resource', Properties: { Foo: 'Bar' } }
+    }
+  };
+  expect(synthStack).to(matchTemplate(expected, MatchStyle.SUPERSET));
+});
 passingExample('sugar for matching stack to a template', () => {
   const stack = new Stack();
   new TestResource(stack, 'TestResource', { type: 'Test::Resource' });
@@ -105,6 +122,24 @@ failingExample('expect <stack> to match (no replaces) <template>', () => {
   };
   expect(synthStack).to(matchTemplate(expected, MatchStyle.NO_REPLACES));
 });
+failingExample('expect <stack> to be a superset of <template>', () => {
+  const resourceType = 'Test::Resource';
+  const synthStack = synthesizedStack(stack => {
+    // Added
+    new TestResource(stack, 'NewResource', { type: 'AWS::S3::Bucket' });
+    // Expected
+    new TestResource(stack, 'TestResourceA', { type: resourceType });
+    // Expected, but has different properties - will break
+    new TestResource(stack, 'TestResourceB', { type: resourceType, properties: { Foo: 'Bar' } });
+  });
+  const expected = {
+    Resources: {
+      TestResourceA: { Type: 'Test::Resource' },
+      TestResourceB: { Type: 'Test::Resource', Properties: { Foo: 'Baz' } }
+    }
+  };
+  expect(synthStack).to(matchTemplate(expected, MatchStyle.SUPERSET));
+});
 
 // countResources
 

packages/@aws-cdk/assert/test/test.have-resource.ts

@@ -34,7 +34,59 @@ export = {
     }, /PropA/);
 
     test.done();
-  }
+  },
+
+  'haveResource value matching is strict by default'(test: Test) {
+    const synthStack = mkStack({
+      Resources: {
+        SomeResource: {
+          Type: 'Some::Resource',
+          Properties: {
+            PropA: {
+              foo: 'somevalue',
+              bar: 'This is unexpected, so the value of PropA doesn\'t strictly match - it shouldn\'t pass'
+            },
+            PropB: 'This property is unexpected, but it\'s allowed'
+          }
+        }
+      }
+    });
+
+    test.throws(() => {
+      expect(synthStack).to(haveResource('Some::Resource', {
+        PropA: {
+          foo: 'somevalue'
+        }
+      }));
+    }, /PropA/);
+
+    test.done();
+  },
+
+  'haveResource allows to opt in value extension'(test: Test) {
+    const synthStack = mkStack({
+      Resources: {
+        SomeResource: {
+          Type: 'Some::Resource',
+          Properties: {
+            PropA: {
+              foo: 'somevalue',
+              bar: 'Additional value is permitted, as we opted in'
+            },
+            PropB: 'Additional properties is always okay!'
+          }
+        }
+      }
+    });
+
+    expect(synthStack).to(haveResource('Some::Resource', {
+      PropA: {
+        foo: 'somevalue'
+      }
+    }, undefined, true));
+
+    test.done();
+  },
 };
 
 function mkStack(template: any) {
@@ -48,4 +100,4 @@ function mkStack(template: any) {
       region: 'test'
     }
   };
-}
+}

packages/@aws-cdk/assets/test/test.asset.ts

@@ -69,22 +69,26 @@ export = {
 
     expect(stack).to(haveResource('AWS::IAM::Policy', {
       PolicyDocument: {
-      Statement: [
-        {
-        Action: ["s3:GetObject*", "s3:GetBucket*", "s3:List*"],
-        Resource: [
-          { "Fn::Join": ["", ["arn:", {Ref: "AWS::Partition"}, ":s3:::", {Ref: "MyAssetS3Bucket68C9B344"}]] },
-          { "Fn::Join": ["",
-            [
-              "arn:", {Ref: "AWS::Partition"}, ":s3:::", {Ref: "MyAssetS3Bucket68C9B344"},
-              "/",
-              { "Fn::Select": [0, { "Fn::Split": [ "||", { Ref: "MyAssetS3VersionKey68E1A45D" }] }] },
-              "*"
+        Version: '2012-10-17',
+        Statement: [
+          {
+            Action: ["s3:GetObject*", "s3:GetBucket*", "s3:List*"],
+            Effect: 'Allow',
+            Resource: [
+              { "Fn::Join": ["", ["arn:", {Ref: "AWS::Partition"}, ":s3:::", {Ref: "MyAssetS3Bucket68C9B344"}]] },
+              { "Fn::Join": ["",
+                [
+                  "arn:", {Ref: "AWS::Partition"}, ":s3:::", {Ref: "MyAssetS3Bucket68C9B344"},
+                  "/",
+                  { "Fn::Select": [0, { "Fn::Split": [ "||", { Ref: "MyAssetS3VersionKey68E1A45D" }] }] },
+                  "*"
+                ]
+              ] }
             ]
-          ] }
+          }
         ]
       }
-    ]}}));
+    }));
 
     test.done();
   },

packages/@aws-cdk/aws-apigateway/test/test.lambda.ts

@@ -1,4 +1,4 @@
-import { expect, haveResource, not } from '@aws-cdk/assert';
+import { expect, haveResource, haveResourceLike, not } from '@aws-cdk/assert';
 import lambda = require('@aws-cdk/aws-lambda');
 import cdk = require('@aws-cdk/cdk');
 import { Test } from 'nodeunit';
@@ -118,7 +118,7 @@ export = {
     api.root.addMethod('GET', integ);
 
     // THEN
-    expect(stack).to(haveResource('AWS::ApiGateway::Method', {
+    expect(stack).to(haveResourceLike('AWS::ApiGateway::Method', {
       Integration: {
         Type: 'AWS'
       }