fix(core): property overrides fail for references (#15018)

If a property override added an `IResolvable` as the value,
it would not be resolved,
but instead treated as a CloudFormation object when rendering the resource's properties,
which would lead to some weird behavior
(the properties of the `IResolvable` would be merged into the template).
This was noticed by a customer using CFN-Include.

As a solution, resolve the overrides before merging them with the properties of the resource.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

packages/@aws-cdk/cloudformation-include/test/test-templates/properties-not-in-cfn-spec.json

@@ -42,6 +42,11 @@
             "Type": "Api",
             "PropertyNotInCfnSchema": "unmodeled property in map"
           }
+        },
+        "ParentPropertyNotInCfnSchema": {
+          "ChildPropertyNotInCfnSchema": {
+            "Ref": "Bucket"
+          }
         }
       }
     }

packages/@aws-cdk/core/lib/cfn-resource.ts

@@ -7,9 +7,11 @@ import { CfnCreationPolicy, CfnDeletionPolicy, CfnUpdatePolicy } from './cfn-res
 import { Construct, IConstruct, Node } from 'constructs';
 import { addDependency } from './deps';
 import { CfnReference } from './private/cfn-reference';
+import { CLOUDFORMATION_TOKEN_RESOLVER } from './private/cloudformation-lang';
 import { Reference } from './reference';
 import { RemovalPolicy, RemovalPolicyOptions } from './removal-policy';
 import { TagManager } from './tag-manager';
+import { Tokenization } from './token';
 import { capitalizePropertyNames, ignoreEmpty, PostResolveToken } from './util';
 
 export interface CfnResourceProps {
@@ -326,7 +328,14 @@ export class CfnResource extends CfnRefElement {
               const hasDefined = Object.values(renderedProps).find(v => v !== undefined);
               resourceDef.Properties = hasDefined !== undefined ? renderedProps : undefined;
             }
-            return deepMerge(resourceDef, this.rawOverrides);
+            const resolvedRawOverrides = Tokenization.resolve(this.rawOverrides, {
+              scope: this,
+              resolver: CLOUDFORMATION_TOKEN_RESOLVER,
+              // we need to preserve the empty elements here,
+              // as that's how removing overrides are represented as
+              removeEmpty: false,
+            });
+            return deepMerge(resourceDef, resolvedRawOverrides);
           }),
         },
       };

packages/@aws-cdk/core/lib/private/resolve.ts

@@ -84,6 +84,13 @@ export interface IResolveOptions {
    * @default false
    */
   allowIntrinsicKeys?: boolean;
+
+  /**
+   * Whether to remove undefined elements from arrays and objects when resolving.
+   *
+   * @default true
+   */
+  removeEmpty?: boolean;
 }
 
 /**
@@ -120,6 +127,9 @@ export function resolve(obj: any, options: IResolveOptions): any {
     throw new Error('Unable to resolve object tree with circular reference. Path: ' + pathName);
   }
 
+  // whether to leave the empty elements when resolving - false by default
+  const leaveEmpty = options.removeEmpty === false;
+
   //
   // undefined
   //
@@ -188,7 +198,7 @@ export function resolve(obj: any, options: IResolveOptions): any {
 
     const arr = obj
       .map((x, i) => makeContext(`${i}`)[0].resolve(x))
-      .filter(x => typeof(x) !== 'undefined');
+      .filter(x => leaveEmpty || typeof(x) !== 'undefined');
 
     return arr;
   }
@@ -221,6 +231,9 @@ export function resolve(obj: any, options: IResolveOptions): any {
 
     // skip undefined
     if (typeof(value) === 'undefined') {
+      if (leaveEmpty) {
+        result[key] = undefined;
+      }
       continue;
     }
 
@@ -326,4 +339,4 @@ function tagResolvedValue(value: any, typeHint: ResolutionTypeHint): any {
 export function resolvedTypeHint(value: any): ResolutionTypeHint | undefined {
   if (typeof value !== 'object' || value == null) { return undefined; }
   return value[RESOLUTION_TYPEHINT_SYM];
-}
+}

packages/@aws-cdk/core/lib/token.ts

@@ -191,6 +191,7 @@ export class Tokenization {
       scope: options.scope,
       resolver: options.resolver,
       preparing: (options.preparing ?? false),
+      removeEmpty: options.removeEmpty,
     });
   }
 
@@ -265,6 +266,13 @@ export interface ResolveOptions {
    * @default false
    */
   readonly preparing?: boolean;
+
+  /**
+   * Whether to remove undefined elements from arrays and objects when resolving.
+   *
+   * @default true
+   */
+  readonly removeEmpty?: boolean;
 }
 
 /**

packages/@aws-cdk/core/test/resource.test.ts

@@ -420,6 +420,37 @@ nodeunitShim({
       test.done();
     },
 
+    'addPropertyOverride() allows assigning an attribute of a different resource'(test: Test) {
+      // GIVEN
+      const stack = new Stack();
+      const r1 = new CfnResource(stack, 'MyResource1', { type: 'AWS::Resource::Type' });
+      const r2 = new CfnResource(stack, 'MyResource2', { type: 'AWS::Resource::Type' });
+
+      // WHEN
+      r2.addPropertyOverride('A', {
+        B: r1.getAtt('Arn'),
+      });
+
+      // THEN
+      test.deepEqual(toCloudFormation(stack), {
+        Resources: {
+          MyResource1: {
+            Type: 'AWS::Resource::Type',
+          },
+          MyResource2: {
+            Type: 'AWS::Resource::Type',
+            Properties: {
+              A: {
+                B: { 'Fn::GetAtt': ['MyResource1', 'Arn'] },
+              },
+            },
+          },
+        },
+      });
+
+      test.done();
+    },
+
     'addOverride(p, null) will assign an "null" value'(test: Test) {
       // GIVEN
       const stack = new Stack();
@@ -513,7 +544,7 @@ nodeunitShim({
       test.done();
     },
 
-    'addDeletionOverride(p) and addPropertyDeletionOverride(pp) are sugar `undefined`'(test: Test) {
+    'addDeletionOverride(p) and addPropertyDeletionOverride(pp) are sugar for `undefined`'(test: Test) {
       // GIVEN
       const stack = new Stack();
 
@@ -904,4 +935,4 @@ class CustomizableResource extends CfnResource {
 /**
  * Because Resource is abstract
  */
-class TestResource extends Resource {}
+class TestResource extends Resource {}