move check for noop managed field timestamp updates

[alexzielenski]

What type of PR is this?

/kind bug

What this PR does / why we need it:

this check needs to go after any mutations. After the mutating admission chain, rest.BeforeUpdate (which is responsible for reverting updates to immutable timestamp fields, among other things.) is called in the store.Update function. This makes it possible for an object to be written to etcd with only a change to its managed fields timestamp.

This PR moves the check for these noop writes deeper in the handler chain, to the after the last point the object is modified.

Which issue(s) this PR fixes:

Fixes #116861

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixes creationTimestamp: null causing unnecessary writes to etcd

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[apelisse]

Fixes #116861

Thanks Alex!
/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 2fa81a38143c06dd5e7c93686f462be5feac2ddb

[lavalamp]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, apelisse, lavalamp

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:

• staging/src/k8s.io/apiserver/OWNERS [lavalamp]
• test/integration/apiserver/OWNERS [alexzielenski,apelisse,lavalamp]

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

[lavalamp]

I'm not sure what stage we are in for the release and if it's OK to dump the milestone label on this or not

[enj]

I'm not sure what stage we are in for the release and if it's OK to dump the milestone label on this or not

@kubernetes/release-managers

[saschagrunert]

I'm not sure what stage we are in for the release and if it's OK to dump the milestone label on this or not

We already released rc.0 yesterday so only exceptional test-fixes should be part of the milestone: https://github.com/kubernetes/sig-release/tree/master/releases/release-1.27#summary

[lavalamp]

@saschagrunert This is kind of an exceptional bug fix, but also the bug has existed for several releases so we can probably just wait and cherry pick for the .1 release. I will assume the latter is your preference unless you say otherwise.

[cici37]

/triage accepted

[pacoxu]

/release-note-none

[lavalamp]

@alexzielenski can you back port this please? :)

[alexzielenski]

/release-note-edit

Fixes creationTimestamp: null triggering unnecessary resource writes

[k8s-ci-robot]

@alexzielenski: /release-note-edit must be used with a release note block.

In response to this:

/release-note-edit

Fixes creationTimestamp: null triggering unnecessary resource writes

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.

[pastequo]

@alexzielenski I think your fix wasn't back ported in 1.27

@apelisse @lavalamp Would it possible to have the other cherry pick approved ? I think without this fix, controllers trying to use SSA are creating an infinite reconcile loop (if they are watching the resources they own and not using "unstructured" client)

[liggitt]

@alexzielenski @apelisse should this be picked to 1.27, and the 1.26 and 1.25 picks reviewed?

[apelisse]

Yeah, absolutely. @alexzielenski would you mind doing it?