Add latest changes from gitlab-org/gitlab@13-9-stable-eev13.9.0-rc42

10 files changed, 118 insertions, 100 deletions

diff --git a/workhorse/internal/api/api.go b/workhorse/internal/api/api.go
index 988bb73f256..a420288a95a 100644
--- a/workhorse/internal/api/api.go
+++ b/workhorse/internal/api/api.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/textproto"
 	"net/url"
 	"strconv"
 	"strings"
@@ -188,6 +189,8 @@ func (api *API) newRequest(r *http.Request, suffix string) (*http.Request, error
 
 	authReq = authReq.WithContext(r.Context())
 
+	removeConnectionHeaders(authReq.Header)
+
 	// Clean some headers when issuing a new request without body
 	authReq.Header.Del("Content-Type")
 	authReq.Header.Del("Content-Encoding")
@@ -203,7 +206,9 @@ func (api *API) newRequest(r *http.Request, suffix string) (*http.Request, error
 	authReq.Header.Del("Proxy-Authenticate")
 	authReq.Header.Del("Proxy-Authorization")
 	authReq.Header.Del("Te")
-	authReq.Header.Del("Trailers")
+	// "Trailer", not "Trailers" as per rfc2616; See errata https://www.rfc-editor.org/errata_search.php?eid=4522
+	// See https://httpwg.org/http-core/draft-ietf-httpbis-semantics-latest.html#field.connection
+	authReq.Header.Del("Trailer")
 	authReq.Header.Del("Upgrade")
 
 	// Also forward the Host header, which is excluded from the Header map by the http library.
@@ -290,6 +295,18 @@ func (api *API) doRequestWithoutRedirects(authReq *http.Request) (*http.Response
 	return signingTripper.RoundTrip(authReq)
 }
 
+// removeConnectionHeaders removes hop-by-hop headers listed in the "Connection" header of h.
+// See https://tools.ietf.org/html/rfc7230#section-6.1
+func removeConnectionHeaders(h http.Header) {
+	for _, f := range h["Connection"] {
+		for _, sf := range strings.Split(f, ",") {
+			if sf = textproto.TrimString(sf); sf != "" {
+				h.Del(sf)
+			}
+		}
+	}
+}
+
 func copyAuthHeader(httpResponse *http.Response, w http.ResponseWriter) {
 	// Negotiate authentication (Kerberos) may need to return a WWW-Authenticate
 	// header to the client even in case of success as per RFC4559.
diff --git a/workhorse/internal/errortracker/sentry.go b/workhorse/internal/errortracker/sentry.go
new file mode 100644
index 00000000000..72a32c8d349
--- /dev/null
+++ b/workhorse/internal/errortracker/sentry.go
@@ -0,0 +1,60 @@
+package errortracker
+
+import (
+	"fmt"
+	"net/http"
+	"os"
+	"runtime/debug"
+
+	"gitlab.com/gitlab-org/labkit/errortracking"
+
+	"gitlab.com/gitlab-org/labkit/log"
+)
+
+// NewHandler allows us to handle panics in upstreams gracefully, by logging them
+// using structured logging and reporting them into Sentry as `error`s with a
+// proper correlation ID attached.
+func NewHandler(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		defer func() {
+			if p := recover(); p != nil {
+				fields := log.ContextFields(r.Context())
+				log.WithFields(fields).Error(p)
+				debug.PrintStack()
+				// A panic isn't always an `error`, so we may have to convert it into one.
+				e, ok := p.(error)
+				if !ok {
+					e = fmt.Errorf("%v", p)
+				}
+				TrackFailedRequest(r, e, fields)
+			}
+		}()
+
+		next.ServeHTTP(w, r)
+	})
+}
+
+func TrackFailedRequest(r *http.Request, err error, fields log.Fields) {
+	captureOpts := []errortracking.CaptureOption{
+		errortracking.WithContext(r.Context()),
+		errortracking.WithRequest(r),
+	}
+	for k, v := range fields {
+		captureOpts = append(captureOpts, errortracking.WithField(k, fmt.Sprintf("%v", v)))
+	}
+
+	errortracking.Capture(err, captureOpts...)
+}
+
+func Initialize(version string) error {
+	// Use a custom environment variable (not SENTRY_DSN) to prevent
+	// clashes with gitlab-rails.
+	sentryDSN := os.Getenv("GITLAB_WORKHORSE_SENTRY_DSN")
+	sentryEnvironment := os.Getenv("GITLAB_WORKHORSE_SENTRY_ENVIRONMENT")
+
+	return errortracking.Initialize(
+		errortracking.WithSentryDSN(sentryDSN),
+		errortracking.WithSentryEnvironment(sentryEnvironment),
+		errortracking.WithVersion(version),
+	)
+}
diff --git a/workhorse/internal/filestore/save_file_opts.go b/workhorse/internal/filestore/save_file_opts.go
index 1eb708c3f55..d0b2c6ec809 100644
--- a/workhorse/internal/filestore/save_file_opts.go
+++ b/workhorse/internal/filestore/save_file_opts.go
@@ -146,7 +146,7 @@ func (c *ObjectStorageConfig) IsGoCloud() bool {
 
 func (c *ObjectStorageConfig) IsValid() bool {
 	if c.IsAWS() {
-		return c.S3Config.Bucket != "" && c.S3Config.Region != "" && c.s3CredentialsValid()
+		return c.S3Config.Bucket != "" && c.s3CredentialsValid()
 	} else if c.IsGoCloud() {
 		// We could parse and validate the URL, but GoCloud providers
 		// such as AzureRM don't have a fallback to normal HTTP, so we
diff --git a/workhorse/internal/filestore/save_file_opts_test.go b/workhorse/internal/filestore/save_file_opts_test.go
index 2d6cd683b51..facfb1cdc85 100644
--- a/workhorse/internal/filestore/save_file_opts_test.go
+++ b/workhorse/internal/filestore/save_file_opts_test.go
@@ -187,6 +187,9 @@ func TestUseWorkhorseClientEnabled(t *testing.T) {
 	iamConfig := missingCfg
 	iamConfig.S3Config.UseIamProfile = true
 
+	missingRegion := cfg
+	missingRegion.S3Config.Region = ""
+
 	tests := []struct {
 		name                string
 		UseWorkhorseClient  bool
@@ -245,6 +248,13 @@ func TestUseWorkhorseClientEnabled(t *testing.T) {
 			},
 			expected: false,
 		},
+		{
+			name:                "missing S3 region",
+			UseWorkhorseClient:  true,
+			remoteTempObjectID:  "test-object",
+			objectStorageConfig: missingRegion,
+			expected:            true,
+		},
 	}
 
 	for _, test := range tests {
diff --git a/workhorse/internal/helper/helpers.go b/workhorse/internal/helper/helpers.go
index f9b46181579..2e23f50b913 100644
--- a/workhorse/internal/helper/helpers.go
+++ b/workhorse/internal/helper/helpers.go
@@ -14,50 +14,31 @@ import (
 	"syscall"
 
 	"github.com/sebest/xff"
-	"gitlab.com/gitlab-org/labkit/log"
-	"gitlab.com/gitlab-org/labkit/mask"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/log"
 )
 
 const NginxResponseBufferHeader = "X-Accel-Buffering"
 
-func logErrorWithFields(r *http.Request, err error, fields log.Fields) {
-	if err != nil {
-		CaptureRavenError(r, err, fields)
-	}
-
-	printError(r, err, fields)
-}
-
-func CaptureAndFail(w http.ResponseWriter, r *http.Request, err error, msg string, code int) {
+func CaptureAndFail(w http.ResponseWriter, r *http.Request, err error, msg string, code int, loggerCallbacks ...log.ConfigureLogger) {
 	http.Error(w, msg, code)
-	logErrorWithFields(r, err, nil)
-}
+	logger := log.WithRequest(r).WithError(err)
+
+	for _, cb := range loggerCallbacks {
+		logger = cb(logger)
+	}
 
-func Fail500(w http.ResponseWriter, r *http.Request, err error) {
-	CaptureAndFail(w, r, err, "Internal server error", http.StatusInternalServerError)
+	logger.Error(msg)
 }
 
-func Fail500WithFields(w http.ResponseWriter, r *http.Request, err error, fields log.Fields) {
-	http.Error(w, "Internal server error", http.StatusInternalServerError)
-	logErrorWithFields(r, err, fields)
+func Fail500(w http.ResponseWriter, r *http.Request, err error, loggerCallbacks ...log.ConfigureLogger) {
+	CaptureAndFail(w, r, err, "Internal server error", http.StatusInternalServerError, loggerCallbacks...)
 }
 
 func RequestEntityTooLarge(w http.ResponseWriter, r *http.Request, err error) {
 	CaptureAndFail(w, r, err, "Request Entity Too Large", http.StatusRequestEntityTooLarge)
 }
 
-func printError(r *http.Request, err error, fields log.Fields) {
-	if r != nil {
-		entry := log.WithContextFields(r.Context(), log.Fields{
-			"method": r.Method,
-			"uri":    mask.URL(r.RequestURI),
-		})
-		entry.WithFields(fields).WithError(err).Error()
-	} else {
-		log.WithFields(fields).WithError(err).Error("unknown error")
-	}
-}
-
 func SetNoCacheHeaders(header http.Header) {
 	header.Set("Cache-Control", "no-cache, no-store, max-age=0, must-revalidate")
 	header.Set("Pragma", "no-cache")
@@ -97,7 +78,7 @@ func OpenFile(path string) (file *os.File, fi os.FileInfo, err error) {
 func URLMustParse(s string) *url.URL {
 	u, err := url.Parse(s)
 	if err != nil {
-		log.WithError(err).WithField("url", s).Fatal("urlMustParse")
+		log.WithError(err).WithFields(log.Fields{"url": s}).Error("urlMustParse")
 	}
 	return u
 }
diff --git a/workhorse/internal/helper/raven.go b/workhorse/internal/helper/raven.go
deleted file mode 100644
index 898e8ec85f8..00000000000
--- a/workhorse/internal/helper/raven.go
+++ /dev/null
@@ -1,58 +0,0 @@
-package helper
-
-import (
-	"net/http"
-	"reflect"
-
-	raven "github.com/getsentry/raven-go"
-
-	//lint:ignore SA1019 this was recently deprecated. Update workhorse to use labkit errortracking package.
-	correlation "gitlab.com/gitlab-org/labkit/correlation/raven"
-
-	"gitlab.com/gitlab-org/labkit/log"
-)
-
-var ravenHeaderBlacklist = []string{
-	"Authorization",
-	"Private-Token",
-}
-
-func CaptureRavenError(r *http.Request, err error, fields log.Fields) {
-	client := raven.DefaultClient
-	extra := raven.Extra{}
-
-	for k, v := range fields {
-		extra[k] = v
-	}
-
-	interfaces := []raven.Interface{}
-	if r != nil {
-		CleanHeadersForRaven(r)
-		interfaces = append(interfaces, raven.NewHttp(r))
-
-		//lint:ignore SA1019 this was recently deprecated. Update workhorse to use labkit errortracking package.
-		extra = correlation.SetExtra(r.Context(), extra)
-	}
-
-	exception := &raven.Exception{
-		Stacktrace: raven.NewStacktrace(2, 3, nil),
-		Value:      err.Error(),
-		Type:       reflect.TypeOf(err).String(),
-	}
-	interfaces = append(interfaces, exception)
-
-	packet := raven.NewPacketWithExtra(err.Error(), extra, interfaces...)
-	client.Capture(packet, nil)
-}
-
-func CleanHeadersForRaven(r *http.Request) {
-	if r == nil {
-		return
-	}
-
-	for _, key := range ravenHeaderBlacklist {
-		if r.Header.Get(key) != "" {
-			r.Header.Set(key, "[redacted]")
-		}
-	}
-}
diff --git a/workhorse/internal/imageresizer/image_resizer.go b/workhorse/internal/imageresizer/image_resizer.go
index 69e9496aec2..7d423b80067 100644
--- a/workhorse/internal/imageresizer/image_resizer.go
+++ b/workhorse/internal/imageresizer/image_resizer.go
@@ -428,16 +428,18 @@ func logFields(startTime time.Time, params *resizeParams, outcome *resizeOutcome
 
 func handleOutcome(w http.ResponseWriter, req *http.Request, startTime time.Time, params *resizeParams, outcome *resizeOutcome) {
 	fields := logFields(startTime, params, outcome)
-	log := log.WithRequest(req).WithFields(fields)
+	logger := log.WithRequest(req).WithFields(fields)
 
 	switch outcome.status {
 	case statusRequestFailure:
 		if outcome.bytesWritten <= 0 {
-			helper.Fail500WithFields(w, req, outcome.err, fields)
+			helper.Fail500(w, req, outcome.err, func(b *log.Builder) *log.Builder {
+				return b.WithFields(fields)
+			})
 		} else {
-			log.WithError(outcome.err).Error(outcome.status)
+			logger.WithError(outcome.err).Error(outcome.status)
 		}
 	default:
-		log.Info(outcome.status)
+		logger.Info(outcome.status)
 	}
 }
diff --git a/workhorse/internal/log/logging.go b/workhorse/internal/log/logging.go
index c65ec07743a..9c19cde1395 100644
--- a/workhorse/internal/log/logging.go
+++ b/workhorse/internal/log/logging.go
@@ -8,11 +8,13 @@ import (
 	"gitlab.com/gitlab-org/labkit/mask"
 	"golang.org/x/net/context"
 
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/errortracker"
 )
 
 type Fields = log.Fields
 
+type ConfigureLogger func(*Builder) *Builder
+
 type Builder struct {
 	entry  *logrus.Entry
 	fields log.Fields
@@ -83,6 +85,6 @@ func (b *Builder) Error(args ...interface{}) {
 	b.entry.Error(args...)
 
 	if b.req != nil && b.err != nil {
-		helper.CaptureRavenError(b.req, b.err, b.fields)
+		errortracker.TrackFailedRequest(b.req, b.err, b.fields)
 	}
 }
diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go
index 5bbd245719b..edcbfa88a67 100644
--- a/workhorse/internal/upstream/routes.go
+++ b/workhorse/internal/upstream/routes.go
@@ -55,7 +55,7 @@ type uploadPreparers struct {
 const (
 	apiPattern           = `^/api/`
 	ciAPIPattern         = `^/ci/api/`
-	gitProjectPattern    = `^/([^/]+/){1,}[^/]+\.git/`
+	gitProjectPattern    = `^/.+\.git/`
 	projectPattern       = `^/([^/]+/){1,}[^/]+/`
 	snippetUploadPattern = `^/uploads/personal_snippet`
 	userUploadPattern    = `^/uploads/user`
@@ -262,7 +262,10 @@ func (u *upstream) configureRoutes() {
 		u.route("POST", apiPattern+`v4/projects/[0-9]+/packages/pypi`, upload.Accelerate(api, signingProxy, preparers.packages)),
 
 		// Debian Artifact Repository
-		u.route("PUT", apiPattern+`v4/projects/[0-9]+/-/packages/debian/incoming/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
+		u.route("PUT", apiPattern+`v4/projects/[0-9]+/packages/debian/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
+
+		// Gem Artifact Repository
+		u.route("POST", apiPattern+`v4/projects/[0-9]+/packages/rubygems/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
 
 		// We are porting API to disk acceleration
 		// we need to declare each routes until we have fixed all the routes on the rails codebase.
diff --git a/workhorse/internal/upstream/upstream.go b/workhorse/internal/upstream/upstream.go
index c81a21c0ecd..fd655a07679 100644
--- a/workhorse/internal/upstream/upstream.go
+++ b/workhorse/internal/upstream/upstream.go
@@ -16,6 +16,7 @@ import (
 	"gitlab.com/gitlab-org/labkit/correlation"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/errortracker"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/rejectmethods"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upload"
@@ -63,7 +64,7 @@ func NewUpstream(cfg config.Config, accessLogger *logrus.Logger) http.Handler {
 		correlationOpts = append(correlationOpts, correlation.WithPropagation())
 	}
 
-	handler := correlation.InjectCorrelationID(&up, correlationOpts...)
+	handler := correlation.InjectCorrelationID(errortracker.NewHandler(&up), correlationOpts...)
 	// TODO: move to LabKit https://gitlab.com/gitlab-org/gitlab-workhorse/-/issues/339
 	handler = rejectmethods.NewMiddleware(handler)
 	return handler