diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index e7783a5c8e2..5b7c9fd5099 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -2667,15 +2667,6 @@ func ValidateServiceUpdate(service, oldService *api.Service) field.ErrorList { } } - // TODO(freehan): allow user to update loadbalancerSourceRanges - // Only allow removing LoadBalancerSourceRanges when change service type from LoadBalancer - // to non-LoadBalancer or adding LoadBalancerSourceRanges when change service type from - // non-LoadBalancer to LoadBalancer. - if service.Spec.Type != api.ServiceTypeLoadBalancer && oldService.Spec.Type != api.ServiceTypeLoadBalancer || - service.Spec.Type == api.ServiceTypeLoadBalancer && oldService.Spec.Type == api.ServiceTypeLoadBalancer { - allErrs = append(allErrs, ValidateImmutableField(service.Spec.LoadBalancerSourceRanges, oldService.Spec.LoadBalancerSourceRanges, field.NewPath("spec", "loadBalancerSourceRanges"))...) - } - allErrs = append(allErrs, validateServiceFields(service)...) allErrs = append(allErrs, validateServiceAnnotations(service, oldService)...) return allErrs diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 094a4a0d19a..6a7a02b74e2 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -6675,7 +6675,7 @@ func TestValidateServiceUpdate(t *testing.T) { newSvc.Spec.Type = api.ServiceTypeLoadBalancer newSvc.Spec.LoadBalancerSourceRanges = []string{"10.0.0.0/8"} }, - numErrs: 1, + numErrs: 0, }, { name: "update loadBalancerSourceRanges", @@ -6685,7 +6685,7 @@ func TestValidateServiceUpdate(t *testing.T) { newSvc.Spec.Type = api.ServiceTypeLoadBalancer newSvc.Spec.LoadBalancerSourceRanges = []string{"10.180.0.0/16"} }, - numErrs: 1, + numErrs: 0, }, { name: "LoadBalancer type cannot have None ClusterIP", diff --git a/pkg/controller/service/servicecontroller.go b/pkg/controller/service/servicecontroller.go index 44a171e89b4..da8fb1ec6a5 100644 --- a/pkg/controller/service/servicecontroller.go +++ b/pkg/controller/service/servicecontroller.go @@ -430,6 +430,13 @@ func (s *ServiceController) needsUpdate(oldService *v1.Service, newService *v1.S oldService.Spec.Type, newService.Spec.Type) return true } + + if wantsLoadBalancer(newService) && !reflect.DeepEqual(oldService.Spec.LoadBalancerSourceRanges, newService.Spec.LoadBalancerSourceRanges) { + s.eventRecorder.Eventf(newService, v1.EventTypeNormal, "LoadBalancerSourceRanges", "%v -> %v", + oldService.Spec.LoadBalancerSourceRanges, newService.Spec.LoadBalancerSourceRanges) + return true + } + if !portsEqualForLB(oldService, newService) || oldService.Spec.SessionAffinity != newService.Spec.SessionAffinity { return true } diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 4c1d1205ff9..ede19349581 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -388,6 +388,9 @@ func (proxier *Proxier) sameConfig(info *serviceInfo, service *api.Service, port if info.onlyNodeLocalEndpoints != onlyNodeLocalEndpoints { return false } + if !reflect.DeepEqual(info.loadBalancerSourceRanges, service.Spec.LoadBalancerSourceRanges) { + return false + } return true } diff --git a/test/e2e/service.go b/test/e2e/service.go index 914766295db..b0f8d0fa050 100644 --- a/test/e2e/service.go +++ b/test/e2e/service.go @@ -1103,6 +1103,73 @@ var _ = framework.KubeDescribe("Services", func() { framework.Failf("expected un-ready endpoint for Service %v within %v, stdout: %v", t.name, kubeProxyLagTimeout, stdout) } }) + + It("should only allow access from service loadbalancer source ranges [Slow]", func() { + // this feature currently supported only on GCE/GKE/AWS + framework.SkipUnlessProviderIs("gce", "gke", "aws") + + loadBalancerCreateTimeout := loadBalancerCreateTimeoutDefault + if nodes := framework.GetReadySchedulableNodesOrDie(cs); len(nodes.Items) > largeClusterMinNodesNumber { + loadBalancerCreateTimeout = loadBalancerCreateTimeoutLarge + } + + namespace := f.Namespace.Name + serviceName := "lb-sourcerange" + jig := NewServiceTestJig(cs, serviceName) + + By("Prepare allow source ips") + // prepare the exec pods + // acceptPod are allowed to access the loadbalancer + acceptPodName := createExecPodOrFail(cs, namespace, "execpod-accept") + dropPodName := createExecPodOrFail(cs, namespace, "execpod-drop") + + accpetPod, err := cs.Core().Pods(namespace).Get(acceptPodName) + Expect(err).NotTo(HaveOccurred()) + dropPod, err := cs.Core().Pods(namespace).Get(dropPodName) + Expect(err).NotTo(HaveOccurred()) + + By("creating a pod to be part of the service " + serviceName) + // This container is an nginx container listening on port 80 + // See kubernetes/contrib/ingress/echoheaders/nginx.conf for content of response + jig.RunOrFail(namespace, nil) + // Create loadbalancer service with source range from node[0] and podAccept + svc := jig.CreateTCPServiceOrFail(namespace, func(svc *v1.Service) { + svc.Spec.Type = v1.ServiceTypeLoadBalancer + svc.Spec.LoadBalancerSourceRanges = []string{accpetPod.Status.PodIP + "/32"} + }) + + // Clean up loadbalancer service + defer func() { + jig.UpdateServiceOrFail(svc.Namespace, svc.Name, func(svc *v1.Service) { + svc.Spec.Type = v1.ServiceTypeNodePort + svc.Spec.LoadBalancerSourceRanges = nil + }) + Expect(cs.Core().Services(svc.Namespace).Delete(svc.Name, nil)).NotTo(HaveOccurred()) + }() + + svc = jig.WaitForLoadBalancerOrFail(namespace, serviceName, loadBalancerCreateTimeout) + jig.SanityCheckService(svc, v1.ServiceTypeLoadBalancer) + + By("check reachability from different sources") + svcIP := getIngressPoint(&svc.Status.LoadBalancer.Ingress[0]) + checkReachabilityFromPod(true, namespace, acceptPodName, svcIP) + checkReachabilityFromPod(false, namespace, dropPodName, svcIP) + + By("Update service LoadBalancerSourceRange and check reachability") + jig.UpdateServiceOrFail(svc.Namespace, svc.Name, func(svc *v1.Service) { + // only allow access from dropPod + svc.Spec.LoadBalancerSourceRanges = []string{dropPod.Status.PodIP + "/32"} + }) + checkReachabilityFromPod(false, namespace, acceptPodName, svcIP) + checkReachabilityFromPod(true, namespace, dropPodName, svcIP) + + By("Delete LoadBalancerSourceRange field and check reachability") + jig.UpdateServiceOrFail(svc.Namespace, svc.Name, func(svc *v1.Service) { + svc.Spec.LoadBalancerSourceRanges = nil + }) + checkReachabilityFromPod(true, namespace, acceptPodName, svcIP) + checkReachabilityFromPod(true, namespace, dropPodName, svcIP) + }) }) var _ = framework.KubeDescribe("ESIPP [Slow][Feature:ExternalTrafficLocalOnly]", func() { @@ -2773,3 +2840,21 @@ func describeSvc(ns string) { "describe", "svc", fmt.Sprintf("--namespace=%v", ns)) framework.Logf(desc) } + +func checkReachabilityFromPod(expectToBeReachable bool, namespace, pod, target string) { + cmd := fmt.Sprintf("wget -T 5 -qO- %q", target) + err := wait.PollImmediate(framework.Poll, 2*time.Minute, func() (bool, error) { + _, err := framework.RunHostCmd(namespace, pod, cmd) + if expectToBeReachable && err != nil { + framework.Logf("Expect target to be reachable. But got err: %v. Retry until timeout", err) + return false, nil + } + + if !expectToBeReachable && err == nil { + framework.Logf("Expect target NOT to be reachable. But it is reachable. Retry until timeout") + return false, nil + } + return true, nil + }) + Expect(err).NotTo(HaveOccurred()) +} diff --git a/test/test_owners.csv b/test/test_owners.csv index 838c3c1f706..50de3e6b4b5 100644 --- a/test/test_owners.csv +++ b/test/test_owners.csv @@ -444,6 +444,7 @@ Services should be able to create a functioning NodePort service,bprashanth,0 Services should be able to up and down services,bprashanth,0 Services should check NodePort out-of-range,bprashanth,0 Services should create endpoints for unready pods,maisem,0 +Serivces should only allow access from service loadbalancer source ranges,freehan,0 Services should preserve source pod IP for traffic thru service cluster IP,Random-Liu,1 Services should prevent NodePort collisions,bprashanth,0 Services should provide secure master service,bprashanth,0