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: Transport TLS config example does not support http2 #17051

Closed
joneskoo opened this issue Sep 10, 2016 · 5 comments
Closed

net/http: Transport TLS config example does not support http2 #17051

joneskoo opened this issue Sep 10, 2016 · 5 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@joneskoo
Copy link
Contributor

In https://golang.org/pkg/net/http/

For control over proxies, TLS configuration, keep-alives, compression, and other settings, create a Transport:

tr := &http.Transport{
    TLSClientConfig:    &tls.Config{RootCAs: pool},
    DisableCompression: true,
}
client := &http.Client{Transport: tr}
resp, err := client.Get("https://example.com")

If one follows this example, http2 is not supported by the client, because
http2 depends on non-nil values for the TLS config.

The documentation should reflect the best practice how to initialize tls.Config,
which in this case should include HTTP/2 support.

This is somewhat similar to #14391?

What version of Go are you using (go version)?

go version devel +7b26919 Sat Sep 10 00:35:48 2016 +0000 darwin/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/joneskoo"
GORACE=""
GOROOT="/Users/joneskoo/src/github.com/golang/go"
GOTOOLDIR="/Users/joneskoo/src/github.com/golang/go/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/p9/8mb6mhcx7p3br8f18crtczzw0000gn/T/go-build695312228=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

https://play.golang.org/p/0hEow6aME1

I run this locally:

$ go run h2.go|grep "using HTTP"

What did you expect to see?

$ go run h2.go|grep "using HTTP"
<p>Congratulations, <b>you're using HTTP/2 right now</b>.</p>

I can reproduce this if I don't set the transport for the client, which uses
the default transport.

What did you see instead?

$ go run h2.go|grep "using HTTP"
<p>Unfortunately, you're <b>not</b> using HTTP/2 right now. To do so:</p>
@joneskoo
Copy link
Contributor Author

CC @bradfitz?

If the tls Config copying is elsewhere in the documentation, great, but the net/http should
certainly have a working example of changing TLS client configuration in the correct way (for
HTTP/2 to work).

@joneskoo
Copy link
Contributor Author

What is the right fix anyway? Is it to call x/net/http2

    http2.ConfigureTransport(transport)

after setting the TLSClientConfig?

@bradfitz
Copy link
Contributor

Yes, if you need to do something custom, import golang.org/x/net/http2 directly.

I'll keep this open to add some more docs around this.

@bradfitz bradfitz added this to the Go1.8 milestone Sep 10, 2016
@joneskoo
Copy link
Contributor Author

@bradfitz Excellent. Related, I'd want the net/http code examples in overview to reflect more real usage. Namely, the examples suggest using &http.Transport{...} but leave out any mentions of e.g. MaxIdleConns etc. that are part of DefaultTransport. Should the examples suggest copying DefaultTransport then or do we need a NewTransport that gives a copy of default transport sensible settings, including HTTP/2 and idle connection limit?

I've been bitten by getting default zero values following the example, as zero value is different from ones from DefaultTransport. This may be fixable with a doc-only change. The doc should reflect best practice.

Let me know if you want me to create another issue for this or if it's basically the same thing.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@joneskoo joneskoo changed the title doc: net/http Transport TLS config example does not support http2 net/http: Transport TLS config example does not support http2 Nov 7, 2016
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Nov 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants