Remove shouldAssignNodePorts logic in initNodePort; add test cases.

pkg/registry/core/service/rest.go

@@ -106,7 +106,7 @@ func (rs *REST) Create(ctx genericapirequest.Context, obj runtime.Object, includ
 	defer nodePortOp.Finish()
 
 	if service.Spec.Type == api.ServiceTypeNodePort || service.Spec.Type == api.ServiceTypeLoadBalancer {
-		if err := rs.initNodePort(service, nodePortOp); err != nil {
+		if err := rs.initNodePorts(service, nodePortOp); err != nil {
 			return nil, err
 		}
 	}
@@ -333,11 +333,11 @@ func (rs *REST) Update(ctx genericapirequest.Context, name string, objInfo rest.
 	// Update service from NodePort or LoadBalancer to ExternalName or ClusterIP, should release NodePort if exists.
 	if (oldService.Spec.Type == api.ServiceTypeNodePort || oldService.Spec.Type == api.ServiceTypeLoadBalancer) &&
 		(service.Spec.Type == api.ServiceTypeExternalName || service.Spec.Type == api.ServiceTypeClusterIP) {
-		rs.releaseNodePort(oldService, nodePortOp)
+		rs.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 {
-		if err := rs.updateNodePort(oldService, service, nodePortOp); err != nil {
+		if err := rs.updateNodePorts(oldService, service, nodePortOp); err != nil {
 			return nil, false, err
 		}
 	}
@@ -456,22 +456,6 @@ func CollectServiceNodePorts(service *api.Service) []int {
 	return servicePorts
 }
 
-func shouldAssignNodePorts(service *api.Service) bool {
-	switch service.Spec.Type {
-	case api.ServiceTypeLoadBalancer:
-		return true
-	case api.ServiceTypeNodePort:
-		return true
-	case api.ServiceTypeClusterIP:
-		return false
-	case api.ServiceTypeExternalName:
-		return false
-	default:
-		glog.Errorf("Unknown service type: %v", service.Spec.Type)
-		return false
-	}
-}
-
 // Loop through the service ports list, find one with the same port number and
 // NodePort specified, return this NodePort otherwise return 0.
 func findRequestedNodePort(port int, servicePorts []api.ServicePort) int {
@@ -534,8 +518,7 @@ func (rs *REST) initClusterIP(service *api.Service) (bool, error) {
 	return false, nil
 }
 
-func (rs *REST) initNodePort(service *api.Service, nodePortOp *portallocator.PortAllocationOperation) error {
+func (rs *REST) initNodePorts(service *api.Service, nodePortOp *portallocator.PortAllocationOperation) error {
-	assignNodePorts := shouldAssignNodePorts(service)
 	svcPortToNodePort := map[int]int{}
 	for i := range service.Spec.Ports {
 		servicePort := &service.Spec.Ports[i]
@@ -554,7 +537,7 @@ func (rs *REST) initNodePort(service *api.Service, nodePortOp *portallocator.Por
 				}
 				servicePort.NodePort = int32(np)
 				svcPortToNodePort[int(servicePort.Port)] = np
-			} else if assignNodePorts {
+			} else {
 				nodePort, err := nodePortOp.AllocateNext()
 				if err != nil {
 					// TODO: what error should be returned here?  It's not a
@@ -566,6 +549,8 @@ func (rs *REST) initNodePort(service *api.Service, nodePortOp *portallocator.Por
 				svcPortToNodePort[int(servicePort.Port)] = nodePort
 			}
 		} else if int(servicePort.NodePort) != allocatedNodePort {
+			// TODO(xiangpengzhao): do we need to allocate a new NodePort in this case?
+			// Note: the current implementation is better, because it saves a NodePort.
 			if servicePort.NodePort == 0 {
 				servicePort.NodePort = int32(allocatedNodePort)
 			} else {
@@ -582,7 +567,7 @@ func (rs *REST) initNodePort(service *api.Service, nodePortOp *portallocator.Por
 	return nil
 }
 
-func (rs *REST) updateNodePort(oldService, newService *api.Service, nodePortOp *portallocator.PortAllocationOperation) error {
+func (rs *REST) updateNodePorts(oldService, newService *api.Service, nodePortOp *portallocator.PortAllocationOperation) error {
 	oldNodePorts := CollectServiceNodePorts(oldService)
 
 	newNodePorts := []int{}
@@ -626,7 +611,7 @@ func (rs *REST) updateNodePort(oldService, newService *api.Service, nodePortOp *
 	return nil
 }
 
-func (rs *REST) releaseNodePort(service *api.Service, nodePortOp *portallocator.PortAllocationOperation) {
+func (rs *REST) releaseNodePorts(service *api.Service, nodePortOp *portallocator.PortAllocationOperation) {
 	nodePorts := CollectServiceNodePorts(service)
 
 	for _, nodePort := range nodePorts {

pkg/registry/core/service/rest_test.go

@@ -1362,7 +1362,186 @@ func TestInitClusterIP(t *testing.T) {
 	}
 }
 
-func TestUpdateNodePort(t *testing.T) {
+func TestInitNodePorts(t *testing.T) {
+	storage, _ := NewTestREST(t, nil)
+	nodePortOp := portallocator.StartOperation(storage.serviceNodePorts)
+	defer nodePortOp.Finish()
+
+	testCases := []struct {
+		name                     string
+		service                  *api.Service
+		expectSpecifiedNodePorts []int
+	}{
+		{
+			name: "Service doesn't have specified NodePort",
+			service: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"bar": "baz"},
+					Type:     api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Name:       "port-tcp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolTCP,
+						},
+					},
+				},
+			},
+			expectSpecifiedNodePorts: []int{},
+		},
+		{
+			name: "Service has one specified NodePort",
+			service: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"bar": "baz"},
+					Type:     api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{{
+						Name:       "port-tcp",
+						Port:       53,
+						TargetPort: intstr.FromInt(6502),
+						Protocol:   api.ProtocolTCP,
+						NodePort:   30053,
+					}},
+				},
+			},
+			expectSpecifiedNodePorts: []int{30053},
+		},
+		{
+			name: "Service has two same ports with different protocols and specifies same NodePorts",
+			service: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"bar": "baz"},
+					Type:     api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Name:       "port-tcp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolTCP,
+							NodePort:   30054,
+						},
+						{
+							Name:       "port-udp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolUDP,
+							NodePort:   30054,
+						},
+					},
+				},
+			},
+			expectSpecifiedNodePorts: []int{30054, 30054},
+		},
+		{
+			name: "Service has two same ports with different protocols and specifies different NodePorts",
+			service: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"bar": "baz"},
+					Type:     api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Name:       "port-tcp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolTCP,
+							NodePort:   30055,
+						},
+						{
+							Name:       "port-udp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolUDP,
+							NodePort:   30056,
+						},
+					},
+				},
+			},
+			expectSpecifiedNodePorts: []int{30055, 30056},
+		},
+		{
+			name: "Service has two different ports with different protocols and specifies different NodePorts",
+			service: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"bar": "baz"},
+					Type:     api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Name:       "port-tcp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolTCP,
+							NodePort:   30057,
+						},
+						{
+							Name:       "port-udp",
+							Port:       54,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolUDP,
+							NodePort:   30058,
+						},
+					},
+				},
+			},
+			expectSpecifiedNodePorts: []int{30057, 30058},
+		},
+		{
+			name: "Service has two same ports with different protocols but only specifies one NodePort",
+			service: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector: map[string]string{"bar": "baz"},
+					Type:     api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Name:       "port-tcp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolTCP,
+							NodePort:   30059,
+						},
+						{
+							Name:       "port-udp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolUDP,
+						},
+					},
+				},
+			},
+			expectSpecifiedNodePorts: []int{30059, 30059},
+		},
+	}
+
+	for _, test := range testCases {
+		err := storage.initNodePorts(test.service, nodePortOp)
+		if err != nil {
+			t.Errorf("%q: unexpected error: %v", test.name, err)
+			continue
+		}
+		_ = nodePortOp.Commit()
+
+		serviceNodePorts := CollectServiceNodePorts(test.service)
+
+		if len(test.expectSpecifiedNodePorts) == 0 {
+			for _, nodePort := range serviceNodePorts {
+				if !storage.serviceNodePorts.Has(nodePort) {
+					t.Errorf("%q: unexpected NodePort %d, out of range", test.name, nodePort)
+				}
+			}
+		} else if !reflect.DeepEqual(serviceNodePorts, test.expectSpecifiedNodePorts) {
+			t.Errorf("%q: expected NodePorts %v, but got %v", test.name, test.expectSpecifiedNodePorts, serviceNodePorts)
+		}
+
+	}
+}
+
+func TestUpdateNodePorts(t *testing.T) {
 	storage, _ := NewTestREST(t, nil)
 	nodePortOp := portallocator.StartOperation(storage.serviceNodePorts)
 	defer nodePortOp.Finish()
@@ -1521,7 +1700,7 @@ func TestUpdateNodePort(t *testing.T) {
 	}
 
 	for _, test := range testCases {
-		err := storage.updateNodePort(test.oldService, test.newService, nodePortOp)
+		err := storage.updateNodePorts(test.oldService, test.newService, nodePortOp)
 		if err != nil {
 			t.Errorf("%q: unexpected error: %v", test.name, err)
 			continue