fix(ec2-alpha): readme updates, new unit tests, logic update #33086

shikha372 · 2025-01-23T07:44:24Z

Issue # (if applicable)

Closes #30762 .

Reason for this change

Adding more unit tests to meet the global coverage before module graduation to developer-preview.

Description of changes

Add more unit test case to cover all if branches test cases.
As per the discussion with service team, added optional field under IGW to allow users to choose the subnets for gateway routing, as there can be Public Subnet without an IGW attached( eg. using VPNGW to access internet).
Update IPAM README to higlight the problem of IPAM pool deletion as discussed with service team.
Update SubnetV2 README to higlight that a custom route table is being created through CDK.

Describe any new or updated permissions being added

No changes to IAM permissions.

Description of how you validated changes

yarn build
yarn test

yarn run v1.22.21
$ cdk-test
PASS test/ipam.test.ts
PASS test/subnet-v2.test.ts
PASS test/vpc-tagging.test.ts
PASS test/util.test.ts
PASS test/route.test.ts
PASS test/vpc-add-method.test.ts
PASS test/vpcv2-import.test.ts
PASS test/vpc-v2.test.ts

=============================== Coverage summary ===============================
Statements   : 89.88% ( 640/712 )
Branches     : 81.68% ( 223/273 )
Functions    : 82.6% ( 133/161 )
Lines        : 89.89% ( 614/683 )
================================================================================

Test Suites: 8 passed, 8 total
Tests:       126 passed, 126 total
Snapshots:   0 total
Time:        2.244 s
Ran all test suites.

Verifying integration test snapshots...

  UNCHANGED  integ.byoip-ipv6 0.888s
  UNCHANGED  integ.ipam 0.928s
  UNCHANGED  integ.subnet-v2 1.036s
  UNCHANGED  integ.vpc-v2-alpha 1.047s
  UNCHANGED  integ.test-import 1.053s
  UNCHANGED  integ.peering-cross-account 1.101s
  UNCHANGED  integ.vpc-v2-tagging 1.264s
  UNCHANGED  integ.route-v2 1.29s

Snapshot Results: 

Tests:    8 passed, 8 total
Tests successful. Total time (4.5s) | /Users/shikagg/vpc_peering/aws-cdk/node_modules/jest/bin/jest.js (2.7s) | integ-runner (1.8s)
✨  Done in 4.87s.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

BREAKING CHANGE: operatingRegion property under IPAM class is now renamed to operatingRegions.

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

codecov · 2025-01-23T08:09:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.78%. Comparing base (1b666db) to head (515c6fb).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33086   +/-   ##
=======================================
  Coverage   80.78%   80.78%           
=======================================
  Files         232      232           
  Lines       14111    14111           
  Branches     2453     2453           
=======================================
  Hits        11400    11400           
  Misses       2431     2431           
  Partials      280      280

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

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

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

samson-keung · 2025-01-24T18:06:07Z

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

@@ -514,7 +514,7 @@ export class Ipam extends Resource {
    if (props?.ipamName) {


Is it possible for ipamName to be a token?

samson-keung · 2025-01-24T18:13:46Z

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

@@ -514,7 +514,7 @@ export class Ipam extends Resource {
    if (props?.ipamName) {
      Tags.of(this).add(NAME_TAG, props.ipamName);
    }
-    if (!props?.operatingRegion && !Stack.of(this).region) {
+    if (!props?.operatingRegion && Token.isUnresolved(Stack.of(this).region)) {


2 questions:

The !props?.operatingRegion condition catch the case where the operatingRegion prop is not provided by user (i.e. value being undefined). What if operatingRegion is an empty array?

For the 2nd condition, why do we add Token.isUnresolved? I would thought that if user is not providing any regions, then we can use Stack.of(this).region, even if it is a token. I see further down on line 521 that token is allowed as a value assigned to this.operatingRegions.

Good point for 1, changed the condition to check for empty array as well.
For 2, I wanted to prevent the deployment for stacks that has any unresolved token reference while synth, but looking at the template again i think even with no environment variable, it will look like below which is going to be resolved at deployment time which should be okay, in any case there will be always be a region value and i don't think we need to check for this, updated the condition.
{
"RegionName": {
"Ref": "AWS::Region"
}
}

samson-keung · 2025-01-24T18:15:03Z

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

@@ -608,12 +608,8 @@ export class RouteTargetType {
  readonly endpoint?: IVpcEndpoint;

  constructor(props: RouteTargetProps) {
-    if ((props.gateway && props.endpoint) || (!props.gateway && !props.endpoint)) {
-      throw new Error('Exactly one of `gateway` or `endpoint` must be specified.');


I see this is moved into the Route constructor. Curious about the reasoning

While adding unit tests, stack was not throwing expected error while using addRoute method with both endpoint and target as an input, so moved it from RouteTargetType class to Route
addRoute

samson-keung · 2025-01-24T18:31:18Z

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

+          this.addDefaultInternetRoute(subnet, igw, options);
+          processedSubnets.add(subnet.node.id);
+        };
+      }); // If there are no input subnets defined, default route will be added to all public subnets


My understanding is that if a subnet has a route to the internet, then it becomes a public subnet.

So should we allow adding internet gateway to private subnets? If we do, should we warn the users so that they do not accidentally make their subnets, intended to be private, public?

Yes, this was my understanding as well but after talking to service team there can be cases with VPNGW type of attachment where customer might be leveraging their on-premises network to route the traffic to internet.
I was thinking of adding warning as well for private subnets, but there can be different categories under private subnets too. But I still think notifying is good idea, so added a warning for all kinds of subnets except PUBLIC.

there can be cases with VPNGW type of attachment where customer might be leveraging their on-premises network to route the traffic to internet.

And in such case, it won't be a private subnet? Is it correct to say that "Private Subnet" is a concept to describe a subnet that is configured to not able to access the internet? I am thinking if we should keep the public vs private subnet concept in CDK since, say, a private subnet may become public due to drifting.

Anyways, this is not a blocking comment. We can discuss further offline. Thank you for explaining.

aws-cdk-automation

(This review is outdated)

fixing tests

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

samson-keung · 2025-01-27T18:06:04Z

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

+          this.addDefaultInternetRoute(subnet, igw, options);
+          processedSubnets.add(subnet.node.id);
+        };
+      }); // If there are no input subnets defined, default route will be added to all public subnets


there can be cases with VPNGW type of attachment where customer might be leveraging their on-premises network to route the traffic to internet.

And in such case, it won't be a private subnet? Is it correct to say that "Private Subnet" is a concept to describe a subnet that is configured to not able to access the internet? I am thinking if we should keep the public vs private subnet concept in CDK since, say, a private subnet may become public due to drifting.

Anyways, this is not a blocking comment. We can discuss further offline. Thank you for explaining.

samson-keung · 2025-01-27T18:09:27Z

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

@@ -511,7 +511,7 @@ export class Ipam extends Resource {
    if (props?.ipamName) {
      Tags.of(this).add(NAME_TAG, props.ipamName);
    }
-    if (!props?.operatingRegion && !Stack.of(this).region) {
+    if (props?.operatingRegion && (props.operatingRegion.length === 0)) {


nit: The operatingRegion is a list so it should be named operatingRegions.

addressed in latest rev but this would be breaking change which should be okay for alpha, added in PR description

BREAKING CHANGE: operatingRegion property under IPAM class is now renamed to operatingRegions.

samson-keung · 2025-01-27T18:12:26Z

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

+   *
+   * @default - route created for all subnets with Type `SubnetType.Public`
+   */
+  readonly subnets?: SubnetSelection[];


nit: Naming this subnetSelections seems better so users are not confused that it may be of the type SubnetV2[].

actually, tried to keep it consistent per the functions in main lib with the current options in main lib for adding interfaces and endpoints, https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ec2/lib/vpc-endpoint.ts#L110 , where mySubnet is a new subnet defined using SubnetV2.

myVpc.addInternetGateway({
ipv4Destination: '192.168.0.0/16',
subnets: [mySubnet],
});

aws-cdk-automation

(This review is outdated)

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

aws-cdk-automation

(This review is outdated)

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

samson-keung

Thank you. LGTM

samson-keung · 2025-01-27T21:52:55Z

Code looks good. I missed that the title needs to be updated as well. chore will not be included in change log, hence, the BERAKING CHANGE line will not be picked up I think.

aws-cdk-automation

(This review is outdated)

aws-cdk-automation

(This review is outdated)

Dismissing outdated PRLinter review.

aws-cdk-automation · 2025-01-27T21:56:14Z

AWS CodeBuild CI Report

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

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

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

shikha372 · 2025-01-27T22:15:01Z

@Mergifyio update

mergify · 2025-01-27T22:15:18Z

update

☑️ Nothing to do

#commits-behind > 0 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]
queue-position = -1 [📌 update requirement]

mergify · 2025-01-27T22:26:40Z

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 · 2025-01-27T22:27:04Z

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.

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

aws-cdk-automation requested a review from a team January 23, 2025 07:44

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

shikha372 force-pushed the shikagg/vpcv2_graduation branch 2 times, most recently from c2f2e6b to 252fcec Compare January 23, 2025 07:52

shikha372 changed the title ~~chore(VpcV2): increasing test coverage for graduation~~ chore(vpcv2): increasing test coverage for graduation Jan 23, 2025

shikha372 force-pushed the shikagg/vpcv2_graduation branch 4 times, most recently from 5f3b3c6 to bf65780 Compare January 24, 2025 00:36

shikha372 changed the title ~~chore(vpcv2): increasing test coverage for graduation~~ chore(vpcv2): fulfill prerequisites for module graduation Jan 24, 2025

shikha372 changed the title ~~chore(vpcv2): fulfill prerequisites for module graduation~~ chore(ec2-alpha): fulfill prerequisites for module graduation Jan 24, 2025

shikha372 marked this pull request as ready for review January 24, 2025 00:51

shikha372 force-pushed the shikagg/vpcv2_graduation branch 3 times, most recently from d0d5d22 to 186fa82 Compare January 24, 2025 01:19

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 24, 2025

shikha372 force-pushed the shikagg/vpcv2_graduation branch from 186fa82 to e273b35 Compare January 24, 2025 06:10

samson-keung self-assigned this Jan 24, 2025

samson-keung requested changes Jan 24, 2025

View reviewed changes

samson-keung changed the title ~~chore(ec2-alpha): fulfill prerequisites for module graduation~~ chore(ec2-alpha): readme updates, new unit tests, logic update Jan 24, 2025

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 24, 2025

shikha372 force-pushed the shikagg/vpcv2_graduation branch from e273b35 to b258ed7 Compare January 25, 2025 00:00

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

View reviewed changes

chore(vpcv2): increasing test coverage for graduation

e7125f3

fixing tests

shikha372 force-pushed the shikagg/vpcv2_graduation branch from b258ed7 to e7125f3 Compare January 25, 2025 01:25

Merge branch 'main' into shikagg/vpcv2_graduation

0de3fd3

samson-keung requested changes Jan 27, 2025

View reviewed changes

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

View reviewed changes

addressing last comments

9f997a6

shikha372 force-pushed the shikagg/vpcv2_graduation branch from 6da0263 to 9f997a6 Compare January 27, 2025 20:39

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

View reviewed changes

Merge branch 'main' into shikagg/vpcv2_graduation

515c6fb

samson-keung approved these changes Jan 27, 2025

View reviewed changes

samson-keung added the pr/do-not-merge This PR should not be merged at this time. label Jan 27, 2025

shikha372 changed the title ~~chore(ec2-alpha): readme updates, new unit tests, logic update~~ fix(ec2-alpha): readme updates, new unit tests, logic update Jan 27, 2025

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

View reviewed changes

shikha372 added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jan 27, 2025

shikha372 removed the pr/do-not-merge This PR should not be merged at this time. label Jan 27, 2025

mergify bot merged commit bcb7f9b into main Jan 27, 2025
44 checks passed

mergify bot deleted the shikagg/vpcv2_graduation branch January 27, 2025 22:26

github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ec2-alpha): readme updates, new unit tests, logic update #33086

fix(ec2-alpha): readme updates, new unit tests, logic update #33086

shikha372 commented Jan 23, 2025 •

edited

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading

samson-keung Jan 24, 2025

samson-keung Jan 24, 2025

shikha372 Jan 25, 2025 •

edited

Loading

samson-keung Jan 24, 2025

shikha372 Jan 24, 2025

samson-keung Jan 24, 2025

shikha372 Jan 25, 2025

samson-keung Jan 27, 2025

aws-cdk-automation left a comment •

edited

Loading

samson-keung Jan 27, 2025

samson-keung Jan 27, 2025

shikha372 Jan 27, 2025

samson-keung Jan 27, 2025

shikha372 Jan 27, 2025

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

samson-keung left a comment

samson-keung commented Jan 27, 2025

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation commented Jan 27, 2025

shikha372 commented Jan 27, 2025

mergify bot commented Jan 27, 2025

mergify bot commented Jan 27, 2025

github-actions bot commented Jan 27, 2025

		@@ -514,7 +514,7 @@ export class Ipam extends Resource {
		if (props?.ipamName) {

fix(ec2-alpha): readme updates, new unit tests, logic update #33086

fix(ec2-alpha): readme updates, new unit tests, logic update #33086

Conversation

shikha372 commented Jan 23, 2025 • edited Loading

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

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shikha372 Jan 25, 2025 • 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

aws-cdk-automation left a comment • 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

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

samson-keung left a comment

Choose a reason for hiding this comment

samson-keung commented Jan 27, 2025

aws-cdk-automation left a comment • edited Loading

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 27, 2025

AWS CodeBuild CI Report

shikha372 commented Jan 27, 2025

mergify bot commented Jan 27, 2025

☑️ Nothing to do

mergify bot commented Jan 27, 2025

github-actions bot commented Jan 27, 2025

shikha372 commented Jan 23, 2025 •

edited

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading

shikha372 Jan 25, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading