Revert "Merge pull request #113151 from ncdc/refactor-crd-conversion"

This reverts commit f524d765f464f5eec165b7bcd5b95aeb4d019676, reversing
changes made to c2b5457dfab44cb9ebf136be1d19e2f3dee05bc2.

Kubernetes-commit: a1f97a35fcb3835b6731455d34fcd0ae766bdb13

pkg/apiserver/apiserver.go

@@ -25,7 +25,6 @@ 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"
@@ -50,6 +49,7 @@ 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 (
@@ -84,8 +84,10 @@ type ExtraConfig struct {
 	// the CRD Establishing will be hold by 5 seconds.
 	MasterCount int
 
-	// ConversionFactory is used to provider converters for CRs.
-	ConversionFactory conversion.Factory
+	// 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
 }
 
 type Config struct {
@@ -196,7 +198,8 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
 		c.ExtraConfig.CRDRESTOptionsGetter,
 		c.GenericConfig.AdmissionControl,
 		establishingController,
-		c.ExtraConfig.ConversionFactory,
+		c.ExtraConfig.ServiceResolver,
+		c.ExtraConfig.AuthResolverWrapper,
 		c.ExtraConfig.MasterCount,
 		s.GenericAPIServer.Authorizer,
 		c.GenericConfig.RequestTimeout,

pkg/apiserver/conversion/converter_test.go

@@ -21,11 +21,11 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/google/go-cmp/cmp"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"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,6 +154,10 @@ 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{
@@ -167,7 +171,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 := NewDelegatingConverter(&testCRD, NewNOPConverter())
+		safeConverter, _, err := CRConverterFactory.NewConverter(&testCRD)
 		if err != nil {
 			t.Fatalf("Cannot create converter: %v", err)
 		}
@@ -189,102 +193,3 @@ func TestConversion(t *testing.T) {
 		}
 	}
 }
-
-func TestGetObjectsToConvert(t *testing.T) {
-	v1Object := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v1", "kind": "Widget", "metadata": map[string]interface{}{"name": "myv1"}}}
-	v2Object := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v2", "kind": "Widget", "metadata": map[string]interface{}{"name": "myv2"}}}
-	v3Object := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v3", "kind": "Widget", "metadata": map[string]interface{}{"name": "myv3"}}}
-
-	testcases := []struct {
-		Name          string
-		List          *unstructured.UnstructuredList
-		APIVersion    string
-		ValidVersions map[schema.GroupVersion]bool
-
-		ExpectObjects []*unstructured.Unstructured
-		ExpectError   bool
-	}{
-		{
-			Name:       "empty list",
-			List:       &unstructured.UnstructuredList{},
-			APIVersion: "foo/v1",
-			ValidVersions: map[schema.GroupVersion]bool{
-				{Group: "foo", Version: "v1"}: true,
-			},
-			ExpectObjects: nil,
-		},
-		{
-			Name: "one-item list, in desired version",
-			List: &unstructured.UnstructuredList{
-				Items: []unstructured.Unstructured{*v1Object},
-			},
-			ValidVersions: map[schema.GroupVersion]bool{
-				{Group: "foo", Version: "v1"}: true,
-			},
-			APIVersion:    "foo/v1",
-			ExpectObjects: nil,
-		},
-		{
-			Name: "one-item list, not in desired version",
-			List: &unstructured.UnstructuredList{
-				Items: []unstructured.Unstructured{*v2Object},
-			},
-			ValidVersions: map[schema.GroupVersion]bool{
-				{Group: "foo", Version: "v1"}: true,
-				{Group: "foo", Version: "v2"}: true,
-			},
-			APIVersion:    "foo/v1",
-			ExpectObjects: []*unstructured.Unstructured{v2Object},
-		},
-		{
-			Name: "multi-item list, in desired version",
-			List: &unstructured.UnstructuredList{
-				Items: []unstructured.Unstructured{*v1Object, *v1Object, *v1Object},
-			},
-			ValidVersions: map[schema.GroupVersion]bool{
-				{Group: "foo", Version: "v1"}: true,
-				{Group: "foo", Version: "v2"}: true,
-			},
-			APIVersion:    "foo/v1",
-			ExpectObjects: nil,
-		},
-		{
-			Name: "multi-item list, mixed versions",
-			List: &unstructured.UnstructuredList{
-				Items: []unstructured.Unstructured{*v1Object, *v2Object, *v3Object},
-			},
-			ValidVersions: map[schema.GroupVersion]bool{
-				{Group: "foo", Version: "v1"}: true,
-				{Group: "foo", Version: "v2"}: true,
-				{Group: "foo", Version: "v3"}: true,
-			},
-			APIVersion:    "foo/v1",
-			ExpectObjects: []*unstructured.Unstructured{v2Object, v3Object},
-		},
-		{
-			Name: "multi-item list, invalid versions",
-			List: &unstructured.UnstructuredList{
-				Items: []unstructured.Unstructured{*v1Object, *v2Object, *v3Object},
-			},
-			ValidVersions: map[schema.GroupVersion]bool{
-				{Group: "foo", Version: "v2"}: true,
-				{Group: "foo", Version: "v3"}: true,
-			},
-			APIVersion:    "foo/v1",
-			ExpectObjects: nil,
-			ExpectError:   true,
-		},
-	}
-	for _, tc := range testcases {
-		t.Run(tc.Name, func(t *testing.T) {
-			objects, err := getObjectsToConvert(tc.List, tc.APIVersion, tc.ValidVersions)
-			gotError := err != nil
-			if e, a := tc.ExpectError, gotError; e != a {
-				t.Fatalf("error: expected %t, got %t", e, a)
-			}
-			if !reflect.DeepEqual(objects, tc.ExpectObjects) {
-				t.Errorf("unexpected diff: %s", cmp.Diff(tc.ExpectObjects, objects))
-			}
-		})
-	}
-}

pkg/apiserver/conversion/metrics.go

@@ -21,7 +21,7 @@ import (
 	"sync"
 	"time"
 
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/component-base/metrics"
 	"k8s.io/component-base/metrics/legacyregistry"
@@ -43,15 +43,15 @@ func newConverterMetricFactory() *converterMetricFactory {
 	return &converterMetricFactory{durations: map[string]*metrics.HistogramVec{}, factoryLock: sync.Mutex{}}
 }
 
-var _ CRConverter = &converterMetric{}
+var _ crConverterInterface = &converterMetric{}
 
 type converterMetric struct {
-	delegate  CRConverter
+	delegate  crConverterInterface
 	latencies *metrics.HistogramVec
 	crdName   string
 }
 
-func (c *converterMetricFactory) addMetrics(crdName string, converter CRConverter) (CRConverter, error) {
+func (c *converterMetricFactory) addMetrics(crdName string, converter crConverterInterface) (crConverterInterface, error) {
 	c.factoryLock.Lock()
 	defer c.factoryLock.Unlock()
 	metric, exists := c.durations["webhook"]
@@ -73,7 +73,7 @@ func (c *converterMetricFactory) addMetrics(crdName string, converter CRConverte
 	return &converterMetric{latencies: metric, delegate: converter, crdName: crdName}, nil
 }
 
-func (m *converterMetric) Convert(in *unstructured.UnstructuredList, targetGV schema.GroupVersion) (*unstructured.UnstructuredList, error) {
+func (m *converterMetric) Convert(in runtime.Object, targetGV schema.GroupVersion) (runtime.Object, error) {
 	start := time.Now()
 	obj, err := m.delegate.Convert(in, targetGV)
 	fromVersion := in.GetObjectKind().GroupVersionKind().Version

pkg/apiserver/conversion/nop_converter.go

@@ -18,25 +18,24 @@ package conversion
 
 import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 // nopConverter is a converter that only sets the apiVersion fields, but does not real conversion.
 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{}
+var _ crConverterInterface = &nopConverter{}
 
 // ConvertToVersion converts in object to the given gv in place and returns the same `in` object.
-func (c *nopConverter) Convert(list *unstructured.UnstructuredList, targetGV schema.GroupVersion) (*unstructured.UnstructuredList, error) {
-	for i := range list.Items {
-		list.Items[i].SetGroupVersionKind(targetGV.WithKind(list.Items[i].GroupVersionKind().Kind))
+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))
+		}
 	}
-	return list, nil
+	in.GetObjectKind().SetGroupVersionKind(targetGV.WithKind(in.GetObjectKind().GroupVersionKind().Kind))
+	return in, nil
 }