Initial VPC Validations for CDK #533
Conversation
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
lewijacn
requested review from
AndreKurait,
chelma,
gregschohn,
kartg,
mikaylathompson,
okhasawn and
sumobrian
as code owners
# Conflicts: # deployment/cdk/opensearch-service-migration/lib/network-stack.ts # deployment/cdk/opensearch-service-migration/lib/stack-composer.ts
Signed-off-by: Tanner Lewis <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #533 +/- ##
============================================
- Coverage 76.54% 76.31% -0.24%
- Complexity 1373 1386 +13
============================================
Files 154 154
Lines 5930 5986 +56
Branches 532 537 +5
============================================
+ Hits 4539 4568 +29
- Misses 1038 1054 +16
- Partials 353 364 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
deployment/cdk/opensearch-service-migration/lib/migration-assistance-stack.ts
Outdated
Show resolved
Hide resolved
| } | ||
| let desiredSubnets | ||
| if (uniqueAzPrivateSubnets.length >= azCount) { | ||
| desiredSubnets = uniqueAzPrivateSubnets.slice(0, azCount) |
There was a problem hiding this comment.
Will this list be randomized? Would every customer get the same AZs back if they were setup in say the same 5 AZs?
There was a problem hiding this comment.
I'm all for making our code deterministic. The AZ mappings (us-east-1a.. etc.) are randomized by aws account.
There was a problem hiding this comment.
They should get back the same subnets, but from reading this issue, there isn't a firm guarantee. I've added a sort here to give some confidence when the same subnets are used, but if say some subnets are added to the VPC later I believe both cases could potentially cause issues, and maybe this is something we are okay with calling out as a VPC limitation. I'm not aware of a better way to handle this outside of storing state about the subnets ourselves somewhere but that presents other issues.
There was a problem hiding this comment.
See comment I made about the sort
deployment/cdk/opensearch-service-migration/lib/network-stack.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
| } | ||
| let desiredSubnets | ||
| if (uniqueAzPrivateSubnets.length >= azCount) { | ||
| desiredSubnets = uniqueAzPrivateSubnets.slice(0, azCount).sort() |
There was a problem hiding this comment.
I think we need to sort before slicing here
Signed-off-by: Tanner Lewis <[email protected]>
Description
The goal of this change is to add more visibility, and visibility earlier in a deployment, to a user when a provided or created VPC does not meet the current VPC requirements to work with our solution.
Issues Resolved
https://opensearch.atlassian.net/browse/MIGRATIONS-1496
Testing
Jest tests and cloud deployments
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.