Create new conversion Factory interface

Create a new conversion Factory interface for CRDs, and split out
NewDelegatingConverter as a standalone package-level function, instead
of being part of CRConverterFactory.

Signed-off-by: Andy Goldstein <andy.goldstein@redhat.com>

cmd/kube-apiserver/app/apiextensions.go

@@ -23,6 +23,7 @@ import (
 	v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	apiextensionsapiserver "k8s.io/apiextensions-apiserver/pkg/apiserver"
+	"k8s.io/apiextensions-apiserver/pkg/apiserver/conversion"
 	apiextensionsoptions "k8s.io/apiextensions-apiserver/pkg/cmd/server/options"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -71,10 +72,17 @@ func createAPIExtensionsConfig(
 		apiextensionsapiserver.Scheme); err != nil {
 		return nil, err
 	}
+
 	crdRESTOptionsGetter, err := apiextensionsoptions.NewCRDRESTOptionsGetter(etcdOptions)
 	if err != nil {
 		return nil, err
 	}
+
+	conversionFactory, err := conversion.NewCRConverterFactory(serviceResolver, authResolverWrapper)
+	if err != nil {
+		return nil, err
+	}
+
 	apiextensionsConfig := &apiextensionsapiserver.Config{
 		GenericConfig: &genericapiserver.RecommendedConfig{
 			Config:                genericConfig,
@@ -83,8 +91,7 @@ func createAPIExtensionsConfig(
 		ExtraConfig: apiextensionsapiserver.ExtraConfig{
 			CRDRESTOptionsGetter: crdRESTOptionsGetter,
 			MasterCount:          masterCount,
-			AuthResolverWrapper:  authResolverWrapper,
-			ServiceResolver:      serviceResolver,
+			ConversionFactory:    conversionFactory,
 		},
 	}
 

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/install"
 	v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	"k8s.io/apiextensions-apiserver/pkg/apiserver/conversion"
 	"k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	externalinformers "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
 	"k8s.io/apiextensions-apiserver/pkg/controller/apiapproval"
@@ -48,7 +49,6 @@ import (
 	genericapiserver "k8s.io/apiserver/pkg/server"
 	serverstorage "k8s.io/apiserver/pkg/server/storage"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
-	"k8s.io/apiserver/pkg/util/webhook"
 )
 
 var (
@@ -83,10 +83,8 @@ type ExtraConfig struct {
 	// the CRD Establishing will be hold by 5 seconds.
 	MasterCount int
 
-	// ServiceResolver is used in CR webhook converters to resolve webhook's service names
-	ServiceResolver webhook.ServiceResolver
-	// AuthResolverWrapper is used in CR webhook converters
-	AuthResolverWrapper webhook.AuthenticationInfoResolverWrapper
+	// ConversionFactory is used to provider converters for CRs.
+	ConversionFactory conversion.Factory
 }
 
 type Config struct {
@@ -197,8 +195,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
 		c.ExtraConfig.CRDRESTOptionsGetter,
 		c.GenericConfig.AdmissionControl,
 		establishingController,
-		c.ExtraConfig.ServiceResolver,
-		c.ExtraConfig.AuthResolverWrapper,
+		c.ExtraConfig.ConversionFactory,
 		c.ExtraConfig.MasterCount,
 		s.GenericAPIServer.Authorizer,
 		c.GenericConfig.RequestTimeout,

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go

@@ -28,6 +28,18 @@ import (
 	typedscheme "k8s.io/client-go/kubernetes/scheme"
 )
 
+// Factory is able to create a new CRConverter for crd.
+type Factory interface {
+	// NewConverter returns a CRConverter capable of converting crd's versions.
+	//
+	// For proper conversion, the returned CRConverter must be used via NewDelegatingConverter.
+	//
+	// When implementing a CRConverter, you do not need to: test for valid API versions or no-op
+	// conversions, handle field selector logic, or handle scale conversions; these are all handled
+	// via NewDelegatingConverter.
+	NewConverter(crd *apiextensionsv1.CustomResourceDefinition) (CRConverter, error)
+}
+
 // CRConverterFactory is the factory for all CR converters.
 type CRConverterFactory struct {
 	// webhookConverterFactory is the factory for webhook converters.
@@ -39,7 +51,7 @@ type CRConverterFactory struct {
 // apiextensions-apiserver runs.
 var converterMetricFactorySingleton = newConverterMetricFactory()
 
-// NewCRConverterFactory creates a new CRConverterFactory
+// NewCRConverterFactory creates a new CRConverterFactory that supports none and webhook conversion strategies.
 func NewCRConverterFactory(serviceResolver webhook.ServiceResolver, authResolverWrapper webhook.AuthenticationInfoResolverWrapper) (*CRConverterFactory, error) {
 	converterFactory := &CRConverterFactory{}
 	webhookConverterFactory, err := newWebhookConverterFactory(serviceResolver, authResolverWrapper)
@@ -50,30 +62,32 @@ func NewCRConverterFactory(serviceResolver webhook.ServiceResolver, authResolver
 	return converterFactory, nil
 }
 
-// NewConverter returns a new CR converter based on the conversion settings in crd object.
-func (m *CRConverterFactory) NewConverter(crd *apiextensionsv1.CustomResourceDefinition) (safe, unsafe runtime.ObjectConvertor, err error) {
+// NewConverter creates a new CRConverter based on the crd's conversion strategy. Supported strategies are none and
+// webhook.
+func (f *CRConverterFactory) NewConverter(crd *apiextensionsv1.CustomResourceDefinition) (CRConverter, error) {
+	switch crd.Spec.Conversion.Strategy {
+	case apiextensionsv1.NoneConverter:
+		return NewNOPConverter(), nil
+	case apiextensionsv1.WebhookConverter:
+		converter, err := f.webhookConverterFactory.NewWebhookConverter(crd)
+		if err != nil {
+			return nil, err
+		}
+		return converterMetricFactorySingleton.addMetrics(crd.Name, converter)
+	}
+
+	return nil, fmt.Errorf("unknown conversion strategy %q for CRD %s", crd.Spec.Conversion.Strategy, crd.Name)
+}
+
+// NewDelegatingConverter returns new safe and unsafe converters based on the conversion settings in
+// crd. These converters contain logic common to all converters, and they delegate the actual
+// specific version-to-version conversion logic to the delegate.
+func NewDelegatingConverter(crd *apiextensionsv1.CustomResourceDefinition, delegate CRConverter) (safe, unsafe runtime.ObjectConvertor, err error) {
 	validVersions := map[schema.GroupVersion]bool{}
 	for _, version := range crd.Spec.Versions {
 		validVersions[schema.GroupVersion{Group: crd.Spec.Group, Version: version.Name}] = true
 	}
 
-	var converter CRConverter
-	switch crd.Spec.Conversion.Strategy {
-	case apiextensionsv1.NoneConverter:
-		converter = &nopConverter{}
-	case apiextensionsv1.WebhookConverter:
-		converter, err = m.webhookConverterFactory.NewWebhookConverter(crd)
-		if err != nil {
-			return nil, nil, err
-		}
-		converter, err = converterMetricFactorySingleton.addMetrics(crd.Name, converter)
-		if err != nil {
-			return nil, nil, err
-		}
-	default:
-		return nil, nil, fmt.Errorf("unknown conversion strategy %q for CRD %s", crd.Spec.Conversion.Strategy, crd.Name)
-	}
-
 	// Determine whether we should expect to be asked to "convert" autoscaling/v1 Scale types
 	convertScale := false
 	for _, version := range crd.Spec.Versions {
@@ -82,11 +96,11 @@ func (m *CRConverterFactory) NewConverter(crd *apiextensionsv1.CustomResourceDef
 		}
 	}
 
-	unsafe = &crConverter{
+	unsafe = &delegatingCRConverter{
 		convertScale:  convertScale,
 		validVersions: validVersions,
 		clusterScoped: crd.Spec.Scope == apiextensionsv1.ClusterScoped,
-		converter:     converter,
+		converter:     delegate,
 	}
 	return &safeConverterWrapper{unsafe}, unsafe, nil
 }
@@ -99,16 +113,16 @@ type CRConverter interface {
 	Convert(in runtime.Object, targetGVK schema.GroupVersion) (runtime.Object, error)
 }
 
-// crConverter extends the delegate converter with generic CR conversion behaviour. The delegate will implement the
+// delegatingCRConverter extends the delegate converter with generic CR conversion behaviour. The delegate will implement the
 // user defined conversion strategy given in the CustomResourceDefinition.
-type crConverter struct {
+type delegatingCRConverter struct {
 	convertScale  bool
 	converter     CRConverter
 	validVersions map[schema.GroupVersion]bool
 	clusterScoped bool
 }
 
-func (c *crConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, value string) (string, string, error) {
+func (c *delegatingCRConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, value string) (string, string, error) {
 	// We currently only support metadata.namespace and metadata.name.
 	switch {
 	case label == "metadata.name":
@@ -120,7 +134,7 @@ func (c *crConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, valu
 	}
 }
 
-func (c *crConverter) Convert(in, out, context interface{}) error {
+func (c *delegatingCRConverter) Convert(in, out, context interface{}) error {
 	// Special-case typed scale conversion if this custom resource supports a scale endpoint
 	if c.convertScale {
 		_, isInScale := in.(*autoscalingv1.Scale)
@@ -158,7 +172,7 @@ func (c *crConverter) Convert(in, out, context interface{}) error {
 // The in object can be a single object or a UnstructuredList. CRD storage implementation creates an
 // UnstructuredList with the request's GV, populates it from storage, then calls conversion to convert
 // the individual items. This function assumes it never gets a v1.List.
-func (c *crConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) {
+func (c *delegatingCRConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) {
 	fromGVK := in.GetObjectKind().GroupVersionKind()
 	toGVK, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{fromGVK})
 	if !ok {

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go

@@ -25,7 +25,6 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apiserver/pkg/util/webhook"
 )
 
 func TestConversion(t *testing.T) {
@@ -154,10 +153,6 @@ func TestConversion(t *testing.T) {
 		},
 	}
 
-	CRConverterFactory, err := NewCRConverterFactory(nil, func(resolver webhook.AuthenticationInfoResolver) webhook.AuthenticationInfoResolver { return nil })
-	if err != nil {
-		t.Fatalf("Cannot create conversion factory: %v", err)
-	}
 	for _, test := range tests {
 		testCRD := apiextensionsv1.CustomResourceDefinition{
 			Spec: apiextensionsv1.CustomResourceDefinitionSpec{
@@ -171,7 +166,7 @@ func TestConversion(t *testing.T) {
 			testCRD.Spec.Versions = append(testCRD.Spec.Versions, apiextensionsv1.CustomResourceDefinitionVersion{Name: gv.Version, Served: true})
 			testCRD.Spec.Group = gv.Group
 		}
-		safeConverter, _, err := CRConverterFactory.NewConverter(&testCRD)
+		safeConverter, _, err := NewDelegatingConverter(&testCRD, NewNOPConverter())
 		if err != nil {
 			t.Fatalf("Cannot create converter: %v", err)
 		}

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go

@@ -26,6 +26,12 @@ import (
 type nopConverter struct {
 }
 
+// NewNOPConverter creates a new no-op converter. The only "conversion" it performs is to set the group and version to
+// targetGV.
+func NewNOPConverter() *nopConverter {
+	return &nopConverter{}
+}
+
 var _ CRConverter = &nopConverter{}
 
 // ConvertToVersion converts in object to the given gv in place and returns the same `in` object.

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -72,7 +72,6 @@ import (
 	"k8s.io/apiserver/pkg/registry/generic"
 	genericfilters "k8s.io/apiserver/pkg/server/filters"
 	utilopenapi "k8s.io/apiserver/pkg/util/openapi"
-	"k8s.io/apiserver/pkg/util/webhook"
 	"k8s.io/apiserver/pkg/warning"
 	"k8s.io/client-go/scale"
 	"k8s.io/client-go/scale/scheme/autoscalingv1"
@@ -109,7 +108,7 @@ type crdHandler struct {
 	// CRD establishing process for HA clusters.
 	masterCount int
 
-	converterFactory *conversion.CRConverterFactory
+	converterFactory conversion.Factory
 
 	// so that we can do create on update.
 	authorizer authorizer.Authorizer
@@ -172,14 +171,18 @@ func NewCustomResourceDefinitionHandler(
 	restOptionsGetter generic.RESTOptionsGetter,
 	admission admission.Interface,
 	establishingController *establish.EstablishingController,
-	serviceResolver webhook.ServiceResolver,
-	authResolverWrapper webhook.AuthenticationInfoResolverWrapper,
+	converterFactory conversion.Factory,
 	masterCount int,
 	authorizer authorizer.Authorizer,
 	requestTimeout time.Duration,
 	minRequestTimeout time.Duration,
 	staticOpenAPISpec *spec.Swagger,
 	maxRequestBodyBytes int64) (*crdHandler, error) {
+
+	if converterFactory == nil {
+		return nil, fmt.Errorf("converterFactory is required")
+	}
+
 	ret := &crdHandler{
 		versionDiscoveryHandler: versionDiscoveryHandler,
 		groupDiscoveryHandler:   groupDiscoveryHandler,
@@ -189,6 +192,7 @@ func NewCustomResourceDefinitionHandler(
 		restOptionsGetter:       restOptionsGetter,
 		admission:               admission,
 		establishingController:  establishingController,
+		converterFactory:        converterFactory,
 		masterCount:             masterCount,
 		authorizer:              authorizer,
 		requestTimeout:          requestTimeout,
@@ -203,11 +207,6 @@ func NewCustomResourceDefinitionHandler(
 			ret.removeDeadStorage()
 		},
 	})
-	crConverterFactory, err := conversion.NewCRConverterFactory(serviceResolver, authResolverWrapper)
-	if err != nil {
-		return nil, err
-	}
-	ret.converterFactory = crConverterFactory
 
 	ret.customStorage.Store(crdStorageMap{})
 
@@ -690,7 +689,12 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
 		}
 	}
 
-	safeConverter, unsafeConverter, err := r.converterFactory.NewConverter(crd)
+	converter, err := r.converterFactory.NewConverter(crd)
+	if err != nil {
+		return nil, fmt.Errorf("error creating converter for %s: %w", crd.Name, err)
+	}
+
+	safeConverter, unsafeConverter, err := conversion.NewDelegatingConverter(crd, converter)
 	if err != nil {
 		return nil, err
 	}

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go

@@ -23,13 +23,10 @@ import (
 	"errors"
 	"io"
 	"io/ioutil"
-	"net"
 	"net/http"
 	"net/http/httptest"
-	"net/url"
 	"os"
 	"path/filepath"
-	"strconv"
 	"testing"
 	"time"
 
@@ -60,7 +57,6 @@ import (
 	"k8s.io/apiserver/pkg/registry/rest"
 	"k8s.io/apiserver/pkg/server/options"
 	etcd3testing "k8s.io/apiserver/pkg/storage/etcd3/testing"
-	"k8s.io/apiserver/pkg/util/webhook"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/kube-openapi/pkg/validation/spec"
 )
@@ -120,11 +116,7 @@ func TestConvertFieldLabel(t *testing.T) {
 			} else {
 				crd.Spec.Scope = apiextensionsv1.NamespaceScoped
 			}
-			f, err := conversion.NewCRConverterFactory(nil, nil)
-			if err != nil {
-				t.Fatal(err)
-			}
-			_, c, err := f.NewConverter(&crd)
+			_, c, err := conversion.NewDelegatingConverter(&crd, conversion.NewNOPConverter())
 			if err != nil {
 				t.Fatalf("Failed to create CR converter. error: %v", err)
 			}
@@ -466,6 +458,12 @@ func TestHandlerConversionWithoutWatchCache(t *testing.T) {
 	testHandlerConversion(t, false)
 }
 
+type noneConverterFactory struct{}
+
+func (f *noneConverterFactory) NewConverter(_ *apiextensionsv1.CustomResourceDefinition) (conversion.CRConverter, error) {
+	return conversion.NewNOPConverter(), nil
+}
+
 func testHandlerConversion(t *testing.T, enableWatchCache bool) {
 	cl := fake.NewSimpleClientset()
 	informers := informers.NewSharedInformerFactory(fake.NewSimpleClientset(), 0)
@@ -506,8 +504,7 @@ func testHandlerConversion(t *testing.T, enableWatchCache bool) {
 		restOptionsGetter,
 		dummyAdmissionImpl{},
 		&establish.EstablishingController{},
-		dummyServiceResolverImpl{},
-		func(r webhook.AuthenticationInfoResolver) webhook.AuthenticationInfoResolver { return r },
+		&noneConverterFactory{},
 		1,
 		dummyAuthorizerImpl{},
 		time.Minute, time.Minute, nil, 3*1024*1024)
@@ -847,12 +844,6 @@ func (dummyAuthorizerImpl) Authorize(ctx context.Context, a authorizer.Attribute
 	return authorizer.DecisionAllow, "", nil
 }
 
-type dummyServiceResolverImpl struct{}
-
-func (dummyServiceResolverImpl) ResolveEndpoint(namespace, name string, port int32) (*url.URL, error) {
-	return &url.URL{Scheme: "https", Host: net.JoinHostPort(name+"."+namespace+".svc", strconv.Itoa(int(port)))}, nil
-}
-
 var multiVersionFixture = &apiextensionsv1.CustomResourceDefinition{
 	ObjectMeta: metav1.ObjectMeta{Name: "multiversion.stable.example.com", UID: types.UID("12345")},
 	Spec: apiextensionsv1.CustomResourceDefinitionSpec{

staging/src/k8s.io/apiextensions-apiserver/pkg/cmd/server/options/options.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/spf13/pflag"
 	oteltrace "go.opentelemetry.io/otel/trace"
+	"k8s.io/apiextensions-apiserver/pkg/apiserver/conversion"
 
 	v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
@@ -107,6 +108,14 @@ func (o CustomResourceDefinitionsServerOptions) Config() (*apiserver.Config, err
 	if err := o.APIEnablement.ApplyTo(&serverConfig.Config, apiserver.DefaultAPIResourceConfigSource(), apiserver.Scheme); err != nil {
 		return nil, err
 	}
+
+	serviceResolver := &serviceResolver{serverConfig.SharedInformerFactory.Core().V1().Services().Lister()}
+	authResolverWrapper := webhook.NewDefaultAuthenticationInfoResolverWrapper(nil, nil, serverConfig.LoopbackClientConfig, oteltrace.NewNoopTracerProvider())
+	conversionFactory, err := conversion.NewCRConverterFactory(serviceResolver, authResolverWrapper)
+	if err != nil {
+		return nil, err
+	}
+
 	crdRESTOptionsGetter, err := NewCRDRESTOptionsGetter(*o.RecommendedOptions.Etcd)
 	if err != nil {
 		return nil, err
@@ -115,8 +124,7 @@ func (o CustomResourceDefinitionsServerOptions) Config() (*apiserver.Config, err
 		GenericConfig: serverConfig,
 		ExtraConfig: apiserver.ExtraConfig{
 			CRDRESTOptionsGetter: crdRESTOptionsGetter,
-			ServiceResolver:      &serviceResolver{serverConfig.SharedInformerFactory.Core().V1().Services().Lister()},
-			AuthResolverWrapper:  webhook.NewDefaultAuthenticationInfoResolverWrapper(nil, nil, serverConfig.LoopbackClientConfig, oteltrace.NewNoopTracerProvider()),
+			ConversionFactory:    conversionFactory,
 		},
 	}
 	return config, nil