From fe49fa93abfac8ed0e42e5ef153530e5ceb388c8 Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Tue, 4 Aug 2020 00:03:53 +0200 Subject: [PATCH 1/3] Disable HTTP2 by default Signed-off-by: Julien Pivotto --- config/http_config.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/config/http_config.go b/config/http_config.go index 9b1fad93..5f208e9b 100644 --- a/config/http_config.go +++ b/config/http_config.go @@ -84,6 +84,9 @@ type HTTPClientConfig struct { ProxyURL URL `yaml:"proxy_url,omitempty"` // TLSConfig to use to connect to the targets. TLSConfig TLSConfig `yaml:"tls_config,omitempty"` + // EnableHTTP2 enables HTTP2 transport. Not exposed via Yaml to users, as it + // should only be used without persistent connections. + EnableHTTP2 bool `yaml:"-"` } // Validate validates the HTTPClientConfig to check only one of BearerToken, @@ -154,10 +157,12 @@ 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 cfg.EnableHTTP2 { + // 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 From a9d9779f597d0186833d6b5a2d2828a203908175 Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Fri, 7 Aug 2020 21:24:23 +0200 Subject: [PATCH 2/3] Use a function arg Signed-off-by: Julien Pivotto --- config/http_config.go | 11 ++++------- config/http_config_test.go | 16 ++++++++-------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/config/http_config.go b/config/http_config.go index 5f208e9b..6fefe8f1 100644 --- a/config/http_config.go +++ b/config/http_config.go @@ -84,9 +84,6 @@ type HTTPClientConfig struct { ProxyURL URL `yaml:"proxy_url,omitempty"` // TLSConfig to use to connect to the targets. TLSConfig TLSConfig `yaml:"tls_config,omitempty"` - // EnableHTTP2 enables HTTP2 transport. Not exposed via Yaml to users, as it - // should only be used without persistent connections. - EnableHTTP2 bool `yaml:"-"` } // Validate validates the HTTPClientConfig to check only one of BearerToken, @@ -126,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 } @@ -136,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. @@ -157,7 +154,7 @@ func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAli conntrack.DialWithName(name), ), } - if cfg.EnableHTTP2 { + if enableHTTP2 { // TODO: use ForceAttemptHTTP2 when we move to Go 1.13+. err := http2.ConfigureTransport(rt.(*http.Transport)) if err != nil { diff --git a/config/http_config_test.go b/config/http_config_test.go index 12dff968..8596e80b 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) } From a21561760a6b5890d1433a95f4817cb8ba64724b Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Mon, 10 Aug 2020 12:12:43 +0200 Subject: [PATCH 3/3] Add comment Signed-off-by: Julien Pivotto --- config/http_config.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/config/http_config.go b/config/http_config.go index 6fefe8f1..3a49aa7c 100644 --- a/config/http_config.go +++ b/config/http_config.go @@ -155,6 +155,12 @@ func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAli ), } 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 {