Merge pull request #94107 from robscott/kube-proxy-source-ranges-fix

Updating kube-proxy to trim space from loadBalancerSourceRanges

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, err := net.ParseCIDR(src)
-							_, cidr, _ := net.ParseCIDR(src)
+							if err != nil {
-							if cidr.Contains(proxier.nodeIP) {
+								klog.Errorf("Error parsing %s CIDR in LoadBalancerSourceRanges, dropping: %v", cidr, err)
+							} else if cidr.Contains(proxier.nodeIP) {
 								allowFromNode = true
 							}
 						}

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"}
 		}),
 	)
 

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))
+		info.loadBalancerSourceRanges = 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()
@@ -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)
 		}

pkg/proxy/service_test.go

@@ -413,15 +413,42 @@ 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 {
+		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))
+				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)
@@ -432,9 +459,10 @@ func TestServiceToServiceMap(t *testing.T) {
 					!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)
+					t.Errorf("expected new[%v]to be %v, got %v", svcKey, expectedInfo, *svcInfo)
 				}
 			}
+		})
 	}
 }