Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api server: patch operation should use patched object to check admission control #19479

Closed
csrwng opened this issue Jan 11, 2016 · 11 comments
Closed
Assignees
Labels
area/api Indicates an issue on api area. area/apiserver area/security priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@csrwng
Copy link
Contributor

csrwng commented Jan 11, 2016

Currently, patch will check admission control with an empty object and if it passes, then will proceed to update the object with the patch. Admission control plugins don't get a chance to see/validate what is actually going to be updated.

@csrwng
Copy link
Contributor Author

csrwng commented Jan 11, 2016

@lavalamp @deads2k

@liggitt
Copy link
Member

liggitt commented Jan 11, 2016

@derekwaynecarr

@deads2k
Copy link
Contributor

deads2k commented Jan 11, 2016

@smarterclayton @kubernetes/goog-csi @kubernetes/rh-cluster-infra is there a group for API server?

@kubernetes/kube-iam this affects field level authorization.

@deads2k deads2k added area/security area/api Indicates an issue on api area. area/apiserver priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jan 11, 2016
@deads2k deads2k added this to the v1.2-candidate milestone Jan 11, 2016
@deads2k
Copy link
Contributor

deads2k commented Jan 11, 2016

I think I want to apply the admission chain twice. Once for patch as-is and once for "update" with the patched object.

This is necessary because admission may mutate the patch itself before its applied, so you can't patch the object ahead of time.

@csrwng
Copy link
Contributor Author

csrwng commented Jan 11, 2016

Why?

@csrwng
Copy link
Contributor Author

csrwng commented Jan 11, 2016

If you check for the patch as is admission control plugins would have to special case objects that are not complete.

@deads2k
Copy link
Contributor

deads2k commented Jan 11, 2016

If you check for the patch as is admission control plugins would have to special case objects that are not complete.

Since the patch itself may mutate during the admission process, the "patched" object created at the beginning of the admission change is not guaranteed to be correct, so it would be incorrect for an admission controller to do any work against the "patched" object.

@csrwng
Copy link
Contributor Author

csrwng commented Jan 11, 2016

It may make sense then to have a separate operation (Patch?) for admission control plugins to explicitly check / mutate patches and not special case complete/incomplete objects on update.

@smarterclayton
Copy link
Contributor

@kubernetes/sig-api-machinery

On Mon, Jan 11, 2016 at 10:54 AM, Cesar Wong notifications@github.com
wrote:

It may make sense then to have a separate operation (Patch?) for admission
control plugins to explicitly check / mutate patches and not special case
complete/incomplete objects on update.


Reply to this email directly or view it on GitHub
#19479 (comment)
.

@deads2k deads2k self-assigned this Jan 11, 2016
@bgrant0607-nocc bgrant0607-nocc added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 11, 2016
@lavalamp
Copy link
Member

Update is a totally separate kind of operation from create, and IMO needs to have a different admission mechanism. Specifically, I think you need an admission call that passes the old object and the proposed object (patched, if appropriate).

Otherwise, you have no way to forbid certain transitions. Just because the initial and final states are independently valid, it does not necessarily mean that the transition between them is allowed.

@smarterclayton
Copy link
Contributor

smarterclayton commented Jan 19, 2016 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. area/apiserver area/security priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

6 participants