feat(ec2): add support for vpn connections #1899

jogold · 2019-02-27T13:40:39Z

Add support for Site-to-Site VPN connections to VPC networks.

When VPN connections are specified, a VPN gateway is automatically
created and attached to the VPC. By default, routes are propagated on the
route tables associated with the private subnets. Propagation to routes
tables associated with public and/or isolated subnets is supported.

Update VPC context provider to also import vpnGatewayId.

References aws/jsii#231

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- 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.

eladb

Copy: @PaulMaddox

eladb · 2019-02-27T15:55:40Z

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

+
+```ts
+// Dynamic routing
+vpc.newVpnConnection('Dynamic', {


rename to addVpnConnection

eladb · 2019-02-27T15:55:58Z

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

+
+```ts
+const vpc = new ec2.VpcNetwork(stack, 'MyVpc', {
+  vpnGateway: true


allow specifying vpnConnections in props as well via a map from ID to options:

new VpcNetwork(this, 'MyVpc', { vpnConnections: { dynamic: { ip: '1.2.3.4' } } });

eladb · 2019-02-27T15:56:30Z

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

@@ -12,6 +13,12 @@ export interface IVpcSubnet extends IConstruct {
   */
  readonly subnetId: string;

+  /**
+   * The id of the route table associated with this subnet.
+   * Not available for an imported subnet.


Will this throw for imported subnets?

No, it won't. We need routeTableId in order to propagate routes. The AWS::EC2::VPNGatewayRoutePropagation references it and is created inside the VpcNetwork construct. For imported networks/subnets we just don't need this routeTableId

eladb · 2019-02-27T15:58:15Z

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

+
+      // Propagate routes on route tables associated with private subnets
+      const routePropagation = new CfnVPNGatewayRoutePropagation(this, 'RoutePropagation', {
+        routeTableIds: this.privateSubnets.map(subnet => subnet.routeTableId!),


No !s please (it indicates that either the model is wrong or that we need a check with a good error thrown). Seems like routeTableId cannot be undefined

It's indeed a mistake.

eladb · 2019-02-27T15:59:50Z

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


  constructor(scope: cdk.Construct, id: string, private readonly props: VpcSubnetImportProps) {
    super(scope, id);

    this.subnetId = props.subnetId;
    this.availabilityZone = props.availabilityZone;
+    this.routeTableId = '';


that's fishy

eladb · 2019-02-27T16:00:40Z

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

+  tunnelInsideCidr: string;
+}
+
+export interface BaseVpnConnectionProps {


Please rename to VpnConnectionOptions (see #1740)

eladb · 2019-02-27T16:01:17Z

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

+/**
+ * The IPsec 1 VPN connection type.
+ */
+export const IPsec1 = 'ipsec.1';


Should this be an enum maybe?

Switching to an enum with only one member seems problematic for jsii? See build logs...

You are correct: aws/jsii#231

Can you add another member, even a dummy one?

And you can add this to the jsii issue and we'll fix once the jsii bug is fixed.

eladb · 2019-02-27T16:03:21Z

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

+  /**
+   * Indicates whether a VPN gateway should be created and attached to this VPC.
+   *
+   * @default false


Can we make this default smarter, so if I define vpn connections on this VPC, it will default to "true", and otherwise to "false". I would expect this to Just Work (TM):

new VpcNetwork(this, 'VPC', { vpnConnections: { dynamic: { ip: '1.2.3.4' } } });

eladb · 2019-02-27T19:18:21Z

Hey @jogold great work by the way!

eladb

Looks good! I'd like @rix0rrr or @PaulMaddox 's feedback here as well

eladb · 2019-02-27T19:24:10Z

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

+});
+```
+
+To export a VPC that can accept VPN connections, set `vpnGateway` to `true`:


eladb · 2019-02-27T19:26:54Z

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

+      routePropagation.node.addDependency(attachment);
+
+      const vpnConnections = props.vpnConnections || {};
+      Object.keys(vpnConnections).forEach(cId => {


Style: I prefer:

for (const [ id, vpn ] of Object.entries(vpnConnections)) { // ... }

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

packages/@aws-cdk/aws-ec2/test/integ.vpn.ts

eladb · 2019-02-27T21:43:28Z

@leepa if you have some time to take a quick look, it will be much appreciated.

eladb · 2019-02-27T21:43:36Z

@jogold 👍

jogold · 2019-02-27T21:50:49Z

Do we need to support an option to propagate routes on isolated subnets? We could have something like:

export enum RoutesPropagationType {
  PrivateOnly,
  PrivateIsolated
}

export interface VpcNetworkProps {
  ...
  /**
   * Where to propagate VPN routes.
   *
   * @default on the route tables associated with the private subnets 
   */
  vpnRoutesPropagation?: RoutesPropagationType
  ...
}

eladb · 2019-02-27T22:01:44Z

@jogold sounds useful

leepa

Love this addition - it will make handling VPN connections nicer. There's the case of handing subnet groups rather than being opinionated about only allowing private subnets (so for example, some customers want to just SSH directly to hosts via the VPN connection).

leepa · 2019-02-27T21:59:47Z

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

+
+      // Propagate routes on route tables associated with private subnets
+      const routePropagation = new CfnVPNGatewayRoutePropagation(this, 'RoutePropagation', {
+        routeTableIds: (this.privateSubnets as VpcPrivateSubnet[]).map(subnet => subnet.routeTableId),


We should consider allowing an option to allow for subnetgroups to be chosen by the user of this to decide what to propogate - there are reasons you might want to route public (and isolated)

leepa · 2019-02-27T22:19:58Z

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

+   * The range of inside IP addresses for the tunnel. Any specified CIDR blocks must be
+   * unique across all VPN connections that use the same virtual private gateway.
+   */
+  tunnelInsideCidr: string;


Both things here are independently optional https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-vpnconnection-vpntunneloptionsspecification.html and so one might want to set one and not the other.

leepa · 2019-02-27T22:22:09Z

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

+  /**
+   * Tunnel options for the VPN connection.
+   */
+  vpnTunnelOptions?: VpnTunnelOption[];


Tests don't appear to cover this.

eladb · 2019-03-04T10:12:17Z

@leepa are you happy with this? Can we merge?

leepa · 2019-03-04T10:17:59Z

@eladb if the tests are passing, the changes look good!

eladb · 2019-03-04T10:24:10Z

@jogold may I ask that you write up a nice PR description so we can use as a commit message?

jogold · 2019-03-04T11:05:05Z

@eladb updated description, is this ok?

eladb · 2019-03-04T11:07:11Z

Perfect. Thanks!

feat(ec2): add support for vpn connections

45e69c7

jogold requested a review from a team as a code owner February 27, 2019 13:40

eladb reviewed Feb 27, 2019

View reviewed changes

jogold added 4 commits February 27, 2019 19:08

Address PR comments

5312496

Update README

e95b7ff

Remove routeTableId from IVpcSubnet

766c882

Better default for vpnGateway

0692784

eladb reviewed Feb 27, 2019

View reviewed changes

jogold added 5 commits February 27, 2019 21:41

Update README

657ca96

Add dummy member to VpnConnectionType enum

b0cf686

Style

a589734

Better error handling

3aaa1fe

Call addVpnConnection in integ test

b456cc4

Make dummy really dummy

8d30a7f

leepa reviewed Feb 27, 2019

View reviewed changes

jogold added 3 commits February 28, 2019 09:16

Add option to customize route propagation

c77f614

Add tunnel options validation and tests

cbf0235

Use net instead of regex for ip validation

bddf05f

jogold mentioned this pull request Mar 1, 2019

Singleton enums are not typed properly aws/jsii#231

Closed

eladb approved these changes Mar 4, 2019

View reviewed changes

eladb merged commit e150648 into aws:master Mar 4, 2019

This was referenced Mar 12, 2019

VpcNetwork.importFromContext does not work if multiple private subnets are present #1523

Closed

VpcNetwork.importFromContext throws Cannot read property 'VpnGatewayId' of undefined #1998

Closed

jogold deleted the ec2-vpn branch March 12, 2019 11:23

jogold mentioned this pull request Mar 28, 2019

feat(ec2): add support for vpc endpoints #2104

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ec2): add support for vpn connections #1899

feat(ec2): add support for vpn connections #1899

jogold commented Feb 27, 2019 •

edited

Loading

eladb left a comment

eladb Feb 27, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

jogold Feb 27, 2019 •

edited

Loading

eladb Feb 27, 2019

jogold Feb 27, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

jogold Feb 27, 2019

jogold Feb 27, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

eladb Feb 27, 2019

eladb commented Feb 27, 2019

eladb left a comment

eladb Feb 27, 2019

eladb Feb 27, 2019

eladb commented Feb 27, 2019

eladb commented Feb 27, 2019

jogold commented Feb 27, 2019 •

edited

Loading

eladb commented Feb 27, 2019

leepa left a comment

leepa Feb 27, 2019

jogold Feb 28, 2019

leepa Feb 27, 2019

jogold Feb 28, 2019

leepa Feb 27, 2019

eladb commented Mar 4, 2019

leepa commented Mar 4, 2019

eladb commented Mar 4, 2019

jogold commented Mar 4, 2019

eladb commented Mar 4, 2019

feat(ec2): add support for vpn connections #1899

feat(ec2): add support for vpn connections #1899

Conversation

jogold commented Feb 27, 2019 • edited Loading

Pull Request Checklist

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

jogold Feb 27, 2019 • edited Loading

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 Feb 27, 2019

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

eladb commented Feb 27, 2019

eladb commented Feb 27, 2019

jogold commented Feb 27, 2019 • edited Loading

eladb commented Feb 27, 2019

leepa 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

eladb commented Mar 4, 2019

leepa commented Mar 4, 2019

eladb commented Mar 4, 2019

jogold commented Mar 4, 2019

eladb commented Mar 4, 2019

jogold commented Feb 27, 2019 •

edited

Loading

jogold Feb 27, 2019 •

edited

Loading

jogold commented Feb 27, 2019 •

edited

Loading