From 2b01f63b115e19e8ac9f8ee8e00dde65c5f40290 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Wed, 22 Mar 2023 11:18:50 -0700 Subject: [PATCH] move check for noop managed field timestamp updates this check needs to go after any mutations. After the mutating admission chain, rest.BeforeUpdate (which is responsible for reverting updates to immutable timestamp fields, among other things.) is called in the store.Update function. Without moving this check, it will be possible for an object to be written to etcd with only a change to its managed fields timestamp. --- .../apiserver/pkg/endpoints/handlers/patch.go | 3 -- .../pkg/endpoints/handlers/update.go | 9 ---- .../pkg/registry/generic/registry/store.go | 10 ++++ .../integration/apiserver/apply/apply_test.go | 50 +++++++++++++++++++ 4 files changed, 60 insertions(+), 12 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go index 4f5533f34afe..f743e7c5d109 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go @@ -654,9 +654,6 @@ func (p *patcher) patchResource(ctx context.Context, scope *RequestScope) (runti } transformers := []rest.TransformFunc{p.applyPatch, p.applyAdmission, dedupOwnerReferencesTransformer} - if scope.FieldManager != nil { - transformers = append(transformers, fieldmanager.IgnoreManagedFieldsTimestampsTransformer) - } wasCreated := false p.updatedObjectInfo = rest.DefaultUpdatedObjectInfo(nil, transformers...) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go index 630c97cdcdd6..202888639814 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go @@ -189,15 +189,6 @@ func UpdateResource(r rest.Updater, scope *RequestScope, admit admission.Interfa }) } - // Ignore changes that only affect managed fields - // timestamps. FieldManager can't know about changes - // like normalized fields, defaulted fields and other - // mutations. - // Only makes sense when SSA field manager is being used - if scope.FieldManager != nil { - transformers = append(transformers, fieldmanager.IgnoreManagedFieldsTimestampsTransformer) - } - createAuthorizerAttributes := authorizer.AttributesRecord{ User: userInfo, ResourceRequest: true, diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index fa23d29d6c90..55f06f7972b8 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -38,6 +38,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" + "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" "k8s.io/apiserver/pkg/registry/rest" @@ -671,6 +672,15 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj if err := rest.BeforeUpdate(e.UpdateStrategy, ctx, obj, existing); err != nil { return nil, nil, err } + + // Ignore changes that only affect managed fields timestamps. + // FieldManager can't know about changes like normalized fields, defaulted + // fields and other mutations. + obj, err = fieldmanager.IgnoreManagedFieldsTimestampsTransformer(ctx, obj, existing) + if err != nil { + return nil, nil, err + } + // at this point we have a fully formed object. It is time to call the validators that the apiserver // handling chain wants to enforce. if updateValidation != nil { diff --git a/test/integration/apiserver/apply/apply_test.go b/test/integration/apiserver/apply/apply_test.go index 36ac3225bc89..78b8be3a3757 100644 --- a/test/integration/apiserver/apply/apply_test.go +++ b/test/integration/apiserver/apply/apply_test.go @@ -27,6 +27,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" "sigs.k8s.io/yaml" appsv1 "k8s.io/api/apps/v1" @@ -249,6 +250,55 @@ func getRV(obj runtime.Object) (string, error) { return acc.GetResourceVersion(), nil } +func TestNoopChangeCreationTime(t *testing.T) { + client, closeFn := setup(t) + defer closeFn() + + ssBytes := []byte(`{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "myconfig", + "creationTimestamp": null, + "resourceVersion": null + }, + "data": { + "key": "value" + } + }`) + + obj, err := client.CoreV1().RESTClient().Patch(types.ApplyPatchType). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("configmaps"). + Name("myconfig"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + + require.NoError(t, err) + // Sleep for one second to make sure that the times of each update operation is different. + time.Sleep(1200 * time.Millisecond) + + newObj, err := client.CoreV1().RESTClient().Patch(types.ApplyPatchType). + Namespace("default"). + Param("fieldManager", "apply_test"). + Resource("configmaps"). + Name("myconfig"). + Body(ssBytes). + Do(context.TODO()). + Get() + if err != nil { + t.Fatalf("Failed to create object: %v", err) + } + + require.NoError(t, err) + require.Equal(t, obj, newObj) +} + // TestNoSemanticUpdateAppleSameResourceVersion makes sure that APPLY requests which makes no semantic changes // will not change the resource version (no write to etcd is done) //