Set HTTP upgrade headers when hijacking connection #9652

yadutaf · 2014-12-13T13:32:56Z

When setting up a reverse proxy like Nginx behind Docker client and Docker remote API most commands works fine except docker attach and docker exec. When these commands are used, docker "hijacks" the HTTP connection back to raw bi-directional TCP. As the reverse proxy was expecting plain HTTP it is completely lost.

This Pull Request adds Connection: Upgrade and Upgrade: tcp HTTP headers as well as return code of 101 UPGRADED in this scenario. It is fully backward compatible and, while 'tcp' upgrade is not standard, it is enough to trigger Nginx connection upgrade path as used by websockets.

Signed-off-by: Jean-Tiare Le Bigot <jt@yadutaf.fr>

SvenDowideit · 2014-12-15T07:09:44Z

nice!

jessfraz · 2014-12-15T16:09:25Z

LGTM but I feel like @icecrime would know for sure because he just fixed something with TLS and hijack :)

tiborvass · 2014-12-15T16:11:32Z

@yadutaf would you be able to provide us with a simple way of testing this? :)

LK4D4 · 2014-12-15T16:37:59Z

At least we should try this with docker-py, status code can be hardcoded in clients.

icecrime · 2014-12-15T16:53:56Z

Sounds perfectly reasonable, my only remark would be the following: maybe we should check for the Upgrade header on the server side to avoid sending a 101 for clients who didn't ask for it?

yadutaf · 2014-12-15T19:03:43Z

@tiborvass You can easily test this PR with an Nginx config file looking like:

worker_processes  1; 

events {
    worker_connections  1024;
}

http {
    include       mime.types;
    default_type  application/octet-stream;
    sendfile        on;
    keepalive_timeout  65;

    server {
        listen 9000;

        location / {
            proxy_buffering off;
            proxy_set_header Upgrade $http_upgrade;
            proxy_pass http://localhost:8080;
        }  
    }

}

With Docker daemon listening on port 8080: docker -d -D -H tcp://localhost:8080 and DOCKER_HOST=tcp://localhost:9000.

@icecrime Sounds like a good idea to check for the Upgrade header in request. I added a tentative check, can you review this? This is my first time with Go, it may not be the best way to do it.

icecrime · 2014-12-15T19:14:30Z

api/server/server.go

@@ -887,7 +887,12 @@ func postContainersAttach(eng *engine.Engine, version version.Version, w http.Re

 	var errStream io.Writer

-	fmt.Fprintf(outStream, "HTTP/1.1 200 OK\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n")
+
+	if len(r.Header.Get("Upgrade")) > 0 {


I don't know how we should deal with an empty, yet provided "Upgrade" header field. Anyway, I think a more idiomatic way of checking, which doesn't involve testing the field's value length would be:

if _, ok := r.Header.Get("Upgrade"); ok {

I'll let you check the spec if syntax isn't clear (also, parenthesis around conditions are not required in Go).

Signed-off-by: Jean-Tiare Le Bigot <jt@yadutaf.fr>

yadutaf · 2014-12-15T20:13:12Z

@icecrime thanks for the review. Should be fine now. I also fixed the formating issue.

SvenDowideit · 2014-12-16T02:27:18Z

mmm, it occurs to me this should be documented in the API docs :)

Signed-off-by: Jean-Tiare Le Bigot <jt@yadutaf.fr>

yadutaf · 2014-12-16T23:25:46Z

@SvenDowideit Done.

SvenDowideit · 2014-12-17T00:11:44Z

sweet - Docs LGTM - @fredlf @jamtur01

The docs will need to be moved to a newly created 1.17 API doc, but until that gets made, this version gives us a good idea of what things look like.

LK4D4 · 2014-12-17T00:13:52Z

LGTM

icecrime · 2014-12-17T00:20:11Z

Thanks @yadutaf! LGTM (a.k.a: "yapludtaf")

Set HTTP upgrade headers when hijacking connection

SvenDowideit · 2014-12-19T00:30:19Z

NOTE to sven&fred: please do not merge this into a docs update for 1.4.1 - the API doc needed to be in the 1.17 API doc.

SvenDowideit · 2014-12-19T06:55:37Z

@jfrazelle @LK4D4 this needs to be fixed up in master - as this change is not in 1.4, it should not be documented in the 1.16 API docs.

Because the patch is not in 1.4, the v1.16 docs shouldn't have been updated. Docs were promoted to v1.17 by moby#9742. Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>

Revert doc changes from #9652

vieux · 2014-12-23T00:11:52Z

api/server/server.go

@@ -887,7 +887,11 @@ func postContainersAttach(eng *engine.Engine, version version.Version, w http.Re

 	var errStream io.Writer

-	fmt.Fprintf(outStream, "HTTP/1.1 200 OK\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n")
+	if _, ok := r.Header["Upgrade"]; ok {
+		fmt.Fprintf(outStream, "HTTP/1.1 101 UPGRADED\r\nContent-Type: application/vnd.docker.raw-stream\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n")


Instead of hardcoding tcp here should we look at the Upgrade value in the env ?

It would not make sense. Let's say you have a client telling you "I want websocket" protocol. Then you answer "Sure, here is websocket protocol" while actually sending raw tcp, I fear some debugging nightmare.

If need be, I'd rather add a check on the incoming UPGRADE header to make sure this is something we can speak.

The reason why I did not bother doing it is that this is barely a hint for potential proxies. Docker just ignores it (I mean, It won't affect its behavior in any way beyond sending headers) and Nginx at least will just plainly ignore the value. Actually, If the value had been important here, I'd have used a standard, well known one instead.

Opinion ?

Ok, it makes sense for me to leave it like this. Thanks.

Because the patch is not in 1.4, the v1.16 docs shouldn't have been updated. Docs were promoted to v1.17 by moby#9742. Signed-off-by: Arnaud Porterie <arnaud.porterie@docker.com>

Set HTTP upgrade headers when hijacking connection

9256578

Signed-off-by: Jean-Tiare Le Bigot <jt@yadutaf.fr>

yadutaf force-pushed the master branch from 47bfe82 to f45e9a8 Compare December 15, 2014 19:06

icecrime reviewed Dec 15, 2014
View reviewed changes

Set HTTP upgrade header only when requested by client

c230f70

Signed-off-by: Jean-Tiare Le Bigot <jt@yadutaf.fr>

yadutaf force-pushed the master branch from f45e9a8 to c230f70 Compare December 15, 2014 20:06

SvenDowideit added the /project/doc label Dec 16, 2014

Update connection hijacking documentation with HTTP Upgrade Headers

afb06a3

Signed-off-by: Jean-Tiare Le Bigot <jt@yadutaf.fr>

LK4D4 added a commit that referenced this pull request Dec 17, 2014

Merge pull request #9652 from yadutaf/master

f538e4b

Set HTTP upgrade headers when hijacking connection

LK4D4 merged commit f538e4b into moby:master Dec 17, 2014

yadutaf mentioned this pull request Dec 19, 2014

Start new API v1.17 docs #9742

Merged

icecrime added this to the 1.5.0 milestone Dec 19, 2014

icecrime mentioned this pull request Dec 19, 2014

Revert doc changes from #9652 #9750

Merged

jessfraz pushed a commit that referenced this pull request Dec 22, 2014

Merge pull request #9750 from icecrime/9652_revert_doc

b1cf41d

Revert doc changes from #9652

vieux reviewed Dec 23, 2014
View reviewed changes

SvenDowideit mentioned this pull request Jan 27, 2015

Post 1.4.1 docs update 2 #9739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set HTTP upgrade headers when hijacking connection #9652

Set HTTP upgrade headers when hijacking connection #9652

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Set HTTP upgrade headers when hijacking connection #9652

Set HTTP upgrade headers when hijacking connection #9652

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!