From 0bdcb93b06d57c556d35c53444a4ab7d19f80e85 Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Tue, 18 Oct 2022 16:31:24 -0400 Subject: [PATCH] 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 --- cmd/kube-apiserver/app/apiextensions.go | 11 ++- .../pkg/apiserver/apiserver.go | 11 ++- .../pkg/apiserver/conversion/converter.go | 68 +++++++++++-------- .../apiserver/conversion/converter_test.go | 7 +- .../pkg/apiserver/conversion/nop_converter.go | 6 ++ .../pkg/apiserver/customresource_handler.go | 24 ++++--- .../apiserver/customresource_handler_test.go | 25 +++---- .../pkg/cmd/server/options/options.go | 12 +++- 8 files changed, 93 insertions(+), 71 deletions(-) diff --git a/cmd/kube-apiserver/app/apiextensions.go b/cmd/kube-apiserver/app/apiextensions.go index ee80cd9a7c9..59bc0079f30 100644 --- a/cmd/kube-apiserver/app/apiextensions.go +++ b/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, }, } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go index dd2d7a51103..8e999b9bf4c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/apiserver.go +++ b/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, diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go index 0ae9093a39b..aafe44f87c3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go +++ b/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 { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go index 3866dbd36bf..65299c11493 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go +++ b/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) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go index ed7c3e8ac95..9b2a04e3606 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go +++ b/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. diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index dd654ef1a0c..86bfc46641c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/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 } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go index d4be6a9331f..46470bd2f87 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go +++ b/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{ diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/cmd/server/options/options.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/cmd/server/options/options.go index 158b6e13844..4f7331a6828 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/cmd/server/options/options.go +++ b/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