From 417db5f70812f1d5018b188fc6e56c70ff291216 Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Tue, 25 Sep 2018 15:45:02 -0700 Subject: [PATCH] Changes when .metadata.generation of a CR increments. If the custom resource participates the spec/status convention, the metadata.generation of the CR increments whenever there is any change, except for the changes to the metadata or the changes to the status. If the CR does not participate the spec/status convention, the metadata.generation of the CR increments whenever there is any change, except for the changes to the metadata. A CR is considered to participate the spec/status convention if and only if the "CustomResourceSubresources" feature gate is turned on and when the CRD has `.spec.subresources.status={}`. --- .../pkg/registry/customresource/BUILD | 1 + .../pkg/registry/customresource/strategy.go | 51 ++-- .../registry/customresource/strategy_test.go | 257 ++++++++++++++++++ 3 files changed, 284 insertions(+), 25 deletions(-) create mode 100644 staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy_test.go 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) + } + } +}