Merge branch '166-remove-archivereader' into 'master'

Remove local git archive support

Closes #166

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

archive_test.go

-package main
-
-import (
-	"bytes"
-	"fmt"
-	"io/ioutil"
-	"os"
-	"os/exec"
-	"path"
-	"strings"
-	"testing"
-)
-
-func TestAllowedDownloadZip(t *testing.T) {
-	prepareDownloadDir(t)
-
-	// Prepare test server and backend
-	archiveName := "foobar.zip"
-	ts := archiveOKServer(t, archiveName)
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("%s/%s/repository/archive.zip", ws.URL, testProject))
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	extractCmd := exec.Command("unzip", archiveName)
-	extractCmd.Dir = scratchDir
-	runOrFail(t, extractCmd)
-}
-
-func TestAllowedDownloadTar(t *testing.T) {
-	prepareDownloadDir(t)
-
-	// Prepare test server and backend
-	archiveName := "foobar.tar"
-	ts := archiveOKServer(t, archiveName)
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("%s/%s/repository/archive.tar", ws.URL, testProject))
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	extractCmd := exec.Command("tar", "xf", archiveName)
-	extractCmd.Dir = scratchDir
-	runOrFail(t, extractCmd)
-}
-
-func TestAllowedDownloadTarGz(t *testing.T) {
-	prepareDownloadDir(t)
-
-	// Prepare test server and backend
-	archiveName := "foobar.tar.gz"
-	ts := archiveOKServer(t, archiveName)
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("%s/%s/repository/archive.tar.gz", ws.URL, testProject))
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	extractCmd := exec.Command("tar", "zxf", archiveName)
-	extractCmd.Dir = scratchDir
-	runOrFail(t, extractCmd)
-}
-
-func TestAllowedDownloadTarBz2(t *testing.T) {
-	prepareDownloadDir(t)
-
-	// Prepare test server and backend
-	archiveName := "foobar.tar.bz2"
-	ts := archiveOKServer(t, archiveName)
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("%s/%s/repository/archive.tar.bz2", ws.URL, testProject))
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	extractCmd := exec.Command("tar", "jxf", archiveName)
-	extractCmd.Dir = scratchDir
-	runOrFail(t, extractCmd)
-}
-
-func TestAllowedApiDownloadZip(t *testing.T) {
-	prepareDownloadDir(t)
-
-	// Prepare test server and backend
-	archiveName := "foobar.zip"
-	ts := archiveOKServer(t, archiveName)
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("%s/api/v3/projects/123/repository/archive.zip", ws.URL))
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	extractCmd := exec.Command("unzip", archiveName)
-	extractCmd.Dir = scratchDir
-	runOrFail(t, extractCmd)
-}
-
-func TestAllowedApiDownloadZipWithSlash(t *testing.T) {
-	prepareDownloadDir(t)
-
-	// Prepare test server and backend
-	archiveName := "foobar.zip"
-	ts := archiveOKServer(t, archiveName)
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	// Use foo%2Fbar instead of a numeric ID
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("%s/api/v3/projects/foo%%2Fbar/repository/archive.zip", ws.URL))
-	if !strings.Contains(downloadCmd.Args[3], `projects/foo%2Fbar/repository`) {
-		t.Fatalf("Cannot find percent-2F: %v", downloadCmd.Args)
-	}
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	extractCmd := exec.Command("unzip", archiveName)
-	extractCmd.Dir = scratchDir
-	runOrFail(t, extractCmd)
-}
-
-func TestDownloadCacheHit(t *testing.T) {
-	prepareDownloadDir(t)
-
-	// Prepare test server and backend
-	archiveName := "foobar.zip"
-	ts := archiveOKServer(t, archiveName)
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	if err := os.MkdirAll(cacheDir, 0755); err != nil {
-		t.Fatal(err)
-	}
-	cachedContent := []byte("cached")
-	if err := ioutil.WriteFile(path.Join(cacheDir, archiveName), cachedContent, 0644); err != nil {
-		t.Fatal(err)
-	}
-
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("%s/api/v3/projects/123/repository/archive.zip", ws.URL))
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	actual, err := ioutil.ReadFile(path.Join(scratchDir, archiveName))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if !bytes.Equal(actual, cachedContent) {
-		t.Fatal("Unexpected file contents in download")
-	}
-}
-
-func TestDownloadCacheCreate(t *testing.T) {
-	prepareDownloadDir(t)
-
-	// Prepare test server and backend
-	archiveName := "foobar.zip"
-	ts := archiveOKServer(t, archiveName)
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	downloadCmd := exec.Command("curl", "-J", "-O", fmt.Sprintf("%s/api/v3/projects/123/repository/archive.zip", ws.URL))
-	downloadCmd.Dir = scratchDir
-	runOrFail(t, downloadCmd)
-
-	compareCmd := exec.Command("cmp", path.Join(cacheDir, archiveName), path.Join(scratchDir, archiveName))
-	if err := compareCmd.Run(); err != nil {
-		t.Fatalf("Comparison between downloaded file and cache item failed: %s", err)
-	}
-}

gitaly_integration_test.go

@@ -3,10 +3,13 @@
 package main
 
 import (
+	"archive/tar"
+	"bytes"
 	"context"
 	"fmt"
 	"os"
 	"os/exec"
+	"path"
 	"strconv"
 	"testing"
 
@@ -20,11 +23,13 @@ import (
 )
 
 var (
-	gitalyAddress string
+	gitalyAddress    string
+	jsonGitalyServer string
 )
 
 func init() {
 	gitalyAddress = os.Getenv("GITALY_ADDRESS")
+	jsonGitalyServer = fmt.Sprintf(`"GitalyServer":{"Address":"%s", "Token": ""}`, gitalyAddress)
 }
 
 func skipUnlessRealGitaly(t *testing.T) {
@@ -156,14 +161,14 @@ func TestAllowedGetGitBlob(t *testing.T) {
 
 	jsonParams := fmt.Sprintf(
 		`{
-			"GitalyServer":{"Address":"%s", "Token":""},
+			%s,
 			"GetBlobRequest":{
 				"repository":{"storage_name":"%s", "relative_path":"%s"},
 				"oid":"%s",
 				"limit":-1
 			}
 		}`,
-		gitalyAddress, apiResponse.Repository.StorageName, apiResponse.Repository.RelativePath, oid,
+		jsonGitalyServer, apiResponse.Repository.StorageName, apiResponse.Repository.RelativePath, oid,
 	)
 
 	resp, body, err := doSendDataRequest("/something", "git-blob", jsonParams)
@@ -175,3 +180,48 @@ func TestAllowedGetGitBlob(t *testing.T) {
 	testhelper.AssertResponseHeader(t, resp, "Content-Length", strconv.Itoa(bodyLen))
 	assertNginxResponseBuffering(t, "no", resp, "GET %q: nginx response buffering", resp.Request.URL)
 }
+
+func TestAllowedGetGitArchive(t *testing.T) {
+	skipUnlessRealGitaly(t)
+
+	// Create the repository in the Gitaly server
+	apiResponse := realGitalyOkBody(t)
+	repo := apiResponse.Repository
+	require.NoError(t, ensureGitalyRepository(t, apiResponse))
+
+	archivePath := path.Join(scratchDir, "my/path")
+	archivePrefix := "repo-1"
+
+	jsonParams := fmt.Sprintf(
+		`{
+			%s,
+			"GitalyRepository":{"storage_name":"%s","relative_path":"%s"},
+			"ArchivePath":"%s",
+			"ArchivePrefix":"%s",
+			"CommitId":"%s"
+		}`,
+		jsonGitalyServer, repo.StorageName, repo.RelativePath, archivePath, archivePrefix, "HEAD",
+	)
+
+	resp, body, err := doSendDataRequest("/archive.tar", "git-archive", jsonParams)
+	require.NoError(t, err)
+	assert.Equal(t, 200, resp.StatusCode, "GET %q: status code", resp.Request.URL)
+	assertNginxResponseBuffering(t, "no", resp, "GET %q: nginx response buffering", resp.Request.URL)
+
+	// Ensure the tar file is readable
+	foundEntry := false
+	tr := tar.NewReader(bytes.NewReader(body))
+	for {
+		hdr, err := tr.Next()
+		if err != nil {
+			break
+		}
+
+		if hdr.Name == archivePrefix+"/" {
+			foundEntry = true
+			break
+		}
+	}
+
+	assert.True(t, foundEntry, "Couldn't find %v directory entry", archivePrefix)
+}

internal/git/archive.go

@@ -25,7 +25,6 @@ import (
 
 type archive struct{ senddata.Prefix }
 type archiveParams struct {
-	RepoPath         string
 	ArchivePath      string
 	ArchivePrefix    string
 	CommitId         string
@@ -100,17 +99,10 @@ func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string
 	}
 
 	var archiveReader io.Reader
-	if params.GitalyServer.Address != "" {
-		archiveReader, err = handleArchiveWithGitaly(r, params, format)
 
-		if err != nil {
-			err = fmt.Errorf("operations.GetArchive: %v", err)
-		}
-	} else {
-		archiveReader, err = newArchiveReader(r.Context(), params.RepoPath, format, params.ArchivePrefix, params.CommitId)
-	}
+	archiveReader, err = handleArchiveWithGitaly(r, params, format)
 	if err != nil {
-		helper.Fail500(w, r, err)
+		helper.Fail500(w, r, fmt.Errorf("operations.GetArchive: %v", err))
 		return
 	}
 

internal/git/archivereader.go

-package git
-
-import (
-	"context"
-	"fmt"
-	"io"
-	"os/exec"
-	"syscall"
-
-	pb "gitlab.com/gitlab-org/gitaly-proto/go"
-
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
-)
-
-func parseArchiveFormat(format pb.GetArchiveRequest_Format) (*exec.Cmd, string) {
-	switch format {
-	case pb.GetArchiveRequest_TAR:
-		return nil, "tar"
-	case pb.GetArchiveRequest_TAR_GZ:
-		return exec.Command("gzip", "-c", "-n"), "tar"
-	case pb.GetArchiveRequest_TAR_BZ2:
-		return exec.Command("bzip2", "-c"), "tar"
-	case pb.GetArchiveRequest_ZIP:
-		return nil, "zip"
-	default:
-		return nil, "invalid format"
-	}
-}
-
-type archiveReader struct {
-	waitCmds []*exec.Cmd
-	stdout   io.Reader
-}
-
-func (a *archiveReader) Read(p []byte) (int, error) {
-	n, err := a.stdout.Read(p)
-
-	if err != io.EOF {
-		return n, err
-	}
-
-	err = a.wait()
-	if err == nil {
-		err = io.EOF
-	}
-	return n, err
-}
-
-func (a *archiveReader) wait() error {
-	var waitErrors []error
-
-	// Must call Wait() on _all_ commands
-	for _, cmd := range a.waitCmds {
-		waitErrors = append(waitErrors, cmd.Wait())
-	}
-
-	for _, err := range waitErrors {
-		if err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func newArchiveReader(ctx context.Context, repoPath string, format pb.GetArchiveRequest_Format, archivePrefix string, commitId string) (a *archiveReader, err error) {
-	a = &archiveReader{}
-
-	compressCmd, formatArg := parseArchiveFormat(format)
-	archiveCmd := gitCommand("git", "--git-dir="+repoPath, "archive", "--format="+formatArg, "--prefix="+archivePrefix+"/", commitId)
-
-	var archiveStdout io.ReadCloser
-	archiveStdout, err = archiveCmd.StdoutPipe()
-	if err != nil {
-		return nil, fmt.Errorf("SendArchive: archive stdout: %v", err)
-	}
-	defer func() {
-		if err != nil {
-			archiveStdout.Close()
-		}
-	}()
-
-	a.stdout = archiveStdout
-
-	if compressCmd != nil {
-		compressCmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
-		compressCmd.Stdin = archiveStdout
-
-		var compressStdout io.ReadCloser
-		compressStdout, err = compressCmd.StdoutPipe()
-		if err != nil {
-			return nil, fmt.Errorf("SendArchive: compress stdout: %v", err)
-		}
-		defer func() {
-			if err != nil {
-				compressStdout.Close()
-			}
-		}()
-
-		if err := compressCmd.Start(); err != nil {
-			return nil, fmt.Errorf("SendArchive: start %v: %v", compressCmd.Args, err)
-		}
-
-		go ctxKill(ctx, compressCmd)
-		a.waitCmds = append(a.waitCmds, compressCmd)
-
-		a.stdout = compressStdout
-		archiveStdout.Close()
-	}
-
-	if err := archiveCmd.Start(); err != nil {
-		return nil, fmt.Errorf("SendArchive: start %v: %v", archiveCmd.Args, err)
-	}
-
-	go ctxKill(ctx, archiveCmd)
-	a.waitCmds = append(a.waitCmds, archiveCmd)
-
-	return a, nil
-}
-
-func ctxKill(ctx context.Context, cmd *exec.Cmd) {
-	<-ctx.Done()
-	helper.CleanUpProcessGroup(cmd)
-	cmd.Wait()
-}

main_test.go

@@ -527,27 +527,6 @@ func testAuthServer(url *regexp.Regexp, code int, body interface{}) *httptest.Se
 	})
 }
 
-func archiveOKServer(t *testing.T, archiveName string) *httptest.Server {
-	return testhelper.TestServerWithHandler(regexp.MustCompile("."), func(w http.ResponseWriter, r *http.Request) {
-		cwd, err := os.Getwd()
-		require.NoError(t, err)
-
-		archivePath := path.Join(cwd, cacheDir, archiveName)
-
-		params := struct{ RepoPath, ArchivePath, CommitID, ArchivePrefix string }{
-			repoPath(t),
-			archivePath,
-			"c7fbe50c7c7419d9701eebe64b1fdacc3df5b9dd",
-			"foobar123",
-		}
-		jsonData, err := json.Marshal(params)
-		require.NoError(t, err)
-
-		encodedJSON := base64.URLEncoding.EncodeToString(jsonData)
-		w.Header().Set("Gitlab-Workhorse-Send-Data", "git-archive:"+encodedJSON)
-	})
-}
-
 func newUpstreamConfig(authBackend string) *config.Config {
 	return &config.Config{
 		Version:      "123",