Bug OCPBUGS-97953: installer: fix NetworkDeviceSpec CRD incorrectly using format: ipv6#10673
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates vSphere ChangesvSphere NetworkDeviceSpec validation
Estimated code review effort: 2 (Simple) | ~10 minutes Related issues: Fixes vSphere Suggested labels: api-review, vsphere Suggested reviewers: 2uasimojo 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @ankimaha-sys. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
| // +kubebuilder:validation:Format=ipv6 | ||
| // +kubebuilder:example=`192.168.1.100/24` | ||
| // +kubebuilder:example=`2001:DB8:0000:0000:244:17FF:FEB6:D37D/64` | ||
| // +kubebuilder:validation:Required |
There was a problem hiding this comment.
Some level of validation is required, just removing and not replacing won't work here
|
this instead... this is from gemini, you need to test this |
ankimaha-sys
force-pushed
the
fix-vsphere-networkdevice-ipv6-format
branch
from
0cb3220 to
6dd43cd
Compare
|
Thanks for the feedback @jcpowermac! Updated to replace the broken Format markers with CEL XValidation rules instead of removing validation entirely:
Updated the generated CRD YAML as well. |
![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/types/vsphere/platform.go (1)
340-340: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider bounding array size for CEL cost estimation.
self.all(x, isCIDR(x))/self.all(x, isIP(x))iterate overipAddrs/nameservers, but neither field declaresmaxItems. Kubernetes' CRD Validation Rules guidance recommends settingmaxItems/maxLengthon all array/string fields used in CEL rules to keep the estimated cost bounded. GivenNetworkDeviceSpecis nested under a list of hosts, an unbounded inner list technically compounds the estimated cost, though with the small expected list sizes here (a handful of nameservers/IPs per host) this is unlikely to hit real limits in practice.Also applies to: 349-349
🤖 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/vsphere/platform.go` at line 340, The CEL validation on NetworkDeviceSpec’s ipAddrs and nameservers fields uses self.all(...) without any array size bound, so add maxItems to both array definitions and keep the existing XValidation rules intact. Update the relevant field declarations in platform.go near NetworkDeviceSpec so Kubernetes can estimate CEL cost safely while preserving the CIDR/IP checks.
🤖 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/types/vsphere/platform.go`:
- Line 340: The CEL validation on NetworkDeviceSpec’s ipAddrs and nameservers
fields uses self.all(...) without any array size bound, so add maxItems to both
array definitions and keep the existing XValidation rules intact. Update the
relevant field declarations in platform.go near NetworkDeviceSpec so Kubernetes
can estimate CEL cost safely while preserving the CIDR/IP checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9115fed7-8bcb-4730-a6c4-00db34ca051c
⛔ Files ignored due to path filters (1)
data/data/install.openshift.io_installconfigs.yamlis excluded by!data/data/install.openshift.io_installconfigs.yaml
📒 Files selected for processing (1)
pkg/types/vsphere/platform.go
|
/test ? |
|
/test e2e-vsphere-static-ovn |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
@ankimaha-sys please review ^ |
ankimaha-sys
force-pushed
the
fix-vsphere-networkdevice-ipv6-format
branch
from
6dd43cd to
3094cad
Compare
|
Added |
ankimaha-sys
force-pushed
the
fix-vsphere-networkdevice-ipv6-format
branch
from
3094cad to
a7a0b0a
Compare
|
/retest-required |
|
/retest |
| // +kubebuilder:validation:Format=ipv4 | ||
| // +kubebuilder:validation:Format=ipv6 | ||
| // +kubebuilder:validation:XValidation:rule="self.all(x, isIP(x))",message="each nameserver must be a valid IPv4 or IPv6 address" | ||
| // +kubebuilder:validation:MaxItems=10 |
There was a problem hiding this comment.
10 seems excessive for nameservers, and I thought there was a maximum anyway
|
@ankimaha-sys there are no unit tests, there probably should be some |
ankimaha-sys
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@ankimaha-sys: This pull request references Jira Issue OCPBUGS-97953, which is invalid:
Comment 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. |
ankimaha-sys
force-pushed
the
fix-vsphere-networkdevice-ipv6-format
branch
from
666d7a0 to
fcadbcd
Compare
|
/jira refresh |
|
@ankimaha-sys: This pull request references Jira Issue OCPBUGS-97953, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@ankimaha-sys: This pull request references Jira Issue OCPBUGS-97953, which is valid. The bug has been moved to the POST state. 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. |
|
Request: Add a unit test that asserts the generated CRD schema for the vSphere The new tests in Please add a test in Implementation details:
Why this matters: The original bug was Additional note on |
The NetworkDeviceSpec gateway, ipAddrs, and nameservers fields had both +kubebuilder:validation:Format=ipv4 and Format=ipv6 markers. Since controller-gen only keeps the last marker, the generated CRD schema used format: ipv6, causing valid IPv4 addresses to be rejected. Remove the format markers from these fields so both IPv4 and IPv6 addresses are accepted. Fixes: openshift#10377 Signed-off-by: Ankit Mahajan <ankimaha@redhat.com>
ankimaha-sys
force-pushed
the
fix-vsphere-networkdevice-ipv6-format
branch
from
fcadbcd to
e0d242e
Compare
|
Thanks @jcpowermac! Addressed both points:
This ensures any future marker regression in the CRD schema will be caught by tests. Force-pushed with the squashed commit. |
|
/retest |
|
@ankimaha-sys: 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. |
| assert.Equal(t, int64(3), *ns.MaxItems) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The current tests only verify the CRD schema structure (that the CEL rule string is "self.all(x, isCIDR(x))", that format is empty, etc.). They never feed actual IP addresses or CIDRs through validation, so they would pass even if the CEL rules silently accepted garbage — or rejected valid input, which is the exact bug this PR fixes.
Add behavioral validation tests that exercise the CEL rules with real values. This confirms the fix end-to-end: valid IPv4 and IPv6 addresses are accepted, and invalid strings are rejected.
What to add
Add a new test function (e.g. TestCRDNetworkDeviceCELValidation) in the same file that uses apiextensions-apiserver's CEL evaluation to test the rules extracted from the CRD schema. At minimum, cover these cases:
gateway (rule: self == '' || isIP(self)):
| Input | Expected |
|---|---|
"" |
pass |
"192.168.1.1" |
pass |
"2001:db8::1" |
pass |
"not-an-ip" |
fail |
"192.168.1.0/24" |
fail (CIDR, not bare IP) |
ipAddrs (rule: self.all(x, isCIDR(x))):
| Input | Expected |
|---|---|
["192.168.1.100/24"] |
pass |
["2001:db8::1/64"] |
pass |
["192.168.1.100/24", "2001:db8::1/64"] |
pass (mixed families) |
["192.168.1.1"] |
fail (bare IP, no prefix length) |
["not-a-cidr"] |
fail |
nameservers (rule: self.all(x, isIP(x))):
| Input | Expected |
|---|---|
["8.8.8.8"] |
pass |
["2001:4860:4860::8888"] |
pass |
["8.8.8.8", "2001:4860:4860::8888"] |
pass (mixed families) |
["not-an-ip"] |
fail |
["8.8.8.8/32"] |
fail (CIDR, not bare IP) |
Implementation guidance
Use k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel to compile and evaluate the XValidation rules against the field's JSONSchemaProps. The cel.NewValidator or cel.Compile functions accept the schema and return a validator you can call with test values. Wrap the cases in a table-driven test. The existing loadCRD and networkDeviceSchema helpers can be reused to extract the schema and rules.
If the CEL evaluation dependency is too heavy, an acceptable alternative is to shell out to kubectl apply --dry-run=server against a kind cluster, but the in-process CEL approach is preferred since it requires no external dependencies and matches the existing test style.
🤖 Prompt for AI Agents
In pkg/types/vsphere/validation/crd_test.go, add a new TestCRDNetworkDeviceCELValidation test function that evaluates the CEL XValidation rules from the CRD schema against actual IP addresses and CIDRs. Use the k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel package to compile and run the rules in-process. Reuse the existing loadCRD and networkDeviceSchema helpers to extract the schema. Structure as a table-driven test with subtests for each field (gateway, ipAddrs, nameservers). For each field, test both valid inputs (IPv4, IPv6, mixed dual-stack) and invalid inputs (malformed strings, wrong format like CIDR where bare IP is expected and vice versa). Assert that valid inputs produce zero validation errors and invalid inputs produce at least one. See the test case tables in this comment for the minimum required coverage.
|
/retest-failed |
|
Thanks for the detailed feedback @jcpowermac! I looked into adding behavioral CEL tests as you described. The CEL evaluation packages ( A few options:
What approach would you prefer? |
3 participants
Summary
The
NetworkDeviceSpecfields (gateway,ipAddrs,nameservers) inpkg/types/vsphere/platform.gohad both+kubebuilder:validation:Format=ipv4and+kubebuilder:validation:Format=ipv6markers. Sincecontroller-genonly keeps the last marker, the generated CRD schema ended up withformat: ipv6, causing valid IPv4 addresses (e.g.,192.168.1.1,8.8.8.8) to be rejected.This PR removes the
Formatmarkers from these three fields so both IPv4 and IPv6 addresses are accepted. The field descriptions already document the expected input formats.A
Format=ipmarker was considered but is not suitable becauseipAddrsaccepts CIDR notation (e.g.,192.168.1.100/24), which is not a valid IP address format.Fixes #10377
Discovered via: openshift/hive#2851 (comment)
Changes
pkg/types/vsphere/platform.go: Remove+kubebuilder:validation:Format=ipv4and+kubebuilder:validation:Format=ipv6fromgateway,ipAddrs, andnameserversfields inNetworkDeviceSpecdata/data/install.openshift.io_installconfigs.yaml: Remove generatedformat: ipv6from the corresponding CRD schema fieldsVerification
The generated CRD YAML was updated to match the marker changes. The
pkg/asset/store/data/copy is a hardlink todata/data/and is updated automatically.Summary by CodeRabbit
Gatewaycan be empty, or must be a valid IP address.ipAddrsnow requires every entry to be a valid CIDR.nameserversnow requires every entry to be a valid IP address.