diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-02-18 13:34:06 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-02-18 13:34:06 +0300 |
commit | 859a6fb938bb9ee2a317c46dfa4fcc1af49608f0 (patch) | |
tree | d7f2700abe6b4ffcb2dcfc80631b2d87d0609239 /workhorse/internal | |
parent | 446d496a6d000c73a304be52587cd9bbc7493136 (diff) |
Add latest changes from gitlab-org/gitlab@13-9-stable-eev13.9.0-rc42
Diffstat (limited to 'workhorse/internal')
-rw-r--r-- | workhorse/internal/api/api.go | 19 | ||||
-rw-r--r-- | workhorse/internal/errortracker/sentry.go | 60 | ||||
-rw-r--r-- | workhorse/internal/filestore/save_file_opts.go | 2 | ||||
-rw-r--r-- | workhorse/internal/filestore/save_file_opts_test.go | 10 | ||||
-rw-r--r-- | workhorse/internal/helper/helpers.go | 43 | ||||
-rw-r--r-- | workhorse/internal/helper/raven.go | 58 | ||||
-rw-r--r-- | workhorse/internal/imageresizer/image_resizer.go | 10 | ||||
-rw-r--r-- | workhorse/internal/log/logging.go | 6 | ||||
-rw-r--r-- | workhorse/internal/upstream/routes.go | 7 | ||||
-rw-r--r-- | workhorse/internal/upstream/upstream.go | 3 |
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 |