Enhance ExternalIPPool validation (#5898)

* Validate if any IPRange don't overlap with another IPRange of current or existing pool
* Validate if IPRange.Start <= IPRange.End
* Validate if IPRange.Start and IPRange.End belong to same IP family

Signed-off-by: Daman Arora <aroradaman@gmail.com>

cmd/antrea-controller/controller.go

@@ -301,6 +301,7 @@ func run(o *Options) error {
 		networkPolicyController,
 		networkPolicyStatusController,
 		egressController,
+		externalIPPoolController,
 		statsAggregator,
 		bundleCollectionController,
 		traceflowController,
@@ -494,6 +495,7 @@ func createAPIServerConfig(kubeconfig string,
 	npController *networkpolicy.NetworkPolicyController,
 	networkPolicyStatusController *networkpolicy.StatusController,
 	egressController *egress.EgressController,
+	externalIPPoolController *externalippool.ExternalIPPoolController,
 	statsAggregator *stats.Aggregator,
 	bundleCollectionStore *supportbundlecollection.Controller,
 	traceflowController *traceflow.Controller,
@@ -563,6 +565,7 @@ func createAPIServerConfig(kubeconfig string,
 		endpointQuerier,
 		npController,
 		egressController,
+		externalIPPoolController,
 		bundleCollectionStore,
 		traceflowController), nil
 }

pkg/apiserver/apiserver.go

@@ -161,6 +161,7 @@ func NewConfig(
 	endpointQuerier controllernetworkpolicy.EndpointQuerier,
 	npController *controllernetworkpolicy.NetworkPolicyController,
 	egressController *egress.EgressController,
+	externalIPPoolController *externalippool.ExternalIPPoolController,
 	bundleCollectionController *controllerbundlecollection.Controller,
 	traceflowController *traceflow.Controller) *Config {
 	return &Config{
@@ -181,6 +182,7 @@ func NewConfig(
 			networkPolicyController:       npController,
 			networkPolicyStatusController: networkPolicyStatusController,
 			egressController:              egressController,
+			externalIPPoolController:      externalIPPoolController,
 			bundleCollectionController:    bundleCollectionController,
 			traceflowController:           traceflowController,
 		},

pkg/controller/externalippool/validate.go

@@ -17,15 +17,16 @@ package externalippool
 import (
 	"encoding/json"
 	"fmt"
-	"net"
+	"net/netip"
 
 	admv1 "k8s.io/api/admission/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 
 	crdv1beta1 "antrea.io/antrea/pkg/apis/crd/v1beta1"
-	"antrea.io/antrea/pkg/util/ip"
+	utilip "antrea.io/antrea/pkg/util/ip"
 )
 
 func (c *ExternalIPPoolController) ValidateExternalIPPool(review *admv1.AdmissionReview) *admv1.AdmissionResponse {
@@ -48,15 +49,21 @@ func (c *ExternalIPPoolController) ValidateExternalIPPool(review *admv1.Admissio
 		}
 	}
 
+	externalIPPools, err := c.externalIPPoolLister.List(labels.Everything())
+	if err != nil {
+		klog.ErrorS(err, "Error listing ExternalIPPools")
+		return newAdmissionResponseForErr(err)
+	}
+
 	switch review.Request.Operation {
 	case admv1.Create:
 		klog.V(2).Info("Validating CREATE request for ExternalIPPool")
-		if msg, allowed = validateIPRangesAndSubnetInfo(newObj.Spec.IPRanges, newObj.Spec.SubnetInfo); !allowed {
+		if msg, allowed = validateIPRangesAndSubnetInfo(newObj, externalIPPools); !allowed {
 			break
 		}
 	case admv1.Update:
 		klog.V(2).Info("Validating UPDATE request for ExternalIPPool")
-		if msg, allowed = validateIPRangesAndSubnetInfo(newObj.Spec.IPRanges, newObj.Spec.SubnetInfo); !allowed {
+		if msg, allowed = validateIPRangesAndSubnetInfo(newObj, externalIPPools); !allowed {
 			break
 		}
 		oldIPRangeSet := getIPRangeSet(oldObj.Spec.IPRanges)
@@ -83,47 +90,139 @@ func (c *ExternalIPPoolController) ValidateExternalIPPool(review *admv1.Admissio
 	}
 }
 
-func validateIPRangesAndSubnetInfo(ipRanges []crdv1beta1.IPRange, subnetInfo *crdv1beta1.SubnetInfo) (string, bool) {
-	if subnetInfo == nil {
-		return "", true
-	}
-	gatewayIP := net.ParseIP(subnetInfo.Gateway)
-	var mask net.IPMask
-	if gatewayIP.To4() != nil {
-		if subnetInfo.PrefixLength <= 0 || subnetInfo.PrefixLength >= 32 {
-			return fmt.Sprintf("invalid prefixLength %d", subnetInfo.PrefixLength), false
+func validateIPRangesAndSubnetInfo(externalIPPool crdv1beta1.ExternalIPPool, existingExternalIPPools []*crdv1beta1.ExternalIPPool) (string, bool) {
+	subnetInfo := externalIPPool.Spec.SubnetInfo
+	ipRanges := externalIPPool.Spec.IPRanges
+
+	var subnet *netip.Prefix
+	if subnetInfo != nil {
+		gatewayAddr, err := netip.ParseAddr(subnetInfo.Gateway)
+		if err != nil {
+			return fmt.Sprintf("invalid gateway address %s", subnetInfo.Gateway), false
 		}
-		mask = net.CIDRMask(int(subnetInfo.PrefixLength), 32)
-	} else {
-		if subnetInfo.PrefixLength <= 0 || subnetInfo.PrefixLength >= 128 {
-			return fmt.Sprintf("invalid prefixLength %d", subnetInfo.PrefixLength), false
+
+		if gatewayAddr.Is4() {
+			if subnetInfo.PrefixLength <= 0 || subnetInfo.PrefixLength >= 32 {
+				return fmt.Sprintf("invalid prefixLength %d", subnetInfo.PrefixLength), false
+			}
+		} else {
+			if subnetInfo.PrefixLength <= 0 || subnetInfo.PrefixLength >= 128 {
+				return fmt.Sprintf("invalid prefixLength %d", subnetInfo.PrefixLength), false
+			}
 		}
-		mask = net.CIDRMask(int(subnetInfo.PrefixLength), 128)
+		prefix := netip.PrefixFrom(gatewayAddr, int(subnetInfo.PrefixLength)).Masked()
+		subnet = &prefix
 	}
-	subnet := &net.IPNet{
-		IP:   gatewayIP.Mask(mask),
-		Mask: mask,
+
+	// combinedRanges combines both CIDR and start-end style range together mapped to start and end
+	// address of the range. We populate the map with ranges of existing pools and incorporate
+	// the ranges from the current pool as we iterate over them. The map's key is utilized to preserve
+	// the original user-specified input for formatting validation error, if it occurs.
+	combinedRanges := make(map[string][2]netip.Addr)
+	for _, pool := range existingExternalIPPools {
+		// exclude existing ip ranges of the pool which is being updated
+		if pool.Name == externalIPPool.Name {
+			continue
+		}
+		for _, ipRange := range pool.Spec.IPRanges {
+			var key string
+			var start, end netip.Addr
+
+			if ipRange.CIDR != "" {
+				key = fmt.Sprintf("range [%s] of pool %s", ipRange.CIDR, pool.Name)
+				cidr, _ := parseIPRangeCIDR(ipRange.CIDR)
+				start, end = utilip.GetStartAndEndOfPrefix(cidr)
+
+			} else {
+				key = fmt.Sprintf("range [%s-%s] of pool %s", ipRange.Start, ipRange.End, pool.Name)
+				start, end, _ = parseIPRangeStartEnd(ipRange.Start, ipRange.End)
+
+			}
+			combinedRanges[key] = [2]netip.Addr{start, end}
+		}
 	}
+
 	for _, ipRange := range ipRanges {
+		var key string
+		var start, end netip.Addr
+
 		if ipRange.CIDR != "" {
-			_, cidr, err := net.ParseCIDR(ipRange.CIDR)
-			if err != nil {
-				return err.Error(), false
-			}
-			if !ip.IPNetContains(subnet, cidr) {
-				return fmt.Sprintf("cidr %s must be a strict subset of the subnet", ipRange.CIDR), false
+			key = fmt.Sprintf("range [%s]", ipRange.CIDR)
+			cidr, errMsg := parseIPRangeCIDR(ipRange.CIDR)
+			if errMsg != "" {
+				return errMsg, false
 			}
+			start, end = utilip.GetStartAndEndOfPrefix(cidr)
+
 		} else {
-			start := net.ParseIP(ipRange.Start)
-			end := net.ParseIP(ipRange.End)
-			if !subnet.Contains(start) || !subnet.Contains(end) {
-				return fmt.Sprintf("IP range %s-%s must be a strict subset of the subnet", ipRange.Start, ipRange.End), false
+			key = fmt.Sprintf("range [%s-%s]", ipRange.Start, ipRange.End)
+
+			var errMsg string
+			start, end, errMsg = parseIPRangeStartEnd(ipRange.Start, ipRange.End)
+			if errMsg != "" {
+				return errMsg, false
 			}
+
+			// validate if start and end belong to same ip family
+			if start.Is4() != end.Is4() {
+				return fmt.Sprintf("range start %s and range end %s should belong to same family",
+					ipRange.Start, ipRange.End), false
+			}
+
+			// validate if start address <= end address
+			if start.Compare(end) == 1 {
+				return fmt.Sprintf("range start %s should not be greater than range end %s",
+					ipRange.Start, ipRange.End), false
+			}
+		}
+
+		// validate if range is subset of given subnet info
+		if subnet != nil && !(subnet.Contains(start) && subnet.Contains(end)) {
+			return fmt.Sprintf("%s must be a strict subset of the subnet %s/%d",
+				key, subnetInfo.Gateway, subnetInfo.PrefixLength), false
 		}
+
+		// validate if the range overlaps with ranges of any existing pool or already processed
+		// range of current pool.
+		for combinedKey, combinedRange := range combinedRanges {
+			if !(start.Compare(combinedRange[1]) == 1 || end.Compare(combinedRange[0]) == -1) {
+				return fmt.Sprintf("%s overlaps with %s", key, combinedKey), false
+			}
+		}
+
+		combinedRanges[key] = [2]netip.Addr{start, end}
 	}
 	return "", true
 }
 
+func parseIPRangeCIDR(cidrStr string) (netip.Prefix, string) {
+	var cidr netip.Prefix
+	var err error
+
+	cidr, err = netip.ParsePrefix(cidrStr)
+	if err != nil {
+		return cidr, fmt.Sprintf("invalid cidr %s", cidrStr)
+	}
+	cidr = cidr.Masked()
+	return cidr, ""
+}
+
+func parseIPRangeStartEnd(startStr, endStr string) (netip.Addr, netip.Addr, string) {
+	var start, end netip.Addr
+	var err error
+
+	start, err = netip.ParseAddr(startStr)
+	if err != nil {
+		return start, end, fmt.Sprintf("invalid start ip address %s", startStr)
+	}
+
+	end, err = netip.ParseAddr(endStr)
+	if err != nil {
+		return start, end, fmt.Sprintf("invalid end ip address %s", endStr)
+	}
+	return start, end, ""
+}
+
 func getIPRangeSet(ipRanges []crdv1beta1.IPRange) sets.Set[string] {
 	set := sets.New[string]()
 	for _, ipRange := range ipRanges {