diff --git a/pkg/proxy/conntrack/cleanup.go b/pkg/proxy/conntrack/cleanup.go index 6abd7b40c92..6e5046f0c0b 100644 --- a/pkg/proxy/conntrack/cleanup.go +++ b/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) } diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index e6ebdfb9658..f3ed47ae933 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/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, ) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 2e587675b6e..df3b18d430c 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/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, diff --git a/pkg/proxy/nftables/proxier.go b/pkg/proxy/nftables/proxier.go index dc9f3948930..e2f5e6a9ebc 100644 --- a/pkg/proxy/nftables/proxier.go +++ b/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()), }, diff --git a/pkg/proxy/servicechangetracker_test.go b/pkg/proxy/servicechangetracker_test.go index 0e7965c6aa5..d270cc3ea47 100644 --- a/pkg/proxy/servicechangetracker_test.go +++ b/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) diff --git a/pkg/proxy/serviceport.go b/pkg/proxy/serviceport.go index 61513afdcb4..fb33c83c768 100644 --- a/pkg/proxy/serviceport.go +++ b/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)) } diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go index 5d6317c3e14..7f5f8b540be 100644 --- a/pkg/proxy/util/utils.go +++ b/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) } diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index a589171adf4..3da6ae7700c 100644 --- a/pkg/proxy/util/utils_test.go +++ b/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]) } })