From 8fb895f3f1de00c577ad9fd36b0fa28cdd1fe540 Mon Sep 17 00:00:00 2001 From: Basant Amarkhed Date: Fri, 13 Nov 2020 23:17:30 +0000 Subject: [PATCH 1/5] Updating after merging with a conflicting commit --- pkg/proxy/iptables/proxier.go | 19 +++++++++++------- pkg/proxy/ipvs/proxier.go | 19 +++++++++++------- pkg/proxy/service.go | 35 ++++++++++++++++++++++---------- pkg/proxy/service_test.go | 4 +++- pkg/proxy/util/utils.go | 38 +++++++++++++++++------------------ pkg/proxy/util/utils_test.go | 18 +++++++++++------ 6 files changed, 82 insertions(+), 51 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 50d13c57308..6e455c49ddd 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -294,10 +294,14 @@ func NewProxier(ipt utiliptables.Interface, ipFamily = v1.IPv6Protocol } - var incorrectAddresses []string - nodePortAddresses, incorrectAddresses = utilproxy.FilterIncorrectCIDRVersion(nodePortAddresses, ipFamily) - if len(incorrectAddresses) > 0 { - klog.Warningf("NodePortAddresses of wrong family; %s", incorrectAddresses) + isCIDR := true + ipFamilyIPMap := utilproxy.MapIPsToIPFamily(nodePortAddresses, isCIDR) + nodePortAddresses = ipFamilyIPMap[ipFamily] + for ipFam, ips := range ipFamilyIPMap { + // Log the IPs not matching the ipFamily + if ipFam != ipFamily && len(ips) > 0 { + klog.Warningf("IP Family: %s, NodePortAddresses of wrong family; %s", ipFamily, strings.Join(ips, ",")) + } } proxier := &Proxier{ @@ -367,17 +371,18 @@ func NewDualStackProxier( nodePortAddresses []string, ) (proxy.Provider, error) { // Create an ipv4 instance of the single-stack proxier - nodePortAddresses4, nodePortAddresses6 := utilproxy.FilterIncorrectCIDRVersion(nodePortAddresses, v1.IPv4Protocol) + isCIDR := true + ipFamilyIPMap := utilproxy.MapIPsToIPFamily(nodePortAddresses, isCIDR) ipv4Proxier, err := NewProxier(ipt[0], sysctl, exec, syncPeriod, minSyncPeriod, masqueradeAll, masqueradeBit, localDetectors[0], hostname, - nodeIP[0], recorder, healthzServer, nodePortAddresses4) + nodeIP[0], recorder, healthzServer, ipFamilyIPMap[v1.IPv4Protocol]) if err != nil { return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err) } ipv6Proxier, err := NewProxier(ipt[1], sysctl, exec, syncPeriod, minSyncPeriod, masqueradeAll, masqueradeBit, localDetectors[1], hostname, - nodeIP[1], recorder, healthzServer, nodePortAddresses6) + nodeIP[1], recorder, healthzServer, ipFamilyIPMap[v1.IPv6Protocol]) if err != nil { return nil, fmt.Errorf("unable to create ipv6 proxier: %v", err) } diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 7d67008332c..8062619a9ee 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -450,10 +450,14 @@ func NewProxier(ipt utiliptables.Interface, endpointSlicesEnabled := utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceProxying) - var incorrectAddresses []string - nodePortAddresses, incorrectAddresses = utilproxy.FilterIncorrectCIDRVersion(nodePortAddresses, ipFamily) - if len(incorrectAddresses) > 0 { - klog.Warningf("NodePortAddresses of wrong family; %s", incorrectAddresses) + isCIDR := true + ipFamilyIPMap := utilproxy.MapIPsToIPFamily(nodePortAddresses, isCIDR) + nodePortAddresses = ipFamilyIPMap[ipFamily] + for ipFam, ips := range ipFamilyIPMap { + // Log the IPs not matching the ipFamily + if ipFam != ipFamily && len(ips) > 0 { + klog.Warningf("IP Family: %s, NodePortAddresses of wrong family; %s", ipFamily, strings.Join(ips, ",")) + } } proxier := &Proxier{ ipFamily: ipFamily, @@ -532,14 +536,15 @@ func NewDualStackProxier( safeIpset := newSafeIpset(ipset) - nodePortAddresses4, nodePortAddresses6 := utilproxy.FilterIncorrectCIDRVersion(nodePortAddresses, v1.IPv4Protocol) + isCIDR := true + ipFamilyIPMap := utilproxy.MapIPsToIPFamily(nodePortAddresses, isCIDR) // Create an ipv4 instance of the single-stack proxier ipv4Proxier, err := NewProxier(ipt[0], ipvs, safeIpset, sysctl, exec, syncPeriod, minSyncPeriod, filterCIDRs(false, excludeCIDRs), strictARP, tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit, localDetectors[0], hostname, nodeIP[0], - recorder, healthzServer, scheduler, nodePortAddresses4, kernelHandler) + recorder, healthzServer, scheduler, ipFamilyIPMap[v1.IPv4Protocol], kernelHandler) if err != nil { return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err) } @@ -548,7 +553,7 @@ func NewDualStackProxier( exec, syncPeriod, minSyncPeriod, filterCIDRs(true, excludeCIDRs), strictARP, tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit, localDetectors[1], hostname, nodeIP[1], - nil, nil, scheduler, nodePortAddresses6, kernelHandler) + nil, nil, scheduler, ipFamilyIPMap[v1.IPv6Protocol], kernelHandler) if err != nil { return nil, fmt.Errorf("unable to create ipv6 proxier: %v", err) } diff --git a/pkg/proxy/service.go b/pkg/proxy/service.go index a00b2ddab1c..5d1392f6af5 100644 --- a/pkg/proxy/service.go +++ b/pkg/proxy/service.go @@ -156,15 +156,25 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic // services, this is actually expected. Hence we downgraded from reporting by events // to just log lines with high verbosity - var incorrectIPs []string - info.externalIPs, incorrectIPs = utilproxy.FilterIncorrectIPVersion(service.Spec.ExternalIPs, sct.ipFamily) - if len(incorrectIPs) > 0 { - klog.V(4).Infof("service change tracker(%v) ignored the following external IPs(%s) for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(incorrectIPs, ","), service.Namespace, service.Name) + isCIDR := false + ipFamilyIPMap := utilproxy.MapIPsToIPFamily(service.Spec.ExternalIPs, isCIDR) + info.externalIPs = ipFamilyIPMap[sct.ipFamily] + + for ipFam, ips := range ipFamilyIPMap { + // Log the IPs not matching the ipFamily + if ipFam != sct.ipFamily && len(ips) > 0 { + klog.V(4).Infof("service change tracker(%v) ignored the following external IPs(%s) for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(ips, ","), service.Namespace, service.Name) + } } - info.loadBalancerSourceRanges, incorrectIPs = utilproxy.FilterIncorrectCIDRVersion(loadBalancerSourceRanges, sct.ipFamily) - if len(incorrectIPs) > 0 { - klog.V(4).Infof("service change tracker(%v) ignored the following load balancer source ranges(%s) for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(incorrectIPs, ","), service.Namespace, service.Name) + isCIDR = true + ipFamilyCIDRMap := utilproxy.MapIPsToIPFamily(loadBalancerSourceRanges, isCIDR) + info.loadBalancerSourceRanges = ipFamilyCIDRMap[sct.ipFamily] + for ipFam, cidrs := range ipFamilyCIDRMap { + // Log the CIDRs not matching the ipFamily + if ipFam != sct.ipFamily && len(cidrs) > 0 { + klog.V(4).Infof("service change tracker(%v) ignored the following load balancer source ranges(%s) for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(cidrs, ","), service.Namespace, service.Name) + } } // Obtain Load Balancer Ingress IPs @@ -174,14 +184,17 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic } if len(ips) > 0 { - correctIPs, incorrectIPs := utilproxy.FilterIncorrectIPVersion(ips, sct.ipFamily) + isCIDR := false + ipFamilyIPMap := utilproxy.MapIPsToIPFamily(ips, isCIDR) - if len(incorrectIPs) > 0 { - klog.V(4).Infof("service change tracker(%v) ignored the following load balancer(%s) ingress ips for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(incorrectIPs, ","), service.Namespace, service.Name) + for ipFam, ipList := range ipFamilyIPMap { + if ipFam != sct.ipFamily && len(ipList) > 0 { + klog.V(4).Infof("service change tracker(%v) ignored the following load balancer(%s) ingress ips for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(ipList, ","), service.Namespace, service.Name) + } } // Create the LoadBalancerStatus with the filtered IPs - for _, ip := range correctIPs { + for _, ip := range ipFamilyIPMap[sct.ipFamily] { info.loadBalancerStatus.Ingress = append(info.loadBalancerStatus.Ingress, v1.LoadBalancerIngress{IP: ip}) } } diff --git a/pkg/proxy/service_test.go b/pkg/proxy/service_test.go index a690608a885..0329b8ace31 100644 --- a/pkg/proxy/service_test.go +++ b/pkg/proxy/service_test.go @@ -434,7 +434,9 @@ func TestServiceToServiceMap(t *testing.T) { }, }, { - desc: "service with extra space in LoadBalancerSourceRanges", + desc: "service with extra space in LoadBalancerSourceRanges", + ipFamily: v1.IPv4Protocol, + service: &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: "extra-space", diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go index 2031cd1a09e..e3cd9e8e048 100644 --- a/pkg/proxy/util/utils.go +++ b/pkg/proxy/util/utils.go @@ -255,26 +255,26 @@ func LogAndEmitIncorrectIPVersionEvent(recorder record.EventRecorder, fieldName, } } -// FilterIncorrectIPVersion filters out the incorrect IP version case from a slice of IP strings. -func FilterIncorrectIPVersion(ipStrings []string, ipfamily v1.IPFamily) ([]string, []string) { - return filterWithCondition(ipStrings, (ipfamily == v1.IPv6Protocol), utilnet.IsIPv6String) -} - -// FilterIncorrectCIDRVersion filters out the incorrect IP version case from a slice of CIDR strings. -func FilterIncorrectCIDRVersion(ipStrings []string, ipfamily v1.IPFamily) ([]string, []string) { - return filterWithCondition(ipStrings, (ipfamily == v1.IPv6Protocol), utilnet.IsIPv6CIDRString) -} - -func filterWithCondition(strs []string, expectedCondition bool, conditionFunc func(string) bool) ([]string, []string) { - var corrects, incorrects []string - for _, str := range strs { - if conditionFunc(str) != expectedCondition { - incorrects = append(incorrects, str) - } else { - corrects = append(corrects, str) - } +// MapIPsToIPFamily maps a slice of IPs to their respective IP families (v4 or v6) +func MapIPsToIPFamily(ipStrings []string, isCIDR bool) map[v1.IPFamily][]string { + ipFamilyMap := map[v1.IPFamily][]string{} + for _, ip := range ipStrings { + ipFamily := getIPFamilyFromIP(ip, isCIDR) + ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], ip) } - return corrects, incorrects + return ipFamilyMap +} + +func getIPFamilyFromIP(ip string, isCIDR bool) v1.IPFamily { + conditionFunc := utilnet.IsIPv6String + if isCIDR { + conditionFunc = utilnet.IsIPv6CIDRString + } + + if conditionFunc(ip) { + return v1.IPv6Protocol + } + return v1.IPv4Protocol } // AppendPortIfNeeded appends the given port to IP address unless it is already in diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index f10a95c88b3..e545d74687d 100644 --- a/pkg/proxy/util/utils_test.go +++ b/pkg/proxy/util/utils_test.go @@ -567,7 +567,7 @@ func TestShuffleStrings(t *testing.T) { } } -func TestFilterIncorrectIPVersion(t *testing.T) { +func TestMapIPsToIPFamily(t *testing.T) { testCases := []struct { desc string ipString []string @@ -650,15 +650,21 @@ func TestFilterIncorrectIPVersion(t *testing.T) { for _, testcase := range testCases { t.Run(testcase.desc, func(t *testing.T) { ipFamily := v1.IPv4Protocol + otherIPFamily := v1.IPv6Protocol + if testcase.wantIPv6 { ipFamily = v1.IPv6Protocol + otherIPFamily = v1.IPv4Protocol } - correct, incorrect := FilterIncorrectIPVersion(testcase.ipString, ipFamily) - if !reflect.DeepEqual(testcase.expectCorrect, correct) { - t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectCorrect, correct) + + isCIDR := false + ipMap := MapIPsToIPFamily(testcase.ipString, isCIDR) + + if !reflect.DeepEqual(testcase.expectCorrect, ipMap[ipFamily]) { + t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectCorrect, ipMap[ipFamily]) } - if !reflect.DeepEqual(testcase.expectIncorrect, incorrect) { - t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectIncorrect, incorrect) + if !reflect.DeepEqual(testcase.expectIncorrect, ipMap[otherIPFamily]) { + t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectIncorrect, ipMap[otherIPFamily]) } }) } From 09d966c8cc96b64ceb6e6f9c16e5338f7f42bb3a Mon Sep 17 00:00:00 2001 From: Basant Amarkhed Date: Fri, 13 Nov 2020 23:19:17 +0000 Subject: [PATCH 2/5] Adding service.go changes after merge --- pkg/proxy/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/proxy/service.go b/pkg/proxy/service.go index 5d1392f6af5..4615406f637 100644 --- a/pkg/proxy/service.go +++ b/pkg/proxy/service.go @@ -184,8 +184,8 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic } if len(ips) > 0 { - isCIDR := false - ipFamilyIPMap := utilproxy.MapIPsToIPFamily(ips, isCIDR) + isCIDR = false + ipFamilyIPMap = utilproxy.MapIPsToIPFamily(ips, isCIDR) for ipFam, ipList := range ipFamilyIPMap { if ipFam != sct.ipFamily && len(ipList) > 0 { From 707073d2f9e39923413d7f8fe5959dba3b6e08eb Mon Sep 17 00:00:00 2001 From: Basant Amarkhed Date: Tue, 17 Nov 2020 07:13:51 +0000 Subject: [PATCH 3/5] Fixup #1 addressing review comments --- pkg/proxy/iptables/proxier.go | 20 +++++++--------- pkg/proxy/ipvs/proxier.go | 21 +++++++--------- pkg/proxy/service.go | 37 +++++++++++----------------- pkg/proxy/util/utils.go | 45 ++++++++++++++++++++++++++++------- pkg/proxy/util/utils_test.go | 5 ++-- 5 files changed, 69 insertions(+), 59 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 6e455c49ddd..6de792418f1 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -294,14 +294,11 @@ func NewProxier(ipt utiliptables.Interface, ipFamily = v1.IPv6Protocol } - isCIDR := true - ipFamilyIPMap := utilproxy.MapIPsToIPFamily(nodePortAddresses, isCIDR) - nodePortAddresses = ipFamilyIPMap[ipFamily] - for ipFam, ips := range ipFamilyIPMap { - // Log the IPs not matching the ipFamily - if ipFam != ipFamily && len(ips) > 0 { - klog.Warningf("IP Family: %s, NodePortAddresses of wrong family; %s", ipFamily, strings.Join(ips, ",")) - } + ipFamilyMap := utilproxy.MapCIDRsByIPFamily(nodePortAddresses) + nodePortAddresses = ipFamilyMap[ipFamily] + // Log the IPs not matching the ipFamily + if ips, ok := ipFamilyMap[utilproxy.OtherIPFamily(ipFamily)]; ok && len(ips) > 0 { + klog.Warningf("IP Family: %s, NodePortAddresses of wrong family; %s", ipFamily, strings.Join(ips, ",")) } proxier := &Proxier{ @@ -371,18 +368,17 @@ func NewDualStackProxier( nodePortAddresses []string, ) (proxy.Provider, error) { // Create an ipv4 instance of the single-stack proxier - isCIDR := true - ipFamilyIPMap := utilproxy.MapIPsToIPFamily(nodePortAddresses, isCIDR) + ipFamilyMap := utilproxy.MapCIDRsByIPFamily(nodePortAddresses) ipv4Proxier, err := NewProxier(ipt[0], sysctl, exec, syncPeriod, minSyncPeriod, masqueradeAll, masqueradeBit, localDetectors[0], hostname, - nodeIP[0], recorder, healthzServer, ipFamilyIPMap[v1.IPv4Protocol]) + nodeIP[0], recorder, healthzServer, ipFamilyMap[v1.IPv4Protocol]) if err != nil { return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err) } ipv6Proxier, err := NewProxier(ipt[1], sysctl, exec, syncPeriod, minSyncPeriod, masqueradeAll, masqueradeBit, localDetectors[1], hostname, - nodeIP[1], recorder, healthzServer, ipFamilyIPMap[v1.IPv6Protocol]) + nodeIP[1], recorder, healthzServer, ipFamilyMap[v1.IPv6Protocol]) if err != nil { return nil, fmt.Errorf("unable to create ipv6 proxier: %v", err) } diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 8062619a9ee..d0e527ea712 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -450,15 +450,13 @@ func NewProxier(ipt utiliptables.Interface, endpointSlicesEnabled := utilfeature.DefaultFeatureGate.Enabled(features.EndpointSliceProxying) - isCIDR := true - ipFamilyIPMap := utilproxy.MapIPsToIPFamily(nodePortAddresses, isCIDR) - nodePortAddresses = ipFamilyIPMap[ipFamily] - for ipFam, ips := range ipFamilyIPMap { - // Log the IPs not matching the ipFamily - if ipFam != ipFamily && len(ips) > 0 { - klog.Warningf("IP Family: %s, NodePortAddresses of wrong family; %s", ipFamily, strings.Join(ips, ",")) - } + ipFamilyMap := utilproxy.MapCIDRsByIPFamily(nodePortAddresses) + nodePortAddresses = ipFamilyMap[ipFamily] + // Log the IPs not matching the ipFamily + if ips, ok := ipFamilyMap[utilproxy.OtherIPFamily(ipFamily)]; ok && len(ips) > 0 { + klog.Warningf("IP Family: %s, NodePortAddresses of wrong family; %s", ipFamily, strings.Join(ips, ",")) } + proxier := &Proxier{ ipFamily: ipFamily, portsMap: make(map[utilproxy.LocalPort]utilproxy.Closeable), @@ -536,15 +534,14 @@ func NewDualStackProxier( safeIpset := newSafeIpset(ipset) - isCIDR := true - ipFamilyIPMap := utilproxy.MapIPsToIPFamily(nodePortAddresses, isCIDR) + ipFamilyMap := utilproxy.MapCIDRsByIPFamily(nodePortAddresses) // Create an ipv4 instance of the single-stack proxier ipv4Proxier, err := NewProxier(ipt[0], ipvs, safeIpset, sysctl, exec, syncPeriod, minSyncPeriod, filterCIDRs(false, excludeCIDRs), strictARP, tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit, localDetectors[0], hostname, nodeIP[0], - recorder, healthzServer, scheduler, ipFamilyIPMap[v1.IPv4Protocol], kernelHandler) + recorder, healthzServer, scheduler, ipFamilyMap[v1.IPv4Protocol], kernelHandler) if err != nil { return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err) } @@ -553,7 +550,7 @@ func NewDualStackProxier( exec, syncPeriod, minSyncPeriod, filterCIDRs(true, excludeCIDRs), strictARP, tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit, localDetectors[1], hostname, nodeIP[1], - nil, nil, scheduler, ipFamilyIPMap[v1.IPv6Protocol], kernelHandler) + nil, nil, scheduler, ipFamilyMap[v1.IPv6Protocol], kernelHandler) if err != nil { return nil, fmt.Errorf("unable to create ipv6 proxier: %v", err) } diff --git a/pkg/proxy/service.go b/pkg/proxy/service.go index 4615406f637..84c97124857 100644 --- a/pkg/proxy/service.go +++ b/pkg/proxy/service.go @@ -156,25 +156,19 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic // services, this is actually expected. Hence we downgraded from reporting by events // to just log lines with high verbosity - isCIDR := false - ipFamilyIPMap := utilproxy.MapIPsToIPFamily(service.Spec.ExternalIPs, isCIDR) - info.externalIPs = ipFamilyIPMap[sct.ipFamily] + ipFamilyMap := utilproxy.MapIPsByIPFamily(service.Spec.ExternalIPs) + info.externalIPs = ipFamilyMap[sct.ipFamily] - for ipFam, ips := range ipFamilyIPMap { - // Log the IPs not matching the ipFamily - if ipFam != sct.ipFamily && len(ips) > 0 { - klog.V(4).Infof("service change tracker(%v) ignored the following external IPs(%s) for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(ips, ","), service.Namespace, service.Name) - } + // Log the IPs not matching the ipFamily + if ips, ok := ipFamilyMap[utilproxy.OtherIPFamily(sct.ipFamily)]; ok && len(ips) > 0 { + klog.V(4).Infof("service change tracker(%v) ignored the following external IPs(%s) for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(ips, ","), service.Namespace, service.Name) } - isCIDR = true - ipFamilyCIDRMap := utilproxy.MapIPsToIPFamily(loadBalancerSourceRanges, isCIDR) - info.loadBalancerSourceRanges = ipFamilyCIDRMap[sct.ipFamily] - for ipFam, cidrs := range ipFamilyCIDRMap { - // Log the CIDRs not matching the ipFamily - if ipFam != sct.ipFamily && len(cidrs) > 0 { - klog.V(4).Infof("service change tracker(%v) ignored the following load balancer source ranges(%s) for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(cidrs, ","), service.Namespace, service.Name) - } + ipFamilyMap = utilproxy.MapCIDRsByIPFamily(loadBalancerSourceRanges) + info.loadBalancerSourceRanges = ipFamilyMap[sct.ipFamily] + // Log the CIDRs not matching the ipFamily + if cidrs, ok := ipFamilyMap[utilproxy.OtherIPFamily(sct.ipFamily)]; ok && len(cidrs) > 0 { + klog.V(4).Infof("service change tracker(%v) ignored the following load balancer source ranges(%s) for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(cidrs, ","), service.Namespace, service.Name) } // Obtain Load Balancer Ingress IPs @@ -184,17 +178,14 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic } if len(ips) > 0 { - isCIDR = false - ipFamilyIPMap = utilproxy.MapIPsToIPFamily(ips, isCIDR) + ipFamilyMap = utilproxy.MapIPsByIPFamily(ips) - for ipFam, ipList := range ipFamilyIPMap { - if ipFam != sct.ipFamily && len(ipList) > 0 { - klog.V(4).Infof("service change tracker(%v) ignored the following load balancer(%s) ingress ips for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(ipList, ","), service.Namespace, service.Name) + if ipList, ok := ipFamilyMap[utilproxy.OtherIPFamily(sct.ipFamily)]; ok && len(ipList) > 0 { + klog.V(4).Infof("service change tracker(%v) ignored the following load balancer(%s) ingress ips for service %v/%v as they don't match IPFamily", sct.ipFamily, strings.Join(ipList, ","), service.Namespace, service.Name) - } } // Create the LoadBalancerStatus with the filtered IPs - for _, ip := range ipFamilyIPMap[sct.ipFamily] { + for _, ip := range ipFamilyMap[sct.ipFamily] { info.loadBalancerStatus.Ingress = append(info.loadBalancerStatus.Ingress, v1.LoadBalancerIngress{IP: ip}) } } diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go index e3cd9e8e048..66624579008 100644 --- a/pkg/proxy/util/utils.go +++ b/pkg/proxy/util/utils.go @@ -255,23 +255,50 @@ func LogAndEmitIncorrectIPVersionEvent(recorder record.EventRecorder, fieldName, } } -// MapIPsToIPFamily maps a slice of IPs to their respective IP families (v4 or v6) -func MapIPsToIPFamily(ipStrings []string, isCIDR bool) map[v1.IPFamily][]string { +// 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 { - ipFamily := getIPFamilyFromIP(ip, isCIDR) - ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], ip) + // Handle only the valid IPs + if net.ParseIP(ip) != nil { + ipFamily := getIPFamilyFromIP(ip) + ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], ip) + } } return ipFamilyMap } -func getIPFamilyFromIP(ip string, isCIDR bool) v1.IPFamily { - conditionFunc := utilnet.IsIPv6String - if isCIDR { - conditionFunc = utilnet.IsIPv6CIDRString +// MapCIDRsByIPFamily maps a slice of IPs to their respective IP families (v4 or v6) +func MapCIDRsByIPFamily(ipStrings []string) map[v1.IPFamily][]string { + ipFamilyMap := map[v1.IPFamily][]string{} + for _, cidr := range ipStrings { + // Handle only the valid CIDRs + if _, _, err := net.ParseCIDR(cidr); err == nil { + ipFamily := getIPFamilyFromCIDR(cidr) + ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], cidr) + } + } + return ipFamilyMap +} + +func getIPFamilyFromIP(ip string) v1.IPFamily { + if utilnet.IsIPv6String(ip) { + return v1.IPv6Protocol + } + return v1.IPv4Protocol +} + +// OtherIPFamily returns the other ip family +func OtherIPFamily(ipFamily v1.IPFamily) v1.IPFamily { + if ipFamily == v1.IPv6Protocol { + return v1.IPv4Protocol } - if conditionFunc(ip) { + return v1.IPv6Protocol +} + +func getIPFamilyFromCIDR(ip string) v1.IPFamily { + if utilnet.IsIPv6CIDRString(ip) { return v1.IPv6Protocol } return v1.IPv4Protocol diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index e545d74687d..41be8629758 100644 --- a/pkg/proxy/util/utils_test.go +++ b/pkg/proxy/util/utils_test.go @@ -567,7 +567,7 @@ func TestShuffleStrings(t *testing.T) { } } -func TestMapIPsToIPFamily(t *testing.T) { +func TestMapIPsByIPFamily(t *testing.T) { testCases := []struct { desc string ipString []string @@ -657,8 +657,7 @@ func TestMapIPsToIPFamily(t *testing.T) { otherIPFamily = v1.IPv4Protocol } - isCIDR := false - ipMap := MapIPsToIPFamily(testcase.ipString, isCIDR) + ipMap := MapIPsByIPFamily(testcase.ipString) if !reflect.DeepEqual(testcase.expectCorrect, ipMap[ipFamily]) { t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectCorrect, ipMap[ipFamily]) From f11c4e9c8cb9e9c91ca4964198249cc8e574fa97 Mon Sep 17 00:00:00 2001 From: Basant Amarkhed Date: Tue, 17 Nov 2020 07:35:50 +0000 Subject: [PATCH 4/5] Testcases for MapCIDRsByIPFamily --- pkg/proxy/util/utils_test.go | 102 +++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index 41be8629758..fe3d6680fc1 100644 --- a/pkg/proxy/util/utils_test.go +++ b/pkg/proxy/util/utils_test.go @@ -669,6 +669,108 @@ func TestMapIPsByIPFamily(t *testing.T) { } } +func TestMapCIDRsByIPFamily(t *testing.T) { + testCases := []struct { + desc string + ipString []string + wantIPv6 bool + expectCorrect []string + expectIncorrect []string + }{ + { + desc: "empty input IPv4", + ipString: []string{}, + wantIPv6: false, + expectCorrect: nil, + expectIncorrect: nil, + }, + { + desc: "empty input IPv6", + ipString: []string{}, + wantIPv6: true, + expectCorrect: nil, + expectIncorrect: nil, + }, + { + desc: "want IPv4 and receive IPv6", + ipString: []string{"fd00:20::1/64"}, + wantIPv6: false, + expectCorrect: nil, + expectIncorrect: []string{"fd00:20::1/64"}, + }, + { + desc: "want IPv6 and receive IPv4", + ipString: []string{"192.168.200.2/24"}, + wantIPv6: true, + expectCorrect: nil, + expectIncorrect: []string{"192.168.200.2/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"}, + wantIPv6: true, + expectCorrect: []string{"fd00:20::1/64", "2001:db9::3/64"}, + expectIncorrect: []string{"192.168.200.2/24", "192.1.34.23/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"}, + wantIPv6: false, + expectCorrect: []string{"192.168.200.2/24", "192.1.34.23/24"}, + expectIncorrect: []string{"fd00:20::1/64", "2001:db9::3/64"}, + }, + { + desc: "want IPv4 and receive IPv4 only", + ipString: []string{"192.168.200.2/24", "192.1.34.23/24"}, + wantIPv6: false, + expectCorrect: []string{"192.168.200.2/24", "192.1.34.23/24"}, + expectIncorrect: nil, + }, + { + desc: "want IPv6 and receive IPv4 only", + ipString: []string{"192.168.200.2/24", "192.1.34.23/24"}, + wantIPv6: true, + expectCorrect: nil, + expectIncorrect: []string{"192.168.200.2/24", "192.1.34.23/24"}, + }, + { + desc: "want IPv4 and receive IPv6 only", + ipString: []string{"fd00:20::1/64", "2001:db9::3/64"}, + wantIPv6: false, + expectCorrect: nil, + expectIncorrect: []string{"fd00:20::1/64", "2001:db9::3/64"}, + }, + { + desc: "want IPv6 and receive IPv6 only", + ipString: []string{"fd00:20::1/64", "2001:db9::3/64"}, + wantIPv6: true, + expectCorrect: []string{"fd00:20::1/64", "2001:db9::3/64"}, + expectIncorrect: nil, + }, + } + + for _, testcase := range testCases { + t.Run(testcase.desc, func(t *testing.T) { + ipFamily := v1.IPv4Protocol + otherIPFamily := v1.IPv6Protocol + + if testcase.wantIPv6 { + ipFamily = v1.IPv6Protocol + otherIPFamily = v1.IPv4Protocol + } + + 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]) + } + if !reflect.DeepEqual(testcase.expectIncorrect, cidrMap[otherIPFamily]) { + t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectIncorrect, cidrMap[otherIPFamily]) + } + }) + } +} + func TestGetClusterIPByFamily(t *testing.T) { testCases := []struct { name string From 293d4b7c483d708bfce9a5e530d56dbe18a4ded9 Mon Sep 17 00:00:00 2001 From: Basant Amarkhed Date: Fri, 20 Nov 2020 22:22:55 +0000 Subject: [PATCH 5/5] Avoiding double parsing of ip/cidr strings and logging bad ips/cidrs --- pkg/proxy/util/utils.go | 45 +++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go index 66624579008..56752b3722b 100644 --- a/pkg/proxy/util/utils.go +++ b/pkg/proxy/util/utils.go @@ -260,32 +260,50 @@ func MapIPsByIPFamily(ipStrings []string) map[v1.IPFamily][]string { ipFamilyMap := map[v1.IPFamily][]string{} for _, ip := range ipStrings { // Handle only the valid IPs - if net.ParseIP(ip) != nil { - ipFamily := getIPFamilyFromIP(ip) + if ipFamily, err := getIPFamilyFromIP(ip); err == nil { ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], ip) + } else { + klog.Errorf("Skipping invalid IP: %s", ip) } } return ipFamilyMap } // MapCIDRsByIPFamily maps a slice of IPs to their respective IP families (v4 or v6) -func MapCIDRsByIPFamily(ipStrings []string) map[v1.IPFamily][]string { +func MapCIDRsByIPFamily(cidrStrings []string) map[v1.IPFamily][]string { ipFamilyMap := map[v1.IPFamily][]string{} - for _, cidr := range ipStrings { + for _, cidr := range cidrStrings { // Handle only the valid CIDRs - if _, _, err := net.ParseCIDR(cidr); err == nil { - ipFamily := getIPFamilyFromCIDR(cidr) + if ipFamily, err := getIPFamilyFromCIDR(cidr); err == nil { ipFamilyMap[ipFamily] = append(ipFamilyMap[ipFamily], cidr) + } else { + klog.Errorf("Skipping invalid cidr: %s", cidr) } } return ipFamilyMap } -func getIPFamilyFromIP(ip string) v1.IPFamily { - if utilnet.IsIPv6String(ip) { - return v1.IPv6Protocol +func getIPFamilyFromIP(ipStr string) (v1.IPFamily, error) { + netIP := net.ParseIP(ipStr) + if netIP == nil { + return "", ErrAddressNotAllowed } - return v1.IPv4Protocol + + if utilnet.IsIPv6(netIP) { + return v1.IPv6Protocol, nil + } + return v1.IPv4Protocol, nil +} + +func getIPFamilyFromCIDR(cidrStr string) (v1.IPFamily, error) { + _, netCIDR, err := net.ParseCIDR(cidrStr) + if err != nil { + return "", ErrAddressNotAllowed + } + if utilnet.IsIPv6CIDR(netCIDR) { + return v1.IPv6Protocol, nil + } + return v1.IPv4Protocol, nil } // OtherIPFamily returns the other ip family @@ -297,13 +315,6 @@ func OtherIPFamily(ipFamily v1.IPFamily) v1.IPFamily { return v1.IPv6Protocol } -func getIPFamilyFromCIDR(ip string) v1.IPFamily { - if utilnet.IsIPv6CIDRString(ip) { - return v1.IPv6Protocol - } - return v1.IPv4Protocol -} - // AppendPortIfNeeded appends the given port to IP address unless it is already in // "ipv4:port" or "[ipv6]:port" format. func AppendPortIfNeeded(addr string, port int32) string {