The fake client.Update() updates the status fields

[ahmetb]

I'm observing that using the non-subresource fake.Client on both in-tree types (like corev1.Node or corev1.Pod) as well as Custom Resources actually save modifications on status fields to the resource (and are reflected on subsequent Get calls).

A minimal repro: https://gist.github.com/ahmetb/50784526a54244d45022a7ed8d556a62
This is the case even if I do fake.NewClientBuilder().WithStatusSubresource(&corev1.Pod{})

Expected behavior is to be the same as a non-fake client where if you modify a status field and use client.Update(ctx, obj) the modifications on the status field are discarded.

/kind bug

[troy0820]

This seems to replicate the behavior in issue #2474 but the other way around. It seems to me that the fake client is copying over everything and not discarding the status when going through the update chain. PR I have for that #2479 solves that case however long term we may need to decouple the methods that the fakeclient uses.

[ahmetb]

For whatever reason, the test case that's supposed to catch this acts differently on different fields of corev1.Node.Status. I'll debug further, but this patch to the test shows the bug:

diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go
index 1350fa2..c823282 100644
--- a/pkg/client/fake/client_test.go
+++ b/pkg/client/fake/client_test.go
@@ -1420,6 +1420,7 @@ var _ = Describe("Fake client", func() {
 		cl := NewClientBuilder().WithStatusSubresource(obj).WithObjects(obj).Build()
 
 		obj.Status.NodeInfo.MachineID = "updated-machine-id"
+		obj.Status.Phase = corev1.NodeRunning
 		Expect(cl.Update(context.Background(), obj)).To(Succeed())
 
 		Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(Succeed())

[ahmetb]

I think here's where the problem is: fake client uses json marshaling/unmarshaling under the covers to assign the .status field from old object to new.

However, while unmarshaling oldObject's status back into newObject passed to Update(), it uses json.Unmarshal, which doesn't clear the newly set values on newObject (which were previously unset on the oldObject). That's why changing a status value (as the current test does) is reverted, but setting a new value isn't.

controller-runtime/pkg/client/fake/client.go

Lines 1036 to 1038 in 2a553d6

	if err := json.Unmarshal(serialized, &target); err != nil {
	return fmt.Errorf("failed to deserialize: %w", err)
	}

[troy0820]

In this pr #2479 I created a test to show that the other values do not get overwritten when updating the status.
Meaning, sending that through the update “chain” resolves around what you send to the update method. For status client at least. Not sure if we need to do the same thing for fake client update to not carry the updated status.

So it ends up being like this: (status update)

1. Call update with new obj,
2. Get old object from tracker
3. Copy status from new (new meaning what was passed into update) object to “tracker” object
4. It passes it to the fake client update method which then goes to the tracker getting the old object again and splitting the difference.

So it’ll only see the difference in the status when doing the update.

[troy0820]

There was a reason we have to serialize it like that if I’m not mistaken.

[alvaroaleman]

/assign

The root of the issue is the go json unmarshaller not zeroing the target, as a result, if the status is empty and the new status is not-empty, the non-empty status remains. This is also why the test didn't catch this, as it doesn't test with an empty status. I'll file a fix.