feat(ec2-alpha): add Transit Gateway L2 #32956

paulhcsun · 2025-01-15T21:37:55Z

Issue # (if applicable)

Closes #.

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

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

aws-cdk-automation

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

codecov · 2025-01-15T21:59:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.79%. Comparing base (2eff2bd) to head (39e8ae9).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32956   +/-   ##
=======================================
  Coverage   80.79%   80.79%           
=======================================
  Files         232      232           
  Lines       14110    14110           
  Branches     2453     2453           
=======================================
  Hits        11400    11400           
  Misses       2430     2430           
  Partials      280      280

Flag	Coverage Δ
suite.unit	`80.79% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`79.53% <ø> (ø)`
packages/aws-cdk-lib/core	`82.17% <ø> (ø)`

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

GavinZZ

Great work! Left some comments to help me understand.

GavinZZ · 2025-01-24T20:00:34Z

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

+
+> Note: When you create a default Transit Gateway in AWS Console, EC2 creates the default `TransitGatewayRouteTable` for you. When using this construct, CDK will create the default `TransitGatewayRouteTable` for you instead with the automatic Association/Propagation features being mimicked by the CDK. 
+>
+> **Default association route table** and **Default propagation route table** will show as disabled in your AWS console but can still be toggled within CDK using the `defaultRouteTableAssociation` and `defaultRouteTablePropagation` properties respectively. 


Want to clarify this sentence a bit. So CDK will by default create TGRT but on console it shows disabled meaning console saying no default TGRT is created?

When both defaultRouteTableAssociation and defaultRouteTablePropagation are disabled in the CFN template, EC2 will not create the default route table. Instead, the CDK will always create a default route table in place of EC2.

The Default association route table and Default propagation route table settings are shown on every TGRT and it will show as disabled in AWS console for the CDK created default route table (because it can only be enabled on the EC2 created default route table), but we still provide properties within CDK, defaultRouteTableAssociation and defaultRouteTablePropagation, to allow customers to enable the setting (we mimic it being enabled with CDK implementation) even though it shows disabled in console.

GavinZZ · 2025-01-24T20:01:52Z

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

+>
+> **Default association route table** and **Default propagation route table** will show as disabled in your AWS console but can still be toggled within CDK using the `defaultRouteTableAssociation` and `defaultRouteTablePropagation` properties respectively. 
+
+You can disable the automatic Association/Propagation on the default `TransitGatewayRouteTable` via the `TransitGateway` properties. This will still create a default route table for you:


Wonder what's the behaviour when we supply a custom default route table but enable default route table association and default route table propagation?

You actually cannot supply a custom route table, because to create a route table you need to pass in a Transit Gateway, so you wouldn't just have a random extra route table lying around.

GavinZZ · 2025-01-24T20:04:55Z

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

+If you want to automatically associate and propagate routes with transit gateway route tables, you can pass the `associationRouteTable` and `propagationRouteTables` parameters. This will automatically create the necessary associations and propagations based on the provided route tables.
+
+```ts
+const attachmentWithRoutes = transitGateway.attachVpc('VpcAttachment', vpc, [subnet], undefined, associationRouteTable, [propagationRouteTable1, propagationRouteTable2]);


This feel like a lot of parmeters to me. Can we put them into a Options interface instead?

Agreed, I don't like having to skip over the optional parameters either. I'll see if there's a clean way to set up the interfaces with a base interface so it's not a duplicate of the ITransitGatewayVpcAttachment interface.

GavinZZ · 2025-01-24T20:07:31Z

packages/@aws-cdk/aws-ec2-alpha/lib/transit-gateway-attachment.ts

+  readonly transitGatewayVpcAttachmentId: string;
+}
+
+export abstract class TransitGatewayAttachmentBase extends Resource implements ITransitGatewayAttachment {


Why is this abstract class needed? Do you foresee that we will add more subclasses in the future?

Yes, there will be different types of TransitGatewayAttachments such as an attachment to connect one TGW to another, VPN Attachments, Connect Attachments.

GavinZZ · 2025-01-24T20:08:54Z

packages/@aws-cdk/aws-ec2-alpha/lib/transit-gateway-vpc-attachment.ts

+  /**
+   * The VPC attachment options.
+   */
+  readonly transitGatewayVpcAttachmentOptions?: ITransitGatewayVpcAttachmentOptions;


Suggested change

readonly transitGatewayVpcAttachmentOptions?: ITransitGatewayVpcAttachmentOptions;

readonly vpcAttachmentOptions?: ITransitGatewayVpcAttachmentOptions;

This is in transitGateway file already, I think we can omit the prefix.

You're right, this should be fine to omit the prefix.

GavinZZ · 2025-01-24T20:15:24Z

packages/@aws-cdk/aws-ec2-alpha/lib/transit-gateway.ts

+    id: string, vpc: IVpc,
+    subnets: ISubnet[],
+    transitGatewayAttachmentOptions?: ITransitGatewayVpcAttachmentOptions,
+    associationRouteTable?: ITransitGatewayRouteTable,
+    propagationRouteTables?: ITransitGatewayRouteTable[],


IMO, everything after id can be put in an XXXOptions interface. wdyt?

Agreed. Will look into a clean solution for this.

GavinZZ · 2025-01-24T20:17:49Z

packages/@aws-cdk/aws-ec2-alpha/lib/transit-gateway.ts

+      autoAcceptSharedAttachments: (props?.autoAcceptSharedAttachments ?? false)
+        ? TransitGatewayFeatureStatus.ENABLE : TransitGatewayFeatureStatus.DISABLE,
+      // Default Association/Propagation will always be false when creating the L1 to prevent EC2 from creating the default route table.
+      // Instead, CDK will create a custom default route table and use the properties to mimic the automatic assocation/propagation behaviour.
+      defaultRouteTableAssociation: (props?.defaultRouteTableAssociation ?? false)
+        ? TransitGatewayFeatureStatus.ENABLE : TransitGatewayFeatureStatus.DISABLE,
+      defaultRouteTablePropagation: (props?.defaultRouteTablePropagation ?? false)
+        ? TransitGatewayFeatureStatus.ENABLE : TransitGatewayFeatureStatus.DISABLE,
+      description: props?.description,
+      dnsSupport: (props?.dnsSupport ?? true) ? TransitGatewayFeatureStatus.ENABLE : TransitGatewayFeatureStatus.DISABLE,
+      multicastSupport: (props?.multicastSupport ?? false) ? TransitGatewayFeatureStatus.ENABLE : TransitGatewayFeatureStatus.DISABLE,
+      vpnEcmpSupport: (props?.vpnEcmpSupport ?? true) ? TransitGatewayFeatureStatus.ENABLE : TransitGatewayFeatureStatus.DISABLE,


same as above comment, take out the common functionality.

GavinZZ · 2025-01-24T20:19:39Z

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

+  public abstract destinationCidrBlock: string;
+}
+
+export class TransitGatewayActiveRoute extends TransitGatewayRouteBase {


Suggested change

export class TransitGatewayActiveRoute extends TransitGatewayRouteBase {

export class TransitGatewayRoute extends TransitGatewayRouteBase {

IMO Route is more commonly used, wdyt?

I named it ActiveRoute to distinguish that it is different from the BlackholeRoute but I think just naming it TransitGatewayRoute also works since the method name is called addRoute() and addBlackholeRoute().

GavinZZ · 2025-01-24T20:23:00Z

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

+  addRoute(id: string, transitGatewayAttachment: ITransitGatewayAttachment, destinationCidr: string): TransitGatewayActiveRoute {
+    return new TransitGatewayActiveRoute(this, id, {
+      transitGatewayRouteTable: this,
+      transitGatewayAttachment,
+      destinationCidrBlock: destinationCidr,
+    });
+  };
+
+  addBlackholeRoute(id: string, destinationCidr: string): TransitGatewayBlackholeRoute {
+    return new TransitGatewayBlackholeRoute(this, id, {
+      transitGatewayRouteTable: this,
+      destinationCidrBlock: destinationCidr,
+    });
+  }
+
+  addAssociation(id: string, transitGatewayAttachment: ITransitGatewayAttachment): TransitGatewayRouteTableAssociation {
+    return new TransitGatewayRouteTableAssociation(this, id, {
+      transitGatewayVpcAttachment: transitGatewayAttachment,
+      transitGatewayRouteTable: this,
+    });
+  }
+
+  enablePropagation(id: string, transitGatewayAttachment: ITransitGatewayAttachment): TransitGatewayRouteTablePropagation {


Should this functionality be exposed ITransitGatewayRouteTable? i.e. I see that imported TGRT is not supported yet, but when it is supported, should the imported TGRT have the capability of adding routes/ adding balckhole routes/etc?

I think it is okay to leave for now. When we do support imported route tables then we can move the declarations for these methods into the interface which will not be a breaking change.

GavinZZ · 2025-01-24T20:24:41Z

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

+const customRouteTable = new ec2.TransitGatewayRouteTable(this, 'TransitGatewayRouteTable');
+
+const transitGateway = new ec2.TransitGateway(this, 'MyTransitGateway', {
+  customDefaultRouteTable: customRouteTable,


I don't see this option customDefaultRouteTable?

You are right, similar to the above, I realized it's not possible to create a TGW with a custom route table that gets passed in so I've removed the property. Will also remove this example from the README.

aws-cdk-automation

(This review is outdated)

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aws-cdk-automation · 2025-01-25T00:59:56Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 39e8ae9
Result: FAILED
Build Logs (available for 30 days)

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

paulhcsun added 3 commits January 9, 2025 11:58

WIP feat(ec2-alpha): transit gateway L2

50b9d34

skeleton

d2af8a5

updated classes

a76835c

github-actions bot added the p2 label Jan 15, 2025

aws-cdk-automation requested a review from a team January 15, 2025 21:38

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 15, 2025

aws-cdk-automation previously requested changes Jan 15, 2025

View reviewed changes

paulhcsun added 2 commits January 15, 2025 14:03

remove add/removeSubnetIds property + implement methods

9c0bf44

some unit tests + class updates

353ca25

paulhcsun changed the title ~~feat(ec2-alpha): Transit Gateway L2~~ feat(ec2-alpha): add Transit Gateway L2 Jan 22, 2025

paulhcsun added 2 commits January 22, 2025 15:53

update vpc attachment resource + unit tests

5f0c30b

README draft

e123638

paulhcsun added 3 commits January 23, 2025 14:45

integ snapshots + update attachment options

faf600d

fix unit test

3b47333

remove unused import

c38c8b0

paulhcsun marked this pull request as ready for review January 23, 2025 23:08

paulhcsun and others added 3 commits January 23, 2025 15:08

Merge branch 'main' into vpcv2-transit-gateway

6bdd545

Merge branch 'main' into vpcv2-transit-gateway

55f16a3

remove unused test

a5db2e5

GavinZZ requested changes Jan 24, 2025

View reviewed changes

pr feedback

4a53920

aws-cdk-automation previously requested changes Jan 25, 2025

View reviewed changes

Merge branch 'main' into vpcv2-transit-gateway

39e8ae9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ec2-alpha): add Transit Gateway L2 #32956

feat(ec2-alpha): add Transit Gateway L2 #32956

paulhcsun commented Jan 15, 2025

aws-cdk-automation left a comment •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

GavinZZ left a comment

GavinZZ Jan 24, 2025

paulhcsun Jan 24, 2025

GavinZZ Jan 24, 2025

paulhcsun Jan 24, 2025

GavinZZ Jan 24, 2025

paulhcsun Jan 24, 2025

GavinZZ Jan 24, 2025

paulhcsun Jan 24, 2025

GavinZZ Jan 24, 2025

paulhcsun Jan 24, 2025

GavinZZ Jan 24, 2025

paulhcsun Jan 24, 2025

GavinZZ Jan 24, 2025

GavinZZ Jan 24, 2025

paulhcsun Jan 24, 2025

GavinZZ Jan 24, 2025

paulhcsun Jan 24, 2025

GavinZZ Jan 24, 2025

paulhcsun Jan 24, 2025

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation commented Jan 25, 2025

	readonly transitGatewayVpcAttachmentOptions?: ITransitGatewayVpcAttachmentOptions;
	readonly vpcAttachmentOptions?: ITransitGatewayVpcAttachmentOptions;

	export class TransitGatewayActiveRoute extends TransitGatewayRouteBase {
	export class TransitGatewayRoute extends TransitGatewayRouteBase {

feat(ec2-alpha): add Transit Gateway L2 #32956

Are you sure you want to change the base?

feat(ec2-alpha): add Transit Gateway L2 #32956

Conversation

paulhcsun commented Jan 15, 2025

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

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

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

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 25, 2025

AWS CodeBuild CI Report

aws-cdk-automation left a comment •

edited

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading