-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
drop rest.ConnectRequest in webhook admission #66633
Conversation
Instead of trying to convert them to something they are not. This mitigates kubernetes#59759 by turning an INTERNAL error into a successful admission request that omits some of the request information. Eventually we will probably want to forward CONNECT options, but they aren't currently versioned.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mikedanese If they are not already assigned, you can assign the PR to them by writing 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:
Approvers can indicate their approval by writing |
@mikedanese: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
What object do in-tree admission plugins get in admission attributes for these requests? |
they get *rest.ConnectRequests: kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go Lines 125 to 138 in f1b6611
e.g.:
|
/cc @caesarxuchao |
@mikedanese I'm working on a complete fix at #66807. I tried locally it seemed to work. |
Let's go with #66807 |
Automatic merge from submit-queue (batch tested with PRs 66196, 67016, 66807, 67023). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Make admission webhooks conversion convert CONNECT body correctly Fix #59759. 1. Make apiserver pass connectRequest.Options directly to the admission layer. All other information in rest.ConnectRequest is present in admission attributes. 2. Make the scope.Kind of pod/attach, pod/exec, pod/portforward, node/proxy, service/proxy to their respective options Kind, instead of the parent Kind. I've tested it locally, the conversion is working correctly for "kubectl attach". I'll add e2e tests. I'll keep this to myself until I add the tests, but cc @mikedanese @liggitt RE. #66633.
Automatic merge from submit-queue (batch tested with PRs 66196, 67016, 66807, 67023). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Make admission webhooks conversion convert CONNECT body correctly Fix #59759. 1. Make apiserver pass connectRequest.Options directly to the admission layer. All other information in rest.ConnectRequest is present in admission attributes. 2. Make the scope.Kind of pod/attach, pod/exec, pod/portforward, node/proxy, service/proxy to their respective options Kind, instead of the parent Kind. I've tested it locally, the conversion is working correctly for "kubectl attach". I'll add e2e tests. I'll keep this to myself until I add the tests, but cc @mikedanese @liggitt RE. kubernetes/kubernetes#66633. Kubernetes-commit: d1636b8019fa042eb1135263b00293b1c806b1d7
Automatic merge from submit-queue (batch tested with PRs 66196, 67016, 66807, 67023). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Make admission webhooks conversion convert CONNECT body correctly Fix #59759. 1. Make apiserver pass connectRequest.Options directly to the admission layer. All other information in rest.ConnectRequest is present in admission attributes. 2. Make the scope.Kind of pod/attach, pod/exec, pod/portforward, node/proxy, service/proxy to their respective options Kind, instead of the parent Kind. I've tested it locally, the conversion is working correctly for "kubectl attach". I'll add e2e tests. I'll keep this to myself until I add the tests, but cc @mikedanese @liggitt RE. kubernetes/kubernetes#66633. Kubernetes-commit: d1636b8019fa042eb1135263b00293b1c806b1d7
Instead of trying to convert them to something they are not.
This mitigates #59759 by turning an INTERNAL error into a successful
admission request that omits some of the request information. Eventually
we will probably want to forward CONNECT options, but they aren't
currently versioned.
cc @cjcullen @alexcope @kubernetes/sig-api-machinery-pr-reviews