kube-proxy: store ExternalIPs as net.IP

They were stored as strings which could be non-canonical
and cause problems

pkg/proxy/conntrack/cleanup.go

@@ -50,8 +50,8 @@ func deleteStaleServiceConntrackEntries(isIPv6 bool, exec utilexec.Interface, sv
 		if svcInfo, ok := svcPortMap[svcPortName]; ok {
 			klog.V(4).InfoS("Newly-active UDP service may have stale conntrack entries", "servicePortName", svcPortName)
 			conntrackCleanupServiceIPs.Insert(svcInfo.ClusterIP().String())
-			for _, extIP := range svcInfo.ExternalIPStrings() {
-				conntrackCleanupServiceIPs.Insert(extIP)
+			for _, extIP := range svcInfo.ExternalIPs() {
+				conntrackCleanupServiceIPs.Insert(extIP.String())
 			}
 			for _, lbIP := range svcInfo.LoadBalancerVIPs() {
 				conntrackCleanupServiceIPs.Insert(lbIP.String())
@@ -97,8 +97,8 @@ func deleteStaleEndpointConntrackEntries(exec utilexec.Interface, svcPortMap pro
 			if err != nil {
 				klog.ErrorS(err, "Failed to delete endpoint connections", "servicePortName", epSvcPair.ServicePortName)
 			}
-			for _, extIP := range svcInfo.ExternalIPStrings() {
-				err := ClearEntriesForNAT(exec, extIP, endpointIP, v1.ProtocolUDP)
+			for _, extIP := range svcInfo.ExternalIPs() {
+				err := ClearEntriesForNAT(exec, extIP.String(), endpointIP, v1.ProtocolUDP)
 				if err != nil {
 					klog.ErrorS(err, "Failed to delete endpoint connections for externalIP", "servicePortName", epSvcPair.ServicePortName, "externalIP", extIP)
 				}

pkg/proxy/iptables/proxier.go

@@ -1077,7 +1077,7 @@ func (proxier *Proxier) syncProxyRules() {
 		}
 
 		// Capture externalIPs.
-		for _, externalIP := range svcInfo.ExternalIPStrings() {
+		for _, externalIP := range svcInfo.ExternalIPs() {
 			if hasEndpoints {
 				// Send traffic bound for external IPs to the "external
 				// destinations" chain.
@@ -1085,7 +1085,7 @@ func (proxier *Proxier) syncProxyRules() {
 					"-A", string(kubeServicesChain),
 					"-m", "comment", "--comment", fmt.Sprintf(`"%s external IP"`, svcPortNameString),
 					"-m", protocol, "-p", protocol,
-					"-d", externalIP,
+					"-d", externalIP.String(),
 					"--dport", strconv.Itoa(svcInfo.Port()),
 					"-j", string(externalTrafficChain))
 			}
@@ -1097,7 +1097,7 @@ func (proxier *Proxier) syncProxyRules() {
 					"-A", string(kubeExternalServicesChain),
 					"-m", "comment", "--comment", externalTrafficFilterComment,
 					"-m", protocol, "-p", protocol,
-					"-d", externalIP,
+					"-d", externalIP.String(),
 					"--dport", strconv.Itoa(svcInfo.Port()),
 					"-j", externalTrafficFilterTarget,
 				)

pkg/proxy/ipvs/proxier.go

@@ -1097,10 +1097,10 @@ func (proxier *Proxier) syncProxyRules() {
 		}
 
 		// Capture externalIPs.
-		for _, externalIP := range svcInfo.ExternalIPStrings() {
+		for _, externalIP := range svcInfo.ExternalIPs() {
 			// ipset call
 			entry := &utilipset.Entry{
-				IP:       externalIP,
+				IP:       externalIP.String(),
 				Port:     svcInfo.Port(),
 				Protocol: protocol,
 				SetType:  utilipset.HashIPPort,
@@ -1123,7 +1123,7 @@ func (proxier *Proxier) syncProxyRules() {
 
 			// ipvs call
 			serv := &utilipvs.VirtualServer{
-				Address:   netutils.ParseIPSloppy(externalIP),
+				Address:   externalIP,
 				Port:      uint16(svcInfo.Port()),
 				Protocol:  string(svcInfo.Protocol()),
 				Scheduler: proxier.ipvsScheduler,

pkg/proxy/nftables/proxier.go

@@ -1153,14 +1153,14 @@ func (proxier *Proxier) syncProxyRules() {
 		}
 
 		// Capture externalIPs.
-		for _, externalIP := range svcInfo.ExternalIPStrings() {
+		for _, externalIP := range svcInfo.ExternalIPs() {
 			if hasEndpoints {
 				// Send traffic bound for external IPs to the "external
 				// destinations" chain.
 				tx.Add(&knftables.Element{
 					Map: kubeServiceIPsMap,
 					Key: []string{
-						externalIP,
+						externalIP.String(),
 						protocol,
 						strconv.Itoa(svcInfo.Port()),
 					},
@@ -1176,7 +1176,7 @@ func (proxier *Proxier) syncProxyRules() {
 				tx.Add(&knftables.Element{
 					Map: kubeNoEndpointServicesMap,
 					Key: []string{
-						externalIP,
+						externalIP.String(),
 						protocol,
 						strconv.Itoa(svcInfo.Port()),
 					},

pkg/proxy/servicechangetracker_test.go

@@ -411,7 +411,7 @@ func TestServiceToServiceMap(t *testing.T) {
 			},
 			expected: map[ServicePortName]*BaseServicePortInfo{
 				makeServicePortName("test", "validIPv4", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv4, 12345, "TCP", 0, func(bsvcPortInfo *BaseServicePortInfo) {
-					bsvcPortInfo.externalIPs = []string{testExternalIPv4}
+					bsvcPortInfo.externalIPs = makeIPs(testExternalIPv4)
 					bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv4}
 					bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv4)
 				}),
@@ -449,7 +449,7 @@ func TestServiceToServiceMap(t *testing.T) {
 			},
 			expected: map[ServicePortName]*BaseServicePortInfo{
 				makeServicePortName("test", "validIPv6", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv6, 12345, "TCP", 0, func(bsvcPortInfo *BaseServicePortInfo) {
-					bsvcPortInfo.externalIPs = []string{testExternalIPv6}
+					bsvcPortInfo.externalIPs = makeIPs(testExternalIPv6)
 					bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv6}
 					bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv6)
 				}),
@@ -487,7 +487,7 @@ func TestServiceToServiceMap(t *testing.T) {
 			},
 			expected: map[ServicePortName]*BaseServicePortInfo{
 				makeServicePortName("test", "filterIPv6InIPV4Mode", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv4, 12345, "TCP", 0, func(bsvcPortInfo *BaseServicePortInfo) {
-					bsvcPortInfo.externalIPs = []string{testExternalIPv4}
+					bsvcPortInfo.externalIPs = makeIPs(testExternalIPv4)
 					bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv4}
 					bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv4)
 				}),
@@ -525,7 +525,7 @@ func TestServiceToServiceMap(t *testing.T) {
 			},
 			expected: map[ServicePortName]*BaseServicePortInfo{
 				makeServicePortName("test", "filterIPv4InIPV6Mode", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv6, 12345, "TCP", 0, func(bsvcPortInfo *BaseServicePortInfo) {
-					bsvcPortInfo.externalIPs = []string{testExternalIPv6}
+					bsvcPortInfo.externalIPs = makeIPs(testExternalIPv6)
 					bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv6}
 					bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv6)
 				}),
@@ -580,7 +580,7 @@ func TestServiceToServiceMap(t *testing.T) {
 					svcInfo.port != expectedInfo.port ||
 					svcInfo.protocol != expectedInfo.protocol ||
 					svcInfo.healthCheckNodePort != expectedInfo.healthCheckNodePort ||
-					!sets.New[string](svcInfo.externalIPs...).Equal(sets.New[string](expectedInfo.externalIPs...)) ||
+					!reflect.DeepEqual(svcInfo.externalIPs, expectedInfo.externalIPs) ||
 					!sets.New[string](svcInfo.loadBalancerSourceRanges...).Equal(sets.New[string](expectedInfo.loadBalancerSourceRanges...)) ||
 					!reflect.DeepEqual(svcInfo.loadBalancerVIPs, expectedInfo.loadBalancerVIPs) {
 					t.Errorf("[%s] expected new[%v]to be %v, got %v", tc.desc, svcKey, expectedInfo, *svcInfo)
@@ -591,7 +591,7 @@ func TestServiceToServiceMap(t *testing.T) {
 						svcInfo.port != expectedInfo.port ||
 						svcInfo.protocol != expectedInfo.protocol ||
 						svcInfo.healthCheckNodePort != expectedInfo.healthCheckNodePort ||
-						!sets.New[string](svcInfo.externalIPs...).Equal(sets.New[string](expectedInfo.externalIPs...)) ||
+						!reflect.DeepEqual(svcInfo.externalIPs, expectedInfo.externalIPs) ||
 						!sets.New[string](svcInfo.loadBalancerSourceRanges...).Equal(sets.New[string](expectedInfo.loadBalancerSourceRanges...)) ||
 						!reflect.DeepEqual(svcInfo.loadBalancerVIPs, expectedInfo.loadBalancerVIPs) {
 						t.Errorf("expected new[%v]to be %v, got %v", svcKey, expectedInfo, *svcInfo)

pkg/proxy/serviceport.go

@@ -40,8 +40,8 @@ type ServicePort interface {
 	SessionAffinityType() v1.ServiceAffinity
 	// StickyMaxAgeSeconds returns service max connection age
 	StickyMaxAgeSeconds() int
-	// ExternalIPStrings returns service ExternalIPs as a string array.
-	ExternalIPStrings() []string
+	// ExternalIPs returns service ExternalIPs
+	ExternalIPs() []net.IP
 	// LoadBalancerVIPs returns service LoadBalancerIPs which are VIP mode
 	LoadBalancerVIPs() []net.IP
 	// Protocol returns service protocol.
@@ -81,7 +81,7 @@ type BaseServicePortInfo struct {
 	loadBalancerVIPs         []net.IP
 	sessionAffinityType      v1.ServiceAffinity
 	stickyMaxAgeSeconds      int
-	externalIPs              []string
+	externalIPs              []net.IP
 	loadBalancerSourceRanges []string
 	healthCheckNodePort      int
 	externalPolicyLocal      bool
@@ -136,8 +136,8 @@ func (bsvcPortInfo *BaseServicePortInfo) NodePort() int {
 	return bsvcPortInfo.nodePort
 }
 
-// ExternalIPStrings is part of ServicePort interface.
-func (bsvcPortInfo *BaseServicePortInfo) ExternalIPStrings() []string {
+// ExternalIPs is part of ServicePort interface.
+func (bsvcPortInfo *BaseServicePortInfo) ExternalIPs() []net.IP {
 	return bsvcPortInfo.externalIPs
 }
 
@@ -216,20 +216,19 @@ func newBaseServiceInfo(service *v1.Service, ipFamily v1.IPFamily, port *v1.Serv
 	// prior to dual stack services, this was considered an error, but with dual stack
 	// services, this is actually expected. Hence we downgraded from reporting by events
 	// to just log lines with high verbosity
-
 	ipFamilyMap := proxyutil.MapIPsByIPFamily(service.Spec.ExternalIPs)
 	info.externalIPs = ipFamilyMap[ipFamily]
 
 	// Log the IPs not matching the ipFamily
 	if ips, ok := ipFamilyMap[proxyutil.OtherIPFamily(ipFamily)]; ok && len(ips) > 0 {
 		klog.V(4).InfoS("Service change tracker ignored the following external IPs for given service as they don't match IP Family",
-			"ipFamily", ipFamily, "externalIPs", strings.Join(ips, ", "), "service", klog.KObj(service))
+			"ipFamily", ipFamily, "externalIPs", ips, "service", klog.KObj(service))
 	}
 
-	ipFamilyMap = proxyutil.MapCIDRsByIPFamily(loadBalancerSourceRanges)
-	info.loadBalancerSourceRanges = ipFamilyMap[ipFamily]
+	cidrFamilyMap := proxyutil.MapCIDRsByIPFamily(loadBalancerSourceRanges)
+	info.loadBalancerSourceRanges = cidrFamilyMap[ipFamily]
 	// Log the CIDRs not matching the ipFamily
-	if cidrs, ok := ipFamilyMap[proxyutil.OtherIPFamily(ipFamily)]; ok && len(cidrs) > 0 {
+	if cidrs, ok := cidrFamilyMap[proxyutil.OtherIPFamily(ipFamily)]; ok && len(cidrs) > 0 {
 		klog.V(4).InfoS("Service change tracker ignored the following load balancer source ranges for given Service as they don't match IP Family",
 			"ipFamily", ipFamily, "loadBalancerSourceRanges", strings.Join(cidrs, ", "), "service", klog.KObj(service))
 	}

pkg/proxy/util/utils.go

@@ -180,19 +180,18 @@ func LogAndEmitIncorrectIPVersionEvent(recorder events.EventRecorder, fieldName,
 }
 
 // MapIPsByIPFamily maps a slice of IPs to their respective IP families (v4 or v6)
-func MapIPsByIPFamily(ipStrings []string) map[v1.IPFamily][]string {
-	ipFamilyMap := map[v1.IPFamily][]string{}
+func MapIPsByIPFamily(ipStrings []string) map[v1.IPFamily][]net.IP {
+	ipFamilyMap := map[v1.IPFamily][]net.IP{}
 	for _, ipStr := range ipStrings {
 		ip := netutils.ParseIPSloppy(ipStr)
-		// Handle only the valid IPs
-		if ipFamily := GetIPFamilyFromIP(ip); ipFamily != v1.IPFamilyUnknown {
-			ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], ipStr)
+		if ip != nil {
+			// Since ip is parsed ok, GetIPFamilyFromIP will never return v1.IPFamilyUnknown
+			ipFamily := GetIPFamilyFromIP(ip)
+			ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], ip)
 		} else {
-			// this function is called in multiple places. All of which
-			// have sanitized data. Except the case of ExternalIPs which is
-			// not validated by api-server. Specifically empty strings
-			// validation. Which yields into a lot of bad error logs.
-			// check for empty string
+			// ExternalIPs may not be validated by the api-server.
+			// Specifically empty strings validation, which yields into a lot
+			// of bad error logs.
 			if len(strings.TrimSpace(ipStr)) != 0 {
 				klog.ErrorS(nil, "Skipping invalid IP", "ip", ipStr)
 			}

pkg/proxy/util/utils_test.go

@@ -338,10 +338,18 @@ func TestMapIPsByIPFamily(t *testing.T) {
 
 			ipMap := MapIPsByIPFamily(testcase.ipString)
 
-			if !reflect.DeepEqual(testcase.expectCorrect, ipMap[ipFamily]) {
+			var ipStr []string
+			for _, ip := range ipMap[ipFamily] {
+				ipStr = append(ipStr, ip.String())
+			}
+			if !reflect.DeepEqual(testcase.expectCorrect, ipStr) {
 				t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectCorrect, ipMap[ipFamily])
 			}
-			if !reflect.DeepEqual(testcase.expectIncorrect, ipMap[otherIPFamily]) {
+			ipStr = nil
+			for _, ip := range ipMap[otherIPFamily] {
+				ipStr = append(ipStr, ip.String())
+			}
+			if !reflect.DeepEqual(testcase.expectIncorrect, ipStr) {
 				t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectIncorrect, ipMap[otherIPFamily])
 			}
 		})