🐛 Do not update anything but status when using subresource client

[troy0820]

During updating with the status client, it is shown that the metadata/labels will come with the subresource client. To mitigate this, updating the method to only use the old object in the tracker, will allow the status to be copied from the new object and put in the update chain to the fake client tracker.

Resolves: #2474

• Update fake client subreresource client method update to only copy status before passing to tracker.
• Include test from issue to show this is working properly cc @berlin-ab (cherry-picked from draft PR)
• Create test case to show other status fields do not get written over.

[alvaroaleman]

Hey @troy0820 , thanks so much for jumping into this and providing a fix this quickly!

The root of the problem seems to be that copyNonStatusFrom is basically broken, because it is not going to clear fields that exist on the status object but not on the main object. Rather than working around that like you suggested, I'd prefer it if we kept the status handling in one place and avoid the code duplication, something like this:

index 18dd639d..d7b8427a 100644
--- a/pkg/client/fake/client.go
+++ b/pkg/client/fake/client.go
@@ -399,10 +399,11 @@ func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Ob
        }

        if t.withStatusSubresource.Has(gvk) {
-               if isStatus { // copy everything but status and metadata.ResourceVersion from original object
-                       if err := copyNonStatusFrom(oldObject, obj); err != nil {
-                               return fmt.Errorf("failed to copy non-status field for object with status subresouce: %w", err)
+               if isStatus { // copy status from object into oldObject, then set object to a copy of oldObject
+                       if err := copyStatusFrom(obj, oldObject); err != nil {
+                               return fmt.Errorf("failed to copy the status for object with status subresource: %w", err)
                        }
+                       obj = oldObject.DeepCopyObject().(client.Object)
                } else { // copy status from original object
                        if err := copyStatusFrom(oldObject, obj); err != nil {
                                return fmt.Errorf("failed to copy the status for object with status subresource: %w", err)

(and delete copyNonStatusFrom)

[troy0820]

Yeah, I can do that @alvaroaleman. I fear we may have this issue with client.Update() using the method to include status with the update there in another issue. I can fix this and push up the changes and we can discuss on the other issue.

[troy0820]

This is going to fail lint because the function is not used. I can add the lint override to not flag this if that is what we want to do.

[alvaroaleman]

Why not remove the func?

[troy0820]

I can but didn't want to arbitrarily start deleting code. I'll delete it.

[alvaroaleman]

I think the tittle is incorrect and the number of negations makes me 🤯 . Did you mean to say should not overwrite non-status fields of typed objects that have a status subresource on status update? If yes, maybe express it as Should only override status fields of typed objects that have a status subresource on status update?

[troy0820]

I was trying to capture the impact that doing the workaround (which doesn't exist anymore) would only update the status fields that we bring to the update and not overwrite the status fields that exists already with blank fields, etc. (still a useful test)

Changing it to what you suggested provides a clearer understanding though of what the test provides. Negating the negations always 🤯

The inference of the update should include those fields not changing but asserting them in this test demonstrates that update only touching the fields it needs to write to.

[alvaroaleman]

Thanks for fixing this!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, troy0820

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[troy0820]

No problem, does this need to be cherry-picked?

/cherry-pick release-0.16

[k8s-infra-cherrypick-robot]

@troy0820: new pull request created: #2483

In response to this:

No problem, does this need to be cherry-picked?

/cherry-pick release-0.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.