Merge branch 'id-dry-http-client' into 'master'

Deduplicate http transport creation

See merge request gitlab-org/gitlab!69471

workhorse/internal/dependencyproxy/dependencyproxy.go

@@ -4,37 +4,17 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"net"
 	"net/http"
-	"time"
 
-	"gitlab.com/gitlab-org/labkit/correlation"
 	"gitlab.com/gitlab-org/labkit/log"
-	"gitlab.com/gitlab-org/labkit/tracing"
 
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
+	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/senddata"
 )
 
-// httpTransport defines a http.Transport with values
-// that are more restrictive than for http.DefaultTransport,
-// they define shorter TLS Handshake, and more aggressive connection closing
-// to prevent the connection hanging and reduce FD usage
-var httpTransport = tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{
-	Proxy: http.ProxyFromEnvironment,
-	DialContext: (&net.Dialer{
-		Timeout:   30 * time.Second,
-		KeepAlive: 10 * time.Second,
-	}).DialContext,
-	MaxIdleConns:          2,
-	IdleConnTimeout:       30 * time.Second,
-	TLSHandshakeTimeout:   10 * time.Second,
-	ExpectContinueTimeout: 10 * time.Second,
-	ResponseHeaderTimeout: 30 * time.Second,
-}))
-
 var httpClient = &http.Client{
-	Transport: httpTransport,
+	Transport: httptransport.New(),
 }
 
 type Injector struct {

workhorse/internal/helper/httptransport/http_transport.go

+package httptransport
+
+import (
+	"net/http"
+	"time"
+
+	"gitlab.com/gitlab-org/labkit/correlation"
+	"gitlab.com/gitlab-org/labkit/tracing"
+)
+
+type Option func(*http.Transport)
+
+// Defines a http.Transport with values
+// that are more restrictive than for http.DefaultTransport,
+// they define shorter TLS Handshake, and more aggressive connection closing
+// to prevent the connection hanging and reduce FD usage
+func New(options ...Option) http.RoundTripper {
+	t := http.DefaultTransport.(*http.Transport).Clone()
+
+	// To avoid keep around TCP connections to http servers we're done with
+	t.MaxIdleConns = 2
+
+	// A stricter timeout for fetching from external sources that can be slow
+	t.ResponseHeaderTimeout = 30 * time.Second
+
+	for _, option := range options {
+		option(t)
+	}
+
+	return tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(t))
+}
+
+func WithDisabledCompression() Option {
+	return func(t *http.Transport) {
+		t.DisableCompression = true
+	}
+}

workhorse/internal/imageresizer/image_resizer.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"net"
 	"net/http"
 	"os"
 	"os/exec"
@@ -18,11 +17,11 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 
-	"gitlab.com/gitlab-org/labkit/correlation"
 	"gitlab.com/gitlab-org/labkit/tracing"
 
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/config"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
+	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/log"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/senddata"
 )
@@ -69,23 +68,8 @@ const (
 
 var envInjector = tracing.NewEnvInjector()
 
-// Images might be located remotely in object storage, in which case we need to stream
-// it via http(s)
-var httpTransport = tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{
-	Proxy: http.ProxyFromEnvironment,
-	DialContext: (&net.Dialer{
-		Timeout:   30 * time.Second,
-		KeepAlive: 10 * time.Second,
-	}).DialContext,
-	MaxIdleConns:          2,
-	IdleConnTimeout:       30 * time.Second,
-	TLSHandshakeTimeout:   10 * time.Second,
-	ExpectContinueTimeout: 10 * time.Second,
-	ResponseHeaderTimeout: 30 * time.Second,
-}))
-
 var httpClient = &http.Client{
-	Transport: httpTransport,
+	Transport: httptransport.New(),
 }
 
 const (

workhorse/internal/objectstore/object.go

@@ -5,34 +5,15 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"net"
 	"net/http"
-	"time"
 
-	"gitlab.com/gitlab-org/labkit/correlation"
 	"gitlab.com/gitlab-org/labkit/mask"
-	"gitlab.com/gitlab-org/labkit/tracing"
-)
 
-// httpTransport defines a http.Transport with values
-// that are more restrictive than for http.DefaultTransport,
-// they define shorter TLS Handshake, and more aggressive connection closing
-// to prevent the connection hanging and reduce FD usage
-var httpTransport = tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{
-	Proxy: http.ProxyFromEnvironment,
-	DialContext: (&net.Dialer{
-		Timeout:   30 * time.Second,
-		KeepAlive: 10 * time.Second,
-	}).DialContext,
-	MaxIdleConns:          2,
-	IdleConnTimeout:       30 * time.Second,
-	TLSHandshakeTimeout:   10 * time.Second,
-	ExpectContinueTimeout: 10 * time.Second,
-	ResponseHeaderTimeout: 30 * time.Second,
-}))
+	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport"
+)
 
 var httpClient = &http.Client{
-	Transport: httpTransport,
+	Transport: httptransport.New(),
 }
 
 // Object represents an object on a S3 compatible Object Store service.

workhorse/internal/sendurl/sendurl.go

@@ -3,18 +3,15 @@ package sendurl
 import (
 	"fmt"
 	"io"
-	"net"
 	"net/http"
-	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 
-	"gitlab.com/gitlab-org/labkit/correlation"
 	"gitlab.com/gitlab-org/labkit/mask"
-	"gitlab.com/gitlab-org/labkit/tracing"
 
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
+	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/log"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/senddata"
 )
@@ -47,22 +44,7 @@ var preserveHeaderKeys = map[string]bool{
 	"Pragma":        true, // Support for HTTP 1.0 proxies
 }
 
-// httpTransport defines a http.Transport with values
-// that are more restrictive than for http.DefaultTransport,
-// they define shorter TLS Handshake, and more aggressive connection closing
-// to prevent the connection hanging and reduce FD usage
-var httpTransport = tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{
-	Proxy: http.ProxyFromEnvironment,
-	DialContext: (&net.Dialer{
-		Timeout:   30 * time.Second,
-		KeepAlive: 10 * time.Second,
-	}).DialContext,
-	MaxIdleConns:          2,
-	IdleConnTimeout:       30 * time.Second,
-	TLSHandshakeTimeout:   10 * time.Second,
-	ExpectContinueTimeout: 10 * time.Second,
-	ResponseHeaderTimeout: 30 * time.Second,
-}))
+var httpTransport = httptransport.New()
 
 var httpClient = &http.Client{
 	Transport: httpTransport,

workhorse/internal/upstream/roundtripper/roundtripper.go

@@ -32,19 +32,23 @@ func NewBackendRoundTripper(backend *url.URL, socket string, proxyHeadersTimeout
 }
 
 func newBackendRoundTripper(backend *url.URL, socket string, proxyHeadersTimeout time.Duration, developmentMode bool, tlsConf *tls.Config) http.RoundTripper {
-	// Copied from the definition of http.DefaultTransport. We can't literally copy http.DefaultTransport because of its hidden internal state.
-	transport, dialer := newBackendTransport()
+	transport := http.DefaultTransport.(*http.Transport).Clone()
 	transport.ResponseHeaderTimeout = proxyHeadersTimeout
 	transport.TLSClientConfig = tlsConf
 
+	// Puma does not support http/2, there's no point in reconnecting
+	transport.ForceAttemptHTTP2 = false
+
+	dial := transport.DialContext
+
 	if backend != nil && socket == "" {
 		address := mustParseAddress(backend.Host, backend.Scheme)
 		transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
-			return dialer.DialContext(ctx, "tcp", address)
+			return dial(ctx, "tcp", address)
 		}
 	} else if socket != "" {
 		transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
-			return dialer.DialContext(ctx, "unix", socket)
+			return dial(ctx, "unix", socket)
 		}
 	} else {
 		panic("backend is nil and socket is empty")

workhorse/internal/upstream/roundtripper/transport.go

-package roundtripper
-
-import (
-	"net"
-	"net/http"
-	"time"
-)
-
-// newBackendTransport setups the default HTTP transport which Workhorse uses
-// to communicate with the upstream
-func newBackendTransport() (*http.Transport, *net.Dialer) {
-	dialler := &net.Dialer{
-		Timeout:   30 * time.Second,
-		KeepAlive: 30 * time.Second,
-	}
-
-	transport := &http.Transport{
-		Proxy:                 http.ProxyFromEnvironment,
-		DialContext:           dialler.DialContext,
-		MaxIdleConns:          100,
-		IdleConnTimeout:       90 * time.Second,
-		TLSHandshakeTimeout:   10 * time.Second,
-		ExpectContinueTimeout: 1 * time.Second,
-	}
-
-	return transport, dialler
-}

workhorse/internal/zipartifacts/open_archive.go

@@ -5,32 +5,20 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"net"
 	"net/http"
 	"os"
 	"strings"
-	"time"
 
+	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/httprs"
 
-	"gitlab.com/gitlab-org/labkit/correlation"
 	"gitlab.com/gitlab-org/labkit/mask"
-	"gitlab.com/gitlab-org/labkit/tracing"
 )
 
 var httpClient = &http.Client{
-	Transport: tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{
-		Proxy: http.ProxyFromEnvironment,
-		DialContext: (&net.Dialer{
-			Timeout:   30 * time.Second,
-			KeepAlive: 10 * time.Second,
-		}).DialContext,
-		IdleConnTimeout:       30 * time.Second,
-		TLSHandshakeTimeout:   10 * time.Second,
-		ExpectContinueTimeout: 10 * time.Second,
-		ResponseHeaderTimeout: 30 * time.Second,
-		DisableCompression:    true,
-	})),
+	Transport: httptransport.New(
+		httptransport.WithDisabledCompression(), // To avoid bugs when serving compressed files from object storage
+	),
 }
 
 type archive struct {