Refactor CR converter code to share code between nopConverter and webhookConverter

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

@@ -55,35 +55,42 @@ func (m *CRConverterFactory) NewConverter(crd *apiextensions.CustomResourceDefin
 		validVersions[schema.GroupVersion{Group: crd.Spec.Group, Version: version.Name}] = true
 	}
 
+	var converter crConverterInterface
 	switch crd.Spec.Conversion.Strategy {
 	case apiextensions.NoneConverter:
-		unsafe = &crConverter{
-			clusterScoped: crd.Spec.Scope == apiextensions.ClusterScoped,
-			delegate: &nopConverter{
-				validVersions: validVersions,
-			},
-		}
-		return &safeConverterWrapper{unsafe}, unsafe, nil
+		converter = &nopConverter{}
 	case apiextensions.WebhookConverter:
 		if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) {
 			return nil, nil, fmt.Errorf("webhook conversion is disabled on this cluster")
 		}
-		unsafe, err := m.webhookConverterFactory.NewWebhookConverter(validVersions, crd)
+		converter, err = m.webhookConverterFactory.NewWebhookConverter(crd)
 		if err != nil {
 			return nil, nil, err
 		}
-		return &safeConverterWrapper{unsafe}, unsafe, nil
+	default:
+		return nil, nil, fmt.Errorf("unknown conversion strategy %q for CRD %s", crd.Spec.Conversion.Strategy, crd.Name)
 	}
-
-	return nil, nil, fmt.Errorf("unknown conversion strategy %q for CRD %s", crd.Spec.Conversion.Strategy, crd.Name)
+	unsafe = &crConverter{
+		validVersions: validVersions,
+		clusterScoped: crd.Spec.Scope == apiextensions.ClusterScoped,
+		converter:     converter,
+	}
+	return &safeConverterWrapper{unsafe}, unsafe, nil
 }
 
-var _ runtime.ObjectConvertor = &crConverter{}
+// crConverterInterface is the interface all cr converters must implement
+type crConverterInterface interface {
+	// Convert converts in object to the given gvk and returns the converted object.
+	// Note that the function may mutate in object and return it. A safe wrapper will make sure
+	// a safe converter will be returned.
+	Convert(in runtime.Object, targetGVK schema.GroupVersion) (runtime.Object, error)
+}
 
-// crConverter extends the delegate with generic CR conversion behaviour. The delegate will implement the
+// crConverter 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 {
-	delegate      runtime.ObjectConvertor
+	converter     crConverterInterface
+	validVersions map[schema.GroupVersion]bool
 	clusterScoped bool
 }
 
@@ -100,29 +107,60 @@ func (c *crConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, valu
 }
 
 func (c *crConverter) Convert(in, out, context interface{}) error {
-	return c.delegate.Convert(in, out, context)
+	unstructIn, ok := in.(*unstructured.Unstructured)
+	if !ok {
+		return fmt.Errorf("input type %T in not valid for unstructured conversion", in)
+	}
+
+	unstructOut, ok := out.(*unstructured.Unstructured)
+	if !ok {
+		return fmt.Errorf("output type %T in not valid for unstructured conversion", out)
+	}
+
+	outGVK := unstructOut.GroupVersionKind()
+	converted, err := c.ConvertToVersion(unstructIn, outGVK.GroupVersion())
+	if err != nil {
+		return err
+	}
+	unstructuredConverted, ok := converted.(runtime.Unstructured)
+	if !ok {
+		// this should not happened
+		return fmt.Errorf("CR conversion failed")
+	}
+	unstructOut.SetUnstructuredContent(unstructuredConverted.UnstructuredContent())
+	return nil
 }
 
 // ConvertToVersion converts in object to the given gvk in place and returns the same `in` object.
+// 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) {
-	// Run the converter on the list items instead of list itself
+	fromGVK := in.GetObjectKind().GroupVersionKind()
+	toGVK, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{fromGVK})
+	if !ok {
+		// TODO: should this be a typed error?
+		return nil, fmt.Errorf("%v is unstructured and is not suitable for converting to %q", fromGVK.String(), target)
+	}
+	if !c.validVersions[toGVK.GroupVersion()] {
+		return nil, fmt.Errorf("request to convert CR to an invalid group/version: %s", toGVK.GroupVersion().String())
+	}
+	// Note that even if the request is for a list, the GV of the request UnstructuredList is what
+	// is expected to convert to. As mentioned in the function's document, it is not expected to
+	// get a v1.List.
+	if !c.validVersions[fromGVK.GroupVersion()] {
+		return nil, fmt.Errorf("request to convert CR from an invalid group/version: %s", fromGVK.GroupVersion().String())
+	}
+	// Check list item's apiVersion
 	if list, ok := in.(*unstructured.UnstructuredList); ok {
 		for i := range list.Items {
-			obj, err := c.delegate.ConvertToVersion(&list.Items[i], target)
-			if err != nil {
-				return nil, err
+			expectedGV := list.Items[i].GroupVersionKind().GroupVersion()
+			if !c.validVersions[expectedGV] {
+				return nil, fmt.Errorf("request to convert CR list failed, list index %d has invalid group/version: %s", i, expectedGV.String())
 			}
-
-			u, ok := obj.(*unstructured.Unstructured)
-			if !ok {
-				return nil, fmt.Errorf("output type %T in not valid for unstructured conversion", obj)
-			}
-			list.Items[i] = *u
 		}
-		return list, nil
 	}
-
-	return c.delegate.ConvertToVersion(in, target)
+	return c.converter.Convert(in, toGVK.GroupVersion())
 }
 
 // safeConverterWrapper is a wrapper over an unsafe object converter that makes copy of the input and then delegate to the unsafe converter.
@@ -130,7 +168,7 @@ type safeConverterWrapper struct {
 	unsafe runtime.ObjectConvertor
 }
 
-var _ runtime.ObjectConvertor = &nopConverter{}
+var _ runtime.ObjectConvertor = &safeConverterWrapper{}
 
 // ConvertFieldLabel delegate the call to the unsafe converter.
 func (c *safeConverterWrapper) ConvertFieldLabel(gvk schema.GroupVersionKind, label, value string) (string, string, error) {

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

+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package conversion
+
+import (
+	"reflect"
+	"strings"
+	"testing"
+
+	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
+	"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) {
+	tests := []struct {
+		Name            string
+		ValidVersions   []string
+		ClusterScoped   bool
+		ToVersion       string
+		SourceObject    runtime.Object
+		ExpectedObject  runtime.Object
+		ExpectedFailure string
+	}{
+		{
+			Name:          "simple_conversion",
+			ValidVersions: []string{"example.com/v1", "example.com/v2"},
+			ClusterScoped: false,
+			ToVersion:     "example.com/v2",
+			SourceObject: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "example.com/v1",
+					"other":      "data",
+					"kind":       "foo",
+				},
+			},
+			ExpectedObject: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "example.com/v2",
+					"other":      "data",
+					"kind":       "foo",
+				},
+			},
+			ExpectedFailure: "",
+		},
+		{
+			Name:          "failed_conversion_invalid_gv",
+			ValidVersions: []string{"example.com/v1", "example.com/v2"},
+			ClusterScoped: false,
+			ToVersion:     "example.com/v3",
+			SourceObject: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "example.com/v1",
+					"other":      "data",
+				},
+			},
+			ExpectedFailure: "invalid group/version: example.com/v3",
+		},
+		{
+			Name:          "simple_list_conversion",
+			ValidVersions: []string{"example.com/v1", "example.com/v2"},
+			ClusterScoped: false,
+			ToVersion:     "example.com/v2",
+			SourceObject: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"apiVersion": "example.com/v1",
+					"kind":       "fooList",
+				},
+				Items: []unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"apiVersion": "example.com/v1",
+							"kind":       "foo",
+							"other":      "data",
+						},
+					},
+					{
+						Object: map[string]interface{}{
+							"apiVersion": "example.com/v1",
+							"kind":       "foo",
+							"other":      "data2",
+						},
+					},
+				},
+			},
+			ExpectedObject: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"apiVersion": "example.com/v2",
+					"kind":       "fooList",
+				},
+				Items: []unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"apiVersion": "example.com/v2",
+							"kind":       "foo",
+							"other":      "data",
+						},
+					},
+					{
+						Object: map[string]interface{}{
+							"apiVersion": "example.com/v2",
+							"kind":       "foo",
+							"other":      "data2",
+						},
+					},
+				},
+			},
+			ExpectedFailure: "",
+		},
+		{
+			Name:          "list_with_invalid_gv",
+			ValidVersions: []string{"example.com/v1", "example.com/v2"},
+			ClusterScoped: false,
+			ToVersion:     "example.com/v2",
+			SourceObject: &unstructured.UnstructuredList{
+				Object: map[string]interface{}{
+					"apiVersion": "example.com/v1",
+					"kind":       "fooList",
+				},
+				Items: []unstructured.Unstructured{
+					{
+						Object: map[string]interface{}{
+							"apiVersion": "example.com/v1",
+							"kind":       "foo",
+							"other":      "data",
+						},
+					},
+					{
+						Object: map[string]interface{}{
+							"apiVersion": "example.com/v3",
+							"kind":       "foo",
+							"other":      "data2",
+						},
+					},
+				},
+			},
+			ExpectedFailure: "invalid group/version: example.com/v3",
+		},
+	}
+
+	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 := apiextensions.CustomResourceDefinition{
+			Spec: apiextensions.CustomResourceDefinitionSpec{
+				Conversion: &apiextensions.CustomResourceConversion{
+					Strategy: apiextensions.NoneConverter,
+				},
+			},
+		}
+		for _, v := range test.ValidVersions {
+			gv, _ := schema.ParseGroupVersion(v)
+			testCRD.Spec.Versions = append(testCRD.Spec.Versions, apiextensions.CustomResourceDefinitionVersion{Name: gv.Version, Served: true})
+			testCRD.Spec.Group = gv.Group
+		}
+		safeConverter, _, err := CRConverterFactory.NewConverter(&testCRD)
+		if err != nil {
+			t.Fatalf("Cannot create converter: %v", err)
+		}
+		o := test.SourceObject.DeepCopyObject()
+		toVersion, _ := schema.ParseGroupVersion(test.ToVersion)
+		toVersions := schema.GroupVersions{toVersion}
+		actual, err := safeConverter.ConvertToVersion(o, toVersions)
+		if test.ExpectedFailure != "" {
+			if err == nil || !strings.Contains(err.Error(), test.ExpectedFailure) {
+				t.Fatalf("%s: Expected the call to fail with error message `%s` but err=%v", test.Name, test.ExpectedFailure, err)
+			}
+		} else {
+			if err != nil {
+				t.Fatalf("%s: conversion failed with : %v", test.Name, err)
+			}
+			if !reflect.DeepEqual(test.ExpectedObject, actual) {
+				t.Fatalf("%s: Expected = %v, Actual = %v", test.Name, test.ExpectedObject, actual)
+			}
+		}
+	}
+}

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

@@ -17,9 +17,6 @@ limitations under the License.
 package conversion
 
 import (
-	"errors"
-	"fmt"
-
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -27,53 +24,18 @@ import (
 
 // nopConverter is a converter that only sets the apiVersion fields, but does not real conversion.
 type nopConverter struct {
-	validVersions map[schema.GroupVersion]bool
-}
-
-var _ runtime.ObjectConvertor = &nopConverter{}
-
-func (nopConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, value string) (string, string, error) {
-	return "", "", errors.New("unstructured cannot convert field labels")
 }
 
-func (c *nopConverter) Convert(in, out, context interface{}) error {
-	unstructIn, ok := in.(*unstructured.Unstructured)
-	if !ok {
-		return fmt.Errorf("input type %T in not valid for unstructured conversion", in)
-	}
-
-	unstructOut, ok := out.(*unstructured.Unstructured)
-	if !ok {
-		return fmt.Errorf("output type %T in not valid for unstructured conversion", out)
-	}
-
-	outGVK := unstructOut.GroupVersionKind()
-	if !c.validVersions[outGVK.GroupVersion()] {
-		return fmt.Errorf("request to convert CR from an invalid group/version: %s", outGVK.String())
-	}
-	inGVK := unstructIn.GroupVersionKind()
-	if !c.validVersions[inGVK.GroupVersion()] {
-		return fmt.Errorf("request to convert CR to an invalid group/version: %s", inGVK.String())
-	}
+var _ crConverterInterface = &nopConverter{}
 
-	unstructOut.SetUnstructuredContent(unstructIn.UnstructuredContent())
-	_, err := c.ConvertToVersion(unstructOut, outGVK.GroupVersion())
-	if err != nil {
-		return err
-	}
-	return nil
-}
-
-func (c *nopConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) {
-	kind := in.GetObjectKind().GroupVersionKind()
-	gvk, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{kind})
-	if !ok {
-		// TODO: should this be a typed error?
-		return nil, fmt.Errorf("%v is unstructured and is not suitable for converting to %q", kind, target)
-	}
-	if !c.validVersions[gvk.GroupVersion()] {
-		return nil, fmt.Errorf("request to convert CR to an invalid group/version: %s", gvk.String())
+// ConvertToVersion converts in object to the given gv in place and returns the same `in` object.
+func (c *nopConverter) Convert(in runtime.Object, targetGV schema.GroupVersion) (runtime.Object, error) {
+	// Run the converter on the list items instead of list itself
+	if list, ok := in.(*unstructured.UnstructuredList); ok {
+		for i := range list.Items {
+			list.Items[i].SetGroupVersionKind(targetGV.WithKind(list.Items[i].GroupVersionKind().Kind))
+		}
 	}
-	in.GetObjectKind().SetGroupVersionKind(gvk)
+	in.GetObjectKind().SetGroupVersionKind(targetGV.WithKind(in.GetObjectKind().GroupVersionKind().Kind))
 	return in, nil
 }

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

@@ -18,7 +18,6 @@ package conversion
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -55,7 +54,6 @@ func newWebhookConverterFactory(serviceResolver webhook.ServiceResolver, authRes
 
 // webhookConverter is a converter that calls an external webhook to do the CR conversion.
 type webhookConverter struct {
-	validVersions map[schema.GroupVersion]bool
 	clientManager webhook.ClientManager
 	restClient    *rest.RESTClient
 	name          string
@@ -85,61 +83,23 @@ func webhookClientConfigForCRD(crd *internal.CustomResourceDefinition) *webhook.
 	return &ret
 }
 
-var _ runtime.ObjectConvertor = &webhookConverter{}
+var _ crConverterInterface = &webhookConverter{}
 
-func (f *webhookConverterFactory) NewWebhookConverter(validVersions map[schema.GroupVersion]bool, crd *internal.CustomResourceDefinition) (*webhookConverter, error) {
+func (f *webhookConverterFactory) NewWebhookConverter(crd *internal.CustomResourceDefinition) (*webhookConverter, error) {
 	restClient, err := f.clientManager.HookClient(*webhookClientConfigForCRD(crd))
 	if err != nil {
 		return nil, err
 	}
 	return &webhookConverter{
 		clientManager: f.clientManager,
-		validVersions: validVersions,
 		restClient:    restClient,
 		name:          crd.Name,
-		nopConverter:  nopConverter{validVersions: validVersions},
+		nopConverter:  nopConverter{},
 
 		conversionReviewVersions: crd.Spec.Conversion.ConversionReviewVersions,
 	}, nil
 }
 
-func (webhookConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, value string) (string, string, error) {
-	return "", "", errors.New("unstructured cannot convert field labels")
-}
-
-func (c *webhookConverter) Convert(in, out, context interface{}) error {
-	unstructIn, ok := in.(*unstructured.Unstructured)
-	if !ok {
-		return fmt.Errorf("input type %T in not valid for unstructured conversion", in)
-	}
-
-	unstructOut, ok := out.(*unstructured.Unstructured)
-	if !ok {
-		return fmt.Errorf("output type %T in not valid for unstructured conversion", out)
-	}
-
-	outGVK := unstructOut.GroupVersionKind()
-	if !c.validVersions[outGVK.GroupVersion()] {
-		return fmt.Errorf("request to convert CR from an invalid group/version: %s", outGVK.String())
-	}
-	inGVK := unstructIn.GroupVersionKind()
-	if !c.validVersions[inGVK.GroupVersion()] {
-		return fmt.Errorf("request to convert CR to an invalid group/version: %s", inGVK.String())
-	}
-
-	converted, err := c.ConvertToVersion(unstructIn, outGVK.GroupVersion())
-	if err != nil {
-		return err
-	}
-	unstructuredConverted, ok := converted.(runtime.Unstructured)
-	if !ok {
-		// this should not happened
-		return fmt.Errorf("CR conversion failed")
-	}
-	unstructOut.SetUnstructuredContent(unstructuredConverted.UnstructuredContent())
-	return nil
-}
-
 // hasConversionReviewVersion check whether a version is accepted by a given webhook.
 func (c *webhookConverter) hasConversionReviewVersion(v string) bool {
 	for _, b := range c.conversionReviewVersions {
@@ -187,48 +147,17 @@ func getRawExtensionObject(rx runtime.RawExtension) (runtime.Object, error) {
 	return &u, nil
 }
 
-// getTargetGroupVersion returns group/version which should be used to convert in objects to.
-// String version of the return item is APIVersion.
-func getTargetGroupVersion(in runtime.Object, target runtime.GroupVersioner) (schema.GroupVersion, error) {
-	fromGVK := in.GetObjectKind().GroupVersionKind()
-	toGVK, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{fromGVK})
-	if !ok {
-		// TODO: should this be a typed error?
-		return schema.GroupVersion{}, fmt.Errorf("%v is unstructured and is not suitable for converting to %q", fromGVK.String(), target)
-	}
-	return toGVK.GroupVersion(), nil
-}
-
-func (c *webhookConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) {
+func (c *webhookConverter) Convert(in runtime.Object, toGV schema.GroupVersion) (runtime.Object, error) {
 	// In general, the webhook should not do any defaulting or validation. A special case of that is an empty object
 	// conversion that must result an empty object and practically is the same as nopConverter.
 	// A smoke test in API machinery calls the converter on empty objects. As this case happens consistently
 	// it special cased here not to call webhook converter. The test initiated here:
 	// https://github.com/kubernetes/kubernetes/blob/dbb448bbdcb9e440eee57024ffa5f1698956a054/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go#L201
 	if isEmptyUnstructuredObject(in) {
-		return c.nopConverter.ConvertToVersion(in, target)
+		return c.nopConverter.Convert(in, toGV)
 	}
 
-	toGV, err := getTargetGroupVersion(in, target)
-	if err != nil {
-		return nil, err
-	}
-	if !c.validVersions[toGV] {
-		return nil, fmt.Errorf("request to convert CR to an invalid group/version: %s", toGV.String())
-	}
-	fromGV := in.GetObjectKind().GroupVersionKind().GroupVersion()
-	if !c.validVersions[fromGV] {
-		return nil, fmt.Errorf("request to convert CR from an invalid group/version: %s", fromGV.String())
-	}
 	listObj, isList := in.(*unstructured.UnstructuredList)
-	if isList {
-		for i, item := range listObj.Items {
-			fromGV := item.GroupVersionKind().GroupVersion()
-			if !c.validVersions[fromGV] {
-				return nil, fmt.Errorf("input list has invalid group/version `%v` at `%v` index", fromGV, i)
-			}
-		}
-	}
 
 	// Currently converter only supports `v1beta1` ConversionReview
 	// TODO: Make CRD webhooks caller capable of sending/receiving multiple ConversionReview versions