diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go index 7ba08a78646..056005dac7d 100644 --- a/pkg/registry/core/service/storage/storage.go +++ b/pkg/registry/core/service/storage/storage.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" utilnet "k8s.io/apimachinery/pkg/util/net" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apimachinery/pkg/util/sets" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" @@ -354,7 +355,7 @@ func (r *GenericREST) beginUpdate(ctx context.Context, obj, oldObj runtime.Objec // Fix up allocated values that the client may have not specified (for // idempotence). - svcreg.PatchAllocatedValues(newSvc, oldSvc) + patchAllocatedValues(newSvc, oldSvc) // Make sure ClusterIP and ClusterIPs are in sync. This has to happen // early, before anyone looks at them. @@ -514,3 +515,85 @@ func normalizeClusterIPs(oldSvc, newSvc *api.Service) { } } } + +// 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 && len(oldSvc.Spec.ClusterIPs) > 0 { + newSvc.Spec.ClusterIPs = oldSvc.Spec.ClusterIPs + } + } + + 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{} + for i := range oldSvc.Spec.Ports { + 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 { + oldVal := np[p.Name] + if !used.Has(oldVal) { + p.NodePort = oldVal + } + } + } + } + + if needsHCNodePort(oldSvc) && needsHCNodePort(newSvc) { + if newSvc.Spec.HealthCheckNodePort == 0 { + newSvc.Spec.HealthCheckNodePort = oldSvc.Spec.HealthCheckNodePort + } + } +} + +func needsClusterIP(svc *api.Service) bool { + if svc.Spec.Type == api.ServiceTypeExternalName { + return false + } + return true +} + +func needsNodePort(svc *api.Service) bool { + if svc.Spec.Type == api.ServiceTypeNodePort || svc.Spec.Type == api.ServiceTypeLoadBalancer { + return true + } + return false +} + +func needsHCNodePort(svc *api.Service) bool { + if svc.Spec.Type != api.ServiceTypeLoadBalancer { + return false + } + if svc.Spec.ExternalTrafficPolicy != api.ServiceExternalTrafficPolicyTypeLocal { + return false + } + return true +} diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index 652cb9db10f..2dadd00d68e 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/pkg/registry/core/service/storage/storage_test.go @@ -453,6 +453,126 @@ func TestNormalizeClusterIPs(t *testing.T) { } } +func TestPatchAllocatedValues(t *testing.T) { + testCases := []struct { + name string + before *api.Service + update *api.Service + expectSameClusterIPs bool + expectReducedClusterIPs bool + expectSameNodePort bool + expectSameHCNP bool + }{{ + name: "all_patched", + before: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetClusterIPs("10.0.0.93", "2000::76"), + svctest.SetUniqueNodePorts, + svctest.SetHealthCheckNodePort(31234)), + update: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)), + expectSameClusterIPs: true, + expectSameNodePort: true, + expectSameHCNP: true, + }, { + name: "IPs_patched", + before: svctest.MakeService("foo", + svctest.SetTypeClusterIP, + svctest.SetClusterIPs("10.0.0.93", "2000::76"), + // these are not valid, but prove the test + svctest.SetUniqueNodePorts, + svctest.SetHealthCheckNodePort(31234)), + update: svctest.MakeService("foo", + svctest.SetTypeClusterIP), + expectSameClusterIPs: true, + }, { + name: "NPs_patched", + before: svctest.MakeService("foo", + svctest.SetTypeNodePort, + svctest.SetClusterIPs("10.0.0.93", "2000::76"), + svctest.SetUniqueNodePorts, + // this is not valid, but proves the test + svctest.SetHealthCheckNodePort(31234)), + update: svctest.MakeService("foo", + svctest.SetTypeNodePort, + svctest.SetClusterIPs("10.0.0.93", "2000::76")), + expectSameClusterIPs: true, + expectSameNodePort: true, + }, { + name: "HCNP_patched", + before: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetClusterIPs("10.0.0.93", "2000::76"), + svctest.SetUniqueNodePorts, + svctest.SetHealthCheckNodePort(31234)), + update: svctest.MakeService("foo", + svctest.SetTypeLoadBalancer, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetClusterIPs("10.0.0.93", "2000::76"), + svctest.SetUniqueNodePorts), + expectSameClusterIPs: true, + expectSameNodePort: true, + expectSameHCNP: true, + }, { + name: "nothing_patched", + before: svctest.MakeService("foo", + svctest.SetTypeExternalName, + // these are not valid, but prove the test + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), + svctest.SetClusterIPs("10.0.0.93", "2000::76"), + svctest.SetUniqueNodePorts, + svctest.SetHealthCheckNodePort(31234)), + update: svctest.MakeService("foo", + svctest.SetTypeExternalName, + svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)), + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + update := tc.update.DeepCopy() + patchAllocatedValues(update, tc.before) + + beforeIP := tc.before.Spec.ClusterIP + updateIP := update.Spec.ClusterIP + if tc.expectSameClusterIPs || tc.expectReducedClusterIPs { + if beforeIP != updateIP { + t.Errorf("expected clusterIP to be patched: %q != %q", beforeIP, updateIP) + } + } else if beforeIP == updateIP { + t.Errorf("expected clusterIP to not be patched: %q == %q", beforeIP, updateIP) + } + + beforeIPs := tc.before.Spec.ClusterIPs + updateIPs := update.Spec.ClusterIPs + if tc.expectSameClusterIPs { + if !cmp.Equal(beforeIPs, updateIPs) { + t.Errorf("expected clusterIPs to be patched: %q != %q", beforeIPs, updateIPs) + } + } else if tc.expectReducedClusterIPs { + if len(updateIPs) != 1 || beforeIPs[0] != updateIPs[0] { + t.Errorf("expected clusterIPs to be trim-patched: %q -> %q", beforeIPs, updateIPs) + } + } else if cmp.Equal(beforeIPs, updateIPs) { + t.Errorf("expected clusterIPs to not be patched: %q == %q", beforeIPs, updateIPs) + } + if b, u := tc.before.Spec.Ports[0].NodePort, update.Spec.Ports[0].NodePort; tc.expectSameNodePort && b != u { + t.Errorf("expected nodePort to be patched: %d != %d", b, u) + } else if !tc.expectSameNodePort && b == u { + t.Errorf("expected nodePort to not be patched: %d == %d", b, u) + } + + if b, u := tc.before.Spec.HealthCheckNodePort, update.Spec.HealthCheckNodePort; tc.expectSameHCNP && b != u { + t.Errorf("expected healthCheckNodePort to be patched: %d != %d", b, u) + } else if !tc.expectSameHCNP && b == u { + t.Errorf("expected healthCheckNodePort to not be patched: %d == %d", b, u) + } + }) + } +} + func TestServiceDefaultOnRead(t *testing.T) { // Helper makes a mostly-valid ServiceList. Test-cases can tweak it as needed. makeServiceList := func(tweaks ...svctest.Tweak) *api.ServiceList { diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index b4bd7fa45fd..473757a258d 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -299,64 +299,6 @@ func (serviceStatusStrategy) WarningsOnUpdate(ctx context.Context, obj, old runt 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. -//FIXME: move this to pkg/registry/core/service/storage -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 && len(oldSvc.Spec.ClusterIPs) > 0 { - newSvc.Spec.ClusterIPs = oldSvc.Spec.ClusterIPs - } - } - - 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{} - for i := range oldSvc.Spec.Ports { - 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 { - oldVal := np[p.Name] - if !used.Has(oldVal) { - p.NodePort = oldVal - } - } - } - } - - if needsHCNodePort(oldSvc) && needsHCNodePort(newSvc) { - if newSvc.Spec.HealthCheckNodePort == 0 { - newSvc.Spec.HealthCheckNodePort = oldSvc.Spec.HealthCheckNodePort - } - } -} - func sameStringSlice(a []string, b []string) bool { if len(a) != len(b) { return false diff --git a/pkg/registry/core/service/strategy_test.go b/pkg/registry/core/service/strategy_test.go index 6bb5d8d785d..da9e0d05461 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/pkg/registry/core/service/strategy_test.go @@ -20,7 +20,6 @@ import ( "reflect" "testing" - "github.com/google/go-cmp/cmp" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/diff" @@ -29,7 +28,6 @@ import ( "k8s.io/apiserver/pkg/registry/rest" utilfeature "k8s.io/apiserver/pkg/util/feature" featuregatetesting "k8s.io/component-base/featuregate/testing" - svctest "k8s.io/kubernetes/pkg/api/service/testing" api "k8s.io/kubernetes/pkg/apis/core" _ "k8s.io/kubernetes/pkg/apis/core/install" "k8s.io/kubernetes/pkg/features" @@ -771,124 +769,3 @@ func TestDropTypeDependentFields(t *testing.T) { }) } } - -func TestPatchAllocatedValues(t *testing.T) { - testCases := []struct { - name string - before *api.Service - update *api.Service - expectSameClusterIPs bool - expectReducedClusterIPs bool - expectSameNodePort bool - expectSameHCNP bool - }{{ - name: "all_patched", - before: svctest.MakeService("foo", - svctest.SetTypeLoadBalancer, - svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), - svctest.SetClusterIPs("10.0.0.93", "2000::76"), - svctest.SetUniqueNodePorts, - svctest.SetHealthCheckNodePort(31234)), - update: svctest.MakeService("foo", - svctest.SetTypeLoadBalancer, - svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)), - expectSameClusterIPs: true, - expectSameNodePort: true, - expectSameHCNP: true, - }, { - name: "IPs_patched", - before: svctest.MakeService("foo", - svctest.SetTypeClusterIP, - svctest.SetClusterIPs("10.0.0.93", "2000::76"), - // these are not valid, but prove the test - svctest.SetUniqueNodePorts, - svctest.SetHealthCheckNodePort(31234)), - update: svctest.MakeService("foo", - svctest.SetTypeClusterIP), - expectSameClusterIPs: true, - }, { - name: "NPs_patched", - before: svctest.MakeService("foo", - svctest.SetTypeNodePort, - svctest.SetClusterIPs("10.0.0.93", "2000::76"), - svctest.SetUniqueNodePorts, - // this is not valid, but proves the test - svctest.SetHealthCheckNodePort(31234)), - update: svctest.MakeService("foo", - svctest.SetTypeNodePort, - svctest.SetClusterIPs("10.0.0.93", "2000::76")), - expectSameClusterIPs: true, - expectSameNodePort: true, - }, { - name: "HCNP_patched", - before: svctest.MakeService("foo", - svctest.SetTypeLoadBalancer, - svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), - svctest.SetClusterIPs("10.0.0.93", "2000::76"), - svctest.SetUniqueNodePorts, - svctest.SetHealthCheckNodePort(31234)), - update: svctest.MakeService("foo", - svctest.SetTypeLoadBalancer, - svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), - svctest.SetClusterIPs("10.0.0.93", "2000::76"), - svctest.SetUniqueNodePorts), - expectSameClusterIPs: true, - expectSameNodePort: true, - expectSameHCNP: true, - }, { - name: "nothing_patched", - before: svctest.MakeService("foo", - svctest.SetTypeExternalName, - // these are not valid, but prove the test - svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal), - svctest.SetClusterIPs("10.0.0.93", "2000::76"), - svctest.SetUniqueNodePorts, - svctest.SetHealthCheckNodePort(31234)), - update: svctest.MakeService("foo", - svctest.SetTypeExternalName, - svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)), - }} - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - update := tc.update.DeepCopy() - PatchAllocatedValues(update, tc.before) - - beforeIP := tc.before.Spec.ClusterIP - updateIP := update.Spec.ClusterIP - if tc.expectSameClusterIPs || tc.expectReducedClusterIPs { - if beforeIP != updateIP { - t.Errorf("expected clusterIP to be patched: %q != %q", beforeIP, updateIP) - } - } else if beforeIP == updateIP { - t.Errorf("expected clusterIP to not be patched: %q == %q", beforeIP, updateIP) - } - - beforeIPs := tc.before.Spec.ClusterIPs - updateIPs := update.Spec.ClusterIPs - if tc.expectSameClusterIPs { - if !cmp.Equal(beforeIPs, updateIPs) { - t.Errorf("expected clusterIPs to be patched: %q != %q", beforeIPs, updateIPs) - } - } else if tc.expectReducedClusterIPs { - if len(updateIPs) != 1 || beforeIPs[0] != updateIPs[0] { - t.Errorf("expected clusterIPs to be trim-patched: %q -> %q", beforeIPs, updateIPs) - } - } else if cmp.Equal(beforeIPs, updateIPs) { - t.Errorf("expected clusterIPs to not be patched: %q == %q", beforeIPs, updateIPs) - } - - if b, u := tc.before.Spec.Ports[0].NodePort, update.Spec.Ports[0].NodePort; tc.expectSameNodePort && b != u { - t.Errorf("expected nodePort to be patched: %d != %d", b, u) - } else if !tc.expectSameNodePort && b == u { - t.Errorf("expected nodePort to not be patched: %d == %d", b, u) - } - - if b, u := tc.before.Spec.HealthCheckNodePort, update.Spec.HealthCheckNodePort; tc.expectSameHCNP && b != u { - t.Errorf("expected healthCheckNodePort to be patched: %d != %d", b, u) - } else if !tc.expectSameHCNP && b == u { - t.Errorf("expected healthCheckNodePort to not be patched: %d == %d", b, u) - } - }) - } -}