Bug: fix NetworkDeviceSpec CRD rejecting valid IPv4 addresses - #10755
Bug: fix NetworkDeviceSpec CRD rejecting valid IPv4 addresses#10755kchawlani19 wants to merge 1 commit into
Conversation
controller-gen keeps only the last Format marker, so dual Format=ipv4/Format=ipv6 collapsed to format: ipv6 and rejected IPv4 values. Use Format=ip for gateway and nameservers, and drop Format on ipAddrs (CIDR values). Fixes openshift#10377. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThe vSphere ChangesvSphere network validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/types/vsphere/platform.go`:
- Line 348: Change the Nameservers marker in pkg/types/vsphere/platform.go:348
to apply IP format to array items rather than the array itself. Update the
related assertions in
pkg/types/vsphere/validation/crd_networkdevice_test.go:69-75 and :77-83 so
ns.Items.Schema.Format is "ip", ns.Format is empty, and ipAddrs item format
remains empty for CIDR values.
🪄 Autofix
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: Pro Plus
Run ID: 3b340ebe-1885-46c1-9773-3caba9912736
⛔ 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 (3)
pkg/types/vsphere/platform.gopkg/types/vsphere/validation/crd_networkdevice_test.gopkg/types/vsphere/validation/platform_test.go
| // source of IP addresses for this network device, nameservers should include a valid nameserver. | ||
| // +kubebuilder:validation:Format=ipv4 | ||
| // +kubebuilder:validation:Format=ipv6 | ||
| // +kubebuilder:validation:Format=ip |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'validation:(items:)?Format=ip|Nameservers' \
pkg/types/vsphere/platform.go
rg -n -C 6 'ipAddrs|nameservers|Items\.Schema|\.Format' \
pkg/types/vsphere/validation/crd_networkdevice_test.go
rg -n -C 8 'gateway:|ipAddrs:|nameservers:' \
data/data/install.openshift.io_installconfigs.yamlRepository: openshift/installer
Length of output: 5380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '320,355p' pkg/types/vsphere/platform.go
sed -n '54,90p' pkg/types/vsphere/validation/crd_networkdevice_test.go
rg -n -C 8 'nameservers:|ipAddrs:' data/data/install.openshift.io_installconfigs.yaml | head -80
rg -n 'controller-tools|controller-gen|kubebuilder' go.mod go.sum hack Makefile* 2>/dev/null | head -80
python3 - <<'PY'
from pathlib import Path
import re
path = Path("data/data/install.openshift.io_installconfigs.yaml")
text = path.read_text()
for name in ("ipAddrs:", "nameservers:"):
start = text.find(" " + name)
print(f"--- {name} ---")
print(text[start:start + 650] if start >= 0 else "not found")
PYRepository: openshift/installer
Length of output: 5104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import yaml
path = Path("data/data/install.openshift.io_installconfigs.yaml")
doc = None
for raw in yaml.safe_load_all(path.read_text()):
if isinstance(raw, dict) and raw.get("kind") == "CustomResourceDefinition":
doc = raw
break
if doc is None:
raise SystemExit("CRD document not found")
schemas = []
for version in doc["spec"]["versions"]:
schema = version.get("schema", {}).get("openAPIV3Schema", {})
schemas.append(schema)
def find(node, path=()):
if isinstance(node, dict):
if "nameservers" in node.get("properties", {}):
yield path + ("nameservers",), node["properties"]["nameservers"]
for key, value in node.items():
yield from find(value, path + (key,))
elif isinstance(node, list):
for i, value in enumerate(node):
yield from find(value, path + (str(i),))
for schema in schemas:
for path, value in find(schema):
print("path:", ".".join(path))
print("nameservers type:", value.get("type"))
print("nameservers format:", value.get("format"))
print("item type:", value.get("items", {}).get("type"))
print("item format:", value.get("items", {}).get("format"))
PY
sed -n '8660,8678p' data/data/install.openshift.io_installconfigs.yaml
curl -fsSL \
https://raw.githubusercontent.com/kubernetes-sigs/controller-tools/v0.19.0/pkg/crd/markers/validation.go \
| rg -n -C 8 'items|Format'Repository: openshift/installer
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from pathlib import Path
lines = Path("data/data/install.openshift.io_installconfigs.yaml").read_text().splitlines()
for name in ("ipAddrs:", "nameservers:"):
for i, line in enumerate(lines):
if line.strip() == name and i > 0 and "networkDevice" in "\n".join(lines[max(0, i-20):i]):
print(f"--- {name} at line {i+1} ---")
for n, value in enumerate(lines[i:i+12], i+1):
print(f"{n}: {value}")
break
PY
curl -fsSL \
https://raw.githubusercontent.com/kubernetes-sigs/controller-tools/v0.19.0/pkg/crd/markers/validation.go \
| rg -n -C 8 'items|Format'Repository: openshift/installer
Length of output: 9718
Apply the IP format to array items.
format: ip currently applies to the Nameservers array, not its string items. Use +kubebuilder:validation:items:Format=ip. Update the test to check ns.Items.Schema.Format == "ip" and an empty ns.Format. Keep ipAddrs item format empty because its values are CIDRs.
📍 Affects 2 files
pkg/types/vsphere/platform.go#L348-L348(this comment)pkg/types/vsphere/validation/crd_networkdevice_test.go#L69-L75pkg/types/vsphere/validation/crd_networkdevice_test.go#L77-L83
🤖 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 348, Change the Nameservers marker in
pkg/types/vsphere/platform.go:348 to apply IP format to array items rather than
the array itself. Update the related assertions in
pkg/types/vsphere/validation/crd_networkdevice_test.go:69-75 and :77-83 so
ns.Items.Schema.Format is "ip", ns.Format is empty, and ipAddrs item format
remains empty for CIDR values.
|
@kchawlani19: all tests passed! 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. |
|
/cc @jcpowermac @patrickdillon @rvanderp3 @vr4manta |
Summary
NetworkDeviceSpecCRD schema incorrectly usingformat: ipv6for fields that must accept both IPv4 and IPv6 (gateway,ipAddrs,nameservers).Format=ipconvention forgatewayandnameservers; remove Format markers fromipAddrsbecause entries are CIDRs (e.g.192.168.1.100/24), whichFormat=ipwould reject.install.openshift.io_installconfigs.yamland add regression coverage so dualFormat=ipv4/Format=ipv6markers cannot silently collapse back toformat: ipv6.Fixes #10377
Test plan
go generate ./pkg/types/installconfig.go(idempotent)go test ./pkg/types/vsphere/validation/ -run 'TestCRDNetworkDeviceSpecFormats|TestValidatePlatform'format: ipforgateway/nameserversand no format onipAddrsSummary by CodeRabbit
Bug Fixes
Tests