Merge branch 'fix-no-tmpfile' into 'master'

Artifacts and Uploads must allow Objects Storage only requests

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

internal/artifacts/artifacts_store_test.go

@@ -63,7 +63,12 @@ func TestUploadHandlerSendingToExternalStorage(t *testing.T) {
 	defer os.RemoveAll(tempPath)
 
 	archiveData, md5 := createTestZipArchive(t)
-	contentBuffer, contentType := createTestMultipartForm(t, archiveData)
+	archiveFile, err := ioutil.TempFile("", "artifact.zip")
+	require.NoError(t, err)
+	defer os.Remove(archiveFile.Name())
+	_, err = archiveFile.Write(archiveData)
+	require.NoError(t, err)
+	archiveFile.Close()
 
 	storeServerCalled := 0
 	storeServerMux := http.NewServeMux()
@@ -78,6 +83,9 @@ func TestUploadHandlerSendingToExternalStorage(t *testing.T) {
 		w.Header().Set("ETag", md5)
 		w.WriteHeader(200)
 	})
+	storeServerMux.HandleFunc("/store-id", func(w http.ResponseWriter, r *http.Request) {
+		http.ServeFile(w, r, archiveFile.Name())
+	})
 
 	responseProcessorCalled := 0
 	responseProcessor := func(w http.ResponseWriter, r *http.Request) {
@@ -90,22 +98,48 @@ func TestUploadHandlerSendingToExternalStorage(t *testing.T) {
 	storeServer := httptest.NewServer(storeServerMux)
 	defer storeServer.Close()
 
-	authResponse := api.Response{
+	tests := []struct {
+		name    string
+		preauth api.Response
+	}{
+		{
+			name: "ObjectStore Upload",
+			preauth: api.Response{
+				RemoteObject: api.RemoteObject{
+					StoreURL: storeServer.URL + "/url/put",
+					ID:       "store-id",
+					GetURL:   storeServer.URL + "/store-id",
+				},
+			},
+		},
+		{
+			name: "ObjectStore and FileStore Upload",
+			preauth: api.Response{
 				TempPath: tempPath,
 				RemoteObject: api.RemoteObject{
 					StoreURL: storeServer.URL + "/url/put",
 					ID:       "store-id",
 					GetURL:   storeServer.URL + "/store-id",
 				},
+			},
+		},
 	}
 
-	ts := testArtifactsUploadServer(t, authResponse, responseProcessor)
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			storeServerCalled = 0
+			responseProcessorCalled = 0
+
+			ts := testArtifactsUploadServer(t, test.preauth, responseProcessor)
 			defer ts.Close()
 
+			contentBuffer, contentType := createTestMultipartForm(t, archiveData)
 			response := testUploadArtifacts(contentType, &contentBuffer, t, ts)
 			testhelper.AssertResponseCode(t, response, 200)
 			assert.Equal(t, 1, storeServerCalled, "store should be called only once")
 			assert.Equal(t, 1, responseProcessorCalled, "response processor should be called only once")
+		})
+	}
 }
 
 func TestUploadHandlerSendingToExternalStorageAndStorageServerUnreachable(t *testing.T) {

internal/artifacts/artifacts_upload.go

@@ -31,6 +31,9 @@ func (a *artifactsUploadProcessor) generateMetadataFromZip(ctx context.Context,
 		LocalTempPath:  a.opts.LocalTempPath,
 		TempFilePrefix: "metadata.gz",
 	}
+	if metaOpts.LocalTempPath == "" {
+		metaOpts.LocalTempPath = os.TempDir()
+	}
 
 	fileName := file.LocalPath
 	if fileName == "" {
@@ -115,11 +118,6 @@ func (a *artifactsUploadProcessor) Name() string {
 
 func UploadArtifacts(myAPI *api.API, h http.Handler) http.Handler {
 	return myAPI.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
-		if a.TempPath == "" {
-			helper.Fail500(w, r, fmt.Errorf("UploadArtifacts: TempPath is empty"))
-			return
-		}
-
 		mg := &artifactsUploadProcessor{opts: filestore.GetOpts(a)}
 
 		upload.HandleFileUploads(w, r, h, a, mg)

internal/artifacts/artifacts_upload_test.go

@@ -16,6 +16,7 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/proxy"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
@@ -38,23 +39,33 @@ func testArtifactsUploadServer(t *testing.T, authResponse api.Response, bodyProc
 		w.Write(data)
 	})
 	mux.HandleFunc("/url/path", func(w http.ResponseWriter, r *http.Request) {
+		opts := filestore.GetOpts(&authResponse)
+
 		if r.Method != "POST" {
 			t.Fatal("Expected POST request")
 		}
+		if opts.IsLocal() {
 			if r.FormValue("file.path") == "" {
 				w.WriteHeader(501)
 				return
 			}
-		if r.FormValue("metadata.path") == "" {
-			w.WriteHeader(502)
-			return
-		}
 
 			_, err := ioutil.ReadFile(r.FormValue("file.path"))
 			if err != nil {
 				w.WriteHeader(404)
 				return
 			}
+		}
+
+		if opts.IsRemote() && r.FormValue("file.remote_url") == "" {
+			w.WriteHeader(501)
+			return
+		}
+
+		if r.FormValue("metadata.path") == "" {
+			w.WriteHeader(502)
+			return
+		}
 
 		metadata, err := ioutil.ReadFile(r.FormValue("metadata.path"))
 		if err != nil {

internal/upload/uploads.go

@@ -22,8 +22,9 @@ type MultipartFormProcessor interface {
 }
 
 func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor) {
-	if preauth.TempPath == "" {
-		helper.Fail500(w, r, fmt.Errorf("handleFileUploads: tempPath empty"))
+	opts := filestore.GetOpts(preauth)
+	if !opts.IsLocal() && !opts.IsRemote() {
+		helper.Fail500(w, r, fmt.Errorf("handleFileUploads: missing destination storage"))
 		return
 	}
 

internal/upload/uploads_test.go

@@ -20,6 +20,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/badgateway"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/objectstore/test"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/proxy"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
 )
@@ -29,9 +30,6 @@ var nilHandler = http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})
 type testFormProcessor struct{}
 
 func (a *testFormProcessor) ProcessFile(ctx context.Context, formName string, file *filestore.FileHandler, writer *multipart.Writer) error {
-	if formName != "file" && file.LocalPath != "my.file" {
-		return errors.New("illegal file")
-	}
 	return nil
 }
 
@@ -233,10 +231,37 @@ func TestUploadProcessingFile(t *testing.T) {
 	}
 	defer os.RemoveAll(tempPath)
 
-	var buffer bytes.Buffer
+	_, testServer := test.StartObjectStore()
+	defer testServer.Close()
+
+	storeUrl := testServer.URL + test.ObjectPath
 
+	tests := []struct {
+		name    string
+		preauth api.Response
+	}{
+		{
+			name:    "FileStore Upload",
+			preauth: api.Response{TempPath: tempPath},
+		},
+		{
+			name:    "ObjectStore Upload",
+			preauth: api.Response{RemoteObject: api.RemoteObject{StoreURL: storeUrl}},
+		},
+		{
+			name: "ObjectStore and FileStore Upload",
+			preauth: api.Response{
+				TempPath:     tempPath,
+				RemoteObject: api.RemoteObject{StoreURL: storeUrl},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			var buffer bytes.Buffer
 			writer := multipart.NewWriter(&buffer)
-	file, err := writer.CreateFormFile("file2", "my.file")
+			file, err := writer.CreateFormFile("file", "my.file")
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -250,8 +275,11 @@ func TestUploadProcessingFile(t *testing.T) {
 			httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
 
 			response := httptest.NewRecorder()
-	HandleFileUploads(response, httpRequest, nilHandler, &api.Response{TempPath: tempPath}, &testFormProcessor{})
-	testhelper.AssertResponseCode(t, response, 500)
+			HandleFileUploads(response, httpRequest, nilHandler, &test.preauth, &testFormProcessor{})
+			testhelper.AssertResponseCode(t, response, 200)
+		})
+	}
+
 }
 
 func TestInvalidFileNames(t *testing.T) {