feat(aws-ec2): add VPC context provider #1168

rix0rrr · 2018-11-14T10:48:39Z

Add a context provider for looking up existing VPCs in an account. This
is useful if the VPC is defined outside of your CDK app, such as in a
different CDK app, by hand or in a CloudFormation template.

Addresses some need of #1095.

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

Add a context provider for looking up existing VPCs in an account. This is useful if the VPC is defined outside of your CDK app, such as in a different CDK app, by hand or in a CloudFormation template. Addresses some need of #1095.

eladb · 2018-11-14T12:13:58Z

packages/@aws-cdk/aws-ec2/README.md

+#### Sharing VPCs across stacks
+
+If you are creating multiple `Stack`s inside the same CDK application,
+you can reuse a VPC from one Stack in another by using `export()` and


"to another"

No, I don't think that's correct.

packages/@aws-cdk/aws-ec2/test/integ.import-default-vpc.lit.ts

eladb · 2018-11-14T12:49:25Z

packages/@aws-cdk/cx-api/lib/context/vpc.ts

@@ -0,0 +1,21 @@
+export const VPC_PROVIDER = 'vpc-provider';
+
+export interface VpcContextQuery {


packages/aws-cdk/lib/context-providers/index.ts

eladb · 2018-11-14T12:51:48Z

packages/aws-cdk/lib/context-providers/vpcs.ts

+    const tags: {[key: string]: string} | undefined = args.tags;
+    const isDefault: boolean | undefined = args.isDefault;
+
+    // Builter request filter


eladb · 2018-11-14T12:52:31Z

packages/aws-cdk/lib/context-providers/vpcs.ts

+
+    // Builter request filter
+    const filters: AWS.EC2.Filter[] = [];
+    if (vpcId) { filters.push({ Name: 'vpc-id', Values: [vpcId] }); }


Why not just expose the filters API in the context provider API and move the logic of building the filters to the construct library?

@rix0rrr might still be recovering from when I used the word filter and he really didn't like it, but this context is different and I won't give him too hard a time if filter comes back.

I guess I wanted as narrow an API as possible. But yeah, I suppose it makes it easier to add features in the future.

Just feels like we can expose this to the user and give them more power without sacrificing too much usability (maybe we can add a bunch of sugar methods for the common use cases)

eladb · 2018-11-14T12:55:09Z

packages/aws-cdk/lib/context-providers/vpcs.ts

+    azs.sort();
+
+    const subnets: Subnet[] = listedSubnets.map(subnet => {
+      let type = getTag('aws-cdk:SubnetType', subnet.Tags);


I think we try to standardize around something like aws:cdk:subnet-type

can we set an aws: tag? or does it have to be aws-cdk?

Don't believe we can set the aws: tag.

eladb · 2018-11-14T12:56:46Z

packages/aws-cdk/lib/context-providers/vpcs.ts

+    // - Type tag, we tag subnets with their type
+    // - Name tag, we tag subnets with their subnet group name
+    // - MapPublicIpOnLaunch in absence of tags => must be a Public subnet, anything else is either Isolated or Private
+


Also mention that"type" is used as a name if we don't have a name tag

The actual source of truth for this is the route table. I was thinking the logic would be check for our Type tag and trust that CDK is the source of truth if it exists. If that is not there pull the route tables. If a route exists to an IGW (public), only NAT (private), neither (isolated). Yes there are egress only gateways and I'm not sure if those are public or private, my vote is private.

I know, but that seemed like overcomplicating.

So for non-CDK-exported VPCs we only support public/private nets, for CDK-exported VPCs we support isolated subnets as well. Seems like a fair compromise.

I happen to know my legacy VPCs do not set MapPublicIpOnLaunch to true for public subnets. I'm not positive if it was a secure by default argument or a non-repudiation (the non EIP public IPs are never in Cloud Trail). I do agree this is more complicated, and if we launch without it and let the community drive the request I'm probably ok.

I would argue in those cases you can always add the tags manually.

moofish32

Small comments, but my big question is should route table be the subnet type source of truth.

moofish32 · 2018-11-14T14:24:05Z

packages/@aws-cdk/aws-ec2/lib/vpc.ts

+const SUBNETTYPE_TAG = 'aws-cdk:SubnetType';
+const SUBNETNAME_TAG = 'aws-cdk:SubnetName';
+
+function subnetTypeTagValue(type: SubnetType) {


This makes me think we should have used String enums for this type.

Maybe so. It's not too onerous to have this function I think.

packages/@aws-cdk/aws-ec2/test/integ.import-default-vpc.lit.ts

moofish32 · 2018-11-14T14:30:31Z

packages/aws-cdk/lib/context-providers/vpcs.ts

+
+    // Builter request filter
+    const filters: AWS.EC2.Filter[] = [];
+    if (vpcId) { filters.push({ Name: 'vpc-id', Values: [vpcId] }); }


@rix0rrr might still be recovering from when I used the word filter and he really didn't like it, but this context is different and I won't give him too hard a time if filter comes back.

moofish32 · 2018-11-14T14:33:57Z

packages/aws-cdk/lib/context-providers/vpcs.ts

+    // - Type tag, we tag subnets with their type
+    // - Name tag, we tag subnets with their subnet group name
+    // - MapPublicIpOnLaunch in absence of tags => must be a Public subnet, anything else is either Isolated or Private
+


The actual source of truth for this is the route table. I was thinking the logic would be check for our Type tag and trust that CDK is the source of truth if it exists. If that is not there pull the route tables. If a route exists to an IGW (public), only NAT (private), neither (isolated). Yes there are egress only gateways and I'm not sure if those are public or private, my vote is private.

moofish32 · 2018-11-14T14:35:32Z

packages/aws-cdk/lib/context-providers/vpcs.ts

+    azs.sort();
+
+    const subnets: Subnet[] = listedSubnets.map(subnet => {
+      let type = getTag('aws-cdk:SubnetType', subnet.Tags);


can we set an aws: tag? or does it have to be aws-cdk?

moofish32 · 2018-11-14T14:37:07Z

packages/aws-cdk/lib/context-providers/vpcs.ts

+/**
+ * Group subnets of the same type together, and order by AZ
+ */
+function groupSubnets(subnets: Subnet[]): SubnetGroups {


+1 have to sort these

It's not even optional :)

eladb · 2018-11-14T19:56:38Z

docs/src/context.rst

+       Purpose: 'WebServices'
+     }
+   });
+   const vpc = VpcNetworkRef.import(this, 'VPC', provider.vpcProps);


Should we also include the VpcNetwork.importFromContext usage in this example?

eladb · 2018-11-14T19:57:34Z

packages/@aws-cdk/aws-ec2/lib/vpc-network-provider.ts

+
+/**
+ * Properties for looking up an existing VPC.
+ *


Reference AWS docs page that describes the query API

eladb · 2018-11-14T19:58:17Z

packages/@aws-cdk/aws-ec2/lib/vpc.ts

      }
+
+      // These values will be used to recover the config upon provider import


Mention the name of the context provider class here

packages/@aws-cdk/aws-ec2/test/integ.import-default-vpc.lit.ts

eladb · 2018-11-14T20:01:27Z

packages/@aws-cdk/cx-api/lib/context/availability-zones.ts

+   * Query region
+   */
+  region?: string;
+


It's not, though I suppose I could model it differently.

eladb · 2018-11-14T20:02:38Z

packages/@aws-cdk/cx-api/lib/context/vpc.ts

+   * Query region
+   */
+  region?: string;
+


I would put this under filters: any[] to avoid the need to define this in 100% places.

eladb · 2018-11-14T20:03:52Z

packages/aws-cdk/lib/context-providers/vpcs.ts

+
+    // Builter request filter
+    const filters: AWS.EC2.Filter[] = [];
+    if (vpcId) { filters.push({ Name: 'vpc-id', Values: [vpcId] }); }


Just feels like we can expose this to the user and give them more power without sacrificing too much usability (maybe we can add a bunch of sugar methods for the common use cases)

…JSII-compliant

rix0rrr requested review from eladb and moofish32 November 14, 2018 10:48

feat(aws-ec2): add VPC context provider

631eeb1

Add a context provider for looking up existing VPCs in an account. This is useful if the VPC is defined outside of your CDK app, such as in a different CDK app, by hand or in a CloudFormation template. Addresses some need of #1095.

rix0rrr force-pushed the huijbers/vpc-context branch from 89d5266 to 631eeb1 Compare November 14, 2018 10:56

rix0rrr added 2 commits November 14, 2018 13:08

Fix test runs

7586b91

Fix tests

396866b

eladb reviewed Nov 14, 2018

View reviewed changes

moofish32 reviewed Nov 14, 2018

View reviewed changes

rix0rrr added 2 commits November 14, 2018 16:37

Schematize Hosted Zone provider into CXAPI library as well

e1b2ea6

Schematize other context providers as well

42357d7

eladb approved these changes Nov 14, 2018

View reviewed changes

rix0rrr added 4 commits November 15, 2018 13:06

Update integ test expectations with new tags, CXAPI now contains filter

b0dc9c6

Don't really export HostedZoneContextResponse since the names aren't …

b92e20e

…JSII-compliant

Merge branch 'master' into huijbers/vpc-context

8be0691

More expectation updates

838999c

rix0rrr merged commit e8380fa into master Nov 15, 2018

rix0rrr deleted the huijbers/vpc-context branch November 15, 2018 14:41

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-ec2): add VPC context provider #1168

feat(aws-ec2): add VPC context provider #1168

rix0rrr commented Nov 14, 2018

eladb Nov 14, 2018

rix0rrr Nov 14, 2018

eladb Nov 14, 2018

eladb Nov 14, 2018

eladb Nov 14, 2018

moofish32 Nov 14, 2018

rix0rrr Nov 14, 2018

eladb Nov 14, 2018

eladb Nov 14, 2018

moofish32 Nov 14, 2018

rix0rrr Nov 14, 2018

eladb Nov 14, 2018

moofish32 Nov 14, 2018

rix0rrr Nov 14, 2018

moofish32 Nov 14, 2018

rix0rrr Nov 14, 2018

moofish32 left a comment

moofish32 Nov 14, 2018

rix0rrr Nov 14, 2018

moofish32 Nov 14, 2018

moofish32 Nov 14, 2018

moofish32 Nov 14, 2018

moofish32 Nov 14, 2018

rix0rrr Nov 14, 2018

eladb Nov 14, 2018

eladb Nov 14, 2018

eladb Nov 14, 2018

eladb Nov 14, 2018

rix0rrr Nov 15, 2018

eladb Nov 14, 2018

eladb Nov 14, 2018

		@@ -0,0 +1,21 @@
		export const VPC_PROVIDER = 'vpc-provider';

		export interface VpcContextQuery {

		}

		// These values will be used to recover the config upon provider import

feat(aws-ec2): add VPC context provider #1168

feat(aws-ec2): add VPC context provider #1168

Conversation

rix0rrr commented Nov 14, 2018

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

moofish32 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