Fix multi-version CRD + admission webhook

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/BUILD

@@ -14,6 +14,7 @@ go_library(
         "//staging/src/k8s.io/api/admission/v1beta1:go_default_library",
         "//staging/src/k8s.io/api/admissionregistration/v1beta1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/conversion.go

@@ -19,6 +19,7 @@ package generic
 import (
 	"fmt"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
@@ -36,6 +37,16 @@ func (c *convertor) ConvertToGVK(obj runtime.Object, gvk schema.GroupVersionKind
 		return obj, nil
 	}
 	out, err := c.Scheme.New(gvk)
+	// This is a workaround for http://issue.k8s.io/73752 for sending multi-version custom resources to admission webhooks.
+	// If we're being asked to convert an unstructured object for a kind that is not registered, it must be a custom resource.
+	// In 1.13, only no-op custom resource conversion is possible, so setting the group version is sufficient to "convert".
+	// In 1.14+, this was fixed correctly in http://issue.k8s.io/74154 by plumbing the actual object converter here.
+	if runtime.IsNotRegisteredError(err) {
+		if u, ok := obj.(*unstructured.Unstructured); ok {
+			u.GetObjectKind().SetGroupVersionKind(gvk)
+			return u, nil
+		}
+	}
 	if err != nil {
 		return nil, err
 	}

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/conversion_test.go

@@ -126,6 +126,27 @@ func TestConvertToGVK(t *testing.T) {
 				},
 			},
 		},
+		"no-op conversion for Unstructured object whose gvk does not match the desired gvk": {
+			obj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "mygroup.k8s.io/v1",
+					"kind":       "Flunder",
+					"data": map[string]interface{}{
+						"Key": "Value",
+					},
+				},
+			},
+			gvk: schema.GroupVersionKind{Group: "mygroup.k8s.io", Version: "v2", Kind: "Flunder"},
+			expectedObj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "mygroup.k8s.io/v2",
+					"kind":       "Flunder",
+					"data": map[string]interface{}{
+						"Key": "Value",
+					},
+				},
+			},
+		},
 	}
 
 	for name, test := range table {

test/e2e/apimachinery/webhook.go

@@ -183,13 +183,31 @@ var _ = SIGDescribe("AdmissionWebhook", func() {
 		testCRDDenyWebhook(f)
 	})
 
+	It("Should mutate custom resource with different stored version", func() {
+		testcrd, err := framework.CreateMultiVersionTestCRDWithV1Storage(f)
+		if err != nil {
+			return
+		}
+		defer testcrd.CleanUp()
+		webhookCleanup := registerMutatingWebhookForCustomResource(f, context, testcrd)
+		defer webhookCleanup()
+		testMultiVersionCustomResourceWebhook(f, testcrd)
+	})
+
+	It("Should deny crd creation", func() {
+		crdWebhookCleanup := registerValidatingWebhookForCRD(f, context)
+		defer crdWebhookCleanup()
+
+		testCRDDenyWebhook(f)
+	})
+
 	// TODO: add more e2e tests for mutating webhooks
 	// 1. mutating webhook that mutates pod
 	// 2. mutating webhook that sends empty patch
 	//   2.1 and sets status.allowed=true
 	//   2.2 and sets status.allowed=false
 	// 3. mutating webhook that sends patch, but also sets status.allowed=false
-	// 4. mtuating webhook that fail-open v.s. fail-closed
+	// 4. mutating webhook that fail-open v.s. fail-closed
 })
 
 func createAuthReaderRoleBinding(f *framework.Framework, namespace string) {
@@ -1154,7 +1172,7 @@ func registerWebhookForCustomResource(f *framework.Framework, context *certConte
 			{
 				Name: "deny-unwanted-custom-resource-data.k8s.io",
 				Rules: []v1beta1.RuleWithOperations{{
-					Operations: []v1beta1.OperationType{v1beta1.Create},
+					Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update},
 					Rule: v1beta1.Rule{
 						APIGroups:   []string{testcrd.ApiGroup},
 						APIVersions: testcrd.GetAPIVersions(),
@@ -1195,7 +1213,7 @@ func registerMutatingWebhookForCustomResource(f *framework.Framework, context *c
 			{
 				Name: "mutate-custom-resource-data-stage-1.k8s.io",
 				Rules: []v1beta1.RuleWithOperations{{
-					Operations: []v1beta1.OperationType{v1beta1.Create},
+					Operations: []v1beta1.OperationType{v1beta1.Create, v1beta1.Update},
 					Rule: v1beta1.Rule{
 						APIGroups:   []string{testcrd.ApiGroup},
 						APIVersions: testcrd.GetAPIVersions(),
@@ -1292,6 +1310,37 @@ func testMutatingCustomResourceWebhook(f *framework.Framework, crd *apiextension
 	}
 }
 
+func testMultiVersionCustomResourceWebhook(f *framework.Framework, testcrd *framework.TestCrd) {
+	customResourceClient := testcrd.GetV1DynamicClient()
+	By("Creating a custom resource while v1 is storage version")
+	crName := "cr-instance-1"
+	cr := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"kind":       testcrd.Crd.Spec.Names.Kind,
+			"apiVersion": testcrd.Crd.Spec.Group + "/" + testcrd.Crd.Spec.Version,
+			"metadata": map[string]interface{}{
+				"name":      crName,
+				"namespace": f.Namespace.Name,
+			},
+			"data": map[string]interface{}{
+				"mutation-start": "yes",
+			},
+		},
+	}
+	_, err := customResourceClient.Create(cr, metav1.CreateOptions{})
+	Expect(err).NotTo(HaveOccurred(), "failed to create custom resource %s in namespace: %s", crName, f.Namespace.Name)
+
+	By("Patching Custom Resource Definition to set v2 as storage")
+	apiVersionWithV2StoragePatch := fmt.Sprint(`{"spec": {"versions": [{"name": "v1", "storage": false, "served": true},{"name": "v2", "storage": true, "served": true}]}}`)
+	_, err = testcrd.ApiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Patch(testcrd.Crd.Name, types.StrategicMergePatchType, []byte(apiVersionWithV2StoragePatch))
+	Expect(err).NotTo(HaveOccurred(), "failed to patch custom resource definition %s in namespace: %s", testcrd.Crd.Name, f.Namespace.Name)
+
+	By("Patching the custom resource while v2 is storage version")
+	crDummyPatch := fmt.Sprint(`[{ "op": "add", "path": "/dummy", "value": "test" }]`)
+	_, err = testcrd.DynamicClients["v2"].Patch(crName, types.JSONPatchType, []byte(crDummyPatch), metav1.UpdateOptions{})
+	Expect(err).NotTo(HaveOccurred(), "failed to patch custom resource %s in namespace: %s", crName, f.Namespace.Name)
+}
+
 func registerValidatingWebhookForCRD(f *framework.Framework, context *certContext) func() {
 	client := f.ClientSet
 	By("Registering the crd webhook via the AdmissionRegistration API")

test/e2e/framework/crd_util.go

@@ -121,6 +121,24 @@ func CreateTestCRD(f *Framework) (*TestCrd, error) {
 	return CreateMultiVersionTestCRD(f, group, apiVersions, nil)
 }
 
+// CreateTestCRD creates a new CRD specifically for the calling test.
+func CreateMultiVersionTestCRDWithV1Storage(f *Framework) (*TestCrd, error) {
+	group := fmt.Sprintf("%s-multiversion-crd-test.k8s.io", f.BaseName)
+	apiVersions := []apiextensionsv1beta1.CustomResourceDefinitionVersion{
+		{
+			Name:    "v1",
+			Served:  true,
+			Storage: true,
+		},
+		{
+			Name:    "v2",
+			Served:  true,
+			Storage: false,
+		},
+	}
+	return CreateMultiVersionTestCRD(f, group, apiVersions, nil)
+}
+
 // newCRDForTest generates a CRD definition for the test
 func newCRDForTest(testcrd *TestCrd) *apiextensionsv1beta1.CustomResourceDefinition {
 	return &apiextensionsv1beta1.CustomResourceDefinition{