fix(ec2): internet gateway is created even if public subnets are reserved by go-to-k · Pull Request #28607 · aws/aws-cdk

go-to-k · 2024-01-07T16:34:07Z

This PR fixes that Internet Gateway will be created even if (all) public subnets are reserved.

The reserved option is for not actually creating subnet resources. So IGW should not be created if all public subnets are reserved, because there is no public subnets in the VPC.

It would be appropriate to consider the reserved option since we originally did not want to create an IGW if there was no public subnets.

Also, if this bug is not fixed, it will go to the code where the NatGateway is created without public subnets. (This will be stopped with another error, but...)

Closes #28593.

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

…rved

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. 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.

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

go-to-k · 2024-01-07T18:20:41Z

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

  const hasPrivateSubnets = subnetConfig.some(c => (c.subnetType === SubnetType.PRIVATE_WITH_EGRESS
    || c.subnetType === SubnetType.PRIVATE || c.subnetType === SubnetType.PRIVATE_WITH_NAT) && !c.reserved);
-  const hasPublicSubnets = subnetConfig.some(c => c.subnetType === SubnetType.PUBLIC);
+  const hasPublicSubnets = subnetConfig.some(c => c.subnetType === SubnetType.PUBLIC && !c.reserved);


This issue can be solved without this change, but it should be changed in the same way.

go-to-k · 2024-01-07T18:58:22Z

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

    const createInternetGateway = props.createInternetGateway ?? true;
    const allowOutbound = this.subnetConfiguration.filter(
-      subnet => (subnet.subnetType !== SubnetType.PRIVATE_ISOLATED && subnet.subnetType !== SubnetType.ISOLATED)).length > 0;
+      subnet => (subnet.subnetType !== SubnetType.PRIVATE_ISOLATED && subnet.subnetType !== SubnetType.ISOLATED && !subnet.reserved)).length > 0;


I honestly thought the following would be good here, but I'll leave it as is. (Because even as it is, the code after this will make a proper error if there is no PUBLIC and there is PRIVATE_WITH_EGRESS)

Suggested change

subnet => (subnet.subnetType !== SubnetType.PRIVATE_ISOLATED && subnet.subnetType !== SubnetType.ISOLATED && !subnet.reserved)).length > 0;

subnet => (subnet.subnetType === SubnetType.PUBLIC && !subnet.reserved)).length > 0;

The two implementations are different and subnet => (subnet.subnetType === SubnetType.PUBLIC && !subnet.reserved)).length > 0; would not allow the creation of internet or NAT gateways for PRIVATE_WITH_EGRESS subnets (unless at least a public subnet is present).
Can you please elaborate on why you considered making the change?

subnet => (subnet.subnetType === SubnetType.PUBLIC && !subnet.reserved)).length > 0; would not allow the creation of internet or NAT gateways for PRIVATE_WITH_EGRESS subnets (unless at least a public subnet is present).

The reason is that trying to create Nat gateways with PRIVATE_WITH_EGRESS without a public subnet in the first place would result in an error.

https://github.com/aws/aws-cdk/blob/v2.118.0/packages/aws-cdk-lib/aws-ec2/lib/vpc.ts#L2416-L2419

As for the Internet Gateway, I questioned whether there was a need to create it without a public subnet (like the following code). However, since it is not good to carelessly change the behavior, I left it as it was, just in case.

new Vpc(stack, 'TheVPC', { subnetConfiguration: [ { subnetType: SubnetType.PRIVATE_WITH_EGRESS, name: 'egress', }, ], natGateways: 0, });

Thanks for the clarification.
Yeah, I'd just leave it as is unless we have a specific reason to change it to avoid potential errors with existing deployments.

lpizzinidev

Looks good 👍
I just left some notes for super minor adjustments.

lpizzinidev · 2024-01-08T12:15:43Z

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

    const createInternetGateway = props.createInternetGateway ?? true;
    const allowOutbound = this.subnetConfiguration.filter(
-      subnet => (subnet.subnetType !== SubnetType.PRIVATE_ISOLATED && subnet.subnetType !== SubnetType.ISOLATED)).length > 0;
+      subnet => (subnet.subnetType !== SubnetType.PRIVATE_ISOLATED && subnet.subnetType !== SubnetType.ISOLATED && !subnet.reserved)).length > 0;


Thanks for the clarification.
Yeah, I'd just leave it as is unless we have a specific reason to change it to avoid potential errors with existing deployments.

lpizzinidev · 2024-01-08T12:16:39Z

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


    });

+    test('with only reserved subnets as public subnets, should not create the internet gateway', () => {


Can you please add a similar test case with a reserved PRIVATE_WITH_EGRESS subnet?

lpizzinidev · 2024-01-08T12:16:47Z

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

+      });
+      Template.fromStack(stack).resourceCountIs('AWS::EC2::InternetGateway', 0);
+      Template.fromStack(stack).resourceCountIs('AWS::EC2::VPCGatewayAttachment', 0);
+


Suggested change

lpizzinidev · 2024-01-08T12:16:54Z

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

+        ],
+        natGateways: 1,
+      })).toThrow(/If you configure PRIVATE subnets in 'subnetConfiguration', you must also configure PUBLIC subnets to put the NAT gateways into \(got \[{"subnetType":"Private","name":"egress"}\]./);
+


Suggested change

lpizzinidev · 2024-01-08T12:17:01Z

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

+        ],
+        natGateways: 1,
+      })).toThrow(/If you configure PRIVATE subnets in 'subnetConfiguration', you must also configure PUBLIC subnets to put the NAT gateways into \(got \[{"subnetType":"Public","name":"public","reserved":true},{"subnetType":"Private","name":"egress"}\]./);
+


Suggested change

go-to-k · 2024-01-08T13:39:21Z

@lpizzinidev

Thanks, I fixed!

lpizzinidev

Thanks 👍

paulhcsun

Thanks for the fix @go-to-k and thanks for reviewing @lpizzinidev! Thanks for your patience for getting this over the line.

Pull request has been modified.

mergify · 2024-03-01T01:05:16Z

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).

aws-cdk-automation · 2024-03-01T01:34:03Z

AWS CodeBuild CI Report

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

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

mergify · 2024-03-01T01:34:25Z

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).

fix(ec2): internet gateway is created even if public subnets are rese…

40ac987

…rved

github-actions bot added distinguished-contributor [Pilot] contributed 50+ PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p2 labels Jan 7, 2024

aws-cdk-automation requested a review from a team January 7, 2024 16:34

aws-cdk-automation previously requested changes Jan 7, 2024

View reviewed changes

integ test

9bc57ad

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 7, 2024

go-to-k added 2 commits January 8, 2024 02:25

change test name

c0943af

change for determineNatGatewayCount

5d098ea

go-to-k commented Jan 7, 2024

View reviewed changes

lpizzinidev suggested changes Jan 8, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 8, 2024

change unit tests

ae7e200

go-to-k requested a review from lpizzinidev January 8, 2024 13:34

lpizzinidev approved these changes Jan 8, 2024

View reviewed changes

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

github-actions bot mentioned this pull request Feb 1, 2024

Monthly PR metrics report - Jan 2024 #28952

Closed

Merge branch 'main' into ec2-vpc-reserved-ig

1a421ec

paulhcsun previously approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'main' into ec2-vpc-reserved-ig

ad80267

Merge branch 'main' into ec2-vpc-reserved-ig

a601f70

paulhcsun approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'main' into ec2-vpc-reserved-ig

12c82b6

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

mergify bot merged commit 985c7e4 into aws:main Mar 1, 2024

go-to-k deleted the ec2-vpc-reserved-ig branch June 28, 2024 17:39

	subnet => (subnet.subnetType !== SubnetType.PRIVATE_ISOLATED && subnet.subnetType !== SubnetType.ISOLATED && !subnet.reserved)).length > 0;
	subnet => (subnet.subnetType === SubnetType.PUBLIC && !subnet.reserved)).length > 0;


		});

		test('with only reserved subnets as public subnets, should not create the internet gateway', () => {

Conversation

go-to-k commented Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

go-to-k Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

go-to-k commented Jan 8, 2024

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

paulhcsun left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 1, 2024

Uh oh!

aws-cdk-automation commented Mar 1, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Mar 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

go-to-k commented Jan 7, 2024 •

edited

Loading

go-to-k Jan 7, 2024 •

edited

Loading