-
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
AWSCL: External resource references #1546
Conversation
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.
design/external-references.md
Outdated
|
||
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. |
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.
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)
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.
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.
design/external-references.md
Outdated
|
||
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. |
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.
Only if the scope is the same. REQ6 made it sound as if it would be the same object in the entire app.
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.
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.
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.
@rix0rrr does that make sense?
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.
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.
|
||
return new ImportedApplicationLoadBalancer(ctx.scope, ctx.id, { | ||
loadBalancerArn: ctx.readString('LoadBalancerArn'), | ||
securityGroup: ec2.SecurityGroup.deserializeSecurityGroup(ctx.readObject('SecurityGroup')) |
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.
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?
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.
Let me know if the below examples clarify this a little
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.
Not sure which example you're referring to.
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.
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.
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 |
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.
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.
Yes! Love it.
Could you add an example of exports that would be produced by a complex construct?
@@ -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; |
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.
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.
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.
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
.
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.
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.
group.addUser(importedUser); | ||
``` | ||
|
||
Obvsiouly the world is a better place now. Thanks [@rix0rrr](https://github.com/users/rix0rrr). |
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.
🤓
|
||
## 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? |
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.
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
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.
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?
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.
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?
We differentiate two types of references: | ||
|
||
1. Reference a resource created through another CDK app. | ||
2. Reference a resource created by some other means. |
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.
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.
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.
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?
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.
"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.
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 { |
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.
Interestingly, the scope
argument will not be the scope
of the imported construct.
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.
That's true. Do you feel it's an issue?
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.
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).
```ts | ||
public static fromBucketArn(scope: Construct, bucketArn: string): IBucket { | ||
const stack = Stack.find(scope); | ||
const id = `fromBucketArn:${bucketArn}`; |
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.
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.
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.
I've relaxed the requirement to "it can be the same instance". Is that okay?
|
||
## 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. |
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.
Do we even need to require idempotency? What are the benefits?
## 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. |
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.
Implementation details. IMO doesn't warrant specifying here... But I agree generally we should use External
-based naming instead of Import
-based.
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.
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.
We are abandoning this PR (see #2307 for details on why) |
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
|
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
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.