Merge pull request #380 from alphagov/sengi/testability

Move Router code out of the main module etc.

.dockerignore

@@ -1,6 +1,8 @@
-.git
-.gitignore
-Dockerfile
-README.md
-docs
-integration_tests
+#
+# Only files that are untracked by Git should be added here.
+#
+# The builder container needs to see a pristine checkout, otherwise
+# vcs.modified in the BuildInfo will always be true, i.e. the build will always
+# be marked as "dirty".
+#
+/router

.gitignore

@@ -1,2 +1,2 @@
-router
+/router
 __build

.golangci.yml

@@ -20,9 +20,15 @@ linters:
     - promlinter
     - reassign
     - revive
+    - stylecheck
     - tenv
     - tparallel
     - unconvert
     - usestdlibvars
     - wastedassign
     - zerologlint
+linters-settings:
+  stylecheck:
+    dot-import-whitelist:
+      - github.com/onsi/ginkgo/v2
+      - github.com/onsi/gomega

Dockerfile

@@ -1,8 +1,22 @@
-FROM golang:1.20-alpine AS builder
+ARG go_registry=""
+ARG go_version=1.20
+ARG go_tag_suffix=-alpine
+
+FROM ${go_registry}golang:${go_version}${go_tag_suffix} AS builder
 ARG TARGETARCH TARGETOS
+ARG GOARCH=$TARGETARCH GOOS=$TARGETOS
+ARG CGO_ENABLED=0
+ARG GOFLAGS="-trimpath"
+ARG go_ldflags="-s -w"
+# Go needs git for `-buildvcs`, but the alpine version lacks git :( It's still
+# way cheaper to `apk add git` than to pull the Debian-based golang image.
+# hadolint ignore=DL3018
+RUN apk add --no-cache git
 WORKDIR /src
 COPY . ./
-RUN CGO_ENABLED=0 GOARCH=$TARGETARCH GOOS=$TARGETOS go build -trimpath -ldflags="-s -w"
+RUN go build -ldflags="$go_ldflags" && \
+    ./router -version && \
+    go version -m ./router
 
 FROM scratch
 COPY --from=builder /src/router /bin/router

Makefile

@@ -1,53 +1,35 @@
-.PHONY: all build test unit_tests integration_tests clean start_mongo clean_mongo clean_mongo_again
+.PHONY: all clean build test lint unit_tests integration_tests start_mongo stop_mongo update_deps
+.NOTPARALLEL:
 
-BINARY ?= router
-SHELL := /bin/bash
+TARGET_MODULE := router
+GO_BUILD_ENV := CGO_ENABLED=0
+SHELL := /bin/dash
 
-ifdef RELEASE_VERSION
-VERSION := $(RELEASE_VERSION)
-else
-VERSION := $(shell git describe --always | tr -d '\n'; test -z "`git status --porcelain`" || echo '-dirty')
-endif
-
-all: build test
+all: build
 
 clean:
-	rm -f $(BINARY)
+	rm -f $(TARGET_MODULE)
 
 build:
-	go build -ldflags "-X main.version=$(VERSION)" -o $(BINARY)
+	env $(GO_BUILD_ENV) go build
+	./$(TARGET_MODULE) -version
+
+test: lint unit_tests integration_tests
 
 lint:
 	golangci-lint run
 
-test: start_mongo unit_tests integration_tests clean_mongo_again
-
-unit_tests: build
+unit_tests:
 	go test -race $$(go list ./... | grep -v integration_tests)
 
-integration_tests: start_mongo build
-	ROUTER_PUBADDR=localhost:8080 \
-	ROUTER_APIADDR=localhost:8081 \
-		go test -race -v ./integration_tests
-
-start_mongo: clean_mongo
-	@if ! docker run --rm --name router-mongo -dp 27017:27017 mongo:2.4 --replSet rs0 --quiet; then \
-		echo 'Failed to start mongo; if using Docker Desktop, try:' ; \
-		echo ' - disabling Settings -> Features in development -> Use containerd' ; \
-		echo ' - enabling Settings -> Features in development -> Use Rosetta' ; \
-		exit 1 ; \
-	fi
-	@echo -n Waiting for mongo
-	@for n in {1..30}; do \
-		if docker exec router-mongo mongo --quiet --eval 'rs.initiate()' >/dev/null 2>&1; then \
-			sleep 1; \
-			echo ; \
-			break ; \
-		fi ; \
-		echo -n . ; \
-		sleep 1 ; \
-	done ; \
-
-clean_mongo clean_mongo_again:
-	docker rm -f router-mongo >/dev/null 2>&1 || true
-	@sleep 1  # Docker doesn't queue commands so it races with itself :(
+integration_tests: build start_mongo
+	go test -race -v ./integration_tests
+
+start_mongo:
+	./mongo.sh start
+
+stop_mongo:
+	./mongo.sh stop
+
+update_deps:
+	go get -t -u ./... && go mod tidy && go mod vendor

README.md

@@ -59,17 +59,17 @@ make lint
 
 ### Debug output
 
-To see debug messages when running tests, set both the `DEBUG` and
-`DEBUG_ROUTER` environment variables.
+To see debug messages when running tests, set both the `ROUTER_DEBUG` and
+`ROUTER_DEBUG_TESTS` environment variables:
 
 ```sh
-export DEBUG=1 DEBUG_ROUTER=1
+export ROUTER_DEBUG=1 ROUTER_DEBUG_TESTS=1
 ```
 
 or equivalently for a single run:
 
 ```sh
-DEBUG=1 DEBUG_ROUTER=1 make test
+ROUTER_DEBUG=1 ROUTER_DEBUG_TESTS=1 make test
 ```
 
 ### Update the dependencies
@@ -79,7 +79,7 @@ This project uses [Go Modules](https://github.com/golang/go/wiki/Modules) to ven
 1. Update all the dependencies, including test dependencies, in your working copy:
 
     ```sh
-    go get -t -u ./... && go mod tidy && go mod vendor
+    make update_deps
     ```
 
 1. Check for any errors and commit.

handlers/backend_handler.go

@@ -134,15 +134,15 @@ func (bt *backendTransport) RoundTrip(req *http.Request) (resp *http.Response, e
 	var responseCode int
 	var startTime = time.Now()
 
-	BackendHandlerRequestCountMetric.With(prometheus.Labels{
+	backendRequestCountMetric.With(prometheus.Labels{
 		"backend_id":     bt.backendID,
 		"request_method": req.Method,
 	}).Inc()
 
 	defer func() {
 		durationSeconds := time.Since(startTime).Seconds()
 
-		BackendHandlerResponseDurationSecondsMetric.With(prometheus.Labels{
+		backendResponseDurationSecondsMetric.With(prometheus.Labels{
 			"backend_id":     bt.backendID,
 			"request_method": req.Method,
 			"response_code":  fmt.Sprintf("%d", responseCode),

handlers/backend_handler_test.go

@@ -1,4 +1,4 @@
-package handlers_test
+package handlers
 
 import (
 	"io"
@@ -15,7 +15,6 @@ import (
 	promtest "github.com/prometheus/client_golang/prometheus/testutil"
 	prommodel "github.com/prometheus/client_model/go"
 
-	"github.com/alphagov/router/handlers"
 	log "github.com/alphagov/router/logger"
 )
 
@@ -51,7 +50,7 @@ var _ = Describe("Backend handler", func() {
 
 	Context("when the backend times out", func() {
 		BeforeEach(func() {
-			router = handlers.NewBackendHandler(
+			router = NewBackendHandler(
 				"backend-timeout",
 				backendURL,
 				timeout, timeout,
@@ -80,7 +79,7 @@ var _ = Describe("Backend handler", func() {
 
 	Context("when the backend handles the connection", func() {
 		BeforeEach(func() {
-			router = handlers.NewBackendHandler(
+			router = NewBackendHandler(
 				"backend-handle",
 				backendURL,
 				timeout, timeout,
@@ -141,15 +140,14 @@ var _ = Describe("Backend handler", func() {
 
 	Context("metrics", func() {
 		var (
-			beforeRequestCountMetric float64
-
+			beforeRequestCountMetric            float64
 			beforeResponseCountMetric           float64
 			beforeResponseDurationSecondsMetric float64
 		)
 
 		measureRequestCount := func() float64 {
 			return promtest.ToFloat64(
-				handlers.BackendHandlerRequestCountMetric.With(prometheus.Labels{
+				backendRequestCountMetric.With(prometheus.Labels{
 					"backend_id":     "backend-metrics",
 					"request_method": http.MethodGet,
 				}),
@@ -160,7 +158,7 @@ var _ = Describe("Backend handler", func() {
 			var err error
 			metricChan := make(chan prometheus.Metric, 1024)
 
-			handlers.BackendHandlerResponseDurationSecondsMetric.Collect(metricChan)
+			backendResponseDurationSecondsMetric.Collect(metricChan)
 			close(metricChan)
 			for m := range metricChan {
 				metric := new(prommodel.Metric)
@@ -197,7 +195,7 @@ var _ = Describe("Backend handler", func() {
 		}
 
 		BeforeEach(func() {
-			router = handlers.NewBackendHandler(
+			router = NewBackendHandler(
 				"backend-metrics",
 				backendURL,
 				timeout, timeout,

handlers/handlers_suite_test.go

@@ -1,4 +1,4 @@
-package handlers_test
+package handlers
 
 import (
 	"testing"

handlers/metrics.go

@@ -5,32 +5,32 @@ import (
 )
 
 var (
-	RedirectHandlerRedirectCountMetric = prometheus.NewCounterVec(
+	redirectCountMetric = prometheus.NewCounterVec(
 		prometheus.CounterOpts{
 			Name: "router_redirect_handler_redirect_total",
-			Help: "Number of redirects handled by router redirect handlers",
+			Help: "Number of redirects served by redirect handlers",
 		},
 		[]string{
 			"redirect_code",
 			"redirect_type",
 		},
 	)
 
-	BackendHandlerRequestCountMetric = prometheus.NewCounterVec(
+	backendRequestCountMetric = prometheus.NewCounterVec(
 		prometheus.CounterOpts{
 			Name: "router_backend_handler_request_total",
-			Help: "Number of requests handled by router backend handlers",
+			Help: "Number of requests served by backend handlers",
 		},
 		[]string{
 			"backend_id",
 			"request_method",
 		},
 	)
 
-	BackendHandlerResponseDurationSecondsMetric = prometheus.NewHistogramVec(
+	backendResponseDurationSecondsMetric = prometheus.NewHistogramVec(
 		prometheus.HistogramOpts{
 			Name: "router_backend_handler_response_duration_seconds",
-			Help: "Histogram of response durations by router backend handlers",
+			Help: "Histogram of response durations by backend",
 		},
 		[]string{
 			"backend_id",
@@ -40,9 +40,10 @@ var (
 	)
 )
 
-func initMetrics() {
-	prometheus.MustRegister(RedirectHandlerRedirectCountMetric)
-
-	prometheus.MustRegister(BackendHandlerRequestCountMetric)
-	prometheus.MustRegister(BackendHandlerResponseDurationSecondsMetric)
+func RegisterMetrics(r prometheus.Registerer) {
+	r.MustRegister(
+		backendRequestCountMetric,
+		backendResponseDurationSecondsMetric,
+		redirectCountMetric,
+	)
 }

handlers/redirect_handler.go

@@ -61,7 +61,7 @@ func (handler *redirectHandler) ServeHTTP(writer http.ResponseWriter, request *h
 	target := addGAQueryParam(handler.url, request)
 	http.Redirect(writer, request, target, handler.code)
 
-	RedirectHandlerRedirectCountMetric.With(prometheus.Labels{
+	redirectCountMetric.With(prometheus.Labels{
 		"redirect_code": fmt.Sprintf("%d", handler.code),
 		"redirect_type": redirectHandlerType,
 	}).Inc()
@@ -82,7 +82,7 @@ func (handler *pathPreservingRedirectHandler) ServeHTTP(writer http.ResponseWrit
 	addCacheHeaders(writer)
 	http.Redirect(writer, request, target, handler.code)
 
-	RedirectHandlerRedirectCountMetric.With(prometheus.Labels{
+	redirectCountMetric.With(prometheus.Labels{
 		"redirect_code": fmt.Sprintf("%d", handler.code),
 		"redirect_type": pathPreservingRedirectHandlerType,
 	}).Inc()