fix(core): property overrides sometimes don't work with intrinsics (#…

…20608)

When you add a property override, we merge it with any properties set
when the construct is initialized. The merge happens _after_ tokens are
resolved. For example, if we created a resource:

```ts
const resource = new CfnResource(this, 'Resource', {
  type: 'MyResourceType',
  properties: {
    prop1: Fn.ref('Param'),
  },
});
```

And the added an override for `prop1`:

```ts
resource.addPropertyOverride('prop1', Fn.ref('OtherParam'));
```

We would then perform a deep merge on the properties with a priority on
the overrides. The above example would work fine, eventually the merge
would get to the point where it was comparing two objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { Ref: 'OtherParam' } }
result = { prop1: { Ref: 'OtherParam' } }
```

If we instead added an override using a different intrinsic:

```ts
resource.addPropertyOverride('prop1', Fn.join('-', ['hello',
Fn.ref('Param')]));
```

Then we would get to the point in the merge where we were comparing two
different objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] } }
result = { prop1: {
  Ref: 'Param',
  'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] },
}}
```

This PR solves this issue by filtering out any CloudFormation
intrinsics. If the merge finds an intrinsic key in the target it "drops" the
intrinsic and takes whatever value is in the source (override).

fix #19971, #19447


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

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

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

@@ -496,6 +496,33 @@ export interface ICfnResourceOptions {
   metadata?: { [key: string]: any };
 }
 
+/**
+ * Object keys that deepMerge should not consider. Currently these include
+ * CloudFormation intrinsics
+ *
+ * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference.html
+ */
+
+const MERGE_EXCLUDE_KEYS: string[] = [
+  'Ref',
+  'Fn::Base64',
+  'Fn::Cidr',
+  'Fn::FindInMap',
+  'Fn::GetAtt',
+  'Fn::GetAZs',
+  'Fn::ImportValue',
+  'Fn::Join',
+  'Fn::Select',
+  'Fn::Split',
+  'Fn::Sub',
+  'Fn::Transform',
+  'Fn::And',
+  'Fn::Equals',
+  'Fn::If',
+  'Fn::Not',
+  'Fn::Or',
+];
+
 /**
  * Merges `source` into `target`, overriding any existing values.
  * `null`s will cause a value to be deleted.
@@ -513,6 +540,32 @@ function deepMerge(target: any, ...sources: any[]) {
         // object so we can continue the recursion
         if (typeof(target[key]) !== 'object') {
           target[key] = {};
+
+          /**
+           * If we have something that looks like:
+           *
+           *   target: { Type: 'MyResourceType', Properties: { prop1: { Ref: 'Param' } } }
+           *   sources: [ { Properties: { prop1: [ 'Fn::Join': ['-', 'hello', 'world'] ] } } ]
+           *
+           * Eventually we will get to the point where we have
+           *
+           *   target: { prop1: { Ref: 'Param' } }
+           *   sources: [ { prop1: { 'Fn::Join': ['-', 'hello', 'world'] } } ]
+           *
+           * We need to recurse 1 more time, but if we do we will end up with
+           *   { prop1: { Ref: 'Param', 'Fn::Join': ['-', 'hello', 'world'] } }
+           * which is not what we want.
+           *
+           * Instead we check to see whether the `target` value (i.e. target.prop1)
+           * is an object that contains a key that we don't want to recurse on. If it does
+           * then we essentially drop it and end up with:
+           *
+           *   { prop1: { 'Fn::Join': ['-', 'hello', 'world'] } }
+           */
+        } else if (Object.keys(target[key]).length === 1) {
+          if (MERGE_EXCLUDE_KEYS.includes(Object.keys(target[key])[0])) {
+            target[key] = {};
+          }
         }
 
         deepMerge(target[key], value);

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

@@ -734,6 +734,76 @@ describe('resource', () => {
 
     });
 
+    test('overrides allow overriding one intrinsic with another', () => {
+      // GIVEN
+      const stack = new Stack();
+
+      const resource = new CfnResource(stack, 'MyResource', {
+        type: 'MyResourceType',
+        properties: {
+          prop1: Fn.ref('Param'),
+        },
+      });
+
+      // WHEN
+      resource.addPropertyOverride('prop1', Fn.join('-', ['hello', Fn.ref('Param')]));
+      const cfn = toCloudFormation(stack);
+
+      // THEN
+      expect(cfn.Resources.MyResource).toEqual({
+        Type: 'MyResourceType',
+        Properties: {
+          prop1: {
+            'Fn::Join': [
+              '-',
+              [
+                'hello',
+                {
+                  Ref: 'Param',
+                },
+              ],
+            ],
+          },
+        },
+      });
+    });
+
+    test('overrides allow overriding a nested intrinsic', () => {
+      // GIVEN
+      const stack = new Stack();
+
+      const resource = new CfnResource(stack, 'MyResource', {
+        type: 'MyResourceType',
+        properties: {
+          prop1: Fn.importValue(Fn.sub('${Sub}', { Sub: 'Value' })),
+        },
+      });
+
+      // WHEN
+      resource.addPropertyOverride('prop1', Fn.importValue(Fn.join('-', ['abc', Fn.sub('${Sub}', { Sub: 'Value' })])));
+      const cfn = toCloudFormation(stack);
+
+      // THEN
+      expect(cfn.Resources.MyResource).toEqual({
+        Type: 'MyResourceType',
+        Properties: {
+          prop1: {
+            'Fn::ImportValue': {
+              'Fn::Join': [
+                '-',
+                [
+                  'abc',
+                  {
+                    'Fn::Sub': ['${Sub}', { Sub: 'Value' }],
+                  },
+                ],
+              ],
+            },
+          },
+        },
+      });
+    });
+
     describe('using mutable properties', () => {
 
       test('can be used by derived classes to specify overrides before render()', () => {