Repository: prometheus/common
Pull request #249 (2020-08-03 19:43:13)

Disable HTTP/2
HTTP/2 support is golang has many problematic cornercases where dead
connections would be kept.

https://github.com/golang/go/issues/32388
https://github.com/golang/go/issues/39337
https://github.com/golang/go/issues/39750

I suggest we disable HTTP/2 for now and enable it manually on the
blackbox exporter.

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>

brian-brazil: How will the blackbox exporter turn it back on via this code? It doesn't do persistent connections, so those issues likely don't affect it.
roidelapluie: What about this?
brian-brazil: Let's not start hiding things in the YAML, we currently use function arguments for this - and the one that's there is already only used by blackbox.
roidelapluie: @brian-brazil I have adapted it.
roidelapluie: I have put the golang issues in comment.

Patch:
diff --git a/config/http_config.go b/config/http_config.go
index 9b1fad9..3a49aa7 100644
--- a/config/http_config.go
+++ b/config/http_config.go
@@ -123,8 +123,8 @@ func newClient(rt http.RoundTripper) *http.Client {
 
 // NewClientFromConfig returns a new HTTP client configured for the
 // given config.HTTPClientConfig. The name is used as go-conntrack metric label.
-func NewClientFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives bool) (*http.Client, error) {
-	rt, err := NewRoundTripperFromConfig(cfg, name, disableKeepAlives)
+func NewClientFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives, enableHTTP2 bool) (*http.Client, error) {
+	rt, err := NewRoundTripperFromConfig(cfg, name, disableKeepAlives, enableHTTP2)
 	if err != nil {
 		return nil, err
 	}
@@ -133,7 +133,7 @@ func NewClientFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives bo
 
 // NewRoundTripperFromConfig returns a new HTTP RoundTripper configured for the
 // given config.HTTPClientConfig. The name is used as go-conntrack metric label.
-func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives bool) (http.RoundTripper, error) {
+func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives, enableHTTP2 bool) (http.RoundTripper, error) {
 	newRT := func(tlsConfig *tls.Config) (http.RoundTripper, error) {
 		// The only timeout we care about is the configured scrape timeout.
 		// It is applied on request. So we leave out any timings here.
@@ -154,10 +154,18 @@ func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAli
 				conntrack.DialWithName(name),
 			),
 		}
-		// TODO: use ForceAttemptHTTP2 when we move to Go 1.13+.
-		err := http2.ConfigureTransport(rt.(*http.Transport))
-		if err != nil {
-			return nil, err
+		if enableHTTP2 {
+			// HTTP/2 support is golang has many problematic cornercases where
+			// dead connections would be kept and used in connection pools.
+			// https://github.com/golang/go/issues/32388
+			// https://github.com/golang/go/issues/39337
+			// https://github.com/golang/go/issues/39750
+			// TODO: Re-Enable HTTP/2 once upstream issue is fixed.
+			// TODO: use ForceAttemptHTTP2 when we move to Go 1.13+.
+			err := http2.ConfigureTransport(rt.(*http.Transport))
+			if err != nil {
+				return nil, err
+			}
 		}
 
 		// If a bearer token is provided, create a round tripper that will set the


Test patch:
diff --git a/config/http_config_test.go b/config/http_config_test.go
index 12dff96..8596e80 100644
--- a/config/http_config_test.go
+++ b/config/http_config_test.go
@@ -205,7 +205,7 @@ func TestNewClientFromConfig(t *testing.T) {
 		}
 		defer testServer.Close()
 
-		client, err := NewClientFromConfig(validConfig.clientConfig, "test", false)
+		client, err := NewClientFromConfig(validConfig.clientConfig, "test", false, true)
 		if err != nil {
 			t.Errorf("Can't create a client from this config: %+v", validConfig.clientConfig)
 			continue
@@ -255,7 +255,7 @@ func TestNewClientFromInvalidConfig(t *testing.T) {
 	}
 
 	for _, invalidConfig := range newClientInvalidConfig {
-		client, err := NewClientFromConfig(invalidConfig.clientConfig, "test", false)
+		client, err := NewClientFromConfig(invalidConfig.clientConfig, "test", false, true)
 		if client != nil {
 			t.Errorf("A client instance was returned instead of nil using this config: %+v", invalidConfig.clientConfig)
 		}
@@ -294,7 +294,7 @@ func TestMissingBearerAuthFile(t *testing.T) {
 	}
 	defer testServer.Close()
 
-	client, err := NewClientFromConfig(cfg, "test", false)
+	client, err := NewClientFromConfig(cfg, "test", false, true)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -483,7 +483,7 @@ func TestBasicAuthNoPassword(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error loading HTTP client config: %v", err)
 	}
-	client, err := NewClientFromConfig(*cfg, "test", false)
+	client, err := NewClientFromConfig(*cfg, "test", false, true)
 	if err != nil {
 		t.Fatalf("Error creating HTTP Client: %v", err)
 	}
@@ -509,7 +509,7 @@ func TestBasicAuthNoUsername(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error loading HTTP client config: %v", err)
 	}
-	client, err := NewClientFromConfig(*cfg, "test", false)
+	client, err := NewClientFromConfig(*cfg, "test", false, true)
 	if err != nil {
 		t.Fatalf("Error creating HTTP Client: %v", err)
 	}
@@ -535,7 +535,7 @@ func TestBasicAuthPasswordFile(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error loading HTTP client config: %v", err)
 	}
-	client, err := NewClientFromConfig(*cfg, "test", false)
+	client, err := NewClientFromConfig(*cfg, "test", false, true)
 	if err != nil {
 		t.Fatalf("Error creating HTTP Client: %v", err)
 	}
@@ -686,7 +686,7 @@ func TestTLSRoundTripper(t *testing.T) {
 			writeCertificate(bs, tc.cert, cert)
 			writeCertificate(bs, tc.key, key)
 			if c == nil {
-				c, err = NewClientFromConfig(cfg, "test", false)
+				c, err = NewClientFromConfig(cfg, "test", false, true)
 				if err != nil {
 					t.Fatalf("Error creating HTTP Client: %v", err)
 				}
@@ -758,7 +758,7 @@ func TestTLSRoundTripperRaces(t *testing.T) {
 	writeCertificate(bs, TLSCAChainPath, ca)
 	writeCertificate(bs, ClientCertificatePath, cert)
 	writeCertificate(bs, ClientKeyNoPassPath, key)
-	c, err = NewClientFromConfig(cfg, "test", false)
+	c, err = NewClientFromConfig(cfg, "test", false, true)
 	if err != nil {
 		t.Fatalf("Error creating HTTP Client: %v", err)
 	}