diff --git a/pkg/api/service/testing/make.go b/pkg/api/service/testing/make.go index 3709e784598f..6d1473247b7c 100644 --- a/pkg/api/service/testing/make.go +++ b/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 + } +} diff --git a/pkg/registry/core/service/storage/rest_test.go b/pkg/registry/core/service/storage/rest_test.go index c05c2bfa2136..c3cb72c4ecfa 100644 --- a/pkg/registry/core/service/storage/rest_test.go +++ b/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 - svc *api.Service // Need a clusterIP, NodePort, and HealthCheckNodePort allocated - tweak func(*api.Service) + name string + create *api.Service // Needs clusterIP, NodePort, and HealthCheckNodePort allocated + update *api.Service // Needs clusterIP, NodePort, and/or HealthCheckNodePort blank + expectError bool + prove []proof }{{ - name: "single-port", - svc: svctest.MakeService("foo", + name: "single-ip_single-port", + 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", - svc: svctest.MakeService("foo", + name: "multi-ip_multi-port", + 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", 867, intstr.FromInt(867), api.ProtocolTCP), + svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP))), + prove: prove( + proveClusterIP(0, "1.2.3.4"), + proveClusterIP(1, "2000::1"), + proveNodePort(0, 30093), + proveNodePort(1, 30076), + proveHCNP(30118)), + }, { + name: "multi-ip_partial", + create: svctest.MakeService("foo", svctest.SetTypeLoadBalancer, svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetClusterIPs("1.2.3.4", "2000::1"), svctest.SetPorts( - svctest.MakeServicePort("p", 80, intstr.FromInt(80), api.ProtocolTCP), - svctest.MakeServicePort("q", 443, intstr.FromInt(443), api.ProtocolTCP))), - tweak: nil, + 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: "shuffle-ports", - svc: svctest.MakeService("foo", + 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("q", 443, intstr.FromInt(443), api.ProtocolTCP))), - tweak: func(s *api.Service) { - s.Spec.Ports[0], s.Spec.Ports[1] = s.Spec.Ports[1], s.Spec.Ports[0] - }, + 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(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 _, p := range s.Spec.Ports { - 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) + + for _, proof := range tc.prove { + proof(t, updatedSvc) } }) } diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index 6e3553acd381..53fe07cdbec0 100644 --- a/pkg/registry/core/service/strategy.go +++ b/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 + } } } }