From 42281ab7f011f16e7b4cbbb2648594bc70ec2060 Mon Sep 17 00:00:00 2001 From: Patrick Robinson Date: Mon, 23 Sep 2024 14:44:23 +1000 Subject: [PATCH 1/2] Attempt to workaround https://github.com/golang/go/issues/59690 --- api/client.go | 60 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 21 deletions(-) diff --git a/api/client.go b/api/client.go index e62d859025..ef58ec814b 100644 --- a/api/client.go +++ b/api/client.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/http/httptrace" "net/http/httputil" @@ -22,6 +23,7 @@ import ( "github.com/buildkite/agent/v3/logger" "github.com/google/go-querystring/query" + "golang.org/x/net/http2" ) const ( @@ -80,30 +82,44 @@ func NewClient(l logger.Logger, conf Config) *Client { } httpClient := conf.HTTPClient - if conf.HTTPClient == nil { - - // use the default transport as it is optimized and configured for http2 - // and will avoid accidents in the future - tr := http.DefaultTransport.(*http.Transport).Clone() + if conf.HTTPClient == nil { if conf.DisableHTTP2 { - tr.ForceAttemptHTTP2 = false - tr.TLSNextProto = make(map[string]func(authority string, c *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 - tr.TLSClientConfig.NextProtos = []string{"http/1.1"} - } - - if conf.TLSConfig != nil { - tr.TLSClientConfig = conf.TLSConfig - } + 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 + } - httpClient = &http.Client{ - Timeout: 60 * time.Second, - Transport: &authenticatedTransport{ - Token: conf.Token, - Delegate: tr, - }, + httpClient = &http.Client{ + Timeout: 60 * time.Second, + Transport: &authenticatedTransport{ + Token: conf.Token, + Delegate: tr, + }, + } } } @@ -270,6 +286,7 @@ func (c *Client) doRequest(req *http.Request, v any) (*Response, error) { c.logger.Debug("%s %s", req.Method, req.URL) resp, err := c.client.Do(req) + if err != nil { if c.conf.TraceHTTP { tracer.EmitTraceToLog(logger.ERROR) @@ -377,6 +394,7 @@ func traceHTTPRequest(req *http.Request, t *tracer) *http.Request { t.LogField("reused", strconv.FormatBool(info.Reused)) t.LogField("idle", strconv.FormatBool(info.WasIdle)) t.LogDuration("idleTime", info.IdleTime) + t.LogField("localAddr", info.Conn.LocalAddr().String()) }, PutIdleConn: func(err error) { t.LogTiming("putIdleConn") From e407ac340ff0c2c751982cc1d5bf314f2d20b6f4 Mon Sep 17 00:00:00 2001 From: Josh Deprez Date: Mon, 23 Sep 2024 15:33:09 +1000 Subject: [PATCH 2/2] Attempt to workaround golang/go#59690 --- api/auth.go | 54 ++++++++++++++++++++++++++++------ api/client.go | 81 ++++++++++++++++++++++++++++----------------------- go.mod | 2 +- 3 files changed, 90 insertions(+), 47 deletions(-) diff --git a/api/auth.go b/api/auth.go index 1fb28da103..2b9af46476 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,59 @@ 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) { + // Per net/http#RoundTripper: + // + // "RoundTrip must always close the body, including on errors, ..." + reqBodyClosed := false + if req.Body != nil { + defer func() { + if !reqBodyClosed { + req.Body.Close() + } + }() + } + 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)) + // req.Body is assumed to be closed by the delegate. + reqBodyClosed = true 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