net/http/httputil: add WebSocket support to ReverseProxy
bradfitz opened this issue · 36 comments
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.
This would need to be off by default in case current clients rely on the lack of support to prevent websocket connections.
@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.
@bradfitz Fair enough. Its extremely unlikely anyone would not want WebSockets support anyway.
@bradfitz Could you post the code for this, I've written something similar myself, want to make sure I'm doing it right.
We should also support arbitrary upgrades instead of just WebSockets.
@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.
Change https://golang.org/cl/131279 mentions this issue: net/http: make Transport return Writable Response.Body on WebSocket upgrade
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.
@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?
- Will be faster and more responsive because data will be sent directly instead of waiting on the flush interval.
- 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.
@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.
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.)
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.
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)
}@bradfitz so can I work on the CL? Want to avoid duplicate work in case you've already begun.
Go for it. Just don't add any new API. I think there are existing hooks users can use already.
@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.
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.
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.
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.
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.
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.
And a WebSocket client package could reuse the net/http transport read/write buffers.
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.
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.
Change https://golang.org/cl/137437 mentions this issue: net/http: make Transport send WebSocket upgrade requests over HTTP/1
@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.
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?
Change https://golang.org/cl/146437 mentions this issue: net/http/httputil: make ReverseProxy automatically proxy WebSocket requests
I just ran into this problem today. Is there any workaround until a fix/feature is released?
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.
@jrefior, if you can't wait for Go 1.12, you can do something like https://gist.github.com/bradfitz/1d7bdf12278d4d713212ce6c74875dab