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

/exec and /run paths should use POST, not GET #10366

Closed
lavalamp opened this issue Jun 25, 2015 · 9 comments
Closed

/exec and /run paths should use POST, not GET #10366

lavalamp opened this issue Jun 25, 2015 · 9 comments
Assignees
Labels
area/api Indicates an issue on api area. area/kubectl priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node.
Milestone

Comments

@lavalamp
Copy link
Member

GET should never cause a mutation. Otherwise a search engine will pwn you.

Forked from #10351.

@lavalamp lavalamp added sig/node Categorizes an issue or PR as relevant to SIG Node. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Jun 25, 2015
@lavalamp lavalamp added this to the v1.0-candidate milestone Jun 25, 2015
@davidopp
Copy link
Member

You filed this as node, but it's actually master, right?

BTW do you think this is the only 1.0-worth issue mentioned in #10351?

@lavalamp
Copy link
Member Author

I split it off because it's separable. I think the paths are in both master and node, but I need to double check master and I got distracted. :)

@lavalamp
Copy link
Member Author

Yes, there's a /exec on master, too: https://github.com/GoogleCloudPlatform/kubernetes/blob/461fc2b01b29f17b8aebabf463c3fa7fd656d53e/pkg/master/master.go#L467

Also that /portforward should probably also not allow GET.

@davidopp
Copy link
Member

Thanks (and sorry, I meant also master)

@lavalamp
Copy link
Member Author

@ncdc @smarterclayton Question for one of you: if we switch /exec and /portforward to accept POST only (and not GET), will that break any of your tooling?

@nikhiljindal agreed to take this on-- ideally we'll change kubelet & apiserver to accept both, change kubectl to start sending POSTs, and in a few weeks remove GET support from our servers.

@smarterclayton
Copy link
Contributor

It would break our backwards compatibility to clients. Can we preserve that backwards compat via a flag?

On Jun 25, 2015, at 5:37 PM, Daniel Smith notifications@github.com wrote:

@ncdc @smarterclayton Question for one of you: if we switch /exec and /portforward to accept POST only (and not GET), will that break any of your tooling?

@nikhiljindal agreed to take this on-- ideally we'll change kubelet & apiserver to accept both, change kubectl to start sending POSTs, and in a few weeks remove GET support from our servers.


Reply to this email directly or view it on GitHub.

@smarterclayton
Copy link
Contributor

Important note - we still need to pass all params to both as query (since the body is hijacked).

And we're ok with the change, but we need some leve of back compat available for upgrade. The easier we can make that (even if we have to carry a patch for back compat) the better for us. Doesn't have to be a switch.

@bgrant0607 bgrant0607 added area/api Indicates an issue on api area. area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jun 25, 2015
@bgrant0607 bgrant0607 modified the milestones: v1.0, v1.0-candidate Jun 25, 2015
@smarterclayton
Copy link
Contributor

I remember what the concern was - whether most proxies would properly support connection upgrade on a POST. We haven't extensively tested whether POST would work, but we require support for at least HAProxy and an F5 proxy in front of our exec/portforward endpoints. So we probably need to test that.

----- Original Message -----

It would break our backwards compatibility to clients. Can we preserve that
backwards compat via a flag?

On Jun 25, 2015, at 5:37 PM, Daniel Smith notifications@github.com wrote:

@ncdc @smarterclayton Question for one of you: if we switch /exec and
/portforward to accept POST only (and not GET), will that break any of
your tooling?

@nikhiljindal agreed to take this on-- ideally we'll change kubelet &
apiserver to accept both, change kubectl to start sending POSTs, and in a
few weeks remove GET support from our servers.


Reply to this email directly or view it on GitHub.

@smarterclayton
Copy link
Contributor

We have to support GET for web sockets in browsers. There is no other way for a browser to open a websocket to exec or logs. Logs are extremely valuable, exec will be more valuable.

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/kubectl priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node.
Projects
None yet
Development

No branches or pull requests

6 participants