diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 61161aecb5e..ee3fb0d6d60 100644 --- 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 index 9f1eb72d833..f4f094e4a78 100644 --- 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 index 189feab64e1..48bd402f02c 100644 --- 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 index 656dc7306cf..d76afdf2a59 100644 --- 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) + } + } + }) } }