Merge branch 'master' into feature/gb/serverless-serving-enable

* master:
  Add prometheus metrics for GitLab API client
  Fix benchmarks
  Freeze tools version
  Add acceptance test for serverless metrics
  Update documentation on using Gorilla ProxyHeaders
  use gorilla/handlers.ProxyHeaders to get the X-Forwarded-* headers and set them in the appropriate http.Request fields
  Apply suggestion to metrics/metrics.go
  Add serverless serving metrics

Conflicts:
	internal/serving/serverless/serverless.go

10 files changed, 273 insertions, 32 deletions

diff --git a/internal/httptransport/transport.go b/internal/httptransport/transport.go
index 6d946ae0..ba2aa5eb 100644
--- a/internal/httptransport/transport.go
+++ b/internal/httptransport/transport.go
@@ -7,8 +7,11 @@ import (
 	"net"
 	"net/http"
 	"os"
+	"strconv"
 	"sync"
+	"time"
 
+	"github.com/prometheus/client_golang/prometheus"
 	log "github.com/sirupsen/logrus"
 )
 
@@ -16,7 +19,7 @@ var (
 	sysPoolOnce = &sync.Once{}
 	sysPool     *x509.CertPool
 
-	// Transport can be used with httpclient with TLS and certificates
+	// Transport can be used with http.Client with TLS and certificates
 	Transport = &http.Transport{
 		DialTLS: func(network, addr string) (net.Conn, error) {
 			return tls.Dial(network, addr, &tls.Config{RootCAs: pool()})
@@ -25,6 +28,22 @@ var (
 	}
 )
 
+type meteredRoundTripper struct {
+	next      http.RoundTripper
+	durations *prometheus.GaugeVec
+	counter   *prometheus.CounterVec
+}
+
+// NewTransportWithMetrics will create a custom http.RoundTripper that can be used with an http.Client.
+// The RoundTripper will report metrics based on the collectors passed.
+func NewTransportWithMetrics(gaugeVec *prometheus.GaugeVec, counterVec *prometheus.CounterVec) http.RoundTripper {
+	return &meteredRoundTripper{
+		next:      Transport,
+		durations: gaugeVec,
+		counter:   counterVec,
+	}
+}
+
 // This is here because macOS does not support the SSL_CERT_FILE
 // environment variable. We have arrange things to read SSL_CERT_FILE as
 // late as possible to avoid conflicts with file descriptor passing at
@@ -55,3 +74,21 @@ func loadPool() {
 
 	sysPool.AppendCertsFromPEM(certPem)
 }
+
+// withRoundTripper takes an original RoundTripper, reports metrics based on the
+// gauge and counter collectors passed
+func (mrt *meteredRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
+	start := time.Now()
+
+	resp, err := mrt.next.RoundTrip(r)
+	if err != nil {
+		mrt.counter.WithLabelValues("error").Inc()
+		return nil, err
+	}
+
+	statusCode := strconv.Itoa(resp.StatusCode)
+	mrt.durations.WithLabelValues(statusCode).Set(time.Since(start).Seconds())
+	mrt.counter.WithLabelValues(statusCode).Inc()
+
+	return resp, nil
+}
diff --git a/internal/httptransport/transport_test.go b/internal/httptransport/transport_test.go
new file mode 100644
index 00000000..cfb8d708
--- /dev/null
+++ b/internal/httptransport/transport_test.go
@@ -0,0 +1,91 @@
+package httptransport
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+	"testing"
+
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/testutil"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_withRoundTripper(t *testing.T) {
+
+	tests := []struct {
+		name       string
+		statusCode int
+		err        error
+	}{
+		{
+			name:       "successful_response",
+			statusCode: http.StatusNoContent,
+		},
+		{
+			name:       "error_response",
+			statusCode: http.StatusForbidden,
+		},
+		{
+			name:       "internal_error_response",
+			statusCode: http.StatusInternalServerError,
+		},
+		{
+			name:       "unhandled_status_response",
+			statusCode: http.StatusPermanentRedirect,
+		},
+		{
+			name: "client_error",
+			err:  fmt.Errorf("something went wrong"),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gaugeVec := prometheus.NewGaugeVec(prometheus.GaugeOpts{
+				Name: t.Name(),
+			}, []string{"status_code"})
+
+			counterVec := prometheus.NewCounterVec(prometheus.CounterOpts{
+				Name: t.Name(),
+			}, []string{"status_code"})
+
+			next := &mockRoundTripper{
+				res: &http.Response{
+					StatusCode: tt.statusCode,
+				},
+				err: tt.err,
+			}
+
+			mtr := &meteredRoundTripper{next, gaugeVec, counterVec}
+			r := httptest.NewRequest("GET", "/", nil)
+
+			res, err := mtr.RoundTrip(r)
+			if tt.err != nil {
+				counterCount := testutil.ToFloat64(counterVec.WithLabelValues("error"))
+				require.Equal(t, float64(1), counterCount, "error")
+
+				return
+			}
+			require.NoError(t, err)
+			require.NotNil(t, res)
+
+			statusCode := strconv.Itoa(res.StatusCode)
+			gaugeValue := testutil.ToFloat64(gaugeVec.WithLabelValues(statusCode))
+			require.Greater(t, gaugeValue, float64(0))
+
+			counterCount := testutil.ToFloat64(counterVec.WithLabelValues(statusCode))
+			require.Equal(t, float64(1), counterCount, statusCode)
+		})
+	}
+}
+
+type mockRoundTripper struct {
+	res *http.Response
+	err error
+}
+
+func (mrt *mockRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
+	return mrt.res, mrt.err
+}
diff --git a/internal/request/request.go b/internal/request/request.go
index 06a45071..ba56f45b 100644
--- a/internal/request/request.go
+++ b/internal/request/request.go
@@ -5,6 +5,8 @@ import (
 	"net"
 	"net/http"
 
+	log "github.com/sirupsen/logrus"
+
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 )
 
@@ -14,6 +16,11 @@ const (
 	ctxHTTPSKey  ctxKey = "https"
 	ctxHostKey   ctxKey = "host"
 	ctxDomainKey ctxKey = "domain"
+
+	// SchemeHTTP name for the HTTP scheme
+	SchemeHTTP = "http"
+	// SchemeHTTPS name for the HTTPS scheme
+	SchemeHTTPS = "https"
 )
 
 // WithHTTPSFlag saves https flag in request's context
@@ -23,9 +30,23 @@ func WithHTTPSFlag(r *http.Request, https bool) *http.Request {
 	return r.WithContext(ctx)
 }
 
-// IsHTTPS restores https flag from request's context
+// IsHTTPS checks whether the request originated from HTTP or HTTPS.
+// It reads the ctxHTTPSKey from the context and returns its value
+// It also checks that r.URL.Scheme matches the value in ctxHTTPSKey for HTTPS requests
+// TODO: remove the ctxHTTPSKey from the context https://gitlab.com/gitlab-org/gitlab-pages/issues/219
 func IsHTTPS(r *http.Request) bool {
-	return r.Context().Value(ctxHTTPSKey).(bool)
+	https := r.Context().Value(ctxHTTPSKey).(bool)
+
+	if https != (r.URL.Scheme == SchemeHTTPS) {
+		log.WithFields(log.Fields{
+			"ctxHTTPSKey": https,
+			"scheme":      r.URL.Scheme,
+		}).Warn("request: r.URL.Scheme does not match value in ctxHTTPSKey")
+	}
+
+	// Returning the value of ctxHTTPSKey for now, can just switch to r.URL.Scheme == SchemeHTTPS later
+	// and later can remove IsHTTPS method altogether
+	return https
 }
 
 // WithHostAndDomain saves host name and domain in the request's context
diff --git a/internal/request/request_test.go b/internal/request/request_test.go
index 642209f0..3c0f970c 100644
--- a/internal/request/request_test.go
+++ b/internal/request/request_test.go
@@ -5,9 +5,11 @@ import (
 	"net/http/httptest"
 	"testing"
 
+	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
 )
 
 func TestWithHTTPSFlag(t *testing.T) {
@@ -15,10 +17,65 @@ func TestWithHTTPSFlag(t *testing.T) {
 	require.NoError(t, err)
 
 	httpsRequest := WithHTTPSFlag(r, true)
+	httpsRequest.URL.Scheme = SchemeHTTPS
 	require.True(t, IsHTTPS(httpsRequest))
 
 	httpRequest := WithHTTPSFlag(r, false)
+	httpsRequest.URL.Scheme = SchemeHTTP
 	require.False(t, IsHTTPS(httpRequest))
+
+}
+
+func TestIsHTTPS(t *testing.T) {
+	hook := test.NewGlobal()
+
+	tests := []struct {
+		name           string
+		flag           bool
+		scheme         string
+		wantLogEntries string
+	}{
+		{
+			name:   "https",
+			flag:   true,
+			scheme: "https",
+		},
+		{
+			name:   "http",
+			flag:   false,
+			scheme: "http",
+		},
+		{
+			name:           "scheme true but flag is false",
+			flag:           false,
+			scheme:         "https",
+			wantLogEntries: "request: r.URL.Scheme does not match value in ctxHTTPSKey",
+		},
+		{
+			name:           "scheme false but flag is true",
+			flag:           true,
+			scheme:         "http",
+			wantLogEntries: "request: r.URL.Scheme does not match value in ctxHTTPSKey",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			hook.Reset()
+
+			r, err := http.NewRequest("GET", "/", nil)
+			require.NoError(t, err)
+			r.URL.Scheme = tt.scheme
+
+			httpsRequest := WithHTTPSFlag(r, tt.flag)
+
+			got := IsHTTPS(httpsRequest)
+			require.Equal(t, tt.flag, got)
+
+			testhelpers.AssertLogContains(t, tt.wantLogEntries, hook.AllEntries())
+		})
+	}
+
 }
 
 func TestPanics(t *testing.T) {
diff --git a/internal/serving/serverless/serverless.go b/internal/serving/serverless/serverless.go
index 3c360054..bec6db16 100644
--- a/internal/serving/serverless/serverless.go
+++ b/internal/serving/serverless/serverless.go
@@ -7,6 +7,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
+	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
 // Serverless is a servering used to proxy requests between a client and
@@ -55,6 +56,8 @@ func New(function Function, cluster Cluster) serving.Serving {
 
 // ServeFileHTTP handle an incoming request and proxies it to Knative cluster
 func (s *Serverless) ServeFileHTTP(h serving.Handler) bool {
+	metrics.ServerlessRequests.Inc()
+
 	s.proxy.ServeHTTP(h.Writer, h.Request)
 
 	return true
diff --git a/internal/serving/serverless/transport.go b/internal/serving/serverless/transport.go
index 5a0f5165..b7fabb13 100644
--- a/internal/serving/serverless/transport.go
+++ b/internal/serving/serverless/transport.go
@@ -5,6 +5,8 @@ import (
 	"net"
 	"net/http"
 	"time"
+
+	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
 // Transport is a struct that handle the proxy connection round trip to Knative
@@ -39,8 +41,11 @@ func NewTransport(cluster Cluster) *Transport {
 
 // RoundTrip performs a connection to a Knative cluster and returns a response
 func (t *Transport) RoundTrip(request *http.Request) (*http.Response, error) {
+	start := time.Now()
+
 	response, err := t.transport.RoundTrip(request)
 
-	// TODO add prometheus metrics for round trip timing
+	metrics.ServerlessLatency.Observe(time.Since(start).Seconds())
+
 	return response, err
 }
diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go
index bae4b764..5c053fcb 100644
--- a/internal/source/disk/map.go
+++ b/internal/source/disk/map.go
@@ -252,7 +252,7 @@ func Watch(rootDomain string, updater domainsUpdater, interval time.Duration) {
 		fis, err := godirwalk.ReadDirents(".", nil)
 		if err != nil {
 			log.WithError(err).Warn("domain scan failed")
-			metrics.FailedDomainUpdates.Inc()
+			metrics.DomainFailedUpdates.Inc()
 			continue
 		}
 
diff --git a/internal/source/disk/map_test.go b/internal/source/disk/map_test.go
index c15f29c6..d904e6aa 100644
--- a/internal/source/disk/map_test.go
+++ b/internal/source/disk/map_test.go
@@ -13,15 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func getEntries(t *testing.T) godirwalk.Dirents {
-	fis, err := godirwalk.ReadDirents(".", nil)
-
-	require.NoError(t, err)
-
-	return fis
-}
-
-func getEntriesForBenchmark(t *testing.B) godirwalk.Dirents {
+func getEntries(t require.TestingT) godirwalk.Dirents {
 	fis, err := godirwalk.ReadDirents(".", nil)
 
 	require.NoError(t, err)
@@ -199,7 +191,7 @@ func buildFakeDomainsDirectory(t require.TestingT, nGroups, levels int) func() {
 			domain = fmt.Sprintf("%d.%s", j, domain)
 			buildFakeProjectsDirectory(t, parent, domain)
 		}
-		if i%100 == 0 {
+		if testing.Verbose() && i%100 == 0 {
 			fmt.Print(".")
 		}
 	}
@@ -208,7 +200,11 @@ func buildFakeDomainsDirectory(t require.TestingT, nGroups, levels int) func() {
 
 	return func() {
 		defer cleanup()
-		fmt.Printf("cleaning up test directory %s\n", testRoot)
+
+		if testing.Verbose() {
+			fmt.Printf("cleaning up test directory %s\n", testRoot)
+		}
+
 		os.RemoveAll(testRoot)
 	}
 }
@@ -223,18 +219,30 @@ func buildFakeProjectsDirectory(t require.TestingT, groupPath, domain string) {
 	}
 }
 
-func BenchmarkReadGroups(b *testing.B) {
-	nGroups := 10000
-	b.Logf("creating fake domains directory with %d groups", nGroups)
-	cleanup := buildFakeDomainsDirectory(b, nGroups, 0)
+// this is a safeguard against compiler optimizations
+// we use this package variable to make sure the benchmarkReadGroups loop
+// has side effects outside of the loop.
+// Without this the compiler (with the optimizations enabled) may remove the whole loop
+var result int
+
+func benchmarkReadGroups(b *testing.B, groups, levels int) {
+	cleanup := buildFakeDomainsDirectory(b, groups, levels)
 	defer cleanup()
 
-	b.Run("ReadGroups", func(b *testing.B) {
-		var dm Map
-		for i := 0; i < 2; i++ {
-			dm = make(Map)
-			dm.ReadGroups("example.com", getEntriesForBenchmark(b))
-		}
-		b.Logf("found %d domains", len(dm))
-	})
+	b.ResetTimer()
+
+	domainsCnt := 0
+	for i := 0; i < b.N; i++ {
+		dm := make(Map)
+		dm.ReadGroups("example.com", getEntries(b))
+		domainsCnt = len(dm)
+	}
+	result = domainsCnt
+}
+
+func BenchmarkReadGroups(b *testing.B) {
+	b.Run("10 groups 3 levels", func(b *testing.B) { benchmarkReadGroups(b, 10, 3) })
+	b.Run("100 groups 3 levels", func(b *testing.B) { benchmarkReadGroups(b, 100, 3) })
+	b.Run("1000 groups 3 levels", func(b *testing.B) { benchmarkReadGroups(b, 1000, 3) })
+	b.Run("10000 groups 1 levels", func(b *testing.B) { benchmarkReadGroups(b, 10000, 1) })
 }
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 59776a8f..afe9da6f 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -11,10 +11,11 @@ import (
 	"net/url"
 	"time"
 
-	jwt "github.com/dgrijalva/jwt-go"
+	"github.com/dgrijalva/jwt-go"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httptransport"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
+	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
 // Client is a HTTP client to access Pages internal API
@@ -32,7 +33,7 @@ func NewClient(baseURL string, secretKey []byte, connectionTimeout, jwtTokenExpi
 		return nil, errors.New("GitLab API URL or API secret has not been provided")
 	}
 
-	url, err := url.Parse(baseURL)
+	parsedURL, err := url.Parse(baseURL)
 	if err != nil {
 		return nil, err
 	}
@@ -47,10 +48,10 @@ func NewClient(baseURL string, secretKey []byte, connectionTimeout, jwtTokenExpi
 
 	return &Client{
 		secretKey: secretKey,
-		baseURL:   url,
+		baseURL:   parsedURL,
 		httpClient: &http.Client{
 			Timeout:   connectionTimeout,
-			Transport: httptransport.Transport,
+			Transport: httptransport.NewTransportWithMetrics(metrics.DomainsSourceAPICallDuration, metrics.DomainsSourceAPIReqTotal),
 		},
 		jwtTokenExpiry: jwtTokenExpiry,
 	}, nil
@@ -115,6 +116,8 @@ func (gc *Client) get(ctx context.Context, path string, params url.Values) (*htt
 		return resp, nil
 	}
 
+	// nolint: errcheck
+	// best effort to discard and close the response body
 	io.Copy(ioutil.Discard, resp.Body)
 	resp.Body.Close()
 
diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go
index c0b87d93..d703769b 100644
--- a/internal/testhelpers/testhelpers.go
+++ b/internal/testhelpers/testhelpers.go
@@ -7,6 +7,7 @@ import (
 	"net/url"
 	"testing"
 
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 )
 
@@ -40,4 +41,19 @@ func AssertRedirectTo(t *testing.T, handler http.HandlerFunc, method string,
 	handler(recorder, req)
 
 	require.Equal(t, expectedURL, recorder.Header().Get("Location"))
+
+}
+
+// AssertLogContains checks that wantLogEntry is contained in at least one of the log entries
+func AssertLogContains(t *testing.T, wantLogEntry string, entries []*logrus.Entry) {
+	t.Helper()
+
+	if wantLogEntry != "" {
+		messages := make([]string, len(entries))
+		for k, entry := range entries {
+			messages[k] = entry.Message
+		}
+
+		require.Contains(t, messages, wantLogEntry)
+	}
 }