OCPBUGS-98102: Add validations Azure Dualstack#10677
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughAzure platform validation now checks machine-network IPv6 CIDRs for single-stack and dual-stack constraints, including required IPv6 presence and permitted prefix lengths. Tests add per-case InstallConfig inputs and cover valid and invalid networking configurations. TLS tests strengthen private-key round-trip checks. ChangesAzure IPv6 validation
TLS test updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
|
/test |
|
/test e2e-aws-ovn-dualstack-ipv6-primary-techpreview |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/validation/network/dualstack.go (1)
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant CIDR re-parsing.
machineNetworkis already anipnet.IPNet(embedsnet.IPNet). Stringifying it and re-parsing vianet.ParseCIDRto get backip/ipNetis unnecessary work and an avoidable second failure path — the fields are already directly accessible.♻️ Proposed simplification
- for i, machineNetwork := range machineNetworks { - ip, ipNet, err := net.ParseCIDR(machineNetwork.String()) - if err != nil { - // Skip invalid CIDRs - they'll be caught by other validation - continue - } - - if ip.To4() != nil { + for i, machineNetwork := range machineNetworks { + if machineNetwork.IP.To4() != nil { hasIPv4 = true } else { - // IPv6 address - prefixLen, _ := ipNet.Mask.Size() - if prefixLen == 64 { + // IPv6 address + prefixLen, _ := machineNetwork.Mask.Size() + if prefixLen >= 64 { ipv6Slash64Indices = append(ipv6Slash64Indices, i) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/validation/network/dualstack.go` around lines 24 - 29, The dual-stack validation logic is unnecessarily re-parsing each machine network CIDR in the machineNetwork loop, which creates redundant work and a second failure path. Update the validation code in the dual-stack path to use the existing ipnet.IPNet fields directly from machineNetwork instead of calling String() and net.ParseCIDR, and remove the now-unneeded ip/ipNet reparse handling while preserving the invalid-CIDR skip behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/validation/network/dualstack.go`:
- Around line 13-54: The dual-stack CIDR validation wiring is duplicated across
the AWS and Azure platform validators, so move the shared “build []ipnet.IPNet
from ic.MachineNetwork and call ValidateDualStackIPv6CIDR under the networking
path” logic into a single helper in dualstack.go, such as
ValidateDualStackIPv6CIDRFromInstallConfig, and have both platform-specific
validation entrypoints call that helper instead of re-implementing it. Keep the
existing ValidateDualStackIPv6CIDR function as the core checker, and make the
new helper own the InstallConfig-to-networks conversion and fldPath construction
so future platforms can reuse it without copy-paste.
- Around line 34-39: The dual-stack IPv6 prefix check in the validation logic
only matches exact /64 networks, so longer IPv6 prefixes still slip through.
Update the prefix-length condition in the dual-stack validator that uses
ipNet.Mask.Size() so it rejects any IPv6 prefix at or above /64, and add a test
case covering a longer IPv6 prefix (for example in the dualstack validation
tests) to verify it is caught.
---
Nitpick comments:
In `@pkg/validation/network/dualstack.go`:
- Around line 24-29: The dual-stack validation logic is unnecessarily re-parsing
each machine network CIDR in the machineNetwork loop, which creates redundant
work and a second failure path. Update the validation code in the dual-stack
path to use the existing ipnet.IPNet fields directly from machineNetwork instead
of calling String() and net.ParseCIDR, and remove the now-unneeded ip/ipNet
reparse handling while preserving the invalid-CIDR skip behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5a5c29f4-e879-4dba-afb4-0f71c251ed55
📒 Files selected for processing (6)
pkg/types/aws/validation/platform.gopkg/types/aws/validation/platform_test.gopkg/types/azure/validation/platform.gopkg/types/azure/validation/platform_test.gopkg/types/validation/installconfig.gopkg/validation/network/dualstack.go
sadasu
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@sadasu: This pull request references Jira Issue OCPBUGS-98102, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
@sadasu: This pull request references Jira Issue OCPBUGS-98102, which is valid. 3 validation(s) were run on this bug
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 (2)
pkg/validation/network/dualstack.go (2)
1-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo dedicated unit test file for this new validation package.
This is new logic under
pkg/validation/network/with no accompanyingdualstack_test.goin the reviewed files/stack layers. Coverage currently comes only indirectly through AWS/Azure platform tests. As per coding guidelines,{cmd,data,pkg}/**/*.go: "All code under./cmd/...,./data/..., and./pkg/...must have unit tests." Direct tests here would also make it easier to cover edge cases (e.g. multiple invalid IPv6 entries, IPv4-only/IPv6-only single-stack, malformed CIDRs) without needing to route through platform-specific test suites.Do you want me to draft a
dualstack_test.gocovering these cases?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/validation/network/dualstack.go` around lines 1 - 71, This new validation logic in ValidateDualStackIPv6CIDR and ValidateDualStackIPv6CIDRFromInstallConfig needs direct unit test coverage. Add a dualstack_test.go under the same package to exercise dual-stack IPv4+IPv6 rejection for /64-or-longer IPv6 CIDRs, single-stack IPv4-only and IPv6-only acceptance, malformed CIDR handling, and multiple invalid IPv6 entries. Also verify the InstallConfig wrapper path in ValidateDualStackIPv6CIDRFromInstallConfig so the helper’s extraction behavior is covered without relying on platform tests.Source: Coding guidelines
25-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid redundant string round-trip through
net.ParseCIDR.
ipnet.IPNetembedsnet.IPNetdirectly, somachineNetwork.IPandmachineNetwork.Maskare already accessible without formatting to a string and re-parsing. The current code calls.String()thennet.ParseCIDR()for every entry, which is wasted work and adds a needless (currently harmless) parse-failure path that silently skips entries.♻️ Proposed refactor
- for i, machineNetwork := range machineNetworks { - ip, ipNet, err := net.ParseCIDR(machineNetwork.String()) - if err != nil { - // Skip invalid CIDRs - they'll be caught by other validation - continue - } - - if ip.To4() != nil { + for i, machineNetwork := range machineNetworks { + if machineNetwork.IP.To4() != nil { hasIPv4 = true } else { // IPv6 address - reject /64 or longer prefixes (cannot be split) - prefixLen, _ := ipNet.Mask.Size() + prefixLen, _ := machineNetwork.Mask.Size() if prefixLen >= 64 { ipv6TooLongIndices = append(ipv6TooLongIndices, i) } } }Note: the
netimport can then be dropped if unused elsewhere in the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/validation/network/dualstack.go` around lines 25 - 41, The dual-stack validation loop in `machineNetworks` is doing an unnecessary `String()`/`net.ParseCIDR()` round-trip for each `machineNetwork`, which adds extra work and a silent skip path. Update the logic in the loop to read directly from the existing `machineNetwork.IP` and `machineNetwork.Mask` fields, using `Mask.Size()` and `To4()` as needed, so the validation operates on the parsed CIDR object itself. If `net` is no longer referenced elsewhere in `dualstack.go`, remove that import as part of the refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/validation/network/dualstack.go`:
- Around line 1-71: This new validation logic in ValidateDualStackIPv6CIDR and
ValidateDualStackIPv6CIDRFromInstallConfig needs direct unit test coverage. Add
a dualstack_test.go under the same package to exercise dual-stack IPv4+IPv6
rejection for /64-or-longer IPv6 CIDRs, single-stack IPv4-only and IPv6-only
acceptance, malformed CIDR handling, and multiple invalid IPv6 entries. Also
verify the InstallConfig wrapper path in
ValidateDualStackIPv6CIDRFromInstallConfig so the helper’s extraction behavior
is covered without relying on platform tests.
- Around line 25-41: The dual-stack validation loop in `machineNetworks` is
doing an unnecessary `String()`/`net.ParseCIDR()` round-trip for each
`machineNetwork`, which adds extra work and a silent skip path. Update the logic
in the loop to read directly from the existing `machineNetwork.IP` and
`machineNetwork.Mask` fields, using `Mask.Size()` and `To4()` as needed, so the
validation operates on the parsed CIDR object itself. If `net` is no longer
referenced elsewhere in `dualstack.go`, remove that import as part of the
refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 31a9d595-8bf2-4813-be3a-b0df867b809f
📒 Files selected for processing (6)
pkg/types/aws/validation/platform.gopkg/types/aws/validation/platform_test.gopkg/types/azure/validation/platform.gopkg/types/azure/validation/platform_test.gopkg/types/validation/installconfig.gopkg/validation/network/dualstack.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/types/validation/installconfig.go
- pkg/types/aws/validation/platform.go
- pkg/types/aws/validation/platform_test.go
- pkg/types/azure/validation/platform_test.go
| // Report error only if dual-stack (both IPv4 and IPv6 present) and IPv6 uses /64 or longer | ||
| if hasIPv4 && len(ipv6TooLongIndices) > 0 { | ||
| for _, i := range ipv6TooLongIndices { | ||
| allErrs = append(allErrs, field.Invalid( |
There was a problem hiding this comment.
This validation will break our scenario: AWS BYO dual-stack subnets. Specifically, if the user defines subnet CIDRs as machine networks:
machineNetwork:
- cidr: 10.0.128.0/20 # subnet 0
- cidr: 10.0.144.0/20 # subnet 1
- cidr: 2001:db8:1234:1a00::/64 # subnet 0
- cidr: 2001:db8:1234:1a01::/64 # subnet 1There was a problem hiding this comment.
Yeah. I am conflicted about this and still thinking through this. Allowing this for AWS and disallowing for Azure seems weird but that is what we might end up with.
| expected: `^\Qnetworking.machineNetwork[1].cidr: Invalid value: "fd00::/65": IPv6 CIDR with /64 or longer prefix cannot be used in dual-stack configurations. Use a larger prefix (recommended /56) that can be split into multiple subnets\E$`, | ||
| }, | ||
| { | ||
| name: "invalid dual-stack with multiple IPv6 /64", |
There was a problem hiding this comment.
Following above comment, dual-stack with [multiple|one] IPv6 /64 is a valid use case in AWS, specifically, BYO dualstack subnets.
| }, | ||
| }, | ||
| { | ||
| name: "valid IPv6 /64 without dual-stack", |
There was a problem hiding this comment.
I think this should be an invalid case because IPv4 cluster should not define IPv6 machine networks (even though the installer allows it).
We can remove this case for now?
There was a problem hiding this comment.
I added a validation for preventing single stack ipv6 installs and added an appropriate test.
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
|
Test results show the new validation catching errors in the install-config early instead of CAPZ webhook rejecting it later:
|
openshift-ci
Bot
added
the
approved
sadasu
changed the title
|
@sadasu: This pull request references Jira Issue OCPBUGS-98102, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
8716027 to
9e8b6a5
Compare
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rna-afk 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 |
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
|
/test verify-deps |
|
/verified by CI and unit tests. |
openshift-ci-robot
added
the
verified
|
@sadasu: This PR has been marked as verified by 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 resume |
✅ Action performedReviews resumed. |
|
@sadasu: This pull request references Jira Issue OCPBUGS-98102, which is valid. 3 validation(s) were run on this bug
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/asset/tls/utils_test.go (1)
44-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify key material, not only its concrete type.
The round-trip test passes if parsing returns a different valid RSA/ECDSA key of the same type. Compare the decoded key’s material or a canonical serialized representation with the original key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/tls/utils_test.go` around lines 44 - 55, Update the round-trip assertions in the test case around PrivateKeyToPem and PemToPrivateKey to compare the decoded key’s actual material with the original key, using a canonical serialization or equivalent key-value comparison in addition to assert.IsType. Preserve the existing generation, PEM conversion, and error assertions.pkg/types/azure/validation/platform_test.go (1)
294-411: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor coverage gap for
DualStackIPv6Primary.The dual-stack "missing IPv6" case (lines 329-343) only exercises
network.DualStackIPv4Primary;DualStackIPv6Primary(tested separately at line 250 without anic) never goes through the new CIDR checks. Since both values are treated identically byisDualStack, risk is low, but an explicit case would close the gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/types/azure/validation/platform_test.go` around lines 294 - 411, Add an explicit “dual-stack enabled but missing IPv6” table case using network.DualStackIPv6Primary in the validation tests, with an IPv4-only machine network and the same expected required-IPv6 error as the DualStackIPv4Primary case. Ensure this exercises the CIDR validation path with an InstallConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/asset/tls/utils_test.go`:
- Around line 44-55: In the test loop around tc.genFunc, PrivateKeyToPem, and
PemToPrivateKey, replace setup-related assert.NoError and assert.NotEmpty calls
with require equivalents so failures stop the subtest before round-trip checks
continue. Apply the same change to similar cases below, while retaining assert
for the final decoded key type checks.
In `@pkg/types/azure/validation/platform.go`:
- Around line 200-203: Remove the len(machineNetworks) == 0 early-return
validation block from the platform validation helper, including its
field.Required error append. Allow empty machineNetwork values to fall through
to the existing installconfig.go validation, while preserving the Azure
dual-stack CIDR checks that follow.
---
Nitpick comments:
In `@pkg/asset/tls/utils_test.go`:
- Around line 44-55: Update the round-trip assertions in the test case around
PrivateKeyToPem and PemToPrivateKey to compare the decoded key’s actual material
with the original key, using a canonical serialization or equivalent key-value
comparison in addition to assert.IsType. Preserve the existing generation, PEM
conversion, and error assertions.
In `@pkg/types/azure/validation/platform_test.go`:
- Around line 294-411: Add an explicit “dual-stack enabled but missing IPv6”
table case using network.DualStackIPv6Primary in the validation tests, with an
IPv4-only machine network and the same expected required-IPv6 error as the
DualStackIPv4Primary case. Ensure this exercises the CIDR validation path with
an InstallConfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ec5acc76-04d0-4777-b83a-23bd659b62ad
📒 Files selected for processing (3)
pkg/asset/tls/utils_test.gopkg/types/azure/validation/platform.gopkg/types/azure/validation/platform_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/asset/tls/utils_test.go (1)
44-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify key material, not only its concrete type.
The round-trip test passes if parsing returns a different valid RSA/ECDSA key of the same type. Compare the decoded key’s material or a canonical serialized representation with the original key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/tls/utils_test.go` around lines 44 - 55, Update the round-trip assertions in the test case around PrivateKeyToPem and PemToPrivateKey to compare the decoded key’s actual material with the original key, using a canonical serialization or equivalent key-value comparison in addition to assert.IsType. Preserve the existing generation, PEM conversion, and error assertions.pkg/types/azure/validation/platform_test.go (1)
294-411: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor coverage gap for
DualStackIPv6Primary.The dual-stack "missing IPv6" case (lines 329-343) only exercises
network.DualStackIPv4Primary;DualStackIPv6Primary(tested separately at line 250 without anic) never goes through the new CIDR checks. Since both values are treated identically byisDualStack, risk is low, but an explicit case would close the gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/types/azure/validation/platform_test.go` around lines 294 - 411, Add an explicit “dual-stack enabled but missing IPv6” table case using network.DualStackIPv6Primary in the validation tests, with an IPv4-only machine network and the same expected required-IPv6 error as the DualStackIPv4Primary case. Ensure this exercises the CIDR validation path with an InstallConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/asset/tls/utils_test.go`:
- Around line 44-55: In the test loop around tc.genFunc, PrivateKeyToPem, and
PemToPrivateKey, replace setup-related assert.NoError and assert.NotEmpty calls
with require equivalents so failures stop the subtest before round-trip checks
continue. Apply the same change to similar cases below, while retaining assert
for the final decoded key type checks.
In `@pkg/types/azure/validation/platform.go`:
- Around line 200-203: Remove the len(machineNetworks) == 0 early-return
validation block from the platform validation helper, including its
field.Required error append. Allow empty machineNetwork values to fall through
to the existing installconfig.go validation, while preserving the Azure
dual-stack CIDR checks that follow.
---
Nitpick comments:
In `@pkg/asset/tls/utils_test.go`:
- Around line 44-55: Update the round-trip assertions in the test case around
PrivateKeyToPem and PemToPrivateKey to compare the decoded key’s actual material
with the original key, using a canonical serialization or equivalent key-value
comparison in addition to assert.IsType. Preserve the existing generation, PEM
conversion, and error assertions.
In `@pkg/types/azure/validation/platform_test.go`:
- Around line 294-411: Add an explicit “dual-stack enabled but missing IPv6”
table case using network.DualStackIPv6Primary in the validation tests, with an
IPv4-only machine network and the same expected required-IPv6 error as the
DualStackIPv4Primary case. Ensure this exercises the CIDR validation path with
an InstallConfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ec5acc76-04d0-4777-b83a-23bd659b62ad
📒 Files selected for processing (3)
pkg/asset/tls/utils_test.gopkg/types/azure/validation/platform.gopkg/types/azure/validation/platform_test.go
🛑 Comments failed to post (2)
pkg/asset/tls/utils_test.go (1)
44-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the test file structure and relevant lines. wc -l pkg/asset/tls/utils_test.go cat -n pkg/asset/tls/utils_test.go | sed -n '1,180p'Repository: openshift/installer
Length of output: 3639
Use
requirefor setup assertions here.assert.NoErrorandassert.NotEmptylet the test keep going after a failed key-generation or PEM-encoding step, which can cascade into misleading failures in the round-trip checks. Switch the setup checks in this block and the similar cases below torequire.*, and keepassertfor the final type checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/tls/utils_test.go` around lines 44 - 55, In the test loop around tc.genFunc, PrivateKeyToPem, and PemToPrivateKey, replace setup-related assert.NoError and assert.NotEmpty calls with require equivalents so failures stop the subtest before round-trip checks continue. Apply the same change to similar cases below, while retaining assert for the final decoded key type checks.Source: Path instructions
pkg/types/azure/validation/platform.go (1)
200-203: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant validation functions and surrounding context. grep -RIn --line-number --context=3 "cannot proceed with no machine networks specified\|at least one machine network is required\|validateDualStackIPv6CIDRFromInstallConfig\|ValidatePlatform" pkg/types/azure/validation pkg/types/validation | sed -n '1,220p'Repository: openshift/installer
Length of output: 10440
🏁 Script executed:
#!/bin/bash set -euo pipefail grep -RIn --line-number --context=3 \ "cannot proceed with no machine networks specified\|at least one machine network is required\|validateDualStackIPv6CIDRFromInstallConfig\|ValidatePlatform" \ pkg/types/azure/validation pkg/types/validation | sed -n '1,240p'Repository: openshift/installer
Length of output: 10440
Drop the duplicate empty machineNetwork check.
pkg/types/validation/installconfig.goalready addsnetworking.machineNetwork: Required value: at least one machine network is requiredfor an empty list, and this platform-level return adds a second near-identical error on the same field. Let the empty slice fall through and keep this helper focused on the Azure dual-stack CIDR checks below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/types/azure/validation/platform.go` around lines 200 - 203, Remove the len(machineNetworks) == 0 early-return validation block from the platform validation helper, including its field.Required error append. Allow empty machineNetwork values to fall through to the existing installconfig.go validation, while preserving the Azure dual-stack CIDR checks that follow.
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
… length Azure requires IPv6 subnets to have /64 address prefix. That implies that IPv6 VPCs need to be created with address prefixes larger than /64. Azure recommends /56 CIDR while creating dualstack VPCs. This fix adds a validation that ensures the network configuration in the install-config is validated for this condition. Fix also includes unit tests.
openshift-ci-robot
removed
the
verified
|
/test e2e-azure-ovn-dualstack-ipv6-primary-techpreview |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/types/azure/validation/platform.go (1)
232-232: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDiscarded
Mask.Size()validity flag.
net.IPMask.Size()returns(0, 0)when the mask isn't in canonical (ones-then-zeros) form; the second return value is discarded here. A non-canonical mask would silently be treated as/0(passes both the>=64and%4checks), bypassing validation. In practiceipnet.IPNetvalues are populated via CIDR parsing which always yields a canonical mask, so this is a defensive nit rather than a live bug.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/types/azure/validation/platform.go` at line 232, Preserve the validity result from machineNetwork.Mask.Size() in the surrounding platform validation logic, and reject or otherwise fail validation when the mask is non-canonical instead of treating the returned prefix length as /0. Keep the existing prefix-length checks for valid canonical masks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/asset/tls/utils_test.go`:
- Around line 91-96: Update the RSA and EC subtests around PrivateKeyToPem to
construct PEM data directly using x509.MarshalPKCS1PrivateKey with an “RSA
PRIVATE KEY” block and x509.MarshalECPrivateKey with an “EC PRIVATE KEY” block.
Retain PrivateKeyToPem only in the PKCS#8 test case, then continue decoding each
PEM value through PemToPrivateKey.
---
Nitpick comments:
In `@pkg/types/azure/validation/platform.go`:
- Line 232: Preserve the validity result from machineNetwork.Mask.Size() in the
surrounding platform validation logic, and reject or otherwise fail validation
when the mask is non-canonical instead of treating the returned prefix length as
/0. Keep the existing prefix-length checks for valid canonical masks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 95855a5f-dd25-4368-ae7b-f857863f12a5
📒 Files selected for processing (3)
pkg/asset/tls/utils_test.gopkg/types/azure/validation/platform.gopkg/types/azure/validation/platform_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/types/azure/validation/platform_test.go
| pemBytes, pemErr := PrivateKeyToPem(key) | ||
| assert.NoError(t, pemErr) | ||
| if !assert.NoError(t, pemErr) { | ||
| return | ||
| } | ||
|
|
||
| decoded, err := PemToPrivateKey(pemBytes) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test file and surrounding lines.
git ls-files 'pkg/asset/tls/utils_test.go'
wc -l pkg/asset/tls/utils_test.go
cat -n pkg/asset/tls/utils_test.go | sed -n '1,170p'Repository: openshift/installer
Length of output: 4474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation behind the test helpers.
ast-grep outline pkg/asset/tls/utils.go --view expanded
printf '\n---\n'
cat -n pkg/asset/tls/utils.go | sed -n '1,220p'Repository: openshift/installer
Length of output: 4478
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/asset/tls/utils.go --view expanded
printf '\n---\n'
cat -n pkg/asset/tls/utils.go | sed -n '1,220p'Repository: openshift/installer
Length of output: 4478
Use explicit RSA and EC PEM encodings in these subtests. The RSA and EC cases still go through PrivateKeyToPem, so they only exercise the PKCS#8 PRIVATE KEY path. Marshal RSA with x509.MarshalPKCS1PrivateKey and EC with x509.MarshalECPrivateKey, and encode them as RSA PRIVATE KEY / EC PRIVATE KEY; keep PrivateKeyToPem for the PKCS#8 case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/asset/tls/utils_test.go` around lines 91 - 96, Update the RSA and EC
subtests around PrivateKeyToPem to construct PEM data directly using
x509.MarshalPKCS1PrivateKey with an “RSA PRIVATE KEY” block and
x509.MarshalECPrivateKey with an “EC PRIVATE KEY” block. Retain PrivateKeyToPem
only in the PKCS#8 test case, then continue decoding each PEM value through
PemToPrivateKey.
|
@sadasu: 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. |
4 participants
Fix includes unit tests for both validations.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests