Merge branch 'jv-remove-local-plus-remote' into 'master'

Support either local or remote upload but not both NO CHANGELOG

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

internal/artifacts/artifacts_store_test.go

@@ -113,17 +113,6 @@ func TestUploadHandlerSendingToExternalStorage(t *testing.T) {
 				},
 			},
 		},
-		{
-			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",
-				},
-			},
-		},
 	}
 
 	for _, test := range tests {
@@ -197,12 +186,6 @@ func TestUploadHandlerSendingToExternalStorageAndInvalidURLIsUsed(t *testing.T)
 }
 
 func TestUploadHandlerSendingToExternalStorageAndItReturnsAnError(t *testing.T) {
-	tempPath, err := ioutil.TempDir("", "uploads")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(tempPath)
-
 	putCalledTimes := 0
 
 	storeServerMux := http.NewServeMux()
@@ -220,7 +203,6 @@ func TestUploadHandlerSendingToExternalStorageAndItReturnsAnError(t *testing.T)
 	defer storeServer.Close()
 
 	authResponse := api.Response{
-		TempPath: tempPath,
 		RemoteObject: api.RemoteObject{
 			StoreURL: storeServer.URL + "/url/put",
 			ID:       "store-id",
@@ -236,12 +218,6 @@ func TestUploadHandlerSendingToExternalStorageAndItReturnsAnError(t *testing.T)
 }
 
 func TestUploadHandlerSendingToExternalStorageAndSupportRequestTimeout(t *testing.T) {
-	tempPath, err := ioutil.TempDir("", "uploads")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(tempPath)
-
 	putCalledTimes := 0
 
 	storeServerMux := http.NewServeMux()
@@ -260,7 +236,6 @@ func TestUploadHandlerSendingToExternalStorageAndSupportRequestTimeout(t *testin
 	defer storeServer.Close()
 
 	authResponse := api.Response{
-		TempPath: tempPath,
 		RemoteObject: api.RemoteObject{
 			StoreURL: storeServer.URL + "/url/put",
 			ID:       "store-id",

internal/artifacts/artifacts_upload_test.go

@@ -50,7 +50,8 @@ func testArtifactsUploadServer(t *testing.T, authResponse api.Response, bodyProc
 		w.Write(data)
 	})
 	mux.HandleFunc(Path, func(w http.ResponseWriter, r *http.Request) {
-		opts := filestore.GetOpts(&authResponse)
+		opts, err := filestore.GetOpts(&authResponse)
+		require.NoError(t, err)
 
 		if r.Method != "POST" {
 			t.Fatal("Expected POST request")
@@ -66,7 +67,7 @@ func testArtifactsUploadServer(t *testing.T, authResponse api.Response, bodyProc
 				t.Fatal("Expected file to be readable")
 				return
 			}
-		} else if opts.IsRemote() {
+		} else {
 			if r.FormValue("file.remote_url") == "" {
 				t.Fatal("Expected file to be remote accessible")
 				return

internal/filestore/file_handler.go

@@ -118,7 +118,7 @@ func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts
 	}()
 
 	var clientMode string
-	if opts.IsRemote() {
+	if !opts.IsLocal() {
 		switch {
 		case opts.UseWorkhorseClientEnabled() && opts.ObjectStorageConfig.IsGoCloud():
 			clientMode = fmt.Sprintf("go_cloud:%s", opts.ObjectStorageConfig.Provider)
@@ -167,14 +167,9 @@ func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts
 			return nil, err
 		}
 		writers = append(writers, remoteWriter)
-	}
+	} else {
+		clientMode = "local"
 
-	if opts.IsLocal() {
-		if clientMode == "" {
-			clientMode = "local"
-		} else {
-			clientMode += "+local"
-		}
 		fileWriter, err := fh.uploadLocalFile(ctx, opts)
 		if err != nil {
 			return nil, err
@@ -201,7 +196,7 @@ func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts
 		"copied_bytes":     fh.Size,
 		"is_local":         opts.IsLocal(),
 		"is_multipart":     opts.IsMultipart(),
-		"is_remote":        opts.IsRemote(),
+		"is_remote":        !opts.IsLocal(),
 		"remote_id":        opts.RemoteID,
 		"temp_file_prefix": opts.TempFilePrefix,
 		"client_mode":      clientMode,
@@ -209,9 +204,7 @@ func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts
 
 	if opts.IsLocal() {
 		logger = logger.WithField("local_temp_path", opts.LocalTempPath)
-	}
-
-	if opts.IsRemote() {
+	} else {
 		logger = logger.WithField("remote_temp_object", opts.RemoteTempObjectID)
 	}
 
@@ -219,7 +212,7 @@ func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts
 
 	fh.hashes = hashes.finish()
 
-	if opts.IsRemote() {
+	if !opts.IsLocal() {
 		// we need to close the writer in order to get ETag header
 		err = remoteWriter.Close()
 		if err != nil {

internal/filestore/file_handler_test.go

@@ -168,9 +168,7 @@ func TestSaveFile(t *testing.T) {
 	}{
 		{name: "Local only", local: true},
 		{name: "Remote Single only", remote: remoteSingle},
-		{name: "Remote Single and Local", local: true, remote: remoteSingle},
 		{name: "Remote Multipart only", remote: remoteMultipart},
-		{name: "Remote Multipart and Local", local: true, remote: remoteMultipart},
 	}
 
 	for _, spec := range tests {
@@ -217,12 +215,7 @@ func TestSaveFile(t *testing.T) {
 			if spec.local {
 				opts.LocalTempPath = tmpFolder
 				opts.TempFilePrefix = "test-file"
-
-				if expectedClientMode != "" {
-					expectedClientMode += "+local"
-				} else {
-					expectedClientMode = "local"
-				}
+				expectedClientMode = "local"
 			}
 
 			ctx, cancel := context.WithCancel(context.Background())

internal/filestore/save_file_opts.go

 package filestore
 
 import (
+	"errors"
 	"strings"
 	"time"
 
@@ -72,18 +73,13 @@ func (s *SaveFileOpts) IsLocal() bool {
 	return s.LocalTempPath != ""
 }
 
-// IsRemote checks if the options requires a remote upload
-func (s *SaveFileOpts) IsRemote() bool {
-	return s.PresignedPut != "" || s.IsMultipart() || s.UseWorkhorseClient
-}
-
 // IsMultipart checks if the options requires a Multipart upload
 func (s *SaveFileOpts) IsMultipart() bool {
 	return s.PartSize > 0
 }
 
 // GetOpts converts GitLab api.Response to a proper SaveFileOpts
-func GetOpts(apiResponse *api.Response) *SaveFileOpts {
+func GetOpts(apiResponse *api.Response) (*SaveFileOpts, error) {
 	timeout := time.Duration(apiResponse.RemoteObject.Timeout) * time.Second
 	if timeout == 0 {
 		timeout = DefaultObjectStoreTimeout
@@ -101,6 +97,14 @@ func GetOpts(apiResponse *api.Response) *SaveFileOpts {
 		Deadline:           time.Now().Add(timeout),
 	}
 
+	if opts.LocalTempPath != "" && opts.RemoteID != "" {
+		return nil, errors.New("API response has both TempPath and RemoteObject")
+	}
+
+	if opts.LocalTempPath == "" && opts.RemoteID == "" {
+		return nil, errors.New("API response has neither TempPath nor RemoteObject")
+	}
+
 	objectStorageParams := apiResponse.RemoteObject.ObjectStorage
 	if opts.UseWorkhorseClient && objectStorageParams != nil {
 		opts.ObjectStorageConfig.Provider = objectStorageParams.Provider
@@ -122,7 +126,7 @@ func GetOpts(apiResponse *api.Response) *SaveFileOpts {
 		opts.PresignedParts = append([]string(nil), multiParams.PartURLs...)
 	}
 
-	return &opts
+	return &opts, nil
 }
 
 func (c *ObjectStorageConfig) IsAWS() bool {

internal/filestore/save_file_opts_test.go

@@ -28,35 +28,18 @@ func TestSaveFileOptsLocalAndRemote(t *testing.T) {
 			localTempPath: "/tmp",
 			isLocal:       true,
 		},
-		{
-			name:          "Both paths",
-			localTempPath: "/tmp",
-			presignedPut:  "http://example.com",
-			isLocal:       true,
-			isRemote:      true,
-		},
 		{
 			name: "No paths",
 		},
 		{
 			name:         "Only remoteUrl",
 			presignedPut: "http://example.com",
-			isRemote:     true,
 		},
 		{
 			name:        "Multipart",
 			partSize:    10,
-			isRemote:    true,
 			isMultipart: true,
 		},
-		{
-			name:          "Multipart and Local",
-			partSize:      10,
-			localTempPath: "/tmp",
-			isRemote:      true,
-			isMultipart:   true,
-			isLocal:       true,
-		},
 	}
 
 	for _, test := range tests {
@@ -68,7 +51,6 @@ func TestSaveFileOptsLocalAndRemote(t *testing.T) {
 			}
 
 			assert.Equal(t, test.isLocal, opts.IsLocal(), "IsLocal() mismatch")
-			assert.Equal(t, test.isRemote, opts.IsRemote(), "IsRemote() mismatch")
 			assert.Equal(t, test.isMultipart, opts.IsMultipart(), "IsMultipart() mismatch")
 		})
 	}
@@ -112,7 +94,6 @@ func TestGetOpts(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			apiResponse := &api.Response{
-				TempPath: "/tmp",
 				RemoteObject: api.RemoteObject{
 					Timeout:          10,
 					ID:               "id",
@@ -125,7 +106,8 @@ func TestGetOpts(t *testing.T) {
 				},
 			}
 			deadline := time.Now().Add(time.Duration(apiResponse.RemoteObject.Timeout) * time.Second)
-			opts := filestore.GetOpts(apiResponse)
+			opts, err := filestore.GetOpts(apiResponse)
+			require.NoError(t, err)
 
 			assert.Equal(t, apiResponse.TempPath, opts.LocalTempPath)
 			assert.WithinDuration(t, deadline, opts.Deadline, time.Second)
@@ -156,9 +138,33 @@ func TestGetOpts(t *testing.T) {
 	}
 }
 
+func TestGetOptsFail(t *testing.T) {
+	testCases := []struct {
+		desc string
+		in   api.Response
+	}{
+		{
+			desc: "neither local nor remote",
+			in:   api.Response{},
+		},
+		{
+			desc: "both local and remote",
+			in:   api.Response{TempPath: "/foobar", RemoteObject: api.RemoteObject{ID: "id"}},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			_, err := filestore.GetOpts(&tc.in)
+			require.Error(t, err, "expect input to be rejected")
+		})
+	}
+}
+
 func TestGetOptsDefaultTimeout(t *testing.T) {
 	deadline := time.Now().Add(filestore.DefaultObjectStoreTimeout)
-	opts := filestore.GetOpts(&api.Response{})
+	opts, err := filestore.GetOpts(&api.Response{TempPath: "/foo/bar"})
+	require.NoError(t, err)
 
 	assert.WithinDuration(t, deadline, opts.Deadline, time.Minute)
 }
@@ -245,7 +251,6 @@ func TestUseWorkhorseClientEnabled(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			apiResponse := &api.Response{
-				TempPath: "/tmp",
 				RemoteObject: api.RemoteObject{
 					Timeout:            10,
 					ID:                 "id",
@@ -254,7 +259,8 @@ func TestUseWorkhorseClientEnabled(t *testing.T) {
 				},
 			}
 			deadline := time.Now().Add(time.Duration(apiResponse.RemoteObject.Timeout) * time.Second)
-			opts := filestore.GetOpts(apiResponse)
+			opts, err := filestore.GetOpts(apiResponse)
+			require.NoError(t, err)
 			opts.ObjectStorageConfig = test.objectStorageConfig
 
 			require.Equal(t, apiResponse.TempPath, opts.LocalTempPath)
@@ -262,7 +268,6 @@ func TestUseWorkhorseClientEnabled(t *testing.T) {
 			require.Equal(t, apiResponse.RemoteObject.ID, opts.RemoteID)
 			require.Equal(t, apiResponse.RemoteObject.UseWorkhorseClient, opts.UseWorkhorseClient)
 			require.Equal(t, test.expected, opts.UseWorkhorseClientEnabled())
-			require.Equal(t, test.UseWorkhorseClient, opts.IsRemote())
 		})
 	}
 }
@@ -294,7 +299,6 @@ func TestGoCloudConfig(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			apiResponse := &api.Response{
-				TempPath: "/tmp",
 				RemoteObject: api.RemoteObject{
 					Timeout:            10,
 					ID:                 "id",
@@ -309,7 +313,8 @@ func TestGoCloudConfig(t *testing.T) {
 				},
 			}
 			deadline := time.Now().Add(time.Duration(apiResponse.RemoteObject.Timeout) * time.Second)
-			opts := filestore.GetOpts(apiResponse)
+			opts, err := filestore.GetOpts(apiResponse)
+			require.NoError(t, err)
 			opts.ObjectStorageConfig.URLMux = mux
 
 			require.Equal(t, apiResponse.TempPath, opts.LocalTempPath)
@@ -321,7 +326,7 @@ func TestGoCloudConfig(t *testing.T) {
 			require.Equal(t, apiResponse.RemoteObject.ObjectStorage.GoCloudConfig, opts.ObjectStorageConfig.GoCloudConfig)
 			require.True(t, opts.UseWorkhorseClientEnabled())
 			require.Equal(t, test.valid, opts.ObjectStorageConfig.IsValid())
-			require.True(t, opts.IsRemote())
+			require.False(t, opts.IsLocal())
 		})
 	}
 }

internal/lfs/lfs.go

@@ -40,7 +40,11 @@ func NewLfsUploadPreparer(c config.Config, objectPreparer upload.Preparer) uploa
 }
 
 func (l *uploadPreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, upload.Verifier, error) {
-	opts, _, _ := l.objectPreparer.Prepare(a)
+	opts, _, err := l.objectPreparer.Prepare(a)
+	if err != nil {
+		return nil, nil, err
+	}
+
 	opts.TempFilePrefix = a.LfsOid
 
 	return opts, &object{oid: a.LfsOid, size: a.LfsSize}, nil

internal/lfs/lfs_test.go

@@ -28,6 +28,7 @@ func TestLfsUploadPreparerWithConfig(t *testing.T) {
 	r := &api.Response{
 		LfsOid: lfsOid,
 		RemoteObject: api.RemoteObject{
+			ID:                 "the upload ID",
 			UseWorkhorseClient: true,
 			ObjectStorage: &api.ObjectStorageParams{
 				Provider: "AWS",
@@ -49,7 +50,7 @@ func TestLfsUploadPreparerWithConfig(t *testing.T) {
 
 func TestLfsUploadPreparerWithNoConfig(t *testing.T) {
 	c := config.Config{}
-	r := &api.Response{}
+	r := &api.Response{RemoteObject: api.RemoteObject{ID: "the upload ID"}}
 	uploadPreparer := upload.NewObjectStoragePreparer(c)
 	lfsPreparer := lfs.NewLfsUploadPreparer(c, uploadPreparer)
 	opts, verifier, err := lfsPreparer.Prepare(r)

internal/upload/body_uploader.go

@@ -32,7 +32,8 @@ type Preparer interface {
 type DefaultPreparer struct{}
 
 func (s *DefaultPreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, Verifier, error) {
-	return filestore.GetOpts(a), nil, nil
+	opts, err := filestore.GetOpts(a)
+	return opts, nil, err
 }
 
 // BodyUploader is an http.Handler that perform a pre authorization call to rails before hijacking the request body and

internal/upload/body_uploader_test.go

@@ -170,7 +170,12 @@ type alwaysLocalPreparer struct {
 }
 
 func (a *alwaysLocalPreparer) Prepare(_ *api.Response) (*filestore.SaveFileOpts, Verifier, error) {
-	return filestore.GetOpts(&api.Response{TempPath: os.TempDir()}), a.verifier, a.prepareError
+	opts, err := filestore.GetOpts(&api.Response{TempPath: os.TempDir()})
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return opts, a.verifier, a.prepareError
 }
 
 type alwaysFailsVerifier struct{}

internal/upload/object_storage_preparer.go

@@ -22,7 +22,11 @@ func NewObjectStoragePreparer(c config.Config) Preparer {
 }
 
 func (p *ObjectStoragePreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, Verifier, error) {
-	opts := filestore.GetOpts(a)
+	opts, err := filestore.GetOpts(a)
+	if err != nil {
+		return nil, nil, err
+	}
+
 	opts.ObjectStorageConfig.URLMux = p.config.URLMux
 	opts.ObjectStorageConfig.S3Credentials = p.credentials.S3Credentials
 

internal/upload/object_storage_preparer_test.go

@@ -30,6 +30,7 @@ func TestPrepareWithS3Config(t *testing.T) {
 
 	r := &api.Response{
 		RemoteObject: api.RemoteObject{
+			ID:                 "the ID",
 			UseWorkhorseClient: true,
 			ObjectStorage: &api.ObjectStorageParams{
 				Provider: "AWS",
@@ -50,7 +51,7 @@ func TestPrepareWithS3Config(t *testing.T) {
 
 func TestPrepareWithNoConfig(t *testing.T) {
 	c := config.Config{}
-	r := &api.Response{}
+	r := &api.Response{RemoteObject: api.RemoteObject{ID: "id"}}
 	p := upload.NewObjectStoragePreparer(c)
 	opts, v, err := p.Prepare(r)
 

internal/upload/uploads.go

@@ -24,11 +24,6 @@ type MultipartFormProcessor interface {
 }
 
 func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *filestore.SaveFileOpts) {
-	if !opts.IsLocal() && !opts.IsRemote() {
-		helper.Fail500(w, r, fmt.Errorf("handleFileUploads: missing destination storage"))
-		return
-	}
-
 	var body bytes.Buffer
 	writer := multipart.NewWriter(&body)
 	defer writer.Close()

internal/upload/uploads_test.go

@@ -50,16 +50,10 @@ func (a *testFormProcessor) Name() string {
 }
 
 func TestUploadTempPathRequirement(t *testing.T) {
-	response := httptest.NewRecorder()
-	request, err := http.NewRequest("", "", nil)
-	require.NoError(t, err)
 	apiResponse := &api.Response{}
 	preparer := &DefaultPreparer{}
-	opts, _, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-
-	HandleFileUploads(response, request, nilHandler, apiResponse, &testFormProcessor{}, opts)
-	require.Equal(t, 500, response.Code)
+	_, _, err := preparer.Prepare(apiResponse)
+	require.Error(t, err)
 }
 
 func TestUploadHandlerForwardingRawData(t *testing.T) {