Svc REST: Make ipFamilyPolicy authoritative

Previously we would try to infer the `ipFamilyPolicy` from `clusterIPs`
and/or `ipFamilies`.  That is too tricky.  Now you MUST specify
`ipFamilyPolicy` as one of the dual-stack options in order to get a
dual-stack service.

pkg/registry/core/service/storage/rest.go

@@ -824,42 +824,34 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
 		return nil // nothing more to do.
 	}
 
-	// Update-only prep work.
+	// If the user didn't specify ipFamilyPolicy, we can infer a default.  We
-	if oldService != nil {
+	// don't want a static default because we want to make sure that we never
-		np := service.Spec.IPFamilyPolicy
+	// change between single- and dual-stack modes with explicit direction, as
-
+	// provided by ipFamilyPolicy.  Consider these cases:
-		// If they didn't specify policy, or specified anything but
+	//   * Create (POST): If they didn't specify a policy we can assume it's
-		// single-stack AND they reduced these fields, it's an error.  They
+	//     always SingleStack.
-		// need to specify policy.
+	//   * Update (PUT): If they didn't specify a policy we need to adopt the
-		if np == nil || *np != api.IPFamilyPolicySingleStack {
+	//     policy from before.  This is better than always assuming SingleStack
-			el := make(field.ErrorList, 0)
+	//     because a PUT that changes clusterIPs from 2 to 1 value but doesn't
-
+	//     specify ipFamily would work.
-			if reducedClusterIPs(oldService, service) {
+	//   * Update (PATCH): If they didn't specify a policy it will adopt the
-				el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
+	//     policy from before.
-					"must be 'SingleStack' to release the secondary cluster IP"))
+	if service.Spec.IPFamilyPolicy == nil {
-			}
+		if oldService != nil && oldService.Spec.IPFamilyPolicy != nil {
-			if reducedIPFamilies(oldService, service) {
+			// Update from an object with policy, use the old policy
-				el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
+			service.Spec.IPFamilyPolicy = oldService.Spec.IPFamilyPolicy
-					"must be 'SingleStack' to release the secondary IP family"))
+		} else if service.Spec.ClusterIP == api.ClusterIPNone && len(service.Spec.Selector) == 0 {
-			}
+			// Special-case: headless + selectorless defaults to dual.
-
+			requireDualStack := api.IPFamilyPolicyRequireDualStack
-			if len(el) > 0 {
+			service.Spec.IPFamilyPolicy = &requireDualStack
-				return errors.NewInvalid(api.Kind("Service"), service.Name, el)
+		} else {
-			}
+			// create or update from an object without policy (e.g.
-		} else { // policy must be SingleStack
+			// ExternalName) to one that needs policy
-			// Update: As long as ClusterIPs and IPFamilies have not changed,
+			singleStack := api.IPFamilyPolicySingleStack
-			// setting policy to single-stack is clear intent.
+			service.Spec.IPFamilyPolicy = &singleStack
-			if *(service.Spec.IPFamilyPolicy) == api.IPFamilyPolicySingleStack {
-				// ClusterIPs[0] is immutable, so it is safe to keep.
-				if sameClusterIPs(oldService, service) && len(service.Spec.ClusterIPs) > 1 {
-					service.Spec.ClusterIPs = service.Spec.ClusterIPs[0:1]
-				}
-				if sameIPFamilies(oldService, service) && len(service.Spec.IPFamilies) > 1 {
-					service.Spec.IPFamilies = service.Spec.IPFamilies[0:1]
-				}
-			}
 		}
 	}
+	// Henceforth we can assume ipFamilyPolicy is set.
 
 	// Do some loose pre-validation of the input.  This makes it easier in the
 	// rest of allocation code to not have to consider corner cases.
@@ -872,6 +864,32 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
 	//TODO(thockin): Move this logic to validation?
 	el := make(field.ErrorList, 0)
 
+	// Update-only prep work.
+	if oldService != nil {
+		if getIPFamilyPolicy(service) == api.IPFamilyPolicySingleStack {
+			// As long as ClusterIPs and IPFamilies have not changed, setting
+			// the policy to single-stack is clear intent.
+			// ClusterIPs[0] is immutable, so it is safe to keep.
+			if sameClusterIPs(oldService, service) && len(service.Spec.ClusterIPs) > 1 {
+				service.Spec.ClusterIPs = service.Spec.ClusterIPs[0:1]
+			}
+			if sameIPFamilies(oldService, service) && len(service.Spec.IPFamilies) > 1 {
+				service.Spec.IPFamilies = service.Spec.IPFamilies[0:1]
+			}
+		} else {
+			// If the policy is anything but single-stack AND they reduced these
+			// fields, it's an error.  They need to specify policy.
+			if reducedClusterIPs(oldService, service) {
+				el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
+					"must be 'SingleStack' to release the secondary cluster IP"))
+			}
+			if reducedIPFamilies(oldService, service) {
+				el = append(el, field.Invalid(field.NewPath("spec", "ipFamilyPolicy"), service.Spec.IPFamilyPolicy,
+					"must be 'SingleStack' to release the secondary IP family"))
+			}
+		}
+	}
+
 	// Make sure ipFamilyPolicy makes sense for the provided ipFamilies and
 	// clusterIPs.  Further checks happen below - after the special cases.
 	if getIPFamilyPolicy(service) == api.IPFamilyPolicySingleStack {
@@ -916,23 +934,10 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
 		return errors.NewInvalid(api.Kind("Service"), service.Name, el)
 	}
 
-	// Infer IPFamilyPolicy from IPFamilies[].  This block does not handle the
-	// final defaulting - that happens a bit later, after special cases.
-	if service.Spec.IPFamilyPolicy == nil && len(service.Spec.IPFamilies) == 2 {
-		requireDualStack := api.IPFamilyPolicyRequireDualStack
-		service.Spec.IPFamilyPolicy = &requireDualStack
-	}
-
 	// Special-case: headless + selectorless.  This has to happen before other
 	// checks because it explicitly allows combinations of inputs that would
 	// otherwise be errors.
-	if len(service.Spec.ClusterIPs) > 0 && service.Spec.ClusterIPs[0] == api.ClusterIPNone && len(service.Spec.Selector) == 0 {
+	if service.Spec.ClusterIP == api.ClusterIPNone && len(service.Spec.Selector) == 0 {
-		// If the use said nothing about policy and we can't infer it, they get dual-stack
-		if service.Spec.IPFamilyPolicy == nil {
-			requireDualStack := api.IPFamilyPolicyRequireDualStack
-			service.Spec.IPFamilyPolicy = &requireDualStack
-		}
-
 		// If IPFamilies was not set by the user, start with the default
 		// family.
 		if len(service.Spec.IPFamilies) == 0 {
@@ -981,13 +986,6 @@ func (al *RESTAllocStuff) initIPFamilyFields(oldService, service *api.Service) e
 		return errors.NewInvalid(api.Kind("Service"), service.Name, el)
 	}
 
-	// Finally, if IPFamilyPolicy is *still* not set, we can default it to
-	// SingleStack. If there are any webhooks, they have already run.
-	if service.Spec.IPFamilyPolicy == nil {
-		singleStack := api.IPFamilyPolicySingleStack
-		service.Spec.IPFamilyPolicy = &singleStack
-	}
-
 	// nil families, gets cluster default
 	if len(service.Spec.IPFamilies) == 0 {
 		service.Spec.IPFamilies = []api.IPFamily{al.defaultServiceIPFamily}

pkg/registry/core/service/storage/rest_test.go

@@ -349,6 +349,7 @@ func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
 		create: svctest.MakeService("foo",
 			svctest.SetTypeLoadBalancer,
 			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetIPFamilyPolicy(api.IPFamilyPolicyPreferDualStack),
 			svctest.SetClusterIPs("1.2.3.4", "2000::1"),
 			svctest.SetPorts(
 				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
@@ -372,6 +373,7 @@ func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
 		create: svctest.MakeService("foo",
 			svctest.SetTypeLoadBalancer,
 			svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
+			svctest.SetIPFamilyPolicy(api.IPFamilyPolicyPreferDualStack),
 			svctest.SetClusterIPs("1.2.3.4", "2000::1"),
 			svctest.SetPorts(
 				svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),

test/e2e/network/dual_stack.go

@@ -367,7 +367,7 @@ var _ = common.SIGDescribe("[Feature:IPv6DualStack]", func() {
 		expectedPolicy := v1.IPFamilyPolicyRequireDualStack
 		expectedFamilies := []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol}
 
-		service := createService(t.ServiceName, t.Namespace, t.Labels, nil, expectedFamilies)
+		service := createService(t.ServiceName, t.Namespace, t.Labels, &expectedPolicy, expectedFamilies)
 
 		jig.Labels = t.Labels
 		err := jig.CreateServicePods(2)
@@ -412,7 +412,7 @@ var _ = common.SIGDescribe("[Feature:IPv6DualStack]", func() {
 		expectedPolicy := v1.IPFamilyPolicyRequireDualStack
 		expectedFamilies := []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}
 
-		service := createService(t.ServiceName, t.Namespace, t.Labels, nil, expectedFamilies)
+		service := createService(t.ServiceName, t.Namespace, t.Labels, &expectedPolicy, expectedFamilies)
 
 		jig.Labels = t.Labels
 		err := jig.CreateServicePods(2)