From 932553a08c5142fc7751555697e28bdc1f2eff09 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 4 Jun 2019 22:12:34 -0400 Subject: [PATCH 1/2] Set expected in-memory version when decoding unstructured objects from etcd --- .../pkg/registry/customresource/etcd.go | 11 ++++-- .../integration/conversion/conversion_test.go | 35 ++++++++++++++++--- .../apis/meta/v1/unstructured/unstructured.go | 10 ++++++ .../meta/v1/unstructured/unstructured_list.go | 10 ++++++ .../apimachinery/pkg/runtime/interfaces.go | 3 ++ .../apimachinery/pkg/runtime/testing/types.go | 8 +++++ .../k8s.io/apiserver/pkg/storage/etcd3/BUILD | 1 + .../apiserver/pkg/storage/etcd3/store.go | 8 ++++- .../apiserver/pkg/storage/etcd3/store_test.go | 7 +++- 9 files changed, 84 insertions(+), 9 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go index bf0d02f1941..4c5165c69b7 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go @@ -177,12 +177,19 @@ type StatusREST struct { var _ = rest.Patcher(&StatusREST{}) func (r *StatusREST) New() runtime.Object { - return &unstructured.Unstructured{} + return r.store.New() } // Get retrieves the object from the storage. It is required to support Patch. func (r *StatusREST) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { - return r.store.Get(ctx, name, options) + o, err := r.store.Get(ctx, name, options) + if err != nil { + return nil, err + } + if u, ok := o.(*unstructured.Unstructured); ok { + shallowCopyObjectMeta(u) + } + return o, nil } // Update alters the status subset of an object. 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 2e3d8450879..92c26a9e44f 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 @@ -58,15 +58,18 @@ func checks(checkers ...Checker) []Checker { return checkers } -func TestWebhookConverter(t *testing.T) { - testWebhookConverter(t, false) +func TestWebhookConverterWithWatchCache(t *testing.T) { + testWebhookConverter(t, false, true) +} +func TestWebhookConverterWithoutWatchCache(t *testing.T) { + testWebhookConverter(t, false, false) } func TestWebhookConverterWithDefaulting(t *testing.T) { - testWebhookConverter(t, true) + testWebhookConverter(t, true, true) } -func testWebhookConverter(t *testing.T, defaulting bool) { +func testWebhookConverter(t *testing.T, defaulting, watchCache bool) { tests := []struct { group string handler http.Handler @@ -115,7 +118,7 @@ func testWebhookConverter(t *testing.T, defaulting bool) { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CustomResourceDefaulting, true)() } - tearDown, config, options, err := fixtures.StartDefaultServer(t) + tearDown, config, options, err := fixtures.StartDefaultServer(t, fmt.Sprintf("--watch-cache=%v", watchCache)) if err != nil { t.Fatal(err) } @@ -320,6 +323,28 @@ func validateNonTrivialConverted(t *testing.T, ctc *conversionTestContext) { } verifyMultiVersionObject(t, getVersion.Name, obj) } + + // send a non-trivial patch to the main resource to verify the oldObject is in the right version + if _, err := client.Patch(name, types.MergePatchType, []byte(`{"metadata":{"annotations":{"main":"true"}}}`), metav1.PatchOptions{}); err != nil { + t.Fatal(err) + } + // verify that the right, pruned version is in storage + obj, err = ctc.etcdObjectReader.GetStoredCustomResource(ns, name) + if err != nil { + t.Fatal(err) + } + verifyMultiVersionObject(t, "v1beta1", obj) + + // send a non-trivial patch to the status subresource to verify the oldObject is in the right version + if _, err := client.Patch(name, types.MergePatchType, []byte(`{"metadata":{"annotations":{"status":"true"}}}`), metav1.PatchOptions{}, "status"); err != nil { + t.Fatal(err) + } + // verify that the right, pruned version is in storage + obj, err = ctc.etcdObjectReader.GetStoredCustomResource(ns, name) + if err != nil { + t.Fatal(err) + } + verifyMultiVersionObject(t, "v1beta1", obj) }) } } diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go index 8b35a21cbcb..0ba18d45d8d 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go @@ -127,6 +127,16 @@ func (u *Unstructured) UnmarshalJSON(b []byte) error { return err } +// NewEmptyInstance returns a new instance of the concrete type containing only kind/apiVersion and no other data. +// This should be called instead of reflect.New() for unstructured types because the go type alone does not preserve kind/apiVersion info. +func (in *Unstructured) NewEmptyInstance() runtime.Unstructured { + out := new(Unstructured) + if in != nil { + out.GetObjectKind().SetGroupVersionKind(in.GetObjectKind().GroupVersionKind()) + } + return out +} + func (in *Unstructured) DeepCopy() *Unstructured { if in == nil { return nil diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go index 44d897c377d..5028f5fb57a 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_list.go @@ -52,6 +52,16 @@ func (u *UnstructuredList) EachListItem(fn func(runtime.Object) error) error { return nil } +// NewEmptyInstance returns a new instance of the concrete type containing only kind/apiVersion and no other data. +// This should be called instead of reflect.New() for unstructured types because the go type alone does not preserve kind/apiVersion info. +func (u *UnstructuredList) NewEmptyInstance() runtime.Unstructured { + out := new(UnstructuredList) + if u != nil { + out.SetGroupVersionKind(u.GroupVersionKind()) + } + return out +} + // UnstructuredContent returns a map contain an overlay of the Items field onto // the Object field. Items always overwrites overlay. func (u *UnstructuredList) UnstructuredContent() map[string]interface{} { diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go b/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go index f263d961d65..bded5bf1591 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go @@ -260,6 +260,9 @@ type Object interface { // to JSON allowed. type Unstructured interface { Object + // NewEmptyInstance returns a new instance of the concrete type containing only kind/apiVersion and no other data. + // This should be called instead of reflect.New() for unstructured types because the go type alone does not preserve kind/apiVersion info. + NewEmptyInstance() Unstructured // UnstructuredContent returns a non-nil map with this object's contents. Values may be // []interface{}, map[string]interface{}, or any primitive type. Contents are typically serialized to // and from JSON. SetUnstructuredContent should be used to mutate the contents. diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/testing/types.go b/staging/src/k8s.io/apimachinery/pkg/runtime/testing/types.go index b8d67061d4b..7f93631a3ab 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/testing/types.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/testing/types.go @@ -261,6 +261,14 @@ func (obj *Unstructured) EachListItem(fn func(runtime.Object) error) error { return nil } +func (obj *Unstructured) NewEmptyInstance() runtime.Unstructured { + out := new(Unstructured) + if obj != nil { + out.SetGroupVersionKind(obj.GroupVersionKind()) + } + return out +} + func (obj *Unstructured) UnstructuredContent() map[string]interface{} { if obj.Object == nil { return make(map[string]interface{}) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/BUILD b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/BUILD index f03ccf7060b..e8ffc593500 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/BUILD @@ -20,6 +20,7 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/api/apitesting:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/conversion:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/fields:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go index c9ca0a976be..8e26dad5ac1 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go @@ -685,9 +685,15 @@ func (s *store) watch(ctx context.Context, key string, rv string, pred storage.S func (s *store) getState(getResp *clientv3.GetResponse, key string, v reflect.Value, ignoreNotFound bool) (*objState, error) { state := &objState{ - obj: reflect.New(v.Type()).Interface().(runtime.Object), meta: &storage.ResponseMeta{}, } + + if u, ok := v.Addr().Interface().(runtime.Unstructured); ok { + state.obj = u.NewEmptyInstance() + } else { + state.obj = reflect.New(v.Type()).Interface().(runtime.Object) + } + if len(getResp.Kvs) == 0 { if !ignoreNotFound { return nil, storage.NewKeyNotFoundError(key, 0) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go index 72ed4227b2b..2109340cbc0 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go @@ -35,6 +35,7 @@ import ( apitesting "k8s.io/apimachinery/pkg/api/apitesting" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" @@ -1349,7 +1350,11 @@ func testSetup(t *testing.T) (context.Context, *store, *integration.ClusterV3) { func testPropogateStore(ctx context.Context, t *testing.T, store *store, obj *example.Pod) (string, *example.Pod) { // Setup store with a key and grab the output for returning. key := "/testkey" - err := store.conditionalDelete(ctx, key, &example.Pod{}, reflect.ValueOf(example.Pod{}), nil, storage.ValidateAllObjectFunc) + v, err := conversion.EnforcePtr(obj) + if err != nil { + panic("unable to convert output object to pointer") + } + err = store.conditionalDelete(ctx, key, &example.Pod{}, v, nil, storage.ValidateAllObjectFunc) if err != nil && !storage.IsNotFound(err) { t.Fatalf("Cleanup failed: %v", err) } From a3bb81ff327bc4daa7bc7c9819d9ab18343d69a7 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 5 Jun 2019 21:47:50 -0400 Subject: [PATCH 2/2] Ensure defaulting applies to custom resource items in list response --- pkg/printers/internalversion/printers_test.go | 2 + .../test/integration/defaulting_test.go | 230 ++++++++++++++++-- .../serializer/versioning/versioning.go | 15 +- 3 files changed, 215 insertions(+), 32 deletions(-) diff --git a/pkg/printers/internalversion/printers_test.go b/pkg/printers/internalversion/printers_test.go index 92fa1d7fbf2..9082a9a7cc4 100644 --- a/pkg/printers/internalversion/printers_test.go +++ b/pkg/printers/internalversion/printers_test.go @@ -235,6 +235,8 @@ func testPrinter(t *testing.T, printer printers.ResourcePrinter, unmarshalFunc f TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Pod"}, ObjectMeta: metav1.ObjectMeta{Name: "foo"}, } + // our decoder defaults, so we should default our expected object as well + legacyscheme.Scheme.Default(obj) buf.Reset() printer.PrintObj(obj, buf) var objOut v1.Pod diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go index b9c5749ab0c..9e3e8c43de2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/defaulting_test.go @@ -17,6 +17,7 @@ limitations under the License. package integration import ( + "fmt" "strings" "testing" "time" @@ -29,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/apimachinery/pkg/watch" utilfeature "k8s.io/apiserver/pkg/util/feature" utilfeaturetesting "k8s.io/component-base/featuregate/testing" "k8s.io/utils/pointer" @@ -43,6 +45,18 @@ var defaultingFixture = &apiextensionsv1beta1.CustomResourceDefinition{ Spec: apiextensionsv1beta1.CustomResourceDefinitionSpec{ Group: "tests.apiextensions.k8s.io", Version: "v1beta1", + Versions: []apiextensionsv1beta1.CustomResourceDefinitionVersion{ + { + Name: "v1beta1", + Storage: false, + Served: true, + }, + { + Name: "v1beta2", + Storage: true, + Served: false, + }, + }, Names: apiextensionsv1beta1.CustomResourceDefinitionNames{ Plural: "foos", Singular: "foo", @@ -57,7 +71,7 @@ var defaultingFixture = &apiextensionsv1beta1.CustomResourceDefinition{ }, } -const defaultingFooSchema = ` +const defaultingFooV1beta1Schema = ` type: object properties: spec: @@ -69,6 +83,13 @@ properties: b: type: string default: "B" + c: + type: string + v1beta1: + type: string + default: "v1beta1" + v1beta2: + type: string status: type: object properties: @@ -78,20 +99,76 @@ properties: b: type: string default: "B" + c: + type: string + v1beta1: + type: string + default: "v1beta1" + v1beta2: + type: string ` -func TestCustomResourceDefaulting(t *testing.T) { +const defaultingFooV1beta2Schema = ` +type: object +properties: + spec: + type: object + properties: + a: + type: string + default: "A" + b: + type: string + default: "B" + c: + type: string + v1beta1: + type: string + v1beta2: + type: string + default: "v1beta2" + status: + type: object + properties: + a: + type: string + default: "A" + b: + type: string + default: "B" + c: + type: string + v1beta1: + type: string + v1beta2: + type: string + default: "v1beta2" +` + +func TestCustomResourceDefaultingWithWatchCache(t *testing.T) { + testDefaulting(t, true) +} + +func TestCustomResourceDefaultingWithoutWatchCache(t *testing.T) { + testDefaulting(t, false) +} + +func testDefaulting(t *testing.T, watchCache bool) { defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CustomResourceDefaulting, true)() - tearDownFn, apiExtensionClient, dynamicClient, err := fixtures.StartDefaultServerWithClients(t) + tearDownFn, apiExtensionClient, dynamicClient, err := fixtures.StartDefaultServerWithClients(t, fmt.Sprintf("--watch-cache=%v", watchCache)) if err != nil { t.Fatal(err) } defer tearDownFn() crd := defaultingFixture.DeepCopy() - crd.Spec.Validation = &apiextensionsv1beta1.CustomResourceValidation{} - if err := yaml.Unmarshal([]byte(defaultingFooSchema), &crd.Spec.Validation.OpenAPIV3Schema); err != nil { + crd.Spec.Versions[0].Schema = &apiextensionsv1beta1.CustomResourceValidation{} + if err := yaml.Unmarshal([]byte(defaultingFooV1beta1Schema), &crd.Spec.Versions[0].Schema.OpenAPIV3Schema); err != nil { + t.Fatal(err) + } + crd.Spec.Versions[1].Schema = &apiextensionsv1beta1.CustomResourceValidation{} + if err := yaml.Unmarshal([]byte(defaultingFooV1beta2Schema), &crd.Spec.Versions[1].Schema.OpenAPIV3Schema); err != nil { t.Fatal(err) } @@ -101,13 +178,15 @@ func TestCustomResourceDefaulting(t *testing.T) { } mustExist := func(obj map[string]interface{}, pths [][]string) { + t.Helper() for _, pth := range pths { if _, found, _ := unstructured.NestedFieldNoCopy(obj, pth...); !found { - t.Errorf("Expected '%s' field exist", strings.Join(pth, ".")) + t.Errorf("Expected '%s' field was missing", strings.Join(pth, ".")) } } } mustNotExist := func(obj map[string]interface{}, pths [][]string) { + t.Helper() for _, pth := range pths { if fld, found, _ := unstructured.NestedFieldNoCopy(obj, pth...); found { t.Errorf("Expected '%s' field to not exist, but it does: %v", strings.Join(pth, "."), fld) @@ -115,6 +194,7 @@ func TestCustomResourceDefaulting(t *testing.T) { } } updateCRD := func(update func(*apiextensionsv1beta1.CustomResourceDefinition)) { + t.Helper() var err error for retry := 0; retry < 10; retry++ { var obj *apiextensionsv1beta1.CustomResourceDefinition @@ -136,22 +216,34 @@ func TestCustomResourceDefaulting(t *testing.T) { t.Fatal(err) } } - addDefault := func(key string, value interface{}) { + addDefault := func(version string, key string, value interface{}) { + t.Helper() updateCRD(func(obj *apiextensionsv1beta1.CustomResourceDefinition) { for _, root := range []string{"spec", "status"} { - obj.Spec.Validation.OpenAPIV3Schema.Properties[root].Properties[key] = apiextensionsv1beta1.JSONSchemaProps{ - Type: "string", - Default: jsonPtr(value), + for i := range obj.Spec.Versions { + if obj.Spec.Versions[i].Name != version { + continue + } + obj.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties[root].Properties[key] = apiextensionsv1beta1.JSONSchemaProps{ + Type: "string", + Default: jsonPtr(value), + } } } }) } - removeDefault := func(key string) { + removeDefault := func(version string, key string) { + t.Helper() updateCRD(func(obj *apiextensionsv1beta1.CustomResourceDefinition) { for _, root := range []string{"spec", "status"} { - props := obj.Spec.Validation.OpenAPIV3Schema.Properties[root].Properties[key] - props.Default = nil - obj.Spec.Validation.OpenAPIV3Schema.Properties[root].Properties[key] = props + for i := range obj.Spec.Versions { + if obj.Spec.Versions[i].Name != version { + continue + } + props := obj.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties[root].Properties[key] + props.Default = nil + obj.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties[root].Properties[key] = props + } } }) } @@ -168,8 +260,12 @@ func TestCustomResourceDefaulting(t *testing.T) { if err != nil { t.Fatalf("Unable to create CR: %v", err) } + initialResourceVersion := foo.GetResourceVersion() t.Logf("CR created: %#v", foo.UnstructuredContent()) - mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}}) + // spec.a and spec.b are defaulted in both versions + // spec.v1beta1 is defaulted when reading the incoming request + // spec.v1beta2 is defaulted when reading the storage response + mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "v1beta1"}, {"spec", "v1beta2"}}) mustNotExist(foo.Object, [][]string{{"status"}}) t.Logf("Updating status and expecting 'a' and 'b' to show up.") @@ -179,8 +275,90 @@ func TestCustomResourceDefaulting(t *testing.T) { } mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"status", "a"}, {"status", "b"}}) - t.Logf("Add 'c' default and wait until GET sees it in both status and spec") - addDefault("c", "C") + t.Logf("Add 'c' default to the storage version and wait until GET sees it in both status and spec") + addDefault("v1beta2", "c", "C") + + t.Logf("wait until GET sees 'c' in both status and spec") + if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { + obj, err := fooClient.Get(foo.GetName(), metav1.GetOptions{}) + if err != nil { + return false, err + } + if _, found, _ := unstructured.NestedString(obj.Object, "spec", "c"); !found { + t.Log("will retry, did not find spec.c in the object") + return false, nil + } + foo = obj + return true, nil + }); err != nil { + t.Fatal(err) + } + mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}}) + + t.Logf("wait until GET sees 'c' in both status and spec of cached get") + if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { + obj, err := fooClient.Get(foo.GetName(), metav1.GetOptions{ResourceVersion: "0"}) + if err != nil { + return false, err + } + if _, found, _ := unstructured.NestedString(obj.Object, "spec", "c"); !found { + t.Logf("will retry, did not find spec.c in the cached object") + return false, nil + } + foo = obj + return true, nil + }); err != nil { + t.Fatal(err) + } + mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}}) + + t.Logf("verify LIST sees 'c' in both status and spec") + foos, err := fooClient.List(metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + for _, foo := range foos.Items { + mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}}) + } + + t.Logf("verify LIST from cache sees 'c' in both status and spec") + foos, err = fooClient.List(metav1.ListOptions{ResourceVersion: "0"}) + if err != nil { + t.Fatal(err) + } + for _, foo := range foos.Items { + mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}}) + } + + // Omit this test when using the watch cache because changing the CRD resets the watch cache's minimum available resource version. + // The watch cache is populated by list and watch, which are both tested by this test. + // The contents of the watch cache are seen by list with rv=0, which is tested by this test. + if !watchCache { + t.Logf("verify WATCH sees 'c' in both status and spec") + w, err := fooClient.Watch(metav1.ListOptions{ResourceVersion: initialResourceVersion}) + if err != nil { + t.Fatal(err) + } + select { + case event := <-w.ResultChan(): + if event.Type != watch.Modified { + t.Fatalf("unexpected watch event: %v, %#v", event.Type, event.Object) + } + if e, a := "v1beta1", event.Object.GetObjectKind().GroupVersionKind().Version; e != a { + t.Errorf("watch event for v1beta1 API returned %v", a) + } + mustExist( + event.Object.(*unstructured.Unstructured).Object, + [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}}, + ) + case <-time.After(wait.ForeverTestTimeout): + t.Fatal("timed out without getting watch event") + } + } + + t.Logf("Add 'c' default to the REST version, remove it from the storage version, and wait until GET no longer sees it in both status and spec") + addDefault("v1beta1", "c", "C") + removeDefault("v1beta2", "c") if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { obj, err := fooClient.Get(foo.GetName(), metav1.GetOptions{}) if err != nil { @@ -188,22 +366,24 @@ func TestCustomResourceDefaulting(t *testing.T) { } _, found, _ := unstructured.NestedString(obj.Object, "spec", "c") foo = obj - return found, nil + return !found, nil }); err != nil { t.Fatal(err) } - mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}}) + mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"status", "a"}, {"status", "b"}}) + mustNotExist(foo.Object, [][]string{{"spec", "c"}, {"status", "c"}}) - t.Logf("Updating status, expecting 'c' to be set in spec and status") + t.Logf("Updating status, expecting 'c' to be set in status only") if foo, err = fooClient.UpdateStatus(foo, metav1.UpdateOptions{}); err != nil { t.Fatal(err) } - mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"spec", "c"}, {"status", "a"}, {"status", "b"}, {"status", "c"}}) + mustExist(foo.Object, [][]string{{"spec", "a"}, {"spec", "b"}, {"status", "a"}, {"status", "b"}, {"status", "c"}}) + mustNotExist(foo.Object, [][]string{{"spec", "c"}}) - t.Logf("Removing 'a', 'b' and `c` properties. Expecting that 'c' goes away in spec, but not in status. 'a' and 'b' were peristed.") - removeDefault("a") - removeDefault("b") - removeDefault("c") + t.Logf("Removing 'a', 'b' and `c` properties from the REST version. Expecting that 'c' goes away in spec, but not in status. 'a' and 'b' were presisted.") + removeDefault("v1beta1", "a") + removeDefault("v1beta1", "b") + removeDefault("v1beta1", "c") if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { obj, err := fooClient.Get(foo.GetName(), metav1.GetOptions{}) if err != nil { diff --git a/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/versioning/versioning.go b/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/versioning/versioning.go index e770fb3f013..a04a2e98bff 100644 --- a/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/versioning/versioning.go +++ b/staging/src/k8s.io/apimachinery/pkg/runtime/serializer/versioning/versioning.go @@ -113,13 +113,6 @@ func (c *codec) Decode(data []byte, defaultGVK *schema.GroupVersionKind, into ru // if we specify a target, use generic conversion. if into != nil { - if into == obj { - if isVersioned { - return versioned, gvk, nil - } - return into, gvk, nil - } - // perform defaulting if requested if c.defaulter != nil { // create a copy to ensure defaulting is not applied to the original versioned objects @@ -133,6 +126,14 @@ func (c *codec) Decode(data []byte, defaultGVK *schema.GroupVersionKind, into ru } } + // Short-circuit conversion if the into object is same object + if into == obj { + if isVersioned { + return versioned, gvk, nil + } + return into, gvk, nil + } + if err := c.convertor.Convert(obj, into, c.decodeVersion); err != nil { return nil, gvk, err }