net/http/httptest: NewTLSServer doesn't support HTTP/2 requests

[rhysh]

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

$ go1.13 version
go version go1.13.1 darwin/amd64
$ go-tip version
go version devel +f6c624a22a Wed Oct 16 15:58:33 2019 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes, go1.13.1 and tip are affected.

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

go env Output

$ go1.13 env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/rhys/Library/Caches/go-build"
GOENV="/Users/rhys/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="*"
GONOSUMDB="*"
GOOS="darwin"
GOPATH="/Users/rhys/go"
GOPRIVATE="*"
GOPROXY="direct"
GOROOT="/Users/rhys/go/version/go1.13"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/Users/rhys/go/version/go1.13/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/49/zmds5zsn75z1283vtzxyfr5hj7yjq4/T/go-build721053994=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I called httptest.NewTLSServer and used the attached http.Client to make requests to it.

What did you expect to see?

I expected the requests to the local test server to use HTTP/2. (This is important for me at the moment because I'm writing reproducers for bugs I've seen in HTTP/2 client requests.)

What did you see instead?

The requests are sent over HTTP/1.1.

It looks like this is because the httptest package's TLS setup sets a tls.Config on the http.Transport before its first use, which defeats net/http's usual HTTP/2 autoconfiguration.

$  go1.13 test -v ./repro_test.go
=== RUN   TestHTTPTestHTTP2
=== RUN   TestHTTPTestHTTP2/basic
=== RUN   TestHTTPTestHTTP2/advertise-h2
=== RUN   TestHTTPTestHTTP2/transport-hacks
2019/10/16 13:23:20 http: TLS handshake error from 127.0.0.1:64474: remote error: tls: bad certificate
--- FAIL: TestHTTPTestHTTP2 (0.14s)
    --- FAIL: TestHTTPTestHTTP2/basic (0.00s)
        repro_test.go:17: Request is not http/2: "HTTP/1.1"
    --- FAIL: TestHTTPTestHTTP2/advertise-h2 (0.00s)
        repro_test.go:37: Request is not http/2: "HTTP/1.1"
    --- PASS: TestHTTPTestHTTP2/transport-hacks (0.14s)
FAIL
FAIL    command-line-arguments  0.193s
FAIL

$ go-tip test -v ./repro_test.go
=== RUN   TestHTTPTestHTTP2
=== RUN   TestHTTPTestHTTP2/basic
=== RUN   TestHTTPTestHTTP2/advertise-h2
=== RUN   TestHTTPTestHTTP2/transport-hacks
2019/10/16 13:23:55 http: TLS handshake error from 127.0.0.1:64490: remote error: tls: bad certificate
--- FAIL: TestHTTPTestHTTP2 (0.14s)
    --- FAIL: TestHTTPTestHTTP2/basic (0.00s)
        repro_test.go:17: Request is not http/2: "HTTP/1.1"
    --- FAIL: TestHTTPTestHTTP2/advertise-h2 (0.00s)
        repro_test.go:37: Request is not http/2: "HTTP/1.1"
    --- PASS: TestHTTPTestHTTP2/transport-hacks (0.14s)
FAIL
FAIL    command-line-arguments  1.254s
FAIL

package repro

import (
	"crypto/tls"
	"net/http"
	"net/http/httptest"
	"sync/atomic"
	"testing"
)

func TestHTTPTestHTTP2(t *testing.T) {
	t.Run("basic", func(t *testing.T) {
		var calls int64
		s := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		defer s.Close()

		resp, err := s.Client().Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})

	t.Run("advertise-h2", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		s.TLS = &tls.Config{
			NextProtos: []string{"h2"},
		}
		s.StartTLS()
		defer s.Close()

		resp, err := s.Client().Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})

	t.Run("transport-hacks", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		s.TLS = &tls.Config{
			NextProtos: []string{"h2"},
		}
		s.StartTLS()
		defer s.Close()

		transport := s.Client().Transport.(*http.Transport)
		clientConfig := transport.TLSClientConfig
		transport.TLSClientConfig = nil

		// make a request to trigger HTTP/2 autoconfiguration
		resp, err := s.Client().Get(s.URL)
		if err == nil {
			t.Errorf(`Expected harmless "certificate signed by unknown authority" error`)
			resp.Body.Close()
		}
		// now allow the client to connect to the ad-hoc test server
		transport.TLSClientConfig.RootCAs = clientConfig.RootCAs

		resp, err = s.Client().Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})
}

[odeke-em]

@rhysh thank you for filing this issue and great to catch you here!
Here is how you can enable HTTP/2 for httptest but also for other servers

--- before_test.go	2019-10-16 13:42:17.000000000 -0700
+++ fixed_test.go	2019-10-16 13:48:43.000000000 -0700
@@ -6,20 +6,34 @@
 	"net/http/httptest"
 	"sync/atomic"
 	"testing"
+
+	"golang.org/x/net/http2"
 )
 
 func TestHTTPTestHTTP2(t *testing.T) {
 	t.Run("basic", func(t *testing.T) {
 		var calls int64
-		s := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			atomic.AddInt64(&calls, 1)
 			if !r.ProtoAtLeast(2, 0) {
 				t.Errorf("Request is not http/2: %q", r.Proto)
 			}
 		}))
+		if err := http2.ConfigureServer(s.Config, new(http2.Server)); err != nil {
+			t.Fatalf("Failed to configure HTTP/2 server: %v", err)
+		}
+		s.TLS = s.Config.TLSConfig
+		s.StartTLS()
 		defer s.Close()
 
-		resp, err := s.Client().Get(s.URL)
+		tr := &http.Transport{TLSClientConfig: s.Config.TLSConfig}
+		if err := http2.ConfigureTransport(tr); err != nil {
+			t.Fatalf("Failed to configure http2 transport: %v", err)
+		}
+		tr.TLSClientConfig.InsecureSkipVerify = true
+		client := &http.Client{Transport: tr}
+
+		resp, err := client.Get(s.URL)
 		if err != nil {
 			t.Fatalf("HTTP GET: %v", err)
 		}
@@ -37,13 +51,21 @@
 				t.Errorf("Request is not http/2: %q", r.Proto)
 			}
 		}))
-		s.TLS = &tls.Config{
-			NextProtos: []string{"h2"},
+		if err := http2.ConfigureServer(s.Config, new(http2.Server)); err != nil {
+			t.Fatalf("Failed to configure HTTP/2 server: %v", err)
 		}
+		s.TLS = s.Config.TLSConfig
 		s.StartTLS()
 		defer s.Close()
 
-		resp, err := s.Client().Get(s.URL)
+		tr := &http.Transport{TLSClientConfig: s.Config.TLSConfig}
+		if err := http2.ConfigureTransport(tr); err != nil {
+			t.Fatalf("Failed to configure http2 transport: %v", err)
+		}
+		tr.TLSClientConfig.InsecureSkipVerify = true
+		client := &http.Client{Transport: tr}
+
+		resp, err := client.Get(s.URL)
 		if err != nil {
 			t.Fatalf("HTTP GET: %v", err)
 		}

and I have encountered this is in the wild which is why I shared this gist https://gist.github.com/odeke-em/cacfe2ea5efc57c5a7a562aba1478378

to finally give you the file

package repro

import (
	"crypto/tls"
	"net/http"
	"net/http/httptest"
	"sync/atomic"
	"testing"

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

func TestHTTPTestHTTP2(t *testing.T) {
	t.Run("basic", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		if err := http2.ConfigureServer(s.Config, new(http2.Server)); err != nil {
			t.Fatalf("Failed to configure HTTP/2 server: %v", err)
		}
		s.TLS = s.Config.TLSConfig
		s.StartTLS()
		defer s.Close()

		tr := &http.Transport{TLSClientConfig: s.Config.TLSConfig}
		if err := http2.ConfigureTransport(tr); err != nil {
			t.Fatalf("Failed to configure http2 transport: %v", err)
		}
		tr.TLSClientConfig.InsecureSkipVerify = true
		client := &http.Client{Transport: tr}

		resp, err := client.Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})

	t.Run("advertise-h2", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		if err := http2.ConfigureServer(s.Config, new(http2.Server)); err != nil {
			t.Fatalf("Failed to configure HTTP/2 server: %v", err)
		}
		s.TLS = s.Config.TLSConfig
		s.StartTLS()
		defer s.Close()

		tr := &http.Transport{TLSClientConfig: s.Config.TLSConfig}
		if err := http2.ConfigureTransport(tr); err != nil {
			t.Fatalf("Failed to configure http2 transport: %v", err)
		}
		tr.TLSClientConfig.InsecureSkipVerify = true
		client := &http.Client{Transport: tr}

		resp, err := client.Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})

	t.Run("transport-hacks", func(t *testing.T) {
		var calls int64
		s := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			atomic.AddInt64(&calls, 1)
			if !r.ProtoAtLeast(2, 0) {
				t.Errorf("Request is not http/2: %q", r.Proto)
			}
		}))
		s.TLS = &tls.Config{
			NextProtos: []string{"h2"},
		}
		s.StartTLS()
		defer s.Close()

		transport := s.Client().Transport.(*http.Transport)
		clientConfig := transport.TLSClientConfig
		transport.TLSClientConfig = nil

		// make a request to trigger HTTP/2 autoconfiguration
		resp, err := s.Client().Get(s.URL)
		if err == nil {
			t.Errorf(`Expected harmless "certificate signed by unknown authority" error`)
			resp.Body.Close()
		}
		// now allow the client to connect to the ad-hoc test server
		transport.TLSClientConfig.RootCAs = clientConfig.RootCAs

		resp, err = s.Client().Get(s.URL)
		if err != nil {
			t.Fatalf("HTTP GET: %v", err)
		}
		resp.Body.Close()
		if have, want := atomic.LoadInt64(&calls), int64(1); have != want {
			t.Errorf("HTTP handler called %d times, expected %d times", have, want)
		}
	})
}

but anyways the crux of it is that you'll need to:
a) Make an Unstarted server
b) Invoke http2.ConfigureServer
c) do s.TLS = s.Config.TLSConfig and then s.StartTLS
d) For the client: make an http.Transport using s.Config.TLSConfig and obviously insecureSkipVerify

What I think we can do though is for the net/http/httptest package is add a new helper for example NewTLSServerWithHTTP2 which under the hood encompasses all those steps I listed out in a) to d), @bradfitz what do you think?

[odeke-em]

Or perhaps an option for NewTLSServer that enables HTTP/2 instead of the entirely new function NewTLSServerWithHTTP2

[bradfitz]

Howa about a new EnableHTTP2 bool on https://golang.org/pkg/net/http/httptest/#Server that then does the setup in Server.Start? (With any necessary changes to the Client method to return a configured client)

And an example, if docs aren't sufficient.

So the flow would be NewUnstartedServer, set the bool to true, and Start.

[odeke-em]

Perfect, SGTM, thank you @bradfitz! CL coming up shortly with an example too and tests :)

[rhysh]

Thanks @odeke-em and @bradfitz . A new boolean field sounds like the right level of complexity for the user. Do we need even that?

Should http/2 be the default for httptest setups just as it is for production net/http clients and servers? When would a user want to opt out of http/2?

httptest has a mechanism for that opt-out already: setting the server's NextProtos to not include "h2". Do we need a new specialized field?

[gopherbot]

Change https://golang.org/cl/201557 mentions this issue: net/http/httptest: add EnableHTTP2 to Server

[odeke-em]

Should http/2 be the default for httptest setups just as it is for production net/http clients and servers? When would a user want to opt out of http/2?

net/http/httptest has been around for ages before HTTP/2 and the norm when testing is to use HTTP/1.1, and as you can see configuring HTTP/2 on these servers is a little tricky. Changing it to always use HTTP/2 might break lots of people's code who've been using this package for years. I think we can perhaps schedule a gradual roll out to invert the behavior, and like you mention, instead make HTTP/1.1 toggleable by the opt-out but after a while of having EnableHTTP2 around.

[gopherbot]

Change https://golang.org/cl/217131 mentions this issue: doc/go1.14: mention new field net/http/httptest/Server.EnableHTTP2