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 4 commits intoopenshift:mainfrom
Conversation
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.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/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 |
|
/test e2e-aws-ovn-dualstack-ipv6-primary-techpreview e2e-aws-ovn-dualstack-ipv4-primary-techpreview |
sadasu
left a comment
sadasu
left a comment
There was a problem hiding this comment.
Nit: Commit message for 1st commit says intro in place of nitro.
|
/approve Have a couple of minor comments |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sadasu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
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.
|
@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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/asset/installconfig/aws/validation_test.go (2)
2760-2771: Exercise the IPv6-primary ordering branch.Lines 423, 440, and 506 all call
withDualStackMachineNetworkswithnetwork.DualStackIPv4Primary, so theslices.Reversepath on Line 2767 never runs. Add one BYO-subnet case withnetwork.DualStackIPv6Primaryto prove subnet validation does not depend on the IPv4 machine network being first.Possible follow-up
+ { + name: "valid dual-stack byo subnets with IPv6-primary machine networks", + installConfig: icBuild.build( + icBuild.withBaseBYO(), + icBuild.withIPFamily(network.DualStackIPv6Primary), + icBuild.withDualStackMachineNetworks(network.DualStackIPv6Primary), + ), + availRegions: validAvailRegions(), + availZones: validAvailZones(), + instanceTypes: validInstanceTypes(), + subnets: SubnetGroups{ + Private: validDualstackSubnets("private"), + Public: validDualstackSubnets("public"), + VpcID: validVPCID, + }, + },🤖 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 2760 - 2771, The tests never exercise the IPv6-primary branch in withDualStackMachineNetworks; add a BYO-subnet test case that uses network.DualStackIPv6Primary so slices.Reverse(ic.Networking.MachineNetwork) runs. Locate the existing BYO-subnet test group that currently calls withDualStackMachineNetworks(network.DualStackIPv4Primary) and duplicate one case, changing the invocation to withDualStackMachineNetworks(network.DualStackIPv6Primary) and asserting the same validation outcome to prove subnet validation is independent of machine network ordering; ensure you reference the withDualStackMachineNetworks helper and the BYO-subnet test name when adding the case.
418-459: Add a failing case for IPv6 CIDR containment.This block proves
IPv6CIDRis present, but it never asserts the new rule that a provided subnet's IPv6 range must be insidenetworking.machineNetwork. Lines 749-769 already pin the IPv4 equivalent; without the IPv6 negative, a validator that only checks non-emptyIPv6CIDRwould still satisfy this table.Possible follow-up
+ { + name: "invalid dual-stack byo subnets, IPv6 CIDR outside machine network", + installConfig: icBuild.build( + icBuild.withBaseBYO(), + icBuild.withVPCSubnetIDs([]string{"invalid-private-ipv6-cidr-subnet"}, false), + icBuild.withIPFamily(network.DualStackIPv4Primary), + icBuild.withDualStackMachineNetworks(network.DualStackIPv4Primary), + ), + availRegions: validAvailRegions(), + availZones: append(validAvailZones(), "zone-for-invalid-ipv6-cidr-subnet"), + instanceTypes: validInstanceTypes(), + subnets: SubnetGroups{ + Private: mergeSubnets(validDualstackSubnets("private"), Subnets{ + "invalid-private-ipv6-cidr-subnet": { + ID: "invalid-private-ipv6-cidr-subnet", + Zone: &Zone{Name: "zone-for-invalid-ipv6-cidr-subnet"}, + CIDR: "10.0.7.0/24", + IPv6CIDR: "2600:1f13:ff0::/64", + }, + }), + Public: validDualstackSubnets("public"), + VpcID: validVPCID, + }, + expectErr: `platform\.aws\.vpc\.subnets\[\d+\]: Invalid value: "invalid-private-ipv6-cidr-subnet": subnet's CIDR range start 2600:1f13:ff0:: is outside of the specified machine networks`, + },🤖 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 - 459, Add a negative test case that mirrors the IPv4 containment failure but for IPv6: create a case alongside the existing "invalid dual-stack byo subnets, some subnets missing IPv6 CIDR" test that uses icBuild.withDualStackMachineNetworks(network.DualStackIPv4Primary) (so machineNetwork has an IPv6 range) and supply BYO subnets (via icBuild.withVPCSubnetIDs and SubnetGroups.Private/Public) that include an IPv6CIDR value that is outside the configured networking.machineNetwork; use validDualstackSubnets(...) as the base and merge in subnets that set IPv6CIDR to ranges not contained in the machineNetwork, then assert expectErr using a regex similar to the IPv4 containment error (matching platform.aws.vpc.subnets[...] Required value: IPv6 CIDR not contained in machineNetwork or the existing error message format) so the validator must check IPv6 containment for functions/structures around Subnet (SubnetGroups, IPv6CIDR) and the machine network validation path (withDualStackMachineNetworks / networking.machineNetwork).
🤖 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 2760-2771: The tests never exercise the IPv6-primary branch in
withDualStackMachineNetworks; add a BYO-subnet test case that uses
network.DualStackIPv6Primary so slices.Reverse(ic.Networking.MachineNetwork)
runs. Locate the existing BYO-subnet test group that currently calls
withDualStackMachineNetworks(network.DualStackIPv4Primary) and duplicate one
case, changing the invocation to
withDualStackMachineNetworks(network.DualStackIPv6Primary) and asserting the
same validation outcome to prove subnet validation is independent of machine
network ordering; ensure you reference the withDualStackMachineNetworks helper
and the BYO-subnet test name when adding the case.
- Around line 418-459: Add a negative test case that mirrors the IPv4
containment failure but for IPv6: create a case alongside the existing "invalid
dual-stack byo subnets, some subnets missing IPv6 CIDR" test that uses
icBuild.withDualStackMachineNetworks(network.DualStackIPv4Primary) (so
machineNetwork has an IPv6 range) and supply BYO subnets (via
icBuild.withVPCSubnetIDs and SubnetGroups.Private/Public) that include an
IPv6CIDR value that is outside the configured networking.machineNetwork; use
validDualstackSubnets(...) as the base and merge in subnets that set IPv6CIDR to
ranges not contained in the machineNetwork, then assert expectErr using a regex
similar to the IPv4 containment error (matching platform.aws.vpc.subnets[...]
Required value: IPv6 CIDR not contained in machineNetwork or the existing error
message format) so the validator must check IPv6 containment for
functions/structures around Subnet (SubnetGroups, IPv6CIDR) and the machine
network validation path (withDualStackMachineNetworks /
networking.machineNetwork).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ee23c8b-87af-4859-88ba-04ec5a619a8c
📒 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
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/asset/installconfig/aws/instancetypes.go
- pkg/asset/installconfig/aws/subnet.go
- pkg/asset/installconfig/aws/validation.go
4 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