Explicitly disable dry run for connect #66083
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
cncf-cla: yes
|
@kubernetes/sig-api-machinery-pr-reviews |
| @@ -98,6 +98,11 @@ func (scope *RequestScope) AllowsStreamSchema(s string) bool { | |||
| // ConnectResource returns a function that handles a connect request on a rest.Storage object. | |||
| func ConnectResource(connecter rest.Connecter, scope RequestScope, admit admission.Interface, restPath string, isSubresource bool) http.HandlerFunc { | |||
| return func(w http.ResponseWriter, req *http.Request) { | |||
| if isDryRun(req.URL) { | |||
There was a problem hiding this comment.
Hi @jennybuckley , if we were to forbid dryRun requests for all verbs, why not do this via an HTTP handler filter. 😃
There was a problem hiding this comment.
Thanks for reviewing this! The reason is that we plan on enabling it for some verbs in the future, depending on whether or not the dry run feature is enabled. Also, some verbs, like GET, don't make any change in the cluster, so dry running a get is equivalent to just running the get normally, so we don't need to fail if the client sends the dry run flag.
k8s-ci-robot
added
release-note
|
cc @kubernetes/api-reviewers |
|
I'm definitely in favor of this for pods/exec, pods/attach, pods/portforward, etc. Query parameters on proxy subresources were previously opaque to the apiserver. Which seems more appropriate for the proxy subresources:
|
|
I think I'd like to err on the side of rejecting all dryRun requests to these escape hatches. It is just code, we can relax the rules if someone comes up with a compelling use case. Right now dry run requests don't succeed for anything, so I think it's fine if they continue to not succeed here. /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jennybuckley, 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/test all Tests are more than 96 hours old. Re-running tests. |
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 66512, 66946, 66083). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
It isn't clear whether or not dry run would work on connect. I think we should explicitly disable it so no one can try to dry run a connect request and accidentally open a connection.
Release note: