🐛 Handle unstructured status update with fake client #2495
Conversation
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 <[email protected]>
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
|
/retest looks like a test flake |
|
/retest Did this not get retriggered? Looks like a golangci-lint error. Interesting. |
It did. the first run's flake was a timeout in the leader election code. |
|
/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 |
| @@ -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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, we also added code to zero the target, that is why it works. Thanks for the patch and sorry for the review delay
| @@ -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) | |||
There was a problem hiding this comment.
Ah, we also added code to zero the target, that is why it works. Thanks for the patch and sorry for the review delay
pkg/client/fake/client_test.go
Outdated
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(cl.Update(context.Background(), obj)).To(Succeed()) | ||
| Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(Succeed()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <[email protected]>
|
/test pull-controller-runtime-test |
alvaroaleman
added
the
tide/merge-method-squash
k8s-ci-robot
added
the
lgtm
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/cherry-pick release-0.16 |
|
@troy0820: failed to push cherry-picked changes in GitHub: pushToNamedFork is not implemented in the v2 client In response to this:
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 <[email protected]> * 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 <[email protected]> --------- Signed-off-by: Scott Andrews <[email protected]>
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