From 8a2414a6bc12bd1a8d90e14a8a4a0878ac0c2275 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 10 May 2019 17:11:54 +0200 Subject: [PATCH 1/3] apiextensions: little webhook conversion cleanup --- .../apiserver/conversion/webhook_converter.go | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) 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 3e56e819189..dbfe5e6ea5b 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 @@ -115,7 +115,7 @@ func createConversionReview(obj runtime.Object, apiVersion string) *v1beta1.Conv listObj, isList := obj.(*unstructured.UnstructuredList) var objects []runtime.RawExtension if isList { - for i := 0; i < len(listObj.Items); i++ { + for i := range listObj.Items { // Only sent item for conversion, if the apiVersion is different if listObj.Items[i].GetAPIVersion() != apiVersion { objects = append(objects, runtime.RawExtension{Object: &listObj.Items[i]}) @@ -199,33 +199,34 @@ func (c *webhookConverter) Convert(in runtime.Object, toGV schema.GroupVersion) } if isList { + // start a deepcopy of the input and fill in the converted objects from the response at the right spots. + // The response list might be sparse because objects had the right version already. convertedList := listObj.DeepCopy() - // Collection of items sent for conversion is different than list items - // because only items that needed conversion has been sent. convertedIndex := 0 - for i := 0; i < len(listObj.Items); i++ { - if listObj.Items[i].GetAPIVersion() == toGV.String() { - // This item has not been sent for conversion, skip it. + for i := range convertedList.Items { + original := &convertedList.Items[i] + if original.GetAPIVersion() == toGV.String() { + // This item has not been sent for conversion, and therefore does not show up in the response. + // convertedList has the right item already. continue } converted, err := getRawExtensionObject(response.Response.ConvertedObjects[convertedIndex]) - convertedIndex++ - original := listObj.Items[i] if err != nil { return nil, fmt.Errorf("invalid converted object at index %v: %v", convertedIndex, err) } - if e, a := toGV, converted.GetObjectKind().GroupVersionKind().GroupVersion(); e != a { - return nil, fmt.Errorf("invalid converted object at index %v: invalid groupVersion, e=%v, a=%v", convertedIndex, e, a) + convertedIndex++ + if expected, got := toGV, converted.GetObjectKind().GroupVersionKind().GroupVersion(); expected != got { + return nil, fmt.Errorf("invalid converted object at index %v: invalid groupVersion, expected=%v, got=%v", convertedIndex, expected, got) } - if e, a := original.GetObjectKind().GroupVersionKind().Kind, converted.GetObjectKind().GroupVersionKind().Kind; e != a { - return nil, fmt.Errorf("invalid converted object at index %v: invalid kind, e=%v, a=%v", convertedIndex, e, a) + if expected, got := original.GetObjectKind().GroupVersionKind().Kind, converted.GetObjectKind().GroupVersionKind().Kind; expected != got { + return nil, fmt.Errorf("invalid converted object at index %v: invalid kind, expected=%v, got=%v", convertedIndex, expected, got) } unstructConverted, ok := converted.(*unstructured.Unstructured) if !ok { // this should not happened - return nil, fmt.Errorf("CR conversion failed") + return nil, fmt.Errorf("invalid converted object at index %v: invalid type, expected=Unstructured, got=%T", convertedIndex, converted) } - if err := validateConvertedObject(&listObj.Items[i], unstructConverted); err != nil { + if err := validateConvertedObject(original, unstructConverted); err != nil { return nil, fmt.Errorf("invalid converted object at index %v: %v", convertedIndex, err) } convertedList.Items[i] = *unstructConverted From abc0709cb6ca7004ac610649d66d26ccd82bdaa4 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 10 May 2019 17:12:32 +0200 Subject: [PATCH 2/3] apiextensions: ignore ObjectMeta from webhook converted objects other than labels and annotations --- .../apiextensions/v1beta1/generated.proto | 3 +- .../pkg/apis/apiextensions/v1beta1/types.go | 3 +- .../pkg/apiserver/conversion/BUILD | 10 +- .../apiserver/conversion/webhook_converter.go | 96 ++++++++- .../conversion/webhook_converter_test.go | 201 ++++++++++++++++++ 5 files changed, 304 insertions(+), 9 deletions(-) create mode 100644 staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter_test.go 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)) + } + }) + } +} From 9814914b94c2b87b5063c02daf08aff027e6a5b5 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Tue, 14 May 2019 17:09:26 +0200 Subject: [PATCH 3/3] apiextensions: add ObjectMeta mutation test in crd conversion --- .../conversion/webhook_converter_test.go | 14 +- .../integration/conversion/conversion_test.go | 193 ++++++++++++++++++ 2 files changed, 200 insertions(+), 7 deletions(-) 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 index 01c97e90b58..50e337dc352 100644 --- 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 @@ -28,11 +28,11 @@ import ( func TestRestoreObjectMeta(t *testing.T) { tests := []struct { - name string - original map[string]interface{} - converted map[string]interface{} - expected map[string]interface{} - wantErr bool + name string + original map[string]interface{} + converted map[string]interface{} + expected map[string]interface{} + expectedError bool }{ {"no converted metadata", map[string]interface{}{"metadata": map[string]interface{}{}, "spec": map[string]interface{}{}}, @@ -187,9 +187,9 @@ func TestRestoreObjectMeta(t *testing.T) { } 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 { + if err := restoreObjectMeta(&unstructured.Unstructured{Object: tt.original}, &unstructured.Unstructured{Object: tt.converted}); err == nil && tt.expectedError { t.Fatalf("expected error, but didn't get one") - } else if err != nil && !tt.wantErr { + } else if err != nil && !tt.expectedError { t.Fatalf("unexpected error: %v", err) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go index 192f0b9aada..e0c77cb8b06 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go @@ -82,6 +82,16 @@ func testWebhookConverter(t *testing.T, pruning bool) { handler: NewObjectConverterWebhookHandler(t, nontrivialConverter), checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1", "v1beta2"), validateNonTrivialConverted, validateNonTrivialConvertedList, validateStoragePruning), }, + { + group: "metadata-mutating-converter", + handler: NewObjectConverterWebhookHandler(t, metadataMutatingConverter), + checks: checks(validateObjectMetaMutation), + }, + { + group: "metadata-uid-mutating-converter", + handler: NewObjectConverterWebhookHandler(t, uidMutatingConverter), + checks: checks(validateUIDMutation), + }, { group: "empty-response", handler: NewReviewWebhookHandler(t, emptyResponseConverter), @@ -408,6 +418,108 @@ func validateStoragePruning(t *testing.T, ctc *conversionTestContext) { } } +func validateObjectMetaMutation(t *testing.T, ctc *conversionTestContext) { + ns := ctc.namespace + + t.Logf("Creating object in storage version v1beta1") + storageVersion := "v1beta1" + ctc.setAndWaitStorageVersion(t, storageVersion) + name := "objectmeta-mutation-" + storageVersion + client := ctc.versionedClient(ns, storageVersion) + obj, err := client.Create(newConversionMultiVersionFixture(ns, name, storageVersion), metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } + validateObjectMetaMutationObject(t, false, false, obj) + + t.Logf("Getting object in other version v1beta2") + client = ctc.versionedClient(ns, "v1beta2") + obj, err = client.Get(name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + validateObjectMetaMutationObject(t, true, true, obj) + + t.Logf("Creating object in non-storage version") + name = "objectmeta-mutation-v1beta2" + client = ctc.versionedClient(ns, "v1beta2") + obj, err = client.Create(newConversionMultiVersionFixture(ns, name, "v1beta2"), metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } + validateObjectMetaMutationObject(t, true, true, obj) + + t.Logf("Listing objects in non-storage version") + client = ctc.versionedClient(ns, "v1beta2") + list, err := client.List(metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + for _, obj := range list.Items { + validateObjectMetaMutationObject(t, true, true, &obj) + } +} + +func validateObjectMetaMutationObject(t *testing.T, expectAnnotations, expectLabels bool, obj *unstructured.Unstructured) { + if expectAnnotations { + if _, found := obj.GetAnnotations()["from"]; !found { + t.Errorf("expected 'from=stable.example.com/v1beta1' annotation") + } + if _, found := obj.GetAnnotations()["to"]; !found { + t.Errorf("expected 'to=stable.example.com/v1beta2' annotation") + } + } else { + if v, found := obj.GetAnnotations()["from"]; found { + t.Errorf("unexpected 'from' annotation: %s", v) + } + if v, found := obj.GetAnnotations()["to"]; found { + t.Errorf("unexpected 'to' annotation: %s", v) + } + } + if expectLabels { + if _, found := obj.GetLabels()["from"]; !found { + t.Errorf("expected 'from=stable.example.com.v1beta1' label") + } + if _, found := obj.GetLabels()["to"]; !found { + t.Errorf("expected 'to=stable.example.com.v1beta2' label") + } + } else { + if v, found := obj.GetLabels()["from"]; found { + t.Errorf("unexpected 'from' label: %s", v) + } + if v, found := obj.GetLabels()["to"]; found { + t.Errorf("unexpected 'to' label: %s", v) + } + } + if sets.NewString(obj.GetFinalizers()...).Has("foo") { + t.Errorf("unexpected 'foo' finalizer") + } + if obj.GetGeneration() == 42 { + t.Errorf("unexpected generation 42") + } + if v, found, err := unstructured.NestedString(obj.Object, "metadata", "garbage"); err != nil { + t.Errorf("unexpected error accessing 'metadata.garbage': %v", err) + } else if found { + t.Errorf("unexpected 'metadata.garbage': %s", v) + } +} + +func validateUIDMutation(t *testing.T, ctc *conversionTestContext) { + ns := ctc.namespace + + t.Logf("Creating object in non-storage version v1beta1") + storageVersion := "v1beta1" + ctc.setAndWaitStorageVersion(t, storageVersion) + name := "uid-mutation-" + storageVersion + client := ctc.versionedClient(ns, "v1beta2") + obj, err := client.Create(newConversionMultiVersionFixture(ns, name, "v1beta2"), metav1.CreateOptions{}) + if err == nil { + t.Fatalf("expected creation error, but got: %v", obj) + } else if !strings.Contains(err.Error(), "must have the same UID") { + t.Errorf("expected 'must have the same UID' error message, but got: %v", err) + } +} + func expectConversionFailureMessage(id, message string) func(t *testing.T, ctc *conversionTestContext) { return func(t *testing.T, ctc *conversionTestContext) { ns := ctc.namespace @@ -533,6 +645,87 @@ func nontrivialConverter(desiredAPIVersion string, obj runtime.RawExtension) (ru return runtime.RawExtension{Raw: raw}, nil } +func metadataMutatingConverter(desiredAPIVersion string, obj runtime.RawExtension) (runtime.RawExtension, error) { + obj, err := nontrivialConverter(desiredAPIVersion, obj) + if err != nil { + return runtime.RawExtension{}, err + } + + u := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := json.Unmarshal(obj.Raw, u); err != nil { + return runtime.RawExtension{}, fmt.Errorf("failed to deserialize object: %s with error: %v", string(obj.Raw), err) + } + + // do not mutate the marker or the probe objects + if !strings.Contains(u.GetName(), "mutation") { + return obj, nil + } + + currentAPIVersion := u.GetAPIVersion() + + // mutate annotations. This should be persisted. + annotations := u.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations["from"] = currentAPIVersion + annotations["to"] = desiredAPIVersion + u.SetAnnotations(annotations) + + // mutate labels. This should be persisted. + labels := u.GetLabels() + if labels == nil { + labels = map[string]string{} + } + labels["from"] = strings.Replace(currentAPIVersion, "/", ".", 1) // replace / with . because label values do not allow / + labels["to"] = strings.Replace(desiredAPIVersion, "/", ".", 1) + u.SetLabels(labels) + + // mutate other fields. This should be ignored. + u.SetGeneration(42) + u.SetOwnerReferences([]metav1.OwnerReference{{ + APIVersion: "v1", + Kind: "Namespace", + Name: "default", + UID: "1234", + Controller: nil, + BlockOwnerDeletion: nil, + }}) + u.SetResourceVersion("42") + u.SetFinalizers([]string{"foo"}) + if err := unstructured.SetNestedField(u.Object, "foo", "metadata", "garbage"); err != nil { + return runtime.RawExtension{}, err + } + + raw, err := json.Marshal(u) + if err != nil { + return runtime.RawExtension{}, fmt.Errorf("failed to serialize object: %v with error: %v", u, err) + } + return runtime.RawExtension{Raw: raw}, nil +} + +func uidMutatingConverter(desiredAPIVersion string, obj runtime.RawExtension) (runtime.RawExtension, error) { + u := &unstructured.Unstructured{Object: map[string]interface{}{}} + if err := json.Unmarshal(obj.Raw, u); err != nil { + return runtime.RawExtension{}, fmt.Errorf("failed to deserialize object: %s with error: %v", string(obj.Raw), err) + } + + // do not mutate the marker or the probe objects + if strings.Contains(u.GetName(), "mutation") { + // mutate other fields. This should be ignored. + if err := unstructured.SetNestedField(u.Object, "42", "metadata", "uid"); err != nil { + return runtime.RawExtension{}, err + } + } + + u.Object["apiVersion"] = desiredAPIVersion + raw, err := json.Marshal(u) + if err != nil { + return runtime.RawExtension{}, fmt.Errorf("failed to serialize object: %v with error: %v", u, err) + } + return runtime.RawExtension{Raw: raw}, nil +} + func newConversionTestContext(t *testing.T, apiExtensionsClient clientset.Interface, dynamicClient dynamic.Interface, etcdObjectReader *storage.EtcdObjectReader, crd *apiextensionsv1beta1.CustomResourceDefinition) (func(), *conversionTestContext) { crd, err := fixtures.CreateNewCustomResourceDefinition(crd, apiExtensionsClient, dynamicClient) if err != nil {