Added validation for IP string and removing leading zeros from the st…

…ring, if any

Signed-off-by: arkadeepsen <arsen@redhat.com>

go-controller/pkg/node/gateway_iptables.go

@@ -453,13 +453,8 @@ func getGatewayIPTRules(service *kapi.Service, svcHasLocalHostNetEndPnt bool) []
 				rules = append(rules, getNodePortIPTRules(svcPort, clusterIP, svcPort.Port, svcHasLocalHostNetEndPnt, false)...)
 			}
 		}
-		externalIPs := make([]string, 0, len(service.Spec.ExternalIPs)+len(service.Status.LoadBalancer.Ingress))
-		externalIPs = append(externalIPs, service.Spec.ExternalIPs...)
-		for _, ingress := range service.Status.LoadBalancer.Ingress {
-			if len(ingress.IP) > 0 {
-				externalIPs = append(externalIPs, ingress.IP)
-			}
-		}
+
+		externalIPs := util.GetExternalAndLBIPs(service)
 
 		for _, externalIP := range externalIPs {
 			err := util.ValidatePort(svcPort.Protocol, svcPort.Port)

go-controller/pkg/node/gateway_shared_intf.go

@@ -200,15 +200,25 @@ func (npw *nodePortWatcher) updateServiceFlowCache(service *kapi.Service, add, h
 		// NodePort/Ingress access in the OVS bridge will only ever come from outside of the host
 		for _, ing := range service.Status.LoadBalancer.Ingress {
 			if len(ing.IP) > 0 {
-				err = npw.createLbAndExternalSvcFlows(service, &svcPort, add, hasLocalHostNetworkEp, protocol, actions, ing.IP, "Ingress")
+				ingressIPStr, err := util.ValidateAndGetIPString(ing.IP)
+				if err != nil {
+					klog.Errorf("Failed to validate Loadbalancer Ingress IP %s", ing.IP)
+					continue
+				}
+				err = npw.createLbAndExternalSvcFlows(service, &svcPort, add, hasLocalHostNetworkEp, protocol, actions, ingressIPStr, "Ingress")
 				if err != nil {
 					klog.Errorf(err.Error())
 				}
 			}
 		}
 		// flows for externalIPs
 		for _, externalIP := range service.Spec.ExternalIPs {
-			err = npw.createLbAndExternalSvcFlows(service, &svcPort, add, hasLocalHostNetworkEp, protocol, actions, externalIP, "External")
+			externalIPStr, err := util.ValidateAndGetIPString(externalIP)
+			if err != nil {
+				klog.Errorf("Failed to validate External IP %s", externalIP)
+				continue
+			}
+			err = npw.createLbAndExternalSvcFlows(service, &svcPort, add, hasLocalHostNetworkEp, protocol, actions, externalIPStr, "External")
 			if err != nil {
 				klog.Errorf(err.Error())
 			}

go-controller/pkg/node/healthcheck.go

@@ -114,8 +114,13 @@ func hasLocalHostNetworkEndpoints(epSlices []*discovery.EndpointSlice, nodeAddre
 	for _, epSlice := range epSlices {
 		for _, endpoint := range epSlice.Endpoints {
 			for _, ip := range endpoint.Addresses {
+				ipStr, err := util.ValidateAndGetIPString(ip)
+				if err != nil {
+					klog.Errorf("Failed to validate endpoints IP %s", ip)
+					continue
+				}
 				for _, nodeIP := range nodeAddresses {
-					if nodeIP.String() == ip {
+					if nodeIP.String() == ipStr {
 						return true
 					}
 				}

go-controller/pkg/node/port_claim.go

@@ -188,7 +188,12 @@ func handleService(svc *kapi.Service, handler handler) []error {
 		}
 		for _, externalIP := range svc.Spec.ExternalIPs {
 			klog.V(5).Infof("Handle ExternalIPs service %s external IP %s port %d", svc.Name, externalIP, svcPort.Port)
-			if err := handlePort(getDescription(svcPort.Name, svc, false), svc, externalIP, svcPort.Port, svcPort.Protocol, handler); err != nil {
+			externalIPStr, err := util.ValidateAndGetIPString(externalIP)
+			if err != nil {
+				klog.Errorf("Failed to validate External IP %s", externalIP)
+				continue
+			}
+			if err := handlePort(getDescription(svcPort.Name, svc, false), svc, externalIPStr, svcPort.Port, svcPort.Protocol, handler); err != nil {
 				errors = append(errors, err)
 			}
 		}

go-controller/pkg/ovn/controller/services/load_balancer.go

@@ -85,21 +85,9 @@ func buildServiceLBConfigs(service *v1.Service, endpointSlices []*discovery.Endp
 			perNodeConfigs = append(perNodeConfigs, nodePortLBConfig)
 		}
 
-		// Build up list of vips
-		vips := append([]string{}, service.Spec.ClusterIPs...)
-		// Handle old clusters w/o v6 support
-		if len(vips) == 0 {
-			vips = []string{service.Spec.ClusterIP}
-		}
-		externalVips := []string{}
-		// ExternalIP
-		externalVips = append(externalVips, service.Spec.ExternalIPs...)
-		// LoadBalancer status
-		for _, ingress := range service.Status.LoadBalancer.Ingress {
-			if ingress.IP != "" {
-				externalVips = append(externalVips, ingress.IP)
-			}
-		}
+		// Build up list of vips and externalVips
+		vips := util.GetClusterIPs(service)
+		externalVips := util.GetExternalAndLBIPs(service)
 
 		// if ETP=Local, then treat ExternalIPs and LoadBalancer IPs specially
 		// otherwise, they're just cluster IPs

go-controller/pkg/ovn/controller/services/utils.go

@@ -23,14 +23,8 @@ func deleteServiceFromLegacyLBs(nbClient libovsdbclient.Client, service *v1.Serv
 	vipPortsPerProtocol := map[v1.Protocol]sets.String{}
 
 	// Generate list of vip:port by proto
-	ips := append([]string{}, service.Spec.ClusterIPs...)
-	if len(ips) == 0 {
-		ips = append(ips, service.Spec.ClusterIP)
-	}
-	ips = append(ips, service.Spec.ExternalIPs...)
-	for _, ingress := range service.Status.LoadBalancer.Ingress {
-		ips = append(ips, ingress.IP)
-	}
+	ips := util.GetClusterIPs(service)
+	ips = append(ips, util.GetExternalAndLBIPs(service)...)
 	for _, svcPort := range service.Spec.Ports {
 		proto := svcPort.Protocol
 		ipPorts := make([]string, 0, len(ips))

go-controller/pkg/ovn/egressgw.go

@@ -1049,10 +1049,15 @@ func (oc *Controller) buildClusterECMPCacheFromNamespaces(clusterRouteCache map[
 					continue
 				}
 				for _, podIP := range nsPod.Status.PodIPs {
-					if utilnet.IsIPv6String(gwIP) != utilnet.IsIPv6String(podIP.IP) {
+					podIPStr, err := util.ValidateAndGetIPString(podIP.IP)
+					if err != nil {
+						klog.Errorf("Failed to validate Pod IP %s", podIP.IP)
 						continue
 					}
-					if val, ok := clusterRouteCache[podIP.IP]; ok {
+					if utilnet.IsIPv6String(gwIP) != utilnet.IsIPv6String(podIPStr) {
+						continue
+					}
+					if val, ok := clusterRouteCache[podIPStr]; ok {
 						// add gwIP to cache only if buildClusterECMPCacheFromPods hasn't already added it
 						gwIPexists := false
 						for _, existingGwIP := range val {
@@ -1062,10 +1067,10 @@ func (oc *Controller) buildClusterECMPCacheFromNamespaces(clusterRouteCache map[
 							}
 						}
 						if !gwIPexists {
-							clusterRouteCache[podIP.IP] = append(clusterRouteCache[podIP.IP], gwIP)
+							clusterRouteCache[podIPStr] = append(clusterRouteCache[podIPStr], gwIP)
 						}
 					} else {
-						clusterRouteCache[podIP.IP] = []string{gwIP}
+						clusterRouteCache[podIPStr] = []string{gwIP}
 					}
 				}
 			}
@@ -1106,10 +1111,15 @@ func (oc *Controller) buildClusterECMPCacheFromPods(clusterRouteCache map[string
 					continue
 				}
 				for _, podIP := range nsPod.Status.PodIPs {
-					if utilnet.IsIPv6String(gwIP) != utilnet.IsIPv6String(podIP.IP) {
+					podIPStr, err := util.ValidateAndGetIPString(podIP.IP)
+					if err != nil {
+						klog.Errorf("Failed to validate Pod IP %s", podIP.IP)
+						continue
+					}
+					if utilnet.IsIPv6String(gwIP) != utilnet.IsIPv6String(podIPStr) {
 						continue
 					}
-					clusterRouteCache[podIP.IP] = append(clusterRouteCache[podIP.IP], gwIP)
+					clusterRouteCache[podIPStr] = append(clusterRouteCache[podIPStr], gwIP)
 				}
 			}
 		}

go-controller/pkg/ovndbmanager/ovndbmanager.go

@@ -252,7 +252,12 @@ func ensureClusterRaftMembership(db *util.OvsDbProperties, kclient kube.Interfac
 		if !memberFound {
 			for _, dbPod := range dbPods.Items {
 				for _, ip := range dbPod.Status.PodIPs {
-					if ip.IP == matchedServer {
+					ipStr, err := util.ValidateAndGetIPString(ip.IP)
+					if err != nil {
+						klog.Errorf("Failed to validate Pod IP %s", ip.IP)
+						continue
+					}
+					if ip.IP == matchedServer || ipStr == matchedServer {
 						memberFound = true
 						break
 					}

go-controller/pkg/util/kube.go

@@ -162,22 +162,48 @@ func IsClusterIPSet(service *kapi.Service) bool {
 // we need to handle the case where only ClusterIP exist
 func GetClusterIPs(service *kapi.Service) []string {
 	if len(service.Spec.ClusterIPs) > 0 {
-		return service.Spec.ClusterIPs
+		clusterIPs := []string{}
+		for _, clusterIP := range service.Spec.ClusterIPs {
+			clusterIPStr, err := ValidateAndGetIPString(clusterIP)
+			if err != nil {
+				klog.Errorf("Failed to validate Cluster IP %s", clusterIP)
+				continue
+			}
+			clusterIPs = append(clusterIPs, clusterIPStr)
+		}
+		return clusterIPs
 	}
 	if len(service.Spec.ClusterIP) > 0 && service.Spec.ClusterIP != kapi.ClusterIPNone {
-		return []string{service.Spec.ClusterIP}
+		clusterIPStr, err := ValidateAndGetIPString(service.Spec.ClusterIP)
+		if err != nil {
+			klog.Errorf("Failed to validate Cluster IP %s", service.Spec.ClusterIP)
+			return []string{}
+		}
+		return []string{clusterIPStr}
 	}
 	return []string{}
 }
 
 // GetExternalAndLBIPs returns an array with the ExternalIPs and LoadBalancer IPs present in the service
 func GetExternalAndLBIPs(service *kapi.Service) []string {
 	svcVIPs := []string{}
-	svcVIPs = append(svcVIPs, service.Spec.ExternalIPs...)
+	for _, externalIP := range service.Spec.ExternalIPs {
+		externalIPStr, err := ValidateAndGetIPString(externalIP)
+		if err != nil {
+			klog.Errorf("Failed to validate External IP %s", externalIP)
+			continue
+		}
+		svcVIPs = append(svcVIPs, externalIPStr)
+	}
 	if ServiceTypeHasLoadBalancer(service) {
 		for _, ingressVIP := range service.Status.LoadBalancer.Ingress {
 			if len(ingressVIP.IP) > 0 {
-				svcVIPs = append(svcVIPs, ingressVIP.IP)
+				ingressIPStr, err := ValidateAndGetIPString(ingressVIP.IP)
+				if err != nil {
+					klog.Errorf("Failed to validate Loadbalancer Ingress IP %s", ingressVIP.IP)
+					continue
+				}
+				svcVIPs = append(svcVIPs, ingressIPStr)
 			}
 		}
 	}
@@ -230,12 +256,22 @@ func GetNodePrimaryIP(node *kapi.Node) (string, error) {
 	}
 	for _, addr := range node.Status.Addresses {
 		if addr.Type == kapi.NodeInternalIP {
-			return addr.Address, nil
+			nodeAddrStr, err := ValidateAndGetIPString(addr.Address)
+			if err != nil {
+				klog.Errorf("Failed to validate Node Internal IP %s", addr.Address)
+				continue
+			}
+			return nodeAddrStr, nil
 		}
 	}
 	for _, addr := range node.Status.Addresses {
 		if addr.Type == kapi.NodeExternalIP {
-			return addr.Address, nil
+			nodeAddrStr, err := ValidateAndGetIPString(addr.Address)
+			if err != nil {
+				klog.Errorf("Failed to validate Node External IP %s", addr.Address)
+				continue
+			}
+			return nodeAddrStr, nil
 		}
 	}
 	return "", fmt.Errorf("%s doesn't have an address with type %s or %s", node.GetName(),
@@ -328,11 +364,16 @@ func GetLbEndpoints(slices []*discovery.EndpointSlice, svcPort kapi.ServicePort)
 				}
 				for _, ip := range endpoint.Addresses {
 					klog.V(4).Infof("Adding slice %s endpoints: %v, port: %d", slice.Name, endpoint.Addresses, *port.Port)
+					ipStr, err := ValidateAndGetIPString(ip)
+					if err != nil {
+						klog.Errorf("Failed to validate endpoints IP %s", ip)
+						continue
+					}
 					switch slice.AddressType {
 					case discovery.AddressTypeIPv4:
-						v4ips.Insert(ip)
+						v4ips.Insert(ipStr)
 					case discovery.AddressTypeIPv6:
-						v6ips.Insert(ip)
+						v6ips.Insert(ipStr)
 					default:
 						klog.V(5).Infof("Skipping FQDN slice %s/%s", slice.Namespace, slice.Name)
 					}

go-controller/pkg/util/kube_test.go

@@ -296,24 +296,24 @@ func TestGetNodePrimaryIP(t *testing.T) {
 				Status: v1.NodeStatus{
 					Addresses: []v1.NodeAddress{
 						{Type: v1.NodeHostName, Address: "HN"},
-						{Type: v1.NodeInternalIP, Address: "IntIP"},
-						{Type: v1.NodeExternalIP, Address: "ExtIP"},
+						{Type: v1.NodeInternalIP, Address: "192.168.1.1"},
+						{Type: v1.NodeExternalIP, Address: "90.90.90.90"},
 					},
 				},
 			},
-			expOut: "IntIP",
+			expOut: "192.168.1.1",
 		},
 		{
 			desc: "success: node's external IP returned",
 			inp: v1.Node{
 				Status: v1.NodeStatus{
 					Addresses: []v1.NodeAddress{
 						{Type: v1.NodeHostName, Address: "HN"},
-						{Type: v1.NodeExternalIP, Address: "ExtIP"},
+						{Type: v1.NodeExternalIP, Address: "90.90.90.90"},
 					},
 				},
 			},
-			expOut: "ExtIP",
+			expOut: "90.90.90.90",
 		},
 	}
 	for i, tc := range tests {

go-controller/pkg/util/util.go

@@ -313,3 +313,15 @@ func UpdateNodeSwitchExcludeIPs(nbClient libovsdbclient.Client, nodeName string,
 
 	return nil
 }
+
+// ValidateAndGetIPString validates the IP address and returns the IP in
+// string format removing any leading zeros.
+func ValidateAndGetIPString(ip string) (string, error) {
+	// Check IP is valid or not
+	ipNet := utilnet.ParseIPSloppy(ip)
+	if ipNet == nil {
+		return "", fmt.Errorf("failed to parse IP %s", ip)
+	}
+	// Get the IP in string format from ipNet variable to remove any leading zeros
+	return ipNet.String(), nil
+}

go-controller/pkg/util/util_unit_test.go

@@ -12,13 +12,14 @@ import (
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb"
 	ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types"
-	utilnet "k8s.io/utils/net"
 
 	mock_k8s_io_utils_exec "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/mocks/k8s.io/utils/exec"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util/mocks"
 	"github.com/stretchr/testify/assert"
 
 	libovsdbtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb"
+
+	utilnet "k8s.io/utils/net"
 )
 
 func TestGetLegacyK8sMgmtIntfName(t *testing.T) {
@@ -514,3 +515,42 @@ func TestUpdateNodeSwitchExcludeIPs(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateAndGetIPString(t *testing.T) {
+	tests := []struct {
+		desc   string
+		inpIP  string
+		expOut string
+		expErr bool
+	}{
+		{
+			desc:   "Invalid IP",
+			inpIP:  "1111.1.1.1",
+			expOut: "",
+			expErr: true,
+		},
+		{
+			desc:   "Valid IP with leading zeros",
+			inpIP:  "01.01.01.01",
+			expOut: "1.1.1.1",
+			expErr: false,
+		},
+		{
+			desc:   "Valid IP without leading zeros",
+			inpIP:  "1.1.1.1",
+			expOut: "1.1.1.1",
+			expErr: false,
+		},
+	}
+	for i, tc := range tests {
+		t.Run(fmt.Sprintf("%d:%s", i, tc.desc), func(t *testing.T) {
+			result, err := ValidateAndGetIPString(tc.inpIP)
+			t.Log(result, err)
+			if tc.expErr {
+				assert.Error(t, err)
+			} else {
+				assert.Equal(t, result, tc.expOut)
+			}
+		})
+	}
+}