feat(vpcv2): implementation of add gateway method #31224

shikha372 · 2024-08-26T19:07:20Z

Issue # (if applicable)

Tracking #30762.

Reason for this change

implementing below methods for vpcV2.
routeTable.addroute(destination, target): Adds a new route to the existing route table of the subnet.

vpc.enableVpnGatewayV2(): added a new function for the customer to add VPNGateway to their VPC. In the options, user can specify list of subnets for VPNRoutePropogation. This is similar to previous implementation, only difference is with VPNGateway L2, it is now creating VPNGatewayV2 which implements IRouteTarget and hence can be used a destination to be set up in route tables.

addInternetGateway : adds internetGW to the VPC.
Default behaviour: add default route with destination set to ‘0.0.0.0’ and ‘::0’(in case of subnet with ipv6). Also a check in place to verify SubnetType is set to public as IGW is meant to be added to public subnets.

addNatGateway: NatGateways are subnet specific and are usually associated with PRIVATE_WITH_EGRESS or PUBLIC subnet. Also, one can’t attach NGW(Public) to subnet if VPC doesn’t have an IGW attached to it. This is validated in method implementation to prevent runtime deployment error.

No default behaviour for the routes, it takes in the single subnet option and associates a NATGW with it.

vpc.addEgressOnlyInternetGateway(): Egress Only internet GW are meant for outbound ipv6 traffic which can be custom or all ipv6(::/0).

Default behaviour: Associates a EIGW to the vpc and takes optional input for subnets to define a default route in associated route Table, if a destination is not provided, then it is defined as all outbound ipv6 in subnet’s route table.

Additional changes:
-> Modify Readme
-> Separate ipam related Tests

Use Case

Allows user to define gateways in their vpc with a simple method and an optional default route setup on provided subnets.

Note: Breaking change since previously VPNGateway was released under route class, we’ve modified it to VPNGatewayV2.
vpc.enableVpnGateway is marked as deprecated in vpcv2 base class.

Description of how you validated changes

Added unit tests and integration tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

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

packages/@aws-cdk/aws-ec2-alpha/lib/route.ts

packages/@aws-cdk/aws-ec2-alpha/lib/vpc-v2-base.ts

paulhcsun · 2024-09-05T00:31:37Z

packages/@aws-cdk/aws-ec2-alpha/lib/vpc-v2-base.ts

+    const useIpv6 = (this.secondaryCidrBlock.some((secondaryAddress) => secondaryAddress.amazonProvidedIpv6CidrBlock === true ||
+    secondaryAddress.ipv6IpamPoolId != undefined))? true : false;


What is this doing?

Since we can have multiple secondary address attached to VPC, this is to check if any of the secondary address is a IPv6 meaning created using IPAMIpv6 or is a AmazonProvidedIpv6

Can we add a comment to describe that behaviour.

paulhcsun · 2024-09-05T00:32:47Z

packages/@aws-cdk/aws-ec2-alpha/lib/vpc-v2-base.ts

+    if (options?.subnets) {
+      const subnets = flatten(options.subnets.map(s => this.selectSubnets(s).subnets));
+      subnets.forEach((subnet) => {
+        this.createEgressRoute(subnet, egw, options.destination);


What is this doing?

this is the default route that we are creating under provided subnet's routeTable. It will add an entry to subnet's routeTable either with the specified destination address or with default(::/0)

Could you add a comment to describe that here? It's just not super obvious to anyone who might need to work on and understand this code in the future.

add documentation nits Co-authored-by: paulhcsun <[email protected]>

GavinZZ

Only half way through.

I'm a bit worried by the number of breaking changes. I understand it is an alpha module and breaking changes are allowed. However, a number of changes are removing an old construct and introduced V2, i.e. VPNGateway is removed and introduced VPNGatewayV2. It's breaking changes anyway, should we just use the same name?

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

GavinZZ · 2024-09-05T00:05:54Z

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

  routeTable,
  destination: '0.0.0.0/0',
  target:  { gateway: igw },
 });
 ```

+Alternatively, `Route`s can be created via a method in the `RouteTable` class. An example using the `EgressOnlyInternetGateway` construct can be seen below:


Routes looks weird.

Maybe mention the method routeTable.addRoute. Right now it's immediately followed with An example using Egress...which doesn't have anything to do withRoutes can be created via ...`

corrected

it does relate with creating the route in line , you cannot create a route without a target so we need a target first(in this case, it is EgressOnlyInternetGateway).
routeTable.addRoute('::/0', { gateway: eigw });
To make it more clear mentioned the method name

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

GavinZZ · 2024-09-05T00:09:46Z

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

+VPCv2 supports adding an egress only internet gateway to VPC with the help of `addEgressOnlyInternetGateway` method as well.
+
+By Default, it sets up a route to all outbound IPv6 Address ranges unless specified to a specific destination by the user. It can only be set up for IPv6 enabled VPCs.
+`Subnets` takes in value of `SubnetFilter` which can be based on a SubnetType in VPCV2. A new route will be added to route tables of all subnets filtered out with this property.


You explained SubnetFilter but I don't see it in the example below?

Selecting a type of subnet is a SubnetFilter :)
subnets: [subnetType: {SubnetType.PUBLIC}]

GavinZZ · 2024-09-05T05:41:11Z

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

+  subnetType: ec2.SubnetType.PRIVATE });
+
+myVpc.addEgressOnlyInternetGateway({
+  subnets: [{SubnetType.PUBLIC}],


Haven't looked into implementation but this already looks weird to me.

This is as per the current implementations we have in place for addInterfaceEndpoint and addgatewayEndpoint and provides a better way for the users to select multiple subnets at once without forming an array list first and then pass it. This seems to be the best option for specifying multiple subnets as an input.
Let me know if you've thoughts on any other way to do it.

GavinZZ · 2024-09-05T05:41:27Z

packages/@aws-cdk/aws-ec2-alpha/awslint.json

+      "from-method:@aws-cdk/aws-ec2-alpha.SubnetV2",
+      "from-method:@aws-cdk/aws-ec2-alpha.Route"


For my knowledge, what does these two lines do?

this is enforced from awslint to have the import methods in classes extending Resource , since we haven't introduced it yet these are part of exceptions

packages/@aws-cdk/aws-ec2-alpha/lib/route.ts

GavinZZ · 2024-09-05T05:43:39Z

packages/@aws-cdk/aws-ec2-alpha/lib/route.ts

  /**
   * The subnet in which the NAT gateway is located.
   */
-  readonly subnet: ISubnet;


Isn't this a breaking change for users?

if you mean by changing the ISubnet to ISubnetv2, its not breaking change since ISubnetv2 extends ISubnet

Co-authored-by: GZ <[email protected]> Co-authored-by: paulhcsun <[email protected]>

shikha372 · 2024-09-05T07:57:23Z

Only half way through.

I'm a bit worried by the number of breaking changes. I understand it is an alpha module and breaking changes are allowed. However, a number of changes are removing an old construct and introduced V2, i.e. VPNGateway is removed and introduced VPNGatewayV2. It's breaking changes anyway, should we just use the same name?

So one is compatible with Route construct and other lives in the main lib VPNGateway is not implementing IRouteTarget so that can not be set as an target while defining a route. So, the V2 is aligned with features (i.e. Route and RouteTable) in this new module, it is more clear that this is latest. Also, if there is already a construct with same name in main lib, not sure if its best practice to introduce another with same name as it can create issue with graduation.

GavinZZ

Left some more minor feedbacks.

packages/@aws-cdk/aws-ec2-alpha/lib/route.ts

GavinZZ · 2024-09-09T18:15:22Z

packages/@aws-cdk/aws-ec2-alpha/lib/vpc-v2-base.ts

+     * Subnets where the route propagation should be added.
+     * @default noPropagation
+     */
+  readonly vpnRoutePropagation?: SubnetSelection[];


A general comment, for a text explaining @default, we prefer to do @default - <description. For property value used with @default, we can do @default value.

For this specific property, it feels confusing to me because it talks about route propagation but the type is subnet selection.

yeah, got to know it from @paulhcsun , so working to reformat this for entire repo in next revision,

packages/@aws-cdk/aws-ec2-alpha/lib/route.ts

GavinZZ · 2024-09-09T18:28:35Z

packages/@aws-cdk/aws-ec2-alpha/lib/subnet-v2.ts

  /**
   * The type of subnet (public or private) that this subnet represents.
   * @attribute SubnetType
   */
-  public readonly subnetType: SubnetType;
+  public readonly subnetType?: SubnetType;


If this becomes optional, what's the default value?

This is not optional under props, so user still needs to define it but it is marked as optional under the new interface ISubnetV2 because if not it will cause issues with the existing property in SubnetSelection for subnets which we are leveraging in some of the places like here..

Also why this is added as a part of interface : we added a check under some of the add methods to verify whether provided subnets are of correct type eg. SubnetType.Public .

packages/@aws-cdk/aws-ec2-alpha/lib/subnet-v2.ts

packages/@aws-cdk/aws-ec2-alpha/lib/vpc-v2-base.ts

packages/@aws-cdk/aws-ec2-alpha/lib/route.ts

GavinZZ

Thanks Shikha for this great work. The code generally looks good to me. I have two high-level thoughts:

I know I've mentioned this, but seeing the usage and it's in alpha modules, I'm not convinced why we need to remove the original constructs, i.e. VPNGateway, Subnet and create a VPNGatewayV2, SubnetV2. Either way, it's breaking changes for customers, wouldn't keep using the name VPNGateway and Subnet but with new implementation be better if in the future we decide to make it stable and it won't be starting from V2 by default.
Subnets have to be attached with a VPC resource. Can we introduces methods in VPC construct like vpc.addSubnet(...) just like how you added vpc.addNatGateway, vpc.addInternetGateway. I peronsally think it makes sense to add subnet this way and prefer creating subnets this way instead of new SubnetV2. Want to see your thoughts.

packages/@aws-cdk/aws-ec2-alpha/lib/route.ts

packages/@aws-cdk/aws-ec2-alpha/lib/vpc-v2-base.ts

packages/@aws-cdk/aws-ec2-alpha/test/integ.ipam.ts

Co-authored-by: GZ <[email protected]>

paulhcsun

Looks good to me, deferring final approval to @GavinZZ. Nice work @shikha372!

Dismissed

shikha372 · 2024-09-12T15:30:29Z

@Mergifyio update

mergify · 2024-09-12T15:30:39Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2024-09-12T16:02:09Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: f30e1da
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-09-12T16:26:10Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2024-09-12T16:26:31Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

Leonardo Gama and others added 30 commits August 9, 2024 15:12

Add RouteTable addRoute method

31d3f48

Update packages/@aws-cdk/aws-ec2-alpha/lib/route.ts

727b164

Change CfnRoute to Route L2

cb8c941

Update README.md

d21be53

Merge branch 'main' into vpcv2-rt-addroute

42830e7

Update integ test to use only one EIGW

2f48ab2

Add subnet associateRouteTable method

f904a97

Appease the linter

b52d498

Update subnet with public routeTable again

1aef528

Update subnet-v2.ts

9b15a4b

Update subnet-v2.ts

bc56a77

Extend IRouteTable into IRouteTableV2

cc8db35

Merge branch 'main' into vpcv2-rt-addroute

285da27

ok so route table v2 was not a great idea lol

98f9d53

Merge branch 'main' into vpcv2-rt-addroute

b3e08c1

Update testing

1211fde

Merge branch 'main' into vpcv2-rt-addroute

ca9c385

add method associateRouteTable

eaa7ccb

fixing route class

a25bb78

making ipam props private

1721026

successful build

bc393c2

adding validation for ipv4

aed940f

fixing egw route and subnet snapshot

499c6c8

fixing readme and route unit test

f296f3e

fixing vpc snapshot

0002de5

Update route snapshot

909f7da

Update route snapshot

1b907c1

Merge branch 'main' into vpcv2-rt-addroute

0dba8bb

adding EGW method to VPC

8ca0a3c

(feat): add EIGW implementation

f42666d

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 3, 2024

paulhcsun requested changes Sep 5, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 5, 2024

Apply suggestions from code review

5d63c6c

add documentation nits Co-authored-by: paulhcsun <[email protected]>

GavinZZ requested changes Sep 5, 2024

View reviewed changes

Apply suggestions from code review

669c735

Co-authored-by: GZ <[email protected]> Co-authored-by: paulhcsun <[email protected]>

fix readme

a9096f5

GavinZZ requested changes Sep 9, 2024

View reviewed changes

GavinZZ reviewed Sep 9, 2024

View reviewed changes

packages/@aws-cdk/aws-ec2-alpha/lib/route.ts Outdated Show resolved Hide resolved

fixing nits for defining default

5c7548c

GavinZZ previously requested changes Sep 10, 2024

View reviewed changes

Apply suggestions from code review

4b4fb6b

Co-authored-by: GZ <[email protected]>

shikha372 force-pushed the vpcv2-rt-addGateway branch from 8dffdbe to 753273f Compare September 11, 2024 04:13

paulhcsun added the pr/do-not-merge This PR should not be merged at this time. label Sep 11, 2024

paulhcsun approved these changes Sep 11, 2024

View reviewed changes

fix nits

dcb11e6

shikha372 force-pushed the vpcv2-rt-addGateway branch from 753273f to dcb11e6 Compare September 11, 2024 18:26

GavinZZ self-requested a review September 11, 2024 22:37

Merge branch 'main' into vpcv2-rt-addGateway

f30e1da

shikha372 removed the pr/do-not-merge This PR should not be merged at this time. label Sep 12, 2024

mergify bot merged commit 4b90bfc into main Sep 12, 2024
14 of 15 checks passed

mergify bot deleted the vpcv2-rt-addGateway branch September 12, 2024 16:26

github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vpcv2): implementation of add gateway method #31224

feat(vpcv2): implementation of add gateway method #31224

shikha372 commented Aug 26, 2024 •

edited

Loading

paulhcsun Sep 5, 2024

shikha372 Sep 5, 2024

paulhcsun Sep 11, 2024

paulhcsun Sep 5, 2024

shikha372 Sep 5, 2024

paulhcsun Sep 11, 2024

GavinZZ left a comment •

edited

Loading

GavinZZ Sep 5, 2024

shikha372 Sep 5, 2024 •

edited

Loading

GavinZZ Sep 5, 2024

shikha372 Sep 5, 2024 •

edited

Loading

GavinZZ Sep 5, 2024

shikha372 Sep 5, 2024 •

edited

Loading

GavinZZ Sep 5, 2024

shikha372 Sep 5, 2024

GavinZZ Sep 5, 2024

shikha372 Sep 5, 2024 •

edited

Loading

shikha372 commented Sep 5, 2024

GavinZZ left a comment

GavinZZ Sep 9, 2024

shikha372 Sep 9, 2024

GavinZZ Sep 9, 2024

shikha372 Sep 9, 2024 •

edited

Loading

GavinZZ left a comment •

edited

Loading

paulhcsun left a comment

shikha372 commented Sep 12, 2024

mergify bot commented Sep 12, 2024

aws-cdk-automation commented Sep 12, 2024

mergify bot commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

		const useIpv6 = (this.secondaryCidrBlock.some((secondaryAddress) => secondaryAddress.amazonProvidedIpv6CidrBlock === true \|\|
		secondaryAddress.ipv6IpamPoolId != undefined))? true : false;

		"from-method:@aws-cdk/aws-ec2-alpha.SubnetV2",
		"from-method:@aws-cdk/aws-ec2-alpha.Route"

feat(vpcv2): implementation of add gateway method #31224

feat(vpcv2): implementation of add gateway method #31224

Conversation

shikha372 commented Aug 26, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Use Case

Description of how you validated changes

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

GavinZZ left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shikha372 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shikha372 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shikha372 Sep 5, 2024 • 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

shikha372 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

shikha372 commented Sep 5, 2024

GavinZZ 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

shikha372 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

GavinZZ left a comment • edited Loading

Choose a reason for hiding this comment

paulhcsun left a comment

Choose a reason for hiding this comment

shikha372 commented Sep 12, 2024

mergify bot commented Sep 12, 2024

✅ Branch has been successfully updated

aws-cdk-automation commented Sep 12, 2024

AWS CodeBuild CI Report

mergify bot commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

shikha372 commented Aug 26, 2024 •

edited

Loading

GavinZZ left a comment •

edited

Loading

shikha372 Sep 5, 2024 •

edited

Loading

shikha372 Sep 5, 2024 •

edited

Loading

shikha372 Sep 5, 2024 •

edited

Loading

shikha372 Sep 5, 2024 •

edited

Loading

shikha372 Sep 9, 2024 •

edited

Loading

GavinZZ left a comment •

edited

Loading