From 1cf1b107555a43246cee63bfe0ade6abf1f32fa0 Mon Sep 17 00:00:00 2001 From: Stephane Date: Tue, 26 Sep 2023 11:56:54 +1000 Subject: [PATCH 1/3] add http2 connection configuration to handle tcp drop --- pkg/middleware/default_http_client.go | 70 ++++++++++++++++------ pkg/middleware/default_http_client_test.go | 22 +++++++ 2 files changed, 74 insertions(+), 18 deletions(-) create mode 100644 pkg/middleware/default_http_client_test.go diff --git a/pkg/middleware/default_http_client.go b/pkg/middleware/default_http_client.go index d81465f..7885bdd 100644 --- a/pkg/middleware/default_http_client.go +++ b/pkg/middleware/default_http_client.go @@ -23,6 +23,7 @@ import ( "github.com/Azure/go-armbalancer" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" "go.opentelemetry.io/otel/propagation" + "golang.org/x/net/http2" ) var ( @@ -37,30 +38,63 @@ func DefaultHTTPClient() *http.Client { } func init() { + httpTransport := &http.Transport{ + Proxy: http.ProxyFromEnvironment, + DialContext: (&net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + }).DialContext, + ForceAttemptHTTP2: true, + MaxIdleConns: 100, + MaxIdleConnsPerHost: 100, + IdleConnTimeout: 90 * time.Second, + TLSHandshakeTimeout: 10 * time.Second, + ExpectContinueTimeout: 1 * time.Second, + TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, + }, + } + // We call configureHttp2TransportPing() in the package init to ensure that our defaultTransport is always configured + // with the http2 additional settings that work around the issue described here: + // https://github.com/golang/go/issues/59690 + // azure sdk related issue is here: + // https://github.com/Azure/azure-sdk-for-go/issues/21346#issuecomment-1699665586 + configureHttp2TransportPing(httpTransport) defaultTransport = armbalancer.New(armbalancer.Options{ // PoolSize is the number of clientside http/2 persistent connections // we want to have configured in our transport. Note, that without clientside loadbalancing // with arm, HTTP/2 Will force persistent connection to stick to a single arm instance, and will // result in a substantial amount of throttling - PoolSize: 100, - Transport: &http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 30 * time.Second, - }).DialContext, - ForceAttemptHTTP2: true, - MaxIdleConns: 100, - MaxIdleConnsPerHost: 100, - IdleConnTimeout: 90 * time.Second, - TLSHandshakeTimeout: 10 * time.Second, - ExpectContinueTimeout: 1 * time.Second, - TLSClientConfig: &tls.Config{ - MinVersion: tls.VersionTLS12, - }, - }, - }) + PoolSize: 100, + Transport: httpTransport, + }, + ) + defaultHTTPClient = &http.Client{ Transport: otelhttp.NewTransport(defaultTransport, otelhttp.WithPropagators(propagation.TraceContext{})), } } + +// configureHttp2Transport ensures that our defaultTransport is configured +// with the http2 additional settings that work around the issue described here: +// https://github.com/golang/go/issues/59690 +// azure sdk related issue is here: +// https://github.com/Azure/azure-sdk-for-go/issues/21346#issuecomment-1699665586 +// This is called by the package init to ensure that our defaultTransport is always configured +// you should not call this anywhere else. +func configureHttp2TransportPing(tr *http.Transport) { + // http2Transport holds a reference to the default transport and configures "h2" middlewares that + // will use the below settings, making the standard http.Transport behave correctly for dropped connections + http2Transport, err := http2.ConfigureTransports(tr) + if err != nil { + // by initializing in init(), we know it is only called once. + // this errors if called twice. + panic(err) + } + // we give 10s to the server to respond to the ping. if no response is received, + // the transport will close the connection, so that the next request will open a new connection, and not + // hit a context deadline exceeded error. + http2Transport.PingTimeout = 10 * time.Second + // if no frame is received for 30s, the transport will issue a ping health check to the server. + http2Transport.ReadIdleTimeout = 30 * time.Second +} diff --git a/pkg/middleware/default_http_client_test.go b/pkg/middleware/default_http_client_test.go new file mode 100644 index 0000000..60f40cb --- /dev/null +++ b/pkg/middleware/default_http_client_test.go @@ -0,0 +1,22 @@ +package middleware + +import ( + "crypto/tls" + "net/http" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestConfigureHttp2TransportPing(t *testing.T) { + t.Run("transport should be setup with http2Transport h2 middleware", func(t *testing.T) { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, + }, + } + require.NotContains(t, tr.TLSClientConfig.NextProtos, "h2") + configureHttp2TransportPing(tr) + require.Contains(t, tr.TLSClientConfig.NextProtos, "h2") + }) +} From 63da1220b932942ed695156ca73c02006f13c8e4 Mon Sep 17 00:00:00 2001 From: Stephane Date: Tue, 26 Sep 2023 12:12:14 +1000 Subject: [PATCH 2/3] add more tests --- pkg/middleware/default_http_client.go | 16 ++++++++++------ pkg/middleware/default_http_client_test.go | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/pkg/middleware/default_http_client.go b/pkg/middleware/default_http_client.go index 7885bdd..a01f45b 100644 --- a/pkg/middleware/default_http_client.go +++ b/pkg/middleware/default_http_client.go @@ -27,8 +27,12 @@ import ( ) var ( + // defaultHTTPClient is configured with the defaultRoundTripper defaultHTTPClient *http.Client - defaultTransport http.RoundTripper + // defaultTransport is a pre-configured *http.Transport for http/2 + defaultTransport *http.Transport + // defaultRoundTripper wraps the defaultTransport with arm balancer and otel propagation + defaultRoundTripper http.RoundTripper ) // DefaultHTTPClient returns a shared http client, and transport leveraging armbalancer for @@ -38,7 +42,7 @@ func DefaultHTTPClient() *http.Client { } func init() { - httpTransport := &http.Transport{ + defaultTransport = &http.Transport{ Proxy: http.ProxyFromEnvironment, DialContext: (&net.Dialer{ Timeout: 30 * time.Second, @@ -59,19 +63,19 @@ func init() { // https://github.com/golang/go/issues/59690 // azure sdk related issue is here: // https://github.com/Azure/azure-sdk-for-go/issues/21346#issuecomment-1699665586 - configureHttp2TransportPing(httpTransport) - defaultTransport = armbalancer.New(armbalancer.Options{ + configureHttp2TransportPing(defaultTransport) + defaultRoundTripper = armbalancer.New(armbalancer.Options{ // PoolSize is the number of clientside http/2 persistent connections // we want to have configured in our transport. Note, that without clientside loadbalancing // with arm, HTTP/2 Will force persistent connection to stick to a single arm instance, and will // result in a substantial amount of throttling PoolSize: 100, - Transport: httpTransport, + Transport: defaultTransport, }, ) defaultHTTPClient = &http.Client{ - Transport: otelhttp.NewTransport(defaultTransport, otelhttp.WithPropagators(propagation.TraceContext{})), + Transport: otelhttp.NewTransport(defaultRoundTripper, otelhttp.WithPropagators(propagation.TraceContext{})), } } diff --git a/pkg/middleware/default_http_client_test.go b/pkg/middleware/default_http_client_test.go index 60f40cb..3b05395 100644 --- a/pkg/middleware/default_http_client_test.go +++ b/pkg/middleware/default_http_client_test.go @@ -19,4 +19,22 @@ func TestConfigureHttp2TransportPing(t *testing.T) { configureHttp2TransportPing(tr) require.Contains(t, tr.TLSClientConfig.NextProtos, "h2") }) + + t.Run("configuring transport twice panics", func(t *testing.T) { + tr := &http.Transport{ + TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, + }, + } + require.NotContains(t, tr.TLSClientConfig.NextProtos, "h2") + require.NotPanics(t, func() { configureHttp2TransportPing(tr) }) + require.Panics(t, func() { configureHttp2TransportPing(tr) }) + require.Contains(t, tr.TLSClientConfig.NextProtos, "h2") + }) + + t.Run("defaultTransport is configured with h2 by default", func(t *testing.T) { + // should panic because it's already configured + require.Panics(t, func() { configureHttp2TransportPing(defaultTransport) }) + require.Contains(t, defaultTransport.TLSClientConfig.NextProtos, "h2") + }) } From 6549a71778575ec4191c34a42bb3de15899217ea Mon Sep 17 00:00:00 2001 From: Stephane Date: Tue, 26 Sep 2023 12:15:45 +1000 Subject: [PATCH 3/3] new line --- pkg/middleware/default_http_client.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/middleware/default_http_client.go b/pkg/middleware/default_http_client.go index a01f45b..16e8ac4 100644 --- a/pkg/middleware/default_http_client.go +++ b/pkg/middleware/default_http_client.go @@ -71,8 +71,7 @@ func init() { // result in a substantial amount of throttling PoolSize: 100, Transport: defaultTransport, - }, - ) + }) defaultHTTPClient = &http.Client{ Transport: otelhttp.NewTransport(defaultRoundTripper, otelhttp.WithPropagators(propagation.TraceContext{})),