Skip to content

Bug: fix NetworkDeviceSpec CRD rejecting valid IPv4 addresses - #10755

Open
kchawlani19 wants to merge 1 commit into
openshift:mainfrom
kchawlani19:fix-10377-networkdevicespec-ipv4-ipv6
Open

Bug: fix NetworkDeviceSpec CRD rejecting valid IPv4 addresses#10755
kchawlani19 wants to merge 1 commit into
openshift:mainfrom
kchawlani19:fix-10377-networkdevicespec-ipv4-ipv6

Conversation

@kchawlani19

@kchawlani19 kchawlani19 commented Aug 11, 2026

Copy link
Copy Markdown

Summary

  • Fix vSphere NetworkDeviceSpec CRD schema incorrectly using format: ipv6 for fields that must accept both IPv4 and IPv6 (gateway, ipAddrs, nameservers).
  • Use the repository's existing Format=ip convention for gateway and nameservers; remove Format markers from ipAddrs because entries are CIDRs (e.g. 192.168.1.100/24), which Format=ip would reject.
  • Regenerate install.openshift.io_installconfigs.yaml and add regression coverage so dual Format=ipv4/Format=ipv6 markers cannot silently collapse back to format: ipv6.

Fixes #10377

Test plan

  • go generate ./pkg/types/installconfig.go (idempotent)
  • go test ./pkg/types/vsphere/validation/ -run 'TestCRDNetworkDeviceSpecFormats|TestValidatePlatform'
  • Confirm generated CRD shows format: ip for gateway/nameservers and no format on ipAddrs
  • Confirm valid IPv4 gateway/nameserver values are accepted by schema consumers (e.g. hive)

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation for vSphere network settings to support IPv4 and IPv6 gateway and nameserver addresses.
    • Corrected handling of IP address ranges, including IPv6 host addresses and CIDR notation.
    • Added validation coverage for invalid nameserver values.
  • Tests

    • Added coverage to verify network-related schema formats and static IP validation behavior.

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>
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The vSphere NetworkDeviceSpec annotations now use generic IP validation for gateways and nameservers. New tests verify generated CRD formats and static-IP validation for IPv4, IPv6, CIDR, and invalid nameserver values.

Changes

vSphere network validation

Layer / File(s) Summary
Address validation contract
pkg/types/vsphere/platform.go, pkg/types/vsphere/validation/crd_networkdevice_test.go
Gateway and nameserver annotations use format: ip. CRD tests verify these formats and confirm that ipAddrs has no IP format.
Static-IP validation coverage
pkg/types/vsphere/validation/platform_test.go
Tests cover IPv6 host and gateway values, IPv4 and IPv6 nameservers, and invalid nameserver input.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive Source changes and tests address issue #10377, but the generated CRD file is excluded from review and cannot be verified. Verify data/data/install.openshift.io_installconfigs.yaml to confirm the generated CRD uses the intended formats for gateway, ipAddrs, and nameservers.
✅ Passed checks (14 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The source changes and regression tests directly support the linked issue and stated CRD validation objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Stable And Deterministic Test Names ✅ Passed The changed tests use Go testing with static t.Run names; all added titles are fixed labels such as gateway, valid IPv6 CIDR, and valid nameservers, with no runtime values.
Test Structure And Quality ✅ Passed The added tests are local Go tests, not Ginkgo tests; each subtest covers one behavior, creates no resources, has no waits, and uses diagnostic assertion messages or expected error text.
Microshift Test Compatibility ✅ Passed The PR adds standard Go testing unit tests only; no Ginkgo It, Describe, Context, or When e2e tests were added, so MicroShift API compatibility checks do not apply.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The added tests are standard Go tests (TestCRDNetworkDeviceSpecFormats and table cases in TestValidatePlatform), not new Ginkgo e2e tests; SNO compatibility checks do not apply.
Topology-Aware Scheduling Compatibility ✅ Passed The PR changes only vSphere IP validation annotations, the generated CRD schema, and tests; it adds no deployment, controller, or topology scheduling constraints.
Ote Binary Stdout Contract ✅ Passed The PR adds only CRD metadata and regular tests; no added stdout writes, logging, or OTE process hooks were found, and the existing fmt.Printf is inside a regular test.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The PR adds standard Go tests using testing.T, not new Ginkgo e2e tests. They use no external connectivity; IPv4 literals are limited to validation fixtures.
No-Weak-Crypto ✅ Passed The PR changes only IP validation markers, generated CRD metadata, and tests; added-line and changed-file scans found no weak crypto, custom crypto, or secret comparison.
Container-Privileges ✅ Passed The PR changes only vSphere IP validation and tests; added lines contain no privileged:true, host namespaces, SYS_ADMIN, allowPrivilegeEscalation:true, or root settings.
No-Sensitive-Data-In-Logs ✅ Passed The diff adds schema annotations and validation tests only; it introduces no logging calls or sensitive values. The existing fmt.Printf and test-password fixture predate this change.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing the NetworkDeviceSpec CRD so it accepts valid IPv4 addresses.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci

openshift-ci Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign rochacbruno for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
openshift-ci Bot requested review from rvanderp3 and rwsu August 11, 2026 05:17

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3a117ed and 8a9f1f5.

⛔ Files ignored due to path filters (1)
  • data/data/install.openshift.io_installconfigs.yaml is excluded by !data/data/install.openshift.io_installconfigs.yaml
📒 Files selected for processing (3)
  • pkg/types/vsphere/platform.go
  • pkg/types/vsphere/validation/crd_networkdevice_test.go
  • pkg/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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.yaml

Repository: 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")
PY

Repository: 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-L75
  • pkg/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.

@openshift-ci

openshift-ci Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@kchawlani19: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@kchawlani19

Copy link
Copy Markdown
Author

/cc @jcpowermac @patrickdillon @rvanderp3 @vr4manta
Hi — this fixes #10377.
Root cause: dual Format=ipv4/Format=ipv6 markers collapse to format: ipv6 under controller-gen.
Fix: use Format=ip for gateway/nameservers; remove Format on ipAddrs (CIDR values).
Could a maintainer please add /ok-to-test so CI can run, and review when convenient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vsphere NetworkDeviceSpec: CRD schema incorrectly uses format: ipv6 for fields that should accept both IPv4 and IPv6

1 participant