diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/BUILD index df63d504687..1dee9542849 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/BUILD @@ -64,6 +64,7 @@ go_test( srcs = [ "etcd_test.go", "status_strategy_test.go", + "strategy_test.go", ], embed = [":go_default_library"], deps = [ diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go index 4895e287af1..988fdc1fef9 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go @@ -87,45 +87,46 @@ func (a customResourceStrategy) PrepareForCreate(ctx context.Context, obj runtim // PrepareForUpdate clears fields that are not allowed to be set by end users on update. func (a customResourceStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { - if !utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubresources) || a.status == nil { - return - } - newCustomResourceObject := obj.(*unstructured.Unstructured) oldCustomResourceObject := old.(*unstructured.Unstructured) newCustomResource := newCustomResourceObject.UnstructuredContent() oldCustomResource := oldCustomResourceObject.UnstructuredContent() - // update is not allowed to set status - _, ok1 := newCustomResource["status"] - _, ok2 := oldCustomResource["status"] - switch { - case ok2: - newCustomResource["status"] = oldCustomResource["status"] - case ok1: - delete(newCustomResource, "status") + // If the /status subresource endpoint is installed, update is not allowed to set status. + if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubresources) && a.status != nil { + _, ok1 := newCustomResource["status"] + _, ok2 := oldCustomResource["status"] + switch { + case ok2: + newCustomResource["status"] = oldCustomResource["status"] + case ok1: + delete(newCustomResource, "status") + } } - // Any changes to the spec increment the generation number, any changes to the - // status should reflect the generation number of the corresponding object. We push - // the burden of managing the status onto the clients because we can't (in general) - // know here what version of spec the writer of the status has seen. It may seem like - // we can at first -- since obj contains spec -- but in the future we will probably make - // status its own object, and even if we don't, writes may be the result of a - // read-update-write loop, so the contents of spec may not actually be the spec that - // the CustomResource has *seen*. - newSpec, ok1 := newCustomResource["spec"] - oldSpec, ok2 := oldCustomResource["spec"] - - // spec is changed, created or deleted - if (ok1 && ok2 && !apiequality.Semantic.DeepEqual(oldSpec, newSpec)) || (ok1 && !ok2) || (!ok1 && ok2) { + // except for the changes to `metadata`, any other changes + // cause the generation to increment. + newCopyContent := copyNonMetadata(newCustomResource) + oldCopyContent := copyNonMetadata(oldCustomResource) + if !apiequality.Semantic.DeepEqual(newCopyContent, oldCopyContent) { oldAccessor, _ := meta.Accessor(oldCustomResourceObject) newAccessor, _ := meta.Accessor(newCustomResourceObject) newAccessor.SetGeneration(oldAccessor.GetGeneration() + 1) } } +func copyNonMetadata(original map[string]interface{}) map[string]interface{} { + ret := make(map[string]interface{}) + for key, val := range original { + if key == "metadata" { + continue + } + ret[key] = val + } + return ret +} + // Validate validates a new CustomResource. func (a customResourceStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { return a.validator.Validate(ctx, obj, a.scale) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy_test.go new file mode 100644 index 00000000000..8e8f178700b --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy_test.go @@ -0,0 +1,257 @@ +/* +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 customresource + +import ( + "context" + "reflect" + "testing" + + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +func generation1() map[string]interface{} { + return map[string]interface{}{ + "generation": int64(1), + } +} + +func generation2() map[string]interface{} { + return map[string]interface{}{ + "generation": int64(2), + } +} + +func TestStrategyPrepareForUpdate(t *testing.T) { + strategy := customResourceStrategy{} + tcs := []struct { + name string + old *unstructured.Unstructured + obj *unstructured.Unstructured + statusEnabled bool + expected *unstructured.Unstructured + }{ + { + name: "/status is enabled, spec changes increment generation", + statusEnabled: true, + old: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "old", + "status": "old", + }, + }, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "new", + "status": "old", + }, + }, + expected: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation2(), + "spec": "new", + "status": "old", + }, + }, + }, + { + name: "/status is enabled, status changes do not increment generation, status changes removed", + statusEnabled: true, + old: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "old", + "status": "old", + }, + }, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "old", + "status": "new", + }, + }, + expected: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "old", + "status": "old", + }, + }, + }, + { + name: "/status is enabled, metadata changes do not increment generation", + statusEnabled: true, + old: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "generation": int64(1), + "other": "old", + }, + "spec": "old", + "status": "old", + }, + }, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "generation": int64(1), + "other": "new", + }, + "spec": "old", + "status": "old", + }, + }, + expected: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "generation": int64(1), + "other": "new", + }, + "spec": "old", + "status": "old", + }, + }, + }, + { + name: "/status is disabled, spec changes increment generation", + statusEnabled: false, + old: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "old", + "status": "old", + }, + }, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "new", + "status": "old", + }, + }, + expected: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation2(), + "spec": "new", + "status": "old", + }, + }, + }, + { + name: "/status is disabled, status changes increment generation", + statusEnabled: false, + old: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "old", + "status": "old", + }, + }, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "old", + "status": "new", + }, + }, + expected: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation2(), + "spec": "old", + "status": "new", + }, + }, + }, + { + name: "/status is disabled, other top-level field changes increment generation", + statusEnabled: false, + old: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "old", + "image": "old", + "status": "old", + }, + }, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation1(), + "spec": "old", + "image": "new", + "status": "old", + }, + }, + expected: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": generation2(), + "spec": "old", + "image": "new", + "status": "old", + }, + }, + }, + { + name: "/status is disabled, metadata changes do not increment generation", + statusEnabled: false, + old: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "generation": int64(1), + "other": "old", + }, + "spec": "old", + "status": "old", + }, + }, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "generation": int64(1), + "other": "new", + }, + "spec": "old", + "status": "old", + }, + }, + expected: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "generation": int64(1), + "other": "new", + }, + "spec": "old", + "status": "old", + }, + }, + }, + } + for _, tc := range tcs { + if tc.statusEnabled { + strategy.status = &apiextensions.CustomResourceSubresourceStatus{} + } else { + strategy.status = nil + } + strategy.PrepareForUpdate(context.TODO(), tc.obj, tc.old) + if !reflect.DeepEqual(tc.obj, tc.expected) { + t.Errorf("test %q failed: expected: %v, got %v", tc.name, tc.expected, tc.obj) + } + } +}