diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-11-18 13:16:36 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-11-18 13:16:36 +0000 |
commit | 311b0269b4eb9839fa63f80c8d7a58f32b8138a0 (patch) | |
tree | 07e7870bca8aed6d61fdcc810731c50d2c40af47 /workhorse | |
parent | 27909cef6c4170ed9205afa7426b8d3de47cbb0c (diff) | |
download | gitlab-ce-311b0269b4eb9839fa63f80c8d7a58f32b8138a0.tar.gz |
Add latest changes from gitlab-org/gitlab@14-5-stable-eev14.5.0-rc42
Diffstat (limited to 'workhorse')
19 files changed, 239 insertions, 166 deletions
diff --git a/workhorse/.tool-versions b/workhorse/.tool-versions new file mode 100644 index 00000000000..9d3b3b30ff2 --- /dev/null +++ b/workhorse/.tool-versions @@ -0,0 +1 @@ +golang 1.16.9 diff --git a/workhorse/Makefile b/workhorse/Makefile index 0e8c47ae35c..3cf592b0cff 100644 --- a/workhorse/Makefile +++ b/workhorse/Makefile @@ -13,6 +13,8 @@ else BUILD_TIME := $(shell date -u "$(DATE_FMT)") endif GOBUILD := go build -ldflags "-X main.Version=$(VERSION_STRING) -X main.BuildTime=$(BUILD_TIME)" +GITALY := tmp/tests/gitaly/_build/bin/gitaly +GITALY_PID_FILE := gitaly.pid EXE_ALL := gitlab-resize-image gitlab-zip-cat gitlab-zip-metadata gitlab-workhorse INSTALL := install BUILD_TAGS := tracer_static tracer_static_jaeger continuous_profiler_stackdriver @@ -63,7 +65,16 @@ install: $(EXE_ALL) .PHONY: test test: prepare-tests $(call message,$@) - @go test -tags "$(BUILD_TAGS)" ./... + go test -tags "$(BUILD_TAGS)" ./... ;\ + status="$$?" ;\ + if [ -f "$(GITALY_PID_FILE)" ] ; then \ + echo "Clean up Gitaly server for workhorse integration test" ;\ + kill -9 $$(cat $(GITALY_PID_FILE)) ;\ + rm $(GITALY_PID_FILE) ;\ + else \ + echo "Gitaly integration test not running" ;\ + fi ;\ + exit "$$status" @echo SUCCESS .PHONY: clean @@ -82,9 +93,27 @@ clean-build: rm -rf $(TARGET_DIR) .PHONY: prepare-tests +prepare-tests: run-gitaly prepare-tests: testdata/data/group/test.git $(EXE_ALL) prepare-tests: testdata/scratch +.PHONY: run-gitaly +run-gitaly: gitaly.pid + +$(GITALY_PID_FILE): gitaly.toml + @{ \ + if [ -z "$${GITALY_ADDRESS+x}" ] ; then \ + echo "To run gitaly integration tests set GITALY_ADDRESS=tcp://127.0.0.1:8075" ; \ + else \ + cd .. ; \ + GITALY_TESTING_NO_GIT_HOOKS=1 GITALY_PID_FILE=workhorse/$(GITALY_PID_FILE) $(GITALY) workhorse/gitaly.toml ; \ + fi \ + } & + +gitaly.toml: ../tmp/tests/gitaly/config.toml + sed -e 's/^socket_path.*$$/listen_addr = "0.0.0.0:8075"/;s/^\[auth\]$$//;s/^token.*$$//;s/^internal_socket_dir.*$$//' \ + $< > $@ + testdata/data/group/test.git: $(call message,$@) git clone --quiet --bare https://gitlab.com/gitlab-org/gitlab-test.git $@ diff --git a/workhorse/gitaly_integration_test.go b/workhorse/gitaly_integration_test.go index 48de45d7935..95e0a03ab6b 100644 --- a/workhorse/gitaly_integration_test.go +++ b/workhorse/gitaly_integration_test.go @@ -16,6 +16,7 @@ import ( "strings" "testing" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" @@ -53,6 +54,10 @@ func realGitalyOkBody(t *testing.T) *api.Response { return realGitalyAuthResponse(gitOkBody(t)) } +func realGitalyOkBodyWithSidechannel(t *testing.T) *api.Response { + return realGitalyAuthResponse(gitOkBodyWithSidechannel(t)) +} + func ensureGitalyRepository(t *testing.T, apiResponse *api.Response) error { ctx, namespace, err := gitaly.NewNamespaceClient(context.Background(), apiResponse.GitalyServer) if err != nil { @@ -83,10 +88,18 @@ func ensureGitalyRepository(t *testing.T, apiResponse *api.Response) error { } func TestAllowedClone(t *testing.T) { + testAllowedClone(t, realGitalyOkBody(t)) +} + +func TestAllowedCloneWithSidechannel(t *testing.T) { + gitaly.InitializeSidechannelRegistry(logrus.StandardLogger()) + testAllowedClone(t, realGitalyOkBodyWithSidechannel(t)) +} + +func testAllowedClone(t *testing.T, apiResponse *api.Response) { skipUnlessRealGitaly(t) // Create the repository in the Gitaly server - apiResponse := realGitalyOkBody(t) require.NoError(t, ensureGitalyRepository(t, apiResponse)) // Prepare test server and backend @@ -107,10 +120,18 @@ func TestAllowedClone(t *testing.T) { } func TestAllowedShallowClone(t *testing.T) { + testAllowedShallowClone(t, realGitalyOkBody(t)) +} + +func TestAllowedShallowCloneWithSidechannel(t *testing.T) { + gitaly.InitializeSidechannelRegistry(logrus.StandardLogger()) + testAllowedShallowClone(t, realGitalyOkBodyWithSidechannel(t)) +} + +func testAllowedShallowClone(t *testing.T, apiResponse *api.Response) { skipUnlessRealGitaly(t) // Create the repository in the Gitaly server - apiResponse := realGitalyOkBody(t) require.NoError(t, ensureGitalyRepository(t, apiResponse)) // Prepare test server and backend diff --git a/workhorse/internal/dependencyproxy/dependencyproxy.go b/workhorse/internal/dependencyproxy/dependencyproxy.go index cfb3045544f..90f3042a342 100644 --- a/workhorse/internal/dependencyproxy/dependencyproxy.go +++ b/workhorse/internal/dependencyproxy/dependencyproxy.go @@ -4,37 +4,17 @@ import ( "context" "fmt" "io" - "net" "net/http" - "time" - "gitlab.com/gitlab-org/labkit/correlation" "gitlab.com/gitlab-org/labkit/log" - "gitlab.com/gitlab-org/labkit/tracing" "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport" "gitlab.com/gitlab-org/gitlab/workhorse/internal/senddata" ) -// httpTransport defines a http.Transport with values -// that are more restrictive than for http.DefaultTransport, -// they define shorter TLS Handshake, and more aggressive connection closing -// to prevent the connection hanging and reduce FD usage -var httpTransport = tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 10 * time.Second, - }).DialContext, - MaxIdleConns: 2, - IdleConnTimeout: 30 * time.Second, - TLSHandshakeTimeout: 10 * time.Second, - ExpectContinueTimeout: 10 * time.Second, - ResponseHeaderTimeout: 30 * time.Second, -})) - var httpClient = &http.Client{ - Transport: httpTransport, + Transport: httptransport.New(), } type Injector struct { @@ -87,15 +67,28 @@ func (p *Injector) Inject(w http.ResponseWriter, r *http.Request, sendData strin return } + w.Header().Set("Content-Length", dependencyResponse.Header.Get("Content-Length")) + teeReader := io.TeeReader(dependencyResponse.Body, w) saveFileRequest, err := http.NewRequestWithContext(r.Context(), "POST", r.URL.String()+"/upload", teeReader) if err != nil { helper.Fail500(w, r, fmt.Errorf("dependency proxy: failed to create request: %w", err)) } saveFileRequest.Header = helper.HeaderClone(r.Header) - saveFileRequest.ContentLength = dependencyResponse.ContentLength - w.Header().Del("Content-Length") + // forward headers from dependencyResponse to rails and client + for key, values := range dependencyResponse.Header { + saveFileRequest.Header.Del(key) + w.Header().Del(key) + for _, value := range values { + saveFileRequest.Header.Add(key, value) + w.Header().Add(key, value) + } + } + + // workhorse hijack overwrites the Content-Type header, but we need this header value + saveFileRequest.Header.Set("Workhorse-Proxy-Content-Type", dependencyResponse.Header.Get("Content-Type")) + saveFileRequest.ContentLength = dependencyResponse.ContentLength nrw := &nullResponseWriter{header: make(http.Header)} p.uploadHandler.ServeHTTP(nrw, saveFileRequest) diff --git a/workhorse/internal/dependencyproxy/dependencyproxy_test.go b/workhorse/internal/dependencyproxy/dependencyproxy_test.go index 37e54c0b756..d9169b2b4ce 100644 --- a/workhorse/internal/dependencyproxy/dependencyproxy_test.go +++ b/workhorse/internal/dependencyproxy/dependencyproxy_test.go @@ -33,7 +33,7 @@ func (f *fakeUploadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { type errWriter struct{ writes int } -func (w *errWriter) Header() http.Header { return nil } +func (w *errWriter) Header() http.Header { return make(http.Header) } func (w *errWriter) WriteHeader(h int) {} // First call of Write function succeeds while all the subsequent ones fail @@ -112,8 +112,15 @@ func TestInject(t *testing.T) { func TestSuccessfullRequest(t *testing.T) { content := []byte("result") + contentLength := strconv.Itoa(len(content)) + contentType := "foo" + dockerContentDigest := "sha256:asdf1234" + overriddenHeader := "originResourceServer" originResourceServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Length", strconv.Itoa(len(content))) + w.Header().Set("Content-Length", contentLength) + w.Header().Set("Content-Type", contentType) + w.Header().Set("Docker-Content-Digest", dockerContentDigest) + w.Header().Set("Overridden-Header", overriddenHeader) w.Write(content) })) @@ -130,11 +137,16 @@ func TestSuccessfullRequest(t *testing.T) { require.Equal(t, "/target/upload", uploadHandler.request.URL.Path) require.Equal(t, int64(6), uploadHandler.request.ContentLength) + require.Equal(t, contentType, uploadHandler.request.Header.Get("Workhorse-Proxy-Content-Type")) + require.Equal(t, dockerContentDigest, uploadHandler.request.Header.Get("Docker-Content-Digest")) + require.Equal(t, overriddenHeader, uploadHandler.request.Header.Get("Overridden-Header")) require.Equal(t, content, uploadHandler.body) require.Equal(t, 200, response.Code) require.Equal(t, string(content), response.Body.String()) + require.Equal(t, contentLength, response.Header().Get("Content-Length")) + require.Equal(t, dockerContentDigest, response.Header().Get("Docker-Content-Digest")) } func TestIncorrectSendData(t *testing.T) { @@ -175,6 +187,7 @@ func TestFailedOriginServer(t *testing.T) { func makeRequest(injector *Injector, data string) *httptest.ResponseRecorder { w := httptest.NewRecorder() r := httptest.NewRequest("GET", "/target", nil) + r.Header.Set("Overridden-Header", "request") sendData := base64.StdEncoding.EncodeToString([]byte(data)) injector.Inject(w, r, sendData) diff --git a/workhorse/internal/filestore/file_handler.go b/workhorse/internal/filestore/file_handler.go index b4d7250fe0c..dac8d4d6247 100644 --- a/workhorse/internal/filestore/file_handler.go +++ b/workhorse/internal/filestore/file_handler.go @@ -43,6 +43,9 @@ type FileHandler struct { // a map containing different hashes hashes map[string]string + + // Duration of upload in seconds + uploadDuration float64 } type uploadClaims struct { @@ -74,11 +77,12 @@ func (fh *FileHandler) GitLabFinalizeFields(prefix string) (map[string]string, e } for k, v := range map[string]string{ - "name": fh.Name, - "path": fh.LocalPath, - "remote_url": fh.RemoteURL, - "remote_id": fh.RemoteID, - "size": strconv.FormatInt(fh.Size, 10), + "name": fh.Name, + "path": fh.LocalPath, + "remote_url": fh.RemoteURL, + "remote_id": fh.RemoteID, + "size": strconv.FormatInt(fh.Size, 10), + "upload_duration": strconv.FormatFloat(fh.uploadDuration, 'f', -1, 64), } { data[key(k)] = v signedData[k] = v @@ -105,18 +109,20 @@ type consumer interface { // SaveFileFromReader persists the provided reader content to all the location specified in opts. A cleanup will be performed once ctx is Done // Make sure the provided context will not expire before finalizing upload with GitLab Rails. -func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts *SaveFileOpts) (fh *FileHandler, err error) { - var uploadDestination consumer - fh = &FileHandler{ +func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts *SaveFileOpts) (*FileHandler, error) { + fh := &FileHandler{ Name: opts.TempFilePrefix, RemoteID: opts.RemoteID, RemoteURL: opts.RemoteURL, } + uploadStartTime := time.Now() + defer func() { fh.uploadDuration = time.Since(uploadStartTime).Seconds() }() hashes := newMultiHash() reader = io.TeeReader(reader, hashes.Writer) var clientMode string - + var uploadDestination consumer + var err error switch { case opts.IsLocal(): clientMode = "local" @@ -161,23 +167,19 @@ func SaveFileFromReader(ctx context.Context, reader io.Reader, size int64, opts return nil, err } + var hlr *hardLimitReader if opts.MaximumSize > 0 { if size > opts.MaximumSize { return nil, SizeError(fmt.Errorf("the upload size %d is over maximum of %d bytes", size, opts.MaximumSize)) } - hlr := &hardLimitReader{r: reader, n: opts.MaximumSize} + hlr = &hardLimitReader{r: reader, n: opts.MaximumSize} reader = hlr - defer func() { - if hlr.n < 0 { - err = ErrEntityTooLarge - } - }() } fh.Size, err = uploadDestination.Consume(ctx, reader, opts.Deadline) if err != nil { - if err == objectstore.ErrNotEnoughParts { + if (err == objectstore.ErrNotEnoughParts) || (hlr != nil && hlr.n < 0) { err = ErrEntityTooLarge } return nil, err diff --git a/workhorse/internal/filestore/file_handler_test.go b/workhorse/internal/filestore/file_handler_test.go index 16af56dcf48..f57026a59df 100644 --- a/workhorse/internal/filestore/file_handler_test.go +++ b/workhorse/internal/filestore/file_handler_test.go @@ -548,4 +548,5 @@ func checkFileHandlerWithFields(t *testing.T, fh *filestore.FileHandler, fields require.Equal(t, test.ObjectSHA1, fields[key("sha1")]) require.Equal(t, test.ObjectSHA256, fields[key("sha256")]) require.Equal(t, test.ObjectSHA512, fields[key("sha512")]) + require.NotEmpty(t, fields[key("upload_duration")]) } diff --git a/workhorse/internal/helper/httptransport/http_transport.go b/workhorse/internal/helper/httptransport/http_transport.go new file mode 100644 index 00000000000..c7c3c5283f5 --- /dev/null +++ b/workhorse/internal/helper/httptransport/http_transport.go @@ -0,0 +1,37 @@ +package httptransport + +import ( + "net/http" + "time" + + "gitlab.com/gitlab-org/labkit/correlation" + "gitlab.com/gitlab-org/labkit/tracing" +) + +type Option func(*http.Transport) + +// Defines a http.Transport with values +// that are more restrictive than for http.DefaultTransport, +// they define shorter TLS Handshake, and more aggressive connection closing +// to prevent the connection hanging and reduce FD usage +func New(options ...Option) http.RoundTripper { + t := http.DefaultTransport.(*http.Transport).Clone() + + // To avoid keep around TCP connections to http servers we're done with + t.MaxIdleConns = 2 + + // A stricter timeout for fetching from external sources that can be slow + t.ResponseHeaderTimeout = 30 * time.Second + + for _, option := range options { + option(t) + } + + return tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(t)) +} + +func WithDisabledCompression() Option { + return func(t *http.Transport) { + t.DisableCompression = true + } +} diff --git a/workhorse/internal/imageresizer/image_resizer.go b/workhorse/internal/imageresizer/image_resizer.go index cd0fa946530..8c3271b6f11 100644 --- a/workhorse/internal/imageresizer/image_resizer.go +++ b/workhorse/internal/imageresizer/image_resizer.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "io" - "net" "net/http" "os" "os/exec" @@ -18,11 +17,11 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" - "gitlab.com/gitlab-org/labkit/correlation" "gitlab.com/gitlab-org/labkit/tracing" "gitlab.com/gitlab-org/gitlab/workhorse/internal/config" "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport" "gitlab.com/gitlab-org/gitlab/workhorse/internal/log" "gitlab.com/gitlab-org/gitlab/workhorse/internal/senddata" ) @@ -69,23 +68,8 @@ const ( var envInjector = tracing.NewEnvInjector() -// Images might be located remotely in object storage, in which case we need to stream -// it via http(s) -var httpTransport = tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 10 * time.Second, - }).DialContext, - MaxIdleConns: 2, - IdleConnTimeout: 30 * time.Second, - TLSHandshakeTimeout: 10 * time.Second, - ExpectContinueTimeout: 10 * time.Second, - ResponseHeaderTimeout: 30 * time.Second, -})) - var httpClient = &http.Client{ - Transport: httpTransport, + Transport: httptransport.New(), } const ( diff --git a/workhorse/internal/objectstore/object.go b/workhorse/internal/objectstore/object.go index eaf3bfb2e36..b7c4f12f009 100644 --- a/workhorse/internal/objectstore/object.go +++ b/workhorse/internal/objectstore/object.go @@ -5,34 +5,15 @@ import ( "fmt" "io" "io/ioutil" - "net" "net/http" - "time" - "gitlab.com/gitlab-org/labkit/correlation" "gitlab.com/gitlab-org/labkit/mask" - "gitlab.com/gitlab-org/labkit/tracing" -) -// httpTransport defines a http.Transport with values -// that are more restrictive than for http.DefaultTransport, -// they define shorter TLS Handshake, and more aggressive connection closing -// to prevent the connection hanging and reduce FD usage -var httpTransport = tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 10 * time.Second, - }).DialContext, - MaxIdleConns: 2, - IdleConnTimeout: 30 * time.Second, - TLSHandshakeTimeout: 10 * time.Second, - ExpectContinueTimeout: 10 * time.Second, - ResponseHeaderTimeout: 30 * time.Second, -})) + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport" +) var httpClient = &http.Client{ - Transport: httpTransport, + Transport: httptransport.New(), } // Object represents an object on a S3 compatible Object Store service. diff --git a/workhorse/internal/sendurl/sendurl.go b/workhorse/internal/sendurl/sendurl.go index ac2e66f95ab..205ec8a0e9f 100644 --- a/workhorse/internal/sendurl/sendurl.go +++ b/workhorse/internal/sendurl/sendurl.go @@ -3,18 +3,15 @@ package sendurl import ( "fmt" "io" - "net" "net/http" - "time" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" - "gitlab.com/gitlab-org/labkit/correlation" "gitlab.com/gitlab-org/labkit/mask" - "gitlab.com/gitlab-org/labkit/tracing" "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport" "gitlab.com/gitlab-org/gitlab/workhorse/internal/log" "gitlab.com/gitlab-org/gitlab/workhorse/internal/senddata" ) @@ -47,22 +44,7 @@ var preserveHeaderKeys = map[string]bool{ "Pragma": true, // Support for HTTP 1.0 proxies } -// httpTransport defines a http.Transport with values -// that are more restrictive than for http.DefaultTransport, -// they define shorter TLS Handshake, and more aggressive connection closing -// to prevent the connection hanging and reduce FD usage -var httpTransport = tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 10 * time.Second, - }).DialContext, - MaxIdleConns: 2, - IdleConnTimeout: 30 * time.Second, - TLSHandshakeTimeout: 10 * time.Second, - ExpectContinueTimeout: 10 * time.Second, - ResponseHeaderTimeout: 30 * time.Second, -})) +var httpTransport = httptransport.New() var httpClient = &http.Client{ Transport: httpTransport, diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go index e82dcdcbc69..b5db8003d41 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -114,7 +114,7 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) { require.Equal(t, hash, r.FormValue("file."+algo), "file hash %s", algo) } - require.Len(t, r.MultipartForm.Value, 11, "multipart form values") + require.Len(t, r.MultipartForm.Value, 12, "multipart form values") w.WriteHeader(202) fmt.Fprint(w, "RESPONSE") diff --git a/workhorse/internal/upstream/roundtripper/roundtripper.go b/workhorse/internal/upstream/roundtripper/roundtripper.go index fdbca5c0120..fcba50d7975 100644 --- a/workhorse/internal/upstream/roundtripper/roundtripper.go +++ b/workhorse/internal/upstream/roundtripper/roundtripper.go @@ -32,19 +32,23 @@ func NewBackendRoundTripper(backend *url.URL, socket string, proxyHeadersTimeout } func newBackendRoundTripper(backend *url.URL, socket string, proxyHeadersTimeout time.Duration, developmentMode bool, tlsConf *tls.Config) http.RoundTripper { - // Copied from the definition of http.DefaultTransport. We can't literally copy http.DefaultTransport because of its hidden internal state. - transport, dialer := newBackendTransport() + transport := http.DefaultTransport.(*http.Transport).Clone() transport.ResponseHeaderTimeout = proxyHeadersTimeout transport.TLSClientConfig = tlsConf + // Puma does not support http/2, there's no point in reconnecting + transport.ForceAttemptHTTP2 = false + + dial := transport.DialContext + if backend != nil && socket == "" { address := mustParseAddress(backend.Host, backend.Scheme) transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { - return dialer.DialContext(ctx, "tcp", address) + return dial(ctx, "tcp", address) } } else if socket != "" { transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) { - return dialer.DialContext(ctx, "unix", socket) + return dial(ctx, "unix", socket) } } else { panic("backend is nil and socket is empty") diff --git a/workhorse/internal/upstream/roundtripper/transport.go b/workhorse/internal/upstream/roundtripper/transport.go deleted file mode 100644 index 84d9623b129..00000000000 --- a/workhorse/internal/upstream/roundtripper/transport.go +++ /dev/null @@ -1,27 +0,0 @@ -package roundtripper - -import ( - "net" - "net/http" - "time" -) - -// newBackendTransport setups the default HTTP transport which Workhorse uses -// to communicate with the upstream -func newBackendTransport() (*http.Transport, *net.Dialer) { - dialler := &net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 30 * time.Second, - } - - transport := &http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: dialler.DialContext, - MaxIdleConns: 100, - IdleConnTimeout: 90 * time.Second, - TLSHandshakeTimeout: 10 * time.Second, - ExpectContinueTimeout: 1 * time.Second, - } - - return transport, dialler -} diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go index d39ba845dc5..22b30fe8a63 100644 --- a/workhorse/internal/upstream/routes.go +++ b/workhorse/internal/upstream/routes.go @@ -60,6 +60,7 @@ const ( geoGitProjectPattern = `^/[^-].+\.git/` // Prevent matching routes like /-/push_from_secondary projectPattern = `^/([^/]+/){1,}[^/]+/` apiProjectPattern = apiPattern + `v4/projects/[^/]+/` // API: Projects can be encoded via group%2Fsubgroup%2Fproject + apiTopicPattern = apiPattern + `v4/topics` snippetUploadPattern = `^/uploads/personal_snippet` userUploadPattern = `^/uploads/user` importPattern = `^/import/` @@ -295,6 +296,8 @@ func configureRoutes(u *upstream) { // Overall status can be seen at https://gitlab.com/groups/gitlab-org/-/epics/1802#current-status u.route("POST", apiProjectPattern+`wikis/attachments\z`, uploadAccelerateProxy), u.route("POST", apiPattern+`graphql\z`, uploadAccelerateProxy), + u.route("POST", apiTopicPattern, uploadAccelerateProxy), + u.route("PUT", apiTopicPattern, uploadAccelerateProxy), u.route("POST", apiPattern+`v4/groups/import`, upload.Accelerate(api, signingProxy, preparers.uploads)), u.route("POST", apiPattern+`v4/projects/import`, upload.Accelerate(api, signingProxy, preparers.uploads)), @@ -374,6 +377,10 @@ func configureRoutes(u *upstream) { // Geo API routes u.route("", "^/api/v4/geo_nodes", defaultUpstream), u.route("", "^/api/v4/geo_replication", defaultUpstream), + u.route("", "^/api/v4/geo/proxy_git_ssh", defaultUpstream), + + // Internal API routes + u.route("", "^/api/v4/internal", defaultUpstream), // Don't define a catch-all route. If a route does not match, then we know // the request should be proxied. diff --git a/workhorse/internal/upstream/routes_test.go b/workhorse/internal/upstream/routes_test.go new file mode 100644 index 00000000000..f196433f5b4 --- /dev/null +++ b/workhorse/internal/upstream/routes_test.go @@ -0,0 +1,47 @@ +package upstream + +import ( + "testing" +) + +func TestProjectNotExistingGitHttpPullWithGeoProxy(t *testing.T) { + testCases := []testCase{ + {"secondary info/refs", "/group/project.git/info/refs", "Local Rails server received request to path /group/project.git/info/refs"}, + {"primary info/refs", "/-/push_from_secondary/2/group/project.git/info/refs", "Geo primary received request to path /-/push_from_secondary/2/group/project.git/info/refs"}, + {"primary upload-pack", "/-/push_from_secondary/2/group/project.git/git-upload-pack", "Geo primary received request to path /-/push_from_secondary/2/group/project.git/git-upload-pack"}, + } + + runTestCasesWithGeoProxyEnabled(t, testCases) +} + +func TestProjectNotExistingGitHttpPushWithGeoProxy(t *testing.T) { + testCases := []testCase{ + {"secondary info/refs", "/group/project.git/info/refs", "Local Rails server received request to path /group/project.git/info/refs"}, + {"primary info/refs", "/-/push_from_secondary/2/group/project.git/info/refs", "Geo primary received request to path /-/push_from_secondary/2/group/project.git/info/refs"}, + {"primary receive-pack", "/-/push_from_secondary/2/group/project.git/git-receive-pack", "Geo primary received request to path /-/push_from_secondary/2/group/project.git/git-receive-pack"}, + } + + runTestCasesWithGeoProxyEnabled(t, testCases) +} + +func TestProjectNotExistingGitSSHPullWithGeoProxy(t *testing.T) { + testCases := []testCase{ + {"GitLab Shell call to authorized-keys", "/api/v4/internal/authorized_keys", "Local Rails server received request to path /api/v4/internal/authorized_keys"}, + {"GitLab Shell call to allowed", "/api/v4/internal/allowed", "Local Rails server received request to path /api/v4/internal/allowed"}, + {"GitLab Shell call to info/refs", "/api/v4/geo/proxy_git_ssh/info_refs_receive_pack", "Local Rails server received request to path /api/v4/geo/proxy_git_ssh/info_refs_receive_pack"}, + {"GitLab Shell call to receive_pack", "/api/v4/geo/proxy_git_ssh/receive_pack", "Local Rails server received request to path /api/v4/geo/proxy_git_ssh/receive_pack"}, + } + + runTestCasesWithGeoProxyEnabled(t, testCases) +} + +func TestProjectNotExistingGitSSHPushWithGeoProxy(t *testing.T) { + testCases := []testCase{ + {"GitLab Shell call to authorized-keys", "/api/v4/internal/authorized_keys", "Local Rails server received request to path /api/v4/internal/authorized_keys"}, + {"GitLab Shell call to allowed", "/api/v4/internal/allowed", "Local Rails server received request to path /api/v4/internal/allowed"}, + {"GitLab Shell call to info/refs", "/api/v4/geo/proxy_git_ssh/info_refs_upload_pack", "Local Rails server received request to path /api/v4/geo/proxy_git_ssh/info_refs_upload_pack"}, + {"GitLab Shell call to receive_pack", "/api/v4/geo/proxy_git_ssh/upload_pack", "Local Rails server received request to path /api/v4/geo/proxy_git_ssh/upload_pack"}, + } + + runTestCasesWithGeoProxyEnabled(t, testCases) +} diff --git a/workhorse/internal/upstream/upstream_test.go b/workhorse/internal/upstream/upstream_test.go index 3c942767384..53c15bb7e91 100644 --- a/workhorse/internal/upstream/upstream_test.go +++ b/workhorse/internal/upstream/upstream_test.go @@ -88,16 +88,6 @@ func TestGeoProxyFeatureDisabledOnGeoSecondarySite(t *testing.T) { } func TestGeoProxyFeatureEnabledOnGeoSecondarySite(t *testing.T) { - remoteServer, rsDeferredClose := startRemoteServer("Geo primary") - defer rsDeferredClose() - - geoProxyEndpointResponseBody := fmt.Sprintf(`{"geo_proxy_url":"%v"}`, remoteServer.URL) - railsServer, deferredClose := startRailsServer("Local Rails server", &geoProxyEndpointResponseBody) - defer deferredClose() - - ws, wsDeferredClose, _ := startWorkhorseServer(railsServer.URL, true) - defer wsDeferredClose() - testCases := []testCase{ {"push from secondary is forwarded", "/-/push_from_secondary/foo/bar.git/info/refs", "Geo primary received request to path /-/push_from_secondary/foo/bar.git/info/refs"}, {"LFS files are served locally", "/group/project.git/gitlab-lfs/objects/37446575700829a11278ad3a550f244f45d5ae4fe1552778fa4f041f9eaeecf6", "Local Rails server received request to path /group/project.git/gitlab-lfs/objects/37446575700829a11278ad3a550f244f45d5ae4fe1552778fa4f041f9eaeecf6"}, @@ -106,7 +96,7 @@ func TestGeoProxyFeatureEnabledOnGeoSecondarySite(t *testing.T) { {"unknown route is forwarded", "/anything", "Geo primary received request to path /anything"}, } - runTestCases(t, ws, testCases) + runTestCasesWithGeoProxyEnabled(t, testCases) } // This test can be removed when the environment variable `GEO_SECONDARY_PROXY` is removed @@ -227,6 +217,20 @@ func runTestCases(t *testing.T, ws *httptest.Server, testCases []testCase) { } } +func runTestCasesWithGeoProxyEnabled(t *testing.T, testCases []testCase) { + remoteServer, rsDeferredClose := startRemoteServer("Geo primary") + defer rsDeferredClose() + + geoProxyEndpointResponseBody := fmt.Sprintf(`{"geo_proxy_url":"%v"}`, remoteServer.URL) + railsServer, deferredClose := startRailsServer("Local Rails server", &geoProxyEndpointResponseBody) + defer deferredClose() + + ws, wsDeferredClose, _ := startWorkhorseServer(railsServer.URL, true) + defer wsDeferredClose() + + runTestCases(t, ws, testCases) +} + func newUpstreamConfig(authBackend string) *config.Config { return &config.Config{ Version: "123", @@ -284,9 +288,13 @@ func startWorkhorseServer(railsServerURL string, enableGeoProxyFeature bool) (*h } cfg := newUpstreamConfig(railsServerURL) upstreamHandler := newUpstream(*cfg, logrus.StandardLogger(), myConfigureRoutes) - ws := httptest.NewServer(upstreamHandler) + + // Secret should be configured before the first Geo API poll happens on server start + // to prevent race conditions where the first API call happens without a secret path testhelper.ConfigureSecret() + ws := httptest.NewServer(upstreamHandler) + waitForNextApiPoll := func() {} if enableGeoProxyFeature { diff --git a/workhorse/internal/zipartifacts/open_archive.go b/workhorse/internal/zipartifacts/open_archive.go index cf0e38e9ee0..ec2fd691038 100644 --- a/workhorse/internal/zipartifacts/open_archive.go +++ b/workhorse/internal/zipartifacts/open_archive.go @@ -5,32 +5,20 @@ import ( "context" "fmt" "io" - "net" "net/http" "os" "strings" - "time" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/httptransport" "gitlab.com/gitlab-org/gitlab/workhorse/internal/httprs" - "gitlab.com/gitlab-org/labkit/correlation" "gitlab.com/gitlab-org/labkit/mask" - "gitlab.com/gitlab-org/labkit/tracing" ) var httpClient = &http.Client{ - Transport: tracing.NewRoundTripper(correlation.NewInstrumentedRoundTripper(&http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: 30 * time.Second, - KeepAlive: 10 * time.Second, - }).DialContext, - IdleConnTimeout: 30 * time.Second, - TLSHandshakeTimeout: 10 * time.Second, - ExpectContinueTimeout: 10 * time.Second, - ResponseHeaderTimeout: 30 * time.Second, - DisableCompression: true, - })), + Transport: httptransport.New( + httptransport.WithDisabledCompression(), // To avoid bugs when serving compressed files from object storage + ), } type archive struct { diff --git a/workhorse/upload_test.go b/workhorse/upload_test.go index 24c14bb12aa..478cbdb1a44 100644 --- a/workhorse/upload_test.go +++ b/workhorse/upload_test.go @@ -83,7 +83,7 @@ func uploadTestServer(t *testing.T, authorizeTests func(r *http.Request), extraT require.NoError(t, r.ParseMultipartForm(100000)) - const nValues = 10 // file name, path, remote_url, remote_id, size, md5, sha1, sha256, sha512, gitlab-workhorse-upload for just the upload (no metadata because we are not POSTing a valid zip file) + const nValues = 11 // file name, path, remote_url, remote_id, size, md5, sha1, sha256, sha512, upload_duration, gitlab-workhorse-upload for just the upload (no metadata because we are not POSTing a valid zip file) require.Len(t, r.MultipartForm.Value, nValues) require.Empty(t, r.MultipartForm.File, "multipart form files") @@ -123,6 +123,8 @@ func TestAcceleratedUpload(t *testing.T) { {"POST", `/api/v4/projects/group%2Fproject/wikis/attachments`, false}, {"POST", `/api/v4/projects/group%2Fsubgroup%2Fproject/wikis/attachments`, false}, {"POST", `/api/graphql`, false}, + {"POST", `/api/v4/topics`, false}, + {"PUT", `/api/v4/topics`, false}, {"PUT", "/api/v4/projects/9001/packages/nuget/v1/files", true}, {"PUT", "/api/v4/projects/group%2Fproject/packages/nuget/v1/files", true}, {"PUT", "/api/v4/projects/group%2Fsubgroup%2Fproject/packages/nuget/v1/files", true}, |