Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): allow override with cross-stack references #23382

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 3 additions & 93 deletions packages/@aws-cdk/core/lib/cfn-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@ import { CfnCreationPolicy, CfnDeletionPolicy, CfnUpdatePolicy } from './cfn-res
import { Construct, IConstruct, Node } from 'constructs';
import { addDependency, obtainDependencies, removeDependency } 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';
import { FeatureFlags } from './feature-flags';
import { ResolutionTypeHint } from './type-hints';
import { Intrinsic } from './private/intrinsic';

export interface CfnResourceProps {
/**
Expand Down Expand Up @@ -438,14 +437,7 @@ export class CfnResource extends CfnRefElement {
const hasDefined = Object.values(renderedProps).find(v => v !== undefined);
resourceDef.Properties = hasDefined !== undefined ? renderedProps : undefined;
}
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);
return deepMerge(resourceDef, this.rawOverrides);
}),
},
};
Expand Down Expand Up @@ -605,33 +597,6 @@ 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.
Expand All @@ -644,66 +609,11 @@ function deepMerge(target: any, ...sources: any[]) {

for (const key of Object.keys(source)) {
const value = source[key];
if (typeof(value) === 'object' && value != null && !Array.isArray(value)) {
if (typeof(value) === 'object' && value != null && !Array.isArray(value) && !(value instanceof Intrinsic)) {
// if the value at the target is not an object, override it with an
// 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] = {};
}
}

/**
* There might also be the case where the source is an intrinsic
*
* target: {
* Type: 'MyResourceType',
* Properties: {
* prop1: { subprop: { name: { 'Fn::GetAtt': 'abc' } } }
* }
* }
* sources: [ {
* Properties: {
* prop1: { subprop: { 'Fn::If': ['SomeCondition', {...}, {...}] }}
* }
* } ]
*
* We end up in a place that is the reverse of the above check, the source
* becomes an intrinsic before the target
*
* target: { subprop: { name: { 'Fn::GetAtt': 'abc' } } }
* sources: [{
* 'Fn::If': [ 'MyCondition', {...}, {...} ]
* }]
*/
if (Object.keys(value).length === 1) {
if (MERGE_EXCLUDE_KEYS.includes(Object.keys(value)[0])) {
target[key] = {};
}
}

deepMerge(target[key], value);
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/core/lib/resolvable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ export class DefaultTokenResolver implements ITokenResolver {
// The token might have returned more values that need resolving, recurse
resolved = context.resolve(resolved);
resolved = postProcessor.postProcess(resolved, context);
// resolve again since postProcess might have added more tokens (e.g. overriding)
resolved = context.resolve(resolved);
return resolved;
} catch (e) {
let message = `Resolution error: ${e.message}.`;
Expand Down
30 changes: 30 additions & 0 deletions packages/@aws-cdk/core/test/resource.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,36 @@ describe('resource', () => {
});
});

test('overrides allow cross-stack references', () => {
// GIVEN
const app = new App();
const stack1 = new Stack(app, 'Stack1');
const stack2 = new Stack(app, 'Stack2');
const res1 = new CfnResource(stack1, 'SomeResource1', {
type: 'Some::Resource1',
});
const res2 = new CfnResource(stack2, 'SomeResource2', {
type: 'Some::Resource2',
});

// WHEN
res2.addPropertyOverride('Key', res1.getAtt('Value'));

// THEN
expect(
app.synth().getStackByName(stack2.stackName).template?.Resources,
).toEqual({
SomeResource2: {
Properties: {
Key: {
'Fn::ImportValue': 'Stack1:ExportsOutputFnGetAttSomeResource1Value50DD3EF0',
},
},
Type: 'Some::Resource2',
},
});
});

describe('using mutable properties', () => {
test('can be used by derived classes to specify overrides before render()', () => {
const stack = new Stack();
Expand Down