Merge branch 'git-fail500' into 'master'

Don't append error messages to Git response body

Closes #127

See merge request !157

gitaly_test.go

+package main
+
+import (
+	"fmt"
+	"math/rand"
+	"net"
+	"os"
+	"os/exec"
+	"path"
+	"strings"
+	"testing"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
+
+	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
+)
+
+func TestFailedCloneNoGitaly(t *testing.T) {
+	// Prepare clone directory
+	require.NoError(t, os.RemoveAll(scratchDir))
+
+	authBody := &api.Response{
+		GL_ID:    "user-123",
+		RepoPath: repoPath(t),
+		// This will create a failure to connect to Gitaly
+		GitalyAddress: "unix:/nonexistent",
+	}
+
+	// Prepare test server and backend
+	ts := testAuthServer(nil, 200, authBody)
+	defer ts.Close()
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	// Do the git clone
+	cloneCmd := exec.Command("git", "clone", fmt.Sprintf("%s/%s", ws.URL, testRepo), checkoutDir)
+	out, err := cloneCmd.CombinedOutput()
+	t.Log(string(out))
+	assert.Error(t, err, "git clone should have failed")
+}
+
+func TestGetInfoRefsProxiedToGitalySuccessfully(t *testing.T) {
+	apiResponse := gitOkBody(t)
+	gitalyServer, socketPath := startGitalyServer(t, codes.OK)
+	defer gitalyServer.Stop()
+
+	gitalyAddress := "unix://" + socketPath
+	apiResponse.GitalyAddress = gitalyAddress
+
+	ts := testAuthServer(nil, 200, apiResponse)
+	defer ts.Close()
+
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	resource := "/gitlab-org/gitlab-test.git/info/refs?service=git-upload-pack"
+	_, body := httpGet(t, ws.URL+resource)
+
+	expectedContent := string(testhelper.GitalyInfoRefsResponseMock)
+	assert.Equal(t, expectedContent, body, "GET %q: response body", resource)
+}
+
+func TestPostReceivePackProxiedToGitalySuccessfully(t *testing.T) {
+	apiResponse := gitOkBody(t)
+
+	gitalyServer, socketPath := startGitalyServer(t, codes.OK)
+	defer gitalyServer.Stop()
+
+	apiResponse.GitalyAddress = "unix://" + socketPath
+	ts := testAuthServer(nil, 200, apiResponse)
+	defer ts.Close()
+
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	resource := "/gitlab-org/gitlab-test.git/git-receive-pack"
+	resp, body := httpPost(
+		t,
+		ws.URL+resource,
+		"application/x-git-receive-pack-request",
+		testhelper.GitalyReceivePackResponseMock,
+	)
+
+	expectedBody := strings.Join([]string{
+		apiResponse.RepoPath,
+		apiResponse.Repository.StorageName,
+		apiResponse.Repository.RelativePath,
+		apiResponse.GL_ID,
+		string(testhelper.GitalyReceivePackResponseMock),
+	}, "\000")
+
+	assert.Equal(t, 200, resp.StatusCode, "POST %q", resource)
+	assert.Equal(t, expectedBody, body, "POST %q: response body", resource)
+	testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-receive-pack-result")
+}
+
+func TestPostUploadPackProxiedToGitalySuccessfully(t *testing.T) {
+	apiResponse := gitOkBody(t)
+
+	for _, code := range []codes.Code{codes.OK, codes.Unavailable} {
+		func() {
+			gitalyServer, socketPath := startGitalyServer(t, code)
+			defer gitalyServer.Stop()
+
+			apiResponse.GitalyAddress = "unix://" + socketPath
+			ts := testAuthServer(nil, 200, apiResponse)
+			defer ts.Close()
+
+			ws := startWorkhorseServer(ts.URL)
+			defer ws.Close()
+
+			resource := "/gitlab-org/gitlab-test.git/git-upload-pack"
+			resp, body := httpPost(
+				t,
+				ws.URL+resource,
+				"application/x-git-upload-pack-request",
+				testhelper.GitalyUploadPackResponseMock,
+			)
+
+			expectedBody := strings.Join([]string{
+				apiResponse.RepoPath,
+				apiResponse.Repository.StorageName,
+				apiResponse.Repository.RelativePath,
+				string(testhelper.GitalyUploadPackResponseMock),
+			}, "\000")
+
+			assert.Equal(t, 200, resp.StatusCode, "POST %q", resource)
+			assert.Equal(t, expectedBody, body, "POST %q: response body", resource)
+			testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-upload-pack-result")
+		}()
+	}
+}
+
+func TestGetInfoRefsHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
+	gitalyServer, _ := startGitalyServer(t, codes.OK)
+	defer gitalyServer.Stop()
+
+	apiResponse := gitOkBody(t)
+	apiResponse.GitalyAddress = ""
+	ts := testAuthServer(nil, 200, apiResponse)
+	defer ts.Close()
+
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	resource := "/gitlab-org/gitlab-test.git/info/refs?service=git-upload-pack"
+	resp, body := httpGet(t, ws.URL+resource)
+
+	assert.Equal(t, 200, resp.StatusCode, "GET %q", resource)
+	assert.NotContains(t, string(testhelper.GitalyInfoRefsResponseMock), body, "GET %q: should not have been proxied to Gitaly", resource)
+	testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-upload-pack-advertisement")
+}
+
+func TestPostReceivePackHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
+	gitalyServer, _ := startGitalyServer(t, codes.OK)
+	defer gitalyServer.Stop()
+
+	apiResponse := gitOkBody(t)
+	apiResponse.GitalyAddress = ""
+	ts := testAuthServer(nil, 200, apiResponse)
+	defer ts.Close()
+
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	resource := "/gitlab-org/gitlab-test.git/git-receive-pack"
+	payload := []byte("This payload should not reach Gitaly")
+	resp, body := httpPost(t, ws.URL+resource, "application/x-git-receive-pack-request", payload)
+
+	assert.Equal(t, 200, resp.StatusCode, "POST %q: status code", resource)
+	assert.NotContains(t, payload, body, "POST %q: request should not have been proxied to Gitaly", resource)
+	testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-receive-pack-result")
+}
+
+func TestPostUploadPackHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
+	gitalyServer, _ := startGitalyServer(t, codes.OK)
+	defer gitalyServer.Stop()
+
+	apiResponse := gitOkBody(t)
+	apiResponse.GitalyAddress = ""
+	ts := testAuthServer(nil, 200, apiResponse)
+	defer ts.Close()
+
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	resource := "/gitlab-org/gitlab-test.git/git-upload-pack"
+	payload := []byte("This payload should not reach Gitaly")
+	resp, body := httpPost(t, ws.URL+resource, "application/x-git-upload-pack-request", payload)
+
+	assert.Equal(t, 200, resp.StatusCode, "POST %q: status code", resource)
+	assert.NotContains(t, payload, body, "POST %q: request should not have been proxied to Gitaly", resource)
+	testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-upload-pack-result")
+}
+
+func startGitalyServer(t *testing.T, finalMessageCode codes.Code) (*grpc.Server, string) {
+	socketPath := path.Join(scratchDir, fmt.Sprintf("gitaly-%d.sock", rand.Int()))
+	server := grpc.NewServer()
+	listener, err := net.Listen("unix", socketPath)
+	require.NoError(t, err)
+
+	pb.RegisterSmartHTTPServer(server, testhelper.NewGitalyServer(finalMessageCode))
+
+	go server.Serve(listener)
+
+	return server, socketPath
+}

internal/git/git-http.go

@@ -39,7 +39,11 @@ func postRPCHandler(a *api.API, name string, handler func(*GitHttpResponseWriter
 		}()
 
 		if err := handler(w, r, ar); err != nil {
-			helper.Fail500(w, r, fmt.Errorf("%s: %v", name, err))
+			// If the handler already wrote a response this WriteHeader call is a
+			// no-op. It never reaches net/http because GitHttpResponseWriter calls
+			// WriteHeader on its underlying ResponseWriter at most once.
+			w.WriteHeader(500)
+			helper.LogError(r, fmt.Errorf("%s: %v", name, err))
 		}
 	})
 }

internal/testhelper/gitaly.go

@@ -9,9 +9,14 @@ import (
 	"strings"
 
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
 )
 
-type GitalyTestServer struct{}
+type GitalyTestServer struct {
+	finalMessageCode codes.Code
+}
 
 const GitalyInfoRefsResponseMock = "Mock Gitaly InfoRefsResponse data"
 
@@ -28,8 +33,8 @@ func init() {
 	}
 }
 
-func NewGitalyServer() *GitalyTestServer {
-	return &GitalyTestServer{}
+func NewGitalyServer(finalMessageCode codes.Code) *GitalyTestServer {
+	return &GitalyTestServer{finalMessageCode: finalMessageCode}
 }
 
 func (s *GitalyTestServer) InfoRefsUploadPack(in *pb.InfoRefsRequest, stream pb.SmartHTTP_InfoRefsUploadPackServer) error {
@@ -40,7 +45,11 @@ func (s *GitalyTestServer) InfoRefsUploadPack(in *pb.InfoRefsRequest, stream pb.
 	response := &pb.InfoRefsResponse{
 		Data: []byte(GitalyInfoRefsResponseMock),
 	}
-	return stream.Send(response)
+	if err := stream.Send(response); err != nil {
+		return err
+	}
+
+	return s.finalError()
 }
 
 func (s *GitalyTestServer) InfoRefsReceivePack(in *pb.InfoRefsRequest, stream pb.SmartHTTP_InfoRefsReceivePackServer) error {
@@ -51,7 +60,11 @@ func (s *GitalyTestServer) InfoRefsReceivePack(in *pb.InfoRefsRequest, stream pb
 	response := &pb.InfoRefsResponse{
 		Data: []byte(GitalyInfoRefsResponseMock),
 	}
-	return stream.Send(response)
+	if err := stream.Send(response); err != nil {
+		return err
+	}
+
+	return s.finalError()
 }
 
 func (s *GitalyTestServer) PostReceivePack(stream pb.SmartHTTP_PostReceivePackServer) error {
@@ -94,7 +107,7 @@ func (s *GitalyTestServer) PostReceivePack(stream pb.SmartHTTP_PostReceivePackSe
 		}
 	}
 
-	return nil
+	return s.finalError()
 }
 
 func (s *GitalyTestServer) PostUploadPack(stream pb.SmartHTTP_PostUploadPackServer) error {
@@ -136,6 +149,14 @@ func (s *GitalyTestServer) PostUploadPack(stream pb.SmartHTTP_PostUploadPackServ
 		}
 	}
 
+	return s.finalError()
+}
+
+func (s *GitalyTestServer) finalError() error {
+	if code := s.finalMessageCode; code != codes.OK {
+		return grpc.Errorf(code, "error as specified by test")
+	}
+
 	return nil
 }
 

main_test.go

@@ -7,8 +7,6 @@ import (
 	"fmt"
 	"io/ioutil"
 	"log"
-	"math/rand"
-	"net"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -16,7 +14,6 @@ import (
 	"path"
 	"regexp"
 	"strconv"
-	"strings"
 	"testing"
 	"time"
 
@@ -31,7 +28,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"google.golang.org/grpc"
 )
 
 const scratchDir = "testdata/scratch"
@@ -128,30 +124,6 @@ func TestDeniedClone(t *testing.T) {
 	assert.Error(t, err, "git clone should have failed")
 }
 
-func TestFailedCloneNoGitaly(t *testing.T) {
-	// Prepare clone directory
-	require.NoError(t, os.RemoveAll(scratchDir))
-
-	authBody := &api.Response{
-		GL_ID:    "user-123",
-		RepoPath: repoPath(t),
-		// This will create a failure to connect to Gitaly
-		GitalyAddress: "unix:/nonexistent",
-	}
-
-	// Prepare test server and backend
-	ts := testAuthServer(nil, 200, authBody)
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	// Do the git clone
-	cloneCmd := exec.Command("git", "clone", fmt.Sprintf("%s/%s", ws.URL, testRepo), checkoutDir)
-	out, err := cloneCmd.CombinedOutput()
-	t.Log(string(out))
-	assert.Error(t, err, "git clone should have failed")
-}
-
 func TestAllowedPush(t *testing.T) {
 	preparePushRepo(t)
 
@@ -448,156 +420,6 @@ func TestApiContentTypeBlock(t *testing.T) {
 	assert.NotContains(t, wrongResponse, body, "GET %q: response body", resourcePath)
 }
 
-func TestGetInfoRefsProxiedToGitalySuccessfully(t *testing.T) {
-	apiResponse := gitOkBody(t)
-	gitalyServer, socketPath := startGitalyServer(t)
-	defer gitalyServer.Stop()
-
-	gitalyAddress := "unix://" + socketPath
-	apiResponse.GitalyAddress = gitalyAddress
-
-	ts := testAuthServer(nil, 200, apiResponse)
-	defer ts.Close()
-
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	resource := "/gitlab-org/gitlab-test.git/info/refs?service=git-upload-pack"
-	_, body := httpGet(t, ws.URL+resource)
-
-	expectedContent := string(testhelper.GitalyInfoRefsResponseMock)
-	assert.Equal(t, expectedContent, body, "GET %q: response body")
-}
-
-func TestPostReceivePackProxiedToGitalySuccessfully(t *testing.T) {
-	apiResponse := gitOkBody(t)
-
-	gitalyServer, socketPath := startGitalyServer(t)
-	defer gitalyServer.Stop()
-
-	apiResponse.GitalyAddress = "unix://" + socketPath
-	ts := testAuthServer(nil, 200, apiResponse)
-	defer ts.Close()
-
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	resource := "/gitlab-org/gitlab-test.git/git-receive-pack"
-	resp, body := httpPost(
-		t,
-		ws.URL+resource,
-		"application/x-git-receive-pack-request",
-		testhelper.GitalyReceivePackResponseMock,
-	)
-
-	expectedBody := strings.Join([]string{
-		apiResponse.RepoPath,
-		apiResponse.Repository.StorageName,
-		apiResponse.Repository.RelativePath,
-		apiResponse.GL_ID,
-		string(testhelper.GitalyReceivePackResponseMock),
-	}, "\000")
-
-	assert.Equal(t, 200, resp.StatusCode, "POST %q", resource)
-	assert.Equal(t, expectedBody, body, "POST %q: response body", resource)
-	testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-receive-pack-result")
-}
-
-func TestPostUploadPackProxiedToGitalySuccessfully(t *testing.T) {
-	apiResponse := gitOkBody(t)
-
-	gitalyServer, socketPath := startGitalyServer(t)
-	defer gitalyServer.Stop()
-
-	apiResponse.GitalyAddress = "unix://" + socketPath
-	ts := testAuthServer(nil, 200, apiResponse)
-	defer ts.Close()
-
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	resource := "/gitlab-org/gitlab-test.git/git-upload-pack"
-	resp, body := httpPost(
-		t,
-		ws.URL+resource,
-		"application/x-git-upload-pack-request",
-		testhelper.GitalyUploadPackResponseMock,
-	)
-
-	expectedBody := strings.Join([]string{
-		apiResponse.RepoPath,
-		apiResponse.Repository.StorageName,
-		apiResponse.Repository.RelativePath,
-		string(testhelper.GitalyUploadPackResponseMock),
-	}, "\000")
-
-	assert.Equal(t, 200, resp.StatusCode, "POST %q", resource)
-	assert.Equal(t, expectedBody, body, "POST %q: response body", resource)
-	testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-upload-pack-result")
-}
-
-func TestGetInfoRefsHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
-	gitalyServer, _ := startGitalyServer(t)
-	defer gitalyServer.Stop()
-
-	apiResponse := gitOkBody(t)
-	apiResponse.GitalyAddress = ""
-	ts := testAuthServer(nil, 200, apiResponse)
-	defer ts.Close()
-
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	resource := "/gitlab-org/gitlab-test.git/info/refs?service=git-upload-pack"
-	resp, body := httpGet(t, ws.URL+resource)
-
-	assert.Equal(t, 200, resp.StatusCode, "GET %q", resource)
-	assert.NotContains(t, string(testhelper.GitalyInfoRefsResponseMock), body, "GET %q: should not have been proxied to Gitaly", resource)
-	testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-upload-pack-advertisement")
-}
-
-func TestPostReceivePackHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
-	gitalyServer, _ := startGitalyServer(t)
-	defer gitalyServer.Stop()
-
-	apiResponse := gitOkBody(t)
-	apiResponse.GitalyAddress = ""
-	ts := testAuthServer(nil, 200, apiResponse)
-	defer ts.Close()
-
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	resource := "/gitlab-org/gitlab-test.git/git-receive-pack"
-	payload := []byte("This payload should not reach Gitaly")
-	resp, body := httpPost(t, ws.URL+resource, "application/x-git-receive-pack-request", payload)
-
-	assert.Equal(t, 200, resp.StatusCode, "POST %q: status code", resource)
-	assert.NotContains(t, payload, body, "POST %q: request should not have been proxied to Gitaly", resource)
-	testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-receive-pack-result")
-}
-
-func TestPostUploadPackHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
-	gitalyServer, _ := startGitalyServer(t)
-	defer gitalyServer.Stop()
-
-	apiResponse := gitOkBody(t)
-	apiResponse.GitalyAddress = ""
-	ts := testAuthServer(nil, 200, apiResponse)
-	defer ts.Close()
-
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	resource := "/gitlab-org/gitlab-test.git/git-upload-pack"
-	payload := []byte("This payload should not reach Gitaly")
-	resp, body := httpPost(t, ws.URL+resource, "application/x-git-upload-pack-request", payload)
-
-	assert.Equal(t, 200, resp.StatusCode, "POST %q: status code", resource)
-	assert.NotContains(t, payload, body, "POST %q: request should not have been proxied to Gitaly", resource)
-	testhelper.AssertResponseHeader(t, resp, "Content-Type", "application/x-git-upload-pack-result")
-}
-
 func TestAPIFalsePositivesAreProxied(t *testing.T) {
 	goodResponse := []byte(`<html></html>`)
 	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
@@ -744,19 +566,6 @@ func startWorkhorseServerWithConfig(cfg *config.Config) *httptest.Server {
 	return httptest.NewServer(u)
 }
 
-func startGitalyServer(t *testing.T) (*grpc.Server, string) {
-	socketPath := path.Join(scratchDir, fmt.Sprintf("gitaly-%d.sock", rand.Int()))
-	server := grpc.NewServer()
-	listener, err := net.Listen("unix", socketPath)
-	require.NoError(t, err)
-
-	pb.RegisterSmartHTTPServer(server, testhelper.NewGitalyServer())
-
-	go server.Serve(listener)
-
-	return server, socketPath
-}
-
 func runOrFail(t *testing.T, cmd *exec.Cmd) {
 	out, err := cmd.CombinedOutput()
 	t.Logf("%s", out)