apiextensions: ignore ObjectMeta from webhook converted objects other than labels and annotations

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/generated.proto

@@ -51,7 +51,8 @@ message ConversionResponse {
 
   // `convertedObjects` is the list of converted version of `request.objects` if the `result` is successful otherwise empty.
   // The webhook is expected to set apiVersion of these objects to the ConversionRequest.desiredAPIVersion. The list
-  // must also has the same size as input list with the same objects in the same order(i.e. equal UIDs and object meta)
+  // must also have the same size as the input list with the same objects in the same order (equal kind, UID, name and namespace).
+  // The webhook is allowed to mutate labels and annotations. Any other change to the metadata is silently ignored.
   repeated k8s.io.apimachinery.pkg.runtime.RawExtension convertedObjects = 2;
 
   // `result` contains the result of conversion with extra details if the conversion failed. `result.status` determines if

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go

@@ -456,7 +456,8 @@ type ConversionResponse struct {
 	UID types.UID `json:"uid" protobuf:"bytes,1,name=uid"`
 	// `convertedObjects` is the list of converted version of `request.objects` if the `result` is successful otherwise empty.
 	// The webhook is expected to set apiVersion of these objects to the ConversionRequest.desiredAPIVersion. The list
-	// must also has the same size as input list with the same objects in the same order(i.e. equal UIDs and object meta)
+	// must also have the same size as the input list with the same objects in the same order (equal kind, UID, name and namespace).
+	// The webhook is allowed to mutate labels and annotations. Any other change to the metadata is silently ignored.
 	ConvertedObjects []runtime.RawExtension `json:"convertedObjects" protobuf:"bytes,2,rep,name=convertedObjects"`
 	// `result` contains the result of conversion with extra details if the conversion failed. `result.status` determines if
 	// the conversion failed or succeeded. The `result.status` field is required and represent the success or failure of the

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/BUILD

@@ -16,11 +16,14 @@ go_library(
         "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library",
         "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library",
         "//staging/src/k8s.io/apiextensions-apiserver/pkg/features:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/api/validation:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation: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/apimachinery/pkg/util/uuid:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/util/webhook:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes/scheme:go_default_library",
@@ -45,13 +48,18 @@ filegroup(
 
 go_test(
     name = "go_default_test",
-    srcs = ["converter_test.go"],
+    srcs = [
+        "converter_test.go",
+        "webhook_converter_test.go",
+    ],
     embed = [":go_default_library"],
     deps = [
         "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions: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/apimachinery/pkg/util/diff:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/validation:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/util/webhook:go_default_library",
     ],
 )

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

@@ -20,11 +20,14 @@ import (
 	"context"
 	"fmt"
 
-	"k8s.io/apimachinery/pkg/apis/meta/v1"
+	apivalidation "k8s.io/apimachinery/pkg/api/validation"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/uuid"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/util/webhook"
 	"k8s.io/client-go/rest"
 
@@ -229,6 +232,9 @@ func (c *webhookConverter) Convert(in runtime.Object, toGV schema.GroupVersion)
 			if err := validateConvertedObject(original, unstructConverted); err != nil {
 				return nil, fmt.Errorf("invalid converted object at index %v: %v", convertedIndex, err)
 			}
+			if err := restoreObjectMeta(original, unstructConverted); err != nil {
+				return nil, fmt.Errorf("invalid metadata in object at index %v: %v", convertedIndex, err)
+			}
 			convertedList.Items[i] = *unstructConverted
 		}
 		convertedList.SetAPIVersion(toGV.String())
@@ -262,25 +268,103 @@ func (c *webhookConverter) Convert(in runtime.Object, toGV schema.GroupVersion)
 	if err := validateConvertedObject(unstructIn, unstructConverted); err != nil {
 		return nil, fmt.Errorf("invalid converted object: %v", err)
 	}
+	if err := restoreObjectMeta(unstructIn, unstructConverted); err != nil {
+		return nil, fmt.Errorf("invalid metadata in converted object: %v", err)
+	}
 	return converted, nil
 }
 
-func validateConvertedObject(unstructIn, unstructOut *unstructured.Unstructured) error {
+// validateConvertedObject checks that ObjectMeta fields match, with the exception of
-	if e, a := unstructIn.GetKind(), unstructOut.GetKind(); e != a {
+// labels and annotations.
+func validateConvertedObject(in, out *unstructured.Unstructured) error {
+	if e, a := in.GetKind(), out.GetKind(); e != a {
 		return fmt.Errorf("must have the same kind: %v != %v", e, a)
 	}
-	if e, a := unstructIn.GetName(), unstructOut.GetName(); e != a {
+	if e, a := in.GetName(), out.GetName(); e != a {
 		return fmt.Errorf("must have the same name: %v != %v", e, a)
 	}
-	if e, a := unstructIn.GetNamespace(), unstructOut.GetNamespace(); e != a {
+	if e, a := in.GetNamespace(), out.GetNamespace(); e != a {
 		return fmt.Errorf("must have the same namespace: %v != %v", e, a)
 	}
-	if e, a := unstructIn.GetUID(), unstructOut.GetUID(); e != a {
+	if e, a := in.GetUID(), out.GetUID(); e != a {
 		return fmt.Errorf("must have the same UID: %v != %v", e, a)
 	}
 	return nil
 }
 
+// restoreObjectMeta deep-copies metadata from original into converted, while preserving labels and annotations from converted.
+func restoreObjectMeta(original, converted *unstructured.Unstructured) error {
+	obj, found := converted.Object["metadata"]
+	if !found {
+		return fmt.Errorf("missing metadata in converted object")
+	}
+	responseMetaData, ok := obj.(map[string]interface{})
+	if !ok {
+		return fmt.Errorf("invalid metadata of type %T in converted object", obj)
+	}
+
+	if _, ok := original.Object["metadata"]; !ok {
+		// the original will always have metadata. But just to be safe, let's clear in converted
+		// with an empty object instead of nil, to be able to add labels and annotations below.
+		converted.Object["metadata"] = map[string]interface{}{}
+	} else {
+		converted.Object["metadata"] = runtime.DeepCopyJSONValue(original.Object["metadata"])
+	}
+
+	obj = converted.Object["metadata"]
+	convertedMetaData, ok := obj.(map[string]interface{})
+	if !ok {
+		return fmt.Errorf("invalid metadata of type %T in input object", obj)
+	}
+
+	for _, fld := range []string{"labels", "annotations"} {
+		obj, found := responseMetaData[fld]
+		if !found || obj == nil {
+			delete(convertedMetaData, fld)
+			continue
+		}
+		responseField, ok := obj.(map[string]interface{})
+		if !ok {
+			return fmt.Errorf("invalid metadata.%s of type %T in converted object", fld, obj)
+		}
+
+		originalField, ok := convertedMetaData[fld].(map[string]interface{})
+		if !ok && convertedMetaData[fld] != nil {
+			return fmt.Errorf("invalid metadata.%s of type %T in original object", fld, convertedMetaData[fld])
+		}
+
+		somethingChanged := len(originalField) != len(responseField)
+		for k, v := range responseField {
+			if _, ok := v.(string); !ok {
+				return fmt.Errorf("metadata.%s[%s] must be a string, but is %T in converted object", fld, k, v)
+			}
+			if originalField[k] != interface{}(v) {
+				somethingChanged = true
+			}
+		}
+
+		if somethingChanged {
+			stringMap := make(map[string]string, len(responseField))
+			for k, v := range responseField {
+				stringMap[k] = v.(string)
+			}
+			var errs field.ErrorList
+			if fld == "labels" {
+				errs = metav1validation.ValidateLabels(stringMap, field.NewPath("metadata", "labels"))
+			} else {
+				errs = apivalidation.ValidateAnnotations(stringMap, field.NewPath("metadata", "annotation"))
+			}
+			if len(errs) > 0 {
+				return errs.ToAggregate()
+			}
+		}
+
+		convertedMetaData[fld] = responseField
+	}
+
+	return nil
+}
+
 // isEmptyUnstructuredObject returns true if in is an empty unstructured object, i.e. an unstructured object that does
 // not have any field except apiVersion and kind.
 func isEmptyUnstructuredObject(in runtime.Object) bool {

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

@@ -0,0 +1,201 @@
+/*
+Copyright 2018 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/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/util/diff"
+	"k8s.io/apimachinery/pkg/util/validation"
+)
+
+func TestRestoreObjectMeta(t *testing.T) {
+	tests := []struct {
+		name      string
+		original  map[string]interface{}
+		converted map[string]interface{}
+		expected  map[string]interface{}
+		wantErr   bool
+	}{
+		{"no converted metadata",
+			map[string]interface{}{"metadata": map[string]interface{}{}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"spec": map[string]interface{}{}},
+			map[string]interface{}{"spec": map[string]interface{}{}},
+			true,
+		},
+		{"invalid converted metadata",
+			map[string]interface{}{"metadata": map[string]interface{}{}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": []interface{}{"foo"}},
+			map[string]interface{}{"metadata": []interface{}{"foo"}},
+			true,
+		},
+		{"no original metadata",
+			map[string]interface{}{"spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{}, "spec": map[string]interface{}{}},
+			false,
+		},
+		{"invalid original metadata",
+			map[string]interface{}{"metadata": []interface{}{"foo"}},
+			map[string]interface{}{"metadata": map[string]interface{}{}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": []interface{}{"foo"}, "spec": map[string]interface{}{}},
+			true,
+		},
+		{"changed label, annotations and non-label",
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "bar",
+				"labels":      map[string]interface{}{"a": "A", "b": "B"},
+				"annotations": map[string]interface{}{"a": "1", "b": "2"},
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "abc",
+				"labels":      map[string]interface{}{"a": "AA", "b": "B"},
+				"annotations": map[string]interface{}{"a": "1", "b": "22"},
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "bar",
+				"labels":      map[string]interface{}{"a": "AA", "b": "B"},
+				"annotations": map[string]interface{}{"a": "1", "b": "22"},
+			}, "spec": map[string]interface{}{}},
+			false,
+		},
+		{"added labels and annotations",
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo": "bar",
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "abc",
+				"labels":      map[string]interface{}{"a": "AA", "b": "B"},
+				"annotations": map[string]interface{}{"a": "1", "b": "22"},
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "bar",
+				"labels":      map[string]interface{}{"a": "AA", "b": "B"},
+				"annotations": map[string]interface{}{"a": "1", "b": "22"},
+			}, "spec": map[string]interface{}{}},
+			false,
+		},
+		{"added labels and annotations, with nil before",
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "bar",
+				"labels":      nil,
+				"annotations": nil,
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "abc",
+				"labels":      map[string]interface{}{"a": "AA", "b": "B"},
+				"annotations": map[string]interface{}{"a": "1", "b": "22"},
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "bar",
+				"labels":      map[string]interface{}{"a": "AA", "b": "B"},
+				"annotations": map[string]interface{}{"a": "1", "b": "22"},
+			}, "spec": map[string]interface{}{}},
+			false,
+		},
+		{"removed labels and annotations",
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "bar",
+				"labels":      map[string]interface{}{"a": "AA", "b": "B"},
+				"annotations": map[string]interface{}{"a": "1", "b": "22"},
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo": "abc",
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo": "bar",
+			}, "spec": map[string]interface{}{}},
+			false,
+		},
+		{"nil'ed labels and annotations",
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "bar",
+				"labels":      map[string]interface{}{"a": "AA", "b": "B"},
+				"annotations": map[string]interface{}{"a": "1", "b": "22"},
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "abc",
+				"labels":      nil,
+				"annotations": nil,
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo": "bar",
+			}, "spec": map[string]interface{}{}},
+			false,
+		},
+		{"added labels and annotations",
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo": "bar",
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo":         "abc",
+				"labels":      map[string]interface{}{"a": nil, "b": "B"},
+				"annotations": map[string]interface{}{"a": nil, "b": "22"},
+			}, "spec": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{
+				"foo": "bar",
+			}, "spec": map[string]interface{}{}},
+			true,
+		},
+		{"invalid label key",
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{"labels": map[string]interface{}{"some/non-qualified/label": "x"}}},
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			true,
+		},
+		{"invalid annotation key",
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{"labels": map[string]interface{}{"some/non-qualified/label": "x"}}},
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			true,
+		},
+		{"invalid label value",
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{"labels": map[string]interface{}{"foo": "üäö"}}},
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			true,
+		},
+		{"too big label value",
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{"labels": map[string]interface{}{"foo": strings.Repeat("x", validation.LabelValueMaxLength+1)}}},
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			true,
+		},
+		{"too big annotation value",
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			map[string]interface{}{"metadata": map[string]interface{}{"annotations": map[string]interface{}{"foo": strings.Repeat("x", 256*(1<<10)+1)}}},
+			map[string]interface{}{"metadata": map[string]interface{}{}},
+			true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := restoreObjectMeta(&unstructured.Unstructured{Object: tt.original}, &unstructured.Unstructured{Object: tt.converted}); err == nil && tt.wantErr {
+				t.Fatalf("expected error, but didn't get one")
+			} else if err != nil && !tt.wantErr {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			if !reflect.DeepEqual(tt.converted, tt.expected) {
+				t.Errorf("unexpected result: %s", diff.ObjectDiff(tt.expected, tt.converted))
+			}
+		})
+	}
+}