-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
refactor(core): simplify cross-stack reference detection #11254
Conversation
The current design of cross-stack references is needlessly complicated. It is based around being able to hook into the Token resolution mechanism, which is then used to mock-resolve the entire construct tree and remember all the `CfnReference` objects that were encountered while doing so. Then, we poke additional state into each of the encountered tokens so that *next time* that same token object is resolved in the scope of a consuming stack, we can output one of the poked values. This whole solution introduces the following complications: - Scope-based resolution: instead of resolving to a simple value, the token executes code that inspects the scope of the resolver to return a different value. - Time-based resolution: depending on when the token is being resolved, it returns something different. Both of these are the enemy of caching, and of consistency and predictable behavior. A better solution to always resolve to a consistent value that represents the intended special semantics. In that way, the token itself is trivial (all it needs to do is escape the type system and nothing else), and template language postprocessors implement the special features that are necessary. For references, the issue is that the same token must *sometimes* evaluate to a `{ Ref }` intrinsic and sometimes must evaluate to a `{ Fn::ImportValue }` intrinsic. In this PR, we extend the template language that Tokens render to from CloudFormation to **CdkFormation**, which is a superset of CloudFormation that also contains the intrinsic: ``` { "$Cdk::Ref": ["/path/to/node", "Attr"] } ``` This makes references be able to render to a stable value again, independent of scope and time. Finally, in `stack.resolve()` the function `replaceReferences()` is called which translates from CdkFormation -> CloudFormation, replacing `{ $Cdk::Ref }` with `{ Ref }` or `{ Fn::ImportValue }` as the case may be. It has not yet been fully carried through in this PR, but this change will be able to get rid of all the fields on `IResolveContext` and all the resolution hook machinery that make the Token implementation so complicated.
There competing PRs out there that introduce race conditions with this PR. Specifically, there is work to be done in the JSONinification PR. It should be clear that this PR is not production quality yet. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
*/ | ||
public static forPseudo(pseudoName: string, scope: Construct) { | ||
return CfnReference.singletonReference(scope, `Pseudo:${pseudoName}`, undefined, () => { | ||
const cfnIntrinsic = { Ref: pseudoName }; | ||
const cfnIntrinsic = { '$Cdk::Ref': [scope.node.path, pseudoName] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define a const
for "$CDK::Ref"
and use it everywhere. Also add a comment explaining what is it
const cfnIntrinsic = { '$Cdk::Ref': [scope.node.path, pseudoName] }; | |
const cfnIntrinsic = { '$CDK::Ref': [scope.node.path, pseudoName] }; |
if (typeof template === 'object' && template !== null) { | ||
const keys = Object.keys(template); | ||
|
||
if (keys.length === 1 && keys[0] === '$Cdk::Ref') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (keys.length === 1 && keys[0] === '$Cdk::Ref') { | |
if (keys.length === 1 && keys[0] === CDK_REF_KEY) { |
924c117
to
ebfd5f2
Compare
This PR has been deemed to be abandoned, and will be closed. Please create a new PR for these changes if you think this decision has been made in error. |
The current design of cross-stack references is needlessly
complicated.
It is based around being able to hook into the Token resolution
mechanism, which is then used to mock-resolve the entire construct tree
and remember all the
CfnReference
objects that were encountered whiledoing so.
Then, we poke additional state into each of the encountered tokens so
that next time that same token object is resolved in the scope of a
consuming stack, we can output one of the poked values.
This whole solution introduces the following complications:
the token executes code that inspects the scope of the resolver
to return a different value.
resolved, it returns something different.
Both of these are the enemy of caching, and of consistency and
predictable behavior.
A better solution to always resolve to a consistent value that
represents the intended special semantics. In that way, the token itself
is trivial (all it needs to do is escape the type system and nothing
else), and template language postprocessors implement the special
features that are necessary.
For references, the issue is that the same token must sometimes
evaluate to a
{ Ref }
intrinsic and sometimes must evaluate to a{ Fn::ImportValue }
intrinsic.In this PR, we extend the template language that Tokens render to
from CloudFormation to CdkFormation, which is a superset of
CloudFormation that also contains the intrinsic:
This makes references be able to render to a stable value again,
independent of scope and time.
Finally, in
stack.resolve()
the functionreplaceReferences()
is called which translates from CdkFormation -> CloudFormation,
replacing
{ $Cdk::Ref }
with{ Ref }
or{ Fn::ImportValue }
as the case may be.
It has not yet been fully carried through in this PR, but this change
will be able to get rid of all the fields on
IResolveContext
and allthe resolution hook machinery that make the Token implementation
so complicated.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license