-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(vpc): additional validation around Subnet Types #4668
Conversation
Try to improve the usability around VPCs and certain Subnet Type configurations. - Make clear that ISOLATED does not mean "no Internet access at all", just no Internet access from the point of view of the current VPC. - If people configure natGateways=0, tell them that they probably meant to configure no PRIVATE subnets. - If people DO configure PRIVATE subnets, make sure they also configure PUBLIC subnets, otherwise we won't be able to place the NAT gateways anywhere useful. - If people end up with a VPC without PRIVATE subnets, the default behavior of `selectSubnets` is pretty useless, because it can never work, and that's not what people are used to from the CDK. Dynamically adjust the selection default to whatever subnet types *are* available. Fixes #3704.
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -166,7 +165,7 @@ export interface SubnetSelection { | |||
* | |||
* At most one of `subnetType` and `subnetGroupName` can be supplied. | |||
* | |||
* @default SubnetType.PRIVATE | |||
* @default SubnetType.PRIVATE (or ISOLATED or PUBLIC if there are no PRIVATE 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.
I'm not sure this description of @default
is super clear... ISOLATED or PUBLIC
? I don't think this is a valid value... Do you mean whichever value resolves to a subnet first in that order?
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 do mean that. I can come up with a description of that behavior that is super technically correct ("The first one out of PRIVATE, ISOLATED, PUBLIC that has subnets of that type configured"), but not one that's easy to read.
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.
Yes, @rix0rrr I like you proposed change (and the value should be -
). It should be something like:
/**
* @default - the default subnet type will be the first available type in the following order: PRIVATE, ISOLATE, PUBLIC
*/
Something like that.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -166,7 +165,7 @@ export interface SubnetSelection { | |||
* | |||
* At most one of `subnetType` and `subnetGroupName` can be supplied. | |||
* | |||
* @default SubnetType.PRIVATE | |||
* @default SubnetType.PRIVATE (or ISOLATED or PUBLIC if there are no PRIVATE 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.
Yes, @rix0rrr I like you proposed change (and the value should be -
). It should be something like:
/**
* @default - the default subnet type will be the first available type in the following order: PRIVATE, ISOLATE, PUBLIC
*/
Something like that.
new Vpc(stack, 'VPC', { | ||
natGateways: 0, | ||
}); | ||
}, /make sure you don't configure any PRIVATE 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.
Perhaps provide the rationale for this in the error message or an accompanying link to an issue or doc
Thank you for contributing! Your pull request is now being automatically merged. |
1 similar comment
Thank you for contributing! Your pull request is now being automatically merged. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
What about NAT instances (https://docs.aws.amazon.com/vpc/latest/userguide/VPC_NAT_Instance.html)? They have zero NAT gateways and one or more private subnets, therefore failing this validation. |
Try to improve the usability around VPCs and certain Subnet Type
configurations.
Make clear that ISOLATED does not mean "no Internet access at all",
just no Internet access from the point of view of the current VPC.
If people configure natGateways=0, tell them that they probably meant
to configure no PRIVATE subnets.
If people DO configure PRIVATE subnets, make sure they also configure
PUBLIC subnets, otherwise we won't be able to place the NAT gateways
anywhere useful.
If people end up with a VPC without PRIVATE subnets, the default
behavior of
selectSubnets
is pretty useless, because it can neverwork, and that's not what people are used to from the CDK. Dynamically
adjust the selection default to whatever subnet types are available.
Fixes #3704.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license