⚠️ Client: Ensure no stale data remains in target object #1640

alvaroaleman · 2021-08-22T19:38:25Z

The json deserializer of the stdlib and the one from Kube which aims to
be compatible won't zero out all field types in the object it
deserializes into, for example it lets slices be if the json does not
contain that field. This means that if a non-empty variable is used for
any api call with the client, the resulting content might be a mixture
of previous content and what is on the server.

This PR adds a wrapper around the deserializer that will first zero the
target object.

/assign @joelanford @vincepri

Fixes kubernetes-sigs#1639 The json deserializer of the stdlib and the one from Kube which aims to be compatible won't zero out all field types in the object it deserializes into, for example it lets slices be if the json does not contain that field. This means that if a non-empty variable is used for any api call with the client, the resulting content might be a mixture of previous content and what is on the server. This PR adds a wrapper around the deserializer that will first zero the target object.

k8s-ci-robot · 2021-08-22T19:38:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

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

vincepri · 2021-08-23T02:15:39Z

Shouldn't clients make sure that the object passed in is clean?

alvaroaleman · 2021-08-23T02:23:50Z

Shouldn't clients make sure that the object passed in is clean?

Ideally yes, realistically there are a bunch of situation where that might not happen, for example in "Wait for change" loops. An extreme example of this is Update, if for example a mutatingwebhook would patch a slice out, the returned object would still have it (as we re-use the passed-in object to decode the result into).

coderanger · 2021-08-23T02:33:42Z

Is the overhead small enough to handle this unconditionally? We could probably get away with a heuristic like skipping it if .Name == "" but I do like how clean the code is so if we think it's fast enough, maybe just leave it as is.

alvaroaleman · 2021-08-23T03:33:40Z

Is the overhead small enough to handle this unconditionally? We could probably get away with a heuristic like skipping it if .Name == "" but I do like how clean the code is so if we think it's fast enough, maybe just leave it as is.

I haven't done any benchmarking so I don't know how much performance overhead exactly this entails. I did check the generated clients though, which always and uncoditionally allocate a new object to serialize into.

It should also be noted that this only affects the api client. I would think that its generally unlikely someone makes so many mutating request that the performance overhead of this becomes noticeable and for reading requests, it would make sense to use the cache-backed client if one cares about performance. That one doesn't have this issue, as it doesn't deserialize json into the passed object.

vincepri · 2021-08-23T16:05:32Z

Not too worried about performance impacts of this change, but more of masking user errors. This change is definitely a behavioral change in the client code (should be marked as a breaking change), although I'm not sure if we should zero out objects altogether for users.

Shouldn't our users be responsible to either clean out the object or just use a new one?

alvaroaleman · 2021-08-23T16:53:11Z

Not too worried about performance impacts of this change, but more of masking user errors. This change is definitely a behavioral change in the client code (should be marked as a breaking change), although I'm not sure if we should zero out objects altogether for users.

It is a change in behavior (all bugfixes are), but I can not think of an example where someone would want existing data plus the response to be mixed in an object which led me to the conclusion that this is not going to break anyones code, hence it is marked as bugfix.

Shouldn't our users be responsible to either clean out the object or just use a new one?

I don't think we have anything to that effect documented. It also isn't really possible to test this for an existing codebase. I can say that I have written code that re-uses a variable to fetch data to avoid unnecessary allocations (And assuming wrongly that the deserialization would make sure the object gets cleared)

joelanford · 2021-08-23T16:59:30Z

/lgtm

/hold
For @vincepri in case he's got a legit example of this being a breaking change.

alvaroaleman · 2021-08-26T14:56:42Z

Marked it as breaking as it might make tests start failing that (incorrectly) passed before.

vincepri · 2021-08-26T14:56:43Z

/hold cancel

alvaroaleman · 2021-09-08T17:30:45Z

Just testing something...
/cherrypick release-0.9

alvaroaleman · 2021-09-09T13:04:11Z

Lets give this another shot...
/cherrypick release-0.9

alvaroaleman · 2021-09-09T17:32:23Z

Lets try again:
/cherrypick release-0.9

alvaroaleman · 2021-09-09T18:21:49Z

/cherrypick release-0.9

k8s-ci-robot · 2021-09-09T18:22:28Z

@alvaroaleman: new pull request created: #1658

In response to this:

/cherrypick release-0.9

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.

spiffxp · 2021-09-10T20:33:46Z

/cherrypick release-0.9
testing again to see if the new bot is used

spiffxp · 2021-09-10T21:29:39Z

/cherrypick release-0.9
now that kubernetes/test-infra#23556 landed

k8s-infra-cherrypick-robot · 2021-09-10T21:30:18Z

@spiffxp: new pull request created: #1661

In response to this:

/cherrypick release-0.9
now that kubernetes/test-infra#23556 landed

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.

In controller-runtime v0.10.0, the client is updated to clean any stale data in the target object when performing any operation. This results in test failure for the code that constructs an object with both spec and status, and creates the object and updates status it with the same object. The fix is to set the status separately on the object before updating it. Refer: kubernetes-sigs/controller-runtime#1640 Signed-off-by: Sunny <darkowlzz@protonmail.com>

Now that the c-r client zeroes fields before decoding into the object, we can drop our workarounds for this, so basically drop kutil.CreateResetObjectFunc and its usages. ref kubernetes-sigs/controller-runtime#1640

* Upgrade to k8s.io/*@v0.22.2 in go.mod * [automated] make revendor * [automated] make generate * [automated] make revendor github.com/go-openapi/spec seems to be orphaned after previous make generate * Upgrade to c-r@v0.10.2 in go.mod Also, upgrade setup-envtest (doesn't have a tagged release yet, so use release commit instead) * [automated] make revendor * Upgrade to controller-tools@v0.7.0 in go.mod * [automated] make revendor * Add missing WarningsOn{Create,Update} to rest strategies * Replace dot imports for github.com/onsi/gomega/types Fix linting errors: `Assertion` redeclared in this block (typecheck) * Switch to typed values for WebhookInstallOptions.*Webhooks ref kubernetes-sigs/controller-runtime#1626 * RequestCertificate now takes an optional requestedDuration ref kubernetes/kubernetes#99494 * Switch to matchers.DeepEqual to test semantic equality Maps (e.g. labels, selectors, resource requirements) might be sorted differently than expected. Hence, use semantic equality instead of strict equality, as this is what matters to us. Also, DeepEqual outputs yaml and adds a nice diff indicator instead of printing some large confusing go struct representation. * Add new memorySwap field to expected kubelet config ref kubernetes/kubernetes#102823 * Round condition.lastUpdateTime to seconds in test There were several changes in the fake clients that might cause the failure to happen just now. * Correct unit tests falsely succeeding These tests were not preparing the test objects correctly: they only updated them in memory but not on the fake client. This wasn't caught until now because the fake client mimicked the real json decoder, which didn't unset fields not present on the server. Now that the fake client zeroes fields, the tests started failing (which is correct). So fix the tests. ref kubernetes-sigs/controller-runtime#1651 * Remove workarounds for missing zeroing in json decoder Now that the c-r client zeroes fields before decoding into the object, we can drop our workarounds for this, so basically drop kutil.CreateResetObjectFunc and its usages. ref kubernetes-sigs/controller-runtime#1640 * Drop setting webhook gvk explicitly in envtest webhookConfig.SetGroupVersionKind is not needed anymore with kubernetes-sigs/controller-runtime#1665 * Add some follow-up TODO comments * [automated] make generate but with go 1.16.9 * Address review comments

In controller-runtime v0.10.0, the client is updated to clean any stale data in the target object when performing any operation. This results in test failure for the code that constructs an object with both spec and status, and creates the object and updates status it with the same object. The fix is to set the status separately on the object before updating it. Refer: kubernetes-sigs/controller-runtime#1640 Signed-off-by: Sunny <darkowlzz@protonmail.com>

And fix broken unit tests: - elasticsearch/driver.TestUpgradePodsDeletion_WithNodeTypeMutations - elasticsearch/driver.TestUpgradePodsDeletion_Delete - enterprisesearch.TestReconcileEnterpriseSearch_doReconcile_AssociationDelaysVersionUpgrade - license/trial.TestReconcileTrials_Reconcile/valid_trial_secret_inits_trial With: - Set resource version in newTestPod - Do not reuse the same variable to get trial/license secrets - Always SetAssociationConf before doReconcile Because of: - Client: Ensure no stale data remains in target object kubernetes-sigs/controller-runtime#1640 - Fakeclient: Reject Delete with mismatched ResourceVersion kubernetes-sigs/controller-runtime#1582

In controller-runtime v0.10.0, the client is updated to clean any stale data in the target object when performing any operation. This results in test failure for the code that constructs an object with both spec and status, and creates the object and updates status it with the same object. The fix is to set the status separately on the object before updating it. Refer: kubernetes-sigs/controller-runtime#1640 Signed-off-by: Sunny <darkowlzz@protonmail.com>

* Upgrade to k8s.io/*@v0.22.2 in go.mod * [automated] make revendor * [automated] make generate * [automated] make revendor github.com/go-openapi/spec seems to be orphaned after previous make generate * Upgrade to c-r@v0.10.2 in go.mod Also, upgrade setup-envtest (doesn't have a tagged release yet, so use release commit instead) * [automated] make revendor * Upgrade to controller-tools@v0.7.0 in go.mod * [automated] make revendor * Add missing WarningsOn{Create,Update} to rest strategies * Replace dot imports for github.com/onsi/gomega/types Fix linting errors: `Assertion` redeclared in this block (typecheck) * Switch to typed values for WebhookInstallOptions.*Webhooks ref kubernetes-sigs/controller-runtime#1626 * RequestCertificate now takes an optional requestedDuration ref kubernetes/kubernetes#99494 * Switch to matchers.DeepEqual to test semantic equality Maps (e.g. labels, selectors, resource requirements) might be sorted differently than expected. Hence, use semantic equality instead of strict equality, as this is what matters to us. Also, DeepEqual outputs yaml and adds a nice diff indicator instead of printing some large confusing go struct representation. * Add new memorySwap field to expected kubelet config ref kubernetes/kubernetes#102823 * Round condition.lastUpdateTime to seconds in test There were several changes in the fake clients that might cause the failure to happen just now. * Correct unit tests falsely succeeding These tests were not preparing the test objects correctly: they only updated them in memory but not on the fake client. This wasn't caught until now because the fake client mimicked the real json decoder, which didn't unset fields not present on the server. Now that the fake client zeroes fields, the tests started failing (which is correct). So fix the tests. ref kubernetes-sigs/controller-runtime#1651 * Remove workarounds for missing zeroing in json decoder Now that the c-r client zeroes fields before decoding into the object, we can drop our workarounds for this, so basically drop kutil.CreateResetObjectFunc and its usages. ref kubernetes-sigs/controller-runtime#1640 * Drop setting webhook gvk explicitly in envtest webhookConfig.SetGroupVersionKind is not needed anymore with kubernetes-sigs/controller-runtime#1665 * Add some follow-up TODO comments * [automated] make generate but with go 1.16.9 * Address review comments

And fix broken unit tests: - elasticsearch/driver.TestUpgradePodsDeletion_WithNodeTypeMutations - elasticsearch/driver.TestUpgradePodsDeletion_Delete - enterprisesearch.TestReconcileEnterpriseSearch_doReconcile_AssociationDelaysVersionUpgrade - license/trial.TestReconcileTrials_Reconcile/valid_trial_secret_inits_trial With: - Set resource version in newTestPod - Do not reuse the same variable to get trial/license secrets - Always SetAssociationConf before doReconcile Because of: - Client: Ensure no stale data remains in target object kubernetes-sigs/controller-runtime#1640 - Fakeclient: Reject Delete with mismatched ResourceVersion kubernetes-sigs/controller-runtime#1582

k8s-ci-robot assigned joelanford and vincepri Aug 22, 2021

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 22, 2021

k8s-ci-robot requested review from alenkacz and droot August 22, 2021 19:38

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2021

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 23, 2021

alvaroaleman changed the title ~~🐛 Client: Ensure no stale data remains in target object~~ ⚠️ Client: Ensure no stale data remains in target object Aug 26, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 26, 2021

k8s-ci-robot merged commit 8f9d0df into kubernetes-sigs:master Aug 26, 2021

k8s-ci-robot added this to the v0.10.x milestone Aug 26, 2021

alvaroaleman mentioned this pull request Sep 8, 2021

Cherrypicker plugin: Find GitHub account and deploy kubernetes/test-infra#23469

Closed

k8s-ci-robot mentioned this pull request Sep 9, 2021

[release-0.9] ⚠️ Client: Ensure no stale data remains in target object #1658

Closed

alvaroaleman mentioned this pull request Sep 9, 2021

Post http://cherrypicker "connection refused" kubernetes/test-infra#23527

Closed

k8s-infra-cherrypick-robot mentioned this pull request Sep 10, 2021

[release-0.9] ⚠️ Client: Ensure no stale data remains in target object #1661

Closed

sbueringer mentioned this pull request Sep 16, 2021

⚠️ Upgrade to controller-runtime v0.10, controller-tools v0.7 and Kubernetes v1.22 kubernetes-sigs/cluster-api#5249

Merged

This was referenced Oct 1, 2021

Testenv test refactor fluxcd/image-automation-controller#221

Merged

Update controller-runtime to v0.10.1 fluxcd/image-automation-controller#230

Merged

timebertt mentioned this pull request Oct 12, 2021

Upgrade k8s.io/* to v0.22, sigs.k8s.io/controller-runtime to v0.10 gardener/gardener#4772

Closed

6 tasks

thbkrkr mentioned this pull request Nov 17, 2021

Update module sigs.k8s.io/controller-runtime to v0.10.3 elastic/cloud-on-k8s#5038

Merged

1 task

hendrikKahl mentioned this pull request Aug 26, 2022

fix loss of Status subresource information gardener/test-infra#421

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Client: Ensure no stale data remains in target object #1640

⚠️ Client: Ensure no stale data remains in target object #1640

alvaroaleman commented Aug 22, 2021

k8s-ci-robot commented Aug 22, 2021

vincepri commented Aug 23, 2021

alvaroaleman commented Aug 23, 2021

coderanger commented Aug 23, 2021

alvaroaleman commented Aug 23, 2021

vincepri commented Aug 23, 2021

alvaroaleman commented Aug 23, 2021 •

edited

Loading

joelanford commented Aug 23, 2021

alvaroaleman commented Aug 26, 2021

vincepri commented Aug 26, 2021

alvaroaleman commented Sep 8, 2021

alvaroaleman commented Sep 9, 2021

alvaroaleman commented Sep 9, 2021

alvaroaleman commented Sep 9, 2021

k8s-ci-robot commented Sep 9, 2021

spiffxp commented Sep 10, 2021

spiffxp commented Sep 10, 2021

k8s-infra-cherrypick-robot commented Sep 10, 2021

⚠️ Client: Ensure no stale data remains in target object #1640

⚠️ Client: Ensure no stale data remains in target object #1640

Conversation

alvaroaleman commented Aug 22, 2021

k8s-ci-robot commented Aug 22, 2021

vincepri commented Aug 23, 2021

alvaroaleman commented Aug 23, 2021

coderanger commented Aug 23, 2021

alvaroaleman commented Aug 23, 2021

vincepri commented Aug 23, 2021

alvaroaleman commented Aug 23, 2021 • edited Loading

joelanford commented Aug 23, 2021

alvaroaleman commented Aug 26, 2021

vincepri commented Aug 26, 2021

alvaroaleman commented Sep 8, 2021

alvaroaleman commented Sep 9, 2021

alvaroaleman commented Sep 9, 2021

alvaroaleman commented Sep 9, 2021

k8s-ci-robot commented Sep 9, 2021

spiffxp commented Sep 10, 2021

spiffxp commented Sep 10, 2021

k8s-infra-cherrypick-robot commented Sep 10, 2021

alvaroaleman commented Aug 23, 2021 •

edited

Loading