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

@@ -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}
+			}
+		}
+	}
+}

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 {

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

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) {