feat(cdk): transparent cross-stack references #1436

rix0rrr · 2018-12-26T21:57:24Z

It is now no longer necessary to use export() and import() when
sharing constructs between two Stacks inside the same CDK app.
Instead, objects defined in one stack can be used directly in another
stack.

The CDK will detect when an attribute (such as an ARN, ID or URL) of
such an object is used in a different stack, and will automatically
create the required Export in the producing stack and insert the
corresponding Fn::ImportValue in the consuming stack.

BREAKING CHANGE: if you are using export() and import() to share
constructs between stacks, you can stop doing that, instead of
FooImportProps accept an IFoo directly on the consuming stack,
and use that object as usual. ArnUtils.fromComponents() and
ArnUtils.parse() have been moved onto Stack. All CloudFormation
pseudo-parameter (such as AWS::AccountId etc) are now also
accessible via Stack, as stack.accountId etc. resolve() has
been moved to this.node.resolve(). CloudFormationJSON.stringify()
has been moved to this.node.stringifyJson(). validate() now
should be protected.

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

Introduce context in resolve(), so that StackAwareToken can do something else if it's a Token from a different stack. Introduce a two-phase Freeze protocol which is a definite moment at which all lazy tokens must be resolved (so that the StackAwareTokens can build cross-Stack links). Change lock() into the freezing protocol, remove unlock as this was a speculative feature that is unused.

eladb

Pseudo values: I agree that they should just be members of Stack, and we should stop this little game to try and make stack (and tokens) cloudformation-agnostic. I think we've crossed that river long ago, and we should just accept it. When we do #798, so Construct is also stack-aware, the usage will be really nice: this.stack.region will just return the region (and can be even smart about whether we actually know the region as concrete value or use { Ref: AWS::Region } if not.

eladb · 2018-12-27T07:31:04Z

packages/@aws-cdk/cdk/lib/cloudformation/arn.ts

-    const account = components.account == null
-      ? new AwsAccountId()
-      : components.account;
+  public static fromComponents(components: ArnComponents, anchor?: Construct): string {


Should we also make these ARN utils part of Stack, so: this.stack.formatArn and this.stack.parseArn

I would honestly prefer Arn.format and Arn.parse - that makes sense to me intuitively. Looking for string formatting capability on Stack sounds weird.

The problem is that these methods now must be anchored within a stack, so it makes sense to have them as utilities of the stack in my mind.

ArnUtils.parse does not need the stack, only ArnUtils.fromComponents(). To maintain symmetry, we should put both on Stack then, but I also don't know that it's the most discoverable or appropriate place.

+1 for symmetry. As for discoverability - I'd argue that associating those with the stack might make them more discoverable than they are now, which is basically a free floating class that people need to learn about without any way to discover through IDE content assist.

Oh right. Symmetry makes it worth moving both to Stack. I'm still somewhat wary of the namespace pollution, but every mitigation I can think of risks being immensely confusing, so... Good with that.

packages/@aws-cdk/cdk/lib/core/tokens.ts

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

eladb · 2018-12-27T07:50:01Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

+  /**
+   * Export a Token value for use in another stack
+   */
+  public exportValue(tokenValue: Token, consumingStack: Stack): Token {


I wonder if this should be "The Way To Export" values from a stack, in which case we should probably expose the full surface area of "Export" such as type, export name, description, etc.

eladb · 2018-12-27T07:50:32Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

+   * Export a Token value for use in another stack
+   */
+  public exportValue(tokenValue: Token, consumingStack: Stack): Token {
+    if (this.env.account !== consumingStack.env.account || this.env.region !== consumingStack.env.region) {


My intuition is that this check should happen at the consuming side, not the exporting side and that export should not need to know who is going to consume it.

Exporting cross-account/region mutates the exporting stack differently than exporting in-account/in-region, so the exporting stack still needs to know what is up.

In-env: Export/Fn::ImportValue
Out-of-env: depending on how we choose to implement, either Output+IAM::Role/CFN::CustomResource (CFN native) or Output/Parameter+Metadata (toolkit assisted)

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

packages/@aws-cdk/cdk/lib/core/tokens.ts

- stack.accountId - stack.region - stack.partition - stack.urlSuffix etc. BREAKING CHANGE: pseudo parameters have disappeared and should now be retrieved as attributes of Stack. Some account-granting functions now have an additional parameter, 'anchor', in which you should pass the current scope (typically 'this').

eladb

I still feel there's room for simplification/generalization here... I know this might be a bigger change, but now is our time to rethink about this...

Why do we even need resolve as a free floating API at all besides for testing purposes?

I agree that we need two distinct phases, but I am wondering why we can't implement them generically without requiring each node to cooperate. Basically, it requires invoking toCloudFormation twice on each stack element: in the first pass, just to find tokens in the result (without substitution), and let them hook into the "relationship" (i.e. add exports to the producing stack). In the second pass, we just synthesize and substitute all tokens with their resolved values.

Find all stack elements in the tree
Invoke toCloudFormation on each element
Find all values with unresolved tokens in the template
If the value is "a pure token", resolve and replace the JSON node
If the value is a "string with tokens", resolve and produce an FnJoin statement (fail if the resolved value is not a string or an intrinsic function).

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

eladb · 2018-12-30T11:41:36Z

packages/@aws-cdk/aws-cloudwatch/lib/graph.ts

@@ -73,7 +73,7 @@ export class AlarmWidget extends ConcreteWidget {
      properties: {
        view: 'timeSeries',
        title: this.props.title,
-        region: this.props.region || new AwsRegion(),
+        region: this.props.region || new cdk.Token({ Ref: 'AWS::Region' }),


Any reason not to use stack.region here?

I wonder if we should still provide something like Aws.region and Aws.accountId for people to be able to explicitly use these pseudo parameters? Or even Stack.region and Stack.accountId

packages/@aws-cdk/cdk/lib/cloudformation/cloudformation-token.ts

eladb · 2018-12-30T11:44:09Z

packages/@aws-cdk/cdk/lib/cloudformation/pseudo.ts

+ * construct tree, this class takes an anchor parameter; the pseudo parameter
+ * values can be obtained as properties from an anchored object.
+ */
+export class Aws {


Do we really need this as well as the stack.xxx properties?

eladb · 2018-12-30T11:46:25Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

+  /**
+   * Export a Token value for use in another stack
+   */
+  public exportValue(tokenValue: Token, consumingStack: Stack): Token {


Can we somehow hide this method so it won't confuse people. Technically IIUC people are not supposed to use it directly.

eladb · 2018-12-30T12:07:10Z

packages/@aws-cdk/cdk/lib/cloudformation/cloudformation-token.ts

+  /**
+   * In a consuming context, potentially substitute this Token with a different one
+   */
+  public substituteToken(consumingStack: Stack): Token {


Would really love to explore the option to generalize this such that tokens can hook into their consumers (see #1433).

rix0rrr · 2019-01-03T10:47:06Z

Here's my plan for generalizing this so that it can also subsume our current bind() mechanism convention, and potentially other things in the future:

GIST

Tokens can (optionally) be generalized to References, by overriding a referenceType public property with an identifying string for the type of reference.
Constructs can declare what types of references they want to take action on by calling this.node.notifyReferenceType(refType) in the constructor. Usually, declaring that you consume a certain reference type goes hand-in-hand with more implementation responsibilities such as implementing a certain interface (or being of a certain type). Due to lack of generics, there is no way for us to enforce this at compile time, so we'll have to duck-type/runtime-check it.
If any reference of the given type(s) is/are used in the construct or one of its children, we'll call both:
- construct.notifyReferences(refToken), construct should override this method which does nothing by default
- refToken.notifyReferencedBy(construct), token should override this method which does nothing by default
- We'll give both the consuming and consumed side of the references a chance to act upon the linkage. We'll assume that at this point, the code will know what the types of objects involved are and will be able to downcast/invoke whatever it needs at both sides.

HOW DOES THIS HELP

Cross-stack references: StackAwareCloudFormationToken will be renamed CloudFormationReferenceToken, will be a referenceType "cloudFormationReference" that will be consumed by Stack. In the notifyReferencedBy will invoke the appropriate logic on its anchored Stack (exporting itself). In the consuming Stack.notifyReferneces the Stack ordering dependency will be recorded. During resolve() from a different stack it will resolve to a {Fn::ImportValue}.
Bind mechanism: we'll make new subclasses of Token that will do the logic that's currently in bind() in notifyReferencedBy(), such as adding permissions to the consumer.

CHANGES

Construct

resolve() goes here so that it's always clear in what context we are resolving a Token
Add node.notifyReferenceType() to request to be notified of a given reference.
Add protected notifyReferences() which can be overridden.
Add node.reportReference() to report references that cannot be autodetected because they don't end up in CFN output (happens in FunctionBase -> IEventSource).

Token

Add public referenceType?: string.
Add protected consumedBy() which can be overridden.

StackElement

collectReferences(); combines toCloudFormation() with a new feature of resolve() to collect all Tokens that are references, to enable implicit reporting of references that end up in CFN output (90% use case).

App

Add resolveReferences() as an additional synthesis step.

eladb · 2019-01-03T11:01:02Z

Not sure I understand the motivation behind constructs notifying about which reference types they consume instead of notifying about concrete references. What I imagined is basically a virtual method on Construct that allows a construct to indicate which references it consumed (implemented automatically by StackElement to collect references from toCloudFormation output).

I am also not clear as to why we need the "double notification". If the construct is already notifying the framework that it's consuming a reference, why do we need the system to also notify the construct about the same thing? Wouldn't it be sufficient to just notify the reference that it's being consumed in some scope?

Some alternative naming preferences:

CloudFormationReferenceToken => CfnReference (see feat: deprecate "cloudformation" namespace in favor of "CfnXxx" #1311 core: prefix all "DOM" (L1) CloudFormation classes with "Cfn" #1462)
token.notifyConsumedBy => token.bind

rix0rrr · 2019-01-03T11:03:14Z

I guess this confusion is caused by naming. One is:

Upon creation, I'll tell the system which types of references I'm interested in.

The other one is:

Later on, during synthesis, the system will tell me which actual reference instances were found.

eladb

Wow! Looks great.

packages/@aws-cdk/aws-apigateway/lib/deployment.ts

eladb · 2019-01-08T07:05:09Z

packages/@aws-cdk/aws-apigateway/lib/deployment.ts

    // if hash components were added to the deployment, we use them to calculate
    // a logical ID for the deployment resource.
    if (this.hashComponents.length === 0) {
      this.lazyLogicalId = this.originalLogicalId;
    } else {
      const md5 = crypto.createHash('md5');
      this.hashComponents
-        .map(c => cdk.resolve(c))
+        .map(c => this.node.resolve(c))


I am wondering if it would make sense to only expose resolve during prepare and synthesis by passing in something like SynthesisContext to the prepare method? This will sure people don't abuse this capability before the tree is ready.

That's interesting - /me likey.

Are we prepared to have unresolved tokens in error messages?

https://github.com/awslabs/aws-cdk/blob/3a0d5b5cf98e3cbcbd6139ef1e07135b4bece5f7/packages/@aws-cdk/aws-kms/lib/key.ts#L71

https://github.com/awslabs/aws-cdk/blob/3a0d5b5cf98e3cbcbd6139ef1e07135b4bece5f7/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts#L419

Not entirely clear how this should work if it can't resolve:

https://github.com/awslabs/aws-cdk/blob/3a0d5b5cf98e3cbcbd6139ef1e07135b4bece5f7/packages/@aws-cdk/aws-s3/lib/util.ts#L35

The lazy arguments to Token will need to take the SynthesisContext as an argument as well so they can resolve or JSONify inner structures:

https://github.com/awslabs/aws-cdk/blob/3a0d5b5cf98e3cbcbd6139ef1e07135b4bece5f7/packages/@aws-cdk/aws-ecr/lib/repository.ts#L129

https://github.com/awslabs/aws-cdk/blob/3a0d5b5cf98e3cbcbd6139ef1e07135b4bece5f7/packages/@aws-cdk/aws-logs/lib/cross-account-destination.ts#L98

All still in favor?

@rix0rrr thanks for the due diligence. It's so much nicer to have these discussions over actual examples.

Let's punt this change for now for the sake of progress, but I think we should still explore this. The examples you provided are interesting and actually expose some issues, which is precisely

Error messages: the practice of JSON.stringify(resolve) feels like a lucky hack anyway, and only works when specific construct authors are aware that a value might be a token. I think a more robust solution to this problem would be to improve the string representation of tokens of resource references (GetAtt/Refs) in strings so they will be less intimidating. For instance, one can argue that something like ${{ "Fn::GetAtt": [ Logical, "Attribute" ]}} can be used for GetAtt tokens (and this can be used as the map key to the token object.

I'd argue that this use case should actually query unresolved and then just type-check. There is no need to actually resolve the values:

https://github.com/awslabs/aws-cdk/blob/3a0d5b5cf98e3cbcbd6139ef1e07135b4bece5f7/packages/@aws-cdk/aws-s3/lib/util.ts#L35

As for lazy evaluation: totally agree. Lazy evaluation is basically a hook into the synthesis phase. If you recall, I proposed in the past to stop allowing "free floating" lazy tokens have have something like: this.node.onSynthesize(synthesisContext => bla), so yes, passing synthesisContext to such a lazy handler makes total sense to me.

packages/@aws-cdk/aws-apigateway/lib/deployment.ts

packages/@aws-cdk/aws-apigateway/lib/integrations/aws.ts

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

packages/@aws-cdk/cdk/lib/core/tokens/token.ts

packages/@aws-cdk/cdk/lib/runtime.ts

…son()

…ferences

eladb · 2019-01-09T08:18:57Z

Hallelujah!

rix0rrr and others added 9 commits September 29, 2018 14:17

Fix tag manager tests to use resolve(token) instead of token.resolve()

7b97ebb

Merge branch 'master' into huijbers/x-stack-references

929b003

Remove concept of freezing

d02e16d

Add explicit preprocessor step for references

ac062fc

Get rid of "context" parameter for Token resolution

935fd82

Make refs and attributes Stack-aware

9f40aa2

Create tests for things I'll have to do later

c1d4dc1

Fix core tests

5d85318

rix0rrr requested review from RomainMuller, eladb and sam-goodwin December 26, 2018 21:57

rix0rrr requested review from skinny85, SoManyHs and a team as code owners December 26, 2018 21:57

eladb reviewed Dec 27, 2018

View reviewed changes

eladb mentioned this pull request Dec 27, 2018

feat: idiomize cloudformation intrinsics and pseudo parameters #1428

Merged

4 tasks

rix0rrr added 8 commits December 28, 2018 11:49

Merge branch 'master' into huijbers/x-stack-references

4219539

Remove CloudFormationToken

d8876bf

Rename StackAwareCloudFormationToken -> StackAwareToken

0a724b4

CloudFormationConcat is no longer configurable, but just the default

445805f

Remove sourceStack argument to substitution

cc928f8

Make JSII happy

3cb539f

Add cdk deployment ordering; WIP - fighting with dependency cycles

73353c0

eladb reviewed Dec 30, 2018

View reviewed changes

Reorganizing to get rid of cycles, add integ test

f161a4d

Rico Huijbers and others added 2 commits January 7, 2019 17:09

Fix examples

5978c6b

Merge branch 'master' into huijbers/x-stack-references

996f998

eladb reviewed Jan 8, 2019

View reviewed changes

rix0rrr added 7 commits January 8, 2019 10:41

Review comments

e0ec521

Fix compilation

3a0d5b5

Review comments

1d5388d

Undo more refactoring breakage

3ee48f7

Fold ArnUtils.fromComponents() and ArnUtils.parse() into Stack

d1b3c22

Missing commit

85cc4d2

Replace construct library's use of CloudFormationJSON with stringifyJ…

8e64bc2

…son()

eladb approved these changes Jan 8, 2019

View reviewed changes

rix0rrr and others added 8 commits January 8, 2019 15:15

Remove additional scope argument for IAM policies

5694b9c

Merge remote-tracking branch 'origin/master' into huijbers/x-stack-re…

d1a643e

…ferences

arnFromComponents() -> formatArn()

12854e6

Move logic for collecting tokens to token layer

c7d4314

validate() is now protected

d2a656f

Move protected() methods down in source

fdeaa55

Call validateTree() instead of protected validate() in tests

00cbe07

Fix test

0346946

rix0rrr self-assigned this Jan 8, 2019

Fix another test

34bfb34

rix0rrr merged commit d7371f0 into master Jan 9, 2019

rix0rrr deleted the huijbers/x-stack-references branch January 9, 2019 08:17

eladb mentioned this pull request Jan 9, 2019

"import" is a reserved word in Java #89

Closed

RomainMuller mentioned this pull request Jan 30, 2019

Stacks are getting deployed in Alphabetical order #1635

Closed

eladb changed the title ~~feat(cdk): transparent cross-stack references (WIP)~~ feat(cdk): transparent cross-stack references Feb 11, 2019

eladb mentioned this pull request Feb 11, 2019

Allow Imported Lambda to create Permission #1709

Closed

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

SomayaB mentioned this pull request Sep 22, 2020

Docs lack examples or explanation of how to export and import values between stacks #10480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cdk): transparent cross-stack references #1436

feat(cdk): transparent cross-stack references #1436

rix0rrr commented Dec 26, 2018 •

edited

eladb left a comment

eladb Dec 27, 2018

RomainMuller Dec 27, 2018

eladb Dec 27, 2018

rix0rrr Dec 28, 2018

eladb Dec 30, 2018

RomainMuller Jan 8, 2019

eladb Dec 27, 2018

eladb Dec 27, 2018

rix0rrr Dec 28, 2018

eladb left a comment

eladb Dec 30, 2018

eladb Jan 8, 2019

eladb Dec 30, 2018

eladb Dec 30, 2018

eladb Dec 30, 2018

rix0rrr commented Jan 3, 2019

eladb commented Jan 3, 2019

rix0rrr commented Jan 3, 2019 •

edited

eladb left a comment

eladb Jan 8, 2019

RomainMuller Jan 8, 2019

rix0rrr Jan 8, 2019

eladb Jan 8, 2019

eladb commented Jan 9, 2019

feat(cdk): transparent cross-stack references #1436

feat(cdk): transparent cross-stack references #1436

Conversation

rix0rrr commented Dec 26, 2018 • edited

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jan 3, 2019

eladb commented Jan 3, 2019

rix0rrr commented Jan 3, 2019 • edited

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Jan 9, 2019

rix0rrr commented Dec 26, 2018 •

edited

rix0rrr commented Jan 3, 2019 •

edited