-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
fix(ec2): internet gateway is created even if public subnets are reserved #28607
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const hasPublicSubnets = subnetConfig.some(c => c.subnetType === SubnetType.PUBLIC); | ||
const hasPublicSubnets = subnetConfig.some(c => c.subnetType === SubnetType.PUBLIC && !c.reserved); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue can be solved without this change, but it should be changed in the same way.
@@ -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; |
There was a problem hiding this comment.
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
)
subnet => (subnet.subnetType !== SubnetType.PRIVATE_ISOLATED && subnet.subnetType !== SubnetType.ISOLATED && !subnet.reserved)).length > 0; | |
subnet => (subnet.subnetType === SubnetType.PUBLIC && !subnet.reserved)).length > 0; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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,
});
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
I just left some notes for super minor adjustments.
@@ -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; |
There was a problem hiding this comment.
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.
@@ -379,6 +379,59 @@ describe('vpc', () => { | |||
|
|||
}); | |||
|
|||
test('with only reserved subnets as public subnets, should not create the internet gateway', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a similar test case with a reserved PRIVATE_WITH_EGRESS
subnet?
}); | ||
Template.fromStack(stack).resourceCountIs('AWS::EC2::InternetGateway', 0); | ||
Template.fromStack(stack).resourceCountIs('AWS::EC2::VPCGatewayAttachment', 0); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
], | ||
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"}\]./); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
], | ||
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"}\]./); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @go-to-k and thanks for reviewing @lpizzinidev! Thanks for your patience for getting this over the line.
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
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