Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions data/data/install.openshift.io_installconfigs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8666,28 +8666,38 @@ spec:
description: |-
gateway is an IPv4 or IPv6 address which represents the subnet gateway,
for example, 192.168.1.1.
format: ipv6
type: string
x-kubernetes-validations:
- message: gateway must be a valid IPv4 or IPv6 address
rule: self == '' || isIP(self)
ipAddrs:
description: |-
ipAddrs is a list of one or more IPv4 and/or IPv6 addresses and CIDR to assign to
this device, for example, 192.168.1.100/24. IP addresses provided via ipAddrs are
intended to allow explicit assignment of a machine's IP address.
example: 2001:DB8:0000:0000:244:17FF:FEB6:D37D/64
format: ipv6
items:
type: string
maxItems: 10
type: array
x-kubernetes-validations:
- message: each ipAddrs entry must be a valid CIDR (e.g.
192.168.1.100/24)
rule: self.all(x, isCIDR(x))
nameservers:
description: |-
nameservers is a list of IPv4 and/or IPv6 addresses used as DNS nameservers, for example,
8.8.8.8. a nameserver is not provided by a fulfilled IPAddressClaim. If DHCP is not the
source of IP addresses for this network device, nameservers should include a valid nameserver.
example: 8.8.8.8
format: ipv6
items:
type: string
maxItems: 3
type: array
x-kubernetes-validations:
- message: each nameserver must be a valid IPv4 or IPv6
address
rule: self.all(x, isIP(x))
required:
- ipAddrs
type: object
Expand Down
11 changes: 5 additions & 6 deletions pkg/types/vsphere/platform.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,15 +331,14 @@ type Host struct {
type NetworkDeviceSpec struct {
// gateway is an IPv4 or IPv6 address which represents the subnet gateway,
// for example, 192.168.1.1.
// +kubebuilder:validation:Format=ipv4
// +kubebuilder:validation:Format=ipv6
// +kubebuilder:validation:XValidation:rule="self == '' || isIP(self)",message="gateway must be a valid IPv4 or IPv6 address"
Gateway string `json:"gateway,omitempty"`

// ipAddrs is a list of one or more IPv4 and/or IPv6 addresses and CIDR to assign to
// this device, for example, 192.168.1.100/24. IP addresses provided via ipAddrs are
// intended to allow explicit assignment of a machine's IP address.
// +kubebuilder:validation:Format=ipv4
// +kubebuilder:validation:Format=ipv6
// +kubebuilder:validation:XValidation:rule="self.all(x, isCIDR(x))",message="each ipAddrs entry must be a valid CIDR (e.g. 192.168.1.100/24)"
// +kubebuilder:validation:MaxItems=10
// +kubebuilder:example=`192.168.1.100/24`
// +kubebuilder:example=`2001:DB8:0000:0000:244:17FF:FEB6:D37D/64`
// +kubebuilder:validation:Required

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some level of validation is required, just removing and not replacing won't work here

Expand All @@ -348,8 +347,8 @@ type NetworkDeviceSpec struct {
// nameservers is a list of IPv4 and/or IPv6 addresses used as DNS nameservers, for example,
// 8.8.8.8. a nameserver is not provided by a fulfilled IPAddressClaim. If DHCP is not the
// source of IP addresses for this network device, nameservers should include a valid nameserver.
// +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=3
// +kubebuilder:example=`8.8.8.8`
Nameservers []string `json:"nameservers,omitempty"`
}
Expand Down
94 changes: 94 additions & 0 deletions pkg/types/vsphere/validation/crd_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package validation

import (
"os"
"testing"

"github.com/stretchr/testify/assert"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"sigs.k8s.io/yaml"
)

func loadCRD(t *testing.T) *apiextensionsv1.CustomResourceDefinition {
t.Helper()
data, err := os.ReadFile("../../../../data/data/install.openshift.io_installconfigs.yaml")
if err != nil {
t.Fatalf("failed to load CRD: %v", err)
}
var crd apiextensionsv1.CustomResourceDefinition
if err := yaml.Unmarshal(data, &crd); err != nil {
t.Fatalf("failed to unmarshal CRD: %v", err)
}
return &crd
}

func networkDeviceSchema(t *testing.T, crd *apiextensionsv1.CustomResourceDefinition) map[string]apiextensionsv1.JSONSchemaProps {
t.Helper()

schema := crd.Spec.Versions[0].Schema.OpenAPIV3Schema
platform, ok := schema.Properties["platform"]
if !ok {
t.Fatal("missing platform property in CRD schema")
}
vsphere, ok := platform.Properties["vsphere"]
if !ok {
t.Fatal("missing vsphere property in CRD schema")
}
hosts, ok := vsphere.Properties["hosts"]
if !ok {
t.Fatal("missing hosts property in CRD schema")
}
if hosts.Items == nil || hosts.Items.Schema == nil {
t.Fatal("hosts items schema is nil")
}
netDev, ok := hosts.Items.Schema.Properties["networkDevice"]
if !ok {
t.Fatal("missing networkDevice property in CRD schema")
}
return netDev.Properties
}

func TestCRDNetworkDeviceSpec(t *testing.T) {
crd := loadCRD(t)
props := networkDeviceSchema(t, crd)

t.Run("gateway", func(t *testing.T) {
gw, ok := props["gateway"]
if !assert.True(t, ok, "gateway property must exist") {
return
}
assert.Empty(t, gw.Format, "gateway must not have a format constraint")
if assert.Len(t, gw.XValidations, 1, "gateway must have exactly 1 CEL validation rule") {
assert.Equal(t, "self == '' || isIP(self)", gw.XValidations[0].Rule)
assert.NotEmpty(t, gw.XValidations[0].Message)
}
})

t.Run("ipAddrs", func(t *testing.T) {
ip, ok := props["ipAddrs"]
if !assert.True(t, ok, "ipAddrs property must exist") {
return
}
assert.Empty(t, ip.Format, "ipAddrs must not have a format constraint")
if assert.Len(t, ip.XValidations, 1, "ipAddrs must have exactly 1 CEL validation rule") {
assert.Equal(t, "self.all(x, isCIDR(x))", ip.XValidations[0].Rule)
}
if assert.NotNil(t, ip.MaxItems, "ipAddrs must have maxItems set") {
assert.Equal(t, int64(10), *ip.MaxItems)
}
})

t.Run("nameservers", func(t *testing.T) {
ns, ok := props["nameservers"]
if !assert.True(t, ok, "nameservers property must exist") {
return
}
assert.Empty(t, ns.Format, "nameservers must not have a format constraint")
if assert.Len(t, ns.XValidations, 1, "nameservers must have exactly 1 CEL validation rule") {
assert.Equal(t, "self.all(x, isIP(x))", ns.XValidations[0].Rule)
}
if assert.NotNil(t, ns.MaxItems, "nameservers must have maxItems set") {
assert.Equal(t, int64(3), *ns.MaxItems)
}
})
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.