From c98f046d45d39a1d4e5c3270735b1ecf8b5d0f7c Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 6 Jul 2021 18:05:19 -0700 Subject: [PATCH 1/2] Service: Fix semantics for Update wrt allocations It is not uncommon for users to Create a Service and not specify things like ClusterIP and NodePort, which we then allocate for them. They same that YAML somewhere and later use it again in an Update, but then it fails. That's because we detected them trying to set a ClusterIP from a value to "", which is not allowed. If it was just NodePort, they would actually succeed and reallocate a new port. After this change, we try to "patch" updates where the user did not specify those values from the old object. --- pkg/api/service/testing/make.go | 170 ++++++++++++++++++ .../core/service/storage/rest_test.go | 95 ++++++++++ pkg/registry/core/service/strategy.go | 43 +++++ 3 files changed, 308 insertions(+) create mode 100644 pkg/api/service/testing/make.go diff --git a/pkg/api/service/testing/make.go b/pkg/api/service/testing/make.go new file mode 100644 index 000000000000..3709e784598f --- /dev/null +++ b/pkg/api/service/testing/make.go @@ -0,0 +1,170 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package testing + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + utilpointer "k8s.io/utils/pointer" + + api "k8s.io/kubernetes/pkg/apis/core" +) + +// Tweak is a function that modifies a Service. +type Tweak func(*api.Service) + +// MakeService helps construct Service objects (which pass API validation) more +// legibly and tersely than a Go struct definition. By default this produces +// a ClusterIP service with a single port and a trivial selector. The caller +// can pass any number of tweak functions to further modify the result. +func MakeService(name string, tweaks ...Tweak) *api.Service { + // NOTE: Any field that would be populated by defaulting needs to be + // present and valid here. + svc := &api.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: metav1.NamespaceDefault, + }, + Spec: api.ServiceSpec{ + Selector: map[string]string{"k": "v"}, + SessionAffinity: api.ServiceAffinityNone, + }, + } + // Default to ClusterIP + SetTypeClusterIP(svc) + // Default to 1 port + SetPorts(MakeServicePort("", 93, intstr.FromInt(76), api.ProtocolTCP))(svc) + // Default internalTrafficPolicy to "Cluster" + SetInternalTrafficPolicy(api.ServiceInternalTrafficPolicyCluster)(svc) + + for _, tweak := range tweaks { + tweak(svc) + } + + return svc +} + +// SetTypeClusterIP sets the service type to ClusterIP and clears other fields. +func SetTypeClusterIP(svc *api.Service) { + svc.Spec.Type = api.ServiceTypeClusterIP + for i := range svc.Spec.Ports { + svc.Spec.Ports[i].NodePort = 0 + } + svc.Spec.ExternalName = "" + svc.Spec.ExternalTrafficPolicy = "" + svc.Spec.AllocateLoadBalancerNodePorts = nil +} + +// SetTypeNodePort sets the service type to NodePort and clears other fields. +func SetTypeNodePort(svc *api.Service) { + svc.Spec.Type = api.ServiceTypeNodePort + svc.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeCluster + svc.Spec.ExternalName = "" + svc.Spec.AllocateLoadBalancerNodePorts = nil +} + +// SetTypeLoadBalancer sets the service type to LoadBalancer and clears other fields. +func SetTypeLoadBalancer(svc *api.Service) { + svc.Spec.Type = api.ServiceTypeLoadBalancer + svc.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeCluster + svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(true) + svc.Spec.ExternalName = "" +} + +// SetTypeExternalName sets the service type to ExternalName and clears other fields. +func SetTypeExternalName(svc *api.Service) { + svc.Spec.Type = api.ServiceTypeExternalName + svc.Spec.ExternalName = "example.com" + svc.Spec.ExternalTrafficPolicy = "" + svc.Spec.ClusterIP = "" + svc.Spec.ClusterIPs = nil + svc.Spec.AllocateLoadBalancerNodePorts = nil +} + +// SetPorts sets the service ports list. +func SetPorts(ports ...api.ServicePort) Tweak { + return func(svc *api.Service) { + svc.Spec.Ports = ports + } +} + +// MakeServicePort helps construct ServicePort objects which pass API +// validation. +func MakeServicePort(name string, port int, tgtPort intstr.IntOrString, proto api.Protocol) api.ServicePort { + return api.ServicePort{ + Name: name, + Port: int32(port), + TargetPort: tgtPort, + Protocol: proto, + } +} + +// SetClusterIPs sets the service ClusterIP and ClusterIPs fields. +func SetClusterIPs(ips ...string) Tweak { + return func(svc *api.Service) { + svc.Spec.ClusterIP = ips[0] + svc.Spec.ClusterIPs = ips + } +} + +// SetIPFamilies sets the service IPFamilies field. +func SetIPFamilies(families ...api.IPFamily) Tweak { + return func(svc *api.Service) { + svc.Spec.IPFamilies = families + } +} + +// SetIPFamilyPolicy sets the service IPFamilyPolicy field. +func SetIPFamilyPolicy(policy api.IPFamilyPolicyType) Tweak { + return func(svc *api.Service) { + svc.Spec.IPFamilyPolicy = &policy + } +} + +// SetNodePorts sets the values for each node port, in order. If less values +// are specified than there are ports, the rest are untouched. +func SetNodePorts(values ...int) Tweak { + return func(svc *api.Service) { + for i := range svc.Spec.Ports { + if i >= len(values) { + break + } + svc.Spec.Ports[i].NodePort = int32(values[i]) + } + } +} + +// SetInternalTrafficPolicy sets the internalTrafficPolicy field for a Service. +func SetInternalTrafficPolicy(policy api.ServiceInternalTrafficPolicyType) Tweak { + return func(svc *api.Service) { + svc.Spec.InternalTrafficPolicy = &policy + } +} + +// SetExternalTrafficPolicy sets the externalTrafficPolicy field for a Service. +func SetExternalTrafficPolicy(policy api.ServiceExternalTrafficPolicyType) Tweak { + return func(svc *api.Service) { + svc.Spec.ExternalTrafficPolicy = policy + } +} + +// SetAllocateLoadBalancerNodePorts sets the allocate LB node port field. +func SetAllocateLoadBalancerNodePorts(val bool) Tweak { + return func(svc *api.Service) { + svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(val) + } +} diff --git a/pkg/registry/core/service/storage/rest_test.go b/pkg/registry/core/service/storage/rest_test.go index 6d353370eb2e..8cc5b651c5bc 100644 --- a/pkg/registry/core/service/storage/rest_test.go +++ b/pkg/registry/core/service/storage/rest_test.go @@ -38,6 +38,7 @@ import ( "k8s.io/apiserver/pkg/util/dryrun" "k8s.io/kubernetes/pkg/api/service" + svctest "k8s.io/kubernetes/pkg/api/service/testing" api "k8s.io/kubernetes/pkg/apis/core" endpointstore "k8s.io/kubernetes/pkg/registry/core/endpoint/storage" podstore "k8s.io/kubernetes/pkg/registry/core/pod/storage" @@ -898,6 +899,100 @@ func TestServiceRegistryUpdate(t *testing.T) { } } +func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) { + testCases := []struct { + name string + svc *api.Service // Need a clusterIP, NodePort, and HealthCheckNodePort allocated + tweak func(*api.Service) + }{{ + name: "single-port", + svc: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)), + tweak: nil, + }, { + name: "multi-port", + svc: 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: nil, + }, { + name: "shuffle-ports", + svc: 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] + }, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := genericapirequest.NewDefaultContext() + storage, _, server := NewTestREST(t, nil, []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol}) + + defer server.Terminate(t) + + svc := tc.svc.DeepCopy() + obj, err := storage.Create(ctx, svc.DeepCopy(), rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Expected no error: %v", err) + } + createdSvc := obj.(*api.Service) + if createdSvc.Spec.ClusterIP == "" { + t.Fatalf("expected ClusterIP to be set") + } + if len(createdSvc.Spec.ClusterIPs) == 0 { + t.Fatalf("expected ClusterIPs to be set") + } + for i := range createdSvc.Spec.Ports { + if createdSvc.Spec.Ports[i].NodePort == 0 { + t.Fatalf("expected NodePort[%d] to be set", i) + } + } + if createdSvc.Spec.HealthCheckNodePort == 0 { + t.Fatalf("expected HealthCheckNodePort to be set") + } + + // Update from the original object - just change the selector. + svc.Spec.Selector = map[string]string{"bar": "baz2"} + svc.ResourceVersion = "1" + + obj, _, err = storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(svc.DeepCopy()), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err != nil { + t.Fatalf("Expected no error: %v", err) + } + updatedSvc := obj.(*api.Service) + + if want, got := createdSvc.Spec.ClusterIP, updatedSvc.Spec.ClusterIP; want != got { + t.Errorf("expected ClusterIP to not change: wanted %v, got %v", want, got) + } + 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) + } + }) + } +} + func TestServiceRegistryUpdateDryRun(t *testing.T) { ctx := genericapirequest.NewDefaultContext() diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index db60cc2290f6..7c5348a74e02 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -119,6 +119,7 @@ func (strategy svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runti oldService := old.(*api.Service) newService.Status = oldService.Status + patchAllocatedValues(newService, oldService) NormalizeClusterIPs(oldService, newService) dropServiceDisabledFields(newService, oldService) dropTypeDependentFields(newService, oldService) @@ -302,6 +303,48 @@ func (serviceStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtim return validation.ValidateServiceStatusUpdate(obj.(*api.Service), old.(*api.Service)) } +// WarningsOnUpdate returns warnings for the given update. +func (serviceStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runtime.Object) []string { + return nil +} + +// patchAllocatedValues allows clients to avoid a read-modify-write cycle while +// preserving values that we allocated on their behalf. For example, they +// might create a Service without specifying the ClusterIP, in which case we +// allocate one. If they resubmit that same YAML, we want it to succeed. +func patchAllocatedValues(newSvc, oldSvc *api.Service) { + if needsClusterIP(oldSvc) && needsClusterIP(newSvc) { + if newSvc.Spec.ClusterIP == "" { + newSvc.Spec.ClusterIP = oldSvc.Spec.ClusterIP + } + if len(newSvc.Spec.ClusterIPs) == 0 { + newSvc.Spec.ClusterIPs = oldSvc.Spec.ClusterIPs + } + } + + if needsNodePort(oldSvc) && needsNodePort(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{} + for i := range oldSvc.Spec.Ports { + p := &oldSvc.Spec.Ports[i] + np[p.Name] = p.NodePort + } + for i := range newSvc.Spec.Ports { + p := &newSvc.Spec.Ports[i] + if p.NodePort == 0 { + p.NodePort = np[p.Name] + } + } + } + + if needsHCNodePort(oldSvc) && needsHCNodePort(newSvc) { + if newSvc.Spec.HealthCheckNodePort == 0 { + newSvc.Spec.HealthCheckNodePort = oldSvc.Spec.HealthCheckNodePort + } + } +} + // NormalizeClusterIPs adjust clusterIPs based on ClusterIP. This must not // consider any other fields. func NormalizeClusterIPs(oldSvc, newSvc *api.Service) { From 7ef53aeec381a99b57d14392df554ea1e49f6f9e Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 25 Aug 2021 22:21:16 -0700 Subject: [PATCH 2/2] 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 | 232 +++++++++++++++--- pkg/registry/core/service/strategy.go | 22 +- 3 files changed, 224 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 8cc5b651c5bc..86b2f6e42278 100644 --- a/pkg/registry/core/service/storage/rest_test.go +++ b/pkg/registry/core/service/storage/rest_test.go @@ -900,49 +900,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", 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.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: "shuffle-ports", - svc: svctest.MakeService("foo", + 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: 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.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", 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, nil, []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 == "" { @@ -960,13 +1121,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 = "1" - 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) @@ -976,18 +1145,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 7c5348a74e02..1f1fa0771101 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -323,6 +323,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{} @@ -330,10 +344,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 + } } } }