From 975a678ecf1637e2b96d297f6046215f56f8bad5 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Fri, 10 Jun 2022 10:56:08 +0200 Subject: [PATCH] services strategy no longer depends on IPFamilies since the refactor on the Service API registry, the strategy for service no longer needs to keep information about the cluster configuration and its ipFamilies. --- pkg/registry/core/service/storage/storage.go | 16 ++--- pkg/registry/core/service/strategy.go | 65 +++----------------- pkg/registry/core/service/strategy_test.go | 25 ++------ 3 files changed, 21 insertions(+), 85 deletions(-) diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go index b5330da1df4..4ee5428e819 100644 --- a/pkg/registry/core/service/storage/storage.go +++ b/pkg/registry/core/service/storage/storage.go @@ -41,7 +41,6 @@ import ( "k8s.io/kubernetes/pkg/printers" printersinternal "k8s.io/kubernetes/pkg/printers/internalversion" printerstorage "k8s.io/kubernetes/pkg/printers/storage" - "k8s.io/kubernetes/pkg/registry/core/service" svcreg "k8s.io/kubernetes/pkg/registry/core/service" "k8s.io/kubernetes/pkg/registry/core/service/ipallocator" "k8s.io/kubernetes/pkg/registry/core/service/portallocator" @@ -86,18 +85,16 @@ func NewREST( pods PodStorage, proxyTransport http.RoundTripper) (*REST, *StatusREST, *svcreg.ProxyREST, error) { - strategy, _ := svcreg.StrategyForServiceCIDRs(ipAllocs[serviceIPFamily].CIDR(), len(ipAllocs) > 1) - store := &genericregistry.Store{ NewFunc: func() runtime.Object { return &api.Service{} }, NewListFunc: func() runtime.Object { return &api.ServiceList{} }, DefaultQualifiedResource: api.Resource("services"), ReturnDeletedObject: true, - CreateStrategy: strategy, - UpdateStrategy: strategy, - DeleteStrategy: strategy, - ResetFieldsStrategy: strategy, + CreateStrategy: svcreg.Strategy, + UpdateStrategy: svcreg.Strategy, + DeleteStrategy: svcreg.Strategy, + ResetFieldsStrategy: svcreg.Strategy, TableConvertor: printerstorage.TableConvertor{TableGenerator: printers.NewTableGenerator().With(printersinternal.AddHandlers)}, } @@ -107,9 +104,8 @@ func NewREST( } statusStore := *store - statusStrategy := service.NewServiceStatusStrategy(strategy) - statusStore.UpdateStrategy = statusStrategy - statusStore.ResetFieldsStrategy = statusStrategy + statusStore.UpdateStrategy = svcreg.StatusStrategy + statusStore.ResetFieldsStrategy = svcreg.StatusStrategy var primaryIPFamily api.IPFamily = serviceIPFamily var secondaryIPFamily api.IPFamily = "" // sentinel value diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index 6dc188d4c59..c7598b68ae4 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -18,74 +18,29 @@ package service import ( "context" - "net" "reflect" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "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" "sigs.k8s.io/structured-merge-diff/v4/fieldpath" ) -type Strategy interface { - rest.RESTCreateUpdateStrategy - rest.ResetFieldsStrategy -} - // svcStrategy implements behavior for Services type svcStrategy struct { runtime.ObjectTyper names.NameGenerator - - ipFamilies []api.IPFamily } -// 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 -} +// Strategy is the default logic that applies when creating and updating Services +// objects via the REST API. +var Strategy = svcStrategy{legacyscheme.Scheme, names.SimpleNameGenerator} // NamespaceScoped is true for services. func (svcStrategy) NamespaceScoped() bool { @@ -105,7 +60,7 @@ func (svcStrategy) GetResetFields() map[fieldpath.APIVersion]*fieldpath.Set { } // 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) { +func (svcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { service := obj.(*api.Service) service.Status = api.ServiceStatus{} @@ -113,7 +68,7 @@ func (strategy svcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Ob } // 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) { +func (svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { newService := obj.(*api.Service) oldService := old.(*api.Service) newService.Status = oldService.Status @@ -123,7 +78,7 @@ func (strategy svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runti } // Validate validates a new service. -func (strategy svcStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { +func (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)...) @@ -211,13 +166,11 @@ func serviceInternalTrafficPolicyInUse(svc *api.Service) bool { } type serviceStatusStrategy struct { - Strategy + svcStrategy } -// NewServiceStatusStrategy creates a status strategy for the provided base strategy. -func NewServiceStatusStrategy(strategy Strategy) Strategy { - return serviceStatusStrategy{strategy} -} +// StatusStrategy wraps and exports the used svcStrategy for the storage package. +var StatusStrategy = serviceStatusStrategy{Strategy} // GetResetFields returns the set of fields that get reset by the strategy // and should not be modified by the user. diff --git a/pkg/registry/core/service/strategy_test.go b/pkg/registry/core/service/strategy_test.go index 292a87ba784..7769a428b22 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/pkg/registry/core/service/strategy_test.go @@ -31,38 +31,26 @@ import ( api "k8s.io/kubernetes/pkg/apis/core" _ "k8s.io/kubernetes/pkg/apis/core/install" "k8s.io/kubernetes/pkg/features" - netutils "k8s.io/utils/net" utilpointer "k8s.io/utils/pointer" ) -func newStrategy(cidr string, hasSecondary bool) (testStrategy Strategy, testStatusStrategy Strategy) { - _, testCIDR, err := netutils.ParseCIDRSloppy(cidr) - if err != nil { - panic("invalid CIDR") - } - testStrategy, _ = StrategyForServiceCIDRs(*testCIDR, hasSecondary) - testStatusStrategy = NewServiceStatusStrategy(testStrategy) - return -} - func TestCheckGeneratedNameError(t *testing.T) { ctx := genericapirequest.WithRequestInfo(genericapirequest.NewContext(), &genericapirequest.RequestInfo{ Resource: "foos", }) - testStrategy, _ := newStrategy("10.0.0.0/16", false) expect := errors.NewNotFound(api.Resource("foos"), "bar") - if err := rest.CheckGeneratedNameError(ctx, testStrategy, expect, &api.Service{}); err != expect { + if err := rest.CheckGeneratedNameError(ctx, Strategy, expect, &api.Service{}); err != expect { t.Errorf("NotFoundError should be ignored: %v", err) } expect = errors.NewAlreadyExists(api.Resource("foos"), "bar") - if err := rest.CheckGeneratedNameError(ctx, testStrategy, expect, &api.Service{}); err != expect { + if err := rest.CheckGeneratedNameError(ctx, Strategy, 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(ctx, testStrategy, expect, &api.Service{ObjectMeta: metav1.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsAlreadyExists(err) { + if err := rest.CheckGeneratedNameError(ctx, Strategy, expect, &api.Service{ObjectMeta: metav1.ObjectMeta{GenerateName: "foo"}}); err == nil || !errors.IsAlreadyExists(err) { t.Errorf("expected try again later error: %v", err) } } @@ -114,9 +102,8 @@ func makeValidServiceCustom(tweaks ...func(svc *api.Service)) *api.Service { } func TestServiceStatusStrategy(t *testing.T) { - _, testStatusStrategy := newStrategy("10.0.0.0/16", false) ctx := genericapirequest.NewDefaultContext() - if !testStatusStrategy.NamespaceScoped() { + if !StatusStrategy.NamespaceScoped() { t.Errorf("Service must be namespace scoped") } oldService := makeValidService() @@ -131,14 +118,14 @@ func TestServiceStatusStrategy(t *testing.T) { }, }, } - testStatusStrategy.PrepareForUpdate(ctx, newService, oldService) + StatusStrategy.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 := testStatusStrategy.ValidateUpdate(ctx, newService, oldService) + errs := StatusStrategy.ValidateUpdate(ctx, newService, oldService) if len(errs) != 0 { t.Errorf("Unexpected error %v", errs) }