diff --git a/cmd/kube-proxy/app/server_linux.go b/cmd/kube-proxy/app/server_linux.go index b851297d6f3..f36ad10e6f7 100644 --- a/cmd/kube-proxy/app/server_linux.go +++ b/cmd/kube-proxy/app/server_linux.go @@ -508,7 +508,7 @@ func getLocalDetector(logger klog.Logger, ipFamily v1.IPFamily, mode proxyconfig cidrsByFamily := proxyutil.MapCIDRsByIPFamily(strings.Split(clusterCIDRs, ",")) if len(cidrsByFamily[ipFamily]) != 0 { - return proxyutiliptables.NewDetectLocalByCIDR(cidrsByFamily[ipFamily][0]) + return proxyutiliptables.NewDetectLocalByCIDR(cidrsByFamily[ipFamily][0].String()) } logger.Info("Detect-local-mode set to ClusterCIDR, but no cluster CIDR for family", "ipFamily", ipFamily) @@ -516,7 +516,7 @@ func getLocalDetector(logger klog.Logger, ipFamily v1.IPFamily, mode proxyconfig case proxyconfigapi.LocalModeNodeCIDR: cidrsByFamily := proxyutil.MapCIDRsByIPFamily(nodePodCIDRs) if len(cidrsByFamily[ipFamily]) != 0 { - return proxyutiliptables.NewDetectLocalByCIDR(cidrsByFamily[ipFamily][0]) + return proxyutiliptables.NewDetectLocalByCIDR(cidrsByFamily[ipFamily][0].String()) } logger.Info("Detect-local-mode set to NodeCIDR, but no PodCIDR defined at node for family", "ipFamily", ipFamily) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index f3ed47ae933..1719fefa229 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -54,7 +54,6 @@ import ( "k8s.io/kubernetes/pkg/util/async" utiliptables "k8s.io/kubernetes/pkg/util/iptables" utilexec "k8s.io/utils/exec" - netutils "k8s.io/utils/net" ) const ( @@ -1294,12 +1293,9 @@ func (proxier *Proxier) syncProxyRules() { // firewall filter based on each source range allowFromNode := false - for _, src := range svcInfo.LoadBalancerSourceRanges() { - natRules.Write(args, "-s", src, "-j", string(externalTrafficChain)) - _, cidr, err := netutils.ParseCIDRSloppy(src) - if err != nil { - klog.ErrorS(err, "Error parsing CIDR in LoadBalancerSourceRanges, dropping it", "cidr", cidr) - } else if cidr.Contains(proxier.nodeIP) { + for _, cidr := range svcInfo.LoadBalancerSourceRanges() { + natRules.Write(args, "-s", cidr.String(), "-j", string(externalTrafficChain)) + if cidr.Contains(proxier.nodeIP) { allowFromNode = true } } diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index df3b18d430c..6643c432d3c 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1187,13 +1187,13 @@ func (proxier *Proxier) syncProxyRules() { } proxier.ipsetList[kubeLoadBalancerFWSet].activeEntries.Insert(entry.String()) allowFromNode := false - for _, src := range svcInfo.LoadBalancerSourceRanges() { + for _, cidr := range svcInfo.LoadBalancerSourceRanges() { // ipset call entry = &utilipset.Entry{ IP: ingress.String(), Port: svcInfo.Port(), Protocol: protocol, - Net: src, + Net: cidr.String(), SetType: utilipset.HashIPPortNet, } // enumerate all white list source cidr @@ -1203,8 +1203,6 @@ func (proxier *Proxier) syncProxyRules() { } proxier.ipsetList[kubeLoadBalancerSourceCIDRSet].activeEntries.Insert(entry.String()) - // ignore error because it has been validated - _, cidr, _ := netutils.ParseCIDRSloppy(src) if cidr.Contains(proxier.nodeIP) { allowFromNode = true } diff --git a/pkg/proxy/nftables/proxier.go b/pkg/proxy/nftables/proxier.go index e2f5e6a9ebc..39311a7abd2 100644 --- a/pkg/proxy/nftables/proxier.go +++ b/pkg/proxy/nftables/proxier.go @@ -55,7 +55,6 @@ import ( proxyutiliptables "k8s.io/kubernetes/pkg/proxy/util/iptables" "k8s.io/kubernetes/pkg/util/async" utilexec "k8s.io/utils/exec" - netutils "k8s.io/utils/net" "k8s.io/utils/ptr" ) @@ -1194,15 +1193,11 @@ func (proxier *Proxier) syncProxyRules() { ensureChain(fwChain, tx, activeChains) var sources []string allowFromNode := false - for _, src := range svcInfo.LoadBalancerSourceRanges() { - _, cidr, _ := netutils.ParseCIDRSloppy(src) - if cidr == nil { - continue - } + for _, cidr := range svcInfo.LoadBalancerSourceRanges() { if len(sources) > 0 { sources = append(sources, ",") } - sources = append(sources, src) + sources = append(sources, cidr.String()) if cidr.Contains(proxier.nodeIP) { allowFromNode = true } diff --git a/pkg/proxy/servicechangetracker_test.go b/pkg/proxy/servicechangetracker_test.go index d270cc3ea47..d6fbabf680e 100644 --- a/pkg/proxy/servicechangetracker_test.go +++ b/pkg/proxy/servicechangetracker_test.go @@ -27,7 +27,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/dump" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/apimachinery/pkg/util/sets" utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/kubernetes/pkg/features" @@ -94,6 +93,17 @@ func makeIPs(ipStr ...string) []net.IP { } return ips } +func mustMakeCIDRs(cidrStr ...string) []*net.IPNet { + var cidrs []*net.IPNet + for _, s := range cidrStr { + if _, n, err := netutils.ParseCIDRSloppy(s); err == nil { + cidrs = append(cidrs, n) + } else { + panic(err) + } + } + return cidrs +} func TestServiceToServiceMap(t *testing.T) { testClusterIPv4 := "10.0.0.1" @@ -412,7 +422,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 = makeIPs(testExternalIPv4) - bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv4} + bsvcPortInfo.loadBalancerSourceRanges = mustMakeCIDRs(testSourceRangeIPv4) bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv4) }), }, @@ -450,7 +460,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 = makeIPs(testExternalIPv6) - bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv6} + bsvcPortInfo.loadBalancerSourceRanges = mustMakeCIDRs(testSourceRangeIPv6) bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv6) }), }, @@ -488,7 +498,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 = makeIPs(testExternalIPv4) - bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv4} + bsvcPortInfo.loadBalancerSourceRanges = mustMakeCIDRs(testSourceRangeIPv4) bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv4) }), }, @@ -526,7 +536,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 = makeIPs(testExternalIPv6) - bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv6} + bsvcPortInfo.loadBalancerSourceRanges = mustMakeCIDRs(testSourceRangeIPv6) bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv6) }), }, @@ -554,7 +564,7 @@ func TestServiceToServiceMap(t *testing.T) { }, expected: map[ServicePortName]*BaseServicePortInfo{ makeServicePortName("test", "extra-space", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv4, 12345, "TCP", 0, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerSourceRanges = []string{"10.1.2.0/28"} + bsvcPortInfo.loadBalancerSourceRanges = mustMakeCIDRs("10.1.2.0/28") }), }, }, @@ -581,7 +591,7 @@ func TestServiceToServiceMap(t *testing.T) { svcInfo.protocol != expectedInfo.protocol || svcInfo.healthCheckNodePort != expectedInfo.healthCheckNodePort || !reflect.DeepEqual(svcInfo.externalIPs, expectedInfo.externalIPs) || - !sets.New[string](svcInfo.loadBalancerSourceRanges...).Equal(sets.New[string](expectedInfo.loadBalancerSourceRanges...)) || + !reflect.DeepEqual(svcInfo.loadBalancerSourceRanges, expectedInfo.loadBalancerSourceRanges) || !reflect.DeepEqual(svcInfo.loadBalancerVIPs, expectedInfo.loadBalancerVIPs) { t.Errorf("[%s] expected new[%v]to be %v, got %v", tc.desc, svcKey, expectedInfo, *svcInfo) } @@ -592,7 +602,7 @@ func TestServiceToServiceMap(t *testing.T) { svcInfo.protocol != expectedInfo.protocol || svcInfo.healthCheckNodePort != expectedInfo.healthCheckNodePort || !reflect.DeepEqual(svcInfo.externalIPs, expectedInfo.externalIPs) || - !sets.New[string](svcInfo.loadBalancerSourceRanges...).Equal(sets.New[string](expectedInfo.loadBalancerSourceRanges...)) || + !reflect.DeepEqual(svcInfo.loadBalancerSourceRanges, 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 fb33c83c768..fa3f21758cc 100644 --- a/pkg/proxy/serviceport.go +++ b/pkg/proxy/serviceport.go @@ -19,7 +19,6 @@ package proxy import ( "fmt" "net" - "strings" v1 "k8s.io/api/core/v1" "k8s.io/klog/v2" @@ -47,7 +46,7 @@ type ServicePort interface { // Protocol returns service protocol. Protocol() v1.Protocol // LoadBalancerSourceRanges returns service LoadBalancerSourceRanges if present empty array if not - LoadBalancerSourceRanges() []string + LoadBalancerSourceRanges() []*net.IPNet // HealthCheckNodePort returns service health check node port if present. If return 0, it means not present. HealthCheckNodePort() int // NodePort returns a service Node port if present. If return 0, it means not present. @@ -82,7 +81,7 @@ type BaseServicePortInfo struct { sessionAffinityType v1.ServiceAffinity stickyMaxAgeSeconds int externalIPs []net.IP - loadBalancerSourceRanges []string + loadBalancerSourceRanges []*net.IPNet healthCheckNodePort int externalPolicyLocal bool internalPolicyLocal bool @@ -122,7 +121,7 @@ func (bsvcPortInfo *BaseServicePortInfo) Protocol() v1.Protocol { } // LoadBalancerSourceRanges is part of ServicePort interface -func (bsvcPortInfo *BaseServicePortInfo) LoadBalancerSourceRanges() []string { +func (bsvcPortInfo *BaseServicePortInfo) LoadBalancerSourceRanges() []*net.IPNet { return bsvcPortInfo.loadBalancerSourceRanges } @@ -208,10 +207,6 @@ func newBaseServiceInfo(service *v1.Service, ipFamily v1.IPFamily, port *v1.Serv info.hintsAnnotation = service.Annotations[v1.AnnotationTopologyMode] } - loadBalancerSourceRanges := make([]string, len(service.Spec.LoadBalancerSourceRanges)) - for i, sourceRange := range service.Spec.LoadBalancerSourceRanges { - loadBalancerSourceRanges[i] = strings.TrimSpace(sourceRange) - } // filter external ips, source ranges and ingress ips // 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 @@ -225,12 +220,12 @@ func newBaseServiceInfo(service *v1.Service, ipFamily v1.IPFamily, port *v1.Serv "ipFamily", ipFamily, "externalIPs", ips, "service", klog.KObj(service)) } - cidrFamilyMap := proxyutil.MapCIDRsByIPFamily(loadBalancerSourceRanges) + cidrFamilyMap := proxyutil.MapCIDRsByIPFamily(service.Spec.LoadBalancerSourceRanges) info.loadBalancerSourceRanges = cidrFamilyMap[ipFamily] // Log the CIDRs not matching the ipFamily 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)) + "ipFamily", ipFamily, "loadBalancerSourceRanges", cidrs, "service", klog.KObj(service)) } // Obtain Load Balancer Ingress diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go index 7f5f8b540be..a8cfbfd4e7f 100644 --- a/pkg/proxy/util/utils.go +++ b/pkg/proxy/util/utils.go @@ -200,16 +200,22 @@ func MapIPsByIPFamily(ipStrings []string) map[v1.IPFamily][]net.IP { return ipFamilyMap } -// MapCIDRsByIPFamily maps a slice of IPs to their respective IP families (v4 or v6) -func MapCIDRsByIPFamily(cidrStrings []string) map[v1.IPFamily][]string { - ipFamilyMap := map[v1.IPFamily][]string{} - for _, cidr := range cidrStrings { - // Handle only the valid CIDRs - if ipFamily := getIPFamilyFromCIDR(cidr); ipFamily != v1.IPFamilyUnknown { - ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], cidr) - } else { - klog.ErrorS(nil, "Skipping invalid CIDR", "cidr", cidr) +// MapCIDRsByIPFamily maps a slice of CIDRs to their respective IP families (v4 or v6) +func MapCIDRsByIPFamily(cidrsStrings []string) map[v1.IPFamily][]*net.IPNet { + ipFamilyMap := map[v1.IPFamily][]*net.IPNet{} + for _, cidrStrUntrimmed := range cidrsStrings { + cidrStr := strings.TrimSpace(cidrStrUntrimmed) + _, cidr, err := netutils.ParseCIDRSloppy(cidrStr) + if err != nil { + // Ignore empty strings. Same as in MapIPsByIPFamily + if len(cidrStr) != 0 { + klog.ErrorS(err, "Invalid CIDR ignored", "CIDR", cidrStr) + } + continue } + // since we just succefully parsed the CIDR, IPFamilyOfCIDR will never return "IPFamilyUnknown" + ipFamily := convertToV1IPFamily(netutils.IPFamilyOfCIDR(cidr)) + ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], cidr) } return ipFamilyMap } @@ -219,11 +225,6 @@ func GetIPFamilyFromIP(ip net.IP) v1.IPFamily { return convertToV1IPFamily(netutils.IPFamilyOf(ip)) } -// Returns the IP family of cidrStr, or IPFamilyUnknown if cidrStr can't be parsed as a CIDR -func getIPFamilyFromCIDR(cidrStr string) v1.IPFamily { - return convertToV1IPFamily(netutils.IPFamilyOfCIDRString(cidrStr)) -} - // Convert netutils.IPFamily to v1.IPFamily func convertToV1IPFamily(ipFamily netutils.IPFamily) v1.IPFamily { switch ipFamily { diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index 3da6ae7700c..3895cd755db 100644 --- a/pkg/proxy/util/utils_test.go +++ b/pkg/proxy/util/utils_test.go @@ -448,11 +448,20 @@ func TestMapCIDRsByIPFamily(t *testing.T) { cidrMap := MapCIDRsByIPFamily(testcase.ipString) - if !reflect.DeepEqual(testcase.expectCorrect, cidrMap[ipFamily]) { - t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectCorrect, cidrMap[ipFamily]) + var cidrStr []string + for _, cidr := range cidrMap[ipFamily] { + cidrStr = append(cidrStr, cidr.String()) } - if !reflect.DeepEqual(testcase.expectIncorrect, cidrMap[otherIPFamily]) { - t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectIncorrect, cidrMap[otherIPFamily]) + var cidrStrOther []string + for _, cidr := range cidrMap[otherIPFamily] { + cidrStrOther = append(cidrStrOther, cidr.String()) + } + + if !reflect.DeepEqual(testcase.expectCorrect, cidrStr) { + t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectCorrect, cidrStr) + } + if !reflect.DeepEqual(testcase.expectIncorrect, cidrStrOther) { + t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectIncorrect, cidrStrOther) } }) }