From ad947e88ecd021aa06eda04d5c0342527e3ed9ad Mon Sep 17 00:00:00 2001 From: Josh Deprez Date: Mon, 23 Sep 2024 15:33:09 +1000 Subject: [PATCH] Attempt to workaround golang/go#59690 --- api/auth.go | 40 +++++++++++++++++++------ api/client.go | 81 ++++++++++++++++++++++++++++----------------------- go.mod | 2 +- 3 files changed, 76 insertions(+), 47 deletions(-) diff --git a/api/auth.go b/api/auth.go index 1fb28da103..274d92a2d6 100644 --- a/api/auth.go +++ b/api/auth.go @@ -5,11 +5,7 @@ import ( "net/http" ) -type canceler interface { - CancelRequest(*http.Request) -} - -// authenticatedTransport manages injection of the API token +// authenticatedTransport manages injection of the API token. type authenticatedTransport struct { // The Token used for authentication. This can either the be // organizations registration token, or the agents access token. @@ -19,19 +15,45 @@ type authenticatedTransport struct { Delegate http.RoundTripper } -// RoundTrip invoked each time a request is made +// RoundTrip invoked each time a request is made. func (t authenticatedTransport) RoundTrip(req *http.Request) (*http.Response, error) { if t.Token == "" { return nil, fmt.Errorf("Invalid token, empty string supplied") } + // Per net/http#RoundTripper: + // + // "RoundTrip should not modify the request, except for + // consuming and closing the Request's Body. RoundTrip may + // read fields of the request in a separate goroutine. Callers + // should not mutate or reuse the request until the Response's + // Body has been closed." + // + // But we can pass a _different_ request to t.Delegate.RoundTrip. + // req.Clone does a sufficiently deep clone (including Header which we + // modify). + req = req.Clone(req.Context()) req.Header.Set("Authorization", fmt.Sprintf("Token %s", t.Token)) return t.Delegate.RoundTrip(req) } -// CancelRequest cancels an in-flight request by closing its connection. +// CancelRequest forwards the call to t.Delegate, if it implements CancelRequest +// itself. func (t *authenticatedTransport) CancelRequest(req *http.Request) { - cancelableTransport := t.Delegate.(canceler) - cancelableTransport.CancelRequest(req) + canceler, ok := t.Delegate.(interface{ CancelRequest(*http.Request) }) + if !ok { + return + } + canceler.CancelRequest(req) +} + +// CloseIdleConnections forwards the call to t.Delegate, if it implements +// CloseIdleConnections itself. +func (t *authenticatedTransport) CloseIdleConnections() { + closer, ok := t.Delegate.(interface{ CloseIdleConnections() }) + if !ok { + return + } + closer.CloseIdleConnections() } diff --git a/api/client.go b/api/client.go index ef58ec814b..13c891c941 100644 --- a/api/client.go +++ b/api/client.go @@ -10,7 +10,6 @@ import ( "errors" "fmt" "io" - "net" "net/http" "net/http/httptrace" "net/http/httputil" @@ -83,46 +82,54 @@ func NewClient(l logger.Logger, conf Config) *Client { httpClient := conf.HTTPClient - if conf.HTTPClient == nil { - if conf.DisableHTTP2 { - tr := &http.Transport{ - Proxy: http.ProxyFromEnvironment, - DisableCompression: false, - DisableKeepAlives: false, - DialContext: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 30 * time.Second, - }).DialContext, - MaxIdleConns: 100, - IdleConnTimeout: 90 * time.Second, - TLSHandshakeTimeout: 30 * time.Second, - TLSNextProto: make(map[string]func(authority string, c *tls.Conn) http.RoundTripper), - } - httpClient = &http.Client{ - Timeout: 60 * time.Second, - Transport: &authenticatedTransport{ - Token: conf.Token, - Delegate: tr, - }, - } - } else { - tr := &http2.Transport{ - ReadIdleTimeout: 30 * time.Second, - } - if conf.TLSConfig != nil { - tr.TLSClientConfig = conf.TLSConfig - } + if httpClient != nil { + return &Client{ + logger: l, + client: httpClient, + conf: conf, + } + } - httpClient = &http.Client{ - Timeout: 60 * time.Second, - Transport: &authenticatedTransport{ - Token: conf.Token, - Delegate: tr, - }, - } + // Base any modifications on the default transport. + transport := http.DefaultTransport.(*http.Transport).Clone() + // Allow override of TLSConfig. This must be set prior to calling + // http2.ConfigureTransports. + if conf.TLSConfig != nil { + transport.TLSClientConfig = conf.TLSConfig + } + + if conf.DisableHTTP2 { + transport.TLSNextProto = make(map[string]func(string, *tls.Conn) http.RoundTripper) + // The default TLSClientConfig has h2 in NextProtos, so the + // negotiated TLS connection will assume h2 support. + // see https://github.com/golang/go/issues/50571 + transport.TLSClientConfig.NextProtos = []string{"http/1.1"} + } else { + // There is a bug in http2 on Linux regarding using dead connections. + // This is a workaround. See https://github.com/golang/go/issues/59690 + // + // Note that http2.ConfigureTransports alters its argument in order to + // supply http2 functionality, and the http2.Transport does not support + // HTTP/1.1 as a protocol, so we get slightly odd-looking code where + // we use `transport` later on instead of the just-returned `tr2`. + // tr2 is needed merely to configure the http2 option. + tr2, err := http2.ConfigureTransports(transport) + if err != nil { + l.Warn("Failed to configure HTTP2 transports: %v", err) + } + if tr2 != nil { + tr2.ReadIdleTimeout = 30 * time.Second } } + httpClient = &http.Client{ + Timeout: 60 * time.Second, + Transport: &authenticatedTransport{ + Token: conf.Token, + Delegate: transport, + }, + } + return &Client{ logger: l, client: httpClient, diff --git a/go.mod b/go.mod index f42caf83e8..4278459436 100644 --- a/go.mod +++ b/go.mod @@ -50,6 +50,7 @@ require ( go.opentelemetry.io/otel/trace v1.30.0 golang.org/x/crypto v0.27.0 golang.org/x/exp v0.0.0-20231108232855-2478ac86f678 + golang.org/x/net v0.29.0 golang.org/x/oauth2 v0.23.0 golang.org/x/sys v0.25.0 golang.org/x/term v0.24.0 @@ -128,7 +129,6 @@ require ( go.uber.org/atomic v1.11.0 // indirect go.uber.org/multierr v1.11.0 // indirect golang.org/x/mod v0.17.0 // indirect - golang.org/x/net v0.29.0 // indirect golang.org/x/sync v0.8.0 // indirect golang.org/x/text v0.18.0 // indirect golang.org/x/time v0.6.0 // indirect