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/httptest: Server.Close mutates http.DefaultTransport #65796

Open
tkashem opened this issue Feb 19, 2024 · 1 comment
Open

net/http/httptest: Server.Close mutates http.DefaultTransport #65796

tkashem opened this issue Feb 19, 2024 · 1 comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@tkashem
Copy link

tkashem commented Feb 19, 2024

Go version

go version go1.21.6 linux/amd64

Output of go env in your module/workspace:

$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/akashem/.cache/go-build'
GOENV='/home/akashem/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/akashem/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/akashem/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/lib/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/lib/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.6'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/akashem/go/src/k8s.io/kubernetes/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2922644753=/tmp/go-build -gno-record-gcc-switches'

What did you do?

It seems that the default transport object http.DefaultTransport gets mutated by the Close method of httptest.Server.

Here is a test that demonstrates it:

func TestHTTPTestServerCloseMutatesDefaultTransport(t *testing.T) {
	server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("pong"))
	}))
	server.EnableHTTP2 = true
	server.StartTLS()

	client := server.Client()
	client.Get(server.URL + "/ping")

	want := http.DefaultTransport.(*http.Transport).TLSClientConfig
	if want != nil {
		t.Errorf("expected TLSClientConfig of http.DefaultTransport to be nil")
	}
	server.Close()
	if got := http.DefaultTransport.(*http.Transport).TLSClientConfig; want != got {
		t.Errorf("TLSClientConfig has been mutated")
	}
}

The TLSClientConfig field of the DefaultTransport object is nil to start with, but then it gets replaced with a non nil instance after the Close method returns. Here is the call chain of the mutation:

httptest/Server.Close invokes the CloseIdleConnections method of the default DefaultTransport object:

// Not part of httptest.Server's correctness, but assume most
// users of httptest.Server will be using the standard
// transport, so help them out and close any idle connections for them.
if t, ok := http.DefaultTransport.(closeIdleTransport); ok {
t.CloseIdleConnections()
}

func (t *Transport) CloseIdleConnections() {
t.nextProtoOnce.Do(t.onceSetNextProtoDefaults)

TLSClientConfig.NextProtos gets set:

go/src/net/http/h2_bundle.go

Lines 7269 to 7274 in cf52e70

if t1.TLSClientConfig == nil {
t1.TLSClientConfig = new(tls.Config)
}
if !http2strSliceContains(t1.TLSClientConfig.NextProtos, "h2") {
t1.TLSClientConfig.NextProtos = append([]string{"h2"}, t1.TLSClientConfig.NextProtos...)
}

I am not certain if it's common for production code to exercise:

	if t, ok := http.DefaultTransport.(closeIdleTransport); ok {
		t.CloseIdleConnections()
	}

I ran into this issue with unit tests that verify that the TLSClientConfig field of a given transport does not get mutated unexpectedly. I can change the test to work around it. But I wanted to bring this to your attention so you can assess whether it can impact production code.

@seankhliao
Copy link
Member

cc @neild

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 19, 2024
@odeke-em odeke-em changed the title net/http: httptest Server Close mutates the http.DefaultTransport object net/http/httptest: Server.Close mutates http.DefaultTransport Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

2 participants