fix field removal in mutating admission webhooks

A mutating admission controller webhook doesn't remove object fields
when instructed to.

E.g. when the JSON patch

  [
    {"op": "remove", "path": "/spec/containers/0/resources/limits/fpga-arria10"},
    {"op": "add", "path": "/spec/containers/0/resources/limits/fpga-interface-id-524abcf", "value": 1}
  ]

is applied to this pod

apiVersion: v1
kind: Pod
metadata:
  name: test-pod
spec:
  restartPolicy: Never
  containers:
    -
      name: test-pod-container
      image: ubuntu:bionic
      imagePullPolicy: IfNotPresent
      command: [ "ls", "-l", "/" ]
      resources:
        limits:
          fpga-arria10: 1

in order to replace the resource name "fpga-arria10" with something understandable
by the device plugin the resulting pod spec still contains the old field plus
a new one. The resulting pod looks like

apiVersion: v1
kind: Pod
metadata:
  name: test-pod
spec:
  restartPolicy: Never
  containers:
    -
      name: test-pod-container
      image: ubuntu:bionic
      imagePullPolicy: IfNotPresent
      command: [ "ls", "-l", "/" ]
      resources:
        limits:
          fpga-arria10: 1
          fpga-interface-id-524abcf: 1

The patch unmarshals patched JSON into a new empty object instead of
existing one. Otherwise JSON unmarshaling reuses existing maps, keeping
existing entries as specified in the "encoding/json" standard package.

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

@@ -15,6 +15,7 @@ go_library(
         "//vendor/k8s.io/api/admission/v1beta1:go_default_library",
         "//vendor/k8s.io/api/admissionregistration/v1beta1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/serializer/json:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go

@@ -29,6 +29,7 @@ import (
 	admissionv1beta1 "k8s.io/api/admission/v1beta1"
 	"k8s.io/api/admissionregistration/v1beta1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	admissionmetrics "k8s.io/apiserver/pkg/admission/metrics"
@@ -113,11 +114,24 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta
 	if err != nil {
 		return apierrors.NewInternalError(err)
 	}
+
+	var newVersionedObject runtime.Object
+	if _, ok := attr.VersionedObject.(*unstructured.Unstructured); ok {
+		// Custom Resources don't have corresponding Go struct's.
+		// They are represented as Unstructured.
+		newVersionedObject = &unstructured.Unstructured{}
+	} else {
+		newVersionedObject, err = a.plugin.scheme.New(attr.GetKind())
+		if err != nil {
+			return apierrors.NewInternalError(err)
+		}
+	}
 	// TODO: if we have multiple mutating webhooks, we can remember the json
 	// instead of encoding and decoding for each one.
-	if _, _, err := a.plugin.jsonSerializer.Decode(patchedJS, nil, attr.VersionedObject); err != nil {
+	if _, _, err := a.plugin.jsonSerializer.Decode(patchedJS, nil, newVersionedObject); err != nil {
 		return apierrors.NewInternalError(err)
 	}
+	attr.VersionedObject = newVersionedObject
 	a.plugin.scheme.Default(attr.VersionedObject)
 	return nil
 }

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go

@@ -18,13 +18,16 @@ package mutating
 
 import (
 	"net/url"
+	"reflect"
 	"strings"
 	"testing"
 
 	"k8s.io/api/admission/v1beta1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apiserver/pkg/admission"
 	webhooktesting "k8s.io/apiserver/pkg/admission/plugin/webhook/testing"
 )
 
@@ -69,10 +72,22 @@ func TestAdmit(t *testing.T) {
 			continue
 		}
 
-		err = wh.Admit(webhooktesting.NewAttribute(ns))
+		var attr admission.Attributes
+		if tt.IsCRD {
+			attr = webhooktesting.NewAttributeUnstructured(ns, tt.AdditionalLabels)
+		} else {
+			attr = webhooktesting.NewAttribute(ns, tt.AdditionalLabels)
+		}
+
+		err = wh.Admit(attr)
 		if tt.ExpectAllow != (err == nil) {
 			t.Errorf("%s: expected allowed=%v, but got err=%v", tt.Name, tt.ExpectAllow, err)
 		}
+		if tt.ExpectLabels != nil {
+			if !reflect.DeepEqual(tt.ExpectLabels, attr.GetObject().(metav1.Object).GetLabels()) {
+				t.Errorf("%s: expected labels '%v', but got '%v'", tt.Name, tt.ExpectLabels, attr.GetObject().(metav1.Object).GetLabels())
+			}
+		}
 		// ErrWebhookRejected is not an error for our purposes
 		if tt.ErrorContains != "" {
 			if err == nil || !strings.Contains(err.Error(), tt.ErrorContains) {
@@ -127,7 +142,7 @@ func TestAdmitCachedClient(t *testing.T) {
 			continue
 		}
 
-		err = wh.Admit(webhooktesting.NewAttribute(ns))
+		err = wh.Admit(webhooktesting.NewAttribute(ns, nil))
 		if tt.ExpectAllow != (err == nil) {
 			t.Errorf("%s: expected allowed=%v, but got err=%v", tt.Name, tt.ExpectAllow, err)
 		}

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

@@ -15,7 +15,9 @@ go_library(
         "//vendor/k8s.io/api/admissionregistration/v1beta1:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/admission:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/admission/plugin/webhook/config:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/admission/plugin/webhook/testcerts:go_default_library",

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go

@@ -22,7 +22,9 @@ import (
 	registrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apiserver/pkg/admission"
 	"k8s.io/apiserver/pkg/admission/plugin/webhook/testcerts"
 	"k8s.io/apiserver/pkg/authentication/user"
@@ -74,36 +76,57 @@ func NewFakeDataSource(name string, webhooks []registrationv1beta1.Webhook, muta
 	return client, informerFactory
 }
 
-// NewAttribute returns static admission Attributes for testing.
+func newAttributesRecord(object metav1.Object, oldObject metav1.Object, kind schema.GroupVersionKind, namespace string, name string, resource string, labels map[string]string) admission.Attributes {
-func NewAttribute(namespace string) admission.Attributes {
+	object.SetName(name)
-	// Set up a test object for the call
+	object.SetNamespace(namespace)
-	kind := corev1.SchemeGroupVersion.WithKind("Pod")
+	objectLabels := map[string]string{resource + ".name": name}
-	name := "my-pod"
+	for k, v := range labels {
-	object := corev1.Pod{
+		objectLabels[k] = v
-		ObjectMeta: metav1.ObjectMeta{
-			Labels: map[string]string{
-				"pod.name": name,
-			},
-			Name:      name,
-			Namespace: namespace,
-		},
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: "v1",
-			Kind:       "Pod",
-		},
 	}
-	oldObject := corev1.Pod{
+	object.SetLabels(objectLabels)
-		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
+
-	}
+	oldObject.SetName(name)
-	operation := admission.Update
+	oldObject.SetNamespace(namespace)
-	resource := corev1.Resource("pods").WithVersion("v1")
+
+	gvr := kind.GroupVersion().WithResource(resource)
 	subResource := ""
 	userInfo := user.DefaultInfo{
 		Name: "webhook-test",
 		UID:  "webhook-test",
 	}
 
-	return admission.NewAttributesRecord(&object, &oldObject, kind, namespace, name, resource, subResource, operation, &userInfo)
+	return admission.NewAttributesRecord(object.(runtime.Object), oldObject.(runtime.Object), kind, namespace, name, gvr, subResource, admission.Update, &userInfo)
+}
+
+// NewAttribute returns static admission Attributes for testing.
+func NewAttribute(namespace string, labels map[string]string) admission.Attributes {
+	// Set up a test object for the call
+	object := corev1.Pod{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "v1",
+			Kind:       "Pod",
+		},
+	}
+	oldObject := corev1.Pod{}
+	kind := corev1.SchemeGroupVersion.WithKind("Pod")
+	name := "my-pod"
+
+	return newAttributesRecord(&object, &oldObject, kind, namespace, name, "pod", labels)
+}
+
+// NewAttributeUnstructured returns static admission Attributes for testing with custom resources.
+func NewAttributeUnstructured(namespace string, labels map[string]string) admission.Attributes {
+	// Set up a test object for the call
+	object := unstructured.Unstructured{}
+	object.SetKind("TestCRD")
+	object.SetAPIVersion("custom.resource/v1")
+	oldObject := unstructured.Unstructured{}
+	oldObject.SetKind("TestCRD")
+	oldObject.SetAPIVersion("custom.resource/v1")
+	kind := object.GroupVersionKind()
+	name := "my-test-crd"
+
+	return newAttributesRecord(&object, &oldObject, kind, namespace, name, "crd", labels)
 }
 
 type urlConfigGenerator struct {
@@ -122,11 +145,14 @@ func (c urlConfigGenerator) ccfgURL(urlPath string) registrationv1beta1.WebhookC
 
 // Test is a webhook test case.
 type Test struct {
-	Name          string
+	Name             string
-	Webhooks      []registrationv1beta1.Webhook
+	Webhooks         []registrationv1beta1.Webhook
-	Path          string
+	Path             string
-	ExpectAllow   bool
+	IsCRD            bool
-	ErrorContains string
+	AdditionalLabels map[string]string
+	ExpectLabels     map[string]string
+	ExpectAllow      bool
+	ErrorContains    string
 }
 
 // NewTestCases returns test cases with a given base url.
@@ -158,6 +184,64 @@ func NewTestCases(url *url.URL) []Test {
 			}},
 			ExpectAllow: true,
 		},
+		{
+			Name: "match & remove label",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "removeLabel",
+				ClientConfig:      ccfgSVC("removeLabel"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			ExpectAllow:      true,
+			AdditionalLabels: map[string]string{"remove": "me"},
+			ExpectLabels:     map[string]string{"pod.name": "my-pod"},
+		},
+		{
+			Name: "match & add label",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "addLabel",
+				ClientConfig:      ccfgSVC("addLabel"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			ExpectAllow:  true,
+			ExpectLabels: map[string]string{"pod.name": "my-pod", "added": "test"},
+		},
+		{
+			Name: "match CRD & add label",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "addLabel",
+				ClientConfig:      ccfgSVC("addLabel"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			IsCRD:        true,
+			ExpectAllow:  true,
+			ExpectLabels: map[string]string{"crd.name": "my-test-crd", "added": "test"},
+		},
+		{
+			Name: "match CRD & remove label",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "removeLabel",
+				ClientConfig:      ccfgSVC("removeLabel"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			IsCRD:            true,
+			ExpectAllow:      true,
+			AdditionalLabels: map[string]string{"remove": "me"},
+			ExpectLabels:     map[string]string{"crd.name": "my-test-crd"},
+		},
+		{
+			Name: "match & invalid mutation",
+			Webhooks: []registrationv1beta1.Webhook{{
+				Name:              "invalidMutation",
+				ClientConfig:      ccfgSVC("invalidMutation"),
+				Rules:             matchEverythingRules,
+				NamespaceSelector: &metav1.LabelSelector{},
+			}},
+			ErrorContains: "invalid character",
+		},
 		{
 			Name: "match & disallow",
 			Webhooks: []registrationv1beta1.Webhook{{

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/webhook_server.go

@@ -85,6 +85,36 @@ func webhookHandler(w http.ResponseWriter, r *http.Request) {
 				Allowed: true,
 			},
 		})
+	case "/removeLabel":
+		w.Header().Set("Content-Type", "application/json")
+		pt := v1beta1.PatchTypeJSONPatch
+		json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{
+			Response: &v1beta1.AdmissionResponse{
+				Allowed:   true,
+				PatchType: &pt,
+				Patch:     []byte(`[{"op": "remove", "path": "/metadata/labels/remove"}]`),
+			},
+		})
+	case "/addLabel":
+		w.Header().Set("Content-Type", "application/json")
+		pt := v1beta1.PatchTypeJSONPatch
+		json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{
+			Response: &v1beta1.AdmissionResponse{
+				Allowed:   true,
+				PatchType: &pt,
+				Patch:     []byte(`[{"op": "add", "path": "/metadata/labels/added", "value": "test"}]`),
+			},
+		})
+	case "/invalidMutation":
+		w.Header().Set("Content-Type", "application/json")
+		pt := v1beta1.PatchTypeJSONPatch
+		json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{
+			Response: &v1beta1.AdmissionResponse{
+				Allowed:   true,
+				PatchType: &pt,
+				Patch:     []byte(`[{"op": "add", "CORRUPTED_KEY":}]`),
+			},
+		})
 	case "/nilResponse":
 		w.Header().Set("Content-Type", "application/json")
 		json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{})

staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin_test.go

@@ -75,7 +75,7 @@ func TestValidate(t *testing.T) {
 			continue
 		}
 
-		err = wh.Validate(webhooktesting.NewAttribute(ns))
+		err = wh.Validate(webhooktesting.NewAttribute(ns, nil))
 		if tt.ExpectAllow != (err == nil) {
 			t.Errorf("%s: expected allowed=%v, but got err=%v", tt.Name, tt.ExpectAllow, err)
 		}
@@ -133,7 +133,7 @@ func TestValidateCachedClient(t *testing.T) {
 			continue
 		}
 
-		err = wh.Validate(webhooktesting.NewAttribute(ns))
+		err = wh.Validate(webhooktesting.NewAttribute(ns, nil))
 		if tt.ExpectAllow != (err == nil) {
 			t.Errorf("%s: expected allowed=%v, but got err=%v", tt.Name, tt.ExpectAllow, err)
 		}