Merge pull request #110502 from aojea/clean_svc_strategies

services strategy no longer depends on IPFamilies

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

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.

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)
 	}