Merge branch 'an-refactor-badgateway' into 'master'

Refactor badgateway to use standardlib interfaces

See merge request gitlab-org/gitlab-workhorse!316

authorization_test.go

@@ -10,10 +10,10 @@ import (
 	"github.com/dgrijalva/jwt-go"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/secret"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream/roundtripper"
 )
 
 func okHandler(w http.ResponseWriter, _ *http.Request, _ *api.Response) {
@@ -34,7 +34,7 @@ func runPreAuthorizeHandler(t *testing.T, ts *httptest.Server, suffix string, ur
 	}
 	parsedURL := helper.URLMustParse(ts.URL)
 	testhelper.ConfigureSecret()
-	a := api.NewAPI(parsedURL, "123", badgateway.TestRoundTripper(parsedURL))
+	a := api.NewAPI(parsedURL, "123", roundtripper.NewTestBackendRoundTripper(parsedURL))
 
 	response := httptest.NewRecorder()
 	a.PreAuthorizeHandler(okHandler, suffix).ServeHTTP(response, httpRequest)

internal/api/api.go

@@ -13,7 +13,6 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/gitaly"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/secret"
@@ -56,7 +55,7 @@ func init() {
 	prometheus.MustRegister(bytesTotal)
 }
 
-func NewAPI(myURL *url.URL, version string, roundTripper *badgateway.RoundTripper) *API {
+func NewAPI(myURL *url.URL, version string, roundTripper http.RoundTripper) *API {
 	return &API{
 		Client:  &http.Client{Transport: roundTripper},
 		URL:     myURL,

internal/artifacts/artifacts_upload_test.go

@@ -15,11 +15,11 @@ import (
 	"testing"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/proxy"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream/roundtripper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/zipartifacts"
 )
 
@@ -113,7 +113,7 @@ func testUploadArtifacts(contentType string, body io.Reader, t *testing.T, ts *h
 	httpRequest.Header.Set("Content-Type", contentType)
 	response := httptest.NewRecorder()
 	parsedURL := helper.URLMustParse(ts.URL)
-	roundTripper := badgateway.TestRoundTripper(parsedURL)
+	roundTripper := roundtripper.NewTestBackendRoundTripper(parsedURL)
 	testhelper.ConfigureSecret()
 	apiClient := api.NewAPI(parsedURL, "123", roundTripper)
 	proxyClient := proxy.NewProxy(parsedURL, "123", roundTripper)

internal/badgateway/roundtripper.go

@@ -2,84 +2,30 @@ package badgateway
 
 import (
 	"bytes"
-	"context"
 	"fmt"
 	"io/ioutil"
-	"net"
 	"net/http"
-	"net/url"
 	"time"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
-// defaultDialer is configured to use the values from http.DefaultTransport
-var defaultDialer = &net.Dialer{
-	Timeout:   30 * time.Second,
-	KeepAlive: 30 * time.Second,
-	DualStack: true,
-}
-
 // Error is a custom error for pretty Sentry 'issues'
-type Error struct{ error }
+type sentryError struct{ error }
 
-type RoundTripper struct {
-	Transport       *http.Transport
+type roundTripper struct {
+	next            http.RoundTripper
 	developmentMode bool
 }
 
-func TestRoundTripper(backend *url.URL) *RoundTripper {
-	return NewRoundTripper(backend, "", 0, true)
-}
-
-// NewRoundTripper returns a new RoundTripper instance using the provided values
-func NewRoundTripper(backend *url.URL, socket string, proxyHeadersTimeout time.Duration, developmentMode bool) *RoundTripper {
-	// Copied from the definition of http.DefaultTransport. We can't literally copy http.DefaultTransport because of its hidden internal state.
-	tr := &http.Transport{
-		Proxy:                 http.ProxyFromEnvironment,
-		DialContext:           defaultDialer.DialContext,
-		MaxIdleConns:          100,
-		IdleConnTimeout:       90 * time.Second,
-		TLSHandshakeTimeout:   10 * time.Second,
-		ExpectContinueTimeout: 1 * time.Second,
-	}
-
-	tr.ResponseHeaderTimeout = proxyHeadersTimeout
-
-	if backend != nil && socket == "" {
-		address := mustParseAddress(backend.Host, backend.Scheme)
-		tr.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
-			return defaultDialer.DialContext(ctx, "tcp", address)
-		}
-	} else if socket != "" {
-		tr.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
-			return defaultDialer.DialContext(ctx, "unix", socket)
-		}
-	} else {
-		panic("backend is nil and socket is empty")
-	}
-
-	return &RoundTripper{Transport: tr, developmentMode: developmentMode}
-}
-
-func mustParseAddress(address, scheme string) string {
-	if scheme == "https" {
-		panic("TLS is not supported for backend connections")
-	}
-
-	for _, suffix := range []string{"", ":" + scheme} {
-		address += suffix
-		if host, port, err := net.SplitHostPort(address); err == nil && host != "" && port != "" {
-			return host + ":" + port
-		}
-	}
-
-	panic(fmt.Errorf("could not parse host:port from address %q and scheme %q", address, scheme))
+// NewRoundTripper creates a RoundTripper with a provided underlying next transport
+func NewRoundTripper(developmentMode bool, next http.RoundTripper) http.RoundTripper {
+	return &roundTripper{next: next, developmentMode: developmentMode}
 }
 
-func (t *RoundTripper) RoundTrip(r *http.Request) (res *http.Response, err error) {
+func (t *roundTripper) RoundTrip(r *http.Request) (res *http.Response, err error) {
 	start := time.Now()
-	res, err = t.Transport.RoundTrip(r)
+	res, err = t.next.RoundTrip(r)
 
 	// httputil.ReverseProxy translates all errors from this
 	// RoundTrip function into 500 errors. But the most likely error
@@ -90,7 +36,7 @@ func (t *RoundTripper) RoundTrip(r *http.Request) (res *http.Response, err error
 	if err != nil {
 		helper.LogError(
 			r,
-			&Error{fmt.Errorf("badgateway: failed after %.fs: %v", time.Since(start).Seconds(), err)},
+			&sentryError{fmt.Errorf("badgateway: failed after %.fs: %v", time.Since(start).Seconds(), err)},
 		)
 
 		message := "GitLab is not responding"

internal/proxy/proxy.go

@@ -7,7 +7,6 @@ import (
 	"net/url"
 	"time"
 
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
@@ -21,7 +20,7 @@ type Proxy struct {
 	AllowResponseBuffering bool
 }
 
-func NewProxy(myURL *url.URL, version string, roundTripper *badgateway.RoundTripper) *Proxy {
+func NewProxy(myURL *url.URL, version string, roundTripper http.RoundTripper) *Proxy {
 	p := Proxy{Version: version, AllowResponseBuffering: true}
 
 	if myURL == nil {

internal/upload/uploads_test.go

@@ -17,12 +17,12 @@ import (
 	"time"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/objectstore/test"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/proxy"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream/roundtripper"
 )
 
 var nilHandler = http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})
@@ -325,5 +325,5 @@ func TestInvalidFileNames(t *testing.T) {
 
 func newProxy(url string) *proxy.Proxy {
 	parsedURL := helper.URLMustParse(url)
-	return proxy.NewProxy(parsedURL, "123", badgateway.TestRoundTripper(parsedURL))
+	return proxy.NewProxy(parsedURL, "123", roundtripper.NewTestBackendRoundTripper(parsedURL))
 }

internal/upstream/roundtripper/roundtripper.go

+package roundtripper
+
+import (
+	"context"
+	"fmt"
+	"net"
+	"net/http"
+	"net/url"
+	"time"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
+)
+
+func mustParseAddress(address, scheme string) string {
+	if scheme == "https" {
+		panic("TLS is not supported for backend connections")
+	}
+
+	for _, suffix := range []string{"", ":" + scheme} {
+		address += suffix
+		if host, port, err := net.SplitHostPort(address); err == nil && host != "" && port != "" {
+			return host + ":" + port
+		}
+	}
+
+	panic(fmt.Errorf("could not parse host:port from address %q and scheme %q", address, scheme))
+}
+
+// NewBackendRoundTripper returns a new RoundTripper instance using the provided values
+func NewBackendRoundTripper(backend *url.URL, socket string, proxyHeadersTimeout time.Duration, developmentMode bool) 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.ResponseHeaderTimeout = proxyHeadersTimeout
+
+	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)
+		}
+	} else if socket != "" {
+		transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+			return dialer.DialContext(ctx, "unix", socket)
+		}
+	} else {
+		panic("backend is nil and socket is empty")
+	}
+
+	return badgateway.NewRoundTripper(developmentMode, transport)
+}
+
+// NewTestBackendRoundTripper sets up a RoundTripper for testing purposes
+func NewTestBackendRoundTripper(backend *url.URL) http.RoundTripper {
+	return NewBackendRoundTripper(backend, "", 0, true)
+}

internal/badgateway/roundtripper_test.go → internal/upstream/roundtripper/roundtripper_test.go

-package badgateway
+package roundtripper
 
 import (
 	"testing"

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,
+		DualStack: true,
+	}
+
+	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
+}

internal/upstream/upstream.go

@@ -13,10 +13,10 @@ import (
 
 	"github.com/sebest/xff"
 
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upload"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream/roundtripper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/urlprefix"
 )
 
@@ -31,7 +31,7 @@ type upstream struct {
 	config.Config
 	URLPrefix    urlprefix.Prefix
 	Routes       []routeEntry
-	RoundTripper *badgateway.RoundTripper
+	RoundTripper http.RoundTripper
 }
 
 func NewUpstream(cfg config.Config) http.Handler {
@@ -41,7 +41,7 @@ func NewUpstream(cfg config.Config) http.Handler {
 	if up.Backend == nil {
 		up.Backend = DefaultBackend
 	}
-	up.RoundTripper = badgateway.NewRoundTripper(up.Backend, up.Socket, up.ProxyHeadersTimeout, cfg.DevelopmentMode)
+	up.RoundTripper = roundtripper.NewBackendRoundTripper(up.Backend, up.Socket, up.ProxyHeadersTimeout, cfg.DevelopmentMode)
 	up.configureURLPrefix()
 	up.configureRoutes()
 	return &up

internal/zipartifacts/open_archive.go

@@ -19,7 +19,7 @@ import (
 // ErrNotAZip will be used when the file is not a zip archive
 var ErrNotAZip = errors.New("not a zip")
 
-// ErrNotAZip will be used when the file can't be found
+// ErrArchiveNotFound will be used when the file can't be found
 var ErrArchiveNotFound = errors.New("archive not found")
 
 var httpClient = &http.Client{

proxy_test.go

@@ -16,14 +16,15 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/proxy"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream/roundtripper"
 )
 
 const testVersion = "123"
 
-func newProxy(url string, rt *badgateway.RoundTripper) *proxy.Proxy {
+func newProxy(url string, rt http.RoundTripper) *proxy.Proxy {
 	parsedURL := helper.URLMustParse(url)
 	if rt == nil {
-		rt = badgateway.TestRoundTripper(parsedURL)
+		rt = roundtripper.NewTestBackendRoundTripper(parsedURL)
 	}
 	return proxy.NewProxy(parsedURL, testVersion, rt)
 }
@@ -96,8 +97,7 @@ func TestProxyReadTimeout(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	rt := &badgateway.RoundTripper{
-		Transport: &http.Transport{
+	rt := badgateway.NewRoundTripper(false, &http.Transport{
 		Proxy: http.ProxyFromEnvironment,
 		Dial: (&net.Dialer{
 			Timeout:   30 * time.Second,
@@ -105,8 +105,7 @@ func TestProxyReadTimeout(t *testing.T) {
 		}).Dial,
 		TLSHandshakeTimeout:   10 * time.Second,
 		ResponseHeaderTimeout: time.Millisecond,
-		},
-	}
+	})
 
 	p := newProxy(ts.URL, rt)
 	w := httptest.NewRecorder()