-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Labels
Comments
Poweranimal
added
bug
This issue is a bug.
needs-triage
This issue or PR still needs to be triaged.
labels
Feb 8, 2022
NGL321
added
p1
and removed
needs-triage
This issue or PR still needs to be triaged.
labels
Feb 14, 2022
otaviomacedo
added
@aws-cdk/core
Related to core CDK functionality
and removed
@aws-cdk/aws-appsync
Related to AWS AppSync
labels
Feb 25, 2022
Any update here? Happy to implement with some guidance (especially around how token resolution works) |
4 tasks
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
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*
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What is the problem?
Token
s that a are passed from one stack into another are not properly resolved in methods likeCfnResource.addPropertyOverride
.Reproduction Steps
What did you expect to happen?
The generated
StackB
should have anAWS::Lambda::Function
with thisVpcConfig
:What actually happened?
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, allToken
s, including the ones of theaddPropertyOverride
, get resolved properly.The text was updated successfully, but these errors were encountered: