From 7af78af1f0e30120eb6048a8fb2624417fcdf678 Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Sat, 8 Jul 2023 07:31:42 -0700 Subject: [PATCH] fix: set request ID in tracing context key (#17602) Since `addCustomerHeaders` middleware was after the `httpTracer` middleware, the request ID was not set in the http tracing context. By reordering these middleware functions, the request ID header becomes available. We also avoid setting the tracing context key again in `newContext`. Bonus: All middleware functions are renamed with a "Middleware" suffix to avoid confusion with http Handler functions. --- cmd/auth-handler.go | 4 +-- cmd/crossdomain-xml-handler.go | 2 +- cmd/crossdomain-xml-handler_test.go | 2 +- cmd/generic-handlers.go | 18 +++++------ cmd/generic-handlers_test.go | 2 +- cmd/http-tracer.go | 2 +- cmd/routers.go | 48 +++++++++++++++-------------- cmd/test-utils_test.go | 4 +-- cmd/utils.go | 10 +----- 9 files changed, 43 insertions(+), 49 deletions(-) diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 46e7bf50d..99cd7d08c 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -595,8 +595,8 @@ func isSupportedS3AuthType(aType authType) bool { return ok } -// setAuthHandler to validate authorization header for the incoming request. -func setAuthHandler(h http.Handler) http.Handler { +// setAuthMiddleware to validate authorization header for the incoming request. +func setAuthMiddleware(h http.Handler) http.Handler { // handler for validating incoming authorization headers. return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { tc, ok := r.Context().Value(mcontext.ContextTraceKey).(*mcontext.TraceCtxt) diff --git a/cmd/crossdomain-xml-handler.go b/cmd/crossdomain-xml-handler.go index e1e0ac625..14856e6af 100644 --- a/cmd/crossdomain-xml-handler.go +++ b/cmd/crossdomain-xml-handler.go @@ -30,7 +30,7 @@ const crossDomainXMLEntity = "/crossdomain.xml" // When clients request content hosted on a particular source domain and that content make requests // directed towards a domain other than its own, the remote domain needs to host a cross-domain // policy file that grants access to the source domain, allowing the client to continue the transaction. -func setCrossDomainPolicy(h http.Handler) http.Handler { +func setCrossDomainPolicyMiddleware(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Look for 'crossdomain.xml' in the incoming request. if r.URL.Path == crossDomainXMLEntity { diff --git a/cmd/crossdomain-xml-handler_test.go b/cmd/crossdomain-xml-handler_test.go index 89fca30d3..a5f0cb5ac 100644 --- a/cmd/crossdomain-xml-handler_test.go +++ b/cmd/crossdomain-xml-handler_test.go @@ -29,7 +29,7 @@ import ( func TestCrossXMLHandler(t *testing.T) { // Server initialization. router := mux.NewRouter().SkipClean(true).UseEncodedPath() - handler := setCrossDomainPolicy(router) + handler := setCrossDomainPolicyMiddleware(router) srv := httptest.NewServer(handler) resp, err := http.Get(srv.URL + crossDomainXMLEntity) diff --git a/cmd/generic-handlers.go b/cmd/generic-handlers.go index 0e5ed8087..a2fa32d1b 100644 --- a/cmd/generic-handlers.go +++ b/cmd/generic-handlers.go @@ -100,7 +100,7 @@ func isHTTPHeaderSizeTooLarge(header http.Header) bool { } // Limits body and header to specific allowed maximum limits as per S3/MinIO API requirements. -func setRequestLimitHandler(h http.Handler) http.Handler { +func setRequestLimitMiddleware(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { tc, ok := r.Context().Value(mcontext.ContextTraceKey).(*mcontext.TraceCtxt) @@ -147,7 +147,7 @@ func guessIsBrowserReq(r *http.Request) bool { globalBrowserEnabled && aType == authTypeAnonymous } -func setBrowserRedirectHandler(h http.Handler) http.Handler { +func setBrowserRedirectMiddleware(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { read := r.Method == http.MethodGet || r.Method == http.MethodHead // Re-direction is handled specifically for browser requests. @@ -325,7 +325,7 @@ func hasMultipleAuth(r *http.Request) bool { // requestValidityHandler validates all the incoming paths for // any malicious requests. -func setRequestValidityHandler(h http.Handler) http.Handler { +func setRequestValidityMiddleware(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { tc, ok := r.Context().Value(mcontext.ContextTraceKey).(*mcontext.TraceCtxt) @@ -425,10 +425,10 @@ func setRequestValidityHandler(h http.Handler) http.Handler { }) } -// setBucketForwardingHandler middleware forwards the path style requests +// setBucketForwardingMiddleware middleware forwards the path style requests // on a bucket to the right bucket location, bucket to IP configuration // is obtained from centralized etcd configuration service. -func setBucketForwardingHandler(h http.Handler) http.Handler { +func setBucketForwardingMiddleware(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if globalDNSConfig == nil || !globalBucketFederation || guessIsHealthCheckReq(r) || guessIsMetricsReq(r) || @@ -493,9 +493,9 @@ func setBucketForwardingHandler(h http.Handler) http.Handler { }) } -// addCustomHeaders adds various HTTP(S) response headers. +// addCustomHeadersMiddleware adds various HTTP(S) response headers. // Security Headers enable various security protections behaviors in the client's browser. -func addCustomHeaders(h http.Handler) http.Handler { +func addCustomHeadersMiddleware(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { header := w.Header() header.Set("X-XSS-Protection", "1; mode=block") // Prevents against XSS attacks @@ -539,9 +539,9 @@ func setCriticalErrorHandler(h http.Handler) http.Handler { }) } -// setUploadForwardingHandler middleware forwards multiparts requests +// setUploadForwardingMiddleware middleware forwards multiparts requests // in a site replication setup to peer that initiated the upload -func setUploadForwardingHandler(h http.Handler) http.Handler { +func setUploadForwardingMiddleware(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !globalSiteReplicationSys.isEnabled() || guessIsHealthCheckReq(r) || guessIsMetricsReq(r) || diff --git a/cmd/generic-handlers_test.go b/cmd/generic-handlers_test.go index 85bc5ad96..70d7177b6 100644 --- a/cmd/generic-handlers_test.go +++ b/cmd/generic-handlers_test.go @@ -155,7 +155,7 @@ func TestSSETLSHandler(t *testing.T) { r.Header = test.Header r.URL = test.URL - h := setRequestValidityHandler(okHandler) + h := setRequestValidityMiddleware(okHandler) h.ServeHTTP(w, r) switch { diff --git a/cmd/http-tracer.go b/cmd/http-tracer.go index 98339c892..a495effbb 100644 --- a/cmd/http-tracer.go +++ b/cmd/http-tracer.go @@ -65,7 +65,7 @@ func getOpName(name string) (op string) { // If trace is enabled, execute the request if it is traced by other handlers // otherwise, generate a trace event with request information but no response. -func httpTracer(h http.Handler) http.Handler { +func httpTracerMiddleware(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Setup a http request response recorder - this is needed for // http stats requests and audit if enabled. diff --git a/cmd/routers.go b/cmd/routers.go index ff8c88e3a..55e5ef62c 100644 --- a/cmd/routers.go +++ b/cmd/routers.go @@ -41,32 +41,34 @@ func registerDistErasureRouters(router *mux.Router, endpointServerPools Endpoint registerLockRESTHandlers(router) } -// List of some generic handlers which are applied for all incoming requests. -var globalHandlers = []mux.MiddlewareFunc{ - // The generic tracer needs to be the first handler - // to catch all requests returned early by any other handler - httpTracer, - // Auth handler verifies incoming authorization headers and - // routes them accordingly. Client receives a HTTP error for - // invalid/unsupported signatures. +// List of some generic middlewares which are applied for all incoming requests. +var globalMiddlewares = []mux.MiddlewareFunc{ + // set x-amz-request-id header and others + addCustomHeadersMiddleware, + // The generic tracer needs to be the first middleware to catch all requests + // returned early by any other middleware (but after the middleware that + // sets the amz request id). + httpTracerMiddleware, + // Auth middleware verifies incoming authorization headers and routes them + // accordingly. Client receives a HTTP error for invalid/unsupported + // signatures. // // Validates all incoming requests to have a valid date header. - setAuthHandler, - // Redirect some pre-defined browser request paths to a static location prefix. - setBrowserRedirectHandler, - // Adds 'crossdomain.xml' policy handler to serve legacy flash clients. - setCrossDomainPolicy, + setAuthMiddleware, + // Redirect some pre-defined browser request paths to a static location + // prefix. + setBrowserRedirectMiddleware, + // Adds 'crossdomain.xml' policy middleware to serve legacy flash clients. + setCrossDomainPolicyMiddleware, // Limits all body and header sizes to a maximum fixed limit - setRequestLimitHandler, + setRequestLimitMiddleware, // Validate all the incoming requests. - setRequestValidityHandler, - // set x-amz-request-id header. - addCustomHeaders, - // Add upload forwarding handler for site replication - setUploadForwardingHandler, - // Add bucket forwarding handler - setBucketForwardingHandler, - // Add new handlers here. + setRequestValidityMiddleware, + // Add upload forwarding middleware for site replication + setUploadForwardingMiddleware, + // Add bucket forwarding middleware + setBucketForwardingMiddleware, + // Add new middlewares here. } // configureServer handler returns final handler for the http server. @@ -98,7 +100,7 @@ func configureServerHandler(endpointServerPools EndpointServerPools) (http.Handl // Add API router registerAPIRouter(router) - router.Use(globalHandlers...) + router.Use(globalMiddlewares...) return router, nil } diff --git a/cmd/test-utils_test.go b/cmd/test-utils_test.go index ac296fa74..6c6c89314 100644 --- a/cmd/test-utils_test.go +++ b/cmd/test-utils_test.go @@ -2070,11 +2070,11 @@ func initTestAPIEndPoints(objLayer ObjectLayer, apiFunctions []string) http.Hand if len(apiFunctions) > 0 { // Iterate the list of API functions requested for and register them in mux HTTP handler. registerAPIFunctions(muxRouter, objLayer, apiFunctions...) - muxRouter.Use(globalHandlers...) + muxRouter.Use(globalMiddlewares...) return muxRouter } registerAPIRouter(muxRouter) - muxRouter.Use(globalHandlers...) + muxRouter.Use(globalMiddlewares...) return muxRouter } diff --git a/cmd/utils.go b/cmd/utils.go index 50bc9b579..9d7a31f5f 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -56,7 +56,6 @@ import ( ioutilx "github.com/minio/minio/internal/ioutil" "github.com/minio/minio/internal/logger" "github.com/minio/minio/internal/logger/message/audit" - "github.com/minio/minio/internal/mcontext" "github.com/minio/minio/internal/rest" "github.com/minio/mux" "github.com/minio/pkg/certs" @@ -855,14 +854,7 @@ func newContext(r *http.Request, w http.ResponseWriter, api string) context.Cont VersionID: strings.TrimSpace(r.Form.Get(xhttp.VersionID)), } - ctx := context.WithValue(r.Context(), - mcontext.ContextTraceKey, - &mcontext.TraceCtxt{ - AmzReqID: reqID, - }, - ) - - return logger.SetReqInfo(ctx, reqInfo) + return logger.SetReqInfo(r.Context(), reqInfo) } // Used for registering with rest handlers (have a look at registerStorageRESTHandlers for usage example)