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

x/net/http2: bidirection streams, write request body data as available #13444

Closed
ncdc opened this issue Dec 1, 2015 · 10 comments
Closed

x/net/http2: bidirection streams, write request body data as available #13444

ncdc opened this issue Dec 1, 2015 · 10 comments

Comments

@ncdc
Copy link

@ncdc ncdc commented Dec 1, 2015

In Kubernetes, we are currently using spdystream as a means to support bidirectional stream-based communication over HTTP connections. We use this to enable ssh-like connectivity and port forwarding from the user's system into a Docker container running in Kubernetes.

The spdystream APIs enable us to create streams at will in both the client and server handler code. For remote command execution, the client creates streams representing stdin, stdout, and stderr. Once the server receives all the streams, it performs a docker exec of whatever process the user wishes, and data is copied between the container process's stdin/stdout/stderr and the spdy streams.

We're hoping that we can eventually move to HTTP/2 and achieve the same functionality; namely, full control over stream creation and data flow in a "hijacked" fashion.

cc @bradfitz @smarterclayton @thockin @lavalamp

(forked from #13443)

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 1, 2015

When you say "streams", you don't mean anything more specific than being able to read & write response bodies and/or (?) request bodies at the same time?

For the client side, can't you just do a RoundTrip with the Request.Body set to the read end of an io.Pipe and then write your stderr/stdout to the write side?

I just put up an /ECHO handler at https://http2.golang.org/ECHO which streams back its output capitalized:

type capitalizeReader struct {
        r io.Reader
}

func (cr capitalizeReader) Read(p []byte) (n int, err error) {
        n, err = cr.r.Read(p)
        for i, b := range p[:n] {
                if b >= 'a' && b <= 'z' {
                        p[i] = b - ('a' - 'A')
                }
        }
        return
}

type flushWriter struct {
        w io.Writer
}

func (fw flushWriter) Write(p []byte) (n int, err error) {
        n, err = fw.w.Write(p)
        if f, ok := fw.w.(http.Flusher); ok {
                f.Flush()
        }
        return
}

func echoCapitalHandler(w http.ResponseWriter, r *http.Request) {
        if r.Method != "PUT" {
                http.Error(w, "PUT required.", 400)
                return
        }
        io.Copy(flushWriter{w}, capitalizeReader{r.Body})
}

And then with a couple modifications to the http2 client, I can now stream an HTTP request body to a server, and read the streamed response body at the same time, even seeing the 1 second delays:

bradfitz@dev-bradfitz-debian2:~$ cat echo.go
package main

import (
        "fmt"
        "io"
        "io/ioutil"
        "log"
        "net/http"
        "os"
        "time"
)

func main() {
        pr, pw := io.Pipe()
        req, err := http.NewRequest("PUT", "https://http2.golang.org/ECHO", ioutil.NopCloser(pr))
        if err != nil {
                log.Fatal(err)
        }
        go func() {
                for {   
                        time.Sleep(1 * time.Second)
                        fmt.Fprintf(pw, "It is now %v\n", time.Now())
                }
        }()
        go func() {
                res, err := http.DefaultClient.Do(req)
                if err != nil {
                        log.Fatal(err)
                }
                log.Printf("Got: %#v", res)
                n, err := io.Copy(os.Stdout, res.Body)
                log.Fatalf("copied %d, %v", n, err)
        }()
        select {}
}
bradfitz@dev-bradfitz-debian2:~$ go run echo.go
2015/12/01 22:10:54 Got: &http.Response{Status:"200 OK", StatusCode:200, Proto:"HTTP/2.0", ProtoMajor:2, ProtoMinor:0, Header:http.Header{"Content-Type":[]string{"text/plain; charset=utf-8"}, "Date":[]string{"Tue, 01 Dec 2015 22:10:54 GMT"}}, Body:http.http2transportResponseBody{cs:(*http.http2clientStream)(0xc8203697a0)}, ContentLength:-1, TransferEncoding:[]string(nil), Close:false, Trailer:http.Header(nil), Request:(*http.Request)(0xc8200c4000), TLS:(*tls.ConnectionState)(0xc8203d69a0)}
IT IS NOW 2015-12-01 22:10:54.592452374 +0000 UTC
IT IS NOW 2015-12-01 22:10:55.592869959 +0000 UTC
IT IS NOW 2015-12-01 22:10:56.593126243 +0000 UTC
IT IS NOW 2015-12-01 22:10:57.593371657 +0000 UTC
IT IS NOW 2015-12-01 22:10:58.593660994 +0000 UTC
IT IS NOW 2015-12-01 22:10:59.59395207 +0000 UTC
IT IS NOW 2015-12-01 22:11:00.594216993 +0000 UTC
IT IS NOW 2015-12-01 22:11:01.594496771 +0000 UTC
IT IS NOW 2015-12-01 22:11:02.594795043 +0000 UTC
IT IS NOW 2015-12-01 22:11:03.594974236 +0000 UTC
IT IS NOW 2015-12-01 22:11:04.595187379 +0000 UTC
IT IS NOW 2015-12-01 22:11:05.59544486 +0000 UTC

Do you need more than that?

The change is at https://golang.org/cl/17310 if you want to patch it in and play. (In your $GOPATH/src/net directory, run git fetch https://go.googlesource.com/net refs/changes/10/17310/1 && git cherry-pick FETCH_HEAD)

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 1, 2015

CL https://golang.org/cl/17310 mentions this issue.

@ncdc

This comment has been minimized.

Copy link
Author

@ncdc ncdc commented Dec 2, 2015

@bradfitz this looks quite promising. I'm in meetings most of today, so I doubt I'll have time to fiddle, but hopefully I will tomorrow. @smarterclayton WDYT about this?

@ncdc

This comment has been minimized.

Copy link
Author

@ncdc ncdc commented Dec 7, 2015

@bradfitz I'm finally getting some time to look at this. If I have a normal http.Transport or even just http.DefaultClient, how do I set AllowResponseBeforeBody to true without modifying the internal http2 source?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 7, 2015

Once that change is submitted you wouldn't need to modify the http2 source.

You'd just write:

package main

import (
    "net/http"
    "golang.org/x/net/http2"
)

func main() {
     c := &http.Client{Transport: &http2.Transport{AllowResponseBeforeBody: true}}
     ....
}
@ncdc

This comment has been minimized.

Copy link
Author

@ncdc ncdc commented Dec 7, 2015

Thanks!

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 8, 2015

Actually, I'm removing the option. It'll just be on by default.

You won't even need to import "golang.org/x/net/http2" as of Go 1.6.

@bradfitz bradfitz changed the title x/net/http2: allow user to manipulate streams x/net/http2: bidirection streams, write request body data as available Dec 8, 2015
bradfitz added a commit to golang/net that referenced this issue Dec 8, 2015
Unlike HTTP/1, we now permit streaming the write of a request body as
we read the response body, since HTTP/2's framing makes it possible.
Our behavior however is based on a heuristic: we always begin writing
the request body right away (like previously, and like HTTP/1), but if
we're still writing the request body and the server replies with a
status code over 299 (not 1xx and not 2xx), then we stop writing the
request body, assuming the server doesn't care about it. There is
currently no switch (and hopefully won't be) to force enable this
behavior. In the case where the server replied with a 1xx/2xx and
we're still writing the request body but the server doesn't want it,
the server can do a RST_STREAM, which we respect as before and stop
sending.

Also in this CL:

* adds an h2demo handler at https://http2.golang.org/ECHO to demo it

* fixes a potential flow control integer truncation bug

* start of clientTester type used for the tests in this CL, similar
  to the serverTester. It's still a bit cumbersome to write client
  tests, though.

* fix potential deadlock where awaitFlowControl could block while
  waiting a stream reset arrived. fix it by moving all checks into
  the sync.Cond loop, rather than having a sync.Cond check followed
  by a select. simplifies code, too.

* fix two data races in test-only code.

Updates golang/go#13444

Change-Id: Idfda6833a212a89fcd65293cdeb4169d1723724f
Reviewed-on: https://go-review.googlesource.com/17310
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>
@ncdc

This comment has been minimized.

Copy link
Author

@ncdc ncdc commented Dec 8, 2015

Even better 😄

I did get a chance to play around a bit yesterday. Our setup is a bit complicated, because have a proxy in the middle, so the flow is client -> proxy -> backend (and it ultimately connects to a Docker exec session). I was able to hack the proxy to get it to somewhat support proxying HTTP/2, and I was able to successfully round trip a request from the client to the backend and back. The one thing I didn't get working was interactive input and the output coming back immediately. I'm not sure if it's an issue with the way I was trying to proxy the request through, or what, but I'll come back to it at some point in my spare time.

To illustrate what wasn't working correctly:

kubectl exec -i nginx bash<CR>
ls<CR>
date<CR>
exit<CR>

The output from ls and date only show up after you type exit and hit enter. So the round trip isn't completing until the backend exec completes. I think given your demonstration of the echo client/server working, there is something wrong in my setup...

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 8, 2015

Yeah, the change just submitted even has a couple new tests showing that this works (and will prevent it from ever not working in the future). So I suspect your proxy is buffering a bit too hard.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 8, 2015

CL https://golang.org/cl/17570 mentions this issue.

@bradfitz bradfitz closed this in 9b8080f Dec 8, 2015
bradfitz added a commit that referenced this issue Dec 9, 2015
Fixes #12796
Updates #13444

Change-Id: I56840c0baf9b32a683086a80f5db1c5ea0a7aedf
Reviewed-on: https://go-review.googlesource.com/17680
Reviewed-by: Russ Cox <rsc@golang.org>
@bictorman bictorman mentioned this issue Mar 23, 2016
@golang golang locked and limited conversation to collaborators Dec 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.