Merge pull request #91400 from danwinship/ipfamily-validation

service: fix IPFamily validation and defaulting problems

pkg/apis/core/validation/conditional_validation.go

@@ -17,14 +17,21 @@ limitations under the License.
 package validation
 
 import (
+	"fmt"
+	"net"
+	"strings"
+
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/features"
+	netutils "k8s.io/utils/net"
 )
 
-// ValidateConditionalService validates conditionally valid fields.
+// ValidateConditionalService validates conditionally valid fields. allowedIPFamilies is an ordered
-func ValidateConditionalService(service, oldService *api.Service) field.ErrorList {
+// list of the valid IP families (IPv4 or IPv6) that are supported. The first family in the slice
+// is the cluster default, although the clusterIP here dictates the family defaulting.
+func ValidateConditionalService(service, oldService *api.Service, allowedIPFamilies []api.IPFamily) field.ErrorList {
 	var errs field.ErrorList
 	// If the SCTPSupport feature is disabled, and the old object isn't using the SCTP feature, prevent the new object from using it
 	if !utilfeature.DefaultFeatureGate.Enabled(features.SCTPSupport) && len(serviceSCTPFields(oldService)) == 0 {
@@ -32,9 +39,82 @@ func ValidateConditionalService(service, oldService *api.Service) field.ErrorLis
 			errs = append(errs, field.NotSupported(f, api.ProtocolSCTP, []string{string(api.ProtocolTCP), string(api.ProtocolUDP)}))
 		}
 	}
+
+	errs = append(errs, validateIPFamily(service, oldService, allowedIPFamilies)...)
+
 	return errs
 }
 
+// validateIPFamily checks the IPFamily field.
+func validateIPFamily(service, oldService *api.Service, allowedIPFamilies []api.IPFamily) field.ErrorList {
+	var errs field.ErrorList
+
+	// specifically allow an invalid value to remain in storage as long as the user isn't changing it, regardless of gate
+	if oldService != nil && oldService.Spec.IPFamily != nil && service.Spec.IPFamily != nil && *oldService.Spec.IPFamily == *service.Spec.IPFamily {
+		return errs
+	}
+
+	// If the gate is off, setting or changing IPFamily is not allowed, but clearing it is
+	if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) {
+		if service.Spec.IPFamily != nil {
+			if oldService != nil {
+				errs = append(errs, ValidateImmutableField(service.Spec.IPFamily, oldService.Spec.IPFamily, field.NewPath("spec", "ipFamily"))...)
+			} else {
+				errs = append(errs, field.Forbidden(field.NewPath("spec", "ipFamily"), "programmer error, must be cleared when the dual-stack feature gate is off"))
+			}
+		}
+		return errs
+	}
+
+	// PrepareCreate, PrepareUpdate, and test cases must all set IPFamily when the gate is on
+	if service.Spec.IPFamily == nil {
+		errs = append(errs, field.Required(field.NewPath("spec", "ipFamily"), "programmer error, must be set or defaulted by other fields"))
+		return errs
+	}
+
+	// A user is not allowed to change the IPFamily field, except for ExternalName services
+	if oldService != nil && oldService.Spec.IPFamily != nil && service.Spec.Type != api.ServiceTypeExternalName {
+		errs = append(errs, ValidateImmutableField(service.Spec.IPFamily, oldService.Spec.IPFamily, field.NewPath("spec", "ipFamily"))...)
+	}
+
+	// Verify the IPFamily is one of the allowed families
+	desiredFamily := *service.Spec.IPFamily
+	if hasIPFamily(allowedIPFamilies, desiredFamily) {
+		// the IP family is one of the allowed families, verify that it matches cluster IP
+		switch ip := net.ParseIP(service.Spec.ClusterIP); {
+		case ip == nil:
+			// do not need to check anything
+		case netutils.IsIPv6(ip) && desiredFamily != api.IPv6Protocol:
+			errs = append(errs, field.Invalid(field.NewPath("spec", "ipFamily"), *service.Spec.IPFamily, "does not match IPv6 cluster IP"))
+		case !netutils.IsIPv6(ip) && desiredFamily != api.IPv4Protocol:
+			errs = append(errs, field.Invalid(field.NewPath("spec", "ipFamily"), *service.Spec.IPFamily, "does not match IPv4 cluster IP"))
+		}
+	} else {
+		errs = append(errs, field.Invalid(field.NewPath("spec", "ipFamily"), desiredFamily, fmt.Sprintf("only the following families are allowed: %s", joinIPFamilies(allowedIPFamilies, ", "))))
+	}
+	return errs
+}
+
+func hasIPFamily(families []api.IPFamily, family api.IPFamily) bool {
+	for _, allow := range families {
+		if allow == family {
+			return true
+		}
+	}
+	return false
+}
+
+func joinIPFamilies(families []api.IPFamily, separator string) string {
+	var b strings.Builder
+	for i, family := range families {
+		if i != 0 {
+			b.WriteString(separator)
+		}
+		b.WriteString(string(family))
+	}
+	return b.String()
+}
+
 func serviceSCTPFields(service *api.Service) []*field.Path {
 	if service == nil {
 		return nil

pkg/apis/core/validation/conditional_validation_test.go

@@ -19,6 +19,7 @@ package validation
 import (
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/util/diff"
@@ -153,7 +154,7 @@ func TestValidateServiceSCTP(t *testing.T) {
 
 				t.Run(fmt.Sprintf("feature enabled=%v, old object %v, new object %v", enabled, oldServiceInfo.description, newServiceInfo.description), func(t *testing.T) {
 					defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SCTPSupport, enabled)()
-					errs := ValidateConditionalService(newService, oldService)
+					errs := ValidateConditionalService(newService, oldService, []api.IPFamily{api.IPv4Protocol})
 					// objects should never be changed
 					if !reflect.DeepEqual(oldService, oldServiceInfo.object()) {
 						t.Errorf("old object changed: %v", diff.ObjectReflectDiff(oldService, oldServiceInfo.object()))
@@ -251,3 +252,240 @@ func TestValidateEndpointsSCTP(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateServiceIPFamily(t *testing.T) {
+	ipv4 := api.IPv4Protocol
+	ipv6 := api.IPv6Protocol
+	var unknown api.IPFamily = "Unknown"
+	testCases := []struct {
+		name             string
+		dualStackEnabled bool
+		ipFamilies       []api.IPFamily
+		svc              *api.Service
+		oldSvc           *api.Service
+		expectErr        []string
+	}{
+		{
+			name:             "allowed ipv4",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &ipv4,
+				},
+			},
+		},
+		{
+			name:             "allowed ipv6",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv6Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &ipv6,
+				},
+			},
+		},
+		{
+			name:             "allowed ipv4 dual stack default IPv6",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &ipv4,
+				},
+			},
+		},
+		{
+			name:             "allowed ipv4 dual stack default IPv4",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &ipv4,
+				},
+			},
+		},
+		{
+			name:             "allowed ipv6 dual stack default IPv6",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &ipv6,
+				},
+			},
+		},
+		{
+			name:             "allowed ipv6 dual stack default IPv4",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &ipv6,
+				},
+			},
+		},
+		{
+			name:             "allow ipfamily to remain invalid if update doesn't change it",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &unknown,
+				},
+			},
+			oldSvc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &unknown,
+				},
+			},
+		},
+		{
+			name:             "not allowed ipfamily/clusterip mismatch",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily:  &ipv4,
+					ClusterIP: "ffd0::1",
+				},
+			},
+			expectErr: []string{"spec.ipFamily: Invalid value: \"IPv4\": does not match IPv6 cluster IP"},
+		},
+		{
+			name:             "not allowed unknown family",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &unknown,
+				},
+			},
+			expectErr: []string{"spec.ipFamily: Invalid value: \"Unknown\": only the following families are allowed: IPv4"},
+		},
+		{
+			name:             "not allowed ipv4 cluster ip without family",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv6Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					ClusterIP: "127.0.0.1",
+				},
+			},
+			expectErr: []string{"spec.ipFamily: Required value: programmer error, must be set or defaulted by other fields"},
+		},
+		{
+			name:             "not allowed ipv6 cluster ip without family",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					ClusterIP: "ffd0::1",
+				},
+			},
+			expectErr: []string{"spec.ipFamily: Required value: programmer error, must be set or defaulted by other fields"},
+		},
+
+		{
+			name:             "not allowed to change ipfamily for default type",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &ipv4,
+				},
+			},
+			oldSvc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily: &ipv6,
+				},
+			},
+			expectErr: []string{"spec.ipFamily: Invalid value: \"IPv4\": field is immutable"},
+		},
+		{
+			name:             "allowed to change ipfamily for external name",
+			dualStackEnabled: true,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					Type:     api.ServiceTypeExternalName,
+					IPFamily: &ipv4,
+				},
+			},
+			oldSvc: &api.Service{
+				Spec: api.ServiceSpec{
+					Type:     api.ServiceTypeExternalName,
+					IPFamily: &ipv6,
+				},
+			},
+		},
+
+		{
+			name:             "ipfamily allowed to be empty when dual stack is off",
+			dualStackEnabled: false,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					ClusterIP: "127.0.0.1",
+				},
+			},
+		},
+		{
+			name:             "ipfamily must be empty when dual stack is off",
+			dualStackEnabled: false,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					IPFamily:  &ipv4,
+					ClusterIP: "127.0.0.1",
+				},
+			},
+			expectErr: []string{"spec.ipFamily: Forbidden: programmer error, must be cleared when the dual-stack feature gate is off"},
+		},
+		{
+			name:             "ipfamily allowed to be cleared when dual stack is off",
+			dualStackEnabled: false,
+			ipFamilies:       []api.IPFamily{api.IPv4Protocol},
+			svc: &api.Service{
+				Spec: api.ServiceSpec{
+					Type:      api.ServiceTypeClusterIP,
+					ClusterIP: "127.0.0.1",
+				},
+			},
+			oldSvc: &api.Service{
+				Spec: api.ServiceSpec{
+					Type:      api.ServiceTypeClusterIP,
+					ClusterIP: "127.0.0.1",
+					IPFamily:  &ipv4,
+				},
+			},
+			expectErr: nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.dualStackEnabled)()
+			oldSvc := tc.oldSvc.DeepCopy()
+			newSvc := tc.svc.DeepCopy()
+			originalNewSvc := newSvc.DeepCopy()
+			errs := ValidateConditionalService(newSvc, oldSvc, tc.ipFamilies)
+			// objects should never be changed
+			if !reflect.DeepEqual(oldSvc, tc.oldSvc) {
+				t.Errorf("old object changed: %v", diff.ObjectReflectDiff(oldSvc, tc.svc))
+			}
+			if !reflect.DeepEqual(newSvc, originalNewSvc) {
+				t.Errorf("new object changed: %v", diff.ObjectReflectDiff(newSvc, originalNewSvc))
+			}
+
+			if len(errs) != len(tc.expectErr) {
+				t.Fatalf("unexpected number of errors: %v", errs)
+			}
+			for i := range errs {
+				if !strings.Contains(errs[i].Error(), tc.expectErr[i]) {
+					t.Errorf("unexpected error %d: %v", i, errs[i])
+				}
+			}
+		})
+	}
+}

pkg/apis/core/validation/validation.go

@@ -3951,7 +3951,6 @@ func ValidatePodTemplateUpdate(newPod, oldPod *core.PodTemplate) field.ErrorList
 var supportedSessionAffinityType = sets.NewString(string(core.ServiceAffinityClientIP), string(core.ServiceAffinityNone))
 var supportedServiceType = sets.NewString(string(core.ServiceTypeClusterIP), string(core.ServiceTypeNodePort),
 	string(core.ServiceTypeLoadBalancer), string(core.ServiceTypeExternalName))
-var supportedServiceIPFamily = sets.NewString(string(core.IPv4Protocol), string(core.IPv6Protocol))
 
 // ValidateService tests if required fields/annotations of a Service are valid.
 func ValidateService(service *core.Service, allowAppProtocol bool) field.ErrorList {
@@ -4152,21 +4151,6 @@ func ValidateService(service *core.Service, allowAppProtocol bool) field.ErrorLi
 		}
 	}
 
-	//if an ipfamily provided then it has to be one of the supported values
-	// note:
-	// - we don't validate service.Spec.IPFamily is supported by the cluster
-	// - we don't validate service.Spec.ClusterIP is within a range supported by the cluster
-	// both of these validations are done by the ipallocator
-
-	// if the gate is on this field is required (and defaulted by REST if not provided by user)
-	if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && service.Spec.IPFamily == nil {
-		allErrs = append(allErrs, field.Required(specPath.Child("ipFamily"), ""))
-	}
-
-	if service.Spec.IPFamily != nil && !supportedServiceIPFamily.Has(string(*service.Spec.IPFamily)) {
-		allErrs = append(allErrs, field.NotSupported(specPath.Child("ipFamily"), service.Spec.IPFamily, supportedServiceIPFamily.List()))
-	}
-
 	allErrs = append(allErrs, validateServiceExternalTrafficFieldsValue(service)...)
 	return allErrs
 }
@@ -4275,19 +4259,12 @@ func ValidateServiceCreate(service *core.Service) field.ErrorList {
 func ValidateServiceUpdate(service, oldService *core.Service) field.ErrorList {
 	allErrs := ValidateObjectMetaUpdate(&service.ObjectMeta, &oldService.ObjectMeta, field.NewPath("metadata"))
 
-	// ClusterIP and IPFamily should be immutable for services using it (every type other than ExternalName)
+	// ClusterIP should be immutable for services using it (every type other than ExternalName)
 	// which do not have ClusterIP assigned yet (empty string value)
 	if service.Spec.Type != core.ServiceTypeExternalName {
 		if oldService.Spec.Type != core.ServiceTypeExternalName && oldService.Spec.ClusterIP != "" {
 			allErrs = append(allErrs, ValidateImmutableField(service.Spec.ClusterIP, oldService.Spec.ClusterIP, field.NewPath("spec", "clusterIP"))...)
 		}
-		// notes:
-		// we drop the IPFamily field when the Dualstack gate is off.
-		// once the gate is on, we start assigning default ipfamily according to cluster settings. in other words
-		// though the field is immutable, we allow (onetime) change from nil==> to value
-		if oldService.Spec.IPFamily != nil {
-			allErrs = append(allErrs, ValidateImmutableField(service.Spec.IPFamily, oldService.Spec.IPFamily, field.NewPath("spec", "ipFamily"))...)
-		}
 	}
 
 	// allow AppProtocol value if the feature gate is set or the field is

pkg/apis/core/validation/validation_test.go

@@ -10193,12 +10193,12 @@ func TestValidateServiceCreate(t *testing.T) {
 			numErrs: 0,
 		},
 		{
-			name: "invalid, service with invalid IPFamily",
+			name: "allowed valid, service with invalid IPFamily is ignored (tested in conditional validation)",
 			tweakSvc: func(s *core.Service) {
 				invalidServiceIPFamily := core.IPFamily("not-a-valid-ip-family")
 				s.Spec.IPFamily = &invalidServiceIPFamily
 			},
-			numErrs: 1,
+			numErrs: 0,
 		},
 		{
 			name: "valid topology keys",
@@ -12204,18 +12204,18 @@ func TestValidateServiceUpdate(t *testing.T) {
 			numErrs: 0,
 		},
 		{
-			name: "remove ipfamily",
+			name: "remove ipfamily (covered by conditional validation)",
 			tweakSvc: func(oldSvc, newSvc *core.Service) {
 				ipv6Service := core.IPv6Protocol
 				oldSvc.Spec.IPFamily = &ipv6Service
 
 				newSvc.Spec.IPFamily = nil
 			},
-			numErrs: 1,
+			numErrs: 0,
 		},
 
 		{
-			name: "change ServiceIPFamily",
+			name: "change ServiceIPFamily (covered by conditional validation)",
 			tweakSvc: func(oldSvc, newSvc *core.Service) {
 				ipv4Service := core.IPv4Protocol
 				oldSvc.Spec.Type = core.ServiceTypeClusterIP
@@ -12225,7 +12225,7 @@ func TestValidateServiceUpdate(t *testing.T) {
 				newSvc.Spec.Type = core.ServiceTypeClusterIP
 				newSvc.Spec.IPFamily = &ipv6Service
 			},
-			numErrs: 1,
+			numErrs: 0,
 		},
 		{
 			name: "update with valid app protocol, field unset, gate disabled",

pkg/registry/core/rest/storage_core.go

@@ -190,11 +190,6 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(restOptionsGetter generi
 		return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, err
 	}
 
-	serviceRESTStorage, serviceStatusStorage, err := servicestore.NewGenericREST(restOptionsGetter)
-	if err != nil {
-		return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, err
-	}
-
 	var serviceClusterIPRegistry rangeallocation.RangeRegistry
 	serviceClusterIPRange := c.ServiceIPRange
 	if serviceClusterIPRange.IP == nil {
@@ -262,6 +257,11 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(restOptionsGetter generi
 		return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, err
 	}
 
+	serviceRESTStorage, serviceStatusStorage, err := servicestore.NewGenericREST(restOptionsGetter, serviceClusterIPRange, secondaryServiceClusterIPAllocator != nil)
+	if err != nil {
+		return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, err
+	}
+
 	serviceRest, serviceRestProxy := servicestore.NewREST(serviceRESTStorage,
 		endpointsStorage,
 		podStorage.Pod,

pkg/registry/core/service/BUILD

@@ -27,6 +27,7 @@ go_library(
         "//staging/src/k8s.io/apiserver/pkg/registry/rest:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/storage/names:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
+        "//vendor/k8s.io/utils/net:go_default_library",
     ],
 )
 

pkg/registry/core/service/storage/BUILD

@@ -29,6 +29,7 @@ go_test(
         "//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/net:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/rand:go_default_library",

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

@@ -53,14 +53,14 @@ import (
 
 // REST adapts a service registry into apiserver's RESTStorage model.
 type REST struct {
-	services               ServiceStorage
+	strategy            rest.RESTCreateUpdateStrategy
-	endpoints              EndpointsStorage
+	services            ServiceStorage
-	serviceIPs             ipallocator.Interface
+	endpoints           EndpointsStorage
-	secondaryServiceIPs    ipallocator.Interface
+	serviceIPs          ipallocator.Interface
-	defaultServiceIPFamily api.IPFamily
+	secondaryServiceIPs ipallocator.Interface
-	serviceNodePorts       portallocator.Interface
+	serviceNodePorts    portallocator.Interface
-	proxyTransport         http.RoundTripper
+	proxyTransport      http.RoundTripper
-	pods                   rest.Getter
+	pods                rest.Getter
 }
 
 // ServiceNodePort includes protocol and port number of a service NodePort.
@@ -102,26 +102,20 @@ func NewREST(
 	serviceNodePorts portallocator.Interface,
 	proxyTransport http.RoundTripper,
 ) (*REST, *registry.ProxyREST) {
-	// detect this cluster default Service IPFamily (ipfamily of --service-cluster-ip-range)
-	// we do it once here, to avoid having to do it over and over during ipfamily assignment
-	serviceIPFamily := api.IPv4Protocol
-	cidr := serviceIPs.CIDR()
-	if netutil.IsIPv6CIDR(&cidr) {
-		serviceIPFamily = api.IPv6Protocol
-	}
 
-	klog.V(0).Infof("the default service ipfamily for this cluster is: %s", string(serviceIPFamily))
+	strategy, _ := registry.StrategyForServiceCIDRs(serviceIPs.CIDR(), secondaryServiceIPs != nil)
 
 	rest := &REST{
-		services:               services,
+		strategy:            strategy,
-		endpoints:              endpoints,
+		services:            services,
-		serviceIPs:             serviceIPs,
+		endpoints:           endpoints,
-		secondaryServiceIPs:    secondaryServiceIPs,
+		serviceIPs:          serviceIPs,
-		serviceNodePorts:       serviceNodePorts,
+		secondaryServiceIPs: secondaryServiceIPs,
-		defaultServiceIPFamily: serviceIPFamily,
+		serviceNodePorts:    serviceNodePorts,
-		proxyTransport:         proxyTransport,
+		proxyTransport:      proxyTransport,
-		pods:                   pods,
+		pods:                pods,
 	}
+
 	return rest, &registry.ProxyREST{Redirector: rest, ProxyTransport: proxyTransport}
 }
 
@@ -177,12 +171,7 @@ func (rs *REST) Export(ctx context.Context, name string, opts metav1.ExportOptio
 func (rs *REST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
 	service := obj.(*api.Service)
 
-	// set the service ip family, if it was not already set
+	if err := rest.BeforeCreate(rs.strategy, ctx, obj); err != nil {
-	if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && service.Spec.IPFamily == nil {
-		service.Spec.IPFamily = &rs.defaultServiceIPFamily
-	}
-
-	if err := rest.BeforeCreate(registry.Strategy, ctx, obj); err != nil {
 		return nil, err
 	}
 
@@ -228,7 +217,7 @@ func (rs *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 
 	out, err := rs.services.Create(ctx, service, createValidation, options)
 	if err != nil {
-		err = rest.CheckGeneratedNameError(registry.Strategy, err, service)
+		err = rest.CheckGeneratedNameError(rs.strategy, err, service)
 	}
 
 	if err == nil {
@@ -396,7 +385,7 @@ func (rs *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObj
 	}
 
 	// Copy over non-user fields
-	if err := rest.BeforeUpdate(registry.Strategy, ctx, service, oldService); err != nil {
+	if err := rest.BeforeUpdate(rs.strategy, ctx, service, oldService); err != nil {
 		return nil, false, err
 	}
 
@@ -666,6 +655,8 @@ func allocateHealthCheckNodePort(service *api.Service, nodePortOp *portallocator
 
 // The return bool value indicates if a cluster IP is allocated successfully.
 func initClusterIP(service *api.Service, allocator ipallocator.Interface) (bool, error) {
+	var allocatedIP net.IP
+
 	switch {
 	case service.Spec.ClusterIP == "":
 		// Allocate next available.
@@ -676,19 +667,32 @@ func initClusterIP(service *api.Service, allocator ipallocator.Interface) (bool,
 			// not really an internal error.
 			return false, errors.NewInternalError(fmt.Errorf("failed to allocate a serviceIP: %v", err))
 		}
+		allocatedIP = ip
 		service.Spec.ClusterIP = ip.String()
-		return true, nil
 	case service.Spec.ClusterIP != api.ClusterIPNone && service.Spec.ClusterIP != "":
 		// Try to respect the requested IP.
-		if err := allocator.Allocate(net.ParseIP(service.Spec.ClusterIP)); err != nil {
+		ip := net.ParseIP(service.Spec.ClusterIP)
+		if err := allocator.Allocate(ip); err != nil {
 			// TODO: when validation becomes versioned, this gets more complicated.
 			el := field.ErrorList{field.Invalid(field.NewPath("spec", "clusterIP"), service.Spec.ClusterIP, err.Error())}
 			return false, errors.NewInvalid(api.Kind("Service"), service.Name, el)
 		}
-		return true, nil
+		allocatedIP = ip
 	}
 
-	return false, nil
+	// assuming the object was valid prior to setting, always force the IPFamily
+	// to match the allocated IP at this point
+	if allocatedIP != nil {
+		if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) {
+			ipFamily := api.IPv4Protocol
+			if netutil.IsIPv6(allocatedIP) {
+				ipFamily = api.IPv6Protocol
+			}
+			service.Spec.IPFamily = &ipFamily
+		}
+	}
+
+	return allocatedIP != nil, nil
 }
 
 func initNodePorts(service *api.Service, nodePortOp *portallocator.PortAllocationOperation) error {

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

@@ -18,6 +18,7 @@ package storage
 
 import (
 	"context"
+	"net"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -29,6 +30,7 @@ import (
 	printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
 	printerstorage "k8s.io/kubernetes/pkg/printers/storage"
 	"k8s.io/kubernetes/pkg/registry/core/service"
+	registry "k8s.io/kubernetes/pkg/registry/core/service"
 )
 
 type GenericREST struct {
@@ -36,17 +38,19 @@ type GenericREST struct {
 }
 
 // NewREST returns a RESTStorage object that will work against services.
-func NewGenericREST(optsGetter generic.RESTOptionsGetter) (*GenericREST, *StatusREST, error) {
+func NewGenericREST(optsGetter generic.RESTOptionsGetter, serviceCIDR net.IPNet, hasSecondary bool) (*GenericREST, *StatusREST, error) {
+	strategy, _ := registry.StrategyForServiceCIDRs(serviceCIDR, hasSecondary)
+
 	store := &genericregistry.Store{
 		NewFunc:                  func() runtime.Object { return &api.Service{} },
 		NewListFunc:              func() runtime.Object { return &api.ServiceList{} },
 		DefaultQualifiedResource: api.Resource("services"),
 		ReturnDeletedObject:      true,
 
-		CreateStrategy: service.Strategy,
+		CreateStrategy: strategy,
-		UpdateStrategy: service.Strategy,
+		UpdateStrategy: strategy,
-		DeleteStrategy: service.Strategy,
+		DeleteStrategy: strategy,
-		ExportStrategy: service.Strategy,
+		ExportStrategy: strategy,
 
 		TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)},
 	}
@@ -56,7 +60,7 @@ func NewGenericREST(optsGetter generic.RESTOptionsGetter) (*GenericREST, *Status
 	}
 
 	statusStore := *store
-	statusStore.UpdateStrategy = service.StatusStrategy
+	statusStore.UpdateStrategy = service.NewServiceStatusStrategy(strategy)
 	return &GenericREST{store}, &StatusREST{store: &statusStore}, nil
 }
 

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

@@ -39,7 +39,7 @@ func newStorage(t *testing.T) (*GenericREST, *StatusREST, *etcd3testing.EtcdTest
 		DeleteCollectionWorkers: 1,
 		ResourcePrefix:          "services",
 	}
-	serviceStorage, statusStorage, err := NewGenericREST(restOptions)
+	serviceStorage, statusStorage, err := NewGenericREST(restOptions, *makeIPNet(t), false)
 	if err != nil {
 		t.Fatalf("unexpected error from REST storage: %v", err)
 	}

pkg/registry/core/service/strategy.go

@@ -19,54 +19,113 @@ package service
 import (
 	"context"
 	"fmt"
+	"net"
 
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/apiserver/pkg/registry/rest"
 	"k8s.io/apiserver/pkg/storage/names"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/apis/core/validation"
 	"k8s.io/kubernetes/pkg/features"
+	netutil "k8s.io/utils/net"
 )
 
+type Strategy interface {
+	rest.RESTCreateUpdateStrategy
+	rest.RESTExportStrategy
+}
+
 // svcStrategy implements behavior for Services
 type svcStrategy struct {
 	runtime.ObjectTyper
 	names.NameGenerator
+
+	ipFamilies []api.IPFamily
 }
 
-// Services is the default logic that applies when creating and updating Service
+// StrategyForServiceCIDRs returns the appropriate service strategy for the given configuration.
-// objects.
+func StrategyForServiceCIDRs(primaryCIDR net.IPNet, hasSecondary bool) (Strategy, api.IPFamily) {
-var Strategy = svcStrategy{legacyscheme.Scheme, names.SimpleNameGenerator}
+	// detect this cluster default Service IPFamily (ipfamily of --service-cluster-ip-range)
+	// we do it once here, to avoid having to do it over and over during ipfamily assignment
+	serviceIPFamily := api.IPv4Protocol
+	if netutil.IsIPv6CIDR(&primaryCIDR) {
+		serviceIPFamily = api.IPv6Protocol
+	}
+
+	var strategy Strategy
+	switch {
+	case hasSecondary && serviceIPFamily == api.IPv4Protocol:
+		strategy = svcStrategy{
+			ObjectTyper:   legacyscheme.Scheme,
+			NameGenerator: names.SimpleNameGenerator,
+			ipFamilies:    []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol},
+		}
+	case hasSecondary && serviceIPFamily == api.IPv6Protocol:
+		strategy = svcStrategy{
+			ObjectTyper:   legacyscheme.Scheme,
+			NameGenerator: names.SimpleNameGenerator,
+			ipFamilies:    []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol},
+		}
+	case serviceIPFamily == api.IPv6Protocol:
+		strategy = svcStrategy{
+			ObjectTyper:   legacyscheme.Scheme,
+			NameGenerator: names.SimpleNameGenerator,
+			ipFamilies:    []api.IPFamily{api.IPv6Protocol},
+		}
+	default:
+		strategy = svcStrategy{
+			ObjectTyper:   legacyscheme.Scheme,
+			NameGenerator: names.SimpleNameGenerator,
+			ipFamilies:    []api.IPFamily{api.IPv4Protocol},
+		}
+	}
+	return strategy, serviceIPFamily
+}
 
 // NamespaceScoped is true for services.
 func (svcStrategy) NamespaceScoped() bool {
 	return true
 }
 
-// PrepareForCreate clears fields that are not allowed to be set by end users on creation.
+// PrepareForCreate sets contextual defaults and clears fields that are not allowed to be set by end users on creation.
-func (svcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
+func (strategy svcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) {
 	service := obj.(*api.Service)
 	service.Status = api.ServiceStatus{}
 
+	if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && service.Spec.IPFamily == nil {
+		family := strategy.ipFamilies[0]
+		service.Spec.IPFamily = &family
+	}
+
 	dropServiceDisabledFields(service, nil)
 }
 
-// PrepareForUpdate clears fields that are not allowed to be set by end users on update.
+// PrepareForUpdate sets contextual defaults and clears fields that are not allowed to be set by end users on update.
-func (svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
+func (strategy svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
 	newService := obj.(*api.Service)
 	oldService := old.(*api.Service)
 	newService.Status = oldService.Status
 
+	if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && newService.Spec.IPFamily == nil {
+		if oldService.Spec.IPFamily != nil {
+			newService.Spec.IPFamily = oldService.Spec.IPFamily
+		} else {
+			family := strategy.ipFamilies[0]
+			newService.Spec.IPFamily = &family
+		}
+	}
+
 	dropServiceDisabledFields(newService, oldService)
 }
 
 // Validate validates a new service.
-func (svcStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
+func (strategy svcStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
 	service := obj.(*api.Service)
 	allErrs := validation.ValidateServiceCreate(service)
-	allErrs = append(allErrs, validation.ValidateConditionalService(service, nil)...)
+	allErrs = append(allErrs, validation.ValidateConditionalService(service, nil, strategy.ipFamilies)...)
 	return allErrs
 }
 
@@ -78,9 +137,9 @@ func (svcStrategy) AllowCreateOnUpdate() bool {
 	return true
 }
 
-func (svcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
+func (strategy svcStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
 	allErrs := validation.ValidateServiceUpdate(obj.(*api.Service), old.(*api.Service))
-	allErrs = append(allErrs, validation.ValidateConditionalService(obj.(*api.Service), old.(*api.Service))...)
+	allErrs = append(allErrs, validation.ValidateConditionalService(obj.(*api.Service), old.(*api.Service), strategy.ipFamilies)...)
 	return allErrs
 }
 
@@ -120,6 +179,7 @@ func dropServiceDisabledFields(newSvc *api.Service, oldSvc *api.Service) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && !serviceIPFamilyInUse(oldSvc) {
 		newSvc.Spec.IPFamily = nil
 	}
+
 	// Drop TopologyKeys if ServiceTopology is not enabled
 	if !utilfeature.DefaultFeatureGate.Enabled(features.ServiceTopology) && !topologyKeysInUse(oldSvc) {
 		newSvc.Spec.TopologyKeys = nil
@@ -146,11 +206,13 @@ func topologyKeysInUse(svc *api.Service) bool {
 }
 
 type serviceStatusStrategy struct {
-	svcStrategy
+	Strategy
 }
 
-// StatusStrategy is the default logic invoked when updating service status.
+// NewServiceStatusStrategy creates a status strategy for the provided base strategy.
-var StatusStrategy = serviceStatusStrategy{Strategy}
+func NewServiceStatusStrategy(strategy Strategy) Strategy {
+	return serviceStatusStrategy{strategy}
+}
 
 // PrepareForUpdate clears fields that are not allowed to be set by end users on update of status
 func (serviceStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {

pkg/registry/core/service/strategy_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package service
 
 import (
+	"net"
 	"reflect"
 	"testing"
 
@@ -35,7 +36,18 @@ import (
 	"k8s.io/kubernetes/pkg/features"
 )
 
+func newStrategy(cidr string, hasSecondary bool) (testStrategy Strategy, testStatusStrategy Strategy) {
+	_, testCIDR, err := net.ParseCIDR(cidr)
+	if err != nil {
+		panic("invalid CIDR")
+	}
+	testStrategy, _ = StrategyForServiceCIDRs(*testCIDR, hasSecondary)
+	testStatusStrategy = NewServiceStatusStrategy(testStrategy)
+	return
+}
+
 func TestExportService(t *testing.T) {
+	testStrategy, _ := newStrategy("10.0.0.0/16", false)
 	tests := []struct {
 		objIn     runtime.Object
 		objOut    runtime.Object
@@ -98,7 +110,7 @@ func TestExportService(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		err := Strategy.Export(genericapirequest.NewContext(), test.objIn, test.exact)
+		err := testStrategy.Export(genericapirequest.NewContext(), test.objIn, test.exact)
 		if err != nil {
 			if !test.expectErr {
 				t.Errorf("unexpected error: %v", err)
@@ -116,18 +128,19 @@ func TestExportService(t *testing.T) {
 }
 
 func TestCheckGeneratedNameError(t *testing.T) {
+	testStrategy, _ := newStrategy("10.0.0.0/16", false)
 	expect := errors.NewNotFound(api.Resource("foos"), "bar")
-	if err := rest.CheckGeneratedNameError(Strategy, expect, &api.Service{}); err != expect {
+	if err := rest.CheckGeneratedNameError(testStrategy, expect, &api.Service{}); err != expect {
 		t.Errorf("NotFoundError should be ignored: %v", err)
 	}
 
 	expect = errors.NewAlreadyExists(api.Resource("foos"), "bar")
-	if err := rest.CheckGeneratedNameError(Strategy, expect, &api.Service{}); err != expect {
+	if err := rest.CheckGeneratedNameError(testStrategy, expect, &api.Service{}); err != expect {
 		t.Errorf("AlreadyExists should be returned when no GenerateName field: %v", err)
 	}
 
 	expect = errors.NewAlreadyExists(api.Resource("foos"), "bar")
-	if err := rest.CheckGeneratedNameError(Strategy, expect, &api.Service{ObjectMeta: metav1.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsServerTimeout(err) {
+	if err := rest.CheckGeneratedNameError(testStrategy, expect, &api.Service{ObjectMeta: metav1.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsServerTimeout(err) {
 		t.Errorf("expected try again later error: %v", err)
 	}
 }
@@ -153,11 +166,131 @@ func makeValidService() api.Service {
 }
 
 // TODO: This should be done on types that are not part of our API
+func TestBeforeCreate(t *testing.T) {
+	withIP := func(family *api.IPFamily, ip string) *api.Service {
+		svc := makeValidService()
+		svc.Spec.IPFamily = family
+		svc.Spec.ClusterIP = ip
+		return &svc
+	}
+
+	ipv4 := api.IPv4Protocol
+	ipv6 := api.IPv6Protocol
+	testCases := []struct {
+		name               string
+		cidr               string
+		configureDualStack bool
+		enableDualStack    bool
+		in                 *api.Service
+		expect             *api.Service
+		expectErr          bool
+	}{
+		{
+			name:   "does not set ipfamily when dual stack gate is disabled",
+			cidr:   "10.0.0.0/16",
+			in:     withIP(nil, ""),
+			expect: withIP(nil, ""),
+		},
+
+		{
+			name:   "clears ipfamily when dual stack gate is disabled",
+			cidr:   "10.0.0.0/16",
+			in:     withIP(&ipv4, ""),
+			expect: withIP(nil, ""),
+		},
+
+		{
+			name:            "allows ipfamily to configured ipv4 value",
+			cidr:            "10.0.0.0/16",
+			enableDualStack: true,
+			in:              withIP(nil, ""),
+			expect:          withIP(&ipv4, ""),
+		},
+		{
+			name:               "allows ipfamily to configured ipv4 value when dual stack is in use",
+			cidr:               "10.0.0.0/16",
+			enableDualStack:    true,
+			configureDualStack: true,
+			in:                 withIP(nil, ""),
+			expect:             withIP(&ipv4, ""),
+		},
+		{
+			name:            "allows ipfamily to configured ipv6 value",
+			cidr:            "fd00::/64",
+			enableDualStack: true,
+			in:              withIP(nil, ""),
+			expect:          withIP(&ipv6, ""),
+		},
+		{
+			name:               "allows ipfamily to configured ipv6 value when dual stack is in use",
+			cidr:               "fd00::/64",
+			enableDualStack:    true,
+			configureDualStack: true,
+			in:                 withIP(nil, ""),
+			expect:             withIP(&ipv6, ""),
+		},
+
+		{
+			name:            "rejects ipv6 ipfamily when single-stack ipv4",
+			enableDualStack: true,
+			cidr:            "10.0.0.0/16",
+			in:              withIP(&ipv6, ""),
+			expectErr:       true,
+		},
+		{
+			name:            "rejects ipv4 ipfamily when single-stack ipv6",
+			enableDualStack: true,
+			cidr:            "fd00::/64",
+			in:              withIP(&ipv4, ""),
+			expectErr:       true,
+		},
+		{
+			name:            "rejects implicit ipv4 ipfamily when single-stack ipv6",
+			enableDualStack: true,
+			cidr:            "fd00::/64",
+			in:              withIP(nil, "10.0.1.0"),
+			expectErr:       true,
+		},
+		{
+			name:            "rejects implicit ipv6 ipfamily when single-stack ipv4",
+			enableDualStack: true,
+			cidr:            "10.0.0.0/16",
+			in:              withIP(nil, "fd00::1"),
+			expectErr:       true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)()
+			testStrategy, _ := newStrategy(tc.cidr, tc.configureDualStack)
+			ctx := genericapirequest.NewDefaultContext()
+			err := rest.BeforeCreate(testStrategy, ctx, runtime.Object(tc.in))
+			if tc.expectErr != (err != nil) {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if err != nil {
+				return
+			}
+			if tc.expect != nil && tc.in != nil {
+				tc.expect.ObjectMeta = tc.in.ObjectMeta
+			}
+			if !reflect.DeepEqual(tc.expect, tc.in) {
+				t.Fatalf("unexpected change: %s", diff.ObjectReflectDiff(tc.expect, tc.in))
+			}
+		})
+	}
+}
+
 func TestBeforeUpdate(t *testing.T) {
 	testCases := []struct {
-		name      string
+		name            string
-		tweakSvc  func(oldSvc, newSvc *api.Service) // given basic valid services, each test case can customize them
+		enableDualStack bool
-		expectErr bool
+		defaultIPv6     bool
+		allowSecondary  bool
+		tweakSvc        func(oldSvc, newSvc *api.Service) // given basic valid services, each test case can customize them
+		expectErr       bool
+		expectObj       func(t *testing.T, svc *api.Service)
 	}{
 		{
 			name: "no change",
@@ -195,6 +328,19 @@ func TestBeforeUpdate(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		{
+			name:            "clear IP family is allowed (defaulted back by before update)",
+			enableDualStack: true,
+			tweakSvc: func(oldSvc, newSvc *api.Service) {
+				oldSvc.Spec.IPFamily = nil
+			},
+			expectErr: false,
+			expectObj: func(t *testing.T, svc *api.Service) {
+				if svc.Spec.IPFamily == nil {
+					t.Errorf("ipfamily was not defaulted")
+				}
+			},
+		},
 		{
 			name: "change selector",
 			tweakSvc: func(oldSvc, newSvc *api.Service) {
@@ -205,23 +351,36 @@ func TestBeforeUpdate(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		oldSvc := makeValidService()
+		t.Run(tc.name, func(t *testing.T) {
-		newSvc := makeValidService()
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)()
-		tc.tweakSvc(&oldSvc, &newSvc)
+			var cidr string
-		ctx := genericapirequest.NewDefaultContext()
+			if tc.defaultIPv6 {
-		err := rest.BeforeUpdate(Strategy, ctx, runtime.Object(&oldSvc), runtime.Object(&newSvc))
+				cidr = "ffd0::/64"
-		if tc.expectErr && err == nil {
+			} else {
-			t.Errorf("unexpected non-error for %q", tc.name)
+				cidr = "172.30.0.0/16"
-		}
+			}
-		if !tc.expectErr && err != nil {
+			strategy, _ := newStrategy(cidr, tc.allowSecondary)
-			t.Errorf("unexpected error for %q: %v", tc.name, err)
+			oldSvc := makeValidService()
-		}
+			newSvc := makeValidService()
+			tc.tweakSvc(&oldSvc, &newSvc)
+			ctx := genericapirequest.NewDefaultContext()
+			err := rest.BeforeUpdate(strategy, ctx, runtime.Object(&newSvc), runtime.Object(&oldSvc))
+			if tc.expectObj != nil {
+				tc.expectObj(t, &newSvc)
+			}
+			if tc.expectErr && err == nil {
+				t.Fatalf("unexpected non-error: %v", err)
+			}
+			if !tc.expectErr && err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+		})
 	}
 }
-
 func TestServiceStatusStrategy(t *testing.T) {
+	_, testStatusStrategy := newStrategy("10.0.0.0/16", false)
 	ctx := genericapirequest.NewDefaultContext()
-	if !StatusStrategy.NamespaceScoped() {
+	if !testStatusStrategy.NamespaceScoped() {
 		t.Errorf("Service must be namespace scoped")
 	}
 	oldService := makeValidService()
@@ -236,23 +395,23 @@ func TestServiceStatusStrategy(t *testing.T) {
 			},
 		},
 	}
-	StatusStrategy.PrepareForUpdate(ctx, &newService, &oldService)
+	testStatusStrategy.PrepareForUpdate(ctx, &newService, &oldService)
 	if newService.Status.LoadBalancer.Ingress[0].IP != "127.0.0.2" {
 		t.Errorf("Service status updates should allow change of status fields")
 	}
 	if newService.Spec.SessionAffinity != "None" {
 		t.Errorf("PrepareForUpdate should have preserved old spec")
 	}
-	errs := StatusStrategy.ValidateUpdate(ctx, &newService, &oldService)
+	errs := testStatusStrategy.ValidateUpdate(ctx, &newService, &oldService)
 	if len(errs) != 0 {
 		t.Errorf("Unexpected error %v", errs)
 	}
 }
 
-func makeServiceWithIPFamily(IPFamily *api.IPFamily) *api.Service {
+func makeServiceWithIPFamily(ipFamily *api.IPFamily) *api.Service {
 	return &api.Service{
 		Spec: api.ServiceSpec{
-			IPFamily: IPFamily,
+			IPFamily: ipFamily,
 		},
 	}
 }
@@ -294,6 +453,20 @@ func TestDropDisabledField(t *testing.T) {
 			oldSvc:          nil,
 			compareSvc:      makeServiceWithIPFamily(&ipv6Service),
 		},
+		{
+			name:            "dualstack, field used, changed",
+			enableDualStack: true,
+			svc:             makeServiceWithIPFamily(&ipv6Service),
+			oldSvc:          makeServiceWithIPFamily(&ipv4Service),
+			compareSvc:      makeServiceWithIPFamily(&ipv6Service),
+		},
+		{
+			name:            "dualstack, field used, not changed",
+			enableDualStack: true,
+			svc:             makeServiceWithIPFamily(&ipv6Service),
+			oldSvc:          makeServiceWithIPFamily(&ipv6Service),
+			compareSvc:      makeServiceWithIPFamily(&ipv6Service),
+		},
 
 		/* add more tests for other dropped fields as needed */
 	}