Fix FieldManager Conversion Error for CRD Updates

[kwiesmueller]

What type of PR is this?
/kind bug

What this PR does / why we need it:

When handling a crd update, the fieldManager sometimes fails conversion of the live object an error like:
failed to update object (Update for openfaas.com/v1, Kind=Function) managed fields: failed to convert live object to proper version: /, Kind= is unstructured and is not suitable for converting to "openfaas.com/v1"

This seems to be caused by the fieldManager receiving a liveObject that is empty.

The current approach changes the Update call to not call the UpdateHandler if there is no existing object and createOnUpdate is not allowed.
This way the fieldManager never gets called.

Which issue(s) this PR fixes:

Partial fix for #90904

NONE

/sig api-machinery
/wg api-expression
/cc @apelisse @liggitt

[kwiesmueller]

/retest

[kwiesmueller]

/retest

[liggitt]

one nit on the test to output helpful things if we don't get the error we expected

Reverting the non-test portions of #91690 (which I think was the plan for 1.19?) lets this test exercise the failure condition and produces an unknown error without the accompanying store change. Are we ready to do that?

[kwiesmueller]

When reverting #91690 the test here still won't fail.
Right now, the 404 would also be returned without the change in this PR as #91690 makes the update ignore the error, so storage can proceed until the notFound gets returned.
So the test here is not testing the change directly, but the situation we want which currently both changes achieve separately.
At least that was the result of me reverting #91690 yesterday. Does that make sense?

[fedebongio]

/assign @liggitt @apelisse

[liggitt]

wait.go:211: unexpected response, will retry: nodes "node2" not found
node_test.go:483: Expected forbidden error, got nodes "node2" not found

This line is trying to update a node that doesn't exist with a different node's client, which would have been rejected by admission previously and now receives a NotFound error. This is expected because of the order change, but represents a behavior change I probably wouldn't expect us to backport.

To fix this test, you could change line 483 to:

expectNotFound(t, updateNode2Status(node1Client))

and add this after the // self deletion is not allowed test:

// self deletion is not allowed
expectForbidden(t, deleteNode2(node2Client))
// modification of another node's status is not allowed
expectForbidden(t, updateNode2Status(node1Client))

[kwiesmueller]

Thanks Jordan!
I fixed the test. Didn't expect us to backport this change anyways and as we already backported the ignore PR i don't think it's necessary as the bug is bypassed by it.

[kwiesmueller]

/retest

[liggitt]

thanks, please squash, then lgtm

[liggitt]

/approve
/priority important-soon

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwiesmueller, liggitt

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 [liggitt]
• test/OWNERS [liggitt]

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

[apelisse]

Thanks,
/lgtm

[dims]

/hold Temporary hold to get prow/tide to get back on its feet. Feel free to remove hold in a few hours.

[dims]

/hold cancel

[dims]

/hold Temporary hold to get prow/tide to get back on its feet. Feel free to remove hold in a few hours.

[dims]

/test all

[dims]

/hold cancel

[kwiesmueller]

/retest

[dims]

/retest

[kwiesmueller]

/retest

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.