From 73503a49361031a854267dc211f361f4eeda9b9e Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 25 Aug 2021 22:21:16 -0700 Subject: [PATCH] Fix a small regression in Service updates Prior to 1.22 a user could change NodePort values within a service during an update, and the apiserver would allocate values for any that were not specified. Consider a YAML like: ``` apiVersion: v1 kind: Service metadata: name: foo spec: type: NodePort ports: - name: p port: 80 - name: q port: 81 selector: app: foo ``` When this is created, nodeport values will be allocated for each port. Something like: ``` apiVersion: v1 kind: Service metadata: name: foo spec: clusterIP: 10.0.149.11 type: NodePort ports: - name: p nodePort: 30872 port: 80 protocol: TCP targetPort: 9376 - name: q nodePort: 31310 port: 81 protocol: TCP targetPort: 81 selector: app: foo ``` If the user PUTs (kubectl replace) the original YAML, we would see that `.nodePort = 0`, and allocate new ports. This was ugly at best. In 1.22 we fixed this to not allocate new values if we still had the old values, but instead re-assign them. Net new ports would still be seen as `.nodePort = 0` and so new allocations would be made. This broke a corner case as follows: Prior to 1.22, the user could PUT this YAML: ``` apiVersion: v1 kind: Service metadata: name: foo spec: type: NodePort ports: - name: p nodePort: 31310 # note this is the `q` value port: 80 - name: q # note this nodePort is not specified port: 81 selector: app: foo ``` The `p` port would take the `q` port's value. The `q` port would be seen as `.nodePort = 0` and a new value allocated. In 1.22 this results in an error (duplicate value in `p` and `q`). This is VERY minor but it is an API regression, which we try to avoid, and the fix is not too horrible. This commit adds more robust testing of this logic. --- pkg/api/service/testing/make.go | 7 + .../core/service/storage/rest_test.go | 233 +++++++++++++++--- pkg/registry/core/service/strategy.go | 22 +- 3 files changed, 225 insertions(+), 37 deletions(-) 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 + } } } }