Merge branch '351657-refactor-uploaders' into 'master'

Refactor upload package

See merge request gitlab-org/gitlab!80281

workhorse/internal/artifacts/artifacts_store_test.go → workhorse/internal/upload/artifacts_store_test.go

-package artifacts
+package upload
 
 import (
 	"archive/zip"

workhorse/internal/artifacts/artifacts_upload_test.go → workhorse/internal/upload/artifacts_upload_test.go

-package artifacts
+package upload
 
 import (
 	"archive/zip"
@@ -16,12 +16,13 @@ import (
 
 	"github.com/golang-jwt/jwt/v4"
 
+	"gitlab.com/gitlab-org/labkit/log"
+
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
 	"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"
-	"gitlab.com/gitlab-org/gitlab/workhorse/internal/upload"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/upstream/roundtripper"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/zipartifacts"
 
@@ -35,6 +36,14 @@ const (
 	Path                  = "/url/path"
 )
 
+func TestMain(m *testing.M) {
+	if err := testhelper.BuildExecutables(); err != nil {
+		log.WithError(err).Fatal()
+	}
+
+	os.Exit(m.Run())
+}
+
 func testArtifactsUploadServer(t *testing.T, authResponse *api.Response, bodyProcessor func(w http.ResponseWriter, r *http.Request)) *httptest.Server {
 	mux := http.NewServeMux()
 	mux.HandleFunc(Path+"/authorize", func(w http.ResponseWriter, r *http.Request) {
@@ -162,7 +171,7 @@ func testUploadArtifacts(t *testing.T, contentType, url string, body io.Reader)
 	testhelper.ConfigureSecret()
 	apiClient := api.NewAPI(parsedURL, "123", roundTripper)
 	proxyClient := proxy.NewProxy(parsedURL, "123", roundTripper)
-	UploadArtifacts(apiClient, proxyClient, &upload.DefaultPreparer{}).ServeHTTP(response, httpRequest)
+	Artifacts(apiClient, proxyClient, &DefaultPreparer{}).ServeHTTP(response, httpRequest)
 	return response
 }
 
@@ -193,10 +202,10 @@ func TestUploadHandlerAddingMetadata(t *testing.T) {
 		t.Run(tc.desc, func(t *testing.T) {
 			s := setupWithTmpPath(t, "file", tc.includeFormat, tc.format, nil,
 				func(w http.ResponseWriter, r *http.Request) {
-					token, err := jwt.ParseWithClaims(r.Header.Get(upload.RewrittenFieldsHeader), &upload.MultipartClaims{}, testhelper.ParseJWT)
+					token, err := jwt.ParseWithClaims(r.Header.Get(RewrittenFieldsHeader), &MultipartClaims{}, testhelper.ParseJWT)
 					require.NoError(t, err)
 
-					rewrittenFields := token.Claims.(*upload.MultipartClaims).RewrittenFields
+					rewrittenFields := token.Claims.(*MultipartClaims).RewrittenFields
 					require.Equal(t, 2, len(rewrittenFields))
 
 					require.Contains(t, rewrittenFields, "file")
@@ -225,10 +234,10 @@ func TestUploadHandlerAddingMetadata(t *testing.T) {
 func TestUploadHandlerTarArtifact(t *testing.T) {
 	s := setupWithTmpPath(t, "file", true, "tar", nil,
 		func(w http.ResponseWriter, r *http.Request) {
-			token, err := jwt.ParseWithClaims(r.Header.Get(upload.RewrittenFieldsHeader), &upload.MultipartClaims{}, testhelper.ParseJWT)
+			token, err := jwt.ParseWithClaims(r.Header.Get(RewrittenFieldsHeader), &MultipartClaims{}, testhelper.ParseJWT)
 			require.NoError(t, err)
 
-			rewrittenFields := token.Claims.(*upload.MultipartClaims).RewrittenFields
+			rewrittenFields := token.Claims.(*MultipartClaims).RewrittenFields
 			require.Equal(t, 1, len(rewrittenFields))
 
 			require.Contains(t, rewrittenFields, "file")

workhorse/internal/artifacts/artifacts_upload.go → workhorse/internal/upload/artifacts_uploader.go

-package artifacts
+package upload
 
 import (
 	"context"
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/filestore"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
-	"gitlab.com/gitlab-org/gitlab/workhorse/internal/upload"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/zipartifacts"
 )
 
@@ -39,7 +38,23 @@ type artifactsUploadProcessor struct {
 	opts   *filestore.SaveFileOpts
 	format string
 
-	upload.SavedFileTracker
+	SavedFileTracker
+}
+
+// Artifacts is like a Multipart but specific for artifacts upload.
+func Artifacts(myAPI *api.API, h http.Handler, p Preparer) http.Handler {
+	return myAPI.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
+		opts, _, err := p.Prepare(a)
+		if err != nil {
+			helper.Fail500(w, r, fmt.Errorf("UploadArtifacts: error preparing file storage options"))
+			return
+		}
+
+		format := r.URL.Query().Get(ArtifactFormatKey)
+
+		mg := &artifactsUploadProcessor{opts: opts, format: format, SavedFileTracker: SavedFileTracker{Request: r}}
+		interceptMultipartFiles(w, r, h, a, mg, opts)
+	}, "/authorize")
 }
 
 func (a *artifactsUploadProcessor) generateMetadataFromZip(ctx context.Context, file *filestore.FileHandler) (*filestore.FileHandler, error) {
@@ -150,18 +165,3 @@ func (a *artifactsUploadProcessor) ProcessFile(ctx context.Context, formName str
 func (a *artifactsUploadProcessor) Name() string {
 	return "artifacts"
 }
-
-func UploadArtifacts(myAPI *api.API, h http.Handler, p upload.Preparer) http.Handler {
-	return myAPI.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
-		opts, _, err := p.Prepare(a)
-		if err != nil {
-			helper.Fail500(w, r, fmt.Errorf("UploadArtifacts: error preparing file storage options"))
-			return
-		}
-
-		format := r.URL.Query().Get(ArtifactFormatKey)
-
-		mg := &artifactsUploadProcessor{opts: opts, format: format, SavedFileTracker: upload.SavedFileTracker{Request: r}}
-		upload.InterceptMultipartFiles(w, r, h, a, mg, opts)
-	}, "/authorize")
-}

workhorse/internal/upload/body_uploader.go

@@ -12,10 +12,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
 )
 
-type PreAuthorizer interface {
-	PreAuthorizeHandler(next api.HandleFunc, suffix string) http.Handler
-}
-
 // RequestBody is a request middleware. It will store the request body to
 // a location by determined an api.Response value. It then forwards the
 // request to gitlab-rails without the original request body.

workhorse/internal/upload/accelerate.go → workhorse/internal/upload/multipart_uploader.go

@@ -4,19 +4,10 @@ import (
 	"fmt"
 	"net/http"
 
-	"github.com/golang-jwt/jwt/v4"
-
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
 )
 
-const RewrittenFieldsHeader = "Gitlab-Workhorse-Multipart-Fields"
-
-type MultipartClaims struct {
-	RewrittenFields map[string]string `json:"rewritten_fields"`
-	jwt.StandardClaims
-}
-
 // Multipart is a request middleware. If the request has a MIME multipart
 // request body, the middleware will iterate through the multipart parts.
 // When it finds a file part (filename != ""), the middleware will save
@@ -32,6 +23,6 @@ func Multipart(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler {
 			return
 		}
 
-		InterceptMultipartFiles(w, r, h, a, s, opts)
+		interceptMultipartFiles(w, r, h, a, s, opts)
 	}, "/authorize")
 }

workhorse/internal/upload/uploads.go

@@ -8,6 +8,8 @@ import (
 	"mime/multipart"
 	"net/http"
 
+	"github.com/golang-jwt/jwt/v4"
+
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/filestore"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
@@ -15,6 +17,17 @@ import (
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/zipartifacts"
 )
 
+const RewrittenFieldsHeader = "Gitlab-Workhorse-Multipart-Fields"
+
+type PreAuthorizer interface {
+	PreAuthorizeHandler(next api.HandleFunc, suffix string) http.Handler
+}
+
+type MultipartClaims struct {
+	RewrittenFields map[string]string `json:"rewritten_fields"`
+	jwt.StandardClaims
+}
+
 // MultipartFormProcessor abstracts away implementation differences
 // between generic MIME multipart file uploads and CI artifact uploads.
 type MultipartFormProcessor interface {
@@ -25,10 +38,9 @@ type MultipartFormProcessor interface {
 	Count() int
 }
 
-// InterceptMultipartFiles is the core of the implementation of
-// Multipart. Because it is also used for CI artifact uploads it is a
-// public function.
-func InterceptMultipartFiles(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *filestore.SaveFileOpts) {
+// interceptMultipartFiles is the core of the implementation of
+// Multipart.
+func interceptMultipartFiles(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *filestore.SaveFileOpts) {
 	var body bytes.Buffer
 	writer := multipart.NewWriter(&body)
 	defer writer.Close()

workhorse/internal/upload/uploads_test.go

@@ -78,7 +78,7 @@ func TestUploadHandlerForwardingRawData(t *testing.T) {
 	opts, _, err := preparer.Prepare(apiResponse)
 	require.NoError(t, err)
 
-	InterceptMultipartFiles(response, httpRequest, handler, apiResponse, nil, opts)
+	interceptMultipartFiles(response, httpRequest, handler, apiResponse, nil, opts)
 
 	require.Equal(t, 202, response.Code)
 	require.Equal(t, "RESPONSE", response.Body.String(), "response body")
@@ -149,7 +149,7 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) {
 	opts, _, err := preparer.Prepare(apiResponse)
 	require.NoError(t, err)
 
-	InterceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
+	interceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
 	require.Equal(t, 202, response.Code)
 
 	cancel() // this will trigger an async cleanup
@@ -218,7 +218,7 @@ func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) {
 			opts, _, err := preparer.Prepare(apiResponse)
 			require.NoError(t, err)
 
-			InterceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
+			interceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
 			require.Equal(t, test.response, response.Code)
 
 			cancel() // this will trigger an async cleanup
@@ -248,7 +248,7 @@ func TestUploadProcessingField(t *testing.T) {
 	opts, _, err := preparer.Prepare(apiResponse)
 	require.NoError(t, err)
 
-	InterceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
+	interceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
 
 	require.Equal(t, 500, response.Code)
 }
@@ -279,7 +279,7 @@ func TestUploadingMultipleFiles(t *testing.T) {
 	opts, _, err := preparer.Prepare(apiResponse)
 	require.NoError(t, err)
 
-	InterceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
+	interceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
 
 	require.Equal(t, 400, response.Code)
 	require.Equal(t, "upload request contains more than 10 files\n", response.Body.String())
@@ -335,7 +335,7 @@ func TestUploadProcessingFile(t *testing.T) {
 			opts, _, err := preparer.Prepare(apiResponse)
 			require.NoError(t, err)
 
-			InterceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
+			interceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts)
 
 			require.Equal(t, 200, response.Code)
 		})
@@ -381,7 +381,7 @@ func TestInvalidFileNames(t *testing.T) {
 		opts, _, err := preparer.Prepare(apiResponse)
 		require.NoError(t, err)
 
-		InterceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
+		interceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
 		require.Equal(t, testCase.code, response.Code)
 		require.Equal(t, testCase.expectedPrefix, opts.TempFilePrefix)
 	}
@@ -447,7 +447,7 @@ func TestContentDispositionRewrite(t *testing.T) {
 			opts, _, err := preparer.Prepare(apiResponse)
 			require.NoError(t, err)
 
-			InterceptMultipartFiles(response, httpRequest, customHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
+			interceptMultipartFiles(response, httpRequest, customHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
 
 			upstreamRequest, err := http.ReadRequest(bufio.NewReader(&upstreamRequestBuffer))
 			require.NoError(t, err)
@@ -570,7 +570,7 @@ func runUploadTest(t *testing.T, image []byte, filename string, httpCode int, ts
 	opts, _, err := preparer.Prepare(apiResponse)
 	require.NoError(t, err)
 
-	InterceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
+	interceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
 	require.Equal(t, httpCode, response.Code)
 }
 

workhorse/internal/upstream/routes.go

@@ -250,8 +250,8 @@ func configureRoutes(u *upstream) {
 		u.route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, upload.RequestBody(api, signingProxy, preparers.lfs), withMatcher(isContentType("application/octet-stream"))),
 
 		// CI Artifacts
-		u.route("POST", apiPattern+`v4/jobs/[0-9]+/artifacts\z`, contentEncodingHandler(artifacts.UploadArtifacts(api, signingProxy, preparers.artifacts))),
-		u.route("POST", ciAPIPattern+`v1/builds/[0-9]+/artifacts\z`, contentEncodingHandler(artifacts.UploadArtifacts(api, signingProxy, preparers.artifacts))),
+		u.route("POST", apiPattern+`v4/jobs/[0-9]+/artifacts\z`, contentEncodingHandler(upload.Artifacts(api, signingProxy, preparers.artifacts))),
+		u.route("POST", ciAPIPattern+`v1/builds/[0-9]+/artifacts\z`, contentEncodingHandler(upload.Artifacts(api, signingProxy, preparers.artifacts))),
 
 		// ActionCable websocket
 		u.wsRoute(`^/-/cable\z`, cableProxy),