Pod attach/exec proto #1939

lenartj · 2017-05-12T00:32:13Z

Prototype -- please comment extensively. This is building on code/ideas/comments from #1455 and #1345, so some of the problems/suggestions mentioned there are addressed.

Task list:

I have been running it like this:
DOCKER_RUN_OPTS="-v $HOME/.kube/config:/kubeconfig.yaml:ro -e KUBE_DASHBOARD_KUBECONFIG=/kubeconfig.yaml" build/run-gulp-in-docker.sh serve

codecov-io · 2017-05-14T17:46:00Z

Codecov Report

Merging #1939 into master will decrease coverage by 1.16%.
The diff coverage is 8.88%.

@@            Coverage Diff             @@
##           master    #1939      +/-   ##
==========================================
- Coverage    60.5%   59.33%   -1.17%     
==========================================
  Files         537      543       +6     
  Lines       11186    11445     +259     
==========================================
+ Hits         6768     6791      +23     
- Misses       4246     4482     +236     
  Partials      172      172

Impacted Files	Coverage Δ
src/app/backend/handler/terminal.go	`0% <0%> (ø)`
src/app/frontend/shell/module.js	`100% <100%> (ø)`
src/app/frontend/index_module.js	`100% <100%> (ø)`	⬆️
src/app/backend/handler/apihandler.go	`26.23% <14.81%> (-0.18%)`	⬇️
src/app/frontend/shell/termopen_directive.js	`25% <25%> (ø)`
src/app/frontend/shell/controller.js	`5.76% <5.76%> (ø)`
...app/frontend/pod/detail/containerinfo_component.js	`84.61% <50%> (-6.3%)`	⬇️
src/app/frontend/shell/stateconfig.js	`57.14% <57.14%> (ø)`
src/app/frontend/shell/state.js	`75% <75%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f534482...958ffc5. Read the comment docs.

lenartj · 2017-05-15T14:06:32Z

@ianlewis, @floreks, @maciaszczykm, @rf232, @bryk, @jwforres, @beyondblog, @Dmitry1987, if you have a some time your comments would be much appreciated. I hope I haven't left out anyone who seem to have shown interest previously.. Thanks

cheld · 2017-05-15T15:05:58Z

I just tested it with gulp serve and gulp serve:prod. Cool. The attach function is a bit hidden, but thats a detail... Had no chance yet to look at the code.

lenartj · 2017-05-16T09:26:35Z

Update: I'm working on adding a SockJS based transport to circumvent the lack of WebSocket support in the kubectl proxy. (SockJS is using WebSockets if possible and falling back on a number of different alternatives otherwise. Some of those can pass trough kubectl proxy fine.)

It might be a good idea anyway as not all corp proxies fancy WebSockets.

maciaszczykm · 2017-05-16T09:31:23Z

@lenartj Good idea, I am going to take a look on it after release too.

lenartj · 2017-05-17T21:22:12Z

Update: the SockJS implementation works fine. I will try to push the code tomorrow after some cleanup.

floreks · 2017-05-18T08:50:38Z

I have quickly looked at the code and overall it looks fine. Probably we'll have some comments about where to place the code. You can continue for now. Once it's ready for review just write PTAL.

lenartj · 2017-05-20T17:49:58Z

@floreks, PTAL - also, I have updated the tasks in the first comment, please take a look.

For everyone else: If you want to give this code a try you can do (k8s 1.6+):
kubectl apply -f https://lenart.io/kubernetes-dashboard-head-attach-proto.yaml

If you are running kubectl proxy visit the address http://127.0.0.1:8001/api/v1/namespaces/kube-system/services/kubernetes-dashboard-head/proxy/

I have tested this using the following setups:

Chrome on Linux, via kubectl proxy (transport=xhr stream)
Chrome on Linux, trough an ingress (secured with mandatory client certificate; transport=websocket)
Chrome on Android (trough the ingress; transport=websocket)

floreks · 2017-05-24T13:57:23Z

Sorry for the delay. I will take a look at it tomorrow.

lenartj · 2017-05-26T07:30:21Z

@floreks, would it help if I cut this PR into 2-3 separate PRs?

floreks

I have few comments. Also please add some documentation. For all public functions in go we need to have documentation. It would be nice to have it also for private functions but if logic is simple then it is not required.

Mostly it looks nice. I've tested few cases and it works well. Thanks again for this contribution. It is really great!

floreks · 2017-05-26T08:03:16Z

src/app/backend/handler/apihandler.go

+ podName := request.PathParameter("pod")
+ containerName := request.PathParameter("container")
+
+ req := getApiClient(request).Core().RESTClient().Post().


As this is related to pod I'd move this logic to i.e. attach.go or terminal.go file under pod resource. As this is actually exec then maybe we can keep it under some generic name like terminal/console.

floreks · 2017-05-26T08:31:53Z

src/app/backend/handler/attach.go

+ return 0, err
+ }
+
+ var msg struct {


Refactor structs outside of the functions.

floreks · 2017-05-26T08:32:03Z

src/app/backend/handler/attach.go

+var attachSessions = make(map[string]AttachSession)
+
+func handleAttachSession(session sockjs.Session) {
+ if buf, err := session.Recv(); err == nil {


I'd reverse the logic here to decrease complexity.

if ...; err != nil { log(err) return }

Also in case of any error we can probably just retun immediately, then last part does not have to be in else block.

floreks · 2017-05-26T08:32:08Z

src/app/frontend/attach/controller.js

+ // https://github.com/sockjs/sockjs-client
+
+ this.term = new hterm.Terminal();
+ this.term.onTerminalReady = function() {


I'd extract anonymous functions to class functions. It will be easier to document and understand the code.

By the way instead of function() {} you can use ES6 fat arrow syntax () => {}.

floreks · 2017-05-26T08:35:48Z

@maciaszczykm could you also take a closer look at it? I may have missed something.

lenartj · 2017-05-26T09:19:17Z

Very useful comments. I will get to it in the coming few days.

lenartj · 2017-05-28T11:07:09Z

@floreks, PTAL. Read the commit message for info. Mostly just cosmetical changes but touches a lot of lines.

maciaszczykm · 2017-05-29T12:51:58Z

I have just checked out how it works and I really like it.

One minor thing for a quick fix related to styling:

We should apply padding here, just like in logs view:

I will take a look on the code soon.

maciaszczykm · 2017-05-29T12:55:34Z

We should also add links leading to exec view (something like logs button), but we can work on it later on:

maciaszczykm

Works okay, but I have few comments regarding code itself. We can chat on Slack if something is not clear.

maciaszczykm · 2017-05-29T13:00:53Z

build/serve.js

@@ -19,9 +19,8 @@ import browserSync from 'browser-sync';
 import browserSyncSpa from 'browser-sync-spa';
 import child from 'child_process';
 import gulp from 'gulp';
+import proxyMiddleware from 'http-proxy-middleware';


You should run gulp format.

maciaszczykm · 2017-05-29T13:02:12Z

src/app/backend/handler/apihandler.go

@@ -95,6 +96,10 @@ type CsrfToken struct {
 Token string `json:"token"`
 }

+type TerminalResponse struct {


Please add comments on big-case types and functions.

maciaszczykm · 2017-05-29T13:08:21Z

src/app/backend/handler/terminal.go

+
+// Called from apihandler.handleAttach as a goroutine
+// Waits for the SockJS connection to be opened by the client the session to be bound in handleTerminalSession
+func TerminalWaiter(request *restful.Request, sessionId string) {


Function name should be verb, not a noun.

maciaszczykm · 2017-05-29T13:09:35Z

src/app/backend/handler/terminal.go

+ "k8s.io/kubernetes/pkg/api"
+)
+
+// What remotecommand expects from a pty


Please start comments with type/method name. In this case PtyHandler ....

maciaszczykm · 2017-05-29T13:10:28Z

src/app/backend/handler/terminal.go

+}
+
+type TerminalMessage struct {
+ Op, Data, SessionID string


Can you explain it a little? What is Op etc.?

maciaszczykm · 2017-05-29T13:21:28Z

src/app/backend/handler/terminal.go

+// Called from main for /api/sockjs
+func CreateAttachHandler(path string) (http.Handler) {
+ options := sockjs.DefaultOptions
+ // options.Websocket = false


Do we need it?

I am not sure if if you are asking about options or the whole function. I've removed the variable; it was just a leftover from experiments.

If you meant the function itself: I think it's a good idea to abstract this implementation detail out of the main package (dashboard.go). It is also how the other handlers are setup:

http.Handle("/", handler.MakeGzipHandler(handler.CreateLocaleHandler())) http.Handle("/api/", apiHandler) // TODO(maciaszczykm): Move to /appConfig.json as it was discussed in #640. http.Handle("/api/appConfig.json", handler.AppHandler(handler.ConfigHandler)) http.Handle("/api/sockjs/", handler.CreateAttachHandler("/api/sockjs")) http.Handle("/metrics", prometheus.Handler())

Just about the single line :-) It's good.

maciaszczykm · 2017-05-29T13:23:38Z

src/app/backend/handler/terminal.go

+}
+
+// Checks if the shell is an allowed one
+func isValidShell(validShells, shell []string) bool {


Why is shell an array? Before call you do []string{request.QueryParameter("shell")} which is redundant.

I am thinking of commands like tmux att. Maybe over-engineered?

maciaszczykm · 2017-05-29T13:28:09Z

src/app/backend/handler/terminal.go

+// Called from apihandler.handleAttach as a goroutine
+// Waits for the SockJS connection to be opened by the client the session to be bound in handleTerminalSession
+func TerminalWaiter(request *restful.Request, sessionId string) {
+ shell := []string{request.QueryParameter("shell")}


Use shell := request.QueryParameter("shell") instead.

maciaszczykm · 2017-05-29T13:29:55Z

src/app/backend/handler/terminal.go

+
+ if shell[0] != "" {
+ if isValidShell(validShells, shell) {
+ err = execHelper(request, shell, terminalSessions[sessionId])


If you need to pass an array do it here instead of using array everywhere.

Also, use different name, because using shell everywhere is confusing.

Yes using shell in the loop as well is pretty confusing. I've changed the latter.. Is it any better?

maciaszczykm · 2017-05-29T13:34:38Z

src/app/backend/handler/terminal.go

+ var err error
+ validShells := []string{"bash", "sh"}
+
+ if shell[0] != "" {


Instead of doing this check check if isValidShell(validShells, shell), else try some shells until one succeeds or all fail. Then, we don't need to throw err = fmt.Errorf("Invalid shell").

lenartj · 2017-05-29T15:25:08Z

I've experimented with a couple of different paddings:

Around: padding on the right of the scrollbar looks really odd
padding-left only, black: the actual background color of hterm is not quite black, but #f0f0f0 so black looks odd next to it
padding-left only, same as hterm

... but now any process that changes the background color will have an almost black line next to it:
padding-left only, color inherited (whiteish: this looks the least odd, but doesn't accomplish much in my opinion:

All in all, I am not a fan of any of the paddings above so I haven't added any for now.

lenartj · 2017-05-30T14:52:44Z

I've removed the padding from the left. It turns out that the width of the black vertical bar on the right depends on the exact the width of the box. Something like: rightpaddingwidth = totalwidth%charwidth, so it can be 0 depending on the width.

cheld · 2017-06-02T19:47:18Z

FYI: the old one, I almost forgot https://github.com/kubernetes-ui/container-terminal

adrianchifor · 2017-06-04T18:27:20Z

src/app/backend/handler/terminal.go

+// Can happen if the process exits or if there is an error starting up the process
+// For now the status code is unused and reason is shown to the user (unless "")
+func (t TerminalSession) Close(status uint32, reason string) {
+ t.sockJSSession.Close(status, reason)


session.Close() can return an error which is currently unhandled, crashing the dashboard. Might be good for this Close function to pass the error down to usages if it's not nil, like in line 261 and 265.

Yes, this could/should be done, but it won't fix a crash. The crash happens when a Read() happens on the closed session. I will look into this, thanks

floreks · 2017-06-05T12:23:11Z

I think you have missed @maciaszczykm comment.

Also you need to rebase code. Rest LGTM. Last changes and we are ready to merge.

lenartj · 2017-06-05T12:28:39Z

@maciaszczykm, @floreks, I must be blind, I can't find the comment I've missed (I have followed the link too)

Do you mean this one?

Use shell := request.QueryParameter("shell") instead.

But to this I have replied/explained and have received no further comment (or maybe just can't find it?)

floreks · 2017-06-05T12:47:40Z

Do you mean this one?

Use shell := request.QueryParameter("shell") instead.

hmm... I don't see any reply to this comment. Link is valid and I can see a comment from @maciaszczykm (scroll a bit up if you can't see it).

maciaszczykm · 2017-06-05T13:23:26Z

I may also missed one of the replies, but generally shell := []string{request.QueryParameter("shell")} doesn't look good especially when in isValidShell you need to do shell[0]. In my opinion isValidShell should accept single string, not array and extract last one. You should change that. I think @floreks will agree with me.

lenartj · 2017-06-05T13:24:57Z

Loud and clear. I fix that.

- github.com/gorilla/websocket - gopkg.in/igm/sockjs-go.v2/sockjs - k8s.io/kubernetes/pkg/client/unversioned/remotecommand

floreks

LGTM. We can improve it in subsequent PRs.

Comments resolved.

moon03432 · 2017-07-07T02:36:28Z

@lenartj Thanks for your great feature! I deployed it in k8s 1.6.1 with
kubectl apply -f https://lenart.io/kubernetes-dashboard-head-attach-proto.yaml

And when I want to exec in the container "kubernetes-dashboard-head" itself, an error occurred and the dashboard crashed and restarted. Is that expected?

Dashboard crashes when an oci runtime error occurs. Maybe this error should be handled.

floreks · 2017-07-07T06:12:35Z

Not every container supports exec into it. It depends on what base image it was created. Dashboard does not support it. When you try to exec into it, it will crash.

Sent from my LGE Pixel using FastHub

cheld · 2017-07-07T07:26:36Z

Dashboard container has no shell and as @floreks explained exec into it will not work. However, the running Dashboard instance should not crash and it isn't in my environment. Are you using roket? Anything else special in your environment?

lenartj · 2017-07-07T11:31:08Z

Sorry I only noticed this comment now. I will be able to take a look on Wednesday, I am busy until that :/

moon03432 · 2017-07-19T11:19:04Z

I'm just using docker 1.12.6, nothing special

Kaijun · 2017-08-03T08:14:41Z

Hmm, it seems that the dashboard pod will exit with error if you're executing a non-existing command.

floreks · 2017-08-03T08:21:12Z

This is a different issue related to session handling. We are working on a fix.

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

lenartj force-pushed the attach-proto branch 5 times, most recently from ab5eb75 to 45a478b Compare May 14, 2017 17:35

maciaszczykm requested review from floreks and maciaszczykm May 15, 2017 14:10

maciaszczykm assigned maciaszczykm and floreks May 17, 2017

lenartj force-pushed the attach-proto branch from 45a478b to 941923e Compare May 20, 2017 17:50

floreks requested changes May 26, 2017

View reviewed changes

maciaszczykm previously requested changes May 29, 2017

View reviewed changes

lenartj closed this May 29, 2017

floreks mentioned this pull request Jun 1, 2017

Exec into pod prototype #1455

Closed

maciaszczykm added the priority/P0 label Jun 2, 2017

danielromlein added this to the 2017 roadmap milestone Jun 2, 2017

danielromlein mentioned this pull request Jun 2, 2017

Exec into Pod #1345

Closed

4 tasks

adrianchifor reviewed Jun 4, 2017

View reviewed changes

lenartj added 2 commits June 14, 2017 08:25

Added go vendor dependencies for shell module

748c9a6

- github.com/gorilla/websocket - gopkg.in/igm/sockjs-go.v2/sockjs - k8s.io/kubernetes/pkg/client/unversioned/remotecommand

Added shell module

958ffc5

lenartj force-pushed the attach-proto branch from 05975e7 to 958ffc5 Compare June 14, 2017 09:01

floreks mentioned this pull request Jun 16, 2017

Pod attach/exec enhancements #2029

Open

16 tasks

floreks approved these changes Jun 16, 2017

View reviewed changes

floreks merged commit d8f28e8 into kubernetes:master Jun 16, 2017

floreks mentioned this pull request Jun 16, 2017

Add missing module #2031

Merged

lenartj deleted the attach-proto branch June 16, 2017 18:49

maciaszczykm mentioned this pull request Jul 4, 2017

execution commands on pod's containers - feature request #2082

Closed

jamieklassen mentioned this pull request May 30, 2023

🚀 Feature: shell into pods backstage/backstage#18024

Closed

2 tasks

Pod attach/exec proto #1939

Pod attach/exec proto #1939

Conversation

lenartj commented May 12, 2017 • edited Loading

codecov-io commented May 14, 2017 • edited by codecov bot Loading

Codecov Report

lenartj commented May 15, 2017

cheld commented May 15, 2017

lenartj commented May 16, 2017

maciaszczykm commented May 16, 2017

lenartj commented May 17, 2017

floreks commented May 18, 2017

lenartj commented May 20, 2017 • edited Loading

floreks commented May 24, 2017

lenartj commented May 26, 2017

floreks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

floreks commented May 26, 2017

lenartj commented May 26, 2017

lenartj commented May 28, 2017

maciaszczykm commented May 29, 2017 • edited Loading

maciaszczykm commented May 29, 2017

maciaszczykm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenartj commented May 29, 2017

lenartj commented May 30, 2017 • edited Loading

cheld commented Jun 2, 2017

Choose a reason for hiding this comment

lenartj Jun 4, 2017 • edited Loading

Choose a reason for hiding this comment

floreks commented Jun 5, 2017

lenartj commented Jun 5, 2017

floreks commented Jun 5, 2017 • edited Loading

maciaszczykm commented Jun 5, 2017

lenartj commented Jun 5, 2017

floreks left a comment

Choose a reason for hiding this comment

moon03432 commented Jul 7, 2017 • edited Loading

floreks commented Jul 7, 2017

cheld commented Jul 7, 2017 • edited Loading

lenartj commented Jul 7, 2017

moon03432 commented Jul 19, 2017

Kaijun commented Aug 3, 2017 • edited Loading

floreks commented Aug 3, 2017

lenartj commented May 12, 2017 •

edited

Loading

codecov-io commented May 14, 2017 •

edited by codecov bot

Loading

lenartj commented May 20, 2017 •

edited

Loading

maciaszczykm commented May 29, 2017 •

edited

Loading

lenartj commented May 30, 2017 •

edited

Loading

lenartj Jun 4, 2017 •

edited

Loading

floreks commented Jun 5, 2017 •

edited

Loading

moon03432 commented Jul 7, 2017 •

edited

Loading

cheld commented Jul 7, 2017 •

edited

Loading

Kaijun commented Aug 3, 2017 •

edited

Loading