Support exec/attach/portforward in `kubectl proxy` #49534

smarterclayton · 2017-07-25T03:46:11Z

Use the UpgradeAwareProxy shared code in kubectl proxy. Provide a separate transport for those requests that does not have HTTP/2 enabled. Refactor the code to be a bit cleaner in places and to better separate changes.

Fixes #32026

`kubectl proxy` will now correctly handle the `exec`, `attach`, and `portforward` commands.  You must pass `--disable-filter` to the command in order to allow these endpoints.

smarterclayton · 2017-07-25T03:46:26Z

@kubernetes/sig-cli-pr-reviews @ncdc

smarterclayton · 2017-07-25T22:08:27Z

@kubernetes/sig-testing-misc I don't know how to fix bazel here - that's the result of a clean hack/update-bazel on my end, and it looks correct, but can't figure out why it's broken.

cblecker · 2017-07-25T22:16:13Z

Yeah, that's odd. It's passing hack/verify-bazel.sh too in the verify job.

ixdy · 2017-07-25T22:18:34Z

cc @monopole

//pkg/kubectl/util has limited visibility controlled by this rule.

You'll need to add the new package you're adding (//pkg/kubectl/proxy) there.

More details on the visibility rules and motivation in the README.

smarterclayton · 2017-07-26T04:13:15Z

Ah.

ixdy · 2017-07-27T07:16:45Z

/retest

ncdc · 2017-07-27T13:46:26Z

pkg/kubectl/proxy/proxy_server.go

+}
+
+// NewServer creates and installs a new Server.
+// It automatically registers the created Server to http.DefaultServeMux.


I don't think the bit about DefaultServeMux is still accurate?

Correct, will remove

ncdc · 2017-07-27T13:48:27Z

test/e2e/kubectl/kubectl.go

@@ -473,6 +473,43 @@ var _ = SIGDescribe("Kubectl client", func() {
 			}
 		})

+		It("should support exec through kubectl proxy", func() {
+			// Note: We are skipping local since we want to verify an apiserver with HTTPS.
+			// At this time local only supports plain HTTP.


Is this still true?

I have no idea :)

spiffxp · 2017-07-27T16:40:24Z

/test pull-kubernetes-e2e-gce-etcd3

dims · 2017-07-28T01:58:19Z

/retest

smarterclayton · 2017-08-01T00:20:16Z

@ncdc other comments or need another reviewer?

dims · 2017-08-01T11:40:04Z

test/e2e/kubectl/kubectl.go

+				framework.Failf("Unexpected kubectl exec output. Wanted %q, got  %q", expectedExecOutput, output)
+			}
+
+			// // Verify the proxy server logs saw the connection


@smarterclayton do we want to merge with this commented out?

I need to remove this

ncdc · 2017-08-01T13:08:11Z

@smarterclayton no other comments besides what @dims asked. I think @liggitt/@sttts should review too

smarterclayton · 2017-08-05T23:15:50Z

Applying label as noted

smarterclayton · 2017-08-06T02:26:06Z

/retest

smarterclayton · 2017-08-06T21:38:58Z

/retest

smarterclayton · 2017-08-06T23:30:08Z

This broke sometime since my last rebase in master due to an unrelated change (pre-rebase it passes, post it fails). Looking.

smarterclayton · 2017-08-06T23:30:57Z

Broke between 4a8d041 and ae4fac4

smarterclayton · 2017-08-06T23:49:09Z

Broken by #47353 - my theory is a reused kubectl cache, but needs more investigation.

dims · 2017-08-07T13:53:43Z

Looks like a revert is filed! #50254

smarterclayton · 2017-08-08T17:02:36Z

/retest

smarterclayton · 2017-08-08T19:38:36Z

/retest

smarterclayton · 2017-08-08T21:48:57Z

/retest

smarterclayton · 2017-08-09T01:10:20Z

/retest

smarterclayton · 2017-08-09T06:00:27Z

/retest

smarterclayton · 2017-08-09T15:48:13Z

Reapplying label now that the other change was reverted and everything is green

k8s-github-robot · 2017-08-09T16:04:36Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-09T16:53:35Z

Automatic merge from submit-queue

pwittrock · 2017-08-15T00:08:16Z

@smarterclayton Looks like this test has been failing on GKE. Any ideas why? @apelisse is also seeing this test failing on GCE in his new PR to introduce caching for open-api.

liggitt · 2017-08-15T00:14:26Z

@apelisse is also seeing this test failing on GCE in his new PR to introduce caching for open-api.

Previously, the transport used by that caching approach appeared to have issues with websocket requests

liggitt · 2017-08-15T00:15:51Z

Looks like this test has been failing on GKE. Any ideas why?

GKE is the only CI env I'm aware of that uses a tunneling dialer between the apiserver and the kubelets. It's likely related to that.

pwittrock · 2017-08-15T00:20:59Z

@liggitt That is useful thanks. I'll go ask networking.

pwittrock · 2017-08-15T20:49:21Z

Moving debugging conversation to #50466

ckanner · 2017-11-08T02:52:06Z

@smarterclayton I start the proxy like this kubectl proxy --address='0.0.0.0' --port=8011 --accept-hosts='^*$' --disable-filter=true. The I exec the command kubectl --server=http://127.0.0.1:8011 exec -it nginx-4217019353-4bbxl /bin/sh, it occurs the error Error from server (BadRequest): Upgrade request required. How can I solve it?

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 25, 2017

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jul 25, 2017

smarterclayton force-pushed the separate_proxy branch from fb1c155 to 1f4f1e7 Compare July 25, 2017 03:49

k8s-github-robot assigned derekwaynecarr and dims Jul 25, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jul 25, 2017

smarterclayton mentioned this pull request Jul 25, 2017

"kubectl exec" does not work through "kubectl proxy" #32026

Closed

smarterclayton force-pushed the separate_proxy branch 5 times, most recently from acde645 to 5ec02e0 Compare July 25, 2017 22:02

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Jul 25, 2017

ianlewis mentioned this pull request Jul 26, 2017

Pod attach/exec enhancements kubernetes/dashboard#2029

Open

16 tasks

smarterclayton force-pushed the separate_proxy branch from 5ec02e0 to 92296aa Compare July 27, 2017 00:52

ncdc reviewed Jul 27, 2017

View reviewed changes

smarterclayton force-pushed the separate_proxy branch from 92296aa to 29db109 Compare July 27, 2017 21:31

dims reviewed Aug 1, 2017

View reviewed changes

smarterclayton added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 5, 2017

smarterclayton mentioned this pull request Aug 6, 2017

go-client: Use httpcache client for all requests, even though only openapi returns ETags for caching #47353

Merged

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2017

k8s-github-robot merged commit da54959 into kubernetes:master Aug 9, 2017

mengqiy mentioned this pull request Aug 11, 2017

[e2e test failure] Simple pod should support exec through kubectl proxy #50466

Closed

lenartj mentioned this pull request Oct 27, 2017

kubectl proxy doesn't allow WebSockets #25126

Closed

iocanel mentioned this pull request Nov 7, 2017

Exec issues fabric8io/kubernetes-client#832

Merged

errordeveloper mentioned this pull request Feb 15, 2018

kubectl proxy appears to break WebSockets weaveworks/scope#1427

Closed

Support exec/attach/portforward in kubectl proxy #49534

Support exec/attach/portforward in kubectl proxy #49534

Conversation

smarterclayton commented Jul 25, 2017 • edited Loading

smarterclayton commented Jul 25, 2017 • edited Loading

smarterclayton commented Jul 25, 2017

cblecker commented Jul 25, 2017

ixdy commented Jul 25, 2017

smarterclayton commented Jul 26, 2017

ixdy commented Jul 27, 2017

ncdc Jul 27, 2017

Choose a reason for hiding this comment

smarterclayton Jul 27, 2017

Choose a reason for hiding this comment

ncdc Jul 27, 2017

Choose a reason for hiding this comment

smarterclayton Jul 27, 2017

Choose a reason for hiding this comment

spiffxp commented Jul 27, 2017

dims commented Jul 28, 2017

smarterclayton commented Aug 1, 2017

dims Aug 1, 2017

Choose a reason for hiding this comment

smarterclayton Aug 1, 2017

Choose a reason for hiding this comment

ncdc commented Aug 1, 2017

smarterclayton commented Aug 5, 2017

smarterclayton commented Aug 6, 2017

smarterclayton commented Aug 6, 2017

smarterclayton commented Aug 6, 2017

smarterclayton commented Aug 6, 2017

smarterclayton commented Aug 6, 2017

dims commented Aug 7, 2017

smarterclayton commented Aug 8, 2017

smarterclayton commented Aug 8, 2017

smarterclayton commented Aug 8, 2017

smarterclayton commented Aug 9, 2017

smarterclayton commented Aug 9, 2017

smarterclayton commented Aug 9, 2017

k8s-github-robot commented Aug 9, 2017

k8s-github-robot commented Aug 9, 2017

pwittrock commented Aug 15, 2017

liggitt commented Aug 15, 2017

liggitt commented Aug 15, 2017

pwittrock commented Aug 15, 2017

pwittrock commented Aug 15, 2017

ckanner commented Nov 8, 2017

Support exec/attach/portforward in `kubectl proxy` #49534

Support exec/attach/portforward in `kubectl proxy` #49534

smarterclayton commented Jul 25, 2017 •

edited

Loading

smarterclayton commented Jul 25, 2017 •

edited

Loading