Merge branch '141-add-gitaly-container-for-ci' into 'master'

Resolve "Rework test suite to allow dead code to be removed"

Closes #141

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

.gitlab-ci.yml

@@ -6,6 +6,13 @@ verify:
   - make verify
 
 .test_template: &test_definition
+  services:
+    - name: registry.gitlab.com/gitlab-org/build/cng/gitaly:latest
+      # Disable the hooks so we don't have to stub the GitLab API
+      command: ["bash", "-c", "mkdir /home/git/repositories && rm -rf /srv/gitlab-shell/hooks/* && exec /scripts/process-wrapper"]
+      alias: gitaly
+  variables:
+    GITALY_ADDRESS: "tcp://gitaly:8075"
   script:
   - apt update -qq && apt install -y unzip bzip2
   - go version

gitaly_integration_test.go

+// Tests in this file need access to a real Gitaly server to run. The address
+// is supplied via the GITALY_ADDRESS environment variable
+package main
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"os/exec"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/gitaly"
+)
+
+var (
+	gitalyAddress string
+)
+
+func init() {
+	gitalyAddress = os.Getenv("GITALY_ADDRESS")
+}
+
+func skipUnlessRealGitaly(t *testing.T) {
+	t.Log(gitalyAddress)
+	if gitalyAddress != "" {
+		return
+	}
+
+	t.Skip(`Please set GITALY_ADDRESS="..." to run Gitaly integration tests`)
+}
+
+func realGitalyAuthResponse(apiResponse *api.Response) *api.Response {
+	apiResponse.GitalyServer.Address = gitalyAddress
+
+	return apiResponse
+}
+
+func realGitalyOkBody(t *testing.T) *api.Response {
+	return realGitalyAuthResponse(gitOkBody(t))
+}
+
+func ensureGitalyRepository(t *testing.T, apiResponse *api.Response) error {
+	namespace, err := gitaly.NewNamespaceClient(apiResponse.GitalyServer)
+	if err != nil {
+		return err
+	}
+	repository, err := gitaly.NewRepositoryClient(apiResponse.GitalyServer)
+	if err != nil {
+		return err
+	}
+
+	// Remove the repository if it already exists, for consistency
+	rmNsReq := &pb.RemoveNamespaceRequest{
+		StorageName: apiResponse.Repository.StorageName,
+		Name:        apiResponse.Repository.RelativePath,
+	}
+	_, err = namespace.RemoveNamespace(context.Background(), rmNsReq)
+	if err != nil {
+		return err
+	}
+
+	createReq := &pb.CreateRepositoryFromURLRequest{
+		Repository: &apiResponse.Repository,
+		Url:        "https://gitlab.com/gitlab-org/gitlab-test.git",
+	}
+
+	_, err = repository.CreateRepositoryFromURL(context.Background(), createReq)
+	return err
+}
+
+func TestAllowedClone(t *testing.T) {
+	skipUnlessRealGitaly(t)
+
+	// Create the repository in the Gitaly server
+	apiResponse := realGitalyOkBody(t)
+	require.NoError(t, ensureGitalyRepository(t, apiResponse))
+
+	// Prepare test server and backend
+	ts := testAuthServer(nil, 200, apiResponse)
+	defer ts.Close()
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	// Do the git clone
+	require.NoError(t, os.RemoveAll(scratchDir))
+	cloneCmd := exec.Command("git", "clone", fmt.Sprintf("%s/%s", ws.URL, testRepo), checkoutDir)
+	runOrFail(t, cloneCmd)
+
+	// We may have cloned an 'empty' repository, 'git log' will fail in it
+	logCmd := exec.Command("git", "log", "-1", "--oneline")
+	logCmd.Dir = checkoutDir
+	runOrFail(t, logCmd)
+}
+
+func TestAllowedShallowClone(t *testing.T) {
+	skipUnlessRealGitaly(t)
+
+	// Create the repository in the Gitaly server
+	apiResponse := realGitalyOkBody(t)
+	require.NoError(t, ensureGitalyRepository(t, apiResponse))
+
+	// Prepare test server and backend
+	ts := testAuthServer(nil, 200, apiResponse)
+	defer ts.Close()
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	// Shallow git clone (depth 1)
+	require.NoError(t, os.RemoveAll(scratchDir))
+	cloneCmd := exec.Command("git", "clone", "--depth", "1", fmt.Sprintf("%s/%s", ws.URL, testRepo), checkoutDir)
+	runOrFail(t, cloneCmd)
+
+	// We may have cloned an 'empty' repository, 'git log' will fail in it
+	logCmd := exec.Command("git", "log", "-1", "--oneline")
+	logCmd.Dir = checkoutDir
+	runOrFail(t, logCmd)
+}
+
+func TestAllowedPush(t *testing.T) {
+	skipUnlessRealGitaly(t)
+
+	// Create the repository in the Gitaly server
+	apiResponse := realGitalyOkBody(t)
+	require.NoError(t, ensureGitalyRepository(t, apiResponse))
+
+	// Prepare the test server and backend
+	ts := testAuthServer(nil, 200, realGitalyOkBody(t))
+	defer ts.Close()
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	// Perform the git push
+	pushCmd := exec.Command("git", "push", fmt.Sprintf("%s/%s", ws.URL, testRepo), fmt.Sprintf("master:%s", newBranch()))
+	pushCmd.Dir = checkoutDir
+	runOrFail(t, pushCmd)
+}

gitaly_test.go

@@ -327,78 +327,6 @@ func TestPostUploadPackProxiedToGitalyInterrupted(t *testing.T) {
 	}
 }
 
-func TestGetInfoRefsHandledLocallyDueToEmptyGitalySocketPath(t *testing.T) {
-	gitalyServer, _ := startGitalyServer(t, codes.OK)
-	defer gitalyServer.Stop()
-
-	apiResponse := gitOkBody(t)
-	apiResponse.GitalyServer.Address = ""
-	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, nil)
-
-	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.GitalyServer.Address = ""
-	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,
-		map[string]string{"Content-type": "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.GitalyServer.Address = ""
-	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,
-		map[string]string{"Content-type": "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 TestGetBlobProxiedToGitalySuccessfully(t *testing.T) {
 	gitalyServer, socketPath := startGitalyServer(t, codes.OK)
 	defer gitalyServer.Stop()

internal/git/git-http.go

@@ -95,11 +95,6 @@ func getService(r *http.Request) string {
 	return filepath.Base(r.URL.Path)
 }
 
-func isExitError(err error) bool {
-	_, ok := err.(*exec.ExitError)
-	return ok
-}
-
 func subCommand(rpc string) string {
 	return strings.TrimPrefix(rpc, "git-")
 }

internal/git/git-http_test.go

@@ -31,20 +31,11 @@ func createTestPayload() []byte {
 	return bytes.Repeat([]byte{'0'}, expectedBytes)
 }
 
-func TestHandleUploadPack(t *testing.T) {
-	testHandlePostRpc(t, "git-upload-pack", handleUploadPack)
-}
-
 func TestHandleReceivePack(t *testing.T) {
 	testHandlePostRpc(t, "git-receive-pack", handleReceivePack)
 }
 
 func testHandlePostRpc(t *testing.T, action string, handler func(*HttpResponseWriter, *http.Request, *api.Response) error) {
-	defer func(oldTesting bool) {
-		Testing = oldTesting
-	}(Testing)
-	Testing = true
-
 	execCommand = fakeExecCommand
 	defer func() { execCommand = exec.Command }()
 

internal/git/info-refs.go

@@ -11,11 +11,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
-var (
-	// Testing is only set during workhorse testing.
-	Testing = false
-)
-
 func GetInfoRefsHandler(a *api.API) http.Handler {
 	return repoPreAuthorizeHandler(a, handleGetInfoRefs)
 }
@@ -37,42 +32,13 @@ func handleGetInfoRefs(rw http.ResponseWriter, r *http.Request, a *api.Response)
 
 	gitProtocol := r.Header.Get("Git-Protocol")
 
-	var err error
-	if a.GitalyServer.Address == "" && Testing {
-		err = handleGetInfoRefsLocalTesting(w, a, rpc)
-	} else {
-		err = handleGetInfoRefsWithGitaly(r.Context(), w, a, rpc, gitProtocol)
-	}
+	err := handleGetInfoRefsWithGitaly(r.Context(), w, a, rpc, gitProtocol)
 
 	if err != nil {
 		helper.Fail500(w, r, fmt.Errorf("handleGetInfoRefs: %v", err))
 	}
 }
 
-// This code is not used in production. It is left over from before
-// Gitaly. We left it here to allow local workhorse tests to keep working
-// until we are done migrating Git HTTP to Gitaly.
-func handleGetInfoRefsLocalTesting(w http.ResponseWriter, a *api.Response, rpc string) error {
-	if err := pktLine(w, fmt.Sprintf("# service=%s\n", rpc)); err != nil {
-		return fmt.Errorf("pktLine: %v", err)
-	}
-	if err := pktFlush(w); err != nil {
-		return fmt.Errorf("pktFlush: %v", err)
-	}
-
-	cmd, err := startGitCommand(a, nil, w, rpc, "--advertise-refs")
-	if err != nil {
-		return fmt.Errorf("startGitCommand: %v", err)
-	}
-	defer helper.CleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
-
-	if err := cmd.Wait(); err != nil {
-		return fmt.Errorf("wait for %v: %v", cmd.Args, err)
-	}
-
-	return nil
-}
-
 func handleGetInfoRefsWithGitaly(ctx context.Context, w http.ResponseWriter, a *api.Response, rpc string, gitProtocol string) error {
 	smarthttp, err := gitaly.NewSmartHTTPClient(a.GitalyServer)
 	if err != nil {

internal/git/pktline.go

@@ -8,16 +8,6 @@ import (
 	"strconv"
 )
 
-func pktLine(w io.Writer, s string) error {
-	_, err := fmt.Fprintf(w, "%04x%s", len(s)+4, s)
-	return err
-}
-
-func pktFlush(w io.Writer) error {
-	_, err := fmt.Fprint(w, "0000")
-	return err
-}
-
 func scanDeepen(body io.Reader) bool {
 	scanner := bufio.NewScanner(body)
 	scanner.Split(pktLineSplitter)

internal/git/upload-pack.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"os"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/gitaly"
@@ -28,37 +27,9 @@ func handleUploadPack(w *HttpResponseWriter, r *http.Request, a *api.Response) e
 	action := getService(r)
 	writePostRPCHeader(w, action)
 
-	if Testing && a.GitalyServer.Address == "" {
-		// This code path is no longer reachable in GitLab 10.0
-		err = handleUploadPackLocally(a, r, buffer, w, action)
-	} else {
 	gitProtocol := r.Header.Get("Git-Protocol")
 
-		err = handleUploadPackWithGitaly(r.Context(), a, buffer, w, gitProtocol)
-	}
-
-	return err
-}
-
-func handleUploadPackLocally(a *api.Response, r *http.Request, stdin *os.File, stdout io.Writer, action string) error {
-	isShallowClone := scanDeepen(stdin)
-	if _, err := stdin.Seek(0, 0); err != nil {
-		return fmt.Errorf("seek tempfile: %v", err)
-	}
-
-	cmd, err := startGitCommand(a, stdin, stdout, action)
-	if err != nil {
-		return fmt.Errorf("startGitCommand: %v", err)
-	}
-	defer helper.CleanUpProcessGroup(cmd)
-
-	if err := cmd.Wait(); err != nil && !(isExitError(err) && isShallowClone) {
-		helper.LogError(r, fmt.Errorf("wait for %v: %v", cmd.Args, err))
-		// Return nil because the response body has been written to already.
-		return nil
-	}
-
-	return nil
+	return handleUploadPackWithGitaly(r.Context(), a, buffer, w, gitProtocol)
 }
 
 func handleUploadPackWithGitaly(ctx context.Context, a *api.Response, clientRequest io.Reader, clientResponse io.Writer, gitProtocol string) error {

internal/gitaly/gitaly.go

@@ -51,6 +51,16 @@ func NewRepositoryClient(server Server) (*RepositoryClient, error) {
 	return &RepositoryClient{grpcClient}, nil
 }
 
+// NewNamespaceClient is only used by the Gitaly integration tests at present
+func NewNamespaceClient(server Server) (*NamespaceClient, error) {
+	conn, err := getOrCreateConnection(server)
+	if err != nil {
+		return nil, err
+	}
+	grpcClient := pb.NewNamespaceServiceClient(conn)
+	return &NamespaceClient{grpcClient}, nil
+}
+
 func NewDiffClient(server Server) (*DiffClient, error) {
 	conn, err := getOrCreateConnection(server)
 	if err != nil {

internal/gitaly/namespace.go

+package gitaly
+
+import (
+	pb "gitlab.com/gitlab-org/gitaly-proto/go"
+)
+
+// NamespaceClient encapsulates NamespaceService calls
+type NamespaceClient struct {
+	pb.NamespaceServiceClient
+}

main_test.go

@@ -24,7 +24,6 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/git"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/gitaly"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
@@ -41,8 +40,6 @@ var checkoutDir = path.Join(scratchDir, "test")
 var cacheDir = path.Join(scratchDir, "cache")
 
 func TestMain(m *testing.M) {
-	git.Testing = true
-
 	if _, err := os.Stat(path.Join(testRepoRoot, testRepo)); os.IsNotExist(err) {
 		log.Fatal("cannot find test repository. Please run 'make prepare-tests'")
 	}
@@ -56,46 +53,6 @@ func TestMain(m *testing.M) {
 	os.Exit(m.Run())
 }
 
-func TestAllowedClone(t *testing.T) {
-	// Prepare clone directory
-	require.NoError(t, os.RemoveAll(scratchDir))
-
-	// Prepare test server and backend
-	ts := testAuthServer(nil, 200, gitOkBody(t))
-	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)
-	runOrFail(t, cloneCmd)
-
-	// We may have cloned an 'empty' repository, 'git log' will fail in it
-	logCmd := exec.Command("git", "log", "-1", "--oneline")
-	logCmd.Dir = checkoutDir
-	runOrFail(t, logCmd)
-}
-
-func TestAllowedShallowClone(t *testing.T) {
-	// Prepare clone directory
-	require.NoError(t, os.RemoveAll(scratchDir))
-
-	// Prepare test server and backend
-	ts := testAuthServer(nil, 200, gitOkBody(t))
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	// Shallow git clone (depth 1)
-	cloneCmd := exec.Command("git", "clone", "--depth", "1", fmt.Sprintf("%s/%s", ws.URL, testRepo), checkoutDir)
-	runOrFail(t, cloneCmd)
-
-	// We may have cloned an 'empty' repository, 'git log' will fail in it
-	logCmd := exec.Command("git", "log", "-1", "--oneline")
-	logCmd.Dir = checkoutDir
-	runOrFail(t, logCmd)
-}
-
 func TestDeniedClone(t *testing.T) {
 	// Prepare clone directory
 	require.NoError(t, os.RemoveAll(scratchDir))
@@ -113,24 +70,7 @@ func TestDeniedClone(t *testing.T) {
 	assert.Error(t, err, "git clone should have failed")
 }
 
-func TestAllowedPush(t *testing.T) {
-	preparePushRepo(t)
-
-	// Prepare the test server and backend
-	ts := testAuthServer(nil, 200, gitOkBody(t))
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	// Perform the git push
-	pushCmd := exec.Command("git", "push", fmt.Sprintf("%s/%s", ws.URL, testRepo), fmt.Sprintf("master:%s", newBranch()))
-	pushCmd.Dir = checkoutDir
-	runOrFail(t, pushCmd)
-}
-
 func TestDeniedPush(t *testing.T) {
-	preparePushRepo(t)
-
 	// Prepare the test server and backend
 	ts := testAuthServer(nil, 403, "Access denied")
 	defer ts.Close()
@@ -573,12 +513,6 @@ func prepareDownloadDir(t *testing.T) {
 	require.NoError(t, os.MkdirAll(scratchDir, 0755))
 }
 
-func preparePushRepo(t *testing.T) {
-	require.NoError(t, os.RemoveAll(scratchDir))
-	cloneCmd := exec.Command("git", "clone", path.Join(testRepoRoot, testRepo), checkoutDir)
-	runOrFail(t, cloneCmd)
-}
-
 func newBranch() string {
 	return fmt.Sprintf("branch-%d", time.Now().UnixNano())
 }