🐛 Handle unstructured status update with fake client #2495

scothis · 2023-09-14T20:04:10Z

In order to prevent unintentional mutations outside of the status during a status update, the non-status fields are copied back onto the passed object.

This operation now gracefully handles both unstructured and typed objects. Previously, it would panic if an unstructured object was passed for a GVK known to the scheme, as internally the object within the tracker is converted to the typed equivalent. The two types cannot be directly assigned to each other and instead must be copied.

Adds a failing test to demonstrate the bug, and the fix to make the test pass.

Resolves #2493

In order to prevent unintentional mutations outside of the status during a status update, the non-status fields are copied back onto the passed object. This operation now gracefully handles both unstructured and typed objects. Previously, it would panic if an unstructured object was passed for a GVK known to the scheme, as internally the object within the tracker is converted to the typed equivalent. The two types cannot be directly assigned to each other and instead must be copied. Signed-off-by: Scott Andrews <andrewssc@vmware.com>

scothis · 2023-09-14T20:11:27Z

/retest

looks like a test flake

troy0820 · 2023-09-14T20:49:18Z

/retest

Did this not get retriggered? Looks like a golangci-lint error. Interesting.

scothis · 2023-09-14T20:59:41Z

Did this not get retriggered?

It did. the first run's flake was a timeout in the leader election code.

troy0820 · 2023-09-14T21:06:57Z

/assign @alvaroaleman

Assigning @alvaroaleman as he made the initial change. I'd look to him to give the lgtm. But this looks good to me.

Thanks for the contribution @scothis

alvaroaleman · 2023-09-20T03:13:21Z

pkg/client/fake/client.go

@@ -972,6 +974,19 @@ func copyStatusFrom(old, new runtime.Object) error {
 	return nil
 }

+// copyFrom copies from old into new
+func copyFrom(old, new runtime.Object) error {
+	oldMapStringAny, err := toMapStringAny(old)


The original reason for not doing this anymore is that it might leave stale data in the target. I am confused as to why this isn't a problem anymore. Maybe we just didn't add sufficient test coverage?

This isn't changing existing behavior. No tests were modified or removed. What it does is handle a mix of typed and unstructured objects gracefully, instead of panicing.

What stale data is left behind?

What @alvaroaleman is referring to I believe is that before this function was added, we removed a function similar to this where we were doing a copyFrom because of the issues with that function leaving stale data around.
#2479

#2479 (comment)

If this being added doesn't impact what is already there (covers previous tests and your new test case) and handles the new case to fix the regression, the new test coverage should be sufficient to have this fix be in and resolve the issue with unstructured stuff.

I believe we have unstructured.Unstructured tests cases and if not being caught in testing, it begs the question how did those tests pass, if this problem still presented itself.

IANAM, (I am not a maintainer)

the previous tests and code handled both typed->typed and unstructured->unstructured, but triggered a panic with typed->unstructured. The mix can happen when the GVK is registered in the scheme, so the resource in the tracker is always converted to the typed form, and then the user performs a status update using an unstructured object.

It's a bit esoteric as most users will favor a typed resource when available, but we hit it.

Ah, we also added code to zero the target, that is why it works. Thanks for the patch and sorry for the review delay

alvaroaleman · 2023-09-24T15:19:07Z

pkg/client/fake/client.go

@@ -972,6 +974,19 @@ func copyStatusFrom(old, new runtime.Object) error {
 	return nil
 }

+// copyFrom copies from old into new
+func copyFrom(old, new runtime.Object) error {
+	oldMapStringAny, err := toMapStringAny(old)


Ah, we also added code to zero the target, that is why it works. Thanks for the patch and sorry for the review delay

alvaroaleman · 2023-09-24T15:21:18Z

pkg/client/fake/client_test.go

+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(cl.Update(context.Background(), obj)).To(Succeed())
+		Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(Succeed())


Would you actually mind to do the retrieval and verification using corev1.Pod instead of unstructured.Unstructured? That I believe would simplify the code a bit but more importantly clarify the intention of the testcase. I had to read the title three times to understand it and that is with the context from the PR. Same for the other testcase.

Updated the tests. Sorry for the delay, I was traveling last week.

Using typed objects for the initial and actual object content assertion. Unstructured objects are only used for the update. Signed-off-by: Scott Andrews <andrewssc@vmware.com>

scothis · 2023-10-02T21:39:21Z

/test pull-controller-runtime-test

alvaroaleman

thanks!

k8s-ci-robot · 2023-10-05T00:47:55Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 · 2023-10-05T19:52:21Z

/cherry-pick release-0.16

k8s-infra-cherrypick-robot · 2023-10-05T19:52:59Z

@troy0820: failed to push cherry-picked changes in GitHub: pushToNamedFork is not implemented in the v2 client

In response to this:

/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.

…#2495) * Handle unstructured status update with fake client In order to prevent unintentional mutations outside of the status during a status update, the non-status fields are copied back onto the passed object. This operation now gracefully handles both unstructured and typed objects. Previously, it would panic if an unstructured object was passed for a GVK known to the scheme, as internally the object within the tracker is converted to the typed equivalent. The two types cannot be directly assigned to each other and instead must be copied. Signed-off-by: Scott Andrews <andrewssc@vmware.com> * Review feedback Using typed objects for the initial and actual object content assertion. Unstructured objects are only used for the update. Signed-off-by: Scott Andrews <andrewssc@vmware.com> --------- Signed-off-by: Scott Andrews <andrewssc@vmware.com>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 14, 2023

k8s-ci-robot requested review from joelanford and varshaprasad96 September 14, 2023 20:04

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 14, 2023

scothis mentioned this pull request Sep 14, 2023

Panic in fake client when updating status with Unstructured resource #2493

Closed

k8s-ci-robot assigned alvaroaleman Sep 14, 2023

alvaroaleman reviewed Sep 20, 2023

View reviewed changes

alvaroaleman reviewed Sep 24, 2023

View reviewed changes

Review feedback

42608d3

Using typed objects for the initial and actual object content assertion. Unstructured objects are only used for the update. Signed-off-by: Scott Andrews <andrewssc@vmware.com>

scothis requested a review from alvaroaleman October 4, 2023 14:39

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 5, 2023

alvaroaleman approved these changes Oct 5, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2023

k8s-ci-robot merged commit 067b29f into kubernetes-sigs:main Oct 5, 2023
9 checks passed

scothis deleted the unstructured-status-update branch October 5, 2023 13:48

troy0820 mentioned this pull request Oct 5, 2023

[release-0.16] 🐛 Handle unstructured status update with fake client #2523

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Handle unstructured status update with fake client #2495

🐛 Handle unstructured status update with fake client #2495

scothis commented Sep 14, 2023

scothis commented Sep 14, 2023

troy0820 commented Sep 14, 2023

scothis commented Sep 14, 2023

troy0820 commented Sep 14, 2023

alvaroaleman Sep 20, 2023

scothis Sep 20, 2023 •

edited

troy0820 Sep 20, 2023

scothis Sep 20, 2023

alvaroaleman Sep 24, 2023

alvaroaleman Sep 24, 2023

alvaroaleman Sep 24, 2023

scothis Oct 2, 2023

scothis commented Oct 2, 2023

alvaroaleman left a comment

k8s-ci-robot commented Oct 5, 2023

troy0820 commented Oct 5, 2023

k8s-infra-cherrypick-robot commented Oct 5, 2023

🐛 Handle unstructured status update with fake client #2495

🐛 Handle unstructured status update with fake client #2495

Conversation

scothis commented Sep 14, 2023

scothis commented Sep 14, 2023

troy0820 commented Sep 14, 2023

scothis commented Sep 14, 2023

troy0820 commented Sep 14, 2023

alvaroaleman Sep 20, 2023

Choose a reason for hiding this comment

scothis Sep 20, 2023 • edited

Choose a reason for hiding this comment

troy0820 Sep 20, 2023

Choose a reason for hiding this comment

scothis Sep 20, 2023

Choose a reason for hiding this comment

alvaroaleman Sep 24, 2023

Choose a reason for hiding this comment

alvaroaleman Sep 24, 2023

Choose a reason for hiding this comment

alvaroaleman Sep 24, 2023

Choose a reason for hiding this comment

scothis Oct 2, 2023

Choose a reason for hiding this comment

scothis commented Oct 2, 2023

alvaroaleman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 5, 2023

troy0820 commented Oct 5, 2023

k8s-infra-cherrypick-robot commented Oct 5, 2023

scothis Sep 20, 2023 •

edited