From de15774e10a6ec2d977fced07780ab8390b743f8 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Sun, 12 Jan 2020 18:29:16 +0100 Subject: [PATCH 1/2] kube-proxy unit test FilterIncorrectIPVersion Add an unit test to the kube-proxy FilterIncorrectIPVersion function --- pkg/proxy/util/utils_test.go | 63 ++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index 71794e81c3c..867c9e04c7a 100644 --- a/pkg/proxy/util/utils_test.go +++ b/pkg/proxy/util/utils_test.go @@ -537,3 +537,66 @@ func TestShuffleStrings(t *testing.T) { } } } + +func TestFilterIncorrectIPVersion(t *testing.T) { + testCases := []struct { + desc string + ipString []string + wantIPv6 bool + expectCorrect []string + expectIncorrect []string + }{ + { + desc: "want IPv6 and receive IPv4 and IPv6", + ipString: []string{"192.168.200.2", "192.1.34.23", "fd00:20::1", "2001:db9::3"}, + wantIPv6: true, + expectCorrect: []string{"fd00:20::1", "2001:db9::3"}, + expectIncorrect: []string{"192.168.200.2", "192.1.34.23"}, + }, + { + desc: "want IPv4 and receive IPv4 and IPv6", + ipString: []string{"192.168.200.2", "192.1.34.23", "fd00:20::1", "2001:db9::3"}, + wantIPv6: false, + expectCorrect: []string{"192.168.200.2", "192.1.34.23"}, + expectIncorrect: []string{"fd00:20::1", "2001:db9::3"}, + }, + { + desc: "want IPv4 and receive IPv4 only", + ipString: []string{"192.168.200.2", "192.1.34.23"}, + wantIPv6: false, + expectCorrect: []string{"192.168.200.2", "192.1.34.23"}, + expectIncorrect: nil, + }, + { + desc: "want IPv6 and receive IPv4 only", + ipString: []string{"192.168.200.2", "192.1.34.23"}, + wantIPv6: true, + expectCorrect: nil, + expectIncorrect: []string{"192.168.200.2", "192.1.34.23"}, + }, + { + desc: "want IPv4 and receive IPv6 only", + ipString: []string{"fd00:20::1", "2001:db9::3"}, + wantIPv6: false, + expectCorrect: nil, + expectIncorrect: []string{"fd00:20::1", "2001:db9::3"}, + }, + { + desc: "want IPv6 and receive IPv6 only", + ipString: []string{"fd00:20::1", "2001:db9::3"}, + wantIPv6: true, + expectCorrect: []string{"fd00:20::1", "2001:db9::3"}, + expectIncorrect: nil, + }, + } + + for i := range testCases { + correct, incorrect := FilterIncorrectIPVersion(testCases[i].ipString, testCases[i].wantIPv6) + if !reflect.DeepEqual(testCases[i].expectCorrect, correct) { + t.Errorf("Test %v failed: expected %v, got %v", testCases[i].desc, testCases[i].expectCorrect, correct) + } + if !reflect.DeepEqual(testCases[i].expectIncorrect, incorrect) { + t.Errorf("Test %v failed: expected %v, got %v", testCases[i].desc, testCases[i].expectIncorrect, incorrect) + } + } +} From 11263bb57f909f09a244dbe82a0403c3a2518b47 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Mon, 13 Jan 2020 23:55:07 +0100 Subject: [PATCH 2/2] kube-proxy filter Load Balancer Status ingress kube-proxy, if is configured with an IP family, filters out the incorrect IP version of the services. This commit fix a bug caused by not filtering out the IPs in the LoadBalancer Status Ingress field. --- pkg/proxy/service.go | 30 ++++++++++--- pkg/proxy/service_test.go | 84 +++++++++++++++++++++++++++++------- pkg/proxy/util/utils_test.go | 46 ++++++++++++++++---- 3 files changed, 130 insertions(+), 30 deletions(-) diff --git a/pkg/proxy/service.go b/pkg/proxy/service.go index 5a18d2c7941..7fb28b52808 100644 --- a/pkg/proxy/service.go +++ b/pkg/proxy/service.go @@ -136,12 +136,10 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic stickyMaxAgeSeconds = int(*service.Spec.SessionAffinityConfig.ClientIP.TimeoutSeconds) } info := &BaseServiceInfo{ - clusterIP: net.ParseIP(service.Spec.ClusterIP), - port: int(port.Port), - protocol: port.Protocol, - nodePort: int(port.NodePort), - // Deep-copy in case the service instance changes - loadBalancerStatus: *service.Status.LoadBalancer.DeepCopy(), + clusterIP: net.ParseIP(service.Spec.ClusterIP), + port: int(port.Port), + protocol: port.Protocol, + nodePort: int(port.NodePort), sessionAffinityType: service.Spec.SessionAffinity, stickyMaxAgeSeconds: stickyMaxAgeSeconds, onlyNodeLocalEndpoints: onlyNodeLocalEndpoints, @@ -153,9 +151,11 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic info.loadBalancerSourceRanges = make([]string, len(service.Spec.LoadBalancerSourceRanges)) copy(info.loadBalancerSourceRanges, service.Spec.LoadBalancerSourceRanges) copy(info.externalIPs, service.Spec.ExternalIPs) + // Deep-copy in case the service instance changes + info.loadBalancerStatus = *service.Status.LoadBalancer.DeepCopy() } else { // Filter out the incorrect IP version case. - // If ExternalIPs and LoadBalancerSourceRanges on service contains incorrect IP versions, + // If ExternalIPs, LoadBalancerSourceRanges and LoadBalancerStatus Ingress on service contains incorrect IP versions, // only filter out the incorrect ones. var incorrectIPs []string info.externalIPs, incorrectIPs = utilproxy.FilterIncorrectIPVersion(service.Spec.ExternalIPs, *sct.isIPv6Mode) @@ -166,6 +166,22 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic if len(incorrectIPs) > 0 { utilproxy.LogAndEmitIncorrectIPVersionEvent(sct.recorder, "loadBalancerSourceRanges", strings.Join(incorrectIPs, ","), service.Namespace, service.Name, service.UID) } + // Obtain Load Balancer Ingress IPs + var ips []string + for _, ing := range service.Status.LoadBalancer.Ingress { + ips = append(ips, ing.IP) + } + if len(ips) > 0 { + correctIPs, incorrectIPs := utilproxy.FilterIncorrectIPVersion(ips, *sct.isIPv6Mode) + if len(incorrectIPs) > 0 { + utilproxy.LogAndEmitIncorrectIPVersionEvent(sct.recorder, "Load Balancer ingress IPs", strings.Join(incorrectIPs, ","), service.Namespace, service.Name, service.UID) + } + // Create the LoadBalancerStatus with the filtererd IPs + for _, ip := range correctIPs { + info.loadBalancerStatus.Ingress = append(info.loadBalancerStatus.Ingress, v1.LoadBalancerIngress{IP: ip}) + + } + } } if apiservice.NeedsHealthCheck(service) { diff --git a/pkg/proxy/service_test.go b/pkg/proxy/service_test.go index decf3cdc16c..9f1195968f2 100644 --- a/pkg/proxy/service_test.go +++ b/pkg/proxy/service_test.go @@ -18,6 +18,7 @@ package proxy import ( "net" + "reflect" "testing" "github.com/davecgh/go-spew/spew" @@ -166,15 +167,15 @@ func TestServiceToServiceMap(t *testing.T) { svc.Spec.LoadBalancerIP = "5.6.7.8" svc.Spec.Ports = addTestPort(svc.Spec.Ports, "port3", "UDP", 8675, 30061, 7000) svc.Spec.Ports = addTestPort(svc.Spec.Ports, "port4", "UDP", 8676, 30062, 7001) - svc.Status.LoadBalancer = v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{ - {IP: "10.1.2.4"}, - }, - } + svc.Status.LoadBalancer.Ingress = []v1.LoadBalancerIngress{{IP: "10.1.2.4"}} }), expected: map[ServicePortName]*BaseServiceInfo{ - makeServicePortName("ns1", "load-balancer", "port3", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8675, "UDP", 0), - makeServicePortName("ns1", "load-balancer", "port4", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8676, "UDP", 0), + makeServicePortName("ns1", "load-balancer", "port3", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8675, "UDP", 0, func(info *BaseServiceInfo) { + info.loadBalancerStatus.Ingress = []v1.LoadBalancerIngress{{IP: "10.1.2.4"}} + }), + makeServicePortName("ns1", "load-balancer", "port4", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.11", 8676, "UDP", 0, func(info *BaseServiceInfo) { + info.loadBalancerStatus.Ingress = []v1.LoadBalancerIngress{{IP: "10.1.2.4"}} + }), }, }, { @@ -185,17 +186,17 @@ func TestServiceToServiceMap(t *testing.T) { svc.Spec.LoadBalancerIP = "5.6.7.8" svc.Spec.Ports = addTestPort(svc.Spec.Ports, "portx", "UDP", 8677, 30063, 7002) svc.Spec.Ports = addTestPort(svc.Spec.Ports, "porty", "UDP", 8678, 30064, 7003) - svc.Status.LoadBalancer = v1.LoadBalancerStatus{ - Ingress: []v1.LoadBalancerIngress{ - {IP: "10.1.2.3"}, - }, - } + svc.Status.LoadBalancer.Ingress = []v1.LoadBalancerIngress{{IP: "10.1.2.3"}} svc.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeLocal svc.Spec.HealthCheckNodePort = 345 }), expected: map[ServicePortName]*BaseServiceInfo{ - makeServicePortName("ns1", "only-local-load-balancer", "portx", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.12", 8677, "UDP", 345), - makeServicePortName("ns1", "only-local-load-balancer", "porty", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.12", 8678, "UDP", 345), + makeServicePortName("ns1", "only-local-load-balancer", "portx", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.12", 8677, "UDP", 345, func(info *BaseServiceInfo) { + info.loadBalancerStatus.Ingress = []v1.LoadBalancerIngress{{IP: "10.1.2.3"}} + }), + makeServicePortName("ns1", "only-local-load-balancer", "porty", v1.ProtocolUDP): makeTestServiceInfo("172.16.55.12", 8678, "UDP", 345, func(info *BaseServiceInfo) { + info.loadBalancerStatus.Ingress = []v1.LoadBalancerIngress{{IP: "10.1.2.3"}} + }), }, }, { @@ -225,6 +226,14 @@ func TestServiceToServiceMap(t *testing.T) { }, }, }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: testExternalIPv4}, + {IP: testExternalIPv6}, + }, + }, + }, }, isIPv6Mode: &falseVal, }, @@ -245,6 +254,14 @@ func TestServiceToServiceMap(t *testing.T) { }, }, }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: testExternalIPv4}, + {IP: testExternalIPv6}, + }, + }, + }, }, isIPv6Mode: &trueVal, }, @@ -267,11 +284,20 @@ func TestServiceToServiceMap(t *testing.T) { }, }, }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: testExternalIPv4}, + {IP: testExternalIPv6}, + }, + }, + }, }, expected: map[ServicePortName]*BaseServiceInfo{ makeServicePortName("test", "validIPv4", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv4, 12345, "TCP", 0, func(info *BaseServiceInfo) { info.externalIPs = []string{testExternalIPv4} info.loadBalancerSourceRanges = []string{testSourceRangeIPv4} + info.loadBalancerStatus.Ingress = []v1.LoadBalancerIngress{{IP: testExternalIPv4}} }), }, isIPv6Mode: &falseVal, @@ -295,11 +321,20 @@ func TestServiceToServiceMap(t *testing.T) { }, }, }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: testExternalIPv4}, + {IP: testExternalIPv6}, + }, + }, + }, }, expected: map[ServicePortName]*BaseServiceInfo{ makeServicePortName("test", "validIPv6", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv6, 12345, "TCP", 0, func(info *BaseServiceInfo) { info.externalIPs = []string{testExternalIPv6} info.loadBalancerSourceRanges = []string{testSourceRangeIPv6} + info.loadBalancerStatus.Ingress = []v1.LoadBalancerIngress{{IP: testExternalIPv6}} }), }, isIPv6Mode: &trueVal, @@ -323,11 +358,20 @@ func TestServiceToServiceMap(t *testing.T) { }, }, }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: testExternalIPv4}, + {IP: testExternalIPv6}, + }, + }, + }, }, expected: map[ServicePortName]*BaseServiceInfo{ makeServicePortName("test", "filterIPv6InIPV4Mode", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv4, 12345, "TCP", 0, func(info *BaseServiceInfo) { info.externalIPs = []string{testExternalIPv4} info.loadBalancerSourceRanges = []string{testSourceRangeIPv4} + info.loadBalancerStatus.Ingress = []v1.LoadBalancerIngress{{IP: testExternalIPv4}} }), }, isIPv6Mode: &falseVal, @@ -351,11 +395,20 @@ func TestServiceToServiceMap(t *testing.T) { }, }, }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + {IP: testExternalIPv4}, + {IP: testExternalIPv6}, + }, + }, + }, }, expected: map[ServicePortName]*BaseServiceInfo{ makeServicePortName("test", "filterIPv4InIPV6Mode", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv6, 12345, "TCP", 0, func(info *BaseServiceInfo) { info.externalIPs = []string{testExternalIPv6} info.loadBalancerSourceRanges = []string{testSourceRangeIPv6} + info.loadBalancerStatus.Ingress = []v1.LoadBalancerIngress{{IP: testExternalIPv6}} }), }, isIPv6Mode: &trueVal, @@ -377,7 +430,8 @@ func TestServiceToServiceMap(t *testing.T) { svcInfo.protocol != expectedInfo.protocol || svcInfo.healthCheckNodePort != expectedInfo.healthCheckNodePort || !sets.NewString(svcInfo.externalIPs...).Equal(sets.NewString(expectedInfo.externalIPs...)) || - !sets.NewString(svcInfo.loadBalancerSourceRanges...).Equal(sets.NewString(expectedInfo.loadBalancerSourceRanges...)) { + !sets.NewString(svcInfo.loadBalancerSourceRanges...).Equal(sets.NewString(expectedInfo.loadBalancerSourceRanges...)) || + !reflect.DeepEqual(svcInfo.loadBalancerStatus, expectedInfo.loadBalancerStatus) { t.Errorf("[%s] expected new[%v]to be %v, got %v", tc.desc, svcKey, expectedInfo, *svcInfo) } } diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index 867c9e04c7a..234d2049794 100644 --- a/pkg/proxy/util/utils_test.go +++ b/pkg/proxy/util/utils_test.go @@ -546,6 +546,34 @@ func TestFilterIncorrectIPVersion(t *testing.T) { 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"}, + wantIPv6: false, + expectCorrect: nil, + expectIncorrect: []string{"fd00:20::1"}, + }, + { + desc: "want IPv6 and receive IPv4", + ipString: []string{"192.168.200.2"}, + wantIPv6: true, + expectCorrect: nil, + expectIncorrect: []string{"192.168.200.2"}, + }, { desc: "want IPv6 and receive IPv4 and IPv6", ipString: []string{"192.168.200.2", "192.1.34.23", "fd00:20::1", "2001:db9::3"}, @@ -590,13 +618,15 @@ func TestFilterIncorrectIPVersion(t *testing.T) { }, } - for i := range testCases { - correct, incorrect := FilterIncorrectIPVersion(testCases[i].ipString, testCases[i].wantIPv6) - if !reflect.DeepEqual(testCases[i].expectCorrect, correct) { - t.Errorf("Test %v failed: expected %v, got %v", testCases[i].desc, testCases[i].expectCorrect, correct) - } - if !reflect.DeepEqual(testCases[i].expectIncorrect, incorrect) { - t.Errorf("Test %v failed: expected %v, got %v", testCases[i].desc, testCases[i].expectIncorrect, incorrect) - } + for _, testcase := range testCases { + t.Run(testcase.desc, func(t *testing.T) { + correct, incorrect := FilterIncorrectIPVersion(testcase.ipString, testcase.wantIPv6) + if !reflect.DeepEqual(testcase.expectCorrect, correct) { + t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectCorrect, correct) + } + if !reflect.DeepEqual(testcase.expectIncorrect, incorrect) { + t.Errorf("Test %v failed: expected %v, got %v", testcase.desc, testcase.expectIncorrect, incorrect) + } + }) } }