From 564b80b1e1105bcb982736f6bd193fd2e886372c Mon Sep 17 00:00:00 2001 From: Lars Ekman Date: Tue, 9 Jan 2024 07:15:31 +0100 Subject: [PATCH 1/4] kube-proxy: don't use invalid cidrs in unit test CIDRs like 192.168.200.3/24 and fd00:20::1/64 replaced with 192.168.200.0/24 and fd00:20::/64 --- cmd/kube-proxy/app/server_linux_test.go | 54 ++++++++++++------------- pkg/proxy/util/iptables/traffic_test.go | 14 +++---- pkg/proxy/util/utils_test.go | 36 ++++++++--------- 3 files changed, 50 insertions(+), 54 deletions(-) diff --git a/cmd/kube-proxy/app/server_linux_test.go b/cmd/kube-proxy/app/server_linux_test.go index 6e6abc76bae..66664d0cc47 100644 --- a/cmd/kube-proxy/app/server_linux_test.go +++ b/cmd/kube-proxy/app/server_linux_test.go @@ -129,9 +129,9 @@ func Test_getLocalDetector(t *testing.T) { { name: "LocalModeClusterCIDR, IPv6 cluster", mode: proxyconfigapi.LocalModeClusterCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101/64"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002:0:0:1234::/64"}, family: v1.IPv6Protocol, - expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/64")), + expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002:0:0:1234::/64")), errExpected: false, }, { @@ -145,7 +145,7 @@ func Test_getLocalDetector(t *testing.T) { { name: "LocalModeClusterCIDR, IPv4 cluster with IPv6 config", mode: proxyconfigapi.LocalModeClusterCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101/64"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002:0:0:1234::/64"}, family: v1.IPv4Protocol, expected: proxyutiliptables.NewNoOpLocalDetector(), errExpected: false, @@ -153,7 +153,7 @@ func Test_getLocalDetector(t *testing.T) { { name: "LocalModeClusterCIDR, IPv4 kube-proxy in dual-stack IPv6-primary cluster", mode: proxyconfigapi.LocalModeClusterCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101/64,10.0.0.0/14"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002:0:0:1234::/64,10.0.0.0/14"}, family: v1.IPv4Protocol, expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("10.0.0.0/14")), errExpected: false, @@ -179,10 +179,10 @@ func Test_getLocalDetector(t *testing.T) { { name: "LocalModeNodeCIDR, IPv6 cluster", mode: proxyconfigapi.LocalModeNodeCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101/64"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002:0:0:1234::/64"}, family: v1.IPv6Protocol, - expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/96")), - nodePodCIDRs: []string{"2002::1234:abcd:ffff:c0a8:101/96"}, + expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:0:0/96")), + nodePodCIDRs: []string{"2002::1234:abcd:ffff:0:0/96"}, errExpected: false, }, { @@ -197,19 +197,19 @@ func Test_getLocalDetector(t *testing.T) { { name: "LocalModeNodeCIDR, IPv4 cluster with IPv6 config", mode: proxyconfigapi.LocalModeNodeCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101/64"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002:0:0:1234::/64"}, family: v1.IPv4Protocol, expected: proxyutiliptables.NewNoOpLocalDetector(), - nodePodCIDRs: []string{"2002::1234:abcd:ffff:c0a8:101/96"}, + nodePodCIDRs: []string{"2002::1234:abcd:ffff:0:0/96"}, errExpected: false, }, { name: "LocalModeNodeCIDR, IPv6 kube-proxy in dual-stack IPv4-primary cluster", mode: proxyconfigapi.LocalModeNodeCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0/14,2002::1234:abcd:ffff:c0a8:101/64"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0/14,2002:0:0:1234::/64"}, family: v1.IPv6Protocol, - expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/96")), - nodePodCIDRs: []string{"10.0.0.0/24", "2002::1234:abcd:ffff:c0a8:101/96"}, + expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:0:0/96")), + nodePodCIDRs: []string{"10.0.0.0/24", "2002::1234:abcd:ffff:0:0/96"}, errExpected: false, }, { @@ -307,19 +307,19 @@ func Test_getDualStackLocalDetectorTuple(t *testing.T) { { name: "LocalModeClusterCIDR, dual-stack IPv4-primary cluster", mode: proxyconfigapi.LocalModeClusterCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0/14,2002::1234:abcd:ffff:c0a8:101/64"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0/14,2002:0:0:1234::/64"}, expected: resolveDualStackLocalDetectors(t)( proxyutiliptables.NewDetectLocalByCIDR("10.0.0.0/14"))( - proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/64")), + proxyutiliptables.NewDetectLocalByCIDR("2002:0:0:1234::/64")), errExpected: false, }, { name: "LocalModeClusterCIDR, dual-stack IPv6-primary cluster", mode: proxyconfigapi.LocalModeClusterCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101/64,10.0.0.0/14"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002:0:0:1234::/64,10.0.0.0/14"}, expected: resolveDualStackLocalDetectors(t)( proxyutiliptables.NewDetectLocalByCIDR("10.0.0.0/14"))( - proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/64")), + proxyutiliptables.NewDetectLocalByCIDR("2002:0:0:1234::/64")), errExpected: false, }, { @@ -334,10 +334,10 @@ func Test_getDualStackLocalDetectorTuple(t *testing.T) { { name: "LocalModeClusterCIDR, single-stack IPv6 cluster", mode: proxyconfigapi.LocalModeClusterCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101/64"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002:0:0:1234::/64"}, expected: [2]proxyutiliptables.LocalTrafficDetector{ proxyutiliptables.NewNoOpLocalDetector(), - resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/64"))}, + resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002:0:0:1234::/64"))}, errExpected: false, }, { @@ -351,21 +351,21 @@ func Test_getDualStackLocalDetectorTuple(t *testing.T) { { name: "LocalModeNodeCIDR, dual-stack IPv4-primary cluster", mode: proxyconfigapi.LocalModeNodeCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0/14,2002::1234:abcd:ffff:c0a8:101/64"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0/14,2002:0:0:1234::/64"}, expected: resolveDualStackLocalDetectors(t)( proxyutiliptables.NewDetectLocalByCIDR("10.0.0.0/24"))( - proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/96")), - nodePodCIDRs: []string{"10.0.0.0/24", "2002::1234:abcd:ffff:c0a8:101/96"}, + proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:0:0/96")), + nodePodCIDRs: []string{"10.0.0.0/24", "2002::1234:abcd:ffff:0:0/96"}, errExpected: false, }, { name: "LocalModeNodeCIDR, dual-stack IPv6-primary cluster", mode: proxyconfigapi.LocalModeNodeCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101/64,10.0.0.0/14"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002:0:0:1234::/64,10.0.0.0/14"}, expected: resolveDualStackLocalDetectors(t)( proxyutiliptables.NewDetectLocalByCIDR("10.0.0.0/24"))( - proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/96")), - nodePodCIDRs: []string{"2002::1234:abcd:ffff:c0a8:101/96", "10.0.0.0/24"}, + proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:0:0/96")), + nodePodCIDRs: []string{"2002::1234:abcd:ffff:0:0/96", "10.0.0.0/24"}, errExpected: false, }, { @@ -381,11 +381,11 @@ func Test_getDualStackLocalDetectorTuple(t *testing.T) { { name: "LocalModeNodeCIDR, single-stack IPv6 cluster", mode: proxyconfigapi.LocalModeNodeCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101/64"}, + config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002:0:0:1234::/64"}, expected: [2]proxyutiliptables.LocalTrafficDetector{ proxyutiliptables.NewNoOpLocalDetector(), - resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/96"))}, - nodePodCIDRs: []string{"2002::1234:abcd:ffff:c0a8:101/96"}, + resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:0:0/96"))}, + nodePodCIDRs: []string{"2002::1234:abcd:ffff:0:0/96"}, errExpected: false, }, { diff --git a/pkg/proxy/util/iptables/traffic_test.go b/pkg/proxy/util/iptables/traffic_test.go index f60169de9da..1741261a352 100644 --- a/pkg/proxy/util/iptables/traffic_test.go +++ b/pkg/proxy/util/iptables/traffic_test.go @@ -48,7 +48,7 @@ func TestNewDetectLocalByCIDR(t *testing.T) { errExpected: false, }, { - cidr: "2002::1234:abcd:ffff:c0a8:101/64", + cidr: "2002:0:0:1234::/64", errExpected: false, }, { @@ -56,11 +56,7 @@ func TestNewDetectLocalByCIDR(t *testing.T) { errExpected: true, }, { - cidr: "2002::1234:abcd:ffff:c0a8:101", - errExpected: true, - }, - { - cidr: "", + cidr: "2002:0:0:1234::", errExpected: true, }, { @@ -94,9 +90,9 @@ func TestDetectLocalByCIDR(t *testing.T) { expectedIfNotLocalOutput: []string{"!", "-s", "10.0.0.0/14"}, }, { - cidr: "2002::1234:abcd:ffff:c0a8:101/64", - expectedIfLocalOutput: []string{"-s", "2002::1234:abcd:ffff:c0a8:101/64"}, - expectedIfNotLocalOutput: []string{"!", "-s", "2002::1234:abcd:ffff:c0a8:101/64"}, + cidr: "2002:0:0:1234::/64", + expectedIfLocalOutput: []string{"-s", "2002:0:0:1234::/64"}, + expectedIfNotLocalOutput: []string{"!", "-s", "2002:0:0:1234::/64"}, }, } for _, c := range cases { diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index 537e77570ae..a589171adf4 100644 --- a/pkg/proxy/util/utils_test.go +++ b/pkg/proxy/util/utils_test.go @@ -372,58 +372,58 @@ func TestMapCIDRsByIPFamily(t *testing.T) { }, { desc: "want IPv4 and receive IPv6", - ipString: []string{"fd00:20::1/64"}, + ipString: []string{"fd00:20::/64"}, wantIPv6: false, expectCorrect: nil, - expectIncorrect: []string{"fd00:20::1/64"}, + expectIncorrect: []string{"fd00:20::/64"}, }, { desc: "want IPv6 and receive IPv4", - ipString: []string{"192.168.200.2/24"}, + ipString: []string{"192.168.200.0/24"}, wantIPv6: true, expectCorrect: nil, - expectIncorrect: []string{"192.168.200.2/24"}, + expectIncorrect: []string{"192.168.200.0/24"}, }, { desc: "want IPv6 and receive IPv4 and IPv6", - ipString: []string{"192.168.200.2/24", "192.1.34.23/24", "fd00:20::1/64", "2001:db9::3/64"}, + ipString: []string{"192.168.200.0/24", "192.1.34.0/24", "fd00:20::/64", "2001:db9::/64"}, wantIPv6: true, - expectCorrect: []string{"fd00:20::1/64", "2001:db9::3/64"}, - expectIncorrect: []string{"192.168.200.2/24", "192.1.34.23/24"}, + expectCorrect: []string{"fd00:20::/64", "2001:db9::/64"}, + expectIncorrect: []string{"192.168.200.0/24", "192.1.34.0/24"}, }, { desc: "want IPv4 and receive IPv4 and IPv6", - ipString: []string{"192.168.200.2/24", "192.1.34.23/24", "fd00:20::1/64", "2001:db9::3/64"}, + ipString: []string{"192.168.200.0/24", "192.1.34.0/24", "fd00:20::/64", "2001:db9::/64"}, wantIPv6: false, - expectCorrect: []string{"192.168.200.2/24", "192.1.34.23/24"}, - expectIncorrect: []string{"fd00:20::1/64", "2001:db9::3/64"}, + expectCorrect: []string{"192.168.200.0/24", "192.1.34.0/24"}, + expectIncorrect: []string{"fd00:20::/64", "2001:db9::/64"}, }, { desc: "want IPv4 and receive IPv4 only", - ipString: []string{"192.168.200.2/24", "192.1.34.23/24"}, + ipString: []string{"192.168.200.0/24", "192.1.34.0/24"}, wantIPv6: false, - expectCorrect: []string{"192.168.200.2/24", "192.1.34.23/24"}, + expectCorrect: []string{"192.168.200.0/24", "192.1.34.0/24"}, expectIncorrect: nil, }, { desc: "want IPv6 and receive IPv4 only", - ipString: []string{"192.168.200.2/24", "192.1.34.23/24"}, + ipString: []string{"192.168.200.0/24", "192.1.34.0/24"}, wantIPv6: true, expectCorrect: nil, - expectIncorrect: []string{"192.168.200.2/24", "192.1.34.23/24"}, + expectIncorrect: []string{"192.168.200.0/24", "192.1.34.0/24"}, }, { desc: "want IPv4 and receive IPv6 only", - ipString: []string{"fd00:20::1/64", "2001:db9::3/64"}, + ipString: []string{"fd00:20::/64", "2001:db9::/64"}, wantIPv6: false, expectCorrect: nil, - expectIncorrect: []string{"fd00:20::1/64", "2001:db9::3/64"}, + expectIncorrect: []string{"fd00:20::/64", "2001:db9::/64"}, }, { desc: "want IPv6 and receive IPv6 only", - ipString: []string{"fd00:20::1/64", "2001:db9::3/64"}, + ipString: []string{"fd00:20::/64", "2001:db9::/64"}, wantIPv6: true, - expectCorrect: []string{"fd00:20::1/64", "2001:db9::3/64"}, + expectCorrect: []string{"fd00:20::/64", "2001:db9::/64"}, expectIncorrect: nil, }, } From d2294007b0fb38c7587caa87bdee085dd157158e Mon Sep 17 00:00:00 2001 From: Lars Ekman Date: Sun, 7 Jan 2024 08:33:30 +0100 Subject: [PATCH 2/4] kube-proxy: store LoadBalancerVIPs as net.IP They were stored as strings which could be non-canonical and cause problems --- pkg/proxy/conntrack/cleanup.go | 8 +++--- pkg/proxy/iptables/proxier.go | 16 ++++++------ pkg/proxy/ipvs/proxier.go | 12 ++++----- pkg/proxy/nftables/proxier.go | 16 ++++++------ pkg/proxy/nftables/proxier_test.go | 2 +- pkg/proxy/servicechangetracker_test.go | 36 ++++++++++++++++---------- pkg/proxy/serviceport.go | 21 ++++++++------- pkg/proxy/util/utils.go | 14 +++++----- 8 files changed, 67 insertions(+), 58 deletions(-) diff --git a/pkg/proxy/conntrack/cleanup.go b/pkg/proxy/conntrack/cleanup.go index f99ab89770a..6abd7b40c92 100644 --- a/pkg/proxy/conntrack/cleanup.go +++ b/pkg/proxy/conntrack/cleanup.go @@ -53,8 +53,8 @@ func deleteStaleServiceConntrackEntries(isIPv6 bool, exec utilexec.Interface, sv for _, extIP := range svcInfo.ExternalIPStrings() { conntrackCleanupServiceIPs.Insert(extIP) } - for _, lbIP := range svcInfo.LoadBalancerVIPStrings() { - conntrackCleanupServiceIPs.Insert(lbIP) + for _, lbIP := range svcInfo.LoadBalancerVIPs() { + conntrackCleanupServiceIPs.Insert(lbIP.String()) } nodePort := svcInfo.NodePort() if svcInfo.Protocol() == v1.ProtocolUDP && nodePort != 0 { @@ -103,8 +103,8 @@ func deleteStaleEndpointConntrackEntries(exec utilexec.Interface, svcPortMap pro klog.ErrorS(err, "Failed to delete endpoint connections for externalIP", "servicePortName", epSvcPair.ServicePortName, "externalIP", extIP) } } - for _, lbIP := range svcInfo.LoadBalancerVIPStrings() { - err := ClearEntriesForNAT(exec, lbIP, endpointIP, v1.ProtocolUDP) + for _, lbIP := range svcInfo.LoadBalancerVIPs() { + err := ClearEntriesForNAT(exec, lbIP.String(), endpointIP, v1.ProtocolUDP) if err != nil { klog.ErrorS(err, "Failed to delete endpoint connections for LoadBalancerIP", "servicePortName", epSvcPair.ServicePortName, "loadBalancerIP", lbIP) } diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index ffaa2ed88f1..e6ebdfb9658 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -1014,7 +1014,7 @@ func (proxier *Proxier) syncProxyRules() { // create a firewall chain. loadBalancerTrafficChain := externalTrafficChain fwChain := svcInfo.firewallChainName - usesFWChain := hasEndpoints && len(svcInfo.LoadBalancerVIPStrings()) > 0 && len(svcInfo.LoadBalancerSourceRanges()) > 0 + usesFWChain := hasEndpoints && len(svcInfo.LoadBalancerVIPs()) > 0 && len(svcInfo.LoadBalancerSourceRanges()) > 0 if usesFWChain { loadBalancerTrafficChain = fwChain } @@ -1105,13 +1105,13 @@ func (proxier *Proxier) syncProxyRules() { } // Capture load-balancer ingress. - for _, lbip := range svcInfo.LoadBalancerVIPStrings() { + for _, lbip := range svcInfo.LoadBalancerVIPs() { if hasEndpoints { natRules.Write( "-A", string(kubeServicesChain), "-m", "comment", "--comment", fmt.Sprintf(`"%s loadbalancer IP"`, svcPortNameString), "-m", protocol, "-p", protocol, - "-d", lbip, + "-d", lbip.String(), "--dport", strconv.Itoa(svcInfo.Port()), "-j", string(loadBalancerTrafficChain)) @@ -1121,7 +1121,7 @@ func (proxier *Proxier) syncProxyRules() { "-A", string(kubeProxyFirewallChain), "-m", "comment", "--comment", fmt.Sprintf(`"%s traffic not accepted by %s"`, svcPortNameString, svcInfo.firewallChainName), "-m", protocol, "-p", protocol, - "-d", lbip, + "-d", lbip.String(), "--dport", strconv.Itoa(svcInfo.Port()), "-j", "DROP") } @@ -1130,12 +1130,12 @@ func (proxier *Proxier) syncProxyRules() { // Either no endpoints at all (REJECT) or no endpoints for // external traffic (DROP anything that didn't get short-circuited // by the EXT chain.) - for _, lbip := range svcInfo.LoadBalancerVIPStrings() { + for _, lbip := range svcInfo.LoadBalancerVIPs() { filterRules.Write( "-A", string(kubeExternalServicesChain), "-m", "comment", "--comment", externalTrafficFilterComment, "-m", protocol, "-p", protocol, - "-d", lbip, + "-d", lbip.String(), "--dport", strconv.Itoa(svcInfo.Port()), "-j", externalTrafficFilterTarget, ) @@ -1309,10 +1309,10 @@ func (proxier *Proxier) syncProxyRules() { // will loop back with the source IP set to the VIP. We // need the following rules to allow requests from this node. if allowFromNode { - for _, lbip := range svcInfo.LoadBalancerVIPStrings() { + for _, lbip := range svcInfo.LoadBalancerVIPs() { natRules.Write( args, - "-s", lbip, + "-s", lbip.String(), "-j", string(externalTrafficChain)) } } diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index f3f5f8813fa..2e587675b6e 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1152,10 +1152,10 @@ func (proxier *Proxier) syncProxyRules() { } // Capture load-balancer ingress. - for _, ingress := range svcInfo.LoadBalancerVIPStrings() { + for _, ingress := range svcInfo.LoadBalancerVIPs() { // ipset call entry = &utilipset.Entry{ - IP: ingress, + IP: ingress.String(), Port: svcInfo.Port(), Protocol: protocol, SetType: utilipset.HashIPPort, @@ -1190,7 +1190,7 @@ func (proxier *Proxier) syncProxyRules() { for _, src := range svcInfo.LoadBalancerSourceRanges() { // ipset call entry = &utilipset.Entry{ - IP: ingress, + IP: ingress.String(), Port: svcInfo.Port(), Protocol: protocol, Net: src, @@ -1214,10 +1214,10 @@ func (proxier *Proxier) syncProxyRules() { // Need to add the following rule to allow request on host. if allowFromNode { entry = &utilipset.Entry{ - IP: ingress, + IP: ingress.String(), Port: svcInfo.Port(), Protocol: protocol, - IP2: ingress, + IP2: ingress.String(), SetType: utilipset.HashIPPortIP, } // enumerate all white list source ip @@ -1234,7 +1234,7 @@ func (proxier *Proxier) syncProxyRules() { } // ipvs call serv := &utilipvs.VirtualServer{ - Address: netutils.ParseIPSloppy(ingress), + Address: ingress, 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 d6281089007..dc9f3948930 100644 --- a/pkg/proxy/nftables/proxier.go +++ b/pkg/proxy/nftables/proxier.go @@ -1188,7 +1188,7 @@ func (proxier *Proxier) syncProxyRules() { } } - usesFWChain := len(svcInfo.LoadBalancerVIPStrings()) > 0 && len(svcInfo.LoadBalancerSourceRanges()) > 0 + usesFWChain := len(svcInfo.LoadBalancerVIPs()) > 0 && len(svcInfo.LoadBalancerSourceRanges()) > 0 fwChain := svcInfo.firewallChainName if usesFWChain { ensureChain(fwChain, tx, activeChains) @@ -1213,8 +1213,8 @@ func (proxier *Proxier) syncProxyRules() { // will loop back with the source IP set to the VIP. We // need the following rules to allow requests from this node. if allowFromNode { - for _, lbip := range svcInfo.LoadBalancerVIPStrings() { - sources = append(sources, ",", lbip) + for _, lbip := range svcInfo.LoadBalancerVIPs() { + sources = append(sources, ",", lbip.String()) } } tx.Add(&knftables.Rule{ @@ -1227,12 +1227,12 @@ func (proxier *Proxier) syncProxyRules() { } // Capture load-balancer ingress. - for _, lbip := range svcInfo.LoadBalancerVIPStrings() { + for _, lbip := range svcInfo.LoadBalancerVIPs() { if hasEndpoints { tx.Add(&knftables.Element{ Map: kubeServiceIPsMap, Key: []string{ - lbip, + lbip.String(), protocol, strconv.Itoa(svcInfo.Port()), }, @@ -1246,7 +1246,7 @@ func (proxier *Proxier) syncProxyRules() { tx.Add(&knftables.Element{ Map: kubeFirewallIPsMap, Key: []string{ - lbip, + lbip.String(), protocol, strconv.Itoa(svcInfo.Port()), }, @@ -1261,11 +1261,11 @@ func (proxier *Proxier) syncProxyRules() { // Either no endpoints at all (REJECT) or no endpoints for // external traffic (DROP anything that didn't get short-circuited // by the EXT chain.) - for _, lbip := range svcInfo.LoadBalancerVIPStrings() { + for _, lbip := range svcInfo.LoadBalancerVIPs() { tx.Add(&knftables.Element{ Map: kubeNoEndpointServicesMap, Key: []string{ - lbip, + lbip.String(), protocol, strconv.Itoa(svcInfo.Port()), }, diff --git a/pkg/proxy/nftables/proxier_test.go b/pkg/proxy/nftables/proxier_test.go index e67c89a9a7a..9637227bc7e 100644 --- a/pkg/proxy/nftables/proxier_test.go +++ b/pkg/proxy/nftables/proxier_test.go @@ -175,7 +175,7 @@ func TestDeleteEndpointConnections(t *testing.T) { } endpointIP := proxyutil.IPPart(tc.endpoint) - _, fp := NewFakeProxier(proxyutil.GetIPFamilyFromIP(endpointIP)) + _, fp := NewFakeProxier(proxyutil.GetIPFamilyFromIP(netutils.ParseIPSloppy(endpointIP))) fp.exec = fexec makeServiceMap(fp, diff --git a/pkg/proxy/servicechangetracker_test.go b/pkg/proxy/servicechangetracker_test.go index 864526aaea3..0e7965c6aa5 100644 --- a/pkg/proxy/servicechangetracker_test.go +++ b/pkg/proxy/servicechangetracker_test.go @@ -17,6 +17,7 @@ limitations under the License. package proxy import ( + "net" "reflect" "testing" "time" @@ -86,6 +87,13 @@ func makeServicePortName(ns, name, port string, protocol v1.Protocol) ServicePor Protocol: protocol, } } +func makeIPs(ipStr ...string) []net.IP { + var ips []net.IP + for _, s := range ipStr { + ips = append(ips, netutils.ParseIPSloppy(s)) + } + return ips +} func TestServiceToServiceMap(t *testing.T) { testClusterIPv4 := "10.0.0.1" @@ -187,10 +195,10 @@ func TestServiceToServiceMap(t *testing.T) { }), expected: map[ServicePortName]*BaseServicePortInfo{ makeServicePortName("ns1", "load-balancer", "port3", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8675, "UDP", 0, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.4"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.4") }), makeServicePortName("ns1", "load-balancer", "port4", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8676, "UDP", 0, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.4"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.4") }), }, }, @@ -208,10 +216,10 @@ func TestServiceToServiceMap(t *testing.T) { }), expected: map[ServicePortName]*BaseServicePortInfo{ makeServicePortName("ns1", "load-balancer", "port3", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8675, "UDP", 0, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.4"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.4") }), makeServicePortName("ns1", "load-balancer", "port4", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8676, "UDP", 0, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.4"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.4") }), }, }, @@ -229,10 +237,10 @@ func TestServiceToServiceMap(t *testing.T) { }), expected: map[ServicePortName]*BaseServicePortInfo{ makeServicePortName("ns1", "load-balancer", "port3", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8675, "UDP", 0, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.4"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.4") }), makeServicePortName("ns1", "load-balancer", "port4", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8676, "UDP", 0, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.4"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.4") }), }, }, @@ -251,10 +259,10 @@ func TestServiceToServiceMap(t *testing.T) { }), expected: map[ServicePortName]*BaseServicePortInfo{ makeServicePortName("ns1", "load-balancer", "port3", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8675, "UDP", 0, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.4"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.4") }), makeServicePortName("ns1", "load-balancer", "port4", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8676, "UDP", 0, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.4"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.4") }), }, }, @@ -294,10 +302,10 @@ func TestServiceToServiceMap(t *testing.T) { }), expected: map[ServicePortName]*BaseServicePortInfo{ makeServicePortName("ns1", "only-local-load-balancer", "portx", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.12", 8677, "UDP", 345, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.3"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.3") }), makeServicePortName("ns1", "only-local-load-balancer", "porty", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.12", 8678, "UDP", 345, func(bsvcPortInfo *BaseServicePortInfo) { - bsvcPortInfo.loadBalancerVIPs = []string{"10.1.2.3"} + bsvcPortInfo.loadBalancerVIPs = makeIPs("10.1.2.3") }), }, }, @@ -405,7 +413,7 @@ func TestServiceToServiceMap(t *testing.T) { makeServicePortName("test", "validIPv4", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv4, 12345, "TCP", 0, func(bsvcPortInfo *BaseServicePortInfo) { bsvcPortInfo.externalIPs = []string{testExternalIPv4} bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv4} - bsvcPortInfo.loadBalancerVIPs = []string{testExternalIPv4} + bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv4) }), }, }, @@ -443,7 +451,7 @@ func TestServiceToServiceMap(t *testing.T) { makeServicePortName("test", "validIPv6", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv6, 12345, "TCP", 0, func(bsvcPortInfo *BaseServicePortInfo) { bsvcPortInfo.externalIPs = []string{testExternalIPv6} bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv6} - bsvcPortInfo.loadBalancerVIPs = []string{testExternalIPv6} + bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv6) }), }, }, @@ -481,7 +489,7 @@ func TestServiceToServiceMap(t *testing.T) { makeServicePortName("test", "filterIPv6InIPV4Mode", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv4, 12345, "TCP", 0, func(bsvcPortInfo *BaseServicePortInfo) { bsvcPortInfo.externalIPs = []string{testExternalIPv4} bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv4} - bsvcPortInfo.loadBalancerVIPs = []string{testExternalIPv4} + bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv4) }), }, }, @@ -519,7 +527,7 @@ func TestServiceToServiceMap(t *testing.T) { makeServicePortName("test", "filterIPv4InIPV6Mode", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv6, 12345, "TCP", 0, func(bsvcPortInfo *BaseServicePortInfo) { bsvcPortInfo.externalIPs = []string{testExternalIPv6} bsvcPortInfo.loadBalancerSourceRanges = []string{testSourceRangeIPv6} - bsvcPortInfo.loadBalancerVIPs = []string{testExternalIPv6} + bsvcPortInfo.loadBalancerVIPs = makeIPs(testExternalIPv6) }), }, }, diff --git a/pkg/proxy/serviceport.go b/pkg/proxy/serviceport.go index 5ed0b5d79b3..61513afdcb4 100644 --- a/pkg/proxy/serviceport.go +++ b/pkg/proxy/serviceport.go @@ -42,8 +42,8 @@ type ServicePort interface { StickyMaxAgeSeconds() int // ExternalIPStrings returns service ExternalIPs as a string array. ExternalIPStrings() []string - // LoadBalancerVIPStrings returns service LoadBalancerIPs which are VIP mode as a string array. - LoadBalancerVIPStrings() []string + // LoadBalancerVIPs returns service LoadBalancerIPs which are VIP mode + LoadBalancerVIPs() []net.IP // Protocol returns service protocol. Protocol() v1.Protocol // LoadBalancerSourceRanges returns service LoadBalancerSourceRanges if present empty array if not @@ -78,7 +78,7 @@ type BaseServicePortInfo struct { port int protocol v1.Protocol nodePort int - loadBalancerVIPs []string + loadBalancerVIPs []net.IP sessionAffinityType v1.ServiceAffinity stickyMaxAgeSeconds int externalIPs []string @@ -141,8 +141,8 @@ func (bsvcPortInfo *BaseServicePortInfo) ExternalIPStrings() []string { return bsvcPortInfo.externalIPs } -// LoadBalancerVIPStrings is part of ServicePort interface. -func (bsvcPortInfo *BaseServicePortInfo) LoadBalancerVIPStrings() []string { +// LoadBalancerVIPs is part of ServicePort interface. +func (bsvcPortInfo *BaseServicePortInfo) LoadBalancerVIPs() []net.IP { return bsvcPortInfo.loadBalancerVIPs } @@ -235,7 +235,7 @@ func newBaseServiceInfo(service *v1.Service, ipFamily v1.IPFamily, port *v1.Serv } // Obtain Load Balancer Ingress - var invalidIPs []string + var invalidIPs []net.IP for _, ing := range service.Status.LoadBalancer.Ingress { if ing.IP == "" { continue @@ -252,15 +252,16 @@ func newBaseServiceInfo(service *v1.Service, ipFamily v1.IPFamily, port *v1.Serv // kube-proxy does not implement IP family translation, skip addresses with // different IP family - if ingFamily := proxyutil.GetIPFamilyFromIP(ing.IP); ingFamily == ipFamily { - info.loadBalancerVIPs = append(info.loadBalancerVIPs, ing.IP) + ip := netutils.ParseIPSloppy(ing.IP) // (already verified as an IP-address) + if ingFamily := proxyutil.GetIPFamilyFromIP(ip); ingFamily == ipFamily { + info.loadBalancerVIPs = append(info.loadBalancerVIPs, ip) } else { - invalidIPs = append(invalidIPs, ing.IP) + invalidIPs = append(invalidIPs, ip) } } if len(invalidIPs) > 0 { klog.V(4).InfoS("Service change tracker ignored the following load balancer ingress IPs for given Service as they don't match the IP Family", - "ipFamily", ipFamily, "loadBalancerIngressIPs", strings.Join(invalidIPs, ", "), "service", klog.KObj(service)) + "ipFamily", ipFamily, "loadBalancerIngressIPs", invalidIPs, "service", klog.KObj(service)) } if apiservice.NeedsHealthCheck(service) { diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go index 807bced0b56..5d6317c3e14 100644 --- a/pkg/proxy/util/utils.go +++ b/pkg/proxy/util/utils.go @@ -182,19 +182,19 @@ 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{} - for _, ip := range ipStrings { + 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], ip) + ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], ipStr) } 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 - if len(strings.TrimSpace(ip)) != 0 { - klog.ErrorS(nil, "Skipping invalid IP", "ip", ip) - + if len(strings.TrimSpace(ipStr)) != 0 { + klog.ErrorS(nil, "Skipping invalid IP", "ip", ipStr) } } } @@ -216,8 +216,8 @@ func MapCIDRsByIPFamily(cidrStrings []string) map[v1.IPFamily][]string { } // GetIPFamilyFromIP Returns the IP family of ipStr, or IPFamilyUnknown if ipStr can't be parsed as an IP -func GetIPFamilyFromIP(ipStr string) v1.IPFamily { - return convertToV1IPFamily(netutils.IPFamilyOfString(ipStr)) +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 From 9eac24c656322cc9e07fcd4c48f88743c7604bd5 Mon Sep 17 00:00:00 2001 From: Lars Ekman Date: Sun, 7 Jan 2024 10:48:39 +0100 Subject: [PATCH 3/4] 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 | 8 ++++---- pkg/proxy/iptables/proxier.go | 6 +++--- pkg/proxy/ipvs/proxier.go | 6 +++--- pkg/proxy/nftables/proxier.go | 6 +++--- pkg/proxy/servicechangetracker_test.go | 12 ++++++------ pkg/proxy/serviceport.go | 19 +++++++++---------- pkg/proxy/util/utils.go | 19 +++++++++---------- pkg/proxy/util/utils_test.go | 12 ++++++++++-- 8 files changed, 47 insertions(+), 41 deletions(-) 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]) } }) From 50b3ffc71fded939aa086b33e2d4a0aadb956db1 Mon Sep 17 00:00:00 2001 From: Lars Ekman Date: Tue, 9 Jan 2024 09:08:30 +0100 Subject: [PATCH 4/4] kube-proxy: LoadBalancerSourceRanges as *net.IPNet --- cmd/kube-proxy/app/server_linux.go | 4 ++-- pkg/proxy/iptables/proxier.go | 10 +++------ pkg/proxy/ipvs/proxier.go | 6 ++---- pkg/proxy/nftables/proxier.go | 9 ++------ pkg/proxy/servicechangetracker_test.go | 26 ++++++++++++++++------- pkg/proxy/serviceport.go | 15 +++++-------- pkg/proxy/util/utils.go | 29 +++++++++++++------------- pkg/proxy/util/utils_test.go | 17 +++++++++++---- 8 files changed, 60 insertions(+), 56 deletions(-) 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) } }) }