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

Updating kubectl to use POST instead of GET for port-forward and exec #10654

Merged
merged 1 commit into from
Jul 6, 2015

Conversation

nikhiljindal
Copy link
Contributor

Fixes #10366

@k8s-bot
Copy link

k8s-bot commented Jul 2, 2015

GCE e2e build/test passed for commit f6bc8140857ee3caf7e183243d5f035653ccd5c7.

@k8s-bot
Copy link

k8s-bot commented Jul 2, 2015

GCE e2e build/test passed for commit b38d8b1.

@nikhiljindal
Copy link
Contributor Author

cc @smarterclayton @bgrant0607 @lavalamp

@smarterclayton You said that we need to test this with HAProxy and an F5 proxy in front of our exec/portforward endpoints.
Can you please point me to how can I test it or can you test it?

Verified that e2e tests basic exec and port-forward: https://github.com/GoogleCloudPlatform/kubernetes/blob/master/test/e2e/kubectl.go#L155

@smarterclayton
Copy link
Contributor

I believe @ncdc did some quick testing.

On Jul 1, 2015, at 11:03 PM, Nikhil Jindal notifications@github.com wrote:

cc @smarterclayton https://github.com/smarterclayton @bgrant0607
https://github.com/bgrant0607 @lavalamp https://github.com/lavalamp

@smarterclayton https://github.com/smarterclayton You said that we need
to test this with HAProxy and an F5 proxy in front of our exec/portforward
endpoints.
Can you please point me to how can I test it or can you test it?

Verified that e2e tests basic exec and port-forward:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/test/e2e/kubectl.go#L155


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

@zmerlynn
Copy link
Member

zmerlynn commented Jul 2, 2015

@jlowdermilk loves exec.

@ncdc
Copy link
Member

ncdc commented Jul 2, 2015

I tested adding support for exec and port forwarding to use http_proxy/https_proxy env vars to be able to use a proxy to reach the master, as they currently don't. That worked against squid (I didn't test proxy auth though). I didn't test this with a reverse proxy in front of the master.

@j3ffml
Copy link
Contributor

j3ffml commented Jul 2, 2015

LGTM. Don't know about the ha/f5 proxy, but jenkins green is good enough for me.

@j3ffml j3ffml added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2015
@smarterclayton
Copy link
Contributor

This is fine since we continue to support GET. LGTM

On Thu, Jul 2, 2015 at 12:53 PM, Jeff Lowdermilk notifications@github.com
wrote:

LGTM. Don't know about the ha/f5 proxy, but jenkins green is good enough
for me.


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

Clayton Coleman | Lead Engineer, OpenShift

@nikhiljindal
Copy link
Contributor Author

@bgrant0607 for the final ok-to-merge

@bprashanth bprashanth added this to the v1.0 milestone Jul 4, 2015
@nikhiljindal
Copy link
Contributor Author

Maybe @zmerlynn or @thockin can give me a second LGTM?

@lavalamp
Copy link
Member

lavalamp commented Jul 5, 2015

Did you manually test? I didn't think there was an e2e test that covered this stuff. (If there is, then consider this an LGTM)

@nikhiljindal
Copy link
Contributor Author

Yes there is an e2e test: https://github.com/GoogleCloudPlatform/kubernetes/blob/master/test/e2e/kubectl.go#L155

I wasnt able to test it locally, since it required some additional softwares to be installed.

@bprashanth
Copy link
Contributor

@lavalamp please apply ok-to-merge if you're fine with the test coverage

@thockin
Copy link
Member

thockin commented Jul 6, 2015

LGTM

Does the serve accept either POST or GET? Or is it currently broken?

@nikhiljindal
Copy link
Contributor Author

Server accepts both POST and GET for now: #10509
Eventually, we will stop supporting GET: #10366

@bgrant0607
Copy link
Member

LGTM
ok to merge

yujuhong added a commit that referenced this pull request Jul 6, 2015
Updating kubectl to use POST instead of GET for port-forward and exec
@yujuhong yujuhong merged commit d5ae819 into kubernetes:master Jul 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet