Merge pull request #104601 from thockin/patchAllocatedValues_port_reuse

Fix a small regression in Service updates

pkg/api/service/testing/make.go

@@ -168,3 +168,10 @@ func SetAllocateLoadBalancerNodePorts(val bool) Tweak {
 		svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(val)
 	}
 }
+
+// SetHealthCheckNodePort sets the healthCheckNodePort field for a Service.
+func SetHealthCheckNodePort(value int32) Tweak {
+	return func(svc *api.Service) {
+		svc.Spec.HealthCheckNodePort = value
+	}
+}

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

@@ -622,48 +622,210 @@ func TestServiceRegistryUpdate(t *testing.T) {
 }
 
 func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
+	type proof func(t *testing.T, s *api.Service)
+	prove := func(proofs ...proof) []proof {
+		return proofs
+	}
+	proveClusterIP := func(idx int, ip string) proof {
+		return func(t *testing.T, s *api.Service) {
+			if want, got := ip, s.Spec.ClusterIPs[idx]; want != got {
+				t.Errorf("wrong ClusterIPs[%d]: want %q, got %q", idx, want, got)
+			}
+		}
+	}
+	proveNodePort := func(idx int, port int32) proof {
+		return func(t *testing.T, s *api.Service) {
+			got := s.Spec.Ports[idx].NodePort
+			if port > 0 && got != port {
+				t.Errorf("wrong Ports[%d].NodePort: want %d, got %d", idx, port, got)
+			} else if port < 0 && got == -port {
+				t.Errorf("wrong Ports[%d].NodePort: wanted anything but %d", idx, got)
+			}
+		}
+	}
+	proveHCNP := func(port int32) proof {
+		return func(t *testing.T, s *api.Service) {
+			got := s.Spec.HealthCheckNodePort
+			if port > 0 && got != port {
+				t.Errorf("wrong HealthCheckNodePort: want %d, got %d", port, got)
+			} else if port < 0 && got == -port {
+				t.Errorf("wrong HealthCheckNodePort: wanted anything but %d", got)
+			}
+		}
+	}
+
 	testCases := []struct {
-		name  string
+		name        string
-		svc   *api.Service // Need a clusterIP, NodePort, and HealthCheckNodePort allocated
+		create      *api.Service // Needs clusterIP, NodePort, and HealthCheckNodePort allocated
-		tweak func(*api.Service)
+		update      *api.Service // Needs clusterIP, NodePort, and/or HealthCheckNodePort blank
+		expectError bool
+		prove       []proof
 	}{{
-		name: "single-port",
+		name: "single-ip_single-port",
-		svc: svctest.MakeService("foo",
+		create: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetClusterIPs("1.2.3.4"),
+			svctest.SetNodePorts(30093),
+			svctest.SetHealthCheckNodePort(30118)),
+		update: svctest.MakeService("foo",
 			svctest.SetTypeLoadBalancer,
 			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
-		tweak: nil,
+		prove: prove(
+			proveClusterIP(0, "1.2.3.4"),
+			proveNodePort(0, 30093),
+			proveHCNP(30118)),
 	}, {
-		name: "multi-port",
+		name: "multi-ip_multi-port",
-		svc: svctest.MakeService("foo",
+		create: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetClusterIPs("1.2.3.4", "2000::1"),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30093, 30076),
+			svctest.SetHealthCheckNodePort(30118)),
+		update: svctest.MakeService("foo",
 			svctest.SetTypeLoadBalancer,
 			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
 			svctest.SetPorts(
-				svctest.MakeServicePort("p", 80, intstr.FromInt(80), api.ProtocolTCP),
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
-				svctest.MakeServicePort("q", 443, intstr.FromInt(443), api.ProtocolTCP))),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP))),
-		tweak: nil,
+		prove: prove(
+			proveClusterIP(0, "1.2.3.4"),
+			proveClusterIP(1, "2000::1"),
+			proveNodePort(0, 30093),
+			proveNodePort(1, 30076),
+			proveHCNP(30118)),
 	}, {
-		name: "shuffle-ports",
+		name: "multi-ip_partial",
-		svc: svctest.MakeService("foo",
+		create: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetClusterIPs("1.2.3.4", "2000::1"),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30093, 30076),
+			svctest.SetHealthCheckNodePort(30118)),
+		update: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetClusterIPs("1.2.3.4")),
+		expectError: true,
+	}, {
+		name: "multi-port_partial",
+		create: svctest.MakeService("foo",
 			svctest.SetTypeLoadBalancer,
 			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
 			svctest.SetPorts(
-				svctest.MakeServicePort("p", 80, intstr.FromInt(80), api.ProtocolTCP),
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
-				svctest.MakeServicePort("q", 443, intstr.FromInt(443), api.ProtocolTCP))),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
-		tweak: func(s *api.Service) {
+			svctest.SetNodePorts(30093, 30076),
-			s.Spec.Ports[0], s.Spec.Ports[1] = s.Spec.Ports[1], s.Spec.Ports[0]
+			svctest.SetHealthCheckNodePort(30118)),
-		},
+		update: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30093, 0)), // provide just 1 value
+		prove: prove(
+			proveNodePort(0, 30093),
+			proveNodePort(1, 30076),
+			proveHCNP(30118)),
+	}, {
+		name: "swap-ports",
+		create: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30093, 30076),
+			svctest.SetHealthCheckNodePort(30118)),
+		update: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetPorts(
+				// swapped from above
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP),
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP))),
+		prove: prove(
+			proveNodePort(0, 30076),
+			proveNodePort(1, 30093),
+			proveHCNP(30118)),
+	}, {
+		name: "partial-swap-ports",
+		create: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30093, 30076),
+			svctest.SetHealthCheckNodePort(30118)),
+		update: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30076, 0), // set [0] to [1], omit [1]
+			svctest.SetHealthCheckNodePort(30118)),
+		prove: prove(
+			proveNodePort(0, 30076),
+			proveNodePort(1, -30076),
+			proveHCNP(30118)),
+	}, {
+		name: "swap-port-with-hcnp",
+		create: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30093, 30076),
+			svctest.SetHealthCheckNodePort(30118)),
+		update: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30076, 30118)), // set [0] to [1], set [1] to HCNP
+		expectError: true,
+	}, {
+		name: "partial-swap-port-with-hcnp",
+		create: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30093, 30076),
+			svctest.SetHealthCheckNodePort(30118)),
+		update: svctest.MakeService("foo",
+			svctest.SetTypeLoadBalancer,
+			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetPorts(
+				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
+				svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
+			svctest.SetNodePorts(30118, 0)), // set [0] to HCNP, omit [1]
+		expectError: true,
 	}}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			ctx := genericapirequest.NewDefaultContext()
-			storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})
+			storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol})
 			defer server.Terminate(t)
 
-			svc := tc.svc.DeepCopy()
+			svc := tc.create.DeepCopy()
 			obj, err := storage.Create(ctx, svc.DeepCopy(), rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
 			if err != nil {
-				t.Fatalf("Expected no error: %v", err)
+				t.Fatalf("unexpected error on create: %v", err)
 			}
 			createdSvc := obj.(*api.Service)
 			if createdSvc.Spec.ClusterIP == "" {
@@ -681,13 +843,21 @@ func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
 				t.Fatalf("expected HealthCheckNodePort to be set")
 			}
 
-			// Update from the original object - just change the selector.
+			// Update - change the selector to be sure.
+			svc = tc.update.DeepCopy()
 			svc.Spec.Selector = map[string]string{"bar": "baz2"}
 			svc.ResourceVersion = createdSvc.ResourceVersion
 
-			obj, _, err = storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(svc.DeepCopy()), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{})
+			obj, _, err = storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(svc.DeepCopy()),
+				rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{})
+			if tc.expectError {
+				if err == nil {
+					t.Fatalf("unexpected success on update")
+				}
+				return
+			}
 			if err != nil {
-				t.Fatalf("Expected no error: %v", err)
+				t.Fatalf("unexpected error on update: %v", err)
 			}
 			updatedSvc := obj.(*api.Service)
 
@@ -697,18 +867,9 @@ func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
 			if want, got := createdSvc.Spec.ClusterIPs, updatedSvc.Spec.ClusterIPs; !reflect.DeepEqual(want, got) {
 				t.Errorf("expected ClusterIPs to not change: wanted %v, got %v", want, got)
 			}
-			portmap := func(s *api.Service) map[string]int32 {
+
-				ret := map[string]int32{}
+			for _, proof := range tc.prove {
-				for _, p := range s.Spec.Ports {
+				proof(t, updatedSvc)
-					ret[p.Name] = p.NodePort
-				}
-				return ret
-			}
-			if want, got := portmap(createdSvc), portmap(updatedSvc); !reflect.DeepEqual(want, got) {
-				t.Errorf("expected NodePort to not change: wanted %v, got %v", want, got)
-			}
-			if want, got := createdSvc.Spec.HealthCheckNodePort, updatedSvc.Spec.HealthCheckNodePort; want != got {
-				t.Errorf("expected HealthCheckNodePort to not change: wanted %v, got %v", want, got)
 			}
 		})
 	}

pkg/registry/core/service/strategy.go

@@ -318,6 +318,20 @@ func patchAllocatedValues(newSvc, oldSvc *api.Service) {
 	}
 
 	if needsNodePort(oldSvc) && needsNodePort(newSvc) {
+		nodePortsUsed := func(svc *api.Service) sets.Int32 {
+			used := sets.NewInt32()
+			for _, p := range svc.Spec.Ports {
+				if p.NodePort != 0 {
+					used.Insert(p.NodePort)
+				}
+			}
+			return used
+		}
+
+		// Build a set of all the ports in oldSvc that are also in newSvc.  We know
+		// we can't patch these values.
+		used := nodePortsUsed(oldSvc).Intersection(nodePortsUsed(newSvc))
+
 		// Map NodePorts by name.  The user may have changed other properties
 		// of the port, but we won't see that here.
 		np := map[string]int32{}
@@ -325,10 +339,16 @@ func patchAllocatedValues(newSvc, oldSvc *api.Service) {
 			p := &oldSvc.Spec.Ports[i]
 			np[p.Name] = p.NodePort
 		}
+
+		// If newSvc is missing values, try to patch them in when we know them and
+		// they haven't been used for another port.
 		for i := range newSvc.Spec.Ports {
 			p := &newSvc.Spec.Ports[i]
 			if p.NodePort == 0 {
-				p.NodePort = np[p.Name]
+				oldVal := np[p.Name]
+				if !used.Has(oldVal) {
+					p.NodePort = oldVal
+				}
 			}
 		}
 	}