From c6b833ac3c7e921b7fd256ee627f8280f328b1ca Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Mon, 6 Jan 2020 19:50:04 -0500 Subject: [PATCH] service: fix IPFamily validation and defaulting problems If the dual-stack flag is enabled and the cluster is single stack IPv6, the allocator logic for service clusterIP does not properly handle rejecting a request for an IPv4 family. Return a 422 Invalid on the ipFamily field when the dual stack flag is on (as it would when it hits beta) and the cluster is configured for single-stack IPv6. The family is now defaulted or cleared in BeforeCreate/BeforeUpdate, and is either inherited from the previous object (if nil or unchanged), or set to the default strategy's family as necessary. The existing family defaulting when cluster ip is provided remains in the api section. We add additonal family defaulting at the time we allocate the IP to ensure that IPFamily is a consequence of the ClusterIP and prevent accidental reversion. This defaulting also ensures that old clients that submit a nil IPFamily for non ClusterIP services receive a default. To properly handle validation, make the strategy and the validation code path condition on which configuration options are passed to service storage. Move validation and preparation logic inside the strategy where it belongs. Service validation is now dependent on the configuration of the server, and as such ValidateConditionService needs to know what the allowed families are. --- .../core/validation/conditional_validation.go | 84 +- .../validation/conditional_validation_test.go | 240 ++++- pkg/apis/core/validation/validation.go | 25 +- pkg/apis/core/validation/validation_test.go | 12 +- pkg/registry/core/rest/storage_core.go | 10 +- pkg/registry/core/service/BUILD | 1 + pkg/registry/core/service/storage/BUILD | 1 + pkg/registry/core/service/storage/rest.go | 76 +- .../core/service/storage/rest_test.go | 848 +++++++++++++----- pkg/registry/core/service/storage/storage.go | 16 +- .../core/service/storage/storage_test.go | 2 +- pkg/registry/core/service/strategy.go | 90 +- pkg/registry/core/service/strategy_test.go | 221 ++++- 13 files changed, 1263 insertions(+), 363 deletions(-) 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 ce4ffa345a7..90df9f35e04 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 */ }