Merge branch '350-healthcheck-middleware' into 'master'

Extract health check in its own middleware

See merge request gitlab-org/gitlab-pages!247

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{