Merge pull request kubernetes#103467 from thockin/svc-alloc-lb-nodepo…

…rts-bug

Fix small bug with AllocateLoadBalancerNodePorts

pkg/api/service/testing/make.go

@@ -66,19 +66,22 @@ func SetTypeClusterIP(svc *api.Service) {
 	}
 	svc.Spec.ExternalName = ""
 	svc.Spec.ExternalTrafficPolicy = ""
+	svc.Spec.AllocateLoadBalancerNodePorts = nil
 }
 
 // SetTypeNodePort sets the service type to NodePort and clears other fields.
 func SetTypeNodePort(svc *api.Service) {
 	svc.Spec.Type = api.ServiceTypeNodePort
 	svc.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeCluster
 	svc.Spec.ExternalName = ""
+	svc.Spec.AllocateLoadBalancerNodePorts = nil
 }
 
 // SetTypeLoadBalancer sets the service type to LoadBalancer and clears other fields.
 func SetTypeLoadBalancer(svc *api.Service) {
 	svc.Spec.Type = api.ServiceTypeLoadBalancer
 	svc.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeCluster
+	svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(true)
 	svc.Spec.ExternalName = ""
 }
 
@@ -89,16 +92,7 @@ func SetTypeExternalName(svc *api.Service) {
 	svc.Spec.ExternalTrafficPolicy = ""
 	svc.Spec.ClusterIP = ""
 	svc.Spec.ClusterIPs = nil
-}
-
-// SetTypeExternalNameTrue sets the allocate LB node port to true.
-func SetAllocateLBNodePortTrue(svc *api.Service) {
-	svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(true)
-}
-
-// SetTypeExternalNameFalse sets the allocate LB node port to false.
-func SetAllocateLBNodePortFalse(svc *api.Service) {
-	svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(false)
+	svc.Spec.AllocateLoadBalancerNodePorts = nil
 }
 
 // SetPorts sets the service ports list.
@@ -160,3 +154,10 @@ func SetInternalTrafficPolicy(policy api.ServiceInternalTrafficPolicyType) Tweak
 		svc.Spec.InternalTrafficPolicy = &policy
 	}
 }
+
+// SetAllocateLoadBalancerNodePorts sets the allocate LB node port field.
+func SetAllocateLoadBalancerNodePorts(val bool) Tweak {
+	return func(svc *api.Service) {
+		svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(val)
+	}
+}

pkg/apis/core/types.go

@@ -3749,11 +3749,13 @@ type ServiceSpec struct {
 	PublishNotReadyAddresses bool
 
 	// allocateLoadBalancerNodePorts defines if NodePorts will be automatically
-	// allocated for services with type LoadBalancer.  Default is "true". It may be
-	// set to "false" if the cluster load-balancer does not rely on NodePorts.
-	// allocateLoadBalancerNodePorts may only be set for services with type LoadBalancer
-	// and will be cleared if the type is changed to any other type.
-	// This field is alpha-level and is only honored by servers that enable the ServiceLBNodePortControl feature.
+	// allocated for services with type LoadBalancer.  Default is "true". It
+	// may be set to "false" if the cluster load-balancer does not rely on
+	// NodePorts.  If the caller requests specific NodePorts (by specifying a
+	// value), those requests will be respected, regardless of this field.
+	// This field may only be set for services with type LoadBalancer and will
+	// be cleared if the type is changed to any other type.
+	// This field is beta-level and is only honored by servers that enable the ServiceLBNodePortControl feature.
 	// +optional
 	AllocateLoadBalancerNodePorts *bool
 

pkg/registry/core/service/storage/rest.go

@@ -226,10 +226,7 @@ func (rs *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 	nodePortOp := portallocator.StartOperation(rs.serviceNodePorts, dryrun.IsDryRun(options.DryRun))
 	defer nodePortOp.Finish()
 
-	// TODO: This creates nodePorts if needed. In the future nodePorts may be cleared if *never* used.
-	// But for now we stick to the KEP "don't allocate new node ports but do not deallocate existing node ports if set"
-	if service.Spec.Type == api.ServiceTypeNodePort ||
-		(service.Spec.Type == api.ServiceTypeLoadBalancer && shouldAllocateNodePorts(service)) {
+	if service.Spec.Type == api.ServiceTypeNodePort || service.Spec.Type == api.ServiceTypeLoadBalancer {
 		if err := initNodePorts(service, nodePortOp); err != nil {
 			return nil, err
 		}
@@ -334,10 +331,16 @@ func (rs *REST) releaseAllocatedResources(svc *api.Service) {
 }
 
 func shouldAllocateNodePorts(service *api.Service) bool {
-	if utilfeature.DefaultFeatureGate.Enabled(features.ServiceLBNodePortControl) {
-		return *service.Spec.AllocateLoadBalancerNodePorts
+	if service.Spec.Type == api.ServiceTypeNodePort {
+		return true
 	}
-	return true
+	if service.Spec.Type == api.ServiceTypeLoadBalancer {
+		if utilfeature.DefaultFeatureGate.Enabled(features.ServiceLBNodePortControl) {
+			return *service.Spec.AllocateLoadBalancerNodePorts
+		}
+		return true
+	}
+	return false
 }
 
 // externalTrafficPolicyUpdate adjusts ExternalTrafficPolicy during service update if needed.
@@ -477,8 +480,7 @@ func (rs *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObj
 		releaseNodePorts(oldService, nodePortOp)
 	}
 	// Update service from any type to NodePort or LoadBalancer, should update NodePort.
-	if service.Spec.Type == api.ServiceTypeNodePort ||
-		(service.Spec.Type == api.ServiceTypeLoadBalancer && shouldAllocateNodePorts(service)) {
+	if service.Spec.Type == api.ServiceTypeNodePort || service.Spec.Type == api.ServiceTypeLoadBalancer {
 		if err := updateNodePorts(oldService, service, nodePortOp); err != nil {
 			return nil, false, err
 		}
@@ -1172,6 +1174,10 @@ func initNodePorts(service *api.Service, nodePortOp *portallocator.PortAllocatio
 	svcPortToNodePort := map[int]int{}
 	for i := range service.Spec.Ports {
 		servicePort := &service.Spec.Ports[i]
+		if servicePort.NodePort == 0 && !shouldAllocateNodePorts(service) {
+			// Don't allocate new ports, but do respect specific requests.
+			continue
+		}
 		allocatedNodePort := svcPortToNodePort[int(servicePort.Port)]
 		if allocatedNodePort == 0 {
 			// This will only scan forward in the service.Spec.Ports list because any matches
@@ -1224,6 +1230,10 @@ func updateNodePorts(oldService, newService *api.Service, nodePortOp *portalloca
 
 	for i := range newService.Spec.Ports {
 		servicePort := &newService.Spec.Ports[i]
+		if servicePort.NodePort == 0 && !shouldAllocateNodePorts(newService) {
+			// Don't allocate new ports, but do respect specific requests.
+			continue
+		}
 		nodePort := ServiceNodePort{Protocol: servicePort.Protocol, NodePort: servicePort.NodePort}
 		if nodePort.NodePort != 0 {
 			if !containsNumber(oldNodePortsNumbers, int(nodePort.NodePort)) && !portAllocated[int(nodePort.NodePort)] {

pkg/registry/core/service/storage/rest_test.go

@@ -736,7 +736,7 @@ func TestServiceRegistryLoadBalancerService(t *testing.T) {
 	ctx := genericapirequest.NewDefaultContext()
 	storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})
 	defer server.Terminate(t)
-	svc := svctest.MakeService("foo", svctest.SetTypeLoadBalancer, svctest.SetAllocateLBNodePortTrue)
+	svc := svctest.MakeService("foo", svctest.SetTypeLoadBalancer)
 	_, err := storage.Create(ctx, svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
 	if err != nil {
 		t.Errorf("Failed to create service: %#v", err)
@@ -762,28 +762,56 @@ func TestAllocateLoadBalancerNodePorts(t *testing.T) {
 		allocateNodePortGate bool
 		expectError          bool
 	}{{
-		name:                 "allocate false, gate on",
-		svc:                  svctest.MakeService("alloc-false", svctest.SetTypeLoadBalancer, svctest.SetAllocateLBNodePortFalse),
+		name: "allocate false, gate on, not specified",
+		svc: svctest.MakeService("alloc-false",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetAllocateLoadBalancerNodePorts(false)),
 		expectNodePorts:      false,
 		allocateNodePortGate: true,
 	}, {
-		name:                 "allocate true, gate on",
-		svc:                  svctest.MakeService("alloc-true", svctest.SetTypeLoadBalancer, svctest.SetAllocateLBNodePortTrue),
+		name: "allocate true, gate on, not specified",
+		svc: svctest.MakeService("alloc-true",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetAllocateLoadBalancerNodePorts(true)),
 		expectNodePorts:      true,
 		allocateNodePortGate: true,
 	}, {
-		name:                 "allocate nil, gate off",
-		svc:                  svctest.MakeService("alloc-nil", svctest.SetTypeLoadBalancer),
+		name: "allocate false, gate on, port specified",
+		svc: svctest.MakeService("alloc-false-specific",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetNodePorts(30000),
+			svctest.SetAllocateLoadBalancerNodePorts(false)),
+		expectNodePorts:      true,
+		allocateNodePortGate: true,
+	}, {
+		name: "allocate true, gate on, port specified",
+		svc: svctest.MakeService("alloc-true-specific",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetNodePorts(30000),
+			svctest.SetAllocateLoadBalancerNodePorts(true)),
+		expectNodePorts:      true,
+		allocateNodePortGate: true,
+	}, {
+		name: "allocate nil, gate off",
+		svc: svctest.MakeService("alloc-nil",
+			svctest.SetTypeLoadBalancer,
+			func(s *api.Service) {
+				s.Spec.AllocateLoadBalancerNodePorts = nil
+			}),
 		expectNodePorts:      true,
 		allocateNodePortGate: false,
 	}, {
-		name:                 "allocate false, gate off",
-		svc:                  svctest.MakeService("alloc-false", svctest.SetTypeLoadBalancer, svctest.SetAllocateLBNodePortFalse),
+		name: "allocate false, gate off",
+		svc: svctest.MakeService("alloc-false",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetAllocateLoadBalancerNodePorts(false)),
 		expectNodePorts:      true,
 		allocateNodePortGate: false,
 	}, {
-		name:                 "allocate true, gate off",
-		svc:                  svctest.MakeService("alloc-true", svctest.SetTypeLoadBalancer, svctest.SetAllocateLBNodePortTrue),
+		name: "allocate true, gate off",
+		svc: svctest.MakeService("alloc-true",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetAllocateLoadBalancerNodePorts(true)),
 		expectNodePorts:      true,
 		allocateNodePortGate: false,
 	}}
@@ -965,9 +993,7 @@ func TestServiceRegistryUpdateMultiPortLoadBalancerService(t *testing.T) {
 		svctest.SetTypeLoadBalancer,
 		svctest.SetPorts(
 			svctest.MakeServicePort("p", 6502, intstr.FromInt(6502), api.ProtocolTCP),
-			svctest.MakeServicePort("q", 8086, intstr.FromInt(8086), api.ProtocolTCP)),
-		svctest.SetAllocateLBNodePortTrue,
-	)
+			svctest.MakeServicePort("q", 8086, intstr.FromInt(8086), api.ProtocolTCP)))
 	obj, err := storage.Create(ctx, svc1, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
 	if err != nil {
 		t.Fatalf("Unexpected error: %v", err)
@@ -1321,7 +1347,7 @@ func TestServiceRegistryIPLoadBalancer(t *testing.T) {
 	storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})
 	defer server.Terminate(t)
 
-	svc := svctest.MakeService("foo", svctest.SetTypeLoadBalancer, svctest.SetAllocateLBNodePortTrue)
+	svc := svctest.MakeService("foo", svctest.SetTypeLoadBalancer)
 	ctx := genericapirequest.NewDefaultContext()
 	createdSvc, err := storage.Create(ctx, svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
 	if createdSvc == nil || err != nil {
@@ -1352,7 +1378,6 @@ func TestServiceRegistryExternalTrafficHealthCheckNodePortAllocation(t *testing.
 	defer server.Terminate(t)
 	svc := svctest.MakeService("external-lb-esipp",
 		svctest.SetTypeLoadBalancer,
-		svctest.SetAllocateLBNodePortTrue,
 		func(s *api.Service) {
 			s.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeLocal
 		},
@@ -1380,7 +1405,6 @@ func TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation(t *test
 	defer server.Terminate(t)
 	svc := svctest.MakeService("external-lb-esipp",
 		svctest.SetTypeLoadBalancer,
-		svctest.SetAllocateLBNodePortTrue,
 		func(s *api.Service) {
 			// hard-code NodePort to make sure it doesn't conflict with the healthport.
 			// TODO: remove this once http://issue.k8s.io/93922 fixes auto-allocation conflicting with user-specified health check ports
@@ -1430,7 +1454,6 @@ func TestServiceRegistryExternalTrafficGlobal(t *testing.T) {
 	defer server.Terminate(t)
 	svc := svctest.MakeService("external-lb-esipp",
 		svctest.SetTypeLoadBalancer,
-		svctest.SetAllocateLBNodePortTrue,
 		func(s *api.Service) {
 			s.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeCluster
 		},

staging/src/k8s.io/api/core/v1/types.go

@@ -4294,11 +4294,14 @@ type ServiceSpec struct {
 	IPFamilyPolicy *IPFamilyPolicyType `json:"ipFamilyPolicy,omitempty" protobuf:"bytes,17,opt,name=ipFamilyPolicy,casttype=IPFamilyPolicyType"`
 
 	// allocateLoadBalancerNodePorts defines if NodePorts will be automatically
-	// allocated for services with type LoadBalancer.  Default is "true". It may be
-	// set to "false" if the cluster load-balancer does not rely on NodePorts.
-	// allocateLoadBalancerNodePorts may only be set for services with type LoadBalancer
-	// and will be cleared if the type is changed to any other type.
-	// This field is alpha-level and is only honored by servers that enable the ServiceLBNodePortControl feature.
+	// allocated for services with type LoadBalancer.  Default is "true". It
+	// may be set to "false" if the cluster load-balancer does not rely on
+	// NodePorts.  If the caller requests specific NodePorts (by specifying a
+	// value), those requests will be respected, regardless of this field.
+	// This field may only be set for services with type LoadBalancer and will
+	// be cleared if the type is changed to any other type.
+	// This field is beta-level and is only honored by servers that enable the ServiceLBNodePortControl feature.
+	// +featureGate=ServiceLBNodePortControl
 	// +optional
 	AllocateLoadBalancerNodePorts *bool `json:"allocateLoadBalancerNodePorts,omitempty" protobuf:"bytes,20,opt,name=allocateLoadBalancerNodePorts"`