AWSCL: External resource references #1546

eladb · 2019-01-14T18:11:32Z

now that we have automatic references of resources across stacks within
the same app, we are revisiting our model for referencing external resources.

this rfc proposes to add a serialization scheme for cdk constructs
and on top of it implement cross-app export/imports. it further describes
how external resources can be referenced via a physical resource name
such as an arn or a name, and proposes naming conventions for the
various elements.

fixes: #1095
related: #1496, #89

Pull Request Checklist

Testing
- Unit test added
- CLI change?: manually run integration tests and paste output as a PR comment
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

now that we have automatic references of resources across stacks within the same app, we are revisiting our model for referencing external resources. this rfc proposes to add a serialization scheme for cdk constructs and on top of it implement cross-app export/imports. it further describes how external resources can be referenced via a physical resource name such as an arn or a name, and proposes naming conventions for the various elements.

* Add serialization and export options * Rename `deserizlize` to `deserizlizeXxx` * Discuss how to address constraints in export names * Describe a few other applications * Add "implementation notes"

rix0rrr · 2019-01-15T09:18:16Z

design/external-references.md

+
+However, there are definitely use cases for being able to reference resources across environments, and we are also aware that some customers are reluctant to use CloudFormation import/exports due to the strong-coupling this mechanism creates between the stacks (for example, if an Output of one stack is being referenced by another stack, the original stack cannot be deleted).
+
+Another related use case is referencing resources by runtime code. An application's runtime code (AWS Lambda functions, CodeBuild projects, ECS containers, etc) needs to be able to interact with resources defined in CDK apps (e.g. read/write from a Amazon DynamoDB table, send messages to an SQS Queue, etc). This is a very similar use case to the cross-app references because basically the same information needs to be transferred between the apps.


I'm not sure I agree with this. For example, a Load Balancer has like 5 attributes that need to be transported for infra construction (among which a VPC and a list of security groups), but ultimately the runtime code is only interested in the name. I don't think the needs are similar at all (I will agree insofar as that "they're both ways of transporting information across process boundaries", but that's not saying a lot, that happens all the time)

I was thinking in line with @sam-goodwin’s model where you can basically access CDK constructs at runtime. Granted, in most cases you don’t need all runtime attributes, but if you have an IVpcNetwork object in your hands, then technically you are able to reference any of those inner resources at runtime. Maybe that’s not always needed (or even never needed), but if we can easily and transparently make these attributes available at runtime, then maybe that’s not a problem.

rix0rrr · 2019-01-15T09:23:44Z

design/external-references.md

+
+Since `deserializeXxx` will normally need to create new construct objects, the deserialization context should supply a consistent `scope` and `id` which can be used to instantiate a construct object that represents this object.
+
+Implementers of `deserializeXxx` should check if a construct with `id` already exists within `scope` and return it instead of instantiating and new object. This will satisfy REQ6.


Only if the scope is the same. REQ6 made it sound as if it would be the same object in the entire app.

It should be the same object for the entire stack and it is the deserializatiom context’s job to ensure by always passing in the same scope and id. See the code below for an example how they are used.

I am not super fond of that model, but couldn’t come up with something more elegant. Ideas are welcome.

@rix0rrr does that make sense?

Yes, sure. In my head the subobjects would be created under the deserialized object, but that's (1) impossible and also (2) unnecessary, so why even bother :).

I think this is a good solution.

rix0rrr · 2019-01-15T09:31:35Z

design/external-references.md

+
+    return new ImportedApplicationLoadBalancer(ctx.scope, ctx.id, {
+      loadBalancerArn: ctx.readString('LoadBalancerArn'),
+      securityGroup: ec2.SecurityGroup.deserializeSecurityGroup(ctx.readObject('SecurityGroup'))


How does readObject() know how to change scope and id for the inner object?

It has to, otherwise the SG and the LB will be created in the same scope and with the same ID. But I just don't see the implementation of readObject, how will it know what to change scope and id to?

Let me know if the below examples clarify this a little

Not sure which example you're referring to.

Basically readObject creates a new deserialization context that’s “derived” from the parent context that contains the scope (probably will always be the stack or some global scope maintained by the deserializer) and a unique id that’s derived from the fully qualified export name. Then, the deserizlize method (in this case “deserializeSecurityGroup”) will implement the same pattern depicted here to conditionally create a new instance.

design/external-references.md

rix0rrr · 2019-01-15T09:45:01Z

Might work, but the devil is in the details. And as mentioned, for constructs with lists (VPCs with a list of AZs, ECS Cluster with a list of Security Groups) we must have the exported values at synthesis time, so we can build a JavaScript list of the appropriate length.

That can never be satisfied with {Fn::ImportValue}.

@rix0rrr

Following feedback from @rix0rrr, change the default behavior of "importObject" to lookup and resolve export values during synthesis instead of during deployment. This will provide a much better experience for CDK users. Users can opt-in to use deploy-time resolution. Also, in order to maintain the strong-coupling behavior between stacks, we will synthesis a Metadata entry that contains an ImportValue for the export. This will create a binding between the stacks and protect against accidental deletion. This behavior can be opted-out by specifying "weak: true" when importing.

rix0rrr

Yes! Love it.

Could you add an example of exports that would be produced by a complex construct?

design/external-references.md

rix0rrr · 2019-01-15T14:35:52Z

design/external-references.md

@@ -144,18 +154,22 @@ The deserialization context is an object that implements the following interface
 interface IDeserializationContext {
  scope: Construct;
  id: string;
-  readString(key: string): string;
+  readString(key: string, options?: ReadStringOptions): string;
  readObject(key: string): IDeserializationContext;


We still need lists; I'd rather not leave it to users to have to manually split and join if we can do the same.

Deployment-time lists would work by {Fn::Join}/{Fn::Split}, synthesis-time lists could do the same eagerly.

We will need to escape the delimiter and I can't see a safe way to do this with deployment-time imports. Maybe we can say that readList() is always allowUnresolved: false.

We can also say "list values are not allowed to contain this string: ||" (or some other unlikely separator). I think this is defensible.

We don't handle arbitrary data, there's a limited set of things that pass around this way (IDs, ARNs, names).

If construct authors pass around data they're worried will contain these separators, they can always drop down to doing it themselves with readString().

Not dealing with it at the framework level doesn't make the need go away, it just pushes the concern to construct authors who (a) are all going to write similar code and (b) might write it poorly.

design/external-references.md

RomainMuller · 2019-01-15T17:38:28Z

design/external-references.md

+group.addUser(importedUser);
+```
+
+Obvsiouly the world is a better place now. Thanks [@rix0rrr](https://github.com/users/rix0rrr).


RomainMuller · 2019-01-15T17:40:07Z

design/external-references.md

+
+## Problem Statement
+
+So, "what seems to be the problem?", you ask. Well, we want more! The current model only supports references that occur between stacks within the same CDK app. What about the use case where I want to reference a resource that was created in a totally different app?


The source would read better if it was flowing on the 100 or 120 character mark. You cna probably leave it as-is now, but maybe keep in mind next time around :D

I usually wrap manually but wanted to play around without wrapping, see how the experience is. Why do you feel the source would read better with manual wrapping?

When I started reviewing I felt mildly bothered by the line length. I noted it as a readability issue, but the more I think about it, the more I think the real thing is I felt like it impairs my ability to submit a review comment "as granular" as I'd have liked (since the comment is on the line). Shorter lines mean more "geography-specific" commenting, I guess.

It's not a major thing though, I think by the end of the review I didn't feel so frustrated anyway, so maybe it's just a case of getting used to it?

RomainMuller · 2019-01-15T17:43:29Z

design/external-references.md

+We differentiate two types of references:
+
+1. Reference a resource created through another CDK app.
+2. Reference a resource created by some other means.


Maybe the "created by CloudFormation, but not via CDK" case should be expressly listed there. I feel there's a case where we can inform users how to "CDK-friendly-export" from their "native" CloudFormation stacks, and that's maybe a "cleaner" way out than for example referring to resources created by hand in the console.

to make sure I understand the proposal: you are saying there's a 3rd case for resources created by cloudformation but not via the CDK, so technically people can use the "import" mechanism to import resource physical attributes and create constructs from them, correct?

"CloudFormation-not-CDK" could possibly "manually" create the required Outputs/exports and refer to them from the CDK app. This would mean the naming protocol needs to be something that can be manually derived, ideally.

design/external-references.md

RomainMuller · 2019-01-15T19:46:27Z

design/external-references.md

+Since these methods need to create a new construct, they should utilize the resource's physical name as the construct ID, and also ensure idempotency. Here's an example:
+
+```ts
+public static fromBucketArn(scope: Construct, bucketArn: string): IBucket {


Interestingly, the scope argument will not be the scope of the imported construct.

That's true. Do you feel it's an issue?

Not a huge one, but I reckon it could be surprising. Maybe renaming the argument is enough, or maybe we want to mandate a Stack is passed (also, would require IConstruct.stack to be a thing, otherwise it'll be painful).

RomainMuller · 2019-01-15T19:51:14Z

design/external-references.md

+```ts
+public static fromBucketArn(scope: Construct, bucketArn: string): IBucket {
+  const stack = Stack.find(scope);
+  const id = `fromBucketArn:${bucketArn}`;


That's where I think we shouldn't try so hard to ensure single-reference.

const bucketA = Bucket.fromBucketArn(this, 'arn:aws:s3:::BucketName'); const bucketB = Bucket.fromBucketArn(this, stack.formatArn({ service: 's3', resource: 'BucketName', account: '', region: '' })); const bucketC = Bucket.fromBucketArn(this, 'BucketName'); bucketA === bucketB; // => true bucketA === bucketC; // => false, WTF?

To me, if you cannot guarantee all imports of a given entity result in the same instance, then the "best effort" attempt to reduce churn shouldn't be normative and doesn't warrant being documented so much.

I've relaxed the requirement to "it can be the same instance". Is that okay?

design/external-references.md

RomainMuller · 2019-01-15T19:56:35Z

design/external-references.md

+
+## Open issues/questions
+
+- [ ] Can we provide a nicer API for implementing idempotency? Seems like this is a repeating pattern. We can definitely implement something very nice that's not jsii-compatible, but that might be fine as long as non-jsii users can still use the same mechanism.


Do we even need to require idempotency? What are the benefits?

RomainMuller · 2019-01-15T19:57:35Z

design/external-references.md

+## Open issues/questions
+
+- [ ] Can we provide a nicer API for implementing idempotency? Seems like this is a repeating pattern. We can definitely implement something very nice that's not jsii-compatible, but that might be fine as long as non-jsii users can still use the same mechanism.
+- [ ] Consider renaming the `ImportXxx` classes to something that's not coupled with export/import. Maybe `ExternalXxx` or `ExistingXxx`. Those are internal classes, so it doesn't really matter, but still.


Implementation details. IMO doesn't warrant specifying here... But I agree generally we should use External-based naming instead of Import-based.

This is, again, mentioned only under "implementation notes"

Interfaces: - ISerializable - ISerializationContext - IDeserializationContext Stack: - stack.exportString - stack.importString - stack.exportObject - stack.importObject

a new version of scripts/foreach.sh which keeps a state file with the remaining work items. this allows resuming easily from where you left off. updated contribution guide with details.

eladb · 2019-04-16T14:00:50Z

We are abandoning this PR (see #2307 for details on why)

mergify · 2019-09-23T15:08:06Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

eladb requested a review from a team as a code owner January 14, 2019 18:11

Updates

d36b8af

* Add serialization and export options * Rename `deserizlize` to `deserizlizeXxx` * Discuss how to address constraints in export names * Describe a few other applications * Add "implementation notes"

rix0rrr reviewed Jan 15, 2019

View reviewed changes

Elad Ben-Israel added 2 commits January 15, 2019 16:26

fix reverse logic of "weak" in description

c823dce

rix0rrr approved these changes Jan 15, 2019

View reviewed changes

RomainMuller reviewed Jan 15, 2019

View reviewed changes

Elad Ben-Israel added 2 commits January 17, 2019 23:23

Add support for string lists

6c18910

Implement basic building blocks

8b8f1a7

Interfaces: - ISerializable - ISerializationContext - IDeserializationContext Stack: - stack.exportString - stack.importString - stack.exportObject - stack.importObject

eladb self-assigned this Feb 4, 2019

eladb changed the title ~~rfc: external resource references~~ External resource references Feb 11, 2019

fulghum added the effort/large Large work item – several weeks of effort label Feb 11, 2019

Elad Ben-Israel added 3 commits February 13, 2019 16:57

Merge remote-tracking branch 'origin/master' into benisrae/external-refs

f528780

build: new and improved stateful scripts/foreach.sh

b7f00c6

a new version of scripts/foreach.sh which keeps a state file with the remaining work items. this allows resuming easily from where you left off. updated contribution guide with details.

Merge remote-tracking branch 'origin/master' into benisrae/external-refs

f777f6a

Doug-AWS mentioned this pull request Feb 20, 2019

Split up "Pass in a Value from Another Stack" section #1823

Closed

This was referenced Feb 26, 2019

Best practices for cross-stack CDK to CFN and vice versa #603

Closed

S3 export/import description is off #652

Closed

Doug-AWS mentioned this pull request Feb 26, 2019

Missing topic about cross stack references #1525

Closed

eladb added package/awscl Cross-cutting issues related to the AWS Construct Library @aws-cdk/core Related to core CDK functionality labels Mar 4, 2019

eladb changed the title ~~External resource references~~ AWSCL: External resource references Mar 4, 2019

Doug-AWS mentioned this pull request Mar 5, 2019

[Question]: Adding an existing Security Group to an ALB #1948

Closed

rix0rrr mentioned this pull request Apr 9, 2019

We have to rename complex ImportProps fields #2206

Closed

eladb added the parked label Apr 16, 2019

eladb closed this Apr 16, 2019

eladb deleted the benisrae/external-refs branch June 23, 2019 20:44

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWSCL: External resource references #1546

AWSCL: External resource references #1546

eladb commented Jan 14, 2019 •

edited

Loading

rix0rrr Jan 15, 2019

eladb Jan 15, 2019

rix0rrr Jan 15, 2019

eladb Jan 15, 2019

eladb Jan 15, 2019

rix0rrr Jan 15, 2019

rix0rrr Jan 15, 2019

eladb Jan 15, 2019

rix0rrr Jan 15, 2019

eladb Jan 15, 2019

rix0rrr commented Jan 15, 2019

rix0rrr left a comment

rix0rrr Jan 15, 2019

eladb Jan 15, 2019

rix0rrr Jan 15, 2019

RomainMuller Jan 15, 2019

RomainMuller Jan 15, 2019

eladb Jan 15, 2019

RomainMuller Jan 15, 2019

RomainMuller Jan 15, 2019

eladb Jan 15, 2019

RomainMuller Jan 15, 2019

RomainMuller Jan 15, 2019

eladb Jan 15, 2019

RomainMuller Jan 15, 2019

RomainMuller Jan 15, 2019

eladb Jan 15, 2019

RomainMuller Jan 15, 2019

RomainMuller Jan 15, 2019

eladb Jan 15, 2019

eladb commented Apr 16, 2019

mergify bot commented Sep 23, 2019


		However, there are definitely use cases for being able to reference resources across environments, and we are also aware that some customers are reluctant to use CloudFormation import/exports due to the strong-coupling this mechanism creates between the stacks (for example, if an Output of one stack is being referenced by another stack, the original stack cannot be deleted).

		Another related use case is referencing resources by runtime code. An application's runtime code (AWS Lambda functions, CodeBuild projects, ECS containers, etc) needs to be able to interact with resources defined in CDK apps (e.g. read/write from a Amazon DynamoDB table, send messages to an SQS Queue, etc). This is a very similar use case to the cross-app references because basically the same information needs to be transferred between the apps.


		Since `deserializeXxx` will normally need to create new construct objects, the deserialization context should supply a consistent `scope` and `id` which can be used to instantiate a construct object that represents this object.

		Implementers of `deserializeXxx` should check if a construct with `id` already exists within `scope` and return it instead of instantiating and new object. This will satisfy REQ6.


		## Problem Statement

		So, "what seems to be the problem?", you ask. Well, we want more! The current model only supports references that occur between stacks within the same CDK app. What about the use case where I want to reference a resource that was created in a totally different app?


		## Open issues/questions

		- [ ] Can we provide a nicer API for implementing idempotency? Seems like this is a repeating pattern. We can definitely implement something very nice that's not jsii-compatible, but that might be fine as long as non-jsii users can still use the same mechanism.

AWSCL: External resource references #1546

AWSCL: External resource references #1546

Conversation

eladb commented Jan 14, 2019 • edited Loading

Pull Request Checklist

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

rix0rrr commented Jan 15, 2019

rix0rrr 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

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 commented Apr 16, 2019

mergify bot commented Sep 23, 2019

eladb commented Jan 14, 2019 •

edited

Loading