OwnerReferencesPermissionEnforcement ignores pods/status

[derekwaynecarr]

What this PR does / why we need it:
It makes the OwnerReferencesPermissionEnforcement admission plug-in ignore pods/status operations. If a cluster has version skew with its master (1.6) and nodes (1.6-N), the kubelet is doing a pods/status operation and that code path is causing an error because ownerRef has new fields unknown to kubelet.

Release note:

OwnerReferencesPermissionEnforcement admission plugin ignores pods/status.

[derekwaynecarr]

/cc @deads2k @smarterclayton @eparis -- i think this should stop the bleeding we are experiencing right now until we understand the root cause better.

[deads2k]

/cc @deads2k @smarterclayton @eparis -- i think this should stop the bleeding we are experiencing right now until we understand the root cause better.

@derekwaynecarr I'm against this. We haven't determined that the kubelet isn't really mutating it. How about a reproducer that uses node permissions with a different user and then update status without any change via yaml to show it.

Example of a simple story where kubelet would be modifying the ownerRefs.

1. old kubelet does a get, then decodes, resulting in dropping new fields like blockownerdeletion.
2. old kubelet updates status and doesn't send back blockownerdeletion
3. the new api server sees a difference and (properly) rejects in admission, thus saving our metadata from corruption.

Dropping out new metadata fields on status updates because it's deleted practically guarantees GC problems.

[deads2k]

/cc @deads2k @smarterclayton @eparis -- i think this should stop the bleeding we are experiencing right now until we understand the root cause better.

Also, if you just want to clear them out (though getting a reproduction REST request seems really important), simply create a role with the permission to delete pods/status and bind it to the kubelets. Should clear out, but I'd expect that to do bad things to GC since it will allow whatever mutation is happening.

[derekwaynecarr]

@deads2k -- i was under the impression that all nodes had been upgraded in this environment. for your scenario, are you basically saying that old kubelets can no longer communicate to new api servers if this plug-in is enabled? isn't that a problem?

[deads2k]

@deads2k -- i was under the impression that all nodes had been upgraded in this environment. for your scenario, are you basically saying that old kubelets can no longer communicate to new api servers if this plug-in is enabled? isn't that a problem?

It's another reason why #45552 is a good idea. @lavalamp

You could try patching pod/status from the kubelet.

[derekwaynecarr]

@deads2k -- this does appear tied to upgrade scenarios, i think its worth asking if users should expect their 1.5 kubelets to stop working when upgrading to 1.6 masters with n-2 version skew. to properly fix this, we would need patches to 1.4, 1.5, 1.6 kubelets. i think we should evaluate taking this change in addition to getting work lined up to change how kubelet sends status.

[deads2k]

think we should evaluate taking this change in addition to getting work lined up to change how kubelet sends status.

This change allows data corruption. This admission plugin caught a data corruption problem by accident. "Fixing" the admission plugin to allow the current actors to corrupt data is a not a reasonable solution. Something like #45826 would prevent the kubelet from accidentally destroying data like this, but the kubelet should be updated because you may not be able to fix the next one as easily.

[derekwaynecarr]

@deads2k -- telling users to upgrade old kubelets before upgrading masters seems like a non-tenable solution. i agree we need to fix future kubelets, but requiring to upgrade old kubelets appears to me to be a non-starter.

[derekwaynecarr]

@kubernetes/sig-node-bugs --- i added a discussion of the source problem this PR exposed in our next sig meeting. it appears that kubelets cannot safely update status without doing a patch.

[derekwaynecarr]

@deads2k -- i updated this to use the whitelist as discussed.

[deads2k]

/lgtm

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr

Needs approval from an approver in each of these OWNERS Files:

• pkg/registry/OWNERS [deads2k]
• plugin/pkg/admission/OWNERS [derekwaynecarr]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[caesarxuchao]

@derekwaynecarr could you link to the bug that's being fixed?

IIUC, this change can be reverted when the all kubelet are updated to use patch when updating node status, right? Can you open an issue to remind ourselves to remove the whitelist in future releases?

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 45826, 45747, 45548, 45606, 41766)

[eparis]

It was found in https://bugzilla.redhat.com/show_bug.cgi?id=1450461 which i think has basically no public data.I'll see what all I can open.

[k8s-cherrypick-bot]

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.