CORS-4352: aws: validations to ensure install-config is valid for dualstack install#10380
CORS-4352: aws: validations to ensure install-config is valid for dualstack install#10380tthvo wants to merge 5 commits intoopenshift:mainfrom
Conversation
Instance types must be Nitro-based in order to enable the IPv6 HTTP Endpoint for IMDS [0] and be able to reach IPv6 nameserver [1]. References [0] https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/configuring-instance-metadata-service.html [1] https://docs.aws.amazon.com/whitepapers/latest/ipv6-on-aws/designing-dns-for-ipv6.html#dns-resolution-within-a-vpc
In dualstack networking, we expect the BYO subnets to have an associated IPv4 and IPv6 CIDR to assign both IP family addresses to nodes.
In dualstack networking, edge compute pool is only valid if: - using existing subnets; and - using local zones Wavelength zones do not support IPv6.
The subnet types "public", "private" and "edge" are extracted into constants to pass golint.
openshift-ci-robot
added
the
jira/valid-reference
|
@tthvo: This pull request references CORS-4352 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/cc @sadasu |
|
@tthvo: This pull request references CORS-4352 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/asset/installconfig/aws/validation_test.go (1)
2322-2328: Consider addingHypervisorfield tom6a.xlargefor consistency.The
m6a.xlargeinstance type is missing theHypervisorfield, while all other instance types now include it. Althoughm6a.xlargeis currently only used for SEV-SNP tests and not dual-stack tests, adding the field maintains consistency and prevents potential issues if this type is used in future dual-stack test scenarios.Proposed fix
"m6a.xlarge": { DefaultVCpus: 4, MemInMiB: 16384, Arches: []string{string(ec2types.ArchitectureTypeX8664)}, + Hypervisor: string(ec2types.InstanceTypeHypervisorNitro), Features: []string{"amd-sev-snp"}, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/validation_test.go` around lines 2322 - 2328, Add the missing Hypervisor field to the m6a.xlarge instance map entry so it matches other entries; edit the map entry for "m6a.xlarge" in pkg/asset/installconfig/aws/validation_test.go and add a Hypervisor key with the same value used by other AMD/SEV-SNP instance entries (for example "nitro") to maintain consistency and prevent future dual-stack/test issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/asset/installconfig/aws/validation_test.go`:
- Around line 2322-2328: Add the missing Hypervisor field to the m6a.xlarge
instance map entry so it matches other entries; edit the map entry for
"m6a.xlarge" in pkg/asset/installconfig/aws/validation_test.go and add a
Hypervisor key with the same value used by other AMD/SEV-SNP instance entries
(for example "nitro") to maintain consistency and prevent future dual-stack/test
issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 948a71d4-64e3-43a2-9387-9232f7eaca31
📒 Files selected for processing (4)
pkg/asset/installconfig/aws/instancetypes.gopkg/asset/installconfig/aws/subnet.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/aws/validation_test.go
|
@tthvo: This pull request references CORS-4352 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@tthvo: This pull request references CORS-4352 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/asset/installconfig/aws/validation_test.go (1)
418-458: Add one IPv6-primary containment failure case.Line 2609 adds an IPv6-primary branch by reversing
MachineNetwork, but every new BYO dual-stack subnet case here still useswithDualStackMachineNetworks(network.DualStackIPv4Primary)and only exercises the “missing IPv6 CIDR” path. That leaves the IPv6-primary ordering and the “subnet IPv6 CIDR is outsidenetworking.machineNetwork” validation untested. A single negative case withnetwork.DualStackIPv6Primaryand one out-of-range subnetIPv6CIDRwould close both gaps.Also applies to: 500-556, 2609-2620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/aws/validation_test.go` around lines 418 - 458, Add a new negative test case beside the existing dual-stack BYO subnet tests that switches the machine network ordering to IPv6-primary by calling icBuild.withDualStackMachineNetworks(network.DualStackIPv6Primary) (and keep withIPFamily(network.DualStackIPv4Primary) or appropriate IPFamily), and supply a BYO subnet in SubnetGroups (use mergeSubnets like the other cases) whose IPv6 CIDR is intentionally outside the configured networking.machineNetwork; set withVPCSubnetIDs to include that subnet ID and assert expectErr matches the containment-failure message for IPv6 CIDR being outside networking.machineNetwork (the same style as the existing expectErr regex for missing IPv6 CIDR) so the validation for DualStackIPv6Primary and out-of-range IPv6 CIDR is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/asset/installconfig/aws/validation_test.go`:
- Around line 418-458: Add a new negative test case beside the existing
dual-stack BYO subnet tests that switches the machine network ordering to
IPv6-primary by calling
icBuild.withDualStackMachineNetworks(network.DualStackIPv6Primary) (and keep
withIPFamily(network.DualStackIPv4Primary) or appropriate IPFamily), and supply
a BYO subnet in SubnetGroups (use mergeSubnets like the other cases) whose IPv6
CIDR is intentionally outside the configured networking.machineNetwork; set
withVPCSubnetIDs to include that subnet ID and assert expectErr matches the
containment-failure message for IPv6 CIDR being outside
networking.machineNetwork (the same style as the existing expectErr regex for
missing IPv6 CIDR) so the validation for DualStackIPv6Primary and out-of-range
IPv6 CIDR is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2469be9e-555e-4554-ba58-f30c615f5b6b
📒 Files selected for processing (1)
pkg/asset/installconfig/aws/validation_test.go
|
/retest-required |
|
@tthvo: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
This PR added validations to ensure the install-config fields are valid in order to install a dualstack cluster. The following validations are performed when
ipFamilyis dualstack:networking.machineNetworkmachineNetworkfieldFor local zones, the following zones support IPv6, according to AWS docs:
/label platform/aws
Summary by CodeRabbit
New Features
Bug Fixes