Extract health check in its own middleware

This way we short-circuit health check requests and avoid doing domain
lookup for them. We also do not report them in exported Prometheus
metrics and this way avoid trigger alerts during deploys.

Related to:

* https://gitlab.com/gitlab-org/gitlab-pages/issues/350
* https://gitlab.com/gitlab-com/gl-infra/production/issues/1681

3 files changed, 101 insertions, 11 deletions

diff --git a/app.go b/app.go
index 2626be49..22264eb8 100644
--- a/app.go
+++ b/app.go
@@ -124,12 +124,6 @@ func (a *theApp) checkAuthenticationIfNotExists(domain *domain.Domain, w http.Re
 }
 
 func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, https bool, host string, domain *domain.Domain) bool {
-	// short circuit content serving to check for a status page
-	if r.RequestURI == a.appConfig.StatusPath {
-		a.healthCheck(w, r, https)
-		return true
-	}
-
 	// Add auto redirect
 	if !https && a.RedirectHTTP {
 		a.redirectToHTTPS(w, r, http.StatusTemporaryRedirect)
@@ -177,6 +171,27 @@ func (a *theApp) routingMiddleware(handler http.Handler) http.Handler {
 	})
 }
 
+// healthCheckMiddleware is serving the application status check
+func (a *theApp) healthCheckMiddleware(handler http.Handler) (http.Handler, error) {
+	healthCheck := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		a.healthCheck(w, r, request.IsHTTPS(r))
+	})
+
+	loggedHealthCheck, err := logging.BasicAccessLogger(healthCheck, a.LogFormat, nil)
+	if err != nil {
+		return nil, err
+	}
+
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.RequestURI == a.appConfig.StatusPath {
+			loggedHealthCheck.ServeHTTP(w, r)
+			return
+		}
+
+		handler.ServeHTTP(w, r)
+	}), nil
+}
+
 // customHeadersMiddleware will inject custom headers into the response
 func (a *theApp) customHeadersMiddleware(handler http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -310,7 +325,6 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) {
 	handler = a.auxiliaryMiddleware(handler)
 	handler = a.authMiddleware(handler)
 	handler = a.acmeMiddleware(handler)
-	handler = a.customHeadersMiddleware(handler)
 	handler, err := logging.AccessLogger(handler, a.LogFormat)
 	if err != nil {
 		return nil, err
@@ -322,6 +336,15 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) {
 
 	handler = a.routingMiddleware(handler)
 
+	// Health Check
+	handler, err = a.healthCheckMiddleware(handler)
+	if err != nil {
+		return nil, err
+	}
+
+	// Custom response headers
+	handler = a.customHeadersMiddleware(handler)
+
 	return handler, nil
 }
 
diff --git a/app_test.go b/app_test.go
index d78d6737..3c485804 100644
--- a/app_test.go
+++ b/app_test.go
@@ -3,12 +3,15 @@ package main
 import (
 	"crypto/tls"
 	"fmt"
+	"io"
 	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source"
 )
 
 func Test_setRequestScheme(t *testing.T) {
@@ -58,3 +61,54 @@ func newGetRequestWithScheme(t *testing.T, scheme string, withTLS bool) *http.Re
 
 	return req
 }
+
+func TestHealthCheckMiddleware(t *testing.T) {
+	tests := []struct {
+		name   string
+		path   string
+		status int
+		body   string
+	}{
+		{
+			name:   "Not a healthcheck request",
+			path:   "/foo/bar",
+			status: http.StatusOK,
+			body:   "Hello from inner handler",
+		},
+		{
+			name:   "Healthcheck request",
+			path:   "/-/healthcheck",
+			status: http.StatusServiceUnavailable,
+			body:   "not yet ready\n",
+		},
+	}
+
+	app := theApp{
+		appConfig: appConfig{
+			StatusPath: "/-/healthcheck",
+		},
+	}
+
+	domains, err := source.NewDomains(app.appConfig)
+	require.NoError(t, err)
+	app.domains = domains
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		io.WriteString(w, "Hello from inner handler")
+	})
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			r := httptest.NewRequest("GET", tc.path, nil)
+			rr := httptest.NewRecorder()
+
+			middleware, err := app.healthCheckMiddleware(handler)
+			require.NoError(t, err)
+			middleware.ServeHTTP(rr, r)
+
+			require.Equal(t, tc.status, rr.Code)
+			require.Equal(t, tc.body, rr.Body.String())
+		})
+	}
+}
diff --git a/internal/logging/logging.go b/internal/logging/logging.go
index 28c43c2e..4ba86985 100644
--- a/internal/logging/logging.go
+++ b/internal/logging/logging.go
@@ -65,20 +65,33 @@ func getExtraLogFields(r *http.Request) log.Fields {
 	}
 }
 
-// AccessLogger configures the GitLab pages HTTP access logger middleware
-func AccessLogger(handler http.Handler, format string) (http.Handler, error) {
-
+// BasicAccessLogger configures the GitLab pages basic HTTP access logger middleware
+func BasicAccessLogger(handler http.Handler, format string, extraFields log.ExtraFieldsGeneratorFunc) (http.Handler, error) {
 	accessLogger, err := getAccessLogger(format)
 	if err != nil {
 		return nil, err
 	}
 
+	if extraFields == nil {
+		extraFields = func(r *http.Request) log.Fields {
+			return log.Fields{
+				"pages_https": request.IsHTTPS(r),
+				"pages_host":  r.Host,
+			}
+		}
+	}
+
 	return log.AccessLogger(handler,
-		log.WithExtraFields(getExtraLogFields),
+		log.WithExtraFields(extraFields),
 		log.WithAccessLogger(accessLogger),
 	), nil
 }
 
+// AccessLogger configures the GitLab pages HTTP access logger middleware with extra log fields
+func AccessLogger(handler http.Handler, format string) (http.Handler, error) {
+	return BasicAccessLogger(handler, format, getExtraLogFields)
+}
+
 // LogRequest will inject request host and path to the logged messages
 func LogRequest(r *http.Request) *logrus.Entry {
 	return log.WithFields(log.Fields{