Fakeclient Apply Update: strip status and other issues

alvaroaleman · alvaroaleman · commit 8e536d10d8c0 · 2025-09-14T13:55:10.000-04:00
diff --git 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
@@ -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)).NotTo(HaveOccurred())
+
+			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,24 @@ var _ = Describe("Fake client", func() {
 		Expect(cmp.Diff(objOriginal, actual)).To(BeEmpty())
 	})
 
+	It("should not change the status of typed objects that have a status subresource on 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{})
@@ -2827,7 +2887,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 +2921,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
@@ -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 before the
+// 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 then 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,48 @@ 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, _, err = t.updateObject(gvr, gvk, applyConfiguration, ns, false, false, true, patchOptions.DryRun)
+	if err != nil {
+		return err
+	}
+
+	if applyConfiguration == nil { // Object was deleted in updateObject
+		return nil
+	}
+
 	return t.upstream.Apply(gvr, applyConfiguration, ns, opts...)
 }
 
