fix(ec2): descriptive error message when selecting 0 subnets #2025
Conversation
Selecting 0 subnets is invalid for most CFN resources. We move the check for this upstream to the VPC construct, which can throw a nicely descriptive error message if it happens to not contain any subnets that match the type we're looking for. The alternative is that the error happens during CloudFormation deployment, but then it's not very clear anymore what the cause for the error was. Fixes #2011.
|
|
||
| const nets = this.subnets(placement); | ||
| if (nets.length === 0) { | ||
| throw new Error(`There are no ${describePlacement(placement)} in this VPC. Use a different VPC placement strategy.`); |
There was a problem hiding this comment.
Could we maybe hint at what strategies would have matched something? Or are placement strategies too complex?
There was a problem hiding this comment.
They are not necessarily, but what are you proposing?
"The VPC network contains 1 Public subnet group with name "Public" and 2 Private subnets groups with names "MyDatabases","MyLambdas"
?
Not sure that a longer error message is necessarily going to make things better. We could, of course.
There was a problem hiding this comment.
I mean I wouldn't put the Use a different VPC placement strategy if you can't hint at which are "valid".
There was a problem hiding this comment.
I used that terminology on purpose because:
- It's okay to state an error, but it's better to state a path to a solution.
- I want to use the words "VPC placement" here on purpose, because previously asked questions have shown that people don't think of this term.
Although maybe the better solution is to change the parameter name to subnetSelection, or something, under the "s" where people that already know things about AWS and CFN are going to look for it.
| if (placement.subnetName !== undefined) { | ||
| return `subnets named '${placement.subnetName}'`; | ||
| } | ||
| return '???'; |
There was a problem hiding this comment.
If you get there you've effectively reached the default behavior, I believe. Then you should probably return private subnets. If you are willing to assume that the defaults have always been reified before you get here, then why not throw instead of returning ????
There was a problem hiding this comment.
I was thinking to throw, but at the same time I didn't want to block a customer from using this construct if we ever forget a code path. Rather have ugly behavior but allow the user to continue (so that we can fix the issue when we have capacity) rather than block them completely by throwing an error and now we have to scramble to get a fix out.
There was a problem hiding this comment.
Right but then at least avoid the question marks... Maybe JSON.stringify what you've been given.
BREAKING CHANGE: `vpcPlacement` has been renamed to `vpcSubnets` on all objects, `subnetsToUse` has been renamed to `subnetType`. `natGatewayPlacement` has been renamed to `natGatewaySubnets`.
Selecting 0 subnets is invalid for most CFN resources. We move the check
for this upstream to the VPC construct, which can throw a nicely
descriptive error message if it happens to not contain any subnets that
match the type we're looking for.
The alternative is that the error happens during CloudFormation
deployment, but then it's not very clear anymore what the cause for
the error was.
Fixes #2011.
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.