From 4ff4160e34bfb26816e2d997f7e95d9d92e44833 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 23 Aug 2021 19:36:02 -0700 Subject: [PATCH] Svc REST: Move normalizeClusterIPs to storage pkg All the meaningful callers of it are in that pkg anyway. Removes some FIXMEs. --- pkg/registry/core/service/storage/storage.go | 69 +++++++- .../core/service/storage/storage_test.go | 157 +++++++++++++++++ pkg/registry/core/service/strategy.go | 68 -------- pkg/registry/core/service/strategy_test.go | 158 ------------------ 4 files changed, 223 insertions(+), 229 deletions(-) diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go index 70f718b0dcc..7ba08a78646 100644 --- a/pkg/registry/core/service/storage/storage.go +++ b/pkg/registry/core/service/storage/storage.go @@ -222,7 +222,7 @@ func (r *GenericREST) defaultOnReadService(service *api.Service) { // We might find Services that were written before ClusterIP became plural. // We still want to present a consistent view of them. // NOTE: the args are (old, new) - svcreg.NormalizeClusterIPs(nil, service) + normalizeClusterIPs(nil, service) // The rest of this does not apply unless dual-stack is enabled. if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) { @@ -324,7 +324,7 @@ func (r *GenericREST) beginCreate(ctx context.Context, obj runtime.Object, optio // Make sure ClusterIP and ClusterIPs are in sync. This has to happen // early, before anyone looks at them. // NOTE: the args are (old, new) - svcreg.NormalizeClusterIPs(nil, svc) + normalizeClusterIPs(nil, svc) // Allocate IPs and ports. If we had a transactional store, this would just // be part of the larger transaction. We don't have that, so we have to do @@ -359,7 +359,7 @@ func (r *GenericREST) beginUpdate(ctx context.Context, obj, oldObj runtime.Objec // Make sure ClusterIP and ClusterIPs are in sync. This has to happen // early, before anyone looks at them. // NOTE: the args are (old, new) - svcreg.NormalizeClusterIPs(oldSvc, newSvc) + normalizeClusterIPs(oldSvc, newSvc) // Allocate and initialize fields. txn, err := r.alloc.allocateUpdate(newSvc, oldSvc, dryrun.IsDryRun(options.DryRun)) @@ -451,3 +451,66 @@ func (r *GenericREST) ResourceLocation(ctx context.Context, id string) (*url.URL } return nil, nil, errors.NewServiceUnavailable(fmt.Sprintf("no endpoints available for service %q", id)) } + +// normalizeClusterIPs adjust clusterIPs based on ClusterIP. This must not +// consider any other fields. +func normalizeClusterIPs(oldSvc, newSvc *api.Service) { + // In all cases here, we don't need to over-think the inputs. Validation + // will be called on the new object soon enough. All this needs to do is + // try to divine what user meant with these linked fields. The below + // is verbosely written for clarity. + + // **** IMPORTANT ***** + // as a governing rule. User must (either) + // -- Use singular only (old client) + // -- singular and plural fields (new clients) + + if oldSvc == nil { + // This was a create operation. + // User specified singular and not plural (e.g. an old client), so init + // plural for them. + if len(newSvc.Spec.ClusterIP) > 0 && len(newSvc.Spec.ClusterIPs) == 0 { + newSvc.Spec.ClusterIPs = []string{newSvc.Spec.ClusterIP} + return + } + + // we don't init singular based on plural because + // new client must use both fields + + // Either both were not specified (will be allocated) or both were + // specified (will be validated). + return + } + + // This was an update operation + + // ClusterIPs were cleared by an old client which was trying to patch + // some field and didn't provide ClusterIPs + if len(oldSvc.Spec.ClusterIPs) > 0 && len(newSvc.Spec.ClusterIPs) == 0 { + // if ClusterIP is the same, then it is an old client trying to + // patch service and didn't provide ClusterIPs + if oldSvc.Spec.ClusterIP == newSvc.Spec.ClusterIP { + newSvc.Spec.ClusterIPs = oldSvc.Spec.ClusterIPs + } + } + + // clusterIP is not the same + if oldSvc.Spec.ClusterIP != newSvc.Spec.ClusterIP { + // this is a client trying to clear it + if len(oldSvc.Spec.ClusterIP) > 0 && len(newSvc.Spec.ClusterIP) == 0 { + // if clusterIPs are the same, then clear on their behalf + if sameClusterIPs(oldSvc, newSvc) { + newSvc.Spec.ClusterIPs = nil + } + + // if they provided nil, then we are fine (handled by patching case above) + // if they changed it then validation will catch it + } else { + // ClusterIP has changed but not cleared *and* ClusterIPs are the same + // then we set ClusterIPs based on ClusterIP + if sameClusterIPs(oldSvc, newSvc) { + newSvc.Spec.ClusterIPs = []string{newSvc.Spec.ClusterIP} + } + } + } +} diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index bb06ea575aa..652cb9db10f 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/pkg/registry/core/service/storage/storage_test.go @@ -296,6 +296,163 @@ func TestGenericCategories(t *testing.T) { registrytest.AssertCategories(t, storage, expected) } +func TestNormalizeClusterIPs(t *testing.T) { + makeServiceWithClusterIp := func(clusterIP string, clusterIPs []string) *api.Service { + return &api.Service{ + Spec: api.ServiceSpec{ + ClusterIP: clusterIP, + ClusterIPs: clusterIPs, + }, + } + } + + testCases := []struct { + name string + oldService *api.Service + newService *api.Service + expectedClusterIP string + expectedClusterIPs []string + }{ + { + name: "new - only clusterip used", + oldService: nil, + newService: makeServiceWithClusterIp("10.0.0.10", nil), + expectedClusterIP: "10.0.0.10", + expectedClusterIPs: []string{"10.0.0.10"}, + }, + { + name: "new - only clusterips used", + oldService: nil, + newService: makeServiceWithClusterIp("", []string{"10.0.0.10"}), + expectedClusterIP: "", // this is a validation issue, and validation will catch it + expectedClusterIPs: []string{"10.0.0.10"}, + }, + { + name: "new - both used", + oldService: nil, + newService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + expectedClusterIP: "10.0.0.10", + expectedClusterIPs: []string{"10.0.0.10"}, + }, + { + name: "update - no change", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + newService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + expectedClusterIP: "10.0.0.10", + expectedClusterIPs: []string{"10.0.0.10"}, + }, + { + name: "update - malformed change", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + newService: makeServiceWithClusterIp("10.0.0.11", []string{"10.0.0.11"}), + expectedClusterIP: "10.0.0.11", + expectedClusterIPs: []string{"10.0.0.11"}, + }, + { + name: "update - malformed change on secondary ip", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), + newService: makeServiceWithClusterIp("10.0.0.11", []string{"10.0.0.11", "3000::1"}), + expectedClusterIP: "10.0.0.11", + expectedClusterIPs: []string{"10.0.0.11", "3000::1"}, + }, + { + name: "update - upgrade", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + newService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), + expectedClusterIP: "10.0.0.10", + expectedClusterIPs: []string{"10.0.0.10", "2000::1"}, + }, + { + name: "update - downgrade", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), + newService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + expectedClusterIP: "10.0.0.10", + expectedClusterIPs: []string{"10.0.0.10"}, + }, + { + name: "update - user cleared cluster IP", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + newService: makeServiceWithClusterIp("", []string{"10.0.0.10"}), + expectedClusterIP: "", + expectedClusterIPs: nil, + }, + { + name: "update - user cleared clusterIPs", // *MUST* REMAIN FOR OLD CLIENTS + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + newService: makeServiceWithClusterIp("10.0.0.10", nil), + expectedClusterIP: "10.0.0.10", + expectedClusterIPs: []string{"10.0.0.10"}, + }, + { + name: "update - user cleared both", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + newService: makeServiceWithClusterIp("", nil), + expectedClusterIP: "", + expectedClusterIPs: nil, + }, + { + name: "update - user cleared ClusterIP but changed clusterIPs", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + newService: makeServiceWithClusterIp("", []string{"10.0.0.11"}), + expectedClusterIP: "", /* validation catches this */ + expectedClusterIPs: []string{"10.0.0.11"}, + }, + { + name: "update - user cleared ClusterIPs but changed ClusterIP", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), + newService: makeServiceWithClusterIp("10.0.0.11", nil), + expectedClusterIP: "10.0.0.11", + expectedClusterIPs: nil, + }, + { + name: "update - user changed from None to ClusterIP", + oldService: makeServiceWithClusterIp("None", []string{"None"}), + newService: makeServiceWithClusterIp("10.0.0.10", []string{"None"}), + expectedClusterIP: "10.0.0.10", + expectedClusterIPs: []string{"10.0.0.10"}, + }, + { + name: "update - user changed from ClusterIP to None", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), + newService: makeServiceWithClusterIp("None", []string{"10.0.0.10"}), + expectedClusterIP: "None", + expectedClusterIPs: []string{"None"}, + }, + { + name: "update - user changed from ClusterIP to None and changed ClusterIPs in a dual stack (new client making a mistake)", + oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), + newService: makeServiceWithClusterIp("None", []string{"10.0.0.11", "2000::1"}), + expectedClusterIP: "None", + expectedClusterIPs: []string{"10.0.0.11", "2000::1"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + normalizeClusterIPs(tc.oldService, tc.newService) + + if tc.newService == nil { + t.Fatalf("unexpected new service to be nil") + } + + if tc.newService.Spec.ClusterIP != tc.expectedClusterIP { + t.Fatalf("expected clusterIP [%v] got [%v]", tc.expectedClusterIP, tc.newService.Spec.ClusterIP) + } + + if len(tc.newService.Spec.ClusterIPs) != len(tc.expectedClusterIPs) { + t.Fatalf("expected clusterIPs %v got %v", tc.expectedClusterIPs, tc.newService.Spec.ClusterIPs) + } + + for idx, clusterIP := range tc.newService.Spec.ClusterIPs { + if clusterIP != tc.expectedClusterIPs[idx] { + t.Fatalf("expected clusterIP [%v] at index[%v] got [%v]", tc.expectedClusterIPs[idx], idx, tc.newService.Spec.ClusterIPs[idx]) + + } + } + }) + } +} + 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 346cf2db6eb..b4bd7fa45fd 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -109,8 +109,6 @@ func (strategy svcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Ob service := obj.(*api.Service) service.Status = api.ServiceStatus{} - //FIXME: Normalize is now called from BeginCreate in pkg/registry/core/service/storage - NormalizeClusterIPs(nil, service) dropServiceDisabledFields(service, nil) } @@ -120,8 +118,6 @@ func (strategy svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runti oldService := old.(*api.Service) newService.Status = oldService.Status - //FIXME: Normalize is now called from BeginUpdate in pkg/registry/core/service/storage - NormalizeClusterIPs(oldService, newService) dropServiceDisabledFields(newService, oldService) dropTypeDependentFields(newService, oldService) } @@ -361,70 +357,6 @@ func PatchAllocatedValues(newSvc, oldSvc *api.Service) { } } -// NormalizeClusterIPs adjust clusterIPs based on ClusterIP. This must not -// consider any other fields. -//FIXME: move this to pkg/registry/core/service/storage -func NormalizeClusterIPs(oldSvc, newSvc *api.Service) { - // In all cases here, we don't need to over-think the inputs. Validation - // will be called on the new object soon enough. All this needs to do is - // try to divine what user meant with these linked fields. The below - // is verbosely written for clarity. - - // **** IMPORTANT ***** - // as a governing rule. User must (either) - // -- Use singular only (old client) - // -- singular and plural fields (new clients) - - if oldSvc == nil { - // This was a create operation. - // User specified singular and not plural (e.g. an old client), so init - // plural for them. - if len(newSvc.Spec.ClusterIP) > 0 && len(newSvc.Spec.ClusterIPs) == 0 { - newSvc.Spec.ClusterIPs = []string{newSvc.Spec.ClusterIP} - return - } - - // we don't init singular based on plural because - // new client must use both fields - - // Either both were not specified (will be allocated) or both were - // specified (will be validated). - return - } - - // This was an update operation - - // ClusterIPs were cleared by an old client which was trying to patch - // some field and didn't provide ClusterIPs - if len(oldSvc.Spec.ClusterIPs) > 0 && len(newSvc.Spec.ClusterIPs) == 0 { - // if ClusterIP is the same, then it is an old client trying to - // patch service and didn't provide ClusterIPs - if oldSvc.Spec.ClusterIP == newSvc.Spec.ClusterIP { - newSvc.Spec.ClusterIPs = oldSvc.Spec.ClusterIPs - } - } - - // clusterIP is not the same - if oldSvc.Spec.ClusterIP != newSvc.Spec.ClusterIP { - // this is a client trying to clear it - if len(oldSvc.Spec.ClusterIP) > 0 && len(newSvc.Spec.ClusterIP) == 0 { - // if clusterIPs are the same, then clear on their behalf - if sameStringSlice(oldSvc.Spec.ClusterIPs, newSvc.Spec.ClusterIPs) { - newSvc.Spec.ClusterIPs = nil - } - - // if they provided nil, then we are fine (handled by patching case above) - // if they changed it then validation will catch it - } else { - // ClusterIP has changed but not cleared *and* ClusterIPs are the same - // then we set ClusterIPs based on ClusterIP - if sameStringSlice(oldSvc.Spec.ClusterIPs, newSvc.Spec.ClusterIPs) { - newSvc.Spec.ClusterIPs = []string{newSvc.Spec.ClusterIP} - } - } - } -} - 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 0c9a0f31db2..6bb5d8d785d 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/pkg/registry/core/service/strategy_test.go @@ -115,15 +115,6 @@ func makeValidServiceCustom(tweaks ...func(svc *api.Service)) *api.Service { return svc } -func makeServiceWithClusterIp(clusterIP string, clusterIPs []string) *api.Service { - return &api.Service{ - Spec: api.ServiceSpec{ - ClusterIP: clusterIP, - ClusterIPs: clusterIPs, - }, - } -} - func TestServiceStatusStrategy(t *testing.T) { _, testStatusStrategy := newStrategy("10.0.0.0/16", false) ctx := genericapirequest.NewDefaultContext() @@ -496,155 +487,6 @@ func TestDropDisabledField(t *testing.T) { } -func TestNormalizeClusterIPs(t *testing.T) { - testCases := []struct { - name string - oldService *api.Service - newService *api.Service - expectedClusterIP string - expectedClusterIPs []string - }{ - { - name: "new - only clusterip used", - oldService: nil, - newService: makeServiceWithClusterIp("10.0.0.10", nil), - expectedClusterIP: "10.0.0.10", - expectedClusterIPs: []string{"10.0.0.10"}, - }, - { - name: "new - only clusterips used", - oldService: nil, - newService: makeServiceWithClusterIp("", []string{"10.0.0.10"}), - expectedClusterIP: "", // this is a validation issue, and validation will catch it - expectedClusterIPs: []string{"10.0.0.10"}, - }, - { - name: "new - both used", - oldService: nil, - newService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - expectedClusterIP: "10.0.0.10", - expectedClusterIPs: []string{"10.0.0.10"}, - }, - { - name: "update - no change", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - newService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - expectedClusterIP: "10.0.0.10", - expectedClusterIPs: []string{"10.0.0.10"}, - }, - { - name: "update - malformed change", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - newService: makeServiceWithClusterIp("10.0.0.11", []string{"10.0.0.11"}), - expectedClusterIP: "10.0.0.11", - expectedClusterIPs: []string{"10.0.0.11"}, - }, - { - name: "update - malformed change on secondary ip", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), - newService: makeServiceWithClusterIp("10.0.0.11", []string{"10.0.0.11", "3000::1"}), - expectedClusterIP: "10.0.0.11", - expectedClusterIPs: []string{"10.0.0.11", "3000::1"}, - }, - { - name: "update - upgrade", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - newService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), - expectedClusterIP: "10.0.0.10", - expectedClusterIPs: []string{"10.0.0.10", "2000::1"}, - }, - { - name: "update - downgrade", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), - newService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - expectedClusterIP: "10.0.0.10", - expectedClusterIPs: []string{"10.0.0.10"}, - }, - { - name: "update - user cleared cluster IP", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - newService: makeServiceWithClusterIp("", []string{"10.0.0.10"}), - expectedClusterIP: "", - expectedClusterIPs: nil, - }, - { - name: "update - user cleared clusterIPs", // *MUST* REMAIN FOR OLD CLIENTS - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - newService: makeServiceWithClusterIp("10.0.0.10", nil), - expectedClusterIP: "10.0.0.10", - expectedClusterIPs: []string{"10.0.0.10"}, - }, - { - name: "update - user cleared both", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - newService: makeServiceWithClusterIp("", nil), - expectedClusterIP: "", - expectedClusterIPs: nil, - }, - { - name: "update - user cleared ClusterIP but changed clusterIPs", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - newService: makeServiceWithClusterIp("", []string{"10.0.0.11"}), - expectedClusterIP: "", /* validation catches this */ - expectedClusterIPs: []string{"10.0.0.11"}, - }, - { - name: "update - user cleared ClusterIPs but changed ClusterIP", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), - newService: makeServiceWithClusterIp("10.0.0.11", nil), - expectedClusterIP: "10.0.0.11", - expectedClusterIPs: nil, - }, - { - name: "update - user changed from None to ClusterIP", - oldService: makeServiceWithClusterIp("None", []string{"None"}), - newService: makeServiceWithClusterIp("10.0.0.10", []string{"None"}), - expectedClusterIP: "10.0.0.10", - expectedClusterIPs: []string{"10.0.0.10"}, - }, - { - name: "update - user changed from ClusterIP to None", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10"}), - newService: makeServiceWithClusterIp("None", []string{"10.0.0.10"}), - expectedClusterIP: "None", - expectedClusterIPs: []string{"None"}, - }, - { - name: "update - user changed from ClusterIP to None and changed ClusterIPs in a dual stack (new client making a mistake)", - oldService: makeServiceWithClusterIp("10.0.0.10", []string{"10.0.0.10", "2000::1"}), - newService: makeServiceWithClusterIp("None", []string{"10.0.0.11", "2000::1"}), - expectedClusterIP: "None", - expectedClusterIPs: []string{"10.0.0.11", "2000::1"}, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - NormalizeClusterIPs(tc.oldService, tc.newService) - - if tc.newService == nil { - t.Fatalf("unexpected new service to be nil") - } - - if tc.newService.Spec.ClusterIP != tc.expectedClusterIP { - t.Fatalf("expected clusterIP [%v] got [%v]", tc.expectedClusterIP, tc.newService.Spec.ClusterIP) - } - - if len(tc.newService.Spec.ClusterIPs) != len(tc.expectedClusterIPs) { - t.Fatalf("expected clusterIPs %v got %v", tc.expectedClusterIPs, tc.newService.Spec.ClusterIPs) - } - - for idx, clusterIP := range tc.newService.Spec.ClusterIPs { - if clusterIP != tc.expectedClusterIPs[idx] { - t.Fatalf("expected clusterIP [%v] at index[%v] got [%v]", tc.expectedClusterIPs[idx], idx, tc.newService.Spec.ClusterIPs[idx]) - - } - } - }) - } - -} - func TestDropTypeDependentFields(t *testing.T) { // Tweaks used below. setTypeExternalName := func(svc *api.Service) {