Merge pull request #94979 from robscott/automated-cherry-pick-of-#941…

…07-upstream-release-1.19 Automated cherry pick of #94107: Updating kube-proxy to trim space from
kubernetes · Nov 3, 2020 · 3571121 · 3571121
2 parents 9dfb4c8 + 01c8638
commit 3571121
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 23 deletions.
diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go
@@ -1183,9 +1183,10 @@ func (proxier *Proxier) syncProxyRules() {
 						allowFromNode := false
 						for _, src := range svcInfo.LoadBalancerSourceRanges() {
 							writeLine(proxier.natRules, append(args, "-s", src, "-j", string(chosenChain))...)
-							// ignore error because it has been validated
-							_, cidr, _ := net.ParseCIDR(src)
-							if cidr.Contains(proxier.nodeIP) {
+							_, cidr, err := net.ParseCIDR(src)
+							if err != nil {
+								klog.Errorf("Error parsing %s CIDR in LoadBalancerSourceRanges, dropping: %v", cidr, err)
+							} else if cidr.Contains(proxier.nodeIP) {
 								allowFromNode = true
 							}
 						}

diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go
@@ -687,6 +687,10 @@ func TestLoadBalancer(t *testing.T) {
 			svc.Status.LoadBalancer.Ingress = []v1.LoadBalancerIngress{{
 				IP: svcLBIP,
 			}}
+			// Also ensure that invalid LoadBalancerSourceRanges will not result
+			// in a crash.
+			svc.Spec.ExternalIPs = []string{svcLBIP}
+			svc.Spec.LoadBalancerSourceRanges = []string{" 1.2.3.4/28"}
 		}),
 	)
 

diff --git a/pkg/proxy/service.go b/pkg/proxy/service.go
@@ -146,10 +146,14 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic
 		topologyKeys:           service.Spec.TopologyKeys,
 	}
 
+	loadBalancerSourceRanges := make([]string, len(service.Spec.LoadBalancerSourceRanges))
+	for i, sourceRange := range service.Spec.LoadBalancerSourceRanges {
+		loadBalancerSourceRanges[i] = strings.TrimSpace(sourceRange)
+	}
+
 	if sct.isIPv6Mode == nil {
 		info.externalIPs = make([]string, len(service.Spec.ExternalIPs))
-		info.loadBalancerSourceRanges = make([]string, len(service.Spec.LoadBalancerSourceRanges))
-		copy(info.loadBalancerSourceRanges, service.Spec.LoadBalancerSourceRanges)
+		info.loadBalancerSourceRanges = loadBalancerSourceRanges
 		copy(info.externalIPs, service.Spec.ExternalIPs)
 		// Deep-copy in case the service instance changes
 		info.loadBalancerStatus = *service.Status.LoadBalancer.DeepCopy()
@@ -162,7 +166,7 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic
 		if len(incorrectIPs) > 0 {
 			utilproxy.LogAndEmitIncorrectIPVersionEvent(sct.recorder, "externalIPs", strings.Join(incorrectIPs, ","), service.Namespace, service.Name, service.UID)
 		}
-		info.loadBalancerSourceRanges, incorrectIPs = utilproxy.FilterIncorrectCIDRVersion(service.Spec.LoadBalancerSourceRanges, *sct.isIPv6Mode)
+		info.loadBalancerSourceRanges, incorrectIPs = utilproxy.FilterIncorrectCIDRVersion(loadBalancerSourceRanges, *sct.isIPv6Mode)
 		if len(incorrectIPs) > 0 {
 			utilproxy.LogAndEmitIncorrectIPVersionEvent(sct.recorder, "loadBalancerSourceRanges", strings.Join(incorrectIPs, ","), service.Namespace, service.Name, service.UID)
 		}

diff --git a/pkg/proxy/service_test.go b/pkg/proxy/service_test.go
@@ -413,28 +413,56 @@ func TestServiceToServiceMap(t *testing.T) {
 			},
 			isIPv6Mode: &trueVal,
 		},
+		{
+			desc: "service with extra space in LoadBalancerSourceRanges",
+			service: &v1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "extra-space",
+					Namespace: "test",
+				},
+				Spec: v1.ServiceSpec{
+					ClusterIP:                testClusterIPv4,
+					LoadBalancerSourceRanges: []string{" 10.1.2.0/28"},
+					Ports: []v1.ServicePort{
+						{
+							Name:     "testPort",
+							Port:     int32(12345),
+							Protocol: v1.ProtocolTCP,
+						},
+					},
+				},
+			},
+			expected: map[ServicePortName]*BaseServiceInfo{
+				makeServicePortName("test", "extra-space", "testPort", v1.ProtocolTCP): makeTestServiceInfo(testClusterIPv4, 12345, "TCP", 0, func(info *BaseServiceInfo) {
+					info.loadBalancerSourceRanges = []string{"10.1.2.0/28"}
+				}),
+			},
+			isIPv6Mode: &falseVal,
+		},
 	}
 
 	for _, tc := range testCases {
-		svcTracker.isIPv6Mode = tc.isIPv6Mode
-		// outputs
-		newServices := svcTracker.serviceToServiceMap(tc.service)
+		t.Run(tc.desc, func(t *testing.T) {
+			svcTracker.isIPv6Mode = tc.isIPv6Mode
+			// outputs
+			newServices := svcTracker.serviceToServiceMap(tc.service)
 
-		if len(newServices) != len(tc.expected) {
-			t.Errorf("[%s] expected %d new, got %d: %v", tc.desc, len(tc.expected), len(newServices), spew.Sdump(newServices))
-		}
-		for svcKey, expectedInfo := range tc.expected {
-			svcInfo, _ := newServices[svcKey].(*BaseServiceInfo)
-			if !svcInfo.clusterIP.Equal(expectedInfo.clusterIP) ||
-				svcInfo.port != expectedInfo.port ||
-				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...)) ||
-				!reflect.DeepEqual(svcInfo.loadBalancerStatus, expectedInfo.loadBalancerStatus) {
-				t.Errorf("[%s] expected new[%v]to be %v, got %v", tc.desc, svcKey, expectedInfo, *svcInfo)
+			if len(newServices) != len(tc.expected) {
+				t.Errorf("expected %d new, got %d: %v", len(tc.expected), len(newServices), spew.Sdump(newServices))
 			}
-		}
+			for svcKey, expectedInfo := range tc.expected {
+				svcInfo, _ := newServices[svcKey].(*BaseServiceInfo)
+				if !svcInfo.clusterIP.Equal(expectedInfo.clusterIP) ||
+					svcInfo.port != expectedInfo.port ||
+					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...)) ||
+					!reflect.DeepEqual(svcInfo.loadBalancerStatus, expectedInfo.loadBalancerStatus) {
+					t.Errorf("expected new[%v]to be %v, got %v", svcKey, expectedInfo, *svcInfo)
+				}
+			}
+		})
 	}
 }