diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/generated.proto b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/generated.proto index 7aa8c8b2042..ec030190e6e 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/generated.proto +++ b/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 diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go index 715d107e97c..63e25245d82 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go +++ b/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 diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/BUILD index 19e5972f05a..56ccbcc7e17 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/BUILD +++ b/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", ], ) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go index dbfe5e6ea5b..8cc7e78e220 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go +++ b/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 { - if e, a := unstructIn.GetKind(), unstructOut.GetKind(); e != a { +// validateConvertedObject checks that ObjectMeta fields match, with the exception of +// 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 { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter_test.go new file mode 100644 index 00000000000..01c97e90b58 --- /dev/null +++ b/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)) + } + }) + } +}