Make golint happy and introduce proxying tests covering proxyRoundTripper

deploy_page_test.go

@@ -48,10 +48,6 @@ func TestIfDeployPageExist(t *testing.T) {
 	}
 	w.Flush()
 
-	if w.Code != 200 {
-		t.Error("Page should be 200")
-	}
-	if w.Body.String() != deployPage {
-		t.Error("Page should be deploy: ", w.Body.String())
-	}
+	assertResponseCode(t, w, 200)
+	assertResponseBody(t, w, deployPage)
 }

error_pages_test.go

@@ -28,12 +28,8 @@ func TestIfErrorPageIsPresented(t *testing.T) {
 	})(w, nil)
 	w.Flush()
 
-	if w.Code != 404 {
-		t.Error("Page should be 404")
-	}
-	if w.Body.String() != errorPage {
-		t.Error("Page should be custom error page: ", w.Body.String())
-	}
+	assertResponseCode(t, w, 404)
+	assertResponseBody(t, w, errorPage)
 }
 
 func TestIfErrorPassedIfNoErrorPageIsFound(t *testing.T) {
@@ -52,10 +48,6 @@ func TestIfErrorPassedIfNoErrorPageIsFound(t *testing.T) {
 	})(w, nil)
 	w.Flush()
 
-	if w.Code != 404 {
-		t.Error("Page should be 400")
-	}
-	if w.Body.String() != errorResponse {
-		t.Error("Page should be response error: ", w.Body.String())
-	}
+	assertResponseCode(t, w, 404)
+	assertResponseBody(t, w, errorResponse)
 }

helpers_test.go

@@ -10,3 +10,9 @@ func assertResponseCode(t *testing.T, response *httptest.ResponseRecorder, expec
 		t.Fatalf("for HTTP request expected to get %d, got %d instead", expectedCode, response.Code)
 	}
 }
+
+func assertResponseBody(t *testing.T, response *httptest.ResponseRecorder, expectedBody string) {
+	if response.Body.String() != expectedBody {
+		t.Fatalf("for HTTP request expected to receive %q, got %q instead as body", expectedBody, response.Body.String())
+	}
+}

main.go

@@ -26,6 +26,7 @@ import (
 	"time"
 )
 
+// Current version of GitLab Workhorse
 var Version = "(unknown version)" // Set at build time in the Makefile
 
 var printVersion = flag.Bool("version", false, "Print version and exit")
@@ -35,9 +36,9 @@ var listenUmask = flag.Int("listenUmask", 022, "Umask for Unix socket, default:
 var authBackend = flag.String("authBackend", "http://localhost:8080", "Authentication/authorization backend")
 var authSocket = flag.String("authSocket", "", "Optional: Unix domain socket to dial authBackend at")
 var pprofListenAddr = flag.String("pprofListenAddr", "", "pprof listening address, e.g. 'localhost:6060'")
-var relativeUrlRoot = flag.String("relativeUrlRoot", "/", "GitLab relative URL root")
+var relativeURLRoot = flag.String("relativeURLRoot", "/", "GitLab relative URL root")
 var documentRoot = flag.String("documentRoot", "public", "Path to static files content")
-var proxyTimeout = flag.Duration("proxyTimeout", 5*time.Minute, "Proxy request timeout")
+var responseHeadersTimeout = flag.Duration("proxyHeadersTimeout", time.Minute, "How long to wait for response headers when proxying the request")
 
 type httpRoute struct {
 	method     string
@@ -45,36 +46,46 @@ type httpRoute struct {
 	handleFunc serviceHandleFunc
 }
 
+const projectPattern = `^/[^/]+/[^/]+/`
+const gitProjectPattern = `^/[^/]+/[^/]+\.git/`
+
+const apiPattern = `^/api/`
+const projectsAPIPattern = `^/api/v3/projects/[^/]+/`
+
+const ciAPIPattern = `^/ci/api/`
+
 // Routing table
 // We match against URI not containing the relativeUrlRoot:
 // see upstream.ServeHTTP
 var httpRoutes = [...]httpRoute{
 	// Git Clone
-	httpRoute{"GET", regexp.MustCompile(`^/[^/]+/[^/]+\.git/info/refs\z`), repoPreAuthorizeHandler(handleGetInfoRefs)},
-	httpRoute{"POST", regexp.MustCompile(`/[^/]+/[^/]+\.git/git-upload-pack\z`), repoPreAuthorizeHandler(contentEncodingHandler(handlePostRPC))},
-	httpRoute{"POST", regexp.MustCompile(`/[^/]+/[^/]+\.git/git-receive-pack\z`), repoPreAuthorizeHandler(contentEncodingHandler(handlePostRPC))},
-	httpRoute{"GET", regexp.MustCompile(`/[^/]+/[^/]+/repository/archive\z`), repoPreAuthorizeHandler(handleGetArchive)},
-	httpRoute{"GET", regexp.MustCompile(`/[^/]+/[^/]+/repository/archive.zip\z`), repoPreAuthorizeHandler(handleGetArchive)},
-	httpRoute{"GET", regexp.MustCompile(`/[^/]+/[^/]+/repository/archive.tar\z`), repoPreAuthorizeHandler(handleGetArchive)},
-	httpRoute{"GET", regexp.MustCompile(`/[^/]+/[^/]+/repository/archive.tar.gz\z`), repoPreAuthorizeHandler(handleGetArchive)},
-	httpRoute{"GET", regexp.MustCompile(`/[^/]+/[^/]+/repository/archive.tar.bz2\z`), repoPreAuthorizeHandler(handleGetArchive)},
-	httpRoute{"GET", regexp.MustCompile(`/api/v3/projects/[^/]+/repository/archive\z`), repoPreAuthorizeHandler(handleGetArchive)},
-	httpRoute{"GET", regexp.MustCompile(`/api/v3/projects/[^/]+/repository/archive.zip\z`), repoPreAuthorizeHandler(handleGetArchive)},
-	httpRoute{"GET", regexp.MustCompile(`/api/v3/projects/[^/]+/repository/archive.tar\z`), repoPreAuthorizeHandler(handleGetArchive)},
-	httpRoute{"GET", regexp.MustCompile(`/api/v3/projects/[^/]+/repository/archive.tar.gz\z`), repoPreAuthorizeHandler(handleGetArchive)},
-	httpRoute{"GET", regexp.MustCompile(`/api/v3/projects/[^/]+/repository/archive.tar.bz2\z`), repoPreAuthorizeHandler(handleGetArchive)},
-
-	// Git LFS
-	httpRoute{"PUT", regexp.MustCompile(`/[^/]+/[^/]+\.git/gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`), lfsAuthorizeHandler(handleStoreLfsObject)},
-
-	// CI artifacts
-	httpRoute{"POST", regexp.MustCompile(`^/ci/api/v1/builds/[0-9]+/artifacts\z`), artifactsAuthorizeHandler(contentEncodingHandler(handleFileUploads))},
-
-	// Explicitly proxy API
-	httpRoute{"", regexp.MustCompile(`^/api/`), proxyRequest},
-	httpRoute{"", regexp.MustCompile(`^/ci/api/`), proxyRequest},
-
-	// Serve static files and forward otherwise
+	httpRoute{"GET", regexp.MustCompile(gitProjectPattern + `info/refs\z`), repoPreAuthorizeHandler(handleGetInfoRefs)},
+	httpRoute{"POST", regexp.MustCompile(gitProjectPattern + `git-upload-pack\z`), repoPreAuthorizeHandler(contentEncodingHandler(handlePostRPC))},
+	httpRoute{"POST", regexp.MustCompile(gitProjectPattern + `git-receive-pack\z`), repoPreAuthorizeHandler(contentEncodingHandler(handlePostRPC))},
+	httpRoute{"PUT", regexp.MustCompile(gitProjectPattern + `gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`), lfsAuthorizeHandler(handleStoreLfsObject)},
+
+	// Repository Archive
+	httpRoute{"GET", regexp.MustCompile(projectPattern + `repository/archive\z`), repoPreAuthorizeHandler(handleGetArchive)},
+	httpRoute{"GET", regexp.MustCompile(projectPattern + `repository/archive.zip\z`), repoPreAuthorizeHandler(handleGetArchive)},
+	httpRoute{"GET", regexp.MustCompile(projectPattern + `repository/archive.tar\z`), repoPreAuthorizeHandler(handleGetArchive)},
+	httpRoute{"GET", regexp.MustCompile(projectPattern + `repository/archive.tar.gz\z`), repoPreAuthorizeHandler(handleGetArchive)},
+	httpRoute{"GET", regexp.MustCompile(projectPattern + `repository/archive.tar.bz2\z`), repoPreAuthorizeHandler(handleGetArchive)},
+
+	// Repository Archive API
+	httpRoute{"GET", regexp.MustCompile(projectsAPIPattern + `repository/archive\z`), repoPreAuthorizeHandler(handleGetArchive)},
+	httpRoute{"GET", regexp.MustCompile(projectsAPIPattern + `repository/archive.zip\z`), repoPreAuthorizeHandler(handleGetArchive)},
+	httpRoute{"GET", regexp.MustCompile(projectsAPIPattern + `repository/archive.tar\z`), repoPreAuthorizeHandler(handleGetArchive)},
+	httpRoute{"GET", regexp.MustCompile(projectsAPIPattern + `repository/archive.tar.gz\z`), repoPreAuthorizeHandler(handleGetArchive)},
+	httpRoute{"GET", regexp.MustCompile(projectsAPIPattern + `repository/archive.tar.bz2\z`), repoPreAuthorizeHandler(handleGetArchive)},
+
+	// CI Artifacts API
+	httpRoute{"POST", regexp.MustCompile(ciAPIPattern + `v1/builds/[0-9]+/artifacts\z`), artifactsAuthorizeHandler(contentEncodingHandler(handleFileUploads))},
+
+	// Explicitly proxy API requests
+	httpRoute{"", regexp.MustCompile(apiPattern), proxyRequest},
+	httpRoute{"", regexp.MustCompile(ciAPIPattern), proxyRequest},
+
+	// Serve static files or forward the requests
 	httpRoute{"", nil, handleServeFile(documentRoot,
 		handleDeployPage(documentRoot,
 			handleRailsError(documentRoot,
@@ -125,6 +136,7 @@ func main() {
 			Dial: func(_, _ string) (net.Conn, error) {
 				return dialer.Dial("unix", *authSocket)
 			},
+			ResponseHeaderTimeout: *responseHeadersTimeout,
 		}
 	}
 	proxyTransport := &proxyRoundTripper{transport: authTransport}
@@ -140,12 +152,11 @@ func main() {
 	}
 
 	upstream := newUpstream(*authBackend, proxyTransport)
-	upstream.SetRelativeUrlRoot(*relativeUrlRoot)
-	upstream.SetProxyTimeout(*proxyTimeout)
+	upstream.SetRelativeURLRoot(*relativeURLRoot)
 
 	// Because net/http/pprof installs itself in the DefaultServeMux
 	// we create a fresh one for the Git server.
 	serveMux := http.NewServeMux()
-	serveMux.Handle(upstream.relativeUrlRoot, upstream)
+	serveMux.Handle(upstream.relativeURLRoot, upstream)
 	log.Fatal(http.Serve(listener, serveMux))
 }

proxy.go

@@ -16,8 +16,8 @@ func (p *proxyRoundTripper) RoundTrip(r *http.Request) (res *http.Response, err
 	// Map error to 502 response
 	if err != nil {
 		res = &http.Response{
-			StatusCode: 502,
-			Status:     err.Error(),
+			StatusCode: http.StatusBadGateway,
+			Status:     http.StatusText(http.StatusBadGateway),
 
 			Request:    r,
 			ProtoMajor: r.ProtoMajor,
@@ -25,8 +25,9 @@ func (p *proxyRoundTripper) RoundTrip(r *http.Request) (res *http.Response, err
 			Proto:      r.Proto,
 			Header:     make(http.Header),
 			Trailer:    make(http.Header),
-			Body:       ioutil.NopCloser(&bytes.Buffer{}),
+			Body:       ioutil.NopCloser(bytes.NewBufferString(err.Error())),
 		}
+		res.Header.Set("Content-Type", "text/plain")
 		err = nil
 	}
 	return

proxy_test.go

@@ -4,10 +4,12 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"regexp"
 	"testing"
+	"time"
 )
 
 func TestProxyRequest(t *testing.T) {
@@ -42,15 +44,94 @@ func TestProxyRequest(t *testing.T) {
 		u:       newUpstream(ts.URL, nil),
 	}
 
-	response := httptest.NewRecorder()
-	proxyRequest(response, &request)
-	assertResponseCode(t, response, 202)
+	w := httptest.NewRecorder()
+	proxyRequest(w, &request)
+	assertResponseCode(t, w, 202)
+	assertResponseBody(t, w, "RESPONSE")
 
-	if response.Body.String() != "RESPONSE" {
-		t.Fatal("Expected RESPONSE in response body:", response.Body.String())
+	if w.Header().Get("Custom-Response-Header") != "test" {
+		t.Fatal("Expected custom response header")
 	}
+}
 
-	if response.Header().Get("Custom-Response-Header") != "test" {
-		t.Fatal("Expected custom response header")
+func TestProxyError(t *testing.T) {
+	httpRequest, err := http.NewRequest("POST", "/url/path", bytes.NewBufferString("REQUEST"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	httpRequest.Header.Set("Custom-Header", "test")
+
+	transport := proxyRoundTripper{
+		transport: http.DefaultTransport,
+	}
+
+	request := gitRequest{
+		Request: httpRequest,
+		u:       newUpstream("http://localhost:655575/", &transport),
+	}
+
+	w := httptest.NewRecorder()
+	proxyRequest(w, &request)
+	assertResponseCode(t, w, 502)
+	assertResponseBody(t, w, "dial tcp: invalid port 655575")
+}
+
+func TestProxyReadTimeout(t *testing.T) {
+	ts := testServerWithHandler(nil, func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(time.Minute)
+	})
+
+	httpRequest, err := http.NewRequest("POST", "http://localhost/url/path", nil)
+	if err != nil {
+		t.Fatal(err)
 	}
+
+	transport := &proxyRoundTripper{
+		transport: &http.Transport{
+			Proxy: http.ProxyFromEnvironment,
+			Dial: (&net.Dialer{
+				Timeout:   30 * time.Second,
+				KeepAlive: 30 * time.Second,
+			}).Dial,
+			TLSHandshakeTimeout:   10 * time.Second,
+			ResponseHeaderTimeout: time.Millisecond,
+		},
+	}
+
+	request := gitRequest{
+		Request: httpRequest,
+		u:       newUpstream(ts.URL, transport),
+	}
+
+	w := httptest.NewRecorder()
+	proxyRequest(w, &request)
+	assertResponseCode(t, w, 502)
+	assertResponseBody(t, w, "net/http: timeout awaiting response headers")
+}
+
+func TestProxyHandlerTimeout(t *testing.T) {
+	ts := testServerWithHandler(nil,
+		http.TimeoutHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			time.Sleep(time.Second)
+		}), time.Millisecond, "Request took too long").ServeHTTP,
+	)
+
+	httpRequest, err := http.NewRequest("POST", "http://localhost/url/path", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	transport := &proxyRoundTripper{
+		transport: http.DefaultTransport,
+	}
+
+	request := gitRequest{
+		Request: httpRequest,
+		u:       newUpstream(ts.URL, transport),
+	}
+
+	w := httptest.NewRecorder()
+	proxyRequest(w, &request)
+	assertResponseCode(t, w, 503)
+	assertResponseBody(t, w, "Request took too long")
 }

servefile.go

@@ -10,7 +10,7 @@ import (
 
 func handleServeFile(documentRoot *string, notFoundHandler serviceHandleFunc) serviceHandleFunc {
 	return func(w http.ResponseWriter, r *gitRequest) {
-		file := filepath.Join(*documentRoot, r.relativeUriPath)
+		file := filepath.Join(*documentRoot, r.relativeURIPath)
 
 		// The filepath.Join does Clean traversing directories up
 		if !strings.HasPrefix(file, *documentRoot) {

servefile_test.go

@@ -12,14 +12,12 @@ import (
 func TestServingNonExistingFile(t *testing.T) {
 	dir := "/path/to/non/existing/directory"
 	request := &gitRequest{
-		relativeUriPath: "/static/file",
+		relativeURIPath: "/static/file",
 	}
 
 	w := httptest.NewRecorder()
 	handleServeFile(&dir, nil)(w, request)
-	if w.Code != 404 {
-		t.Fatal("Expected to receive 404, since no default handler is provided")
-	}
+	assertResponseCode(t, w, 404)
 }
 
 func TestServingDirectory(t *testing.T) {
@@ -30,33 +28,29 @@ func TestServingDirectory(t *testing.T) {
 	defer os.RemoveAll(dir)
 
 	request := &gitRequest{
-		relativeUriPath: "/",
+		relativeURIPath: "/",
 	}
 
 	w := httptest.NewRecorder()
 	handleServeFile(&dir, nil)(w, request)
-	if w.Code != 404 {
-		t.Fatal("Expected to receive 404, since we will serve the directory")
-	}
+	assertResponseCode(t, w, 404)
 }
 
 func TestServingMalformedUri(t *testing.T) {
 	dir := "/path/to/non/existing/directory"
 	request := &gitRequest{
-		relativeUriPath: "/../../../static/file",
+		relativeURIPath: "/../../../static/file",
 	}
 
 	w := httptest.NewRecorder()
 	handleServeFile(&dir, nil)(w, request)
-	if w.Code != 500 {
-		t.Fatal("Expected to receive 500, since client provided invalid URI")
-	}
+	assertResponseCode(t, w, 500)
 }
 
 func TestExecutingHandlerWhenNoFileFound(t *testing.T) {
 	dir := "/path/to/non/existing/directory"
 	request := &gitRequest{
-		relativeUriPath: "/static/file",
+		relativeURIPath: "/static/file",
 	}
 
 	executed := false
@@ -78,17 +72,15 @@ func TestServingTheActualFile(t *testing.T) {
 	httpRequest, _ := http.NewRequest("GET", "/file", nil)
 	request := &gitRequest{
 		Request:         httpRequest,
-		relativeUriPath: "/file",
+		relativeURIPath: "/file",
 	}
 
-	fileContent := "DEPLOY"
+	fileContent := "STATIC"
 	ioutil.WriteFile(filepath.Join(dir, "file"), []byte(fileContent), 0600)
 
 	w := httptest.NewRecorder()
 	handleServeFile(&dir, nil)(w, request)
-	if w.Code != 200 {
-		t.Fatal("Expected to receive 200, since we serve existing file")
-	}
+	assertResponseCode(t, w, 200)
 	if w.Body.String() != fileContent {
 		t.Error("We should serve the file: ", w.Body.String())
 	}

upstream.go

@@ -12,7 +12,6 @@ import (
 	"net/http/httputil"
 	"net/url"
 	"strings"
-	"time"
 )
 
 type serviceHandleFunc func(w http.ResponseWriter, r *gitRequest)
@@ -21,7 +20,7 @@ type upstream struct {
 	httpClient      *http.Client
 	httpProxy       *httputil.ReverseProxy
 	authBackend     string
-	relativeUrlRoot string
+	relativeURLRoot string
 }
 
 type authorizationResponse struct {
@@ -60,7 +59,7 @@ type gitRequest struct {
 	u *upstream
 
 	// This field contains the URL.Path stripped from RelativeUrlRoot
-	relativeUriPath string
+	relativeURIPath string
 }
 
 func newUpstream(authBackend string, authTransport http.RoundTripper) *upstream {
@@ -73,24 +72,20 @@ func newUpstream(authBackend string, authTransport http.RoundTripper) *upstream
 		authBackend:     authBackend,
 		httpClient:      &http.Client{Transport: authTransport},
 		httpProxy:       httputil.NewSingleHostReverseProxy(u),
-		relativeUrlRoot: "/",
+		relativeURLRoot: "/",
 	}
 	up.httpProxy.Transport = authTransport
 	return up
 }
 
-func (u *upstream) SetRelativeUrlRoot(relativeUrlRoot string) {
-	u.relativeUrlRoot = relativeUrlRoot
+func (u *upstream) SetRelativeURLRoot(relativeURLRoot string) {
+	u.relativeURLRoot = relativeURLRoot
 
-	if !strings.HasSuffix(u.relativeUrlRoot, "/") {
-		u.relativeUrlRoot += "/"
+	if !strings.HasSuffix(u.relativeURLRoot, "/") {
+		u.relativeURLRoot += "/"
 	}
 }
 
-func (u *upstream) SetProxyTimeout(timeout time.Duration) {
-	u.httpClient.Timeout = timeout
-}
-
 func (u *upstream) ServeHTTP(ow http.ResponseWriter, r *http.Request) {
 	var g httpRoute
 
@@ -100,7 +95,7 @@ func (u *upstream) ServeHTTP(ow http.ResponseWriter, r *http.Request) {
 	// Strip prefix and add "/"
 	// To match against non-relative URL
 	// Making it simpler for our matcher
-	relativeUriPath := "/" + strings.TrimPrefix(r.URL.Path, u.relativeUrlRoot)
+	relativeURIPath := "/" + strings.TrimPrefix(r.URL.Path, u.relativeURLRoot)
 
 	// Look for a matching Git service
 	foundService := false
@@ -109,7 +104,7 @@ func (u *upstream) ServeHTTP(ow http.ResponseWriter, r *http.Request) {
 			continue
 		}
 
-		if g.regex == nil || g.regex.MatchString(relativeUriPath) {
+		if g.regex == nil || g.regex.MatchString(relativeURIPath) {
 			foundService = true
 			break
 		}
@@ -123,7 +118,7 @@ func (u *upstream) ServeHTTP(ow http.ResponseWriter, r *http.Request) {
 
 	request := gitRequest{
 		Request:         r,
-		relativeUriPath: relativeUriPath,
+		relativeURIPath: relativeURIPath,
 		u:               u,
 	}