From 1a57b10ad8efa260abfd3c2fb41376f2d4c708d4 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sat, 13 Sep 2025 18:48:28 -0400 Subject: [PATCH 1/3] Move the versioned tracked into its own file --- pkg/client/fake/client.go | 233 ----------------------- pkg/client/fake/versioned_tracker.go | 266 +++++++++++++++++++++++++++ 2 files changed, 266 insertions(+), 233 deletions(-) create mode 100644 pkg/client/fake/versioned_tracker.go diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 1d4af89abc..bb81c1f87b 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -17,13 +17,10 @@ limitations under the License. package fake import ( - "bytes" "context" "errors" "fmt" "reflect" - "runtime/debug" - "strconv" "strings" "sync" "time" @@ -65,7 +62,6 @@ import ( utilrand "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/strategicpatch" - "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/watch" clientgoapplyconfigurations "k8s.io/client-go/applyconfigurations" "k8s.io/client-go/kubernetes/scheme" @@ -79,13 +75,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/internal/objectutil" ) -type versionedTracker struct { - testing.ObjectTracker - scheme *runtime.Scheme - withStatusSubresource sets.Set[schema.GroupVersionKind] - usesFieldManagedObjectTracker bool -} - type fakeClient struct { // trackerWriteLock must be acquired before writing to // the tracker or performing reads that affect a following @@ -354,83 +343,6 @@ func (f *ClientBuilder) Build() client.WithWatch { const trackerAddResourceVersion = "999" -func (t versionedTracker) Add(obj runtime.Object) error { - var objects []runtime.Object - if meta.IsListType(obj) { - var err error - objects, err = meta.ExtractList(obj) - if err != nil { - return err - } - } else { - objects = []runtime.Object{obj} - } - for _, obj := range objects { - accessor, err := meta.Accessor(obj) - if err != nil { - return fmt.Errorf("failed to get accessor for object: %w", err) - } - if accessor.GetDeletionTimestamp() != nil && len(accessor.GetFinalizers()) == 0 { - return fmt.Errorf("refusing to create obj %s with metadata.deletionTimestamp but no finalizers", accessor.GetName()) - } - if accessor.GetResourceVersion() == "" { - // We use a "magic" value of 999 here because this field - // is parsed as uint and and 0 is already used in Update. - // As we can't go lower, go very high instead so this can - // be recognized - accessor.SetResourceVersion(trackerAddResourceVersion) - } - - obj, err = convertFromUnstructuredIfNecessary(t.scheme, obj) - if err != nil { - return err - } - - // If the fieldManager can not decode fields, it will just silently clear them. This is pretty - // much guaranteed not to be what someone that initializes a fake client with objects that - // have them set wants, so validate them here. - // Ref https://github.com/kubernetes/kubernetes/blob/a956ef4862993b825bcd524a19260192ff1da72d/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/fieldmanager.go#L105 - if t.usesFieldManagedObjectTracker { - if err := managedfields.ValidateManagedFields(accessor.GetManagedFields()); err != nil { - return fmt.Errorf("invalid managedFields on %T: %w", obj, err) - } - } - if err := t.ObjectTracker.Add(obj); err != nil { - return err - } - } - - return nil -} - -func (t versionedTracker) Create(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.CreateOptions) error { - accessor, err := meta.Accessor(obj) - if err != nil { - return fmt.Errorf("failed to get accessor for object: %w", err) - } - if accessor.GetName() == "" { - gvk, _ := apiutil.GVKForObject(obj, t.scheme) - return apierrors.NewInvalid( - gvk.GroupKind(), - accessor.GetName(), - field.ErrorList{field.Required(field.NewPath("metadata.name"), "name is required")}) - } - if accessor.GetResourceVersion() != "" { - return apierrors.NewBadRequest("resourceVersion can not be set for Create requests") - } - accessor.SetResourceVersion("1") - obj, err = convertFromUnstructuredIfNecessary(t.scheme, obj) - if err != nil { - return err - } - if err := t.ObjectTracker.Create(gvr, obj, ns, opts...); err != nil { - accessor.SetResourceVersion("") - return err - } - - return nil -} - // convertFromUnstructuredIfNecessary will convert runtime.Unstructured for a GVK that is recognized // by the schema into the whatever the schema produces with New() for said GVK. // This is required because the tracker unconditionally saves on manipulations, but its List() implementation @@ -465,151 +377,6 @@ func convertFromUnstructuredIfNecessary(s *runtime.Scheme, o runtime.Object) (ru return typed, nil } -func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.UpdateOptions) error { - updateOpts, err := getSingleOrZeroOptions(opts) - if err != nil { - return err - } - - return t.update(gvr, obj, ns, false, false, updateOpts) -} - -func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus, deleting bool, opts metav1.UpdateOptions) error { - gvk, err := apiutil.GVKForObject(obj, t.scheme) - if err != nil { - return err - } - obj, err = t.updateObject(gvr, obj, ns, isStatus, deleting, opts.DryRun) - if err != nil { - return err - } - if obj == nil { - return nil - } - - if u, unstructured := obj.(*unstructured.Unstructured); unstructured { - u.SetGroupVersionKind(gvk) - } - - return t.ObjectTracker.Update(gvr, obj, ns, opts) -} - -func (t versionedTracker) Patch(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.PatchOptions) error { - patchOptions, err := getSingleOrZeroOptions(opts) - if err != nil { - return err - } - - // We apply patches using a client-go reaction that ends up calling the trackers Patch. As we can't change - // that reaction, we use the callstack to figure out if this originated from the status client. - isStatus := bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).statusPatch")) - - obj, err = t.updateObject(gvr, obj, ns, isStatus, false, patchOptions.DryRun) - if err != nil { - return err - } - if obj == nil { - return nil - } - - return t.ObjectTracker.Patch(gvr, obj, ns, patchOptions) -} - -func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus, deleting bool, dryRun []string) (runtime.Object, error) { - accessor, err := meta.Accessor(obj) - if err != nil { - return nil, fmt.Errorf("failed to get accessor for object: %w", err) - } - - if accessor.GetName() == "" { - gvk, _ := apiutil.GVKForObject(obj, t.scheme) - return nil, apierrors.NewInvalid( - gvk.GroupKind(), - accessor.GetName(), - field.ErrorList{field.Required(field.NewPath("metadata.name"), "name is required")}) - } - - gvk, err := apiutil.GVKForObject(obj, t.scheme) - if err != nil { - return nil, err - } - - oldObject, err := t.ObjectTracker.Get(gvr, ns, accessor.GetName()) - if err != nil { - // If the resource is not found and the resource allows create on update, issue a - // create instead. - if apierrors.IsNotFound(err) && allowsCreateOnUpdate(gvk) { - return nil, t.Create(gvr, obj, ns) - } - return nil, err - } - - if t.withStatusSubresource.Has(gvk) { - if isStatus { // copy everything but status and metadata.ResourceVersion from original object - if err := copyStatusFrom(obj, oldObject); err != nil { - return nil, fmt.Errorf("failed to copy non-status field for object with status subresouce: %w", err) - } - passedRV := accessor.GetResourceVersion() - if err := copyFrom(oldObject, obj); err != nil { - return nil, fmt.Errorf("failed to restore non-status fields: %w", err) - } - accessor.SetResourceVersion(passedRV) - } else { // copy status from original object - if err := copyStatusFrom(oldObject, obj); err != nil { - return nil, fmt.Errorf("failed to copy the status for object with status subresource: %w", err) - } - } - } else if isStatus { - return nil, apierrors.NewNotFound(gvr.GroupResource(), accessor.GetName()) - } - - oldAccessor, err := meta.Accessor(oldObject) - if err != nil { - return nil, err - } - - // If the new object does not have the resource version set and it allows unconditional update, - // default it to the resource version of the existing resource - if accessor.GetResourceVersion() == "" { - switch { - case allowsUnconditionalUpdate(gvk): - accessor.SetResourceVersion(oldAccessor.GetResourceVersion()) - // This is needed because if the patch explicitly sets the RV to null, the client-go reaction we use - // to apply it and whose output we process here will have it unset. It is not clear why the Kubernetes - // apiserver accepts such a patch, but it does so we just copy that behavior. - // Kubernetes apiserver behavior can be checked like this: - // `kubectl patch configmap foo --patch '{"metadata":{"annotations":{"foo":"bar"},"resourceVersion":null}}' -v=9` - case bytes. - Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Patch")): - // We apply patches using a client-go reaction that ends up calling the trackers Update. As we can't change - // that reaction, we use the callstack to figure out if this originated from the "fakeClient.Patch" func. - accessor.SetResourceVersion(oldAccessor.GetResourceVersion()) - } - } - - if accessor.GetResourceVersion() != oldAccessor.GetResourceVersion() { - return nil, apierrors.NewConflict(gvr.GroupResource(), accessor.GetName(), errors.New("object was modified")) - } - if oldAccessor.GetResourceVersion() == "" { - oldAccessor.SetResourceVersion("0") - } - intResourceVersion, err := strconv.ParseUint(oldAccessor.GetResourceVersion(), 10, 64) - if err != nil { - return nil, fmt.Errorf("can not convert resourceVersion %q to int: %w", oldAccessor.GetResourceVersion(), err) - } - intResourceVersion++ - accessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10)) - - if !deleting && !deletionTimestampEqual(accessor, oldAccessor) { - return nil, fmt.Errorf("error: Unable to edit %s: metadata.deletionTimestamp field is immutable", accessor.GetName()) - } - - if !accessor.GetDeletionTimestamp().IsZero() && len(accessor.GetFinalizers()) == 0 { - return nil, t.ObjectTracker.Delete(gvr, accessor.GetNamespace(), accessor.GetName(), metav1.DeleteOptions{DryRun: dryRun}) - } - return convertFromUnstructuredIfNecessary(t.scheme, obj) -} - func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { if err := c.addToSchemeIfUnknownAndUnstructuredOrPartial(obj); err != nil { return err diff --git a/pkg/client/fake/versioned_tracker.go b/pkg/client/fake/versioned_tracker.go new file mode 100644 index 0000000000..5a72c6e342 --- /dev/null +++ b/pkg/client/fake/versioned_tracker.go @@ -0,0 +1,266 @@ +/* +Copyright 2025 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 fake + +import ( + "bytes" + "errors" + "fmt" + "runtime/debug" + "strconv" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/managedfields" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/client-go/testing" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" +) + +type versionedTracker struct { + testing.ObjectTracker + scheme *runtime.Scheme + withStatusSubresource sets.Set[schema.GroupVersionKind] + usesFieldManagedObjectTracker bool +} + +func (t versionedTracker) Add(obj runtime.Object) error { + var objects []runtime.Object + if meta.IsListType(obj) { + var err error + objects, err = meta.ExtractList(obj) + if err != nil { + return err + } + } else { + objects = []runtime.Object{obj} + } + for _, obj := range objects { + accessor, err := meta.Accessor(obj) + if err != nil { + return fmt.Errorf("failed to get accessor for object: %w", err) + } + if accessor.GetDeletionTimestamp() != nil && len(accessor.GetFinalizers()) == 0 { + return fmt.Errorf("refusing to create obj %s with metadata.deletionTimestamp but no finalizers", accessor.GetName()) + } + if accessor.GetResourceVersion() == "" { + // We use a "magic" value of 999 here because this field + // is parsed as uint and and 0 is already used in Update. + // As we can't go lower, go very high instead so this can + // be recognized + accessor.SetResourceVersion(trackerAddResourceVersion) + } + + obj, err = convertFromUnstructuredIfNecessary(t.scheme, obj) + if err != nil { + return err + } + + // If the fieldManager can not decode fields, it will just silently clear them. This is pretty + // much guaranteed not to be what someone that initializes a fake client with objects that + // have them set wants, so validate them here. + // Ref https://github.com/kubernetes/kubernetes/blob/a956ef4862993b825bcd524a19260192ff1da72d/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/fieldmanager.go#L105 + if t.usesFieldManagedObjectTracker { + if err := managedfields.ValidateManagedFields(accessor.GetManagedFields()); err != nil { + return fmt.Errorf("invalid managedFields on %T: %w", obj, err) + } + } + if err := t.Add(obj); err != nil { + return err + } + } + + return nil +} + +func (t versionedTracker) Create(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.CreateOptions) error { + accessor, err := meta.Accessor(obj) + if err != nil { + return fmt.Errorf("failed to get accessor for object: %w", err) + } + if accessor.GetName() == "" { + gvk, _ := apiutil.GVKForObject(obj, t.scheme) + return apierrors.NewInvalid( + gvk.GroupKind(), + accessor.GetName(), + field.ErrorList{field.Required(field.NewPath("metadata.name"), "name is required")}) + } + if accessor.GetResourceVersion() != "" { + return apierrors.NewBadRequest("resourceVersion can not be set for Create requests") + } + accessor.SetResourceVersion("1") + obj, err = convertFromUnstructuredIfNecessary(t.scheme, obj) + if err != nil { + return err + } + if err := t.Create(gvr, obj, ns, opts...); err != nil { + accessor.SetResourceVersion("") + return err + } + + return nil +} + +func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.UpdateOptions) error { + updateOpts, err := getSingleOrZeroOptions(opts) + if err != nil { + return err + } + + return t.update(gvr, obj, ns, false, false, updateOpts) +} + +func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus, deleting bool, opts metav1.UpdateOptions) error { + gvk, err := apiutil.GVKForObject(obj, t.scheme) + if err != nil { + return err + } + obj, err = t.updateObject(gvr, obj, ns, isStatus, deleting, opts.DryRun) + if err != nil { + return err + } + if obj == nil { + return nil + } + + if u, unstructured := obj.(*unstructured.Unstructured); unstructured { + u.SetGroupVersionKind(gvk) + } + + return t.Update(gvr, obj, ns, opts) +} + +func (t versionedTracker) Patch(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.PatchOptions) error { + patchOptions, err := getSingleOrZeroOptions(opts) + if err != nil { + return err + } + + // We apply patches using a client-go reaction that ends up calling the trackers Patch. As we can't change + // that reaction, we use the callstack to figure out if this originated from the status client. + isStatus := bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).statusPatch")) + + obj, err = t.updateObject(gvr, obj, ns, isStatus, false, patchOptions.DryRun) + if err != nil { + return err + } + if obj == nil { + return nil + } + + return t.Patch(gvr, obj, ns, patchOptions) +} + +func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus, deleting bool, dryRun []string) (runtime.Object, error) { + accessor, err := meta.Accessor(obj) + if err != nil { + return nil, fmt.Errorf("failed to get accessor for object: %w", err) + } + + if accessor.GetName() == "" { + gvk, _ := apiutil.GVKForObject(obj, t.scheme) + return nil, apierrors.NewInvalid( + gvk.GroupKind(), + accessor.GetName(), + field.ErrorList{field.Required(field.NewPath("metadata.name"), "name is required")}) + } + + gvk, err := apiutil.GVKForObject(obj, t.scheme) + if err != nil { + return nil, err + } + + oldObject, err := t.Get(gvr, ns, accessor.GetName()) + if err != nil { + // If the resource is not found and the resource allows create on update, issue a + // create instead. + if apierrors.IsNotFound(err) && allowsCreateOnUpdate(gvk) { + return nil, t.Create(gvr, obj, ns) + } + return nil, err + } + + if t.withStatusSubresource.Has(gvk) { + if isStatus { // copy everything but status and metadata.ResourceVersion from original object + if err := copyStatusFrom(obj, oldObject); err != nil { + return nil, fmt.Errorf("failed to copy non-status field for object with status subresouce: %w", err) + } + passedRV := accessor.GetResourceVersion() + if err := copyFrom(oldObject, obj); err != nil { + return nil, fmt.Errorf("failed to restore non-status fields: %w", err) + } + accessor.SetResourceVersion(passedRV) + } else { // copy status from original object + if err := copyStatusFrom(oldObject, obj); err != nil { + return nil, fmt.Errorf("failed to copy the status for object with status subresource: %w", err) + } + } + } else if isStatus { + return nil, apierrors.NewNotFound(gvr.GroupResource(), accessor.GetName()) + } + + oldAccessor, err := meta.Accessor(oldObject) + if err != nil { + return nil, err + } + + // If the new object does not have the resource version set and it allows unconditional update, + // default it to the resource version of the existing resource + if accessor.GetResourceVersion() == "" { + switch { + case allowsUnconditionalUpdate(gvk): + accessor.SetResourceVersion(oldAccessor.GetResourceVersion()) + // This is needed because if the patch explicitly sets the RV to null, the client-go reaction we use + // to apply it and whose output we process here will have it unset. It is not clear why the Kubernetes + // apiserver accepts such a patch, but it does so we just copy that behavior. + // Kubernetes apiserver behavior can be checked like this: + // `kubectl patch configmap foo --patch '{"metadata":{"annotations":{"foo":"bar"},"resourceVersion":null}}' -v=9` + case bytes. + Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Patch")): + // We apply patches using a client-go reaction that ends up calling the trackers Update. As we can't change + // that reaction, we use the callstack to figure out if this originated from the "fakeClient.Patch" func. + accessor.SetResourceVersion(oldAccessor.GetResourceVersion()) + } + } + + if accessor.GetResourceVersion() != oldAccessor.GetResourceVersion() { + return nil, apierrors.NewConflict(gvr.GroupResource(), accessor.GetName(), errors.New("object was modified")) + } + if oldAccessor.GetResourceVersion() == "" { + oldAccessor.SetResourceVersion("0") + } + intResourceVersion, err := strconv.ParseUint(oldAccessor.GetResourceVersion(), 10, 64) + if err != nil { + return nil, fmt.Errorf("can not convert resourceVersion %q to int: %w", oldAccessor.GetResourceVersion(), err) + } + intResourceVersion++ + accessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10)) + + if !deleting && !deletionTimestampEqual(accessor, oldAccessor) { + return nil, fmt.Errorf("error: Unable to edit %s: metadata.deletionTimestamp field is immutable", accessor.GetName()) + } + + if !accessor.GetDeletionTimestamp().IsZero() && len(accessor.GetFinalizers()) == 0 { + return nil, t.Delete(gvr, accessor.GetNamespace(), accessor.GetName(), metav1.DeleteOptions{DryRun: dryRun}) + } + return convertFromUnstructuredIfNecessary(t.scheme, obj) +} From 9cb838299f8b06ddf7a0141861ce225234e89c45 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sat, 13 Sep 2025 18:54:17 -0400 Subject: [PATCH 2/3] Versioned tracker: Implement object tracker --- pkg/client/fake/client.go | 2 +- pkg/client/fake/versioned_tracker.go | 33 +++++++++++++++++++++++----- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index bb81c1f87b..220f2dc2c6 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -302,7 +302,7 @@ func (f *ClientBuilder) Build() client.WithWatch { usesFieldManagedObjectTracker = true } tracker := versionedTracker{ - ObjectTracker: f.objectTracker, + upstream: f.objectTracker, scheme: f.scheme, withStatusSubresource: withStatusSubResource, usesFieldManagedObjectTracker: usesFieldManagedObjectTracker, diff --git a/pkg/client/fake/versioned_tracker.go b/pkg/client/fake/versioned_tracker.go index 5a72c6e342..3bb44c11ce 100644 --- a/pkg/client/fake/versioned_tracker.go +++ b/pkg/client/fake/versioned_tracker.go @@ -32,12 +32,15 @@ import ( "k8s.io/apimachinery/pkg/util/managedfields" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/testing" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) +var _ testing.ObjectTracker = (*versionedTracker)(nil) + type versionedTracker struct { - testing.ObjectTracker + upstream testing.ObjectTracker scheme *runtime.Scheme withStatusSubresource sets.Set[schema.GroupVersionKind] usesFieldManagedObjectTracker bool @@ -84,7 +87,7 @@ func (t versionedTracker) Add(obj runtime.Object) error { return fmt.Errorf("invalid managedFields on %T: %w", obj, err) } } - if err := t.Add(obj); err != nil { + if err := t.upstream.Add(obj); err != nil { return err } } @@ -112,7 +115,7 @@ func (t versionedTracker) Create(gvr schema.GroupVersionResource, obj runtime.Ob if err != nil { return err } - if err := t.Create(gvr, obj, ns, opts...); err != nil { + if err := t.upstream.Create(gvr, obj, ns, opts...); err != nil { accessor.SetResourceVersion("") return err } @@ -146,7 +149,7 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob u.SetGroupVersionKind(gvk) } - return t.Update(gvr, obj, ns, opts) + return t.upstream.Update(gvr, obj, ns, opts) } func (t versionedTracker) Patch(gvr schema.GroupVersionResource, obj runtime.Object, ns string, opts ...metav1.PatchOptions) error { @@ -167,7 +170,7 @@ func (t versionedTracker) Patch(gvr schema.GroupVersionResource, obj runtime.Obj return nil } - return t.Patch(gvr, obj, ns, patchOptions) + return t.upstream.Patch(gvr, obj, ns, patchOptions) } func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus, deleting bool, dryRun []string) (runtime.Object, error) { @@ -264,3 +267,23 @@ func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runt } return convertFromUnstructuredIfNecessary(t.scheme, obj) } + +func (t versionedTracker) Apply(gvr schema.GroupVersionResource, applyConfiguration runtime.Object, ns string, opts ...metav1.PatchOptions) error { + return t.upstream.Apply(gvr, applyConfiguration, ns, opts...) +} + +func (t versionedTracker) Delete(gvr schema.GroupVersionResource, ns, name string, opts ...metav1.DeleteOptions) error { + return t.upstream.Delete(gvr, ns, name, opts...) +} + +func (t versionedTracker) Get(gvr schema.GroupVersionResource, ns, name string, opts ...metav1.GetOptions) (runtime.Object, error) { + return t.upstream.Get(gvr, ns, name, opts...) +} + +func (t versionedTracker) List(gvr schema.GroupVersionResource, gvk schema.GroupVersionKind, ns string, opts ...metav1.ListOptions) (runtime.Object, error) { + return t.upstream.List(gvr, gvk, ns, opts...) +} + +func (t versionedTracker) Watch(gvr schema.GroupVersionResource, ns string, opts ...metav1.ListOptions) (watch.Interface, error) { + return t.upstream.Watch(gvr, ns, opts...) +} From 535ae6ba9d89d963a0eef77555b1c3b1bddbc252 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sat, 13 Sep 2025 18:56:44 -0400 Subject: [PATCH 3/3] Fakeclient Apply Update: strip status and other issues --- pkg/client/fake/client.go | 9 +++ pkg/client/fake/client_test.go | 103 ++++++++++++++++++++++- pkg/client/fake/versioned_tracker.go | 117 +++++++++++++++++++++------ 3 files changed, 202 insertions(+), 27 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 220f2dc2c6..41cf233deb 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -1000,6 +1000,15 @@ func (c *fakeClient) patch(obj client.Object, patch client.Patch, opts ...client reaction := testing.ObjectReaction(c.tracker) handled, o, err := reaction(action) if err != nil { + // The reaction calls tracker.Get after tracker.Apply to return the object, + // but we may have deleted it in tracker.Apply if there was no finalizer + // left. + if apierrors.IsNotFound(err) && + patch.Type() == types.ApplyPatchType && + oldAccessor.GetDeletionTimestamp() != nil && + len(obj.GetFinalizers()) == 0 { + return nil + } return err } if !handled { diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 46e2b71209..23f52b9fb8 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -660,6 +660,19 @@ var _ = Describe("Fake client", func() { Expect(obj.ObjectMeta.ResourceVersion).To(Equal(trackerAddResourceVersion)) }) + It("should reject apply with non-matching ResourceVersion", func(ctx SpecContext) { + cl := NewClientBuilder().WithRuntimeObjects(cm).Build() + applyCM := corev1applyconfigurations.ConfigMap(cm.Name, cm.Namespace).WithResourceVersion("0") + err := cl.Apply(ctx, applyCM, client.FieldOwner("test")) + Expect(apierrors.IsConflict(err)).To(BeTrue()) + + obj := &corev1.ConfigMap{} + err = cl.Get(ctx, client.ObjectKeyFromObject(cm), obj) + Expect(err).ToNot(HaveOccurred()) + Expect(obj).To(Equal(cm)) + Expect(obj.ObjectMeta.ResourceVersion).To(Equal(trackerAddResourceVersion)) + }) + It("should reject Delete with a mismatched ResourceVersion", func(ctx SpecContext) { bogusRV := "bogus" By("Deleting with a mismatched ResourceVersion Precondition") @@ -714,6 +727,35 @@ var _ = Describe("Fake client", func() { Expect(list.Items).To(ConsistOf(*dep2)) }) + It("should handle finalizers in Apply ", func(ctx SpecContext) { + cl := client.WithFieldOwner(cl, "test") + + By("Creating the object with a finalizer") + cm := corev1applyconfigurations.ConfigMap("test-cm", "delete-with-finalizers"). + WithFinalizers("finalizers.sigs.k8s.io/test") + Expect(cl.Apply(ctx, cm)).To(Succeed()) + + By("Deleting the object") + obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{ + Name: *cm.Name, + Namespace: *cm.Namespace, + }} + Expect(cl.Delete(ctx, obj)).NotTo(HaveOccurred()) + + By("Getting the object") + Expect(cl.Get(ctx, client.ObjectKeyFromObject(obj), obj)).NotTo(HaveOccurred()) + Expect(obj.DeletionTimestamp).NotTo(BeNil()) + + By("Removing the finalizer through SSA") + cm.ResourceVersion = nil + cm.Finalizers = nil + Expect(cl.Apply(ctx, cm)).NotTo(HaveOccurred()) + + By("Getting the object") + err := cl.Get(ctx, client.ObjectKeyFromObject(obj), &corev1.ConfigMap{}) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) + It("should handle finalizers on Update", func(ctx SpecContext) { namespacedName := types.NamespacedName{ Name: "test-cm", @@ -1733,6 +1775,40 @@ var _ = Describe("Fake client", func() { Expect(cmp.Diff(objOriginal, actual)).To(BeEmpty()) }) + It("should not change the status of objects with status subresource when creating through apply ", func(ctx SpecContext) { + obj := corev1applyconfigurations. + Pod("node", ""). + WithStatus( + corev1applyconfigurations.PodStatus().WithPhase("Running"), + ) + + cl := NewClientBuilder().WithStatusSubresource(&corev1.Pod{}).Build() + Expect(cl.Apply(ctx, obj, client.FieldOwner("test"))).To(Succeed()) + + p := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: *obj.Name}} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(p), p)).To(Succeed()) + + Expect(p.Status).To(BeComparableTo(corev1.PodStatus{})) + }) + + It("should not change the status of objects with status subresource when updating through apply ", func(ctx SpecContext) { + + cl := NewClientBuilder().WithStatusSubresource(&corev1.Pod{}).Build() + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod"}} + Expect(cl.Create(ctx, pod)).NotTo(HaveOccurred()) + + obj := corev1applyconfigurations. + Pod(pod.Name, ""). + WithStatus( + corev1applyconfigurations.PodStatus().WithPhase("Running"), + ) + Expect(cl.Apply(ctx, obj, client.FieldOwner("test"))).To(Succeed()) + + Expect(cl.Get(ctx, client.ObjectKeyFromObject(pod), pod)).To(Succeed()) + + Expect(pod.Status).To(BeComparableTo(corev1.PodStatus{})) + }) + It("should Unmarshal the schemaless object with int64 to preserve ints", func(ctx SpecContext) { schemeBuilder := &scheme.Builder{GroupVersion: schema.GroupVersion{Group: "test", Version: "v1"}} schemeBuilder.Register(&WithSchemalessSpec{}) @@ -2781,6 +2857,17 @@ var _ = Describe("Fake client", func() { Expect(cm.Data).To(BeComparableTo(map[string]string{"other": "data"})) }) + It("returns a conflict when trying to Create an object with UID set through Apply", func(ctx SpecContext) { + cl := NewClientBuilder().Build() + obj := corev1applyconfigurations. + ConfigMap("foo", "default"). + WithUID("123") + + err := cl.Apply(ctx, obj, &client.ApplyOptions{FieldManager: "test-manager"}) + Expect(err).To(HaveOccurred()) + Expect(apierrors.IsConflict(err)).To(BeTrue()) + }) + It("errors when trying to server-side apply an object without configuring a FieldManager", func(ctx SpecContext) { cl := NewClientBuilder().Build() obj := corev1applyconfigurations. @@ -2827,7 +2914,7 @@ var _ = Describe("Fake client", func() { Expect(result.Object["spec"]).To(Equal(map[string]any{"other": "data"})) }) - It("sets managed fields through all methods", func(ctx SpecContext) { + It("sets the fieldManager in create, patch and update", func(ctx SpecContext) { owner := "test-owner" cl := client.WithFieldOwner( NewClientBuilder().WithReturnManagedFields().Build(), @@ -2861,6 +2948,20 @@ var _ = Describe("Fake client", func() { } }) + It("sets the fieldManager when creating through update", func(ctx SpecContext) { + owner := "test-owner" + cl := client.WithFieldOwner( + NewClientBuilder().WithReturnManagedFields().Build(), + owner, + ) + + obj := &corev1.Event{ObjectMeta: metav1.ObjectMeta{Name: "foo"}} + Expect(cl.Update(ctx, obj, client.FieldOwner(owner))).NotTo(HaveOccurred()) + for _, f := range obj.ManagedFields { + Expect(f.Manager).To(BeEquivalentTo(owner)) + } + }) + // GH-3267 It("Doesn't leave stale data when updating an object through SSA", func(ctx SpecContext) { obj := corev1applyconfigurations. diff --git a/pkg/client/fake/versioned_tracker.go b/pkg/client/fake/versioned_tracker.go index 3bb44c11ce..c1caa1ca02 100644 --- a/pkg/client/fake/versioned_tracker.go +++ b/pkg/client/fake/versioned_tracker.go @@ -137,11 +137,17 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob if err != nil { return err } - obj, err = t.updateObject(gvr, obj, ns, isStatus, deleting, opts.DryRun) + obj, needsCreate, err := t.updateObject(gvr, gvk, obj, ns, isStatus, deleting, allowsCreateOnUpdate(gvk), opts.DryRun) if err != nil { return err } - if obj == nil { + + if needsCreate { + opts := metav1.CreateOptions{DryRun: opts.DryRun, FieldManager: opts.FieldManager} + return t.Create(gvr, obj, ns, opts) + } + + if obj == nil { // Object was deleted in updateObject return nil } @@ -158,72 +164,94 @@ func (t versionedTracker) Patch(gvr schema.GroupVersionResource, obj runtime.Obj return err } + gvk, err := apiutil.GVKForObject(obj, t.scheme) + if err != nil { + return err + } + // We apply patches using a client-go reaction that ends up calling the trackers Patch. As we can't change // that reaction, we use the callstack to figure out if this originated from the status client. isStatus := bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).statusPatch")) - obj, err = t.updateObject(gvr, obj, ns, isStatus, false, patchOptions.DryRun) + obj, needsCreate, err := t.updateObject(gvr, gvk, obj, ns, isStatus, false, allowsCreateOnUpdate(gvk), patchOptions.DryRun) if err != nil { return err } - if obj == nil { + if needsCreate { + opts := metav1.CreateOptions{DryRun: patchOptions.DryRun, FieldManager: patchOptions.FieldManager} + return t.Create(gvr, obj, ns, opts) + } + + if obj == nil { // Object was deleted in updateObject return nil } return t.upstream.Patch(gvr, obj, ns, patchOptions) } -func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus, deleting bool, dryRun []string) (runtime.Object, error) { +// updateObject performs a number of validations and changes related to +// object updates, such as checking and updating the resourceVersion. +func (t versionedTracker) updateObject( + gvr schema.GroupVersionResource, + gvk schema.GroupVersionKind, + obj runtime.Object, + ns string, + isStatus bool, + deleting bool, + allowCreateOnUpdate bool, + dryRun []string, +) (result runtime.Object, needsCreate bool, _ error) { accessor, err := meta.Accessor(obj) if err != nil { - return nil, fmt.Errorf("failed to get accessor for object: %w", err) + return nil, false, fmt.Errorf("failed to get accessor for object: %w", err) } if accessor.GetName() == "" { - gvk, _ := apiutil.GVKForObject(obj, t.scheme) - return nil, apierrors.NewInvalid( + return nil, false, apierrors.NewInvalid( gvk.GroupKind(), accessor.GetName(), field.ErrorList{field.Required(field.NewPath("metadata.name"), "name is required")}) } - gvk, err := apiutil.GVKForObject(obj, t.scheme) - if err != nil { - return nil, err - } - oldObject, err := t.Get(gvr, ns, accessor.GetName()) if err != nil { // If the resource is not found and the resource allows create on update, issue a // create instead. - if apierrors.IsNotFound(err) && allowsCreateOnUpdate(gvk) { - return nil, t.Create(gvr, obj, ns) + if apierrors.IsNotFound(err) && allowCreateOnUpdate { + // Pass this info to the caller rather than create, because in the SSA case it + // must be created by calling Apply in the upstream tracker, not Create. + // This is because SSA considers Apply and Non-Apply operations to be different + // even when they use the same fieldManager. This behavior is also observable + // with a real Kubernetes apiserver. + // + // Ref https://kubernetes.slack.com/archives/C0EG7JC6T/p1757868204458989?thread_ts=1757808656.002569&cid=C0EG7JC6T + return obj, true, nil } - return nil, err + return obj, false, err } if t.withStatusSubresource.Has(gvk) { if isStatus { // copy everything but status and metadata.ResourceVersion from original object if err := copyStatusFrom(obj, oldObject); err != nil { - return nil, fmt.Errorf("failed to copy non-status field for object with status subresouce: %w", err) + return nil, false, fmt.Errorf("failed to copy non-status field for object with status subresouce: %w", err) } passedRV := accessor.GetResourceVersion() if err := copyFrom(oldObject, obj); err != nil { - return nil, fmt.Errorf("failed to restore non-status fields: %w", err) + return nil, false, fmt.Errorf("failed to restore non-status fields: %w", err) } accessor.SetResourceVersion(passedRV) } else { // copy status from original object if err := copyStatusFrom(oldObject, obj); err != nil { - return nil, fmt.Errorf("failed to copy the status for object with status subresource: %w", err) + return nil, false, fmt.Errorf("failed to copy the status for object with status subresource: %w", err) } } } else if isStatus { - return nil, apierrors.NewNotFound(gvr.GroupResource(), accessor.GetName()) + return nil, false, apierrors.NewNotFound(gvr.GroupResource(), accessor.GetName()) } oldAccessor, err := meta.Accessor(oldObject) if err != nil { - return nil, err + return nil, false, err } // If the new object does not have the resource version set and it allows unconditional update, @@ -246,29 +274,66 @@ func (t versionedTracker) updateObject(gvr schema.GroupVersionResource, obj runt } if accessor.GetResourceVersion() != oldAccessor.GetResourceVersion() { - return nil, apierrors.NewConflict(gvr.GroupResource(), accessor.GetName(), errors.New("object was modified")) + return nil, false, apierrors.NewConflict(gvr.GroupResource(), accessor.GetName(), errors.New("object was modified")) } if oldAccessor.GetResourceVersion() == "" { oldAccessor.SetResourceVersion("0") } intResourceVersion, err := strconv.ParseUint(oldAccessor.GetResourceVersion(), 10, 64) if err != nil { - return nil, fmt.Errorf("can not convert resourceVersion %q to int: %w", oldAccessor.GetResourceVersion(), err) + return nil, false, fmt.Errorf("can not convert resourceVersion %q to int: %w", oldAccessor.GetResourceVersion(), err) } intResourceVersion++ accessor.SetResourceVersion(strconv.FormatUint(intResourceVersion, 10)) if !deleting && !deletionTimestampEqual(accessor, oldAccessor) { - return nil, fmt.Errorf("error: Unable to edit %s: metadata.deletionTimestamp field is immutable", accessor.GetName()) + return nil, false, fmt.Errorf("error: Unable to edit %s: metadata.deletionTimestamp field is immutable", accessor.GetName()) } if !accessor.GetDeletionTimestamp().IsZero() && len(accessor.GetFinalizers()) == 0 { - return nil, t.Delete(gvr, accessor.GetNamespace(), accessor.GetName(), metav1.DeleteOptions{DryRun: dryRun}) + return nil, false, t.Delete(gvr, accessor.GetNamespace(), accessor.GetName(), metav1.DeleteOptions{DryRun: dryRun}) } - return convertFromUnstructuredIfNecessary(t.scheme, obj) + + obj, err = convertFromUnstructuredIfNecessary(t.scheme, obj) + return obj, false, err } func (t versionedTracker) Apply(gvr schema.GroupVersionResource, applyConfiguration runtime.Object, ns string, opts ...metav1.PatchOptions) error { + patchOptions, err := getSingleOrZeroOptions(opts) + if err != nil { + return err + } + gvk, err := apiutil.GVKForObject(applyConfiguration, t.scheme) + if err != nil { + return err + } + applyConfiguration, needsCreate, err := t.updateObject(gvr, gvk, applyConfiguration, ns, false, false, true, patchOptions.DryRun) + if err != nil { + return err + } + + if needsCreate { + // https://github.com/kubernetes/kubernetes/blob/81affffa1b8d8079836f4cac713ea8d1b2bbf10f/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L606 + accessor, err := meta.Accessor(applyConfiguration) + if err != nil { + return fmt.Errorf("failed to get accessor for object: %w", err) + } + if accessor.GetUID() != "" { + return apierrors.NewConflict(gvr.GroupResource(), accessor.GetName(), fmt.Errorf("uid mismatch: the provided object specified uid %s, and no existing object was found", accessor.GetUID())) + } + + if t.withStatusSubresource.Has(gvk) { + // Clear out status for create, for update this is handled in updateObject + if err := copyStatusFrom(&unstructured.Unstructured{}, applyConfiguration); err != nil { + return err + } + } + } + + if applyConfiguration == nil { // Object was deleted in updateObject + return nil + } + return t.upstream.Apply(gvr, applyConfiguration, ns, opts...) }