FakeClient update on status subresource not returning error despite conflicting resource version since v0.15.0

[nbam-e]

After upgrading to controller-runtime 0.15.0 one of our unit tests started failing.
The tested controller performs a client.Status().Update(...) on Pod resources. The now failing unit test aims to test the controller behavior in case of a resource conflict (i.e. The fake client has a pod object, the controller performs a status update on the same pod object, but with a different resource version -> expected behavior: Conflict error, actual behavior since upgrading to 0.15.0: no error).

This can be reproduced with the following testcase: (controller-runtime 0.15.0, k8s api 0.27.2)

func Test_FakeStatusUpdate(t *testing.T) {
	pod := &corev1.Pod{
		ObjectMeta: metav1.ObjectMeta{
			Namespace:       "test",
			Name:            "test",
			ResourceVersion: "1",
		},
	}

	c := fake.NewClientBuilder().
		WithRuntimeObjects(pod).
		Build()

	pod.Status.Phase = "testphase"
	pod.ResourceVersion = "2"
	err := c.Status().Update(context.Background(), pod)
	if err == nil {
		t.Fatal("Expected conflict error, but got nil")
	}
}

I think it happens because the result of SetResourceVersion is immediately overwritten again by fromMapStringAny.

There appears to be a unit test for this case, but it doesn't fail because it uses client.Update(...) instead of client.Status().Update(...). https://github.com/kubernetes-sigs/controller-runtime/blob/30eae58f1b984c1b8139dd9b9f68dd2d530ed429/pkg/client/fake/client_test.go#LL1441C2-L1441C2

Related PR: #2259

[sbueringer]

Agree, sounds like a bug

[iiiceoo]

/assign

[AmFlint]

Hello, I encountered the exact same issue.

While I'm waiting for a fix to be released, I rolled back my controller-runtime to use 0.14.6 (with all k8s go libs using 0.26.1) and it works for now...

[sbueringer]

@AmFlint Did the fix implemented in #2365 work for you?

[RealAnna]

@sbueringer this did not work entirely for us. With the new changes we still had to explicitly add our crds as Statussubresurce to be able to use status updates.
https://github.com/keptn/lifecycle-toolkit/pull/1701/files#diff-a1bebf6a5efdcbe0c8bd59ac1387c9d1f80f42781e1dc463cdc121222dafa089R17

Generating the client and adding a resource with client.Create() does not work as before.
All tests return a status not found since the object is not added to the StatusSubresource.

[austincunningham]

Same issue using #2365 didn't fix the issue

replace sigs.k8s.io/controller-runtime => github.com/kubernetes-sigs/controller-runtime v0.15.1-0.20230602030726-6c1419808849

[troy0820]

/kind bug

With the release of v0.16.0 did this not work? I see above that the #2365 wasn't successful but wanted to know if anything else in that release fixed this issue.

[troy0820]

Re-doing this test like this:

 func Test_FakeStatusUpdate(t *testing.T) {
    pod := &corev1.Pod{
      ObjectMeta: metav1.ObjectMeta{
        Namespace:       "test",
        Name:            "test",
        ResourceVersion: "1",
      },
    }
 
    c := fake.NewClientBuilder().
      WithRuntimeObjects(pod).WithStatusSubresource(pod).
      Build()
 
    pod.Status.Phase = "testphase"
    pod.ResourceVersion = "2"
    err := c.Status().Update(context.Background(), pod)
    if err == nil {
      t.Fatal("Expected conflict error, but got nil")
    }
  }

now passes with the intended error as before.

The snippet was updated here

Doing a subsequent Get of the pod will show that the pod is not updated with no status.Phase and resourceVersion of 1.

This test is running against controller-runtime v0.16.0 and go1.20

Does this help @nbam-e ?

[nbam-e]

@troy0820
I did some tests, in our case the issue with our unit test (which used a Pod resource, not a CRD) is already fixed with controller-runtime 0.15.1 (controller-runtime 0.16.0 also works).
The testcase from my description now passes both with and without the addition of .WithStatusSubresource(pod), so I think at least this issue with the missing conflict response is fixed.

I think some of the other commenters here are referring to a different behavior change when using the fake client with CRDs. Since controller-runtime 0.15.0 you have to manually add the WithStatusSubresource(...) invocation to the fake client builder, otherwise it returns a "not found" error when updating the status of a corresponding CR. This was not necessary before 0.15.0 (probably because Subresource semantics were ignored (#2230)) and is somewhat unexpected from a user's perspective. I assume the explicit WithStatusSubresource is necessary because the fake client cannot infer whether the status object in a CRD is a subresource from the Scheme alone?

[troy0820]

This was not necessary before 0.15.0 (probably because Subresource semantics were ignored (#2230)) and is somewhat unexpected from a user's perspective. I assume the explicit WithStatusSubresource is necessary because the fake client cannot infer whether the status object in a CRD is a subresource from the Scheme alone?

There is another thread around here describing the reason why this is now necessary and the inference of the subresource on resources that are not core are said to be difficult to assume. In controller-runtime the function inTreeResourcesWithStatus() returns a list of resources' GVK that have "subresources". To demonstrate the behavior expected from the api-server it is this fix that make those changes necessary. (Again, the release notes explain this but using this to explain the change of behavior)

However supplementing the builder to have this on instantiation with WithStatusSubresource(...) can fill in gaps where this should be applied.

The issue I have been observing is if you are to create a resource, (You typically don't put a status on them just spec) and then try to update the status of the object you just created (without building it in the client with subresource), you'll get a not found error as if the tracker isn't finding it.

I'm looking to see how to resolve that, but I fear that this can't be done because it can't see if it is supposed to have a subresource before you try to update the status.

[ericabramov]

I've came up with a hacky workaround that solved the gap for me
A POC code is posted in this stackoverflow thread

https://stackoverflow.com/questions/77489441/go-k8s-controller-runtime-upgrade-fake-client-lacks-functionality

[filanov]

Is there another workaround for this? my test is trying to check a status change on a reconcile, meaning i cannot provide WithStatusSubresource when creating the client because this is what the test need to validate.
I saw that update just update everything including the status but it's a bad solution because a real client doesn't behave that way.

Is there a workaround? or another client that i can use?
I was thinking maybe wrap the client and override the Status().Update() function to call Update in the tests but it sounds like a fix to the fake client would be better.

In older versions (0.13.x) it worked as expected why did that flow changed?

[alvaroaleman]

The original issue was fixed and the latter discussion seems to evolve around the question of how to configure a CRD to have a status subresource in the fake client. The tl,dr on how to configure the fakeclient to provide a status subresource for a given resource is to do the following:

fake.NewClientBuilder().WithStatusSubresource(&MyType{}).Build()

If this is needed or not can not be automatically inferred, refer to #2386 (comment) as for why.

As the original issue was fixed, I am going to close this. Please limit discussions on this issue to the bug it was created for and that was since fixed.