-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix(ec2): dual-stack vpc without private subnets creates EgressOnlyInternetGateway (under feature flag) #34437
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
base: main
Are you sure you want to change the base?
fix(ec2): dual-stack vpc without private subnets creates EgressOnlyInternetGateway (under feature flag) #34437
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.
(This review is outdated)
82ac2e3
to
a0d833f
Compare
7332c64
to
427b4a2
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
For the PR title please change it to describe the bug and not the actual fix. |
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.
Nice work with this change! Just a few comments regarding the naming of the feature flag.
@@ -137,6 +137,7 @@ export const DYNAMODB_TABLE_RETAIN_TABLE_REPLICA = '@aws-cdk/aws-dynamodb:retain | |||
export const LOG_USER_POOL_CLIENT_SECRET_VALUE = '@aws-cdk/cognito:logUserPoolClientSecretValue'; | |||
export const PIPELINE_REDUCE_CROSS_ACCOUNT_ACTION_ROLE_TRUST_SCOPE = '@aws-cdk/pipelines:reduceCrossAccountActionRoleTrustScope'; | |||
export const S3_TRUST_KEY_POLICY_FOR_SNS_SUBSCRIPTIONS = '@aws-cdk/s3-notifications:addS3TrustKeyPolicyForSnsSubscriptions'; | |||
export const ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC = '@aws-cdk/ec2:removeEgressOnlyGatewayFromPublicSubnetVPC'; |
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.
nit: I know it's not consistent but the convention is to prefix the service here with aws-
. Also we should prefix the constant with the module name EC2-
and update the name of the feature flag to more accurately reflect the behaviour.
export const ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC = '@aws-cdk/ec2:removeEgressOnlyGatewayFromPublicSubnetVPC'; | |
export const EC2_REQUIRE_PRIVATE_SUBNETS_FOR_EGRESSONLYINTERNETGATEWAY = '@aws-cdk/aws-ec2:requirePrivateSubnetsForEgressOnlyInternetGateway'; |
@@ -1576,6 +1577,17 @@ export const FLAGS: Record<string, FlagInfo> = { | |||
}, | |||
|
|||
////////////////////////////////////////////////////////////////////// | |||
[ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC]: { | |||
type: FlagType.BugFix, | |||
summary: 'Remove EgressOnlyGateway resource when a a double stack vpc has only public subnets', |
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 doesn't seem entirely correct. Rather than "removing EgressOnlyGateway
when a stack only has public subnets", a more accurate summary could be "When enabled, the EgressOnlyGateway resource is only created if private subnets are defined in the VPC."
@@ -1649,7 +1649,13 @@ export class Vpc extends VpcBase { | |||
} | |||
|
|||
// Create an Egress Only Internet Gateway and attach it if necessary | |||
if (this.useIpv6 && this.privateSubnets) { | |||
|
|||
const redundantEgressOnlyGatewayRemovalFeatureFlag = |
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.
nit: variable name
const redundantEgressOnlyGatewayRemovalFeatureFlag = | |
const isRequirePrivateSubnetsForEgressOnlyIgw = |
@@ -2747,6 +2747,90 @@ describe('vpc', () => { | |||
}, | |||
}); | |||
}); | |||
test('(default)EgressOnlyInternetGateWay is created when no private subnet configured in dual stack', () => { |
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.
Instead of (default) EgressOnlyIGW....
maybe change this to say "EgressOnlyIGW is created when....in dual stack when feature flag is enabled."
// THEN | ||
Template.fromStack(stack).resourceCountIs('AWS::EC2::EgressOnlyInternetGateway', 1); | ||
}); | ||
test('(default)EgressOnlyInternetGateWay is created when private subnet configured in dual stack', () => { |
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.
Same as above test.
Template.fromStack(stack).resourceCountIs('AWS::EC2::EgressOnlyInternetGateway', 1); | ||
}); | ||
|
||
test('(feature flag ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC)EgressOnlyInternetGateWay is created when private subnet configured in dual stack', () => { |
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.
nit:
test('(feature flag ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC)EgressOnlyInternetGateWay is created when private subnet configured in dual stack', () => { | |
test('EgressOnlyInternetGateWay is created when private subnet configured in dual stack when feature flag is enabled', () => { |
// THEN | ||
Template.fromStack(stack).resourceCountIs('AWS::EC2::EgressOnlyInternetGateway', 1); | ||
}); | ||
test(' (feature flag ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC)EgressOnlyInternetGateWay is not created when private subnet configured in dual stack', () => { |
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.
test(' (feature flag ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC)EgressOnlyInternetGateWay is not created when private subnet configured in dual stack', () => { | |
test(' (feature flag ENABLE_E2_REMOVE_EGRESSONLYGATEWAY_FROM_PUBLIC_SUBNET_VPC)EgressOnlyInternetGateWay is not created when no private subnets are configured in dual stack', () => { |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Issue # (if applicable)
Closes #30981.
Reason for this change
-> EgressOnlyInternetGateway was been created even without any private subnets
Description of changes
-> Fixed the condition that determins if a EgressOnlyInternetGateway will be created
-> Added feature flag
Describe any new or updated permissions being added
N/A
Description of how you validated changes
I added two new unit tests that checks if EgressOnlyInternetGateway is created without a private subnet
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license