Merge branch 'zip-subprocess' into 'master'

Use gitlab-zip-cat to send zip entries

Fixes https://gitlab.com/gitlab-org/gitlab-workhorse/issues/17

See merge request !31

.gitignore

@@ -2,3 +2,5 @@ gitlab-workhorse
 testdata/data
 testdata/scratch
 testdata/public
+gitlab-zip-cat
+gitlab-zip-metadata

Makefile

 PREFIX=/usr/local
 VERSION=$(shell git describe)-$(shell date -u +%Y%m%d.%H%M%S)
+GOBUILD=go build -ldflags "-X main.Version=${VERSION}"
+
+all: gitlab-zip-cat gitlab-zip-metadata gitlab-workhorse
+
+gitlab-zip-cat:	$(shell find cmd/gitlab-zip-cat/ -name '*.go')
+	${GOBUILD} -o $@ ./cmd/$@
+	
+gitlab-zip-metadata:	$(shell find cmd/gitlab-zip-metadata/ -name '*.go')
+	${GOBUILD} -o $@ ./cmd/$@
 
 gitlab-workhorse: $(shell find . -name '*.go')
-	go build -ldflags "-X main.Version=${VERSION}" -o gitlab-workhorse
+	${GOBUILD} -o $@
 
-install: gitlab-workhorse
-	install gitlab-workhorse ${PREFIX}/bin/
+install: gitlab-workhorse gitlab-zip-cat gitlab-zip-metadata
+	install gitlab-workhorse gitlab-zip-cat gitlab-zip-metadata ${PREFIX}/bin/
 
 .PHONY: test
-test: testdata/data/group/test.git clean-workhorse gitlab-workhorse
+test: testdata/data/group/test.git clean-workhorse all
 	go fmt ./... | awk '{ print } END { if (NR > 0) { print "Please run go fmt"; exit 1 } }'
-	go test ./...
+	support/path go test ./...
 	@echo SUCCESS
 
 coverage: testdata/data/group/test.git
@@ -30,4 +39,4 @@ clean:	clean-workhorse
 
 .PHONY:	clean-workhorse
 clean-workhorse:
-	rm -f gitlab-workhorse
+	rm -f gitlab-workhorse gitlab-zip-cat gitlab-zip-metadata

cmd/gitlab-zip-cat/main.go

+package main
+
+import (
+	"../../internal/zipartifacts"
+	"archive/zip"
+	"flag"
+	"fmt"
+	"io"
+	"os"
+)
+
+const progName = "gitlab-zip-cat"
+
+var Version = "unknown"
+
+var printVersion = flag.Bool("version", false, "Print version and exit")
+
+func main() {
+	flag.Parse()
+
+	version := fmt.Sprintf("%s %s", progName, Version)
+	if *printVersion {
+		fmt.Println(version)
+		os.Exit(0)
+	}
+
+	if len(os.Args) != 3 {
+		fmt.Fprintf(os.Stderr, "Usage: %s FILE.ZIP ENTRY", progName)
+		os.Exit(1)
+	}
+
+	archiveFileName := os.Args[1]
+
+	fileName, err := zipartifacts.DecodeFileEntry(os.Args[2])
+	if err != nil {
+		fatalError(fmt.Errorf("decode entry %q: %v", os.Args[2], err))
+	}
+
+	archive, err := zip.OpenReader(archiveFileName)
+	if err != nil {
+		notFoundError(fmt.Errorf("open %q: %v", archiveFileName, err))
+	}
+	defer archive.Close()
+
+	file := findFileInZip(fileName, &archive.Reader)
+	if file == nil {
+		notFoundError(fmt.Errorf("find %q in %q: not found", fileName, archiveFileName))
+	}
+	// Start decompressing the file
+	reader, err := file.Open()
+	if err != nil {
+		fatalError(fmt.Errorf("open %q in %q: %v", fileName, archiveFileName, err))
+	}
+	defer reader.Close()
+
+	if _, err := fmt.Printf("%d\n", file.UncompressedSize64); err != nil {
+		fatalError(fmt.Errorf("write file size: %v", err))
+	}
+
+	if _, err := io.Copy(os.Stdout, reader); err != nil {
+		fatalError(fmt.Errorf("write %q from %q to stdout: %v", fileName, archiveFileName, err))
+	}
+}
+
+func findFileInZip(fileName string, archive *zip.Reader) *zip.File {
+	for _, file := range archive.File {
+		if file.Name == fileName {
+			return file
+		}
+	}
+	return nil
+}
+
+func printError(err error) {
+	fmt.Fprintf(os.Stderr, "%s: %v", progName, err)
+}
+
+func fatalError(err error) {
+	printError(err)
+	os.Exit(1)
+}
+
+func notFoundError(err error) {
+	printError(err)
+	os.Exit(zipartifacts.StatusEntryNotFound)
+}

cmd/gitlab-zip-metadata/main.go

+package main
+
+import (
+	"../../internal/zipartifacts"
+	"flag"
+	"fmt"
+	"os"
+)
+
+const progName = "gitlab-zip-metadata"
+
+var Version = "unknown"
+
+var printVersion = flag.Bool("version", false, "Print version and exit")
+
+func main() {
+	flag.Parse()
+
+	version := fmt.Sprintf("%s %s", progName, Version)
+	if *printVersion {
+		fmt.Println(version)
+		os.Exit(0)
+	}
+
+	if len(os.Args) != 2 {
+		fmt.Fprintf(os.Stderr, "Usage: %s FILE.ZIP", progName)
+		os.Exit(1)
+	}
+	if err := zipartifacts.GenerateZipMetadataFromFile(os.Args[1], os.Stdout); err != nil {
+		fmt.Fprintf(os.Stderr, "%s: %v\n", progName, err)
+		if err == os.ErrInvalid {
+			os.Exit(zipartifacts.StatusNotZip)
+		}
+		os.Exit(1)
+	}
+}

internal/artifacts/artifact_download.go

@@ -3,26 +3,20 @@ package artifacts
 import (
 	"../api"
 	"../helper"
-	"archive/zip"
-	"encoding/base64"
+	"../zipartifacts"
+	"bufio"
 	"errors"
 	"fmt"
 	"io"
 	"mime"
 	"net/http"
 	"os"
+	"os/exec"
 	"path/filepath"
-	"strconv"
+	"strings"
+	"syscall"
 )
 
-func decodeFileEntry(entry string) (string, error) {
-	decoded, err := base64.StdEncoding.DecodeString(entry)
-	if err != nil {
-		return "", err
-	}
-	return string(decoded), nil
-}
-
 func detectFileContentType(fileName string) string {
 	contentType := mime.TypeByExtension(filepath.Ext(fileName))
 	if contentType == "" {
@@ -31,44 +25,59 @@ func detectFileContentType(fileName string) string {
 	return contentType
 }
 
-func findFileInZip(fileName string, archive *zip.Reader) *zip.File {
-	for _, file := range archive.File {
-		if file.Name == fileName {
-			return file
-		}
-	}
-	return nil
-}
-
-func unpackFileFromZip(archiveFileName, fileName string, headers http.Header, output io.Writer) error {
-	archive, err := zip.OpenReader(archiveFileName)
+func unpackFileFromZip(archiveFileName, encodedFilename string, headers http.Header, output io.Writer) error {
+	fileName, err := zipartifacts.DecodeFileEntry(encodedFilename)
 	if err != nil {
 		return err
 	}
-	defer archive.Close()
 
-	file := findFileInZip(fileName, &archive.Reader)
-	if file == nil {
-		return os.ErrNotExist
+	catFile := exec.Command("gitlab-zip-cat", archiveFileName, encodedFilename)
+	catFile.Stderr = os.Stderr
+	catFile.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+	stdout, err := catFile.StdoutPipe()
+	if err != nil {
+		return fmt.Errorf("create gitlab-zip-cat stdout pipe: %v", err)
 	}
 
-	// Start decompressing the file
-	reader, err := file.Open()
-	if err != nil {
-		return err
+	if err := catFile.Start(); err != nil {
+		return fmt.Errorf("start %v: %v", catFile.Args, err)
 	}
-	defer reader.Close()
+	defer helper.CleanUpProcessGroup(catFile)
 
 	basename := filepath.Base(fileName)
+	reader := bufio.NewReader(stdout)
+	contentLength, err := reader.ReadString('\n')
+	if err != nil {
+		if catFileErr := waitCatFile(catFile); catFileErr != nil {
+			return catFileErr
+		}
+		return fmt.Errorf("read content-length: %v", err)
+	}
+	contentLength = strings.TrimSuffix(contentLength, "\n")
 
 	// Write http headers about the file
-	headers.Set("Content-Length", strconv.FormatInt(int64(file.UncompressedSize64), 10))
-	headers.Set("Content-Type", detectFileContentType(file.Name))
+	headers.Set("Content-Length", contentLength)
+	headers.Set("Content-Type", detectFileContentType(fileName))
 	headers.Set("Content-Disposition", "attachment; filename=\""+escapeQuotes(basename)+"\"")
-
 	// Copy file body to client
-	_, err = io.Copy(output, reader)
-	return err
+	if _, err := io.Copy(output, reader); err != nil {
+		return fmt.Errorf("copy %v stdout: %v", catFile.Args, err)
+	}
+
+	return waitCatFile(catFile)
+}
+
+func waitCatFile(cmd *exec.Cmd) error {
+	err := cmd.Wait()
+	if err == nil {
+		return nil
+	}
+
+	if st, ok := helper.ExitStatus(err); ok && st == zipartifacts.StatusEntryNotFound {
+		return os.ErrNotExist
+	}
+	return fmt.Errorf("wait for %v to finish: %v", cmd.Args, err)
+
 }
 
 // Artifacts downloader doesn't support ranges when downloading a single file
@@ -79,13 +88,7 @@ func DownloadArtifact(myAPI *api.API) http.Handler {
 			return
 		}
 
-		fileName, err := decodeFileEntry(a.Entry)
-		if err != nil {
-			helper.Fail500(w, err)
-			return
-		}
-
-		err = unpackFileFromZip(a.Archive, fileName, w.Header(), w)
+		err := unpackFileFromZip(a.Archive, a.Entry, w.Header(), w)
 		if os.IsNotExist(err) {
 			http.NotFound(w, r)
 			return

internal/artifacts/artifacts_upload.go

@@ -4,12 +4,15 @@ import (
 	"../api"
 	"../helper"
 	"../upload"
+	"../zipartifacts"
 	"errors"
 	"fmt"
 	"io/ioutil"
 	"mime/multipart"
 	"net/http"
 	"os"
+	"os/exec"
+	"syscall"
 )
 
 type artifactsUploadProcessor struct {
@@ -36,10 +39,19 @@ func (a *artifactsUploadProcessor) ProcessFile(formName, fileName string, writer
 	a.metadataFile = tempFile.Name()
 
 	// Generate metadata and save to file
-	err = generateZipMetadataFromFile(fileName, tempFile)
-	if err == os.ErrInvalid {
-		return nil
-	} else if err != nil {
+	zipMd := exec.Command("gitlab-zip-metadata", fileName)
+	zipMd.Stderr = os.Stderr
+	zipMd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+	zipMd.Stdout = tempFile
+
+	if err := zipMd.Start(); err != nil {
+		return err
+	}
+	defer helper.CleanUpProcessGroup(zipMd)
+	if err := zipMd.Wait(); err != nil {
+		if st, ok := helper.ExitStatus(err); ok && st == zipartifacts.StatusNotZip {
+			return nil
+		}
 		return err
 	}
 

internal/artifacts/artifacts_upload_test.go

@@ -5,6 +5,7 @@ import (
 	"../helper"
 	"../proxy"
 	"../testhelper"
+	"../zipartifacts"
 	"archive/zip"
 	"bytes"
 	"compress/gzip"
@@ -71,7 +72,7 @@ func testArtifactsUploadServer(t *testing.T, tempPath string) *httptest.Server {
 			w.WriteHeader(404)
 			return
 		}
-		if !bytes.HasPrefix(metadata, []byte(metadataHeaderPrefix+metadataHeader)) {
+		if !bytes.HasPrefix(metadata, []byte(zipartifacts.MetadataHeaderPrefix+zipartifacts.MetadataHeader)) {
 			w.WriteHeader(400)
 			return
 		}

internal/git/archive.go

@@ -79,7 +79,7 @@ func handleGetArchive(w http.ResponseWriter, r *http.Request, a *api.Response) {
 		helper.Fail500(w, fmt.Errorf("handleGetArchive: start %v: %v", archiveCmd.Args, err))
 		return
 	}
-	defer cleanUpProcessGroup(archiveCmd) // Ensure brute force subprocess clean-up
+	defer helper.CleanUpProcessGroup(archiveCmd) // Ensure brute force subprocess clean-up
 
 	var stdout io.ReadCloser
 	if compressCmd == nil {
@@ -99,7 +99,7 @@ func handleGetArchive(w http.ResponseWriter, r *http.Request, a *api.Response) {
 			helper.Fail500(w, fmt.Errorf("handleGetArchive: start %v: %v", compressCmd.Args, err))
 			return
 		}
-		defer cleanUpProcessGroup(compressCmd)
+		defer helper.CleanUpProcessGroup(compressCmd)
 
 		archiveStdout.Close()
 	}

internal/git/command.go

@@ -23,18 +23,3 @@ func gitCommand(gl_id string, name string, args ...string) *exec.Cmd {
 	cmd.Stderr = os.Stderr
 	return cmd
 }
-
-func cleanUpProcessGroup(cmd *exec.Cmd) {
-	if cmd == nil {
-		return
-	}
-
-	process := cmd.Process
-	if process != nil && process.Pid > 0 {
-		// Send SIGTERM to the process group of cmd
-		syscall.Kill(-process.Pid, syscall.SIGTERM)
-	}
-
-	// reap our child process
-	cmd.Wait()
-}

internal/git/git-http.go

@@ -72,7 +72,7 @@ func handleGetInfoRefs(w http.ResponseWriter, r *http.Request, a *api.Response)
 		helper.Fail500(w, fmt.Errorf("handleGetInfoRefs: start %v: %v", cmd.Args, err))
 		return
 	}
-	defer cleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
+	defer helper.CleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
 
 	// Start writing the response
 	w.Header().Add("Content-Type", fmt.Sprintf("application/x-%s-advertisement", rpc))
@@ -125,7 +125,7 @@ func handlePostRPC(w http.ResponseWriter, r *http.Request, a *api.Response) {
 		helper.Fail500(w, fmt.Errorf("handlePostRPC: start %v: %v", cmd.Args, err))
 		return
 	}
-	defer cleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
+	defer helper.CleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
 
 	// Write the client request body to Git's standard input
 	if _, err := io.Copy(stdin, r.Body); err != nil {

internal/helper/helpers.go

@@ -6,6 +6,8 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"os/exec"
+	"syscall"
 )
 
 func Fail500(w http.ResponseWriter, err error) {
@@ -69,3 +71,32 @@ func HTTPError(w http.ResponseWriter, r *http.Request, error string, code int) {
 
 	http.Error(w, error, code)
 }
+
+func CleanUpProcessGroup(cmd *exec.Cmd) {
+	if cmd == nil {
+		return
+	}
+
+	process := cmd.Process
+	if process != nil && process.Pid > 0 {
+		// Send SIGTERM to the process group of cmd
+		syscall.Kill(-process.Pid, syscall.SIGTERM)
+	}
+
+	// reap our child process
+	cmd.Wait()
+}
+
+func ExitStatus(err error) (int, bool) {
+	exitError, ok := err.(*exec.ExitError)
+	if !ok {
+		return 0, false
+	}
+
+	waitStatus, ok := exitError.Sys().(syscall.WaitStatus)
+	if !ok {
+		return 0, false
+	}
+
+	return waitStatus.ExitStatus(), true
+}

internal/zipartifacts/codes.go

+package zipartifacts
+
+// These are exit codes used by subprocesses in cmd/gitlab-zip-xxx
+const (
+	StatusNotZip = 10 + iota
+	StatusEntryNotFound
+)

internal/zipartifacts/entry.go

+package zipartifacts
+
+import (
+	"encoding/base64"
+)
+
+func DecodeFileEntry(entry string) (string, error) {
+	decoded, err := base64.StdEncoding.DecodeString(entry)
+	if err != nil {
+		return "", err
+	}
+	return string(decoded), nil
+}

internal/artifacts/metadata.go → internal/zipartifacts/metadata.go

-package artifacts
+package zipartifacts
 
 import (
 	"archive/zip"
@@ -19,8 +19,8 @@ type metadata struct {
 	Comment  string `json:"comment,omitempty"`
 }
 
-const metadataHeaderPrefix = "\x00\x00\x00&" // length of string below, encoded properly
-const metadataHeader = "GitLab Build Artifacts Metadata 0.0.2\n"
+const MetadataHeaderPrefix = "\x00\x00\x00&" // length of string below, encoded properly
+const MetadataHeader = "GitLab Build Artifacts Metadata 0.0.2\n"
 
 func newMetadata(file *zip.File) metadata {
 	return metadata{
@@ -56,7 +56,7 @@ func writeZipEntryMetadata(output io.Writer, entry *zip.File) error {
 }
 
 func generateZipMetadata(output io.Writer, archive *zip.Reader) error {
-	err := writeString(output, metadataHeader)
+	err := writeString(output, MetadataHeader)
 	if err != nil {
 		return err
 	}
@@ -77,7 +77,7 @@ func generateZipMetadata(output io.Writer, archive *zip.Reader) error {
 	return nil
 }
 
-func generateZipMetadataFromFile(fileName string, w io.Writer) error {
+func GenerateZipMetadataFromFile(fileName string, w io.Writer) error {
 	archive, err := zip.OpenReader(fileName)
 	if err != nil {
 		// Ignore non-zip archives

main_test.go

@@ -497,8 +497,9 @@ func TestArtifactsUpload(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
-		if len(r.MultipartForm.Value) != 2 { // 1 file name, 1 file path
-			t.Error("Expected to receive exactly 2 values")
+		nValues := 2 // filename + path for just the upload (no metadata because we are not POSTing a valid zip file)
+		if len(r.MultipartForm.Value) != nValues {
+			t.Errorf("Expected to receive exactly %d values", nValues)
 		}
 		if len(r.MultipartForm.File) != 0 {
 			t.Error("Expected to not receive any files")

support/path

+#!/bin/sh
+exec env PATH=$(pwd):${PATH} "$@"