diff --git a/pkg/apis/core/validation/conditional_validation.go b/pkg/apis/core/validation/conditional_validation.go index e11731686bb..fc7661a2bdf 100644 --- a/pkg/apis/core/validation/conditional_validation.go +++ b/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. -func ValidateConditionalService(service, oldService *api.Service) field.ErrorList { +// ValidateConditionalService validates conditionally valid fields. allowedIPFamilies is an ordered +// 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 diff --git a/pkg/apis/core/validation/conditional_validation_test.go b/pkg/apis/core/validation/conditional_validation_test.go index 787d1d70ddf..b6a836eb145 100644 --- a/pkg/apis/core/validation/conditional_validation_test.go +++ b/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]) + } + } + }) + } +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index a72810682f5..e447b8b0ad5 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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 diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index d3c0a75cdf8..26e82c13f04 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/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", diff --git a/pkg/registry/core/rest/storage_core.go b/pkg/registry/core/rest/storage_core.go index 4967e014290..9cbfd2d64d5 100644 --- a/pkg/registry/core/rest/storage_core.go +++ b/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, diff --git a/pkg/registry/core/service/BUILD b/pkg/registry/core/service/BUILD index bb5c9d99943..7974b50fe4a 100644 --- a/pkg/registry/core/service/BUILD +++ b/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", ], ) diff --git a/pkg/registry/core/service/storage/BUILD b/pkg/registry/core/service/storage/BUILD index a38b91a919b..feb59391435 100644 --- a/pkg/registry/core/service/storage/BUILD +++ b/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", diff --git a/pkg/registry/core/service/storage/rest.go b/pkg/registry/core/service/storage/rest.go index 66729e3617f..0d5379f8c11 100644 --- a/pkg/registry/core/service/storage/rest.go +++ b/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 - endpoints EndpointsStorage - serviceIPs ipallocator.Interface - secondaryServiceIPs ipallocator.Interface - defaultServiceIPFamily api.IPFamily - serviceNodePorts portallocator.Interface - proxyTransport http.RoundTripper - pods rest.Getter + strategy rest.RESTCreateUpdateStrategy + services ServiceStorage + endpoints EndpointsStorage + serviceIPs ipallocator.Interface + secondaryServiceIPs ipallocator.Interface + serviceNodePorts portallocator.Interface + proxyTransport http.RoundTripper + 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, - endpoints: endpoints, - serviceIPs: serviceIPs, - secondaryServiceIPs: secondaryServiceIPs, - serviceNodePorts: serviceNodePorts, - defaultServiceIPFamily: serviceIPFamily, - proxyTransport: proxyTransport, - pods: pods, + strategy: strategy, + services: services, + endpoints: endpoints, + serviceIPs: serviceIPs, + secondaryServiceIPs: secondaryServiceIPs, + serviceNodePorts: serviceNodePorts, + proxyTransport: proxyTransport, + pods: pods, } + return rest, ®istry.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 utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && service.Spec.IPFamily == nil { - service.Spec.IPFamily = &rs.defaultServiceIPFamily - } - - if err := rest.BeforeCreate(registry.Strategy, ctx, obj); err != nil { + if err := rest.BeforeCreate(rs.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 { diff --git a/pkg/registry/core/service/storage/rest_test.go b/pkg/registry/core/service/storage/rest_test.go index 4d811ecda4f..043563c23df 100644 --- a/pkg/registry/core/service/storage/rest_test.go +++ b/pkg/registry/core/service/storage/rest_test.go @@ -29,6 +29,7 @@ import ( metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/intstr" utilnet "k8s.io/apimachinery/pkg/util/net" "k8s.io/apimachinery/pkg/util/rand" @@ -52,6 +53,13 @@ import ( "k8s.io/kubernetes/pkg/features" ) +var ( + singleStackIPv4 = []api.IPFamily{api.IPv4Protocol} + singleStackIPv6 = []api.IPFamily{api.IPv6Protocol} + dualStackIPv4Primary = []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol} + dualStackIPv6Primary = []api.IPFamily{api.IPv6Protocol, api.IPv4Protocol} +) + // TODO(wojtek-t): Cleanup this file. // It is now testing mostly the same things as other resources but // in a completely different way. We should unify it. @@ -170,11 +178,11 @@ func generateRandomNodePort() int32 { return int32(rand.IntnRange(30001, 30999)) } -func NewTestREST(t *testing.T, endpoints *api.EndpointsList, dualStack bool) (*REST, *serviceStorage, *etcd3testing.EtcdTestServer) { - return NewTestRESTWithPods(t, endpoints, nil, dualStack) +func NewTestREST(t *testing.T, endpoints *api.EndpointsList, ipFamilies []api.IPFamily) (*REST, *serviceStorage, *etcd3testing.EtcdTestServer) { + return NewTestRESTWithPods(t, endpoints, nil, ipFamilies) } -func NewTestRESTWithPods(t *testing.T, endpoints *api.EndpointsList, pods *api.PodList, dualStack bool) (*REST, *serviceStorage, *etcd3testing.EtcdTestServer) { +func NewTestRESTWithPods(t *testing.T, endpoints *api.EndpointsList, pods *api.PodList, ipFamilies []api.IPFamily) (*REST, *serviceStorage, *etcd3testing.EtcdTestServer) { etcdStorage, server := registrytest.NewEtcdStorage(t, "") serviceStorage := &serviceStorage{} @@ -215,15 +223,31 @@ func NewTestRESTWithPods(t *testing.T, endpoints *api.EndpointsList, pods *api.P } } - r, err := ipallocator.NewCIDRRange(makeIPNet(t)) - if err != nil { - t.Fatalf("cannot create CIDR Range %v", err) - } + var rPrimary ipallocator.Interface var rSecondary ipallocator.Interface - if dualStack { - rSecondary, err = ipallocator.NewCIDRRange(makeIPNet6(t)) - if err != nil { - t.Fatalf("cannot create CIDR Range(secondary) %v", err) + + if len(ipFamilies) < 1 || len(ipFamilies) > 2 { + t.Fatalf("unexpected ipfamilies passed: %v", ipFamilies) + } + for i, family := range ipFamilies { + var r ipallocator.Interface + switch family { + case api.IPv4Protocol: + r, err = ipallocator.NewCIDRRange(makeIPNet(t)) + if err != nil { + t.Fatalf("cannot create CIDR Range %v", err) + } + case api.IPv6Protocol: + r, err = ipallocator.NewCIDRRange(makeIPNet6(t)) + if err != nil { + t.Fatalf("cannot create CIDR Range %v", err) + } + } + switch i { + case 0: + rPrimary = r + case 1: + rSecondary = r } } @@ -233,7 +257,7 @@ func NewTestRESTWithPods(t *testing.T, endpoints *api.EndpointsList, pods *api.P t.Fatalf("cannot create port allocator %v", err) } - rest, _ := NewREST(serviceStorage, endpointStorage, podStorage.Pod, r, rSecondary, portAllocator, nil) + rest, _ := NewREST(serviceStorage, endpointStorage, podStorage.Pod, rPrimary, rSecondary, portAllocator, nil) return rest, serviceStorage, server } @@ -253,20 +277,13 @@ func makeIPNet6(t *testing.T) *net.IPNet { return net } -func ipnetGet(t *testing.T, secondary bool) *net.IPNet { - if secondary { +func ipnetGet(t *testing.T, useIPv6 bool) *net.IPNet { + if useIPv6 { return makeIPNet6(t) } return makeIPNet(t) } -func allocGet(r *REST, secondary bool) ipallocator.Interface { - if secondary { - return r.secondaryServiceIPs - } - return r.serviceIPs -} - func releaseServiceNodePorts(t *testing.T, ctx context.Context, svcName string, rest *REST, registry ServiceStorage) { obj, err := registry.Get(ctx, svcName, &metav1.GetOptions{}) if err != nil { @@ -293,13 +310,14 @@ func TestServiceRegistryCreate(t *testing.T) { testCases := []struct { svc *api.Service name string + families []api.IPFamily enableDualStack bool - useSecondary bool + expectErr string }{ { name: "Service IPFamily default cluster dualstack:off", enableDualStack: false, - useSecondary: false, + families: singleStackIPv4, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -315,9 +333,8 @@ func TestServiceRegistryCreate(t *testing.T) { }, }, { - name: "Service IPFamily:v4 dualstack off", - enableDualStack: false, - useSecondary: false, + name: "Service IPFamily:v4 dualstack off", + families: singleStackIPv4, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -336,7 +353,7 @@ func TestServiceRegistryCreate(t *testing.T) { { name: "Service IPFamily:v4 dualstack on", enableDualStack: true, - useSecondary: false, + families: singleStackIPv4, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -355,7 +372,7 @@ func TestServiceRegistryCreate(t *testing.T) { { name: "Service IPFamily:v6 dualstack on", enableDualStack: true, - useSecondary: true, + families: singleStackIPv6, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -371,17 +388,64 @@ func TestServiceRegistryCreate(t *testing.T) { }, }, }, + { + name: "Service IPFamily:4 dualstack on, single stack ipv6, service CIDR IPv6", + enableDualStack: true, + families: singleStackIPv6, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &ipv4Service, + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + expectErr: "Service \"foo\" is invalid: spec.ipFamily: Invalid value: \"IPv4\": only the following families are allowed: IPv6", + }, + { + name: "Service IP:4 dualstack on, single stack ipv6, service CIDR IPv6", + enableDualStack: true, + families: singleStackIPv6, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + ClusterIP: "10.0.30.0", + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + expectErr: "Service \"foo\" is invalid: spec.ipFamily: Invalid value: \"IPv6\": does not match IPv4 cluster IP", + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)() - storage, registry, server := NewTestREST(t, nil, tc.enableDualStack) + + storage, registry, server := NewTestREST(t, nil, tc.families) defer server.Terminate(t) ctx := genericapirequest.NewDefaultContext() createdSvc, err := storage.Create(ctx, tc.svc, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if (len(tc.expectErr) > 0) != (err != nil) { + t.Fatalf("unexpected error: %v", err) + } if err != nil { - t.Fatalf("Unexpected error: %v", err) + if !strings.Contains(err.Error(), tc.expectErr) { + t.Fatalf("unexpected error message: %v", err) + } + return } createdService := createdSvc.(*api.Service) objMeta, err := meta.Accessor(createdService) @@ -397,7 +461,7 @@ func TestServiceRegistryCreate(t *testing.T) { if createdService.CreationTimestamp.IsZero() { t.Errorf("Expected timestamp to be set, got: %v", createdService.CreationTimestamp) } - allocNet := ipnetGet(t, tc.useSecondary) + allocNet := ipnetGet(t, tc.families[0] == api.IPv6Protocol) if !allocNet.Contains(net.ParseIP(createdService.Spec.ClusterIP)) { t.Errorf("Unexpected ClusterIP: %s", createdService.Spec.ClusterIP) @@ -419,12 +483,12 @@ func TestServiceRegistryCreateDryRun(t *testing.T) { name string svc *api.Service enableDualStack bool - useSecondary bool + families []api.IPFamily }{ { - name: "v4 service", + name: "v4 service featuregate off", enableDualStack: false, - useSecondary: false, + families: singleStackIPv4, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -441,9 +505,130 @@ func TestServiceRegistryCreateDryRun(t *testing.T) { }, }, { - name: "v6 service", + name: "v4 service featuregate on but singlestack", enableDualStack: true, - useSecondary: true, + families: singleStackIPv4, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + ClusterIP: "1.2.3.4", + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + }, + { + name: "v6 service featuregate off", + enableDualStack: false, + families: singleStackIPv6, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &ipv6Service, + ClusterIP: "2000:0:0:0:0:0:0:1", + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + }, + { + name: "v6 service featuregate on but singlestack", + enableDualStack: true, + families: singleStackIPv6, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &ipv6Service, + ClusterIP: "2000:0:0:0:0:0:0:1", + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + }, + + { + name: "v4 service dualstack ipv4 primary", + enableDualStack: true, + families: dualStackIPv4Primary, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + ClusterIP: "1.2.3.4", + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + }, + + { + name: "v4 service dualstack ipv6 primary", + enableDualStack: true, + families: dualStackIPv6Primary, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &singleStackIPv4[0], + ClusterIP: "1.2.3.4", + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + }, + + { + name: "v6 service dualstack ipv4 primary", + enableDualStack: true, + families: dualStackIPv4Primary, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &ipv6Service, + ClusterIP: "2000:0:0:0:0:0:0:1", + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + }, + { + name: "v6 service dualstack ipv6 primary", + enableDualStack: true, + families: dualStackIPv6Primary, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -465,7 +650,7 @@ func TestServiceRegistryCreateDryRun(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)() - storage, registry, server := NewTestREST(t, nil, tc.enableDualStack) + storage, registry, server := NewTestREST(t, nil, tc.families) defer server.Terminate(t) ctx := genericapirequest.NewDefaultContext() @@ -473,11 +658,16 @@ func TestServiceRegistryCreateDryRun(t *testing.T) { if err != nil { t.Fatalf("Unexpected error: %v", err) } - alloc := allocGet(storage, tc.useSecondary) - if alloc.Has(net.ParseIP(tc.svc.Spec.ClusterIP)) { + if storage.serviceIPs.Has(net.ParseIP(tc.svc.Spec.ClusterIP)) { t.Errorf("unexpected side effect: ip allocated") } + if storage.secondaryServiceIPs != nil { + if storage.secondaryServiceIPs.Has(net.ParseIP(tc.svc.Spec.ClusterIP)) { + t.Errorf("unexpected side effect: secondary ip allocated") + } + } + srv, err := registry.GetService(ctx, tc.svc.Name, &metav1.GetOptions{}) if err != nil { t.Errorf("unexpected error: %v", err) @@ -490,7 +680,7 @@ func TestServiceRegistryCreateDryRun(t *testing.T) { } func TestDryRunNodePort(t *testing.T) { - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) // Test dry run create request with a node port @@ -610,7 +800,7 @@ func TestDryRunNodePort(t *testing.T) { func TestServiceRegistryCreateMultiNodePortsService(t *testing.T) { - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) testCases := []struct { @@ -740,7 +930,7 @@ func TestServiceRegistryCreateMultiNodePortsService(t *testing.T) { } func TestServiceStorageValidatesCreate(t *testing.T) { - storage, _, server := NewTestREST(t, nil, false) + storage, _, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) failureCases := map[string]api.Service{ "empty ID": { @@ -793,62 +983,191 @@ func TestServiceStorageValidatesCreate(t *testing.T) { } func TestServiceRegistryUpdate(t *testing.T) { - ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) - defer server.Terminate(t) + testCases := []struct { + name string + in *api.Service + update func(*api.Service) + out *api.Service - obj, err := registry.Create(ctx, &api.Service{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1", Namespace: metav1.NamespaceDefault}, - Spec: api.ServiceSpec{ - Selector: map[string]string{"bar": "baz1"}, - Ports: []api.ServicePort{{ - Port: 6502, - Protocol: api.ProtocolTCP, - TargetPort: intstr.FromInt(6502), - }}, + enableDualStack bool + families []api.IPFamily + + expect *api.Service + expectErr string + }{ + { + name: "simple update", + families: singleStackIPv4, // not actually relevant to test + in: &api.Service{ + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz1"}, + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + update: func(svc *api.Service) { + svc.Spec = api.ServiceSpec{ + Selector: map[string]string{"bar": "baz2"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + } + }, }, - }, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) - svc := obj.(*api.Service) - if err != nil { - t.Fatalf("Expected no error: %v", err) - } - updatedSvc, created, err := storage.Update(ctx, "foo", rest.DefaultUpdatedObjectInfo(&api.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - ResourceVersion: svc.ResourceVersion}, - Spec: api.ServiceSpec{ - Selector: map[string]string{"bar": "baz2"}, - SessionAffinity: api.ServiceAffinityNone, - Type: api.ServiceTypeClusterIP, - Ports: []api.ServicePort{{ - Port: 6502, - Protocol: api.ProtocolTCP, - TargetPort: intstr.FromInt(6502), - }}, + { + name: "ipv4: update to service that drops IPFamily succeeds", + enableDualStack: true, + families: singleStackIPv4, + in: &api.Service{ + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz1"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + update: func(svc *api.Service) { + svc.Spec.IPFamily = nil + }, + out: &api.Service{ + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz1"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &singleStackIPv4[0], + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + }, + { + name: "ipv6: update to service that drops IPFamily succeeds", + enableDualStack: true, + families: singleStackIPv6, + in: &api.Service{ + Spec: api.ServiceSpec{ + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &singleStackIPv6[0], + Selector: map[string]string{"bar": "baz1"}, + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + update: func(svc *api.Service) { + svc.Spec.IPFamily = nil + }, + out: &api.Service{ + Spec: api.ServiceSpec{ + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &singleStackIPv6[0], + Selector: map[string]string{"bar": "baz1"}, + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + }, + { + name: "ipv6: changing IPFamily fails", + enableDualStack: true, + families: singleStackIPv6, + in: &api.Service{ + Spec: api.ServiceSpec{ + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &singleStackIPv6[0], + Selector: map[string]string{"bar": "baz1"}, + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + update: func(svc *api.Service) { + svc.Spec.IPFamily = &singleStackIPv4[0] + }, + expectErr: "spec.ipFamily: Invalid value: \"IPv4\": field is immutable, spec.ipFamily: Invalid value: \"IPv4\": only the following families are allowed: IPv6", }, - }), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) - if err != nil { - t.Fatalf("Expected no error: %v", err) } - if updatedSvc == nil { - t.Errorf("Expected non-nil object") - } - if created { - t.Errorf("expected not created") - } - updatedService := updatedSvc.(*api.Service) - if updatedService.Name != "foo" { - t.Errorf("Expected foo, but got %v", updatedService.Name) - } - if e, a := "foo", registry.UpdatedID; e != a { - t.Errorf("Expected %v, but got %v", e, a) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)() + ctx := genericapirequest.NewDefaultContext() + storage, registry, server := NewTestREST(t, nil, tc.families) + defer server.Terminate(t) + + tc.in.ObjectMeta = metav1.ObjectMeta{Name: "foo", ResourceVersion: "1", Namespace: metav1.NamespaceDefault} + + obj, err := registry.Create(ctx, tc.in, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + svc := obj.(*api.Service) + if err != nil { + t.Fatalf("Expected no error: %v", err) + } + t.Logf("%#v", svc) + tc.update(svc) + updatedSvc, created, err := storage.Update(ctx, "foo", rest.DefaultUpdatedObjectInfo(svc), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if (len(tc.expectErr) > 0) != (err != nil) { + t.Fatalf("unxpected error: %v", err) + } + if err != nil { + if !strings.Contains(err.Error(), tc.expectErr) { + t.Fatalf("unexpected error: %v", err) + } + return + } + if updatedSvc == nil { + t.Errorf("Expected non-nil object") + } + if created { + t.Errorf("expected not created") + } + updatedService := updatedSvc.(*api.Service) + if updatedService.Name != "foo" { + t.Errorf("Expected foo, but got %v", updatedService.Name) + } + + expected := svc + if tc.out != nil { + expected = tc.out + } + expected.ObjectMeta = updatedService.ObjectMeta + if !reflect.DeepEqual(expected, updatedSvc) { + t.Fatalf("unexpected object: %s", diff.ObjectReflectDiff(expected, updatedService)) + } + if e, a := "foo", registry.UpdatedID; e != a { + t.Errorf("Expected %v, but got %v", e, a) + } + }) + } } func TestServiceRegistryUpdateDryRun(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) obj, err := registry.Create(ctx, &api.Service{ @@ -1013,7 +1332,7 @@ func TestServiceRegistryUpdateDryRun(t *testing.T) { func TestServiceStorageValidatesUpdate(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) registry.Create(ctx, &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, @@ -1066,7 +1385,7 @@ func TestServiceStorageValidatesUpdate(t *testing.T) { func TestServiceRegistryExternalService(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc := &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, @@ -1105,7 +1424,7 @@ func TestServiceRegistryExternalService(t *testing.T) { func TestServiceRegistryDelete(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc := &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, @@ -1128,7 +1447,7 @@ func TestServiceRegistryDelete(t *testing.T) { func TestServiceRegistryDeleteDryRun(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) // Test dry run delete request with cluster ip @@ -1194,7 +1513,7 @@ func TestServiceRegistryDeleteDryRun(t *testing.T) { func TestServiceRegistryDeleteExternal(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc := &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, @@ -1217,7 +1536,7 @@ func TestServiceRegistryDeleteExternal(t *testing.T) { func TestServiceRegistryUpdateExternalService(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) // Create non-external load balancer. @@ -1256,7 +1575,7 @@ func TestServiceRegistryUpdateExternalService(t *testing.T) { func TestServiceRegistryUpdateMultiPortExternalService(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) // Create external load balancer. @@ -1294,7 +1613,7 @@ func TestServiceRegistryUpdateMultiPortExternalService(t *testing.T) { func TestServiceRegistryGet(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) registry.Create(ctx, &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, @@ -1380,7 +1699,7 @@ func TestServiceRegistryResourceLocation(t *testing.T) { }, }, } - storage, registry, server := NewTestRESTWithPods(t, endpoints, pods, false) + storage, registry, server := NewTestRESTWithPods(t, endpoints, pods, singleStackIPv4) defer server.Terminate(t) for _, name := range []string{"foo", "bad"} { registry.Create(ctx, &api.Service{ @@ -1546,7 +1865,7 @@ func TestServiceRegistryResourceLocation(t *testing.T) { func TestServiceRegistryList(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) registry.Create(ctx, &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: metav1.NamespaceDefault}, @@ -1578,7 +1897,7 @@ func TestServiceRegistryList(t *testing.T) { } func TestServiceRegistryIPAllocation(t *testing.T) { - storage, _, server := NewTestREST(t, nil, false) + storage, _, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc1 := &api.Service{ @@ -1661,7 +1980,7 @@ func TestServiceRegistryIPAllocation(t *testing.T) { } func TestServiceRegistryIPReallocation(t *testing.T) { - storage, _, server := NewTestREST(t, nil, false) + storage, _, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc1 := &api.Service{ @@ -1717,7 +2036,7 @@ func TestServiceRegistryIPReallocation(t *testing.T) { } func TestServiceRegistryIPUpdate(t *testing.T) { - storage, _, server := NewTestREST(t, nil, false) + storage, _, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc := &api.Service{ @@ -1772,7 +2091,7 @@ func TestServiceRegistryIPUpdate(t *testing.T) { } func TestServiceRegistryIPLoadBalancer(t *testing.T) { - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc := &api.Service{ @@ -1812,7 +2131,7 @@ func TestServiceRegistryIPLoadBalancer(t *testing.T) { } func TestUpdateServiceWithConflictingNamespace(t *testing.T) { - storage, _, server := NewTestREST(t, nil, false) + storage, _, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) service := &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "not-default"}, @@ -1834,7 +2153,7 @@ func TestUpdateServiceWithConflictingNamespace(t *testing.T) { // and type is LoadBalancer. func TestServiceRegistryExternalTrafficHealthCheckNodePortAllocation(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc := &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "external-lb-esipp"}, @@ -1874,7 +2193,7 @@ func TestServiceRegistryExternalTrafficHealthCheckNodePortAllocation(t *testing. func TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation(t *testing.T) { randomNodePort := generateRandomNodePort() ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc := &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "external-lb-esipp"}, @@ -1917,7 +2236,7 @@ func TestServiceRegistryExternalTrafficHealthCheckNodePortUserAllocation(t *test // Validate that the service creation fails when the requested port number is -1. func TestServiceRegistryExternalTrafficHealthCheckNodePortNegative(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, _, server := NewTestREST(t, nil, false) + storage, _, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc := &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "external-lb-esipp"}, @@ -1944,7 +2263,7 @@ func TestServiceRegistryExternalTrafficHealthCheckNodePortNegative(t *testing.T) // Validate that the health check nodePort is not allocated when ExternalTrafficPolicy is set to Global. func TestServiceRegistryExternalTrafficGlobal(t *testing.T) { ctx := genericapirequest.NewDefaultContext() - storage, registry, server := NewTestREST(t, nil, false) + storage, registry, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) svc := &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "external-lb-esipp"}, @@ -1986,14 +2305,16 @@ func TestInitClusterIP(t *testing.T) { name string svc *api.Service + enableDualStack bool + families []api.IPFamily + useSecondaryAlloc bool + expectClusterIP bool - enableDualStack bool - allocateSpecificIP bool - useSecondaryAlloc bool expectedAllocatedIP string }{ { - name: "Allocate new ClusterIP", + name: "Allocate new ClusterIP", + families: singleStackIPv4, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -2008,10 +2329,12 @@ func TestInitClusterIP(t *testing.T) { }, }, expectClusterIP: true, - enableDualStack: false, }, { - name: "Allocate new ClusterIP-v6", + name: "Allocate new ClusterIP-v6 dualstack ipv4 primary", + useSecondaryAlloc: true, + enableDualStack: true, + families: dualStackIPv4Primary, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -2026,12 +2349,53 @@ func TestInitClusterIP(t *testing.T) { }}, }, }, - expectClusterIP: true, - useSecondaryAlloc: true, - enableDualStack: true, + expectClusterIP: true, }, { - name: "Allocate specified ClusterIP", + name: "Allocate new ClusterIP-v6 dualstack ipv6 primary", + useSecondaryAlloc: true, + enableDualStack: true, + families: dualStackIPv6Primary, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &ipv6Service, + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + expectClusterIP: true, + }, + { + name: "Allocate new ClusterIP-v6 single stack", + families: singleStackIPv6, + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + SessionAffinity: api.ServiceAffinityNone, + Type: api.ServiceTypeClusterIP, + IPFamily: &ipv6Service, + Ports: []api.ServicePort{{ + Port: 6502, + Protocol: api.ProtocolTCP, + TargetPort: intstr.FromInt(6502), + }}, + }, + }, + expectClusterIP: true, + }, + { + name: "Allocate specified ClusterIP", + enableDualStack: true, + useSecondaryAlloc: true, + families: dualStackIPv6Primary, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -2048,12 +2412,12 @@ func TestInitClusterIP(t *testing.T) { }, }, expectClusterIP: true, - allocateSpecificIP: true, expectedAllocatedIP: "1.2.3.4", - enableDualStack: true, }, { - name: "Allocate specified ClusterIP-v6", + name: "Allocate specified ClusterIP-v6", + enableDualStack: true, + families: dualStackIPv6Primary, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -2070,13 +2434,11 @@ func TestInitClusterIP(t *testing.T) { }, }, expectClusterIP: true, - allocateSpecificIP: true, expectedAllocatedIP: "2000:0:0:0:0:0:0:1", - useSecondaryAlloc: true, - enableDualStack: true, }, { - name: "Shouldn't allocate ClusterIP", + name: "Shouldn't allocate ClusterIP", + families: singleStackIPv4, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, Spec: api.ServiceSpec{ @@ -2095,42 +2457,43 @@ func TestInitClusterIP(t *testing.T) { }, } - for _, test := range testCases { - t.Run(test.name, func(t *testing.T) { + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)() - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, test.enableDualStack)() - - storage, _, server := NewTestREST(t, nil, test.enableDualStack) + storage, _, server := NewTestREST(t, nil, tc.families) defer server.Terminate(t) - whichAlloc := allocGet(storage, test.useSecondaryAlloc) - hasAllocatedIP, err := initClusterIP(test.svc, whichAlloc) + allocator := storage.serviceIPs + if tc.useSecondaryAlloc { + allocator = storage.secondaryServiceIPs + } + allocateSpecificIP := len(tc.svc.Spec.ClusterIP) > 0 && tc.svc.Spec.ClusterIP != "None" + hasAllocatedIP, err := initClusterIP(tc.svc, allocator) if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } - if hasAllocatedIP != test.expectClusterIP { - t.Errorf("expected %v, but got %v", test.expectClusterIP, hasAllocatedIP) + if hasAllocatedIP != tc.expectClusterIP { + t.Errorf("expected %v, but got %v", tc.expectClusterIP, hasAllocatedIP) } - if test.expectClusterIP { - alloc := allocGet(storage, test.useSecondaryAlloc) - if !alloc.Has(net.ParseIP(test.svc.Spec.ClusterIP)) { - t.Errorf("unexpected ClusterIP %q, out of range", test.svc.Spec.ClusterIP) + if tc.expectClusterIP { + if !allocator.Has(net.ParseIP(tc.svc.Spec.ClusterIP)) { + t.Errorf("unexpected ClusterIP %q, not allocated", tc.svc.Spec.ClusterIP) } } - if test.allocateSpecificIP && test.expectedAllocatedIP != test.svc.Spec.ClusterIP { - t.Errorf(" expected ClusterIP %q, but got %q", test.expectedAllocatedIP, test.svc.Spec.ClusterIP) + if allocateSpecificIP && tc.expectedAllocatedIP != tc.svc.Spec.ClusterIP { + t.Errorf(" expected ClusterIP %q, but got %q", tc.expectedAllocatedIP, tc.svc.Spec.ClusterIP) } - }) } } func TestInitNodePorts(t *testing.T) { - storage, _, server := NewTestREST(t, nil, false) + storage, _, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) nodePortOp := portallocator.StartOperation(storage.serviceNodePorts, false) defer nodePortOp.Finish() @@ -2312,7 +2675,7 @@ func TestInitNodePorts(t *testing.T) { } func TestUpdateNodePorts(t *testing.T) { - storage, _, server := NewTestREST(t, nil, false) + storage, _, server := NewTestREST(t, nil, singleStackIPv4) defer server.Terminate(t) nodePortOp := portallocator.StartOperation(storage.serviceNodePorts, false) defer nodePortOp.Finish() @@ -2588,20 +2951,22 @@ func TestAllocGetters(t *testing.T) { testCases := []struct { name string - isIPv6Primary bool - enableDualStack bool - specExpctPrimary bool - clusterIPExpectPrimary bool + enableDualStack bool + families []api.IPFamily + + expectSpecPrimary bool + expectClusterIPPrimary bool svc *api.Service }{ { - name: "spec:v4 ip:v4 dualstack:off isIPv6Primary:false", + name: "spec:v4 ip:v4 dualstack:off", - isIPv6Primary: false, - specExpctPrimary: true, - clusterIPExpectPrimary: true, - enableDualStack: false, + enableDualStack: false, + families: singleStackIPv4, + + expectSpecPrimary: true, + expectClusterIPPrimary: true, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, @@ -2614,12 +2979,13 @@ func TestAllocGetters(t *testing.T) { }, }, { - name: "spec:v4 ip:v4 dualstack:on isIPv6Primary:false", + name: "spec:v4 ip:v4 dualstack:on primary:v4", - isIPv6Primary: false, - specExpctPrimary: true, - clusterIPExpectPrimary: true, - enableDualStack: true, + enableDualStack: true, + families: dualStackIPv4Primary, + + expectSpecPrimary: true, + expectClusterIPPrimary: true, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, @@ -2633,12 +2999,13 @@ func TestAllocGetters(t *testing.T) { }, { - name: "spec:v4 ip:v6 dualstack:on isIPv6Primary:false", + name: "spec:v4 ip:v6 dualstack:on primary:v4", - isIPv6Primary: false, - specExpctPrimary: true, - clusterIPExpectPrimary: false, - enableDualStack: true, + enableDualStack: true, + families: dualStackIPv4Primary, + + expectSpecPrimary: true, + expectClusterIPPrimary: false, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, @@ -2652,12 +3019,13 @@ func TestAllocGetters(t *testing.T) { }, { - name: "spec:v6 ip:v6 dualstack:on isIPv6Primary:false", + name: "spec:v6 ip:v6 dualstack:on primary:v4", - isIPv6Primary: false, - specExpctPrimary: false, - clusterIPExpectPrimary: false, - enableDualStack: true, + enableDualStack: true, + families: dualStackIPv4Primary, + + expectSpecPrimary: false, + expectClusterIPPrimary: false, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, @@ -2671,12 +3039,13 @@ func TestAllocGetters(t *testing.T) { }, { - name: "spec:v6 ip:v4 dualstack:on isIPv6Primary:false", + name: "spec:v6 ip:v4 dualstack:on primary:v4", - isIPv6Primary: false, - specExpctPrimary: false, - clusterIPExpectPrimary: true, - enableDualStack: true, + enableDualStack: true, + families: dualStackIPv4Primary, + + expectSpecPrimary: false, + expectClusterIPPrimary: true, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, @@ -2688,31 +3057,15 @@ func TestAllocGetters(t *testing.T) { }, }, }, + { - name: "spec:v6 ip:v6 dualstack:off isIPv6Primary:true", + name: "spec:v6 ip:v6 dualstack:off", - isIPv6Primary: true, - specExpctPrimary: true, - clusterIPExpectPrimary: true, - enableDualStack: false, + enableDualStack: false, + families: singleStackIPv6, - svc: &api.Service{ - ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, - Spec: api.ServiceSpec{ - Selector: map[string]string{"bar": "baz"}, - Type: api.ServiceTypeClusterIP, - IPFamily: &ipv6Service, - ClusterIP: "2000::1", - }, - }, - }, - { - name: "spec:v6 ip:v6 dualstack:on isIPv6Primary:true", - - isIPv6Primary: true, - specExpctPrimary: true, - clusterIPExpectPrimary: true, - enableDualStack: true, + expectSpecPrimary: true, + expectClusterIPPrimary: true, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, @@ -2726,12 +3079,33 @@ func TestAllocGetters(t *testing.T) { }, { - name: "spec:v6 ip:v4 dualstack:on isIPv6Primary:true", + name: "spec:v6 ip:v6 dualstack:on primary:v6", - isIPv6Primary: true, - specExpctPrimary: true, - clusterIPExpectPrimary: false, - enableDualStack: true, + enableDualStack: true, + families: dualStackIPv6Primary, + + expectSpecPrimary: true, + expectClusterIPPrimary: true, + + svc: &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, + Spec: api.ServiceSpec{ + Selector: map[string]string{"bar": "baz"}, + Type: api.ServiceTypeClusterIP, + IPFamily: &ipv6Service, + ClusterIP: "2000::1", + }, + }, + }, + + { + name: "spec:v6 ip:v4 dualstack:on primary:v6", + + enableDualStack: true, + families: dualStackIPv6Primary, + + expectSpecPrimary: true, + expectClusterIPPrimary: false, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, @@ -2745,12 +3119,13 @@ func TestAllocGetters(t *testing.T) { }, { - name: "spec:v4 ip:v4 dualstack:on isIPv6Primary:true", + name: "spec:v4 ip:v4 dualstack:on primary:v6", - isIPv6Primary: true, - specExpctPrimary: false, - clusterIPExpectPrimary: false, - enableDualStack: true, + enableDualStack: true, + families: dualStackIPv6Primary, + + expectSpecPrimary: false, + expectClusterIPPrimary: false, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, @@ -2764,12 +3139,13 @@ func TestAllocGetters(t *testing.T) { }, { - name: "spec:v4 ip:v6 dualstack:on isIPv6Primary:true", + name: "spec:v4 ip:v6 dualstack:on primary:v6", - isIPv6Primary: true, - specExpctPrimary: false, - clusterIPExpectPrimary: true, - enableDualStack: true, + enableDualStack: true, + families: dualStackIPv6Primary, + + expectSpecPrimary: false, + expectClusterIPPrimary: true, svc: &api.Service{ ObjectMeta: metav1.ObjectMeta{Name: "foo", ResourceVersion: "1"}, @@ -2786,50 +3162,34 @@ func TestAllocGetters(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)() - storage, _, server := NewTestREST(t, nil, tc.enableDualStack) + storage, _, server := NewTestREST(t, nil, tc.families) defer server.Terminate(t) - if tc.isIPv6Primary { - if storage.secondaryServiceIPs != nil { - storage.serviceIPs, storage.secondaryServiceIPs = storage.secondaryServiceIPs, storage.serviceIPs - } else { - r, err := ipallocator.NewCIDRRange(makeIPNet6(t)) - if err != nil { - t.Fatalf("cannot create CIDR Range(primary) %v", err) - } - if tc.enableDualStack { - storage.secondaryServiceIPs = storage.serviceIPs - } - storage.serviceIPs = r - } - } - - if tc.enableDualStack && storage.secondaryServiceIPs == nil { - t.Errorf("storage must allocate secondary ServiceIPs allocator for dual stack") - return + if len(tc.families) == 2 && storage.secondaryServiceIPs == nil { + t.Fatalf("storage must allocate secondary ServiceIPs allocator for dual stack") } alloc := storage.getAllocatorByClusterIP(tc.svc) - if tc.clusterIPExpectPrimary && !bytes.Equal(alloc.CIDR().IP, storage.serviceIPs.CIDR().IP) { - t.Errorf("expected primary allocator, but primary allocator was not selected") - return - } - - if tc.enableDualStack && !tc.clusterIPExpectPrimary && !bytes.Equal(alloc.CIDR().IP, storage.secondaryServiceIPs.CIDR().IP) { - t.Errorf("expected secondary allocator, but secondary allocator was not selected") + if tc.expectClusterIPPrimary { + if !bytes.Equal(alloc.CIDR().IP, storage.serviceIPs.CIDR().IP) { + t.Fatalf("expected clusterIP primary allocator, but primary allocator was not selected") + } + } else { + if !bytes.Equal(alloc.CIDR().IP, storage.secondaryServiceIPs.CIDR().IP) { + t.Errorf("expected clusterIP secondary allocator, but secondary allocator was not selected") + } } alloc = storage.getAllocatorBySpec(tc.svc) - if tc.specExpctPrimary && !bytes.Equal(alloc.CIDR().IP, storage.serviceIPs.CIDR().IP) { - t.Errorf("expected primary allocator, but primary allocator was not selected") - return + if tc.expectSpecPrimary { + if !bytes.Equal(alloc.CIDR().IP, storage.serviceIPs.CIDR().IP) { + t.Errorf("expected spec primary allocator, but primary allocator was not selected") + } + } else { + if !bytes.Equal(alloc.CIDR().IP, storage.secondaryServiceIPs.CIDR().IP) { + t.Errorf("expected spec secondary allocator, but secondary allocator was not selected") + } } - - if tc.enableDualStack && !tc.specExpctPrimary && !bytes.Equal(alloc.CIDR().IP, storage.secondaryServiceIPs.CIDR().IP) { - t.Errorf("expected secondary allocator, but secondary allocator was not selected") - } - }) } - } diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go index fa557935a05..4a1da79cb99 100644 --- a/pkg/registry/core/service/storage/storage.go +++ b/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, - UpdateStrategy: service.Strategy, - DeleteStrategy: service.Strategy, - ExportStrategy: service.Strategy, + CreateStrategy: strategy, + UpdateStrategy: strategy, + DeleteStrategy: 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 } diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index 0b8d2f1b6cb..0189024e946 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/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) } diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index b69bae1ee86..4e160b3cf04 100644 --- a/pkg/registry/core/service/strategy.go +++ b/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 -// objects. -var Strategy = svcStrategy{legacyscheme.Scheme, names.SimpleNameGenerator} +// StrategyForServiceCIDRs returns the appropriate service strategy for the given configuration. +func StrategyForServiceCIDRs(primaryCIDR net.IPNet, hasSecondary bool) (Strategy, api.IPFamily) { + // 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. -func (svcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { +// PrepareForCreate sets contextual defaults and clears fields that are not allowed to be set by end users on creation. +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. -func (svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { +// PrepareForUpdate sets contextual defaults and clears fields that are not allowed to be set by end users on update. +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. -var StatusStrategy = serviceStatusStrategy{Strategy} +// NewServiceStatusStrategy creates a status strategy for the provided base 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) { diff --git a/pkg/registry/core/service/strategy_test.go b/pkg/registry/core/service/strategy_test.go index 4f089b43246..630a08d9c8b 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/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 - tweakSvc func(oldSvc, newSvc *api.Service) // given basic valid services, each test case can customize them - expectErr bool + name string + enableDualStack 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() - newSvc := makeValidService() - tc.tweakSvc(&oldSvc, &newSvc) - ctx := genericapirequest.NewDefaultContext() - err := rest.BeforeUpdate(Strategy, ctx, runtime.Object(&oldSvc), runtime.Object(&newSvc)) - if tc.expectErr && err == nil { - t.Errorf("unexpected non-error for %q", tc.name) - } - if !tc.expectErr && err != nil { - t.Errorf("unexpected error for %q: %v", tc.name, err) - } + t.Run(tc.name, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)() + var cidr string + if tc.defaultIPv6 { + cidr = "ffd0::/64" + } else { + cidr = "172.30.0.0/16" + } + strategy, _ := newStrategy(cidr, tc.allowSecondary) + 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 */ }