From 65c979e997af3a50d8022f2c978456df7a4ed930 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Wed, 8 Jun 2022 23:16:45 +0100 Subject: [PATCH 1/8] Base for the router --- app.go | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/app.go b/app.go index ac3071a9c..e377afcd8 100644 --- a/app.go +++ b/app.go @@ -133,6 +133,30 @@ func setRequestScheme(r *http.Request) *http.Request { return r } +type Middleware func(http.Handler) http.Handler + +type Server struct { + Handler *http.ServeMux + defaultMiddlewares []Middleware +} + +func NewServer(middlewares ...Middleware) Server { + return Server{ + Handler: http.NewServeMux(), + defaultMiddlewares: middlewares, + } +} + +func (s Server) Handle(route string, handler http.Handler, middlewares ...Middleware) { + ms := append(s.defaultMiddlewares, middlewares...) + + for i := len(ms) - 1; i >= 0; i-- { + handler = ms[i](handler) + } + + s.Handler.Handle(route, handler) +} + // TODO: move the pipeline configuration to internal/pipeline https://gitlab.com/gitlab-org/gitlab-pages/-/issues/670 func (a *theApp) buildHandlerPipeline() (http.Handler, error) { // Handlers should be applied in a reverse order @@ -181,7 +205,11 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { handler = urilimiter.NewMiddleware(handler, a.config.General.MaxURILength) handler = rejectmethods.NewMiddleware(handler) - return handler, nil + router := NewServer() + + router.Handle("/", handler) + + return router.Handler, nil } // nolint: gocyclo // ignore this -- GitLab From 46b96c6a0493c6115451dbbd8b7bdaf0d3226bbf Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Wed, 8 Jun 2022 23:29:03 +0100 Subject: [PATCH 2/8] Add some middlewares to the new router --- app.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/app.go b/app.go index e377afcd8..59b4a8591 100644 --- a/app.go +++ b/app.go @@ -194,18 +194,20 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { if err != nil { return nil, err } - metricsMiddleware := labmetrics.NewHandlerFactory(labmetrics.WithNamespace("gitlab_pages")) - handler = metricsMiddleware(handler) - handler = correlation.InjectCorrelationID(handler, correlationOpts...) - - // These middlewares MUST be added in the end. - // Being last means they will be evaluated first - // preventing any operation on bogus requests. - handler = urilimiter.NewMiddleware(handler, a.config.General.MaxURILength) - handler = rejectmethods.NewMiddleware(handler) - - router := NewServer() + router := NewServer( + rejectmethods.NewMiddleware, + func(next http.Handler) http.Handler { + return urilimiter.NewMiddleware(next, a.config.General.MaxURILength) + }, + func(next http.Handler) http.Handler { + return correlation.InjectCorrelationID(next, correlationOpts...) + }, + func(next http.Handler) http.Handler { + metricsMiddleware := labmetrics.NewHandlerFactory(labmetrics.WithNamespace("gitlab_pages")) + return metricsMiddleware(handler) + }, + ) router.Handle("/", handler) -- GitLab From 76b776aaa01fd77076c55958b70305c914cc0f54 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Thu, 9 Jun 2022 13:33:37 +0100 Subject: [PATCH 3/8] move new router to its own package --- app.go | 31 ++++--------------------------- internal/router/router.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 27 deletions(-) create mode 100644 internal/router/router.go diff --git a/app.go b/app.go index 59b4a8591..ef149148a 100644 --- a/app.go +++ b/app.go @@ -34,6 +34,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/netutil" "gitlab.com/gitlab-org/gitlab-pages/internal/rejectmethods" "gitlab.com/gitlab-org/gitlab-pages/internal/request" + server_router "gitlab.com/gitlab-org/gitlab-pages/internal/router" "gitlab.com/gitlab-org/gitlab-pages/internal/routing" "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/zip" "gitlab.com/gitlab-org/gitlab-pages/internal/source" @@ -133,30 +134,6 @@ func setRequestScheme(r *http.Request) *http.Request { return r } -type Middleware func(http.Handler) http.Handler - -type Server struct { - Handler *http.ServeMux - defaultMiddlewares []Middleware -} - -func NewServer(middlewares ...Middleware) Server { - return Server{ - Handler: http.NewServeMux(), - defaultMiddlewares: middlewares, - } -} - -func (s Server) Handle(route string, handler http.Handler, middlewares ...Middleware) { - ms := append(s.defaultMiddlewares, middlewares...) - - for i := len(ms) - 1; i >= 0; i-- { - handler = ms[i](handler) - } - - s.Handler.Handle(route, handler) -} - // TODO: move the pipeline configuration to internal/pipeline https://gitlab.com/gitlab-org/gitlab-pages/-/issues/670 func (a *theApp) buildHandlerPipeline() (http.Handler, error) { // Handlers should be applied in a reverse order @@ -195,7 +172,7 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { return nil, err } - router := NewServer( + router := server_router.NewRouter( rejectmethods.NewMiddleware, func(next http.Handler) http.Handler { return urilimiter.NewMiddleware(next, a.config.General.MaxURILength) @@ -205,13 +182,13 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { }, func(next http.Handler) http.Handler { metricsMiddleware := labmetrics.NewHandlerFactory(labmetrics.WithNamespace("gitlab_pages")) - return metricsMiddleware(handler) + return metricsMiddleware(next) }, ) router.Handle("/", handler) - return router.Handler, nil + return router, nil } // nolint: gocyclo // ignore this diff --git a/internal/router/router.go b/internal/router/router.go new file mode 100644 index 000000000..6a5bd39b9 --- /dev/null +++ b/internal/router/router.go @@ -0,0 +1,31 @@ +package router + +import "net/http" + +type middleware = func(http.Handler) http.Handler + +type server struct { + handler *http.ServeMux + defaultMiddlewares []middleware +} + +func NewRouter(middlewares ...middleware) server { + return server{ + handler: http.NewServeMux(), + defaultMiddlewares: middlewares, + } +} + +func (s server) Handle(route string, handler http.Handler, middlewares ...middleware) { + ms := append(s.defaultMiddlewares, middlewares...) + + for i := len(ms) - 1; i >= 0; i-- { + handler = ms[i](handler) + } + + s.handler.Handle(route, handler) +} + +func (s server) ServeHTTP(w http.ResponseWriter, r *http.Request) { + s.handler.ServeHTTP(w, r) +} -- GitLab From a8b39ca9e2581386fb1b0f3a32c633154dde7430 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Thu, 9 Jun 2022 19:09:06 +0100 Subject: [PATCH 4/8] playing with some more descriptive names --- app.go | 4 ++-- internal/router/router.go | 21 ++++++++++++--------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/app.go b/app.go index ef149148a..45bd23316 100644 --- a/app.go +++ b/app.go @@ -34,7 +34,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/netutil" "gitlab.com/gitlab-org/gitlab-pages/internal/rejectmethods" "gitlab.com/gitlab-org/gitlab-pages/internal/request" - server_router "gitlab.com/gitlab-org/gitlab-pages/internal/router" + app_router "gitlab.com/gitlab-org/gitlab-pages/internal/router" "gitlab.com/gitlab-org/gitlab-pages/internal/routing" "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/zip" "gitlab.com/gitlab-org/gitlab-pages/internal/source" @@ -172,7 +172,7 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { return nil, err } - router := server_router.NewRouter( + router := app_router.NewRouter( rejectmethods.NewMiddleware, func(next http.Handler) http.Handler { return urilimiter.NewMiddleware(next, a.config.General.MaxURILength) diff --git a/internal/router/router.go b/internal/router/router.go index 6a5bd39b9..65170a70c 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -4,28 +4,31 @@ import "net/http" type middleware = func(http.Handler) http.Handler -type server struct { - handler *http.ServeMux +type Router struct { + server *http.ServeMux defaultMiddlewares []middleware } -func NewRouter(middlewares ...middleware) server { - return server{ - handler: http.NewServeMux(), +// NewRouter creates a new Server. The given middlewares are be executed in the given order. +func NewRouter(middlewares ...middleware) Router { + return Router{ + server: http.NewServeMux(), defaultMiddlewares: middlewares, } } -func (s server) Handle(route string, handler http.Handler, middlewares ...middleware) { +// Handle registers a new handler for the given pattern. The optional middlewares are executed in +// the given order, wrapping the given handler. +func (s Router) Handle(route string, handler http.Handler, middlewares ...middleware) { ms := append(s.defaultMiddlewares, middlewares...) for i := len(ms) - 1; i >= 0; i-- { handler = ms[i](handler) } - s.handler.Handle(route, handler) + s.server.Handle(route, handler) } -func (s server) ServeHTTP(w http.ResponseWriter, r *http.Request) { - s.handler.ServeHTTP(w, r) +func (s Router) ServeHTTP(w http.ResponseWriter, r *http.Request) { + s.server.ServeHTTP(w, r) } -- GitLab From 813401a95394d3d17591ca12721f03230856629a Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Thu, 9 Jun 2022 19:19:36 +0100 Subject: [PATCH 5/8] using embedded structures to avoid duplication --- internal/router/router.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/internal/router/router.go b/internal/router/router.go index 65170a70c..5c353e4b7 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -2,17 +2,18 @@ package router import "net/http" +// middleware alias to avoid the long function description internally type middleware = func(http.Handler) http.Handler type Router struct { - server *http.ServeMux + *http.ServeMux defaultMiddlewares []middleware } // NewRouter creates a new Server. The given middlewares are be executed in the given order. func NewRouter(middlewares ...middleware) Router { return Router{ - server: http.NewServeMux(), + ServeMux: http.NewServeMux(), defaultMiddlewares: middlewares, } } @@ -26,9 +27,5 @@ func (s Router) Handle(route string, handler http.Handler, middlewares ...middle handler = ms[i](handler) } - s.server.Handle(route, handler) -} - -func (s Router) ServeHTTP(w http.ResponseWriter, r *http.Request) { - s.server.ServeHTTP(w, r) + s.ServeMux.Handle(route, handler) } -- GitLab From 9753b3ee7cfbd1ccec3ea0c9d7584a68bd8679d1 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Thu, 9 Jun 2022 23:46:40 +0100 Subject: [PATCH 6/8] move more middlewares to the router --- app.go | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/app.go b/app.go index 45bd23316..2a65258c1 100644 --- a/app.go +++ b/app.go @@ -156,21 +156,11 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { // Health Check handler = health.NewMiddleware(handler, a.config.General.StatusPath) - // Custom response headers - handler = customheaders.NewMiddleware(handler, a.CustomHeaders) - - // Correlation ID injection middleware + metricsMiddleware := labmetrics.NewHandlerFactory(labmetrics.WithNamespace("gitlab_pages")) var correlationOpts []correlation.InboundHandlerOption if a.config.General.PropagateCorrelationID { correlationOpts = append(correlationOpts, correlation.WithPropagation()) } - handler = handlePanicMiddleware(handler) - - // Access logs and metrics - handler, err := logging.BasicAccessLogger(handler, a.config.Log.Format) - if err != nil { - return nil, err - } router := app_router.NewRouter( rejectmethods.NewMiddleware, @@ -181,9 +171,20 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { return correlation.InjectCorrelationID(next, correlationOpts...) }, func(next http.Handler) http.Handler { - metricsMiddleware := labmetrics.NewHandlerFactory(labmetrics.WithNamespace("gitlab_pages")) return metricsMiddleware(next) }, + func(next http.Handler) http.Handler { + logHandler, err := logging.BasicAccessLogger(next, a.config.Log.Format) + if err != nil { + return next + } + + return logHandler + }, + handlePanicMiddleware, + func(next http.Handler) http.Handler { + return customheaders.NewMiddleware(next, a.CustomHeaders) + }, ) router.Handle("/", handler) -- GitLab From 279f4ffbc5ff7b7ba991b8cbed7e21919c264728 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Thu, 9 Jun 2022 23:47:11 +0100 Subject: [PATCH 7/8] transform healthcheck middleware in handler --- app.go | 6 +-- internal/healthcheck/handler.go | 11 ++++++ internal/healthcheck/handler_test.go | 17 +++++++++ internal/healthcheck/middleware.go | 19 ---------- internal/healthcheck/middleware_test.go | 50 ------------------------- 5 files changed, 31 insertions(+), 72 deletions(-) create mode 100644 internal/healthcheck/handler.go create mode 100644 internal/healthcheck/handler_test.go delete mode 100644 internal/healthcheck/middleware.go delete mode 100644 internal/healthcheck/middleware_test.go diff --git a/app.go b/app.go index 2a65258c1..a4bb54618 100644 --- a/app.go +++ b/app.go @@ -153,9 +153,6 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { handler = handlers.Ratelimiter(handler, &a.config.RateLimit) - // Health Check - handler = health.NewMiddleware(handler, a.config.General.StatusPath) - metricsMiddleware := labmetrics.NewHandlerFactory(labmetrics.WithNamespace("gitlab_pages")) var correlationOpts []correlation.InboundHandlerOption if a.config.General.PropagateCorrelationID { @@ -187,6 +184,9 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { }, ) + if a.config.General.StatusPath != "" { + router.Handle(a.config.General.StatusPath, health.Handler()) + } router.Handle("/", handler) return router, nil diff --git a/internal/healthcheck/handler.go b/internal/healthcheck/handler.go new file mode 100644 index 000000000..e65d5f95d --- /dev/null +++ b/internal/healthcheck/handler.go @@ -0,0 +1,11 @@ +package healthcheck + +import "net/http" + +// Handler is serving the application status check +func Handler() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Cache-Control", "no-store") + w.Write([]byte("success\n")) + }) +} diff --git a/internal/healthcheck/handler_test.go b/internal/healthcheck/handler_test.go new file mode 100644 index 000000000..7823c1a4e --- /dev/null +++ b/internal/healthcheck/handler_test.go @@ -0,0 +1,17 @@ +package healthcheck_test + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-pages/internal/healthcheck" +) + +func TestHealthCheckHandler(t *testing.T) { + u := "https://example.com/-/healthcheck" + + require.HTTPStatusCode(t, healthcheck.Handler().ServeHTTP, http.MethodGet, u, nil, http.StatusOK) + require.HTTPBodyContains(t, healthcheck.Handler().ServeHTTP, http.MethodGet, u, nil, "success\n") +} diff --git a/internal/healthcheck/middleware.go b/internal/healthcheck/middleware.go deleted file mode 100644 index 2ddd35b70..000000000 --- a/internal/healthcheck/middleware.go +++ /dev/null @@ -1,19 +0,0 @@ -package healthcheck - -import ( - "net/http" -) - -// NewMiddleware is serving the application status check -func NewMiddleware(handler http.Handler, statusPath string) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == statusPath { - w.Header().Set("Cache-Control", "no-store") - w.Write([]byte("success\n")) - - return - } - - handler.ServeHTTP(w, r) - }) -} diff --git a/internal/healthcheck/middleware_test.go b/internal/healthcheck/middleware_test.go deleted file mode 100644 index 124bb0ff5..000000000 --- a/internal/healthcheck/middleware_test.go +++ /dev/null @@ -1,50 +0,0 @@ -package healthcheck_test - -import ( - "io" - "net/http" - "testing" - - "github.com/stretchr/testify/require" - - "gitlab.com/gitlab-org/gitlab-pages/internal/config" - "gitlab.com/gitlab-org/gitlab-pages/internal/healthcheck" -) - -func TestHealthCheckMiddleware(t *testing.T) { - tests := map[string]struct { - path string - body string - }{ - "Not a healthcheck request": { - path: "/foo/bar", - body: "Hello from inner handler", - }, - "Healthcheck request": { - path: "/-/healthcheck", - body: "success\n", - }, - } - - cfg := config.Config{ - General: config.General{ - StatusPath: "/-/healthcheck", - }, - } - - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - io.WriteString(w, "Hello from inner handler") - }) - - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - middleware := healthcheck.NewMiddleware(handler, cfg.General.StatusPath) - - u := "https://example.com" + tc.path - - require.HTTPStatusCode(t, middleware.ServeHTTP, http.MethodGet, u, nil, http.StatusOK) - require.HTTPBodyContains(t, middleware.ServeHTTP, http.MethodGet, u, nil, tc.body) - }) - } -} -- GitLab From 483fe26f2c78c2afef6b048d383ffdf092003962 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Fri, 10 Jun 2022 00:02:20 +0100 Subject: [PATCH 8/8] Move access logger access to outside the routing definition --- app.go | 13 +++++++------ internal/logging/logging.go | 14 +++++--------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/app.go b/app.go index a4bb54618..f0760a50d 100644 --- a/app.go +++ b/app.go @@ -159,6 +159,12 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { correlationOpts = append(correlationOpts, correlation.WithPropagation()) } + // TODO: ideally, this should not be here. Instead, this check should be done before creating the routes. + accessLogger, err := logging.GetAccessLogger(a.config.Log.Format) + if err != nil { + return nil, err + } + router := app_router.NewRouter( rejectmethods.NewMiddleware, func(next http.Handler) http.Handler { @@ -171,12 +177,7 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { return metricsMiddleware(next) }, func(next http.Handler) http.Handler { - logHandler, err := logging.BasicAccessLogger(next, a.config.Log.Format) - if err != nil { - return next - } - - return logHandler + return logging.BasicAccessLogger(next, accessLogger) }, handlePanicMiddleware, func(next http.Handler) http.Handler { diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 6e8533d87..c47c8d3a0 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -34,7 +34,7 @@ func ConfigureLogging(format string, verbose bool) error { // getAccessLogger will return the default logger, except when // the log format is text, in which case a combined HTTP access // logger will be configured. This behaviour matches Workhorse -func getAccessLogger(format string) (*logrus.Logger, error) { +func GetAccessLogger(format string) (*logrus.Logger, error) { if format != "text" && format != "" { return logrus.StandardLogger(), nil } @@ -52,17 +52,13 @@ func getAccessLogger(format string) (*logrus.Logger, error) { } // BasicAccessLogger configures the GitLab pages basic HTTP access logger middleware -func BasicAccessLogger(handler http.Handler, format string) (http.Handler, error) { - accessLogger, err := getAccessLogger(format) - if err != nil { - return nil, err - } - - return log.AccessLogger(handler, +func BasicAccessLogger(handler http.Handler, accessLogger *logrus.Logger) http.Handler { + return log.AccessLogger( + handler, log.WithExtraFields(extraFields), log.WithAccessLogger(accessLogger), log.WithXFFAllowed(func(sip string) bool { return false }), - ), nil + ) } func extraFields(r *http.Request) log.Fields { -- GitLab