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

Comments

Projects
None yet
4 participants
@joneskoo
Copy link
Contributor

commented Sep 10, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2016

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

    http2.ConfigureTransport(transport)

after setting the TLSClientConfig?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2016

@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 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

This comment has been minimized.

Copy link

commented Nov 10, 2016

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

@gopherbot gopherbot closed this in caa434d Nov 10, 2016

@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.
You can’t perform that action at this time.