-
Notifications
You must be signed in to change notification settings - Fork 38
fix: 320 Subnet validation in Network Configuration fails when multiple subnets exist in the VPC #321
fix: 320 Subnet validation in Network Configuration fails when multiple subnets exist in the VPC #321
Conversation
9347834
to
2dbb32d
Compare
2dbb32d
to
9aab406
Compare
Thanks for the PR @chotalia! Based on a review of the code and the tests passing without any changes, this change appears to be functionally equivalent to the previous implementation. Have you tested this change with your setup to confirm it fixes the error you see? |
…le subnets exist in the VPC. fixes PrefectHQ#320 Subnet validation in Network Configuration fails when multiple subnets exist in the VPC. - Resolved an issue where defining subnets in network configuration would erroneously report them as missing in the VPC. - Adjusted the boto3 describe_subnets call to specifically compare against SubnetId, as it retrieves full subnet details.
9aab406
to
b31d9ed
Compare
Thanks @desertaxle for reviewing the PR. Please find more details. The code was specifically failing when more than 1 subnets exist in VPC in AWS. Existing tests were testing only when VPC has single subnet hence they were passing. I have added 3 new tests. I can confirm these 3 new tests failed without the change of the code. There was two main issues in this line: prefect-aws/prefect_aws/workers/ecs_worker.py Line 1354 in 525c917
List of Boolean Values: For example, let's say config_subnets contains a single valid subnet ID and the VPC has 3 subnets. The resulting boolean list might look like [True, False, False] if only the first subnet in subnets matches. The use of all() on this list will then return False, even though the subnet ID from config_subnets was valid for one of the subnets in the VPC. False Positives: Boto3 describe_subnets retrieves not only subnet ids but all the subnet specific attributes. The conf_sn in sn.values() checks if the subnet ID from config_subnets exists anywhere in the values of the subnet dictionary. This is not precise, as we only want to match against the actual subnet IDs, not other values like availability zones, states, etc. |
@desertaxle, @urimandujano this has been a show stopper for us to migrate to Prefect 2 Appreciate if you could look sooner if possible? Thank you in advance. |
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.
LGTM!
Closes #320
Subnet validation in Network Configuration fails when multiple subnets exist in the VPC.
Checklist
pre-commit
checks.pre-commit install && pre-commit run --all
locally for formatting and linting.mkdocs serve
view documentation locally.