From beb7d6cff1cb3d1f13182a132d07bab5919e8403 Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Tue, 18 Oct 2022 17:43:13 -0400 Subject: [PATCH] CRConverter: change to UnstructuredList Change the CRConverter interface to use *unstructured.UnstructuredList for input/output to simplify what converter implementers must implement. Signed-off-by: Andy Goldstein --- .../pkg/apiserver/conversion/converter.go | 223 +++++++++++++++-- .../apiserver/conversion/converter_test.go | 100 ++++++++ .../pkg/apiserver/conversion/metrics.go | 4 +- .../pkg/apiserver/conversion/nop_converter.go | 13 +- .../apiserver/conversion/webhook_converter.go | 234 ++---------------- .../conversion/webhook_converter_test.go | 90 ++----- 6 files changed, 349 insertions(+), 315 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go index 89600d1b6fb..06c26b3296d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go @@ -21,9 +21,12 @@ import ( autoscalingv1 "k8s.io/api/autoscaling/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apivalidation "k8s.io/apimachinery/pkg/api/validation" "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/validation/field" "k8s.io/apiserver/pkg/util/webhook" typedscheme "k8s.io/client-go/kubernetes/scheme" ) @@ -110,7 +113,7 @@ type CRConverter interface { // Convert converts in object to the given gvk and returns the converted object. // Note that the function may mutate in object and return it. A safe wrapper will make sure // a safe converter will be returned. - Convert(in runtime.Object, targetGVK schema.GroupVersion) (runtime.Object, error) + Convert(in *unstructured.UnstructuredList, targetGVK schema.GroupVersion) (*unstructured.UnstructuredList, error) } // delegatingCRConverter extends the delegate converter with generic CR conversion behaviour. The delegate will implement the @@ -173,13 +176,6 @@ func (c *delegatingCRConverter) Convert(in, out, context interface{}) error { // UnstructuredList with the request's GV, populates it from storage, then calls conversion to convert // the individual items. This function assumes it never gets a v1.List. func (c *delegatingCRConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) { - fromGVK := in.GetObjectKind().GroupVersionKind() - toGVK, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{fromGVK}) - if !ok { - // TODO: should this be a typed error? - return nil, fmt.Errorf("%v is unstructured and is not suitable for converting to %q", fromGVK.String(), target) - } - // Special-case typed scale conversion if this custom resource supports a scale endpoint if c.convertScale { if _, isInScale := in.(*autoscalingv1.Scale); isInScale { @@ -187,8 +183,28 @@ func (c *delegatingCRConverter) ConvertToVersion(in runtime.Object, target runti } } + fromGVK := in.GetObjectKind().GroupVersionKind() + toGVK, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{fromGVK}) + if !ok { + // TODO: should this be a typed error? + return nil, fmt.Errorf("%v is unstructured and is not suitable for converting to %q", fromGVK.String(), target) + } + + isList := false + var list *unstructured.UnstructuredList + switch t := in.(type) { + case *unstructured.Unstructured: + list = &unstructured.UnstructuredList{Items: []unstructured.Unstructured{*t}} + case *unstructured.UnstructuredList: + list = t + isList = true + default: + return nil, fmt.Errorf("unexpected type %T", in) + } + + desiredAPIVersion := toGVK.GroupVersion().String() if !c.validVersions[toGVK.GroupVersion()] { - return nil, fmt.Errorf("request to convert CR to an invalid group/version: %s", toGVK.GroupVersion().String()) + return nil, fmt.Errorf("request to convert CR to an invalid group/version: %s", desiredAPIVersion) } // Note that even if the request is for a list, the GV of the request UnstructuredList is what // is expected to convert to. As mentioned in the function's document, it is not expected to @@ -196,23 +212,101 @@ func (c *delegatingCRConverter) ConvertToVersion(in runtime.Object, target runti if !c.validVersions[fromGVK.GroupVersion()] { return nil, fmt.Errorf("request to convert CR from an invalid group/version: %s", fromGVK.GroupVersion().String()) } - // Check list item's apiVersion - if list, ok := in.(*unstructured.UnstructuredList); ok { - for i := range list.Items { - expectedGV := list.Items[i].GroupVersionKind().GroupVersion() - if !c.validVersions[expectedGV] { - return nil, fmt.Errorf("request to convert CR list failed, list index %d has invalid group/version: %s", i, expectedGV.String()) - } + + var objectsToConvert []*unstructured.Unstructured + objectsToConvert, err := getObjectsToConvert(list, desiredAPIVersion, c.validVersions) + if err != nil { + return nil, err + } + + objCount := len(objectsToConvert) + if objCount == 0 { + // no objects needed conversion + if !isList { + // for a single item, return as-is + return in, nil } + // for a list, set the version of the top-level list object (all individual objects are already in the correct version) + out := list.DeepCopy() + out.SetAPIVersion(desiredAPIVersion) + return out, nil } // A smoke test in API machinery calls the converter on empty objects during startup. The test is initiated here: // https://github.com/kubernetes/kubernetes/blob/dbb448bbdcb9e440eee57024ffa5f1698956a054/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go#L201 if isEmptyUnstructuredObject(in) { - return NewNOPConverter().Convert(in, toGVK.GroupVersion()) + converted, err := NewNOPConverter().Convert(list, toGVK.GroupVersion()) + if err != nil { + return nil, err + } + if !isList { + return &converted.Items[0], nil + } + return converted, nil } - - return c.converter.Convert(in, toGVK.GroupVersion()) + + convertedObjects, err := c.converter.Convert(list, toGVK.GroupVersion()) + if err != nil { + return nil, fmt.Errorf("conversion for %v failed: %w", in.GetObjectKind().GroupVersionKind(), err) + } + if len(convertedObjects.Items) != len(objectsToConvert) { + return nil, fmt.Errorf("conversion for %v returned %d objects, expected %d", in.GetObjectKind().GroupVersionKind(), len(convertedObjects.Items), len(objectsToConvert)) + } + + // 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 := list.DeepCopy() + convertedList.SetAPIVersion(desiredAPIVersion) + convertedIndex := 0 + for i := range convertedList.Items { + original := &convertedList.Items[i] + if original.GetAPIVersion() == desiredAPIVersion { + // 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 := &convertedObjects.Items[convertedIndex] + convertedIndex++ + if expected, got := toGVK.GroupVersion(), converted.GetObjectKind().GroupVersionKind().GroupVersion(); expected != got { + return nil, fmt.Errorf("conversion for %v returned invalid converted object at index %v: invalid groupVersion (expected %v, received %v)", in.GetObjectKind().GroupVersionKind(), convertedIndex, expected, got) + } + if expected, got := original.GetObjectKind().GroupVersionKind().Kind, converted.GetObjectKind().GroupVersionKind().Kind; expected != got { + return nil, fmt.Errorf("conversion for %v returned invalid converted object at index %v: invalid kind (expected %v, received %v)", in.GetObjectKind().GroupVersionKind(), convertedIndex, expected, got) + } + if err := validateConvertedObject(original, converted); err != nil { + return nil, fmt.Errorf("conversion for %v returned invalid converted object at index %v: %v", in.GetObjectKind().GroupVersionKind(), convertedIndex, err) + } + if err := restoreObjectMeta(original, converted); err != nil { + return nil, fmt.Errorf("conversion for %v returned invalid metadata in object at index %v: %v", in.GetObjectKind().GroupVersionKind(), convertedIndex, err) + } + convertedList.Items[i] = *converted + } + + if isList { + return convertedList, nil + } + + return &convertedList.Items[0], nil +} + +func getObjectsToConvert( + list *unstructured.UnstructuredList, + desiredAPIVersion string, + validVersions map[schema.GroupVersion]bool, +) ([]*unstructured.Unstructured, error) { + var objectsToConvert []*unstructured.Unstructured + for i := range list.Items { + expectedGV := list.Items[i].GroupVersionKind().GroupVersion() + if !validVersions[expectedGV] { + return nil, fmt.Errorf("request to convert CR list failed, list index %d has invalid group/version: %s", i, expectedGV.String()) + } + + // Only sent item for conversion, if the apiVersion is different + if list.Items[i].GetAPIVersion() != desiredAPIVersion { + objectsToConvert = append(objectsToConvert, &list.Items[i]) + } + } + return objectsToConvert, nil } // safeConverterWrapper is a wrapper over an unsafe object converter that makes copy of the input and then delegate to the unsafe converter. @@ -259,3 +353,94 @@ func isEmptyUnstructuredObject(in runtime.Object) bool { } return true } + +// 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 := in.GetName(), out.GetName(); e != a { + return fmt.Errorf("must have the same name: %v != %v", 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 := 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 +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go index 65299c11493..8c60cb7f0da 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter_test.go @@ -21,6 +21,7 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -188,3 +189,102 @@ func TestConversion(t *testing.T) { } } } + +func TestGetObjectsToConvert(t *testing.T) { + v1Object := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v1", "kind": "Widget", "metadata": map[string]interface{}{"name": "myv1"}}} + v2Object := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v2", "kind": "Widget", "metadata": map[string]interface{}{"name": "myv2"}}} + v3Object := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v3", "kind": "Widget", "metadata": map[string]interface{}{"name": "myv3"}}} + + testcases := []struct { + Name string + List *unstructured.UnstructuredList + APIVersion string + ValidVersions map[schema.GroupVersion]bool + + ExpectObjects []*unstructured.Unstructured + ExpectError bool + }{ + { + Name: "empty list", + List: &unstructured.UnstructuredList{}, + APIVersion: "foo/v1", + ValidVersions: map[schema.GroupVersion]bool{ + {Group: "foo", Version: "v1"}: true, + }, + ExpectObjects: nil, + }, + { + Name: "one-item list, in desired version", + List: &unstructured.UnstructuredList{ + Items: []unstructured.Unstructured{*v1Object}, + }, + ValidVersions: map[schema.GroupVersion]bool{ + {Group: "foo", Version: "v1"}: true, + }, + APIVersion: "foo/v1", + ExpectObjects: nil, + }, + { + Name: "one-item list, not in desired version", + List: &unstructured.UnstructuredList{ + Items: []unstructured.Unstructured{*v2Object}, + }, + ValidVersions: map[schema.GroupVersion]bool{ + {Group: "foo", Version: "v1"}: true, + {Group: "foo", Version: "v2"}: true, + }, + APIVersion: "foo/v1", + ExpectObjects: []*unstructured.Unstructured{v2Object}, + }, + { + Name: "multi-item list, in desired version", + List: &unstructured.UnstructuredList{ + Items: []unstructured.Unstructured{*v1Object, *v1Object, *v1Object}, + }, + ValidVersions: map[schema.GroupVersion]bool{ + {Group: "foo", Version: "v1"}: true, + {Group: "foo", Version: "v2"}: true, + }, + APIVersion: "foo/v1", + ExpectObjects: nil, + }, + { + Name: "multi-item list, mixed versions", + List: &unstructured.UnstructuredList{ + Items: []unstructured.Unstructured{*v1Object, *v2Object, *v3Object}, + }, + ValidVersions: map[schema.GroupVersion]bool{ + {Group: "foo", Version: "v1"}: true, + {Group: "foo", Version: "v2"}: true, + {Group: "foo", Version: "v3"}: true, + }, + APIVersion: "foo/v1", + ExpectObjects: []*unstructured.Unstructured{v2Object, v3Object}, + }, + { + Name: "multi-item list, invalid versions", + List: &unstructured.UnstructuredList{ + Items: []unstructured.Unstructured{*v1Object, *v2Object, *v3Object}, + }, + ValidVersions: map[schema.GroupVersion]bool{ + {Group: "foo", Version: "v2"}: true, + {Group: "foo", Version: "v3"}: true, + }, + APIVersion: "foo/v1", + ExpectObjects: nil, + ExpectError: true, + }, + } + for _, tc := range testcases { + t.Run(tc.Name, func(t *testing.T) { + objects, err := getObjectsToConvert(tc.List, tc.APIVersion, tc.ValidVersions) + gotError := err != nil + if e, a := tc.ExpectError, gotError; e != a { + t.Fatalf("error: expected %t, got %t", e, a) + } + if !reflect.DeepEqual(objects, tc.ExpectObjects) { + t.Errorf("unexpected diff: %s", cmp.Diff(tc.ExpectObjects, objects)) + } + }) + } +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/metrics.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/metrics.go index e4c6657af60..b1f68723576 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/metrics.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/metrics.go @@ -21,7 +21,7 @@ import ( "sync" "time" - "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/component-base/metrics" "k8s.io/component-base/metrics/legacyregistry" @@ -73,7 +73,7 @@ func (c *converterMetricFactory) addMetrics(crdName string, converter CRConverte return &converterMetric{latencies: metric, delegate: converter, crdName: crdName}, nil } -func (m *converterMetric) Convert(in runtime.Object, targetGV schema.GroupVersion) (runtime.Object, error) { +func (m *converterMetric) Convert(in *unstructured.UnstructuredList, targetGV schema.GroupVersion) (*unstructured.UnstructuredList, error) { start := time.Now() obj, err := m.delegate.Convert(in, targetGV) fromVersion := in.GetObjectKind().GroupVersionKind().Version diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go index 9b2a04e3606..42a27ec1c10 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/nop_converter.go @@ -18,7 +18,6 @@ package conversion import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" ) @@ -35,13 +34,9 @@ func NewNOPConverter() *nopConverter { var _ CRConverter = &nopConverter{} // ConvertToVersion converts in object to the given gv in place and returns the same `in` object. -func (c *nopConverter) Convert(in runtime.Object, targetGV schema.GroupVersion) (runtime.Object, error) { - // Run the converter on the list items instead of list itself - if list, ok := in.(*unstructured.UnstructuredList); ok { - for i := range list.Items { - list.Items[i].SetGroupVersionKind(targetGV.WithKind(list.Items[i].GroupVersionKind().Kind)) - } +func (c *nopConverter) Convert(list *unstructured.UnstructuredList, targetGV schema.GroupVersion) (*unstructured.UnstructuredList, error) { + for i := range list.Items { + list.Items[i].SetGroupVersionKind(targetGV.WithKind(list.Items[i].GroupVersionKind().Kind)) } - in.GetObjectKind().SetGroupVersionKind(targetGV.WithKind(in.GetObjectKind().GroupVersionKind().Kind)) - return in, nil + return list, nil } 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 f89cf448de6..8cdfd065f31 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 @@ -26,15 +26,12 @@ import ( v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" - apivalidation "k8s.io/apimachinery/pkg/api/validation" metav1 "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/types" "k8s.io/apimachinery/pkg/util/uuid" - "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/util/webhook" "k8s.io/client-go/rest" "k8s.io/component-base/tracing" @@ -113,36 +110,21 @@ func (f *webhookConverterFactory) NewWebhookConverter(crd *v1.CustomResourceDefi }, nil } -// getObjectsToConvert returns a list of objects requiring conversion. -// if obj is a list, getObjectsToConvert returns a (potentially empty) list of the items that are not already in the desired version. -// if obj is not a list, and is already in the desired version, getObjectsToConvert returns an empty list. -// if obj is not a list, and is not already in the desired version, getObjectsToConvert returns a list containing only obj. -func getObjectsToConvert(obj runtime.Object, apiVersion string) []runtime.RawExtension { - listObj, isList := obj.(*unstructured.UnstructuredList) - var objects []runtime.RawExtension - if isList { - 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]}) - } - } - } else { - if obj.GetObjectKind().GroupVersionKind().GroupVersion().String() != apiVersion { - objects = []runtime.RawExtension{{Object: obj}} +// createConversionReviewObjects returns ConversionReview request and response objects for the first supported version found in conversionReviewVersions. +func createConversionReviewObjects(conversionReviewVersions []string, objects *unstructured.UnstructuredList, apiVersion string, requestUID types.UID) (request, response runtime.Object, err error) { + rawObjects := make([]runtime.RawExtension, len(objects.Items)) + for i := range objects.Items { + rawObjects[i] = runtime.RawExtension{ + Object: &objects.Items[i], } } - return objects -} -// createConversionReviewObjects returns ConversionReview request and response objects for the first supported version found in conversionReviewVersions. -func createConversionReviewObjects(conversionReviewVersions []string, objects []runtime.RawExtension, apiVersion string, requestUID types.UID) (request, response runtime.Object, err error) { for _, version := range conversionReviewVersions { switch version { case v1beta1.SchemeGroupVersion.Version: return &v1beta1.ConversionReview{ Request: &v1beta1.ConversionRequest{ - Objects: objects, + Objects: rawObjects, DesiredAPIVersion: apiVersion, UID: requestUID, }, @@ -151,7 +133,7 @@ func createConversionReviewObjects(conversionReviewVersions []string, objects [] case v1.SchemeGroupVersion.Version: return &v1.ConversionReview{ Request: &v1.ConversionRequest{ - Objects: objects, + Objects: rawObjects, DesiredAPIVersion: apiVersion, UID: requestUID, }, @@ -162,9 +144,13 @@ func createConversionReviewObjects(conversionReviewVersions []string, objects [] return nil, nil, fmt.Errorf("no supported conversion review versions") } -func getRawExtensionObject(rx runtime.RawExtension) (runtime.Object, error) { +func getRawExtensionObject(rx runtime.RawExtension) (*unstructured.Unstructured, error) { if rx.Object != nil { - return rx.Object, nil + u, ok := rx.Object.(*unstructured.Unstructured) + if !ok { + return nil, fmt.Errorf("unexpected type %T", rx.Object) + } + return u, nil } u := unstructured.Unstructured{} err := u.UnmarshalJSON(rx.Raw) @@ -227,30 +213,16 @@ func getConvertedObjectsFromResponse(expectedUID types.UID, response runtime.Obj } } -func (c *webhookConverter) Convert(in runtime.Object, toGV schema.GroupVersion) (runtime.Object, error) { +func (c *webhookConverter) Convert(in *unstructured.UnstructuredList, toGV schema.GroupVersion) (*unstructured.UnstructuredList, error) { ctx := context.TODO() - listObj, isList := in.(*unstructured.UnstructuredList) - requestUID := uuid.NewUUID() desiredAPIVersion := toGV.String() - objectsToConvert := getObjectsToConvert(in, desiredAPIVersion) - request, response, err := createConversionReviewObjects(c.conversionReviewVersions, objectsToConvert, desiredAPIVersion, requestUID) + request, response, err := createConversionReviewObjects(c.conversionReviewVersions, in, desiredAPIVersion, requestUID) if err != nil { return nil, err } - objCount := len(objectsToConvert) - if objCount == 0 { - // no objects needed conversion - if !isList { - // for a single item, return as-is - return in, nil - } - // for a list, set the version of the top-level list object (all individual objects are already in the correct version) - out := listObj.DeepCopy() - out.SetAPIVersion(toGV.String()) - return out, nil - } + objCount := len(in.Items) ctx, span := tracing.Start(ctx, "Call conversion webhook", attribute.String("custom-resource-definition", c.name), @@ -275,170 +247,14 @@ func (c *webhookConverter) Convert(in runtime.Object, toGV schema.GroupVersion) return nil, fmt.Errorf("conversion webhook for %v failed: %v", in.GetObjectKind().GroupVersionKind(), err) } - if len(convertedObjects) != len(objectsToConvert) { - return nil, fmt.Errorf("conversion webhook for %v returned %d objects, expected %d", in.GetObjectKind().GroupVersionKind(), len(convertedObjects), len(objectsToConvert)) - } - - 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() - convertedIndex := 0 - 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(convertedObjects[convertedIndex]) - if err != nil { - return nil, fmt.Errorf("conversion webhook for %v returned invalid converted object at index %v: %v", in.GetObjectKind().GroupVersionKind(), convertedIndex, err) - } - convertedIndex++ - if expected, got := toGV, converted.GetObjectKind().GroupVersionKind().GroupVersion(); expected != got { - return nil, fmt.Errorf("conversion webhook for %v returned invalid converted object at index %v: invalid groupVersion (expected %v, received %v)", in.GetObjectKind().GroupVersionKind(), convertedIndex, expected, got) - } - if expected, got := original.GetObjectKind().GroupVersionKind().Kind, converted.GetObjectKind().GroupVersionKind().Kind; expected != got { - return nil, fmt.Errorf("conversion webhook for %v returned invalid converted object at index %v: invalid kind (expected %v, received %v)", in.GetObjectKind().GroupVersionKind(), convertedIndex, expected, got) - } - unstructConverted, ok := converted.(*unstructured.Unstructured) - if !ok { - // this should not happened - return nil, fmt.Errorf("conversion webhook for %v returned invalid converted object at index %v: invalid type, expected=Unstructured, got=%T", in.GetObjectKind().GroupVersionKind(), convertedIndex, converted) - } - if err := validateConvertedObject(original, unstructConverted); err != nil { - return nil, fmt.Errorf("conversion webhook for %v returned invalid converted object at index %v: %v", in.GetObjectKind().GroupVersionKind(), convertedIndex, err) - } - if err := restoreObjectMeta(original, unstructConverted); err != nil { - return nil, fmt.Errorf("conversion webhook for %v returned invalid metadata in object at index %v: %v", in.GetObjectKind().GroupVersionKind(), convertedIndex, err) - } - convertedList.Items[i] = *unstructConverted + out := &unstructured.UnstructuredList{} + out.Items = make([]unstructured.Unstructured, len(convertedObjects)) + for i := range convertedObjects { + u, err := getRawExtensionObject(convertedObjects[i]) + if err != nil { + return nil, err } - convertedList.SetAPIVersion(toGV.String()) - return convertedList, nil + out.Items[i] = *u } - - if len(convertedObjects) != 1 { - // This should not happened - return nil, fmt.Errorf("conversion webhook for %v failed, no objects returned", in.GetObjectKind()) - } - converted, err := getRawExtensionObject(convertedObjects[0]) - if err != nil { - return nil, err - } - if e, a := toGV, converted.GetObjectKind().GroupVersionKind().GroupVersion(); e != a { - return nil, fmt.Errorf("conversion webhook for %v returned invalid object at index 0: invalid groupVersion (expected %v, received %v)", in.GetObjectKind().GroupVersionKind(), e, a) - } - if e, a := in.GetObjectKind().GroupVersionKind().Kind, converted.GetObjectKind().GroupVersionKind().Kind; e != a { - return nil, fmt.Errorf("conversion webhook for %v returned invalid object at index 0: invalid kind (expected %v, received %v)", in.GetObjectKind().GroupVersionKind(), e, a) - } - unstructConverted, ok := converted.(*unstructured.Unstructured) - if !ok { - // this should not happened - return nil, fmt.Errorf("conversion webhook for %v failed, unexpected type %T at index 0", in.GetObjectKind().GroupVersionKind(), converted) - } - unstructIn, ok := in.(*unstructured.Unstructured) - if !ok { - // this should not happened - return nil, fmt.Errorf("conversion webhook for %v failed unexpected input type %T", in.GetObjectKind().GroupVersionKind(), in) - } - if err := validateConvertedObject(unstructIn, unstructConverted); err != nil { - return nil, fmt.Errorf("conversion webhook for %v returned invalid object: %v", in.GetObjectKind().GroupVersionKind(), err) - } - if err := restoreObjectMeta(unstructIn, unstructConverted); err != nil { - return nil, fmt.Errorf("conversion webhook for %v returned invalid metadata: %v", in.GetObjectKind().GroupVersionKind(), err) - } - return converted, nil -} - -// 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 := in.GetName(), out.GetName(); e != a { - return fmt.Errorf("must have the same name: %v != %v", 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 := 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 + return out, nil } 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 5c6766c09c1..c53c657a803 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 @@ -206,81 +206,19 @@ func TestRestoreObjectMeta(t *testing.T) { } } -func TestGetObjectsToConvert(t *testing.T) { - v1Object := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v1", "kind": "Widget", "metadata": map[string]interface{}{"name": "myv1"}}} - v2Object := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v2", "kind": "Widget", "metadata": map[string]interface{}{"name": "myv2"}}} - v3Object := &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v3", "kind": "Widget", "metadata": map[string]interface{}{"name": "myv3"}}} - - testcases := []struct { - Name string - Object runtime.Object - APIVersion string - - ExpectObjects []runtime.RawExtension - }{ - { - Name: "empty list", - Object: &unstructured.UnstructuredList{}, - APIVersion: "foo/v1", - ExpectObjects: nil, - }, - { - Name: "one-item list, in desired version", - Object: &unstructured.UnstructuredList{ - Items: []unstructured.Unstructured{*v1Object}, - }, - APIVersion: "foo/v1", - ExpectObjects: nil, - }, - { - Name: "one-item list, not in desired version", - Object: &unstructured.UnstructuredList{ - Items: []unstructured.Unstructured{*v2Object}, - }, - APIVersion: "foo/v1", - ExpectObjects: []runtime.RawExtension{{Object: v2Object}}, - }, - { - Name: "multi-item list, in desired version", - Object: &unstructured.UnstructuredList{ - Items: []unstructured.Unstructured{*v1Object, *v1Object, *v1Object}, - }, - APIVersion: "foo/v1", - ExpectObjects: nil, - }, - { - Name: "multi-item list, mixed versions", - Object: &unstructured.UnstructuredList{ - Items: []unstructured.Unstructured{*v1Object, *v2Object, *v3Object}, - }, - APIVersion: "foo/v1", - ExpectObjects: []runtime.RawExtension{{Object: v2Object}, {Object: v3Object}}, - }, - { - Name: "single item, in desired version", - Object: v1Object, - APIVersion: "foo/v1", - ExpectObjects: nil, - }, - { - Name: "single item, not in desired version", - Object: v2Object, - APIVersion: "foo/v1", - ExpectObjects: []runtime.RawExtension{{Object: v2Object}}, - }, - } - for _, tc := range testcases { - t.Run(tc.Name, func(t *testing.T) { - if objects := getObjectsToConvert(tc.Object, tc.APIVersion); !reflect.DeepEqual(objects, tc.ExpectObjects) { - t.Errorf("unexpected diff: %s", cmp.Diff(tc.ExpectObjects, objects)) - } - }) - } -} - func TestCreateConversionReviewObjects(t *testing.T) { - objects := []runtime.RawExtension{ - {Object: &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "foo/v2", "Kind": "Widget"}}}, + objects := &unstructured.UnstructuredList{ + Items: []unstructured.Unstructured{ + { + Object: map[string]interface{}{"apiVersion": "foo/v2", "Kind": "Widget"}, + }, + }, + } + + rawObjects := []runtime.RawExtension{ + { + Object: &objects.Items[0], + }, } testcases := []struct { @@ -300,7 +238,7 @@ func TestCreateConversionReviewObjects(t *testing.T) { Name: "v1", Versions: []string{"v1", "v1beta1", "v2"}, ExpectRequest: &v1.ConversionReview{ - Request: &v1.ConversionRequest{UID: "uid", DesiredAPIVersion: "foo/v1", Objects: objects}, + Request: &v1.ConversionRequest{UID: "uid", DesiredAPIVersion: "foo/v1", Objects: rawObjects}, Response: &v1.ConversionResponse{}, }, ExpectResponse: &v1.ConversionReview{}, @@ -309,7 +247,7 @@ func TestCreateConversionReviewObjects(t *testing.T) { Name: "v1beta1", Versions: []string{"v1beta1", "v1", "v2"}, ExpectRequest: &v1beta1.ConversionReview{ - Request: &v1beta1.ConversionRequest{UID: "uid", DesiredAPIVersion: "foo/v1", Objects: objects}, + Request: &v1beta1.ConversionRequest{UID: "uid", DesiredAPIVersion: "foo/v1", Objects: rawObjects}, Response: &v1beta1.ConversionResponse{}, }, ExpectResponse: &v1beta1.ConversionReview{},