Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ec2): internet gateway is created even if public subnets are reserved #28607

Merged
merged 9 commits into from
Mar 1, 2024
Merged

fix(ec2): internet gateway is created even if public subnets are reserved #28607

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
40ac987
fix(ec2): internet gateway is created even if public subnets are rese…
go-to-k Jan 7, 2024
9bc57ad
integ test
go-to-k Jan 7, 2024
c0943af
change test name
go-to-k Jan 7, 2024
5d098ea
change for determineNatGatewayCount
go-to-k Jan 7, 2024
ae7e200
change unit tests
go-to-k Jan 8, 2024
1a421ec
Merge branch 'main' into ec2-vpc-reserved-ig
paulhcsun Feb 22, 2024
ad80267
Merge branch 'main' into ec2-vpc-reserved-ig
paulhcsun Mar 1, 2024
a601f70
Merge branch 'main' into ec2-vpc-reserved-ig
paulhcsun Mar 1, 2024
12c82b6
Merge branch 'main' into ec2-vpc-reserved-ig
mergify[bot] Mar 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-ec2/lib/vpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1491,7 +1491,7 @@ export class Vpc extends VpcBase {

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;
Copy link
Contributor Author

@go-to-k go-to-k Jan 7, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
      });

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Create an Internet Gateway and attach it if necessary
if (allowOutbound && createInternetGateway) {
Expand Down
20 changes: 20 additions & 0 deletions packages/aws-cdk-lib/aws-ec2/test/vpc.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,26 @@ describe('vpc', () => {

});

test('with reserved public subnets, should not create the internet gateway', () => {
const stack = getTestStack();
const vpc = new Vpc(stack, 'TheVPC', {
subnetConfiguration: [
{
subnetType: SubnetType.PRIVATE_ISOLATED,
name: 'isolated',
},
{
subnetType: SubnetType.PUBLIC,
name: 'public',
reserved: true,
},
],
});
Template.fromStack(stack).resourceCountIs('AWS::EC2::InternetGateway', 0);
Template.fromStack(stack).resourceCountIs('AWS::EC2::VPCGatewayAttachment', 0);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

});

test('with subnets and reserved subnets defined, VPC subnet count should not contain reserved subnets ', () => {
const stack = getTestStack();
new Vpc(stack, 'TheVPC', {
Expand Down