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

net/http/httputil: add WebSocket support to ReverseProxy #26937

Closed
bradfitz opened this issue Aug 12, 2018 · 36 comments
Closed

net/http/httputil: add WebSocket support to ReverseProxy #26937

bradfitz opened this issue Aug 12, 2018 · 36 comments
Assignees
Milestone

Comments

@bradfitz
Copy link
Member

@bradfitz bradfitz commented Aug 12, 2018

Add WebSocket support to ReverseProxy.

I have code for this that I use myself in various projects, but I keep forgetting to add it to Go.

@bradfitz bradfitz added the NeedsFix label Aug 12, 2018
@bradfitz bradfitz added this to the Go1.12 milestone Aug 12, 2018
@bradfitz bradfitz self-assigned this Aug 12, 2018
@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Aug 12, 2018

This would need to be off by default in case current clients rely on the lack of support to prevent websocket connections.

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Aug 12, 2018

@nhooyr, that seems like a stretch. We'd document it in the release notes and say how to disable it if people wanted to. But if the two sides negotiate it and the Go user expressed their intent to wire up the two sides with a ReverseProxy, I don't think it's crazy to say we'd wire it up all the way.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Aug 12, 2018

@bradfitz Fair enough. Its extremely unlikely anyone would not want WebSockets support anyway.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Aug 12, 2018

@bradfitz Could you post the code for this, I've written something similar myself, want to make sure I'm doing it right.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Aug 12, 2018

We should also support arbitrary upgrades instead of just WebSockets.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Aug 12, 2018

@bradfitz In fact, if you post it, I'd love to work on the CL for this.

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Aug 12, 2018

@nhooyr, I'll send it myself, as I already have the code. It won't save me any time to hand it to you to hand back to me. You can review it once it's on Gerrit, though.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 24, 2018

Change https://golang.org/cl/131279 mentions this issue: net/http: make Transport return Writable Response.Body on WebSocket upgrade

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Aug 24, 2018

I decided to implement this a different way, not using my existing code. Instead, I sent https://golang.org/cl/131279 to add Transport support for WebSockets, so the ReverseProxy code won't need to separately dial the backend.

gopherbot pushed a commit that referenced this issue Aug 25, 2018
…itch

Updates #26937
Updates #17227

Change-Id: I79865938b05c219e1947822e60e4f52bb2604b70
Reviewed-on: https://go-review.googlesource.com/131279
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 6, 2018

Given its not going to use your existing code @bradfitz, can I work on the CL?

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Sep 7, 2018

@bradfitz, I thought ReverseProxy already supported WebSocket connections as long as FlushInterval is set to a non-zero value. How does your proposed solution compare?

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 7, 2018

@dmitshur

  1. Will be faster and more responsive because data will be sent directly instead of waiting on the flush interval.
  2. ReverseProxy does not actually work. It strips the upgrade header in compliance with the RFC and so backend's will not get the upgrade header and not consider the request to be a HTTP upgrade. Furthermore, I don't it would play well with the bidirectional websocket stream. The reverse proxy first writes the entire request and then reads the entire response, it doesn't copy between the two connections concurrently. Additionally, even if it did, I think this could cause a conflict with the HTTP server, it may send the body to the client as a chunked stream as the reverse proxy does not presently hijack it.
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Sep 7, 2018

@nhooyr First point makes sense.

ReverseProxy does not actually work.

This is surprising to me, because I'm using httputil.ReverseProxy to proxy websocket connections (that send data bidirectionally) and I didn't notice any issues. Perhaps there are issues, but not noticeable enough. I don't have the bandwidth to investigate this further for now.

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Sep 7, 2018

I can't imagine how it would be working today.

I'd be surprised. (But probably not pleasantly as it'd probably be some scary accidental bug.)

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 7, 2018

According to my testing this does not work.

I ran a reverse proxy into a gorilla websocket server and made sure to add back the upgrade headers stripped off by the reverse proxy. The handshake succeeded successfully and then I wrote a message to the websocket server but it never got through to the server.

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Sep 8, 2018

Mea culpa. I've figured out where I made a mistake.

I was confused how it was that I had it working, while you were saying it doesn't work. So I tried to reproduce it, and it didn't work. I got WebSocket connection to 'ws://localhost:8090/rp' failed: Error during WebSocket handshake: Unexpected response code: 502.

It turns out I misremembered and never actually sent my websocket connection via a reverse proxy. I had that connecting directly from client to the server.

The thing I did send over a reverse proxy and required a non-zero FlushInterval was a Server-Sent Events connection, not WebSocket. >.< This was the relevant commit. In retrospect, that makes a lot of sense. Sorry about the noise.

Failed Repro Attempt Code
/*
Trying out httputil.ReverseProxy on a WebSocket connection.

Frontend code:

	ws = new WebSocket("ws://localhost:8080/ws");
	ws.onmessage = function(m) { console.log(m); }
	ws.send("hello\n");

	ws = new WebSocket("ws://localhost:8090/rp");
	ws.onmessage = function(m) { console.log(m); }
	ws.send("hello\n");
*/
package main

import (
	"fmt"
	"io"
	"log"
	"net/http"
	"net/http/httputil"
	"os"
	"time"

	"golang.org/x/net/websocket"
)

func main() {
	errCh := make(chan error)

	// Start a WebSocket server at localhost:8080/ws.
	go func() {
		mux := http.NewServeMux()
		mux.Handle("/ws", websocket.Handler(func(ws *websocket.Conn) {
			go io.Copy(os.Stdout, ws)
			for now := range time.Tick(10 * time.Second) {
				fmt.Fprintln(ws, "tick", now.Unix())
			}
		}))
		errCh <- http.ListenAndServe("localhost:8080", mux)
	}()

	// Start a reverse proxy server at localhost:8090/rp.
	go func() {
		mux := http.NewServeMux()
		rp := &httputil.ReverseProxy{
			Director: func(req *http.Request) {
				req.URL.Host = "localhost:8080"
				req.URL.Path = "/ws"
			},
			FlushInterval: 1 * time.Second,
		}
		mux.Handle("/rp", rp)
		errCh <- http.ListenAndServe("localhost:8090", mux)
	}()

	log.Fatalln(<-errCh)
}
@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 8, 2018

@bradfitz so can I work on the CL? Want to avoid duplicate work in case you've already begun.

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Sep 9, 2018

Go for it. Just don't add any new API. I think there are existing hooks users can use already.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 14, 2018

@bradfitz re https://golang.org/cl/131279 , what do you think of reusing http.Hijacker for the response body? Would let callers use a *http.Client to do a websocket handshake in general as then they would have access to the raw net.Conn to set deadlines or enable/disable TCP keep alives etc and also be able to reuse the bufio read/writer.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 15, 2018

Also we need to always use HTTP 1.1 for requests with Connection: Upgrade and a Upgrade header set as we cannot upgrade over HTTP 2.

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Sep 15, 2018

Yes, you'd need to use Hijacker.

As for using HTTP/1.1 for outgoing (Transport) Upgrade requests, I'd prefer that logic be in the Transport itself, not in ReverseProxy. You might need to do that CL first.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 15, 2018

Yes, you'd need to use Hijacker.

Thats for the server, I mean we reuse the Hijacker interface for the client to allow access to the buffered read/writer and the net.Conn directly (setting deadlines, tcp options etc). The response body would implement http.Hijacker.

As for using HTTP/1.1 for outgoing (Transport) Upgrade requests, I'd prefer that logic be in the Transport itself, not in ReverseProxy. You might need to do that CL first.

Got it.

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Sep 16, 2018

Thats for the server, I mean we reuse the Hijacker interface for the client

No, I don't see why we need any new API, as I implied above (#26937 (comment)).

I think TCP keep-alives will take care of cleanup of disappearing connections to the backend and the first failing io.Copy (of ReverseProxy's two client->server and server->client) will tear the whole world down. I think that's sufficient, at least for round one.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 24, 2018

No, I don't see why we need any new API, as I implied above (#26937 (comment)).

I think TCP keep-alives will take care of cleanup of disappearing connections to the backend and the first failing io.Copy (of ReverseProxy's two client->server and server->client) will tear the whole world down. I think that's sufficient, at least for round one.

Yes, its fine for the reverse proxy use case but not for stuff like the gorilla/websocket client connection where you want to control read/write deadlines per WebSocket message or change options on the underlying conn (maybe disable TCP keep alive because you want to do WebSocket pings). It just seems a more orthogonal and flexible API to me. Furthermore, its not really new API, http.Hijacker already exists, we'd just be reusing it. We're already adding new API with this change.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 24, 2018

And a WebSocket client package could reuse the net/http transport read/write buffers.

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Sep 24, 2018

I'm back from vacation now and this isn't done yet, so I can take this over if you want.

I don't want to add any new API for this, including things like repurposing API for new uses, which is still an API change.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 24, 2018

I'm back from vacation now and this isn't done yet, so I can take this over if you want.

I can do this. I just want to discuss.

I don't want to add any new API for this, including things like repurposing API for new uses, which is still an API change.

Didn't this already require new API because now the response body can be a io.ReadWriteCloser? If we're adding new API anyway, might as well go for the more flexible option, especially since the API is mostly already exposed.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 25, 2018

Change https://golang.org/cl/137437 mentions this issue: net/http: make Transport send WebSocket upgrade requests over HTTP/1

gopherbot pushed a commit that referenced this issue Oct 2, 2018
WebSockets requires HTTP/1 in practice (no spec or implementations
work over HTTP/2), so if we get an HTTP request that looks like it's
trying to initiate WebSockets, use HTTP/1, like browsers do.

This is part of a series of commits to make WebSockets work over
httputil.ReverseProxy. See #26937.

Updates #26937

Change-Id: I6ad3df9b0a21fddf62fa7d9cacef48e7d5d9585b
Reviewed-on: https://go-review.googlesource.com/c/137437
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Oct 9, 2018

@nhooyr, if we went with Hijacker for the client side, what would we do for HTTP/2 connections? It's not applicable to WebSockets today, as WS doesn't run on HTTP/2 yet, but I want to use the same mechanism for other stuff like CONNECT requests.

Currently Hijacker only works on the server side and only for HTTP/1. We don't support HTTP/2 because Hijacker returns a net.Conn and in HTTP/2, the actual TCP/TLS connection is shared.

So would we make our own net.Conn impl for the client side hijack? But if we did so, then we'd be in a situation where Hijacker works in 3 of the 4 cases (client HTTP/1 and HTTP/2, server HTTP/1, but not server HTTP/2), which seems like it should then also be fixed.

What were you thinking?

I agree the Response.Body being a ReadWriteCloser is also an API change, but I was thinking it'd only apply when the connection was already effectively taken over for life (Connection: upgrade and CONNECT), so it seemed harmless. Doing the ReadWriteCloser thing also doesn't preclude us from doing client-side Hijack (#28030) in the future if necessary. We could support both and people could choose the API that works best for them. Slightly redundant, but I'm not convinced we'd even get there before the HTTP packages are redesigned anyway.

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Oct 12, 2018

So would we make our own net.Conn impl for the client side hijack? But if we did so, then we'd be in a situation where Hijacker works in 3 of the 4 cases (client HTTP/1 and HTTP/2, server HTTP/1, but not server HTTP/2), which seems like it should then also be fixed.

For server HTTP/2 as you mentioned in #17227 (comment) its kind of useless to hijack. Though it'd be nice if we could close the stream and unlock any read/writes if a timeout of some sort gets triggered. It seems wrong to implement that separately from http.Hijacker though. More API = more user confusion and a harder to use package.

In consideration of that, I'd be for a http.Hijacker based API where we implement net.Conn ourselves for all the http/2 cases. While it is more complicated, it handles every scenario and as a cherry on top, it is more performant as we can reuse the buffered read/writer.

Another advantage is that when WebSockets do run on HTTP/2, existing code will have a very easy time adapting because everything will just work. They'll just need to check the pseudo protocol header described in https://tools.ietf.org/html/draft-ietf-httpbis-h2-websockets-00 which we would need to expose somehow.

Though, would we have a buffered read/writer to return for the HTTP/2 stream?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 31, 2018

Change https://golang.org/cl/146437 mentions this issue: net/http/httputil: make ReverseProxy automatically proxy WebSocket requests

@jrefior

This comment has been minimized.

Copy link

@jrefior jrefior commented Nov 2, 2018

I just ran into this problem today. Is there any workaround until a fix/feature is released?

@jrefior

This comment has been minimized.

Copy link

@jrefior jrefior commented Nov 3, 2018

I've gotten pretty far with writing my own reverse proxy based on your changes, but I'm basically stuck here right now:

    backConn, ok := res.Body.(io.ReadWriteCloser)
    if !ok {
        log.Error("internal error: 101 switching protocols response with non-writable body: %v (%T)",
 res.Body, res.Body)
        return
    }

The type assertion fails. Output is:

ERRO[0012] internal error: 101 switching protocols response with non-writable body: {} (http.noBody)

Any suggestion for how to resolve?

This is what res looks like in my log:

Server response was &{Status:101 Switching Protocols StatusCode:101 Proto:HTTP/1.1 
ProtoMajor:1 ProtoMinor:1 Header:map[Upgrade:[websocket] Connection:[Upgrade]
Sec-Websocket-Accept:[B1fWETPTtNo0bAHFWkFn2lAm9iw=]] Body:{} ContentLength:0 
TransferEncoding:[] Close:false Uncompressed:false Trailer:map[] Request:0xc000112100 
TLS:<nil>}

Ah, I think I figured out why I'm stuck here. The latest release of Go does not include this line in persistConn's readLoop function in transport.go in the net/http package:

if resp.isProtocolSwitch() {
    resp.Body = newReadWriteCloserBody(pc.br, pc.conn)
}

See earlier change here. Any suggestion for a way to work around that?

I also posted this as a StackOverflow question.

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Nov 5, 2018

@jrefior, if you can't wait for Go 1.12, you can do something like https://gist.github.com/bradfitz/1d7bdf12278d4d713212ce6c74875dab

@jrefior

This comment has been minimized.

Copy link

@jrefior jrefior commented Nov 5, 2018

Wow, thank you @bradfitz ! For my purposes, I don't need TLS to the backend as traffic will stay within my network, so I can just replace tls.DialWithDialer with net.DialTimeout, right? Awesome, looks simpler than what I was building.

@bradfitz

This comment has been minimized.

Copy link
Member Author

@bradfitz bradfitz commented Nov 5, 2018

@jrefior, I'll let you play around with it and figure out what you need. I'm focused on Go 1.12. Let's move any pre-Go 1.12 discussion & updates out of this bug, though. I'd like to keep this focused on Go 1.12 stuff.

@gopherbot gopherbot closed this in ee55f08 Nov 13, 2018
teran added a commit to teran/svcproxy that referenced this issue Mar 1, 2019
According to recent changes from [0] ResponseWriter
must implement `http.Hijacker` to allow websockets to work properly.

* [0] golang/go#26937

Signed-off-by: Igor Shishkin <me@teran.ru>
@golang golang locked and limited conversation to collaborators Nov 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.