I can not confirm the behavior described here against a real kube apiserver: If you pass an incorrect RV the request will fail (you can test this with kubectl apply --server-side). Only if you pass no RV (which I think is the common case for SSA) will it succeed regardless of server RV.

I see, makes sense. Still the code from which we hit this was failing even after we were wiping the resource version clean right before the SSA, when in version 0.22 it was working fine without changes.

Let me double check.

I need to dig deeper.

When testing this again removing the fix and rv value I get it to pass. But our original test still fails.

Comparing if accessor.GetResourceVersion() != oldAccessor.GetResourceVersion() from my controller-runtime checkout (using this test modified) the comparison is rv = "1" == old rv = "1" so it passes. But from our code I saw rv="" == old rv = "1000"

Not sure what is the difference between both tests. Also not sure how the passing test is comparing "1 against 1" even though we wiped the rv before calling SSA.

We think we found it:

	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())
		case bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Apply")):
			// 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.Apply" func.
			accessor.SetResourceVersion(oldAccessor.GetResourceVersion())
		}
	}

This POD test cannot reproduce the issue we hit because it is a core resource and follows the first case case allowsUnconditionalUpdate(gvk) which fixed the RV to match

Our resource is a CRD, and we are setting a subresource on it. So we are missing this case:

case bytes.Contains(debug.Stack(), []byte("sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).Apply")):
			// 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())

I shall revamp this PR:

1. Test must reproduce the issue using a CRD, not a core resource.
2. The old code fix goes away.
3. The new fix is just the new entry case in that are missing.

Not all in-tree resources allow unconditional update, so you might find one there you can use for testing

In the end I used an adhoc custom object. It took longer to verify because if the test registers an unstructured custom object, the issue is not reproduced. In that case, the oldAccessor is set with an empty resourceVersion which matches the one in the accessor.

Only when the custom object registered with a proper type, like in our case, you would hit the issue. In that case, the oldAccessor is set with a non empty resourceVersion which does not match the one in the accessor.

This was tricky to figure out, I had to debug our failing test and the reproducer until I spotted the difference between both.

This PR is split into 2 commits, one with the test and one with the fix. If you run without the fix commit you can see the error we hit in our case.

Can we also add a test that validates that status apply requests with an invalid RV fail?

@alvaroaleman I am confused. Maybe I misunderstood some details here.
I would assume that the RV value sent on an SSA will be ignored. In fact, would not the applied config that is what we pass the SSA operation have that already removed?

No, if you send an SSA request with an RV in it and it doesn't match, the apiserver fails the request: #3443 (comment)

ah! OK makes sense.

Test added now

Would you mind using a single case statement for Apply, subresource.Patch and subresource.Apply that ORs them? We don't really need the same comment three times

yeah it was getting too verbose now

done

Please check that we got the correct error here as well and not just any error:

Expect(apierrors.IsConflict(err)).To(BeTrue())

added