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

tmokmss · 2023-04-04T07:41:33Z

The problem is described in the original issue, and below is what I found as the root cause and how it can be fixed.

Previously when we used a cross-stack reference in override, it was not resolved as an Fn::ImportValue.

To make it an Fn::ImportValue, we need to get every token in an app and find references (tokens that references resources outside of its stack) from them. The related code is here:

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

Lines 139 to 140 in 810d736

    
           function findAllReferences(root: IConstruct) { 
        
             const result = new Array<{ source: CfnElement, value: CfnReference }>();

To get all the tokens in an app, we use RememberingTokenResolver, which remembers every token it has found during resolution. So basically this resolver must be used on every resolution to find all the tokens.

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

Lines 270 to 276 in 810d736

    
           export class RememberingTokenResolver extends DefaultTokenResolver { 
        
             private readonly tokensSeen = new Set<IResolvable>(); 
        
             public resolveToken(t: IResolvable, context: IResolveContext, postProcessor: IPostProcessor) { 
        
               this.tokensSeen.add(t); 
        
               return super.resolveToken(t, context, postProcessor); 
        
             }

However, the resolver is not used specifically when we resolve tokens in raw overrides. Actually the current interface of postProcess function of PostResolveToken class makes It difficult to use an external resolver.

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

Lines 374 to 380 in 810d736

    
           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, 
        
           });

That is why, in this PR, we move the resolution process outside of the postProcess, allowing to resolve tokens in raw overrides with the RememberingTokenResolver resolver.

This change also simplifies the current implementation of deepMerge as a side product.

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

tmokmss · 2023-04-04T07:46:07Z

Exemption Request

Existing integration tests already cover the changes of this PR. Also, aws-cdk-lib/core does not have any existing integ test.

TheRealAmazonKendra

Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests).

From the context you've provided I don't totally understand the problem being solved or how your solution fixes it. Please update the body of your PR to provide this context.

We also definitely need an integration test on this. While we don't have any in core, you can pick any module to put this into practice with. We should also run this through the testing pipeline so I'm going to add that label here as well.

TheRealAmazonKendra · 2023-04-05T17:54:17Z

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

@@ -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)) {


instanceof is not safe in all contexts. See this PR for how we do this in other places.

I don't understand why instanceof should not be used here. The PR has no description and it is not even merged. Could you explain a bit more in detail?

Essentially instanceOf cannot be guaranteed to work reliably since it is
possible to have multiple copies of a library installed and instanceOf will view
each copy as a different object.

This PR has some additional context

The method we are using instead is to create an isIntrinsic method

tmokmss · 2023-04-06T00:19:28Z

Clarification Request

Why I should not use instanceof?

tmokmss · 2023-04-06T02:52:57Z

Exemption Request

Currently integ-runner does not seem to support multiple-stacks deployment, which makes it difficult to test this scenario.

Also, this change only affects synth phase, not deployment phase, so I believe we don't need any integration test here.

ERROR      test/integ.cfn-resource 4.098s
      "cdk-integ" can only operate on apps with a single stack.

  If your app has multiple stacks, specify which stack to select by adding this to your test source:

      /// !cdk-integ STACK ...

  Available stacks: Stack1 Stack2 (wildcards are also supported)

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

tmokmss · 2023-04-15T08:23:07Z

I just changed a random integ test to keep the PR open. I'll revert it after a review.

aws-cdk-automation · 2023-04-27T00:08:32Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

gitpod-io · 2023-04-27T01:31:22Z

Pull request has been modified.

corymhall

Couple of questions

corymhall · 2023-05-01T13:54:07Z

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

@@ -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)) {


Essentially instanceOf cannot be guaranteed to work reliably since it is
possible to have multiple copies of a library installed and instanceOf will view
each copy as a different object.

This PR has some additional context

The method we are using instead is to create an isIntrinsic method

corymhall · 2023-05-01T14:02:03Z

packages/aws-cdk-lib/core/lib/resolvable.ts

@@ -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);


It looks like we are passing the context through to the postProcess, but it
isn't making its way all the way through to the processor. Is there a way we
could pass it all the way through and use that to replace the
Tokenization.resolve function?

Thanks, I'll check that way

@corymhall
I investigated it, and please correct me if my understanding is wrong 🙏

If we take that approach, we can use context.resolve here instead of Tokenization.resolve, and it can also settle the issue.

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

Lines 441 to 447 in 04323c4

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,

});

The disadvantage is that, to pass the context, we have to change the signature of _toCloudFormation function something like _toCloudFormation(context?) (context must be optional because currently not all the callers have context available.) It requires changes in many files and will complicate the code. Given that I don't think it is a preferred approach.

Isn't it simpler and more reasonable to resolve tokens outside of the postProcess function?

I was thinking of adding it to the PostResolveToken processor method as an optional property

aws-cdk/packages/aws-cdk-lib/core/lib/util.ts

Line 82 in b1e649a

constructor(value: any, private readonly processor: (x: any) => any) {

.

So then it would be something like

[this.logicalId]: new PostResolveToken({ ... }, (resourceDef, context) => {

I like Cory's solution. PostResolveToken is private and so is free to have its API changed. This will (hopefully?) cleanly achieve the desired solution.

@corymhall @rix0rrr
Thanks, I think I have to study about CDK tokens more 😄 Will try the approach anyway.

One question is, why not resolve tokens after the deepMerge? (i.e. my original approach 905d5ed)
It can remove some hacks currently implemented like removeEmpty: false or MERGE_EXCLUDE_KEYS, and simplify the existing logic.

Is it performance-wise reason (to reduce recursion), or is there any edge case not to work properly that I am not aware of?

corymhall

Meant to request changes.

This reverts commit 13044b3.

This reverts commit e0ee5b5.

Pull request has been modified.

This reverts commit 458e6b4.

This reverts commit 905d5ed.

aws-cdk-automation · 2023-05-23T00:08:16Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

tmokmss · 2023-05-23T03:06:58Z

Hi @corymhall could you review this again? Idk why it's in CHANGES REQUESTED state but it's ready for review.

mergify · 2023-05-23T09:34:57Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-05-23T10:01:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: cdae366
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-05-23T10:01:58Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

tmokmss and others added 5 commits December 17, 2022 17:38

wip

d73508f

fix

0e807eb

Update resolvable.ts

f82f941

Merge branch 'main' into fix_cross_stack_override

11a8ac0

Merge branch 'main' into fix_cross_stack_override

b656fdd

github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK p2 labels Apr 4, 2023

aws-cdk-automation requested a review from a team April 4, 2023 07:41

aws-cdk-automation previously requested changes Apr 4, 2023

View reviewed changes

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Apr 4, 2023

github-actions bot added bug This issue is a bug. p1 and removed p2 labels Apr 4, 2023

TheRealAmazonKendra previously requested changes Apr 5, 2023

View reviewed changes

TheRealAmazonKendra removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Apr 5, 2023

Merge branch 'main' into fix_cross_stack_override

f6d5465

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Apr 6, 2023

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Apr 6, 2023

tmokmss added 2 commits April 15, 2023 16:45

Merge branch 'main' into fix_cross_stack_override

c751a54

add integ diff

e0ee5b5

tmokmss requested a review from TheRealAmazonKendra April 27, 2023 00:56

Update cfn-resource.ts

13044b3

corymhall approved these changes May 1, 2023

View reviewed changes

corymhall previously requested changes May 1, 2023

View reviewed changes

corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label May 1, 2023

tmokmss marked this pull request as draft May 2, 2023 00:21

tmokmss added 3 commits May 3, 2023 09:01

Revert "Update cfn-resource.ts"

c390b52

This reverts commit 13044b3.

Revert "add integ diff"

423fb74

This reverts commit e0ee5b5.

revert

458e6b4

tmokmss and others added 4 commits May 7, 2023 18:30

Revert "revert"

905d5ed

This reverts commit 458e6b4.

Revert "Revert "revert""

7fbc30d

This reverts commit 905d5ed.

propagate context

e971d8b

Merge branch 'main' into fix_cross_stack_override

78d0205

tmokmss marked this pull request as ready for review May 7, 2023 10:06

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 7, 2023

tmokmss added 2 commits May 7, 2023 19:12

Update resolvable.ts

15f59ee

Update cfn-resource.ts

99e4ec5

Merge branch 'main' into fix_cross_stack_override

ee4d400

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 23, 2023

tmokmss requested a review from corymhall May 23, 2023 03:05

corymhall approved these changes May 23, 2023

View reviewed changes

Merge branch 'main' into fix_cross_stack_override

cdae366

mergify bot merged commit 1135356 into aws:main May 23, 2023

tmokmss deleted the fix_cross_stack_override branch May 24, 2023 00:07

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tmokmss commented Apr 4, 2023 •

edited

Loading

aws-cdk-automation left a comment

tmokmss commented Apr 4, 2023 •

edited

Loading

TheRealAmazonKendra left a comment

TheRealAmazonKendra Apr 5, 2023

tmokmss Apr 6, 2023 •

edited

Loading

corymhall May 1, 2023

tmokmss commented Apr 6, 2023

tmokmss commented Apr 6, 2023

tmokmss commented Apr 15, 2023

aws-cdk-automation commented Apr 27, 2023

gitpod-io bot commented Apr 27, 2023

corymhall left a comment

corymhall May 1, 2023

corymhall May 1, 2023

tmokmss May 2, 2023

tmokmss May 3, 2023 •

edited

Loading

corymhall May 4, 2023

rix0rrr May 4, 2023

tmokmss May 6, 2023 •

edited

Loading

corymhall left a comment

aws-cdk-automation commented May 23, 2023

tmokmss commented May 23, 2023 •

edited

Loading

mergify bot commented May 23, 2023

aws-cdk-automation commented May 23, 2023

mergify bot commented May 23, 2023

	function findAllReferences(root: IConstruct) {
	const result = new Array<{ source: CfnElement, value: CfnReference }>();

	export class RememberingTokenResolver extends DefaultTokenResolver {
	private readonly tokensSeen = new Set<IResolvable>();

	public resolveToken(t: IResolvable, context: IResolveContext, postProcessor: IPostProcessor) {
	this.tokensSeen.add(t);
	return super.resolveToken(t, context, postProcessor);
	}

	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,
	});

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

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

Conversation

tmokmss commented Apr 4, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

tmokmss commented Apr 4, 2023 • edited Loading

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Apr 5, 2023

Choose a reason for hiding this comment

tmokmss Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

corymhall May 1, 2023

Choose a reason for hiding this comment

tmokmss commented Apr 6, 2023

tmokmss commented Apr 6, 2023

tmokmss commented Apr 15, 2023

aws-cdk-automation commented Apr 27, 2023

gitpod-io bot commented Apr 27, 2023

corymhall left a comment

Choose a reason for hiding this comment

corymhall May 1, 2023

Choose a reason for hiding this comment

corymhall May 1, 2023

Choose a reason for hiding this comment

tmokmss May 2, 2023

Choose a reason for hiding this comment

tmokmss May 3, 2023 • edited Loading

Choose a reason for hiding this comment

corymhall May 4, 2023

Choose a reason for hiding this comment

rix0rrr May 4, 2023

Choose a reason for hiding this comment

tmokmss May 6, 2023 • edited Loading

Choose a reason for hiding this comment

corymhall left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented May 23, 2023

tmokmss commented May 23, 2023 • edited Loading

mergify bot commented May 23, 2023

aws-cdk-automation commented May 23, 2023

AWS CodeBuild CI Report

mergify bot commented May 23, 2023

tmokmss commented Apr 4, 2023 •

edited

Loading

tmokmss commented Apr 4, 2023 •

edited

Loading

tmokmss Apr 6, 2023 •

edited

Loading

tmokmss May 3, 2023 •

edited

Loading

tmokmss May 6, 2023 •

edited

Loading

tmokmss commented May 23, 2023 •

edited

Loading