Merge pull request #49258 from xiangpengzhao/fix-dup-port-panic

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Check dup NodePort with protocols when update services

**What this PR does / why we need it**:
As the title says.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #48579 fixes: #54898 fixes: #55327

**Special notes for your reviewer**:
/assign @freehan 
/cc @cblecker 

**Release note**:

```release-note
NONE
```

pkg/registry/core/service/rest.go

@@ -61,6 +61,16 @@ type REST struct {
 	proxyTransport   http.RoundTripper
 }
 
+// ServiceNodePort includes protocol and port number of a service NodePort.
+type ServiceNodePort struct {
+	// The IP protocol for this port. Supports "TCP" and "UDP".
+	Protocol api.Protocol
+
+	// The port on each node on which this service is exposed.
+	// Default is to auto-allocate a port if the ServiceType of this Service requires one.
+	NodePort int32
+}
+
 // NewStorage returns a new REST.
 func NewStorage(registry Registry, endpoints endpoint.Registry, serviceIPs ipallocator.Interface,
 	serviceNodePorts portallocator.Interface, proxyTransport http.RoundTripper) *ServiceRest {
@@ -437,7 +447,7 @@ func (rs *REST) ResourceLocation(ctx genericapirequest.Context, id string) (*url
 
 // This is O(N), but we expect haystack to be small;
 // so small that we expect a linear search to be faster
-func contains(haystack []int, needle int) bool {
+func containsNumber(haystack []int, needle int) bool {
 	for _, v := range haystack {
 		if v == needle {
 			return true
@@ -446,6 +456,17 @@ func contains(haystack []int, needle int) bool {
 	return false
 }
 
+// This is O(N), but we expect serviceNodePorts to be small;
+// so small that we expect a linear search to be faster
+func containsNodePort(serviceNodePorts []ServiceNodePort, serviceNodePort ServiceNodePort) bool {
+	for _, snp := range serviceNodePorts {
+		if snp == serviceNodePort {
+			return true
+		}
+	}
+	return false
+}
+
 func CollectServiceNodePorts(service *api.Service) []int {
 	servicePorts := []int{}
 	for i := range service.Spec.Ports {
@@ -569,44 +590,48 @@ func (rs *REST) initNodePorts(service *api.Service, nodePortOp *portallocator.Po
 }
 
 func (rs *REST) updateNodePorts(oldService, newService *api.Service, nodePortOp *portallocator.PortAllocationOperation) error {
-	oldNodePorts := CollectServiceNodePorts(oldService)
+	oldNodePortsNumbers := CollectServiceNodePorts(oldService)
+	newNodePorts := []ServiceNodePort{}
+	portAllocated := map[int]bool{}
 
-	newNodePorts := []int{}
 	for i := range newService.Spec.Ports {
 		servicePort := &newService.Spec.Ports[i]
-		nodePort := int(servicePort.NodePort)
+		nodePort := ServiceNodePort{Protocol: servicePort.Protocol, NodePort: servicePort.NodePort}
-		if nodePort != 0 {
+		if nodePort.NodePort != 0 {
-			if !contains(oldNodePorts, nodePort) {
+			if !containsNumber(oldNodePortsNumbers, int(nodePort.NodePort)) && !portAllocated[int(nodePort.NodePort)] {
-				err := nodePortOp.Allocate(nodePort)
+				err := nodePortOp.Allocate(int(nodePort.NodePort))
 				if err != nil {
-					el := field.ErrorList{field.Invalid(field.NewPath("spec", "ports").Index(i).Child("nodePort"), nodePort, err.Error())}
+					el := field.ErrorList{field.Invalid(field.NewPath("spec", "ports").Index(i).Child("nodePort"), nodePort.NodePort, err.Error())}
 					return errors.NewInvalid(api.Kind("Service"), newService.Name, el)
 				}
+				portAllocated[int(nodePort.NodePort)] = true
 			}
 		} else {
-			nodePort, err := nodePortOp.AllocateNext()
+			nodePortNumber, err := nodePortOp.AllocateNext()
 			if err != nil {
 				// TODO: what error should be returned here?  It's not a
 				// field-level validation failure (the field is valid), and it's
 				// not really an internal error.
 				return errors.NewInternalError(fmt.Errorf("failed to allocate a nodePort: %v", err))
 			}
-			servicePort.NodePort = int32(nodePort)
+			servicePort.NodePort = int32(nodePortNumber)
+			nodePort.NodePort = servicePort.NodePort
 		}
-		// Detect duplicate node ports; this should have been caught by validation, so we panic
+		if containsNodePort(newNodePorts, nodePort) {
-		if contains(newNodePorts, nodePort) {
+			return fmt.Errorf("duplicate nodePort: %v", nodePort)
-			panic("duplicate node port")
 		}
 		newNodePorts = append(newNodePorts, nodePort)
 	}
 
+	newNodePortsNumbers := CollectServiceNodePorts(newService)
+
 	// The comparison loops are O(N^2), but we don't expect N to be huge
 	// (there's a hard-limit at 2^16, because they're ports; and even 4 ports would be a lot)
-	for _, oldNodePort := range oldNodePorts {
+	for _, oldNodePortNumber := range oldNodePortsNumbers {
-		if contains(newNodePorts, oldNodePort) {
+		if containsNumber(newNodePortsNumbers, oldNodePortNumber) {
 			continue
 		}
-		nodePortOp.ReleaseDeferred(oldNodePort)
+		nodePortOp.ReleaseDeferred(int(oldNodePortNumber))
 	}
 
 	return nil

pkg/registry/core/service/rest_test.go

@@ -1570,6 +1570,92 @@ func TestUpdateNodePorts(t *testing.T) {
 			},
 			expectSpecifiedNodePorts: []int{},
 		},
+		{
+			name: "Add new ServicePort with a different protocol without changing port numbers",
+			oldService: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector:        map[string]string{"bar": "baz"},
+					SessionAffinity: api.ServiceAffinityNone,
+					Type:            api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Name:       "port-tcp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolTCP,
+							NodePort:   30053,
+						},
+					},
+				},
+			},
+			newService: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector:        map[string]string{"bar": "baz"},
+					SessionAffinity: api.ServiceAffinityNone,
+					Type:            api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Name:       "port-tcp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolTCP,
+							NodePort:   30053,
+						},
+						{
+							Name:       "port-udp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolUDP,
+							NodePort:   30053,
+						},
+					},
+				},
+			},
+			expectSpecifiedNodePorts: []int{30053, 30053},
+		},
+		{
+			name: "Change service type from ClusterIP to NodePort with same NodePort number but different protocols",
+			oldService: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector:        map[string]string{"bar": "baz"},
+					SessionAffinity: api.ServiceAffinityNone,
+					Type:            api.ServiceTypeClusterIP,
+					Ports: []api.ServicePort{{
+						Port:       53,
+						Protocol:   api.ProtocolTCP,
+						TargetPort: intstr.FromInt(6502),
+					}},
+				},
+			},
+			newService: &api.Service{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: api.ServiceSpec{
+					Selector:        map[string]string{"bar": "baz"},
+					SessionAffinity: api.ServiceAffinityNone,
+					Type:            api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Name:       "port-tcp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolTCP,
+							NodePort:   30053,
+						},
+						{
+							Name:       "port-udp",
+							Port:       53,
+							TargetPort: intstr.FromInt(6502),
+							Protocol:   api.ProtocolUDP,
+							NodePort:   30053,
+						},
+					},
+				},
+			},
+			expectSpecifiedNodePorts: []int{30053, 30053},
+		},
 	}
 
 	for _, test := range testCases {

test/e2e/network/service.go

@@ -808,6 +808,53 @@ var _ = SIGDescribe("Services", func() {
 		}
 	})
 
+	It("should be able to update NodePorts with two same port numbers but different protocols", func() {
+		serviceName := "nodeport-update-service"
+		ns := f.Namespace.Name
+		jig := framework.NewServiceTestJig(cs, serviceName)
+
+		By("creating a TCP service " + serviceName + " with type=ClusterIP in namespace " + ns)
+		tcpService := jig.CreateTCPServiceOrFail(ns, nil)
+		defer func() {
+			framework.Logf("Cleaning up the updating NodePorts test service")
+			err := cs.Core().Services(ns).Delete(serviceName, nil)
+			Expect(err).NotTo(HaveOccurred())
+		}()
+		jig.SanityCheckService(tcpService, v1.ServiceTypeClusterIP)
+		svcPort := int(tcpService.Spec.Ports[0].Port)
+		framework.Logf("service port TCP: %d", svcPort)
+
+		// Change the services to NodePort and add a UDP port.
+
+		By("changing the TCP service to type=NodePort and add a UDP port")
+		newService := jig.UpdateServiceOrFail(ns, tcpService.Name, func(s *v1.Service) {
+			s.Spec.Type = v1.ServiceTypeNodePort
+			s.Spec.Ports = []v1.ServicePort{
+				{
+					Name:     "tcp-port",
+					Port:     80,
+					Protocol: v1.ProtocolTCP,
+				},
+				{
+					Name:     "udp-port",
+					Port:     80,
+					Protocol: v1.ProtocolUDP,
+				},
+			}
+		})
+		jig.SanityCheckService(newService, v1.ServiceTypeNodePort)
+		if len(newService.Spec.Ports) != 2 {
+			framework.Failf("new service should have two Ports")
+		}
+		for _, port := range newService.Spec.Ports {
+			if port.NodePort == 0 {
+				framework.Failf("new service failed to allocate NodePort for Port %s", port.Name)
+			}
+
+			framework.Logf("new service allocates NodePort %d for Port %s", port.NodePort, port.Name)
+		}
+	})
+
 	It("should be able to change the type from ExternalName to ClusterIP", func() {
 		serviceName := "externalname-service"
 		ns := f.Namespace.Name