Skip to content
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

Closed
wants to merge 9 commits into from
Closed

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Jan 14, 2019

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.
@eladb eladb requested a review from a team as a code owner January 14, 2019 18:11
* 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"

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.
Copy link
Contributor

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)

Copy link
Contributor Author

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.


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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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'))
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

design/external-references.md Show resolved Hide resolved
design/external-references.md Show resolved Hide resolved
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 15, 2019

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}.

Elad Ben-Israel added 2 commits January 15, 2019 16:26
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.
Copy link
Contributor

@rix0rrr rix0rrr left a 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?

design/external-references.md Outdated Show resolved Hide resolved
@@ -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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

design/external-references.md Show resolved Hide resolved
group.addUser(importedUser);
```

Obvsiouly the world is a better place now. Thanks [@rix0rrr](https://github.com/users/rix0rrr).
Copy link
Contributor

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?
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

design/external-references.md Outdated Show resolved Hide resolved
design/external-references.md Outdated Show resolved Hide resolved
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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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}`;
Copy link
Contributor

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.

Copy link
Contributor Author

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?

design/external-references.md Outdated Show resolved Hide resolved

## 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.
Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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"

Elad Ben-Israel added 2 commits January 17, 2019 23:23
Interfaces:
- ISerializable
- ISerializationContext
- IDeserializationContext

Stack:
- stack.exportString
- stack.importString
- stack.exportObject
- stack.importObject
@eladb eladb self-assigned this Feb 4, 2019
@eladb eladb changed the title rfc: external resource references External resource references Feb 11, 2019
@fulghum 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
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 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 eladb changed the title External resource references AWSCL: External resource references Mar 4, 2019
@eladb eladb added the parked label Apr 16, 2019
@eladb
Copy link
Contributor Author

eladb commented Apr 16, 2019

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

@eladb eladb closed this Apr 16, 2019
@eladb eladb deleted the benisrae/external-refs branch June 23, 2019 20:44
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 23, 2019
@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality contribution/core This is a PR that came from AWS. effort/large Large work item – several weeks of effort package/awscl Cross-cutting issues related to the AWS Construct Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exporting/importing across CDK apps
5 participants