Add IP family validation for IPPool and ExternalIPPool

[SharanRP]

What this PR does

Fixes #7773
This PR adds IP family validation for IPPool and ExternalIPPool resources. It prevents users from creating pools with IP families that are not enabled in the cluster.

For example, creating an IPv4 pool in an IPv6-only cluster will now be rejected with a clear error message.

How it works

• Added a new ValidateIPRangeIPFamily function that checks if each IP range matches an enabled family.
• The cluster's IP family config is passed to ExternalIPPoolController and AntreaIPAMController during initialization.
• On the controller side, IP family is determined from NodeIPAM.ClusterCIDRs. If NodeIPAM is not configured, both families are allowed.
• On the agent side, the existing IPv4Enabled/IPv6Enabled from networkConfig is used.
• Validation runs on both CREATE and UPDATE operations.

Testing

• 12 unit tests for the core validation function (CIDR, start-end, dual-stack, IPv4-only, IPv6-only, mixed).
• 12 webhook-level integration tests (6 for IPPool, 6 for ExternalIPPool) testing the full admission path.
• All existing tests pass with no regressions.

[Copilot]

Pull request overview

This PR adds IP family validation to IPPool and ExternalIPPool resources to prevent users from creating pools with IP families that are not enabled in the cluster. The implementation follows a defensive approach: IP family configuration is determined from NodeIPAM ClusterCIDRs on the controller side (defaulting to dual-stack if NodeIPAM is not configured), and from networkConfig on the agent side. The validation is performed during both CREATE and UPDATE operations on the admission webhooks.

Changes:

• Added ValidateIPRangeIPFamily function to check IP ranges against cluster IP family configuration
• Updated ExternalIPPoolController and AntreaIPAMController to accept and store IP family flags
• Integrated IP family validation into admission webhooks for both IPPool and ExternalIPPool

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/controller/validation/ippool.go	Added core validation logic with ValidateIPRangeIPFamily and helper functions
pkg/controller/validation/ippool_test.go	Added 12 unit tests covering various IP family validation scenarios
pkg/controller/ipam/antrea_ipam_controller.go	Added ipv4Enabled/ipv6Enabled fields and updated constructor signature
pkg/controller/ipam/validate.go	Integrated IP family validation into CREATE and UPDATE operations for IPPool
pkg/controller/ipam/validate_test.go	Added 6 webhook-level integration tests for IPPool IP family validation
pkg/controller/ipam/antrea_ipam_controller_test.go	Updated test helper to pass dual-stack flags
pkg/controller/externalippool/controller.go	Added ipv4Enabled/ipv6Enabled fields and updated constructor signature
pkg/controller/externalippool/validate.go	Integrated IP family validation into CREATE and UPDATE operations for ExternalIPPool
pkg/controller/externalippool/validate_test.go	Added 6 webhook-level integration tests for ExternalIPPool IP family validation
pkg/controller/externalippool/controller_test.go	Updated test helper to pass dual-stack flags
pkg/controller/egress/controller_test.go	Updated test helper to pass dual-stack flags
pkg/controller/serviceexternalip/controller_test.go	Updated test helper to pass dual-stack flags
cmd/antrea-controller/controller.go	Added IP family detection logic from NodeIPAM ClusterCIDRs and passed to controllers
cmd/antrea-agent/agent.go	Passed network config IP family flags to ExternalIPPoolController

---

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

[Copilot]

The tests don't cover the edge case where both ipv4Enabled and ipv6Enabled are false. While this scenario should ideally not occur in practice, it could happen if all ClusterCIDRs fail to parse in the controller (see related issue in controller.go). Consider adding a test case to document the expected behavior in this scenario.

[Copilot]

The IP family detection logic silently ignores parsing errors for ClusterCIDRs using continue. If all ClusterCIDRs fail to parse (e.g., due to malformed configuration), both ipv4Enabled and ipv6Enabled would remain false, which would cause all IPPool and ExternalIPPool creations to fail with confusing error messages. Consider logging a warning when a ClusterCIDR fails to parse, or validating that at least one IP family is enabled after parsing.

Suggested change

	ipv4Enabled, ipv6Enabled = false, false
	for _, cidrStr := range o.config.NodeIPAM.ClusterCIDRs {
	clusterCIDR, _, err := net.ParseCIDR(cidrStr)
	if err != nil {
	continue
	}
	if clusterCIDR.To4() != nil {
	ipv4Enabled = true
	} else {
	ipv6Enabled = true
	}
	}
	ipv4Enabled, ipv6Enabled = false, false
	hasValidClusterCIDR := false
	for _, cidrStr := range o.config.NodeIPAM.ClusterCIDRs {
	clusterCIDR, _, err := net.ParseCIDR(cidrStr)
	if err != nil {
	klog.Warningf("Failed to parse NodeIPAM ClusterCIDR %q: %v", cidrStr, err)
	continue
	}
	hasValidClusterCIDR = true
	if clusterCIDR.To4() != nil {
	ipv4Enabled = true
	} else {
	ipv6Enabled = true
	}
	}
	if !hasValidClusterCIDR {
	klog.Warning("No valid NodeIPAM ClusterCIDRs found; assuming both IPv4 and IPv6 are enabled")
	ipv4Enabled, ipv6Enabled = true, true
	}

[Copilot]

The ipRangeToString function duplicates logic from GetIPRangeSet (lines 27-38). Consider refactoring to use ipRangeToString within GetIPRangeSet to eliminate code duplication and ensure consistent string representation of IP ranges.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

---

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

[Copilot]

The IP family determination logic has a subtle edge case: if all ClusterCIDRs in NodeIPAM configuration fail to parse, the code correctly falls back to enabling both families (lines 252-255). However, the condition on line 236 checks "len(o.config.NodeIPAM.ClusterCIDRs) > 0" which means if ClusterCIDRs is an empty slice when NodeIPAM is enabled, the code will not enter the block and will leave both families enabled by default. While this is correct behavior, it might be worth adding a log message in this case to indicate that NodeIPAM is enabled but has no ClusterCIDRs configured, so IP family validation is permissive.

Suggested change

	if features.DefaultFeatureGate.Enabled(features.NodeIPAM) && o.config.NodeIPAM.EnableNodeIPAM && len(o.config.NodeIPAM.ClusterCIDRs) > 0 {
	ipv4Enabled, ipv6Enabled = false, false
	hasValidClusterCIDR := false
	for _, cidrStr := range o.config.NodeIPAM.ClusterCIDRs {
	clusterCIDR, _, err := net.ParseCIDR(cidrStr)
	if err != nil {
	klog.Warningf("Failed to parse NodeIPAM ClusterCIDR %q: %v", cidrStr, err)
	continue
	}
	hasValidClusterCIDR = true
	if clusterCIDR.To4() != nil {
	ipv4Enabled = true
	} else {
	ipv6Enabled = true
	}
	}
	if !hasValidClusterCIDR {
	klog.Warning("No valid NodeIPAM ClusterCIDRs found; assuming both IPv4 and IPv6 are enabled")
	ipv4Enabled, ipv6Enabled = true, true
	}
	if features.DefaultFeatureGate.Enabled(features.NodeIPAM) && o.config.NodeIPAM.EnableNodeIPAM {
	if len(o.config.NodeIPAM.ClusterCIDRs) > 0 {
	ipv4Enabled, ipv6Enabled = false, false
	hasValidClusterCIDR := false
	for _, cidrStr := range o.config.NodeIPAM.ClusterCIDRs {
	clusterCIDR, _, err := net.ParseCIDR(cidrStr)
	if err != nil {
	klog.Warningf("Failed to parse NodeIPAM ClusterCIDR %q: %v", cidrStr, err)
	continue
	}
	hasValidClusterCIDR = true
	if clusterCIDR.To4() != nil {
	ipv4Enabled = true
	} else {
	ipv6Enabled = true
	}
	}
	if !hasValidClusterCIDR {
	klog.Warning("No valid NodeIPAM ClusterCIDRs found; assuming both IPv4 and IPv6 are enabled")
	ipv4Enabled, ipv6Enabled = true, true
	}
	} else {
	klog.Info("NodeIPAM is enabled but no ClusterCIDRs are configured; assuming both IPv4 and IPv6 are enabled")
	}

[Copilot]

The validation logic doesn't handle the edge case where both ipv4Enabled and ipv6Enabled are false. In such a scenario, an IPv4 range would incorrectly report "not allowed in an IPv6-only cluster" and an IPv6 range would report "not allowed in an IPv4-only cluster", when neither IP family is actually enabled.

While this edge case is prevented by the controller initialization logic (which defaults to both enabled when NodeIPAM is not configured), it would be more robust to explicitly check for this case at the start of the function and return a clear error like "no IP families are enabled in the cluster".

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

---

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

[Copilot]

isIPRangeIPv4 re-parses CIDRs directly instead of reusing the existing parseIPRangeCIDR helper. This duplicates parsing / error formatting logic (and currently loses the .Masked() canonicalization used elsewhere). Consider calling parseIPRangeCIDR here and deriving the family from the returned prefix to keep parsing behavior and errors consistent in one place.

Suggested change

	cidr, err := netip.ParsePrefix(ipRange.CIDR)
	if err != nil {
	return false, fmt.Errorf("invalid cidr %s", ipRange.CIDR)
	cidr, err := parseIPRangeCIDR(ipRange.CIDR)
	if err != nil {
	return false, err

[Copilot]

For start/end ranges, isIPRangeIPv4 only parses Start and ignores End. If End is invalid (or of a different family) this function will not surface it, and the user will instead get a later error from other validation. Consider reusing parseIPRangeStartEnd (and/or validateIPRange) so both addresses are parsed and errors are reported consistently at the point this validation runs.

Suggested change

	}
	}
	if ipRange.End != "" {
	end, err := netip.ParseAddr(ipRange.End)
	if err != nil {
	return false, fmt.Errorf("invalid end ip address %s", ipRange.End)
	}
	if start.Is4() != end.Is4() {
	return false, fmt.Errorf("start and end ip addresses %s and %s must be in the same IP family", ipRange.Start, ipRange.End)
	}
	}

[Copilot]

This block re-parses NodeIPAM.ClusterCIDRs and has fallback/warning paths for invalid / empty CIDRs, but Options.validateNodeIPAMControllerOptions already rejects invalid or empty ClusterCIDRs when EnableNodeIPAM is true (cmd/antrea-controller/options.go:112-120). Consider reusing netutils.ParseCIDRs (or the already-validated values) here and simplifying away the dead fallback branches to avoid duplicate parsing logic and inconsistent behavior.

Suggested change

	if len(o.config.NodeIPAM.ClusterCIDRs) > 0 {
	ipv4Enabled, ipv6Enabled = false, false
	hasValidClusterCIDR := false
	for _, cidrStr := range o.config.NodeIPAM.ClusterCIDRs {
	clusterCIDR, _, err := net.ParseCIDR(cidrStr)
	if err != nil {
	klog.Warningf("Failed to parse NodeIPAM ClusterCIDR %q: %v", cidrStr, err)
	continue
	}
	hasValidClusterCIDR = true
	if clusterCIDR.To4() != nil {
	ipv4Enabled = true
	} else {
	ipv6Enabled = true
	}
	}
	if !hasValidClusterCIDR {
	klog.Warning("No valid NodeIPAM ClusterCIDRs found; assuming both IPv4 and IPv6 are enabled")
	ipv4Enabled, ipv6Enabled = true, true
	}
	} else {
	klog.Info("NodeIPAM is enabled but no ClusterCIDRs are configured; assuming both IPv4 and IPv6 are enabled")
	cidrs, err := netutils.ParseCIDRs(o.config.NodeIPAM.ClusterCIDRs)
	if err != nil {
	// This should not happen because NodeIPAM options validation enforces valid, non-empty ClusterCIDRs.
	klog.Errorf("Failed to parse NodeIPAM ClusterCIDRs %v; assuming both IPv4 and IPv6 are enabled: %v", o.config.NodeIPAM.ClusterCIDRs, err)
	} else {
	ipv4Enabled, ipv6Enabled = false, false
	for _, cidr := range cidrs {
	if cidr.IP.To4() != nil {
	ipv4Enabled = true
	} else {
	ipv6Enabled = true
	}
	}

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

---

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