feat(ec2): user-defined subnet selectors #10112
Conversation
…n additional filters
| if (placement.subnetName !== undefined) { | ||
| if (placement.subnetGroupName !== undefined) { | ||
| throw new Error('Please use only \'subnetGroupName\' (\'subnetName\' is deprecated and has the same behavior)'); | ||
| } else { | ||
| Annotations.of(this).addWarning('Usage of \'subnetName\' in SubnetSelection is deprecated, use \'subnetGroupName\' instead'); |
There was a problem hiding this comment.
This is API contract misuse and should definitely remain an error that aborts the program.
There was a problem hiding this comment.
The error still occurs if the user specifies both, like before. Now the user will get a friendly reminder that subnetName shouldn't be used, but still allows them to use it -- like before.
Should I change the behavior such that it throws an error?
There was a problem hiding this comment.
Oh I see, I misread.
Well, hmm. I guess it's maybe a little overkill, but fine.
| @@ -460,17 +473,21 @@ abstract class VpcBase extends Resource implements IVpc { | |||
| subnets = this.selectSubnetObjectsByType(type); | |||
| } | |||
|
|
|||
| if (selection.availabilityZones !== undefined) { // Filter by AZs, if specified | |||
| subnets = retainByAZ(subnets, selection.availabilityZones); | |||
There was a problem hiding this comment.
Shouldn't this at least lead to the appropriate filter class being prepended to the list?
Strictly speaking this method is protected, therefore has a public contract to other people that could have inherited from this class and are relying on its current behavior.
There was a problem hiding this comment.
Yes you're correct. That functionality is now in reifySelectionDefaults. I did the same for the one-per-az selection option.
|
Good stuff and great idea! |
mergify
bot
dismissed
rix0rrr’s stale review
Pull request has been modified.
packages/@aws-cdk/aws-ec2/lib/vpc.ts
Outdated
| } | ||
|
|
||
| // Overwrite the provided placement filters | ||
| placement = { ...placement, subnetFilters: subnetFilters }; |
There was a problem hiding this comment.
This leaves the existing .availabilityZones and .onePerAz booleans in place.
Are you sure they're not going to be interpreted again?
There was a problem hiding this comment.
I did remove their interpretation everywhere else but here, but I suppose there's no harm in removing them completely.
…jame-at-amazon/aws-cdk into vpc-subnet-selection-protocol
rix0rrr
changed the title
|
Thank you for contributing! Your pull request will be updated from master 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Hi, @rix0rrr is there a way to filter subnets based on tags? |
|
@buttnomaan9 you can provide your own mechanism to filter subnets https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ec2.SubnetFilter.html |
@flemjame-at-amazon I'm not sure this is actually possible. I've just tried to implement such a filter, but soon hit an issue. (comment edited after rereading and noticing my poor choice of language. No offence intended.) |
You would have to track tags outside of the |
Continuation of #8526
This change adds a feature to SubnetSelection which lets the user provide objects that implements the new
ISubnetSelectorinterface. These objects will be used by the VPC class to choose the subnets from the VPC.This commit also provides an implementation of an ISubnetSelector that lets the user select subnets that contain IP addresses.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license