🌱 improve ip address validation #1249

peppi-lotta · 2025-12-18T08:05:26Z

What this PR does / why we need it: Improve ip address validation. IP address string format is forced with kubebuilder when creating custom resources. No validation is done when IPClaims and IPPools are create with code. I suggest adding more rigorous validation to force ip addresses to be the correct format.

Fixes #1244

Checklist:

Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
E2E tests have been added, if necessary.
Integration tests have been added, if necessary.

metal3-io-bot · 2025-12-18T08:05:31Z

[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 rozzii 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:

OWNERS

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

peppi-lotta · 2025-12-18T08:08:49Z

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

Copilot

Pull request overview

This PR improves IP address validation throughout the codebase by adding format validation for IP addresses when IPClaims and IPPools are created programmatically (complementing the existing kubebuilder validation for custom resources). The changes also enable proper comparison of canonical and non-canonical IPv6 addresses.

Key changes:

Introduced IP address format validation using net.ParseIP in webhook validators
Added ipEqual function to properly compare canonical and non-canonical IPv6 addresses
Extended validation to cover IP addresses in annotations, pools, gateways, DNS servers, and pre-allocations

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
ipam/ippool_manager.go	Added `ipEqual` function and replaced string comparisons with semantic IP equality checks
ipam/ippool_manager_test.go	Added test cases for non-canonical IPv6 address handling
internal/webhooks/v1alpha1/ippool_webhook.go	Added `validateIP` and `validatePoolRanges` functions to validate IP format and pool ranges
internal/webhooks/v1alpha1/ippool_webhook_test.go	Added test cases for pool range validation (start <= end)
internal/webhooks/v1alpha1/ipclaim_webhook.go	Added validation for IP addresses in annotations
internal/webhooks/v1alpha1/ipaddress_webhook.go	Added IP address format validation for the address field and annotations
internal/webhooks/v1alpha1/ipaddress_webhook_test.go	Updated test data to use valid IP addresses and added test for invalid IP format

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-18T09:46:52Z

internal/webhooks/v1alpha1/ipaddress_webhook.go

+	if err := validateIP(c.Spec.Address); err != nil {
+		allErrs = append(allErrs,
+			field.Invalid(
+				field.NewPath("spec", "address"),
+				c.Spec.Address,
+				"invalid IP address",
+			),
+		)
 	}



Duplicate validation of IP address detected. Lines 89-97 already validate the IP address using validateIP, but lines 112-120 perform the same validation again. This results in redundant error messages being added to the error list when an invalid IP address is provided. One of these validation blocks should be removed.

Suggested change

if err := validateIP(c.Spec.Address); err != nil {

allErrs = append(allErrs,

field.Invalid(

field.NewPath("spec", "address"),

c.Spec.Address,

"invalid IP address",

),

)

}

Copilot · 2025-12-18T09:46:53Z

internal/webhooks/v1alpha1/ippool_webhook.go

+			if bytes.Compare(startIP, endIP) > 0 {
+				allErrs = append(allErrs,
+					field.Invalid(
+						field.NewPath("spec", "pools").Index(i),
+						fmt.Sprintf("start: %s, end: %s", *pool.Start, *pool.End),
+						"start address must be less than or equal to end address",
+					),
+				)


Potential nil pointer dereference when comparing IP addresses. If either the start or end IP address is invalid (fails validateIP check on lines 249 or 262), the net.ParseIP calls on lines 313-314 will return nil. The bytes.Compare function will then be called with nil slices, which will panic. The comparison should only be performed if both startIP and endIP are non-nil, or the function should skip comparison when validation errors were already detected for these IPs.

Suggested change

if bytes.Compare(startIP, endIP) > 0 {

allErrs = append(allErrs,

field.Invalid(

field.NewPath("spec", "pools").Index(i),

fmt.Sprintf("start: %s, end: %s", *pool.Start, *pool.End),

"start address must be less than or equal to end address",

),

)

// Only compare if both IPs parsed successfully.

if startIP != nil && endIP != nil {

if bytes.Compare(startIP, endIP) > 0 {

allErrs = append(allErrs,

field.Invalid(

field.NewPath("spec", "pools").Index(i),

fmt.Sprintf("start: %s, end: %s", *pool.Start, *pool.End),

"start address must be less than or equal to end address",

),

)

}

Copilot · 2025-12-18T09:46:53Z

internal/webhooks/v1alpha1/ipclaim_webhook.go

+	// Validate requested IP address if present in annotations
+	if requestedIP, ok := c.ObjectMeta.Annotations["ipAddress"]; ok && requestedIP != "" {
+		if err := validateIP(ipamv1.IPAddressStr(requestedIP)); err != nil {
+			allErrs = append(allErrs,
+				field.Invalid(
+					field.NewPath("metadata", "annotations", "ipAddress"),
+					requestedIP,
+					"is not a valid IP address",
+				),
+			)
+		}
+	}


Missing test coverage for the new IP address validation in annotations. The validation logic added for the "ipAddress" annotation (lines 72-82) should be tested to ensure invalid IP addresses in annotations are properly rejected. Consider adding test cases that verify both valid and invalid IP addresses in the annotation are handled correctly.

Copilot · 2025-12-18T09:46:54Z

internal/webhooks/v1alpha1/ipclaim_webhook.go

+	// Validate requested IP address if present in annotations
+	if requestedIP, ok := newIPClaim.ObjectMeta.Annotations["ipAddress"]; ok && requestedIP != "" {
+		if validateIP(ipamv1.IPAddressStr(requestedIP)) != nil {
+			allErrs = append(allErrs,
+				field.Invalid(
+					field.NewPath("metadata", "annotations", "ipAddress"),
+					requestedIP,
+					"is not a valid IP address",
+				),
+			)
+		}
+	}


Missing test coverage for the new IP address validation in annotations during updates. The validation logic added for the "ipAddress" annotation (lines 130-140) should be tested to ensure invalid IP addresses in annotations are properly rejected during update operations. Consider adding test cases that verify both valid and invalid IP addresses in the annotation are handled correctly.

Copilot · 2025-12-18T09:46:54Z

internal/webhooks/v1alpha1/ippool_webhook.go

+// validatePoolRanges validates that start <= end for each pool and validates all IP addresses.
+func (webhook *IPPool) validatePoolRanges(pool *ipamv1.IPPool) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	// Validate gateway IP if present
+	if pool.Spec.Gateway != nil {
+		if err := validateIP(*pool.Spec.Gateway); err != nil {
+			allErrs = append(allErrs,
+				field.Invalid(
+					field.NewPath("spec", "gateway"),
+					*pool.Spec.Gateway,
+					"is not a valid IP address",
+				),
+			)
+		}
+	}
+
+	// Validate DNS server IPs
+	for i, dnsServer := range pool.Spec.DNSServers {
+		if err := validateIP(dnsServer); err != nil {
+			allErrs = append(allErrs,
+				field.Invalid(
+					field.NewPath("spec", "dnsServers").Index(i),
+					dnsServer,
+					"is not a valid IP address",
+				),
+			)
+		}
+	}
+
+	// Validate preAllocations IPs
+	for name, ipAddr := range pool.Spec.PreAllocations {
+		if err := validateIP(ipAddr); err != nil {
+			allErrs = append(allErrs,
+				field.Invalid(
+					field.NewPath("spec", "preAllocations", name),
+					ipAddr,
+					"is not a valid IP address",
+				),
+			)
+		}
+	}
+
+	for i, pool := range pool.Spec.Pools {
+		// Validate Start IP
+		if pool.Start != nil {
+			if err := validateIP(*pool.Start); err != nil {
+				allErrs = append(allErrs,
+					field.Invalid(
+						field.NewPath("spec", "pools").Index(i).Child("start"),
+						*pool.Start,
+						"is not a valid IP address",
+					),
+				)
+			}
+		}
+
+		// Validate End IP
+		if pool.End != nil {
+			if err := validateIP(*pool.End); err != nil {
+				allErrs = append(allErrs,
+					field.Invalid(
+						field.NewPath("spec", "pools").Index(i).Child("end"),
+						*pool.End,
+						"is not a valid IP address",
+					),
+				)
+			}
+		}
+
+		// Validate Subnet CIDR
+		if pool.Subnet != nil {
+			if _, _, err := net.ParseCIDR(string(*pool.Subnet)); err != nil {
+				allErrs = append(allErrs,
+					field.Invalid(
+						field.NewPath("spec", "pools").Index(i).Child("subnet"),
+						*pool.Subnet,
+						"is not a valid CIDR",
+					),
+				)
+			}
+		}
+
+		// Validate pool-specific gateway IP
+		if pool.Gateway != nil {
+			if err := validateIP(*pool.Gateway); err != nil {
+				allErrs = append(allErrs,
+					field.Invalid(
+						field.NewPath("spec", "pools").Index(i).Child("gateway"),
+						*pool.Gateway,
+						"is not a valid IP address",
+					),
+				)
+			}
+		}
+
+		// Validate pool-specific DNS server IPs
+		for j, dnsServer := range pool.DNSServers {
+			if err := validateIP(dnsServer); err != nil {
+				allErrs = append(allErrs,
+					field.Invalid(
+						field.NewPath("spec", "pools").Index(i).Child("dnsServers").Index(j),
+						dnsServer,
+						"is not a valid IP address",
+					),
+				)
+			}
+		}
+		// Validate start <= end if both are present and valid
+		if pool.Start != nil && pool.End != nil {
+			startIP := net.ParseIP(string(*pool.Start))
+			endIP := net.ParseIP(string(*pool.End))
+
+			if bytes.Compare(startIP, endIP) > 0 {
+				allErrs = append(allErrs,
+					field.Invalid(
+						field.NewPath("spec", "pools").Index(i),
+						fmt.Sprintf("start: %s, end: %s", *pool.Start, *pool.End),
+						"start address must be less than or equal to end address",
+					),
+				)
+			}
+		}
+	}
+	return allErrs
+}


Missing test coverage for IP address format validation in IPPool. While tests verify that start > end is rejected, there are no test cases that verify invalid IP address formats (e.g., malformed IPs like "not-an-ip", "256.256.256.256") are rejected for the Start, End, Gateway, DNSServers, and PreAllocations fields. Consider adding test cases that verify the validateIP function correctly rejects malformed IP addresses.

Copilot · 2025-12-18T09:46:54Z

internal/webhooks/v1alpha1/ipaddress_webhook.go

+	// Validate requested IP address if present in annotations (for CAPI claims)
+	if requestedIP, ok := c.ObjectMeta.Annotations["ipAddress"]; ok && requestedIP != "" {
+		if validateIP(ipamv1.IPAddressStr(requestedIP)) != nil {
+			allErrs = append(allErrs,
+				field.Invalid(
+					field.NewPath("metadata", "annotations", "ipAddress"),
+					requestedIP,
+					"is not a valid IP address",
+				),
+			)
+		}
+	}


Missing test coverage for the new IP address validation in annotations for IPAddress resources. The validation logic added for the "ipAddress" annotation (lines 100-110) should be tested to ensure invalid IP addresses in annotations are properly rejected. Consider adding test cases that verify both valid and invalid IP addresses in the annotation are handled correctly.

tuminoid · 2025-12-18T09:47:36Z

/retitle 🐛 improve ip address validation

tuminoid

#1250 was merged, please rebase, and address Copilot comments.

Since #1250 had the bugfix, this is now improvement.
/retitle 🌱 improve ip address validation

Signed-off-by: peppi-lotta <[email protected]>

adilGhaffarDev · 2025-12-19T09:01:12Z

internal/webhooks/v1alpha1/ipaddress_webhook.go

 	}

+	// Validate requested IP address if present in annotations (for CAPI claims)
+	if requestedIP, ok := c.ObjectMeta.Annotations["ipAddress"]; ok && requestedIP != "" {


Is user allowed to add annotation after creation?
if yes lets add this validation in Update also?

metal3-io-bot requested review from kashifest and smoshiur1237 December 18, 2025 08:05

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 18, 2025

tuminoid requested a review from Copilot December 18, 2025 09:42

Copilot started reviewing on behalf of tuminoid December 18, 2025 09:42 View session

Copilot AI reviewed Dec 18, 2025

View reviewed changes

metal3-io-bot changed the title ~~🐛 Imporove ip address validation~~ 🐛 improve ip address validation Dec 18, 2025

tuminoid mentioned this pull request Dec 18, 2025

🐛 Fix IPv6 preAllocation canonicalization in IPAM #1250

Merged

4 tasks

tuminoid reviewed Dec 18, 2025

View reviewed changes

metal3-io-bot changed the title ~~🐛 improve ip address validation~~ 🌱 improve ip address validation Dec 18, 2025

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2025

peppi-lotta force-pushed the peppi-lotta/improve-validation branch from ecf9886 to 7580343 Compare December 19, 2025 08:06

metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 19, 2025

Imporove ip address validation

da6a292

Signed-off-by: peppi-lotta <[email protected]>

peppi-lotta force-pushed the peppi-lotta/improve-validation branch from 7580343 to da6a292 Compare December 19, 2025 08:13

adilGhaffarDev reviewed Dec 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

🌱 improve ip address validation #1249

🌱 improve ip address validation #1249

peppi-lotta commented Dec 18, 2025 •

edited

Loading

Uh oh!

metal3-io-bot commented Dec 18, 2025

Uh oh!

peppi-lotta commented Dec 18, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

tuminoid commented Dec 18, 2025

Uh oh!

tuminoid left a comment

Uh oh!

adilGhaffarDev Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

🌱 improve ip address validation #1249

Are you sure you want to change the base?

🌱 improve ip address validation #1249

Conversation

peppi-lotta commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metal3-io-bot commented Dec 18, 2025

Uh oh!

peppi-lotta commented Dec 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

tuminoid commented Dec 18, 2025

Uh oh!

tuminoid left a comment

Choose a reason for hiding this comment

Uh oh!

adilGhaffarDev Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

peppi-lotta commented Dec 18, 2025 •

edited

Loading