Svc REST: Move patchAllocatedValues to storage pkg

All the meaningful callers of it are in that pkg anyway.  Removes 1
FIXME.

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

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 {

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

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