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

PropertyOverrides: Tokens not properly resolved in cross stack scenarios when using property overrides #18882

Closed
Poweranimal opened this issue Feb 8, 2022 · 2 comments · Fixed by #24920
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. p2

Comments

@Poweranimal
Copy link

Poweranimal commented Feb 8, 2022

What is the problem?

Tokens that a are passed from one stack into another are not properly resolved in methods like CfnResource.addPropertyOverride.

Reproduction Steps

import * as cdk from "aws-cdk-lib";
import * as ec2 from "aws-cdk-lib/aws-ec2";
import * as lambda from "aws-cdk-lib/aws-lambda";

const app = new cdk.App();

const stackA = new cdk.Stack(app, "StackA");
const vpc = new ec2.Vpc(stackA, "Vpc");

const stackB = new cdk.Stack(app, "StackB");
const fn = new lambda.Function(stackB, "Lambda", {
    runtime: lambda.Runtime.NODEJS_14_X,
    handler: "index.js",
    code: lambda.Code.fromInline("hello"),
});
const cfnFn = (fn.node.defaultChild as cdk.CfnResource);
cfnFn.addPropertyOverride("VpcConfig", {
    subnetIds: vpc.selectSubnets({subnetType: ec2.SubnetType.PRIVATE_WITH_NAT}).subnetIds,
});

// Uncommenting the below code will make it work.
// vpc.selectSubnets({subnetType: ec2.SubnetType.PRIVATE_WITH_NAT}).subnets.forEach((v, i) => {
//     new cdk.CfnResource(stackB, `Res${i}`, {
//         type: "Custom::DeleteMe",
//         properties: {
//             SubnetId: v.subnetId,
//         }
//     });
// });

What did you expect to happen?

The generated StackB should have an AWS::Lambda::Function with this VpcConfig:

...
"VpcConfig": {
  "subnetIds": [
    {
      "Fn::ImportValue": "StackA:ExportsOutputRefVpcPrivateSubnet1Subnet536B997AFD4CC940"
    },
    {
      "Fn::ImportValue": "StackA:ExportsOutputRefVpcPrivateSubnet2Subnet3788AAA1380949A3"
    }
  ]
}
...

What actually happened?

...
"VpcConfig": {
  "subnetIds": [
    {
      "Ref": "VpcPrivateSubnet1Subnet536B997A"
    },
    {
      "Ref": "VpcPrivateSubnet2Subnet3788AAA1"
    }
  ]
}
...

CDK CLI Version

2.10.0 (build e5b301f)

Framework Version

No response

Node.js Version

v14.18.3

OS

Linux

Language

Typescript

Language Version

No response

Other information

There must be a simple workaround, because if one uses the selected subnets in StackB as properties for another resource inside the same Stack, all Tokens, including the ones of the addPropertyOverride, get resolved properly.

@Poweranimal Poweranimal added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 8, 2022
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Feb 8, 2022
@NGL321 NGL321 added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Feb 14, 2022
@otaviomacedo otaviomacedo added @aws-cdk/core Related to core CDK functionality and removed @aws-cdk/aws-appsync Related to AWS AppSync labels Feb 25, 2022
@johnameyer
Copy link

Any update here? Happy to implement with some guidance (especially around how token resolution works)

@peterwoodworth peterwoodworth changed the title PropertyOverrides: Tokens not properly resolved in cross stack scenarios PropertyOverrides: Tokens not properly resolved in cross stack scenarios when using property overrides May 15, 2023
@peterwoodworth peterwoodworth added p1.5 and removed p1 labels May 15, 2023
@otaviomacedo otaviomacedo added p2 and removed p1.5 labels May 22, 2023
@mergify mergify bot closed this as completed in #24920 May 23, 2023
mergify bot pushed a commit that referenced this issue May 23, 2023
closes #18882

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:

https://github.com/aws/aws-cdk/blob/810d736a8d20638e778c5773507f0edb12733a49/packages/%40aws-cdk/core/lib/private/refs.ts#L139-L140

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.

https://github.com/aws/aws-cdk/blob/810d736a8d20638e778c5773507f0edb12733a49/packages/%40aws-cdk/core/lib/private/resolve.ts#L270-L276

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.

https://github.com/aws/aws-cdk/blob/810d736a8d20638e778c5773507f0edb12733a49/packages/%40aws-cdk/core/lib/cfn-resource.ts#L374-L380

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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. p2
Projects
None yet
5 participants