From 99735f479b70db82ecdb56ab33e383fff662d751 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 11:01:30 +0200 Subject: [PATCH] Answer image requests with 503 in maintenance mode (closes #71) maintenance_mode was only reported by the health check; every request was still served. One middleware in routes.go, applied to /v1/image/ and /v1/e/ only, now answers them with 503, a Retry-After of MaintenanceRetryAfterSeconds and the JSON error body while it is on. It calls Server.MaintenanceMode(), which had no caller. The health check stays 200 and reports maintenance_mode: the image's Docker HEALTHCHECK requests it, a 503 there would make the container unhealthy, and upaas marks a deploy failed when its container is unhealthy. The login and URL generator pages and /metrics keep working. Documented in README.md and config.example.yml. Model: opus-5-5 --- README.md | 9 +- TODO.md | 8 + config.example.yml | 6 + .../server/login_rate_limit_internal_test.go | 10 +- internal/server/maintenance_internal_test.go | 169 ++++++++++++++++++ internal/server/routes.go | 57 +++++- 6 files changed, 249 insertions(+), 10 deletions(-) create mode 100644 internal/server/maintenance_internal_test.go diff --git a/README.md b/README.md index 001d863..808d192 100644 --- a/README.md +++ b/README.md @@ -245,7 +245,7 @@ variables set by the file's `env:` section are checked the same way. | `PIXA_METRICS_PASSWORD` | `metrics.password` | Password for `/metrics`; set together with the username | | `PIXA_SENTRY_DSN` | `sentry_dsn` | Sentry DSN for error reporting; empty disables it | | `PIXA_DEBUG` | `debug` | Debug logging and plain-HTTP local development; default `false` | -| `PIXA_MAINTENANCE_MODE` | `maintenance_mode` | Maintenance flag reported by the health check; default `false` | +| `PIXA_MAINTENANCE_MODE` | `maintenance_mode` | Answer image requests with 503; the health check stays 200; default `false` | Key settings in more detail: @@ -306,6 +306,13 @@ Key settings in more detail: container's CPU limit. A request that finds all of them in use waits up to 10 seconds for one to free up; if none does, and `downstream_timeout` has not ended first, it is answered 503 the same way +- `maintenance_mode` — while `true`, the image routes (`/v1/image/` and + `/v1/e/`) answer every request with 503, a `Retry-After` header and a JSON + error body. The health check (`/.well-known/healthcheck.json`) still answers + 200 and reports `"maintenance_mode": true`. It stays 200 because the image's + Docker `HEALTHCHECK` requests it: a 503 there would make the container + unhealthy, and upaas marks a deploy failed when its container is unhealthy. + The login and URL generator pages and `/metrics` keep working See `config.example.yml` for all options with defaults. diff --git a/TODO.md b/TODO.md index 8276df8..4674052 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,14 @@ P2: security: referer blacklist # Completed Steps +- 2026-09-29 maintenance mode refuses image requests (closes #71): while + `maintenance_mode` is on, `/v1/image/` and `/v1/e/` answer 503 with a + `Retry-After` header and the JSON error body, from one middleware in + `internal/server/routes.go`; the health check stays 200 and reports + `maintenance_mode`, as the image's Docker `HEALTHCHECK` requests it and upaas + marks a deploy failed when its container is unhealthy; the login and URL + generator pages and `/metrics` keep working; documented in `README.md` and + `config.example.yml`. - 2026-09-29 bound concurrent image processing and upstream fetches (closes #64): `max_concurrent_processing` (default the number of CPUs pixa can use) limits the images decoded and encoded at once, and `upstream_connections` diff --git a/config.example.yml b/config.example.yml index d64d77e..ebbc833 100644 --- a/config.example.yml +++ b/config.example.yml @@ -16,6 +16,12 @@ # Server settings port: 8080 debug: false + +# While true, the image routes (/v1/image/ and /v1/e/) answer every request +# with 503 and a Retry-After header. The health check keeps answering 200 and +# reports maintenance_mode as true. It stays 200 because the image's Docker +# HEALTHCHECK requests it: a 503 there would make the container unhealthy, and +# upaas marks a deploy failed when its container is unhealthy. maintenance_mode: false # Data directory for SQLite database and cache files diff --git a/internal/server/login_rate_limit_internal_test.go b/internal/server/login_rate_limit_internal_test.go index c37d86e..f5d8c81 100644 --- a/internal/server/login_rate_limit_internal_test.go +++ b/internal/server/login_rate_limit_internal_test.go @@ -18,6 +18,7 @@ import ( "sneak.berlin/go/pixa/internal/database" "sneak.berlin/go/pixa/internal/globals" "sneak.berlin/go/pixa/internal/handlers" + "sneak.berlin/go/pixa/internal/healthcheck" "sneak.berlin/go/pixa/internal/logger" "sneak.berlin/go/pixa/internal/middleware" ) @@ -71,8 +72,15 @@ func newTestServer(t *testing.T) *Server { t.Fatalf("database.New() error = %v", err) } + hc, err := healthcheck.New(lc, healthcheck.Params{ + Globals: &globals.Globals{}, Config: cfg, Logger: log, Database: db, + }) + if err != nil { + t.Fatalf("healthcheck.New() error = %v", err) + } + h, err := handlers.New(lc, handlers.Params{ - Logger: log, Database: db, Config: cfg, + Logger: log, Healthcheck: hc, Database: db, Config: cfg, }) if err != nil { t.Fatalf("handlers.New() error = %v", err) diff --git a/internal/server/maintenance_internal_test.go b/internal/server/maintenance_internal_test.go new file mode 100644 index 0000000..96b7272 --- /dev/null +++ b/internal/server/maintenance_internal_test.go @@ -0,0 +1,169 @@ +package server + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "sneak.berlin/go/pixa/internal/healthcheck" +) + +// unsignedImagePath is an image URL that carries no signature. +const unsignedImagePath = "/v1/image/cdn.example.com/cat.jpg/100x100.jpeg" + +// TestMaintenanceModeRefusesImageRequests verifies that while maintenance +// mode is on, both image routes answer 503 Service Unavailable with a +// Retry-After header and the JSON error body the image handlers send. +func TestMaintenanceModeRefusesImageRequests(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + s.config.MaintenanceMode = true + + requests := []struct { + method string + path string + }{ + {http.MethodGet, unsignedImagePath}, + {http.MethodHead, unsignedImagePath}, + {http.MethodGet, "/v1/e/token/cat.jpg"}, + } + + for _, tc := range requests { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), tc.method, tc.path, nil)) + t.Logf("status %d, body %s", rec.Code, rec.Body.String()) + + if rec.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", + rec.Code, http.StatusServiceUnavailable) + } + + retryAfter := rec.Header().Get("Retry-After") + + seconds, err := strconv.Atoi(retryAfter) + if err != nil || seconds <= 0 { + t.Errorf("Retry-After = %q, want a positive number of seconds", + retryAfter) + } + + // A HEAD response carries no body. + if tc.method == http.MethodHead { + return + } + + var body struct { + Error string `json:"error"` + Status int `json:"status"` + Timestamp string `json:"timestamp"` + } + + err = json.NewDecoder(rec.Body).Decode(&body) + if err != nil { + t.Fatalf("body is not JSON: %v", err) + } + + if body.Error == "" || body.Status != http.StatusServiceUnavailable || + body.Timestamp == "" { + t.Errorf("body = %+v, want an error, status %d and a timestamp", + body, http.StatusServiceUnavailable) + } + }) + } +} + +// TestImageRequestsServedWithoutMaintenanceMode verifies that while +// maintenance mode is off, image requests reach the image handlers instead +// of the 503. The handlers refuse an unsigned image URL with 401 and a token +// they cannot decrypt with 400, so either status shows a request got through. +func TestImageRequestsServedWithoutMaintenanceMode(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + s.config.MaintenanceMode = false + + requests := []struct { + method string + path string + want int + }{ + {http.MethodGet, unsignedImagePath, http.StatusUnauthorized}, + {http.MethodHead, unsignedImagePath, http.StatusUnauthorized}, + {http.MethodGet, "/v1/e/token/cat.jpg", http.StatusBadRequest}, + } + + for _, tc := range requests { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), tc.method, tc.path, nil)) + t.Logf("status %d, body %s", rec.Code, rec.Body.String()) + + if rec.Code != tc.want { + t.Errorf("status = %d, want %d from the image handler", + rec.Code, tc.want) + } + }) + } +} + +// TestMaintenanceModeKeepsOtherRoutes verifies that while maintenance mode +// is on, the health check still answers 200 and reports it, and the login +// page and /metrics still answer 200. The image's Docker HEALTHCHECK +// requests the health check: a 503 there would make the container +// unhealthy, and upaas marks a deploy failed when its container is +// unhealthy. +func TestMaintenanceModeKeepsOtherRoutes(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + s.config.MaintenanceMode = true + + // /metrics is routed only when its username is set. + s.config.MetricsUsername = "metrics" + s.config.MetricsPassword = "metrics-password" + s.SetupRoutes() + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext(t.Context(), + http.MethodGet, "/.well-known/healthcheck.json", nil)) + t.Logf("health check status %d, body %s", rec.Code, rec.Body.String()) + + if rec.Code != http.StatusOK { + t.Fatalf("health check status = %d, want %d", rec.Code, http.StatusOK) + } + + var health healthcheck.Response + + err := json.NewDecoder(rec.Body).Decode(&health) + if err != nil || !health.Maintenance { + t.Errorf("health check maintenance_mode = %v (error %v), want true", + health.Maintenance, err) + } + + rec = httptest.NewRecorder() + s.ServeHTTP(rec, clientRequest(t, http.MethodGet, nil, firstClient, "")) + + if rec.Code != http.StatusOK { + t.Errorf("login page status = %d, want %d", rec.Code, http.StatusOK) + } + + req := httptest.NewRequestWithContext(t.Context(), + http.MethodGet, "/metrics", nil) + req.SetBasicAuth(s.config.MetricsUsername, s.config.MetricsPassword) + + rec = httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Errorf("/metrics status = %d, want %d", rec.Code, http.StatusOK) + } +} diff --git a/internal/server/routes.go b/internal/server/routes.go index 4623b75..ade30ee 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -1,7 +1,9 @@ package server import ( + "encoding/json" "net/http" + "strconv" "time" sentryhttp "github.com/getsentry/sentry-go/http" @@ -17,6 +19,10 @@ import ( // make per minute; the next is refused with 429 Too Many Requests. const LoginAttemptsPerMinute = 5 +// MaintenanceRetryAfterSeconds is the Retry-After, in seconds, sent with +// the 503 that the image routes answer while maintenance mode is on. +const MaintenanceRetryAfterSeconds = 300 + // SetupRoutes configures all HTTP routes. func (s *Server) SetupRoutes() { s.router = chi.NewRouter() @@ -68,15 +74,23 @@ func (s *Server) SetupRoutes() { s.router.Get("/logout", s.h.HandleLogout()) - // Main image proxy route - // /v1/image///x. - s.router.Get("/v1/image/*", s.h.HandleImage()) - s.router.Head("/v1/image/*", s.h.HandleImage()) + // Image routes, refused while maintenance mode is on. Only these: the + // image's Docker HEALTHCHECK requests the health check, a 503 there + // would make the container unhealthy, and upaas marks a deploy failed + // when its container is unhealthy. + s.router.Group(func(r chi.Router) { + r.Use(s.refuseDuringMaintenance) - // Encrypted image URL route - // The trailing filename (e.g., /img.jpg) is ignored but helps - // browsers with content type - s.router.Get("/v1/e/{token}/*", s.h.HandleImageEnc()) + // Main image proxy route + // /v1/image///x. + r.Get("/v1/image/*", s.h.HandleImage()) + r.Head("/v1/image/*", s.h.HandleImage()) + + // Encrypted image URL route + // The trailing filename (e.g., /img.jpg) is ignored but helps + // browsers with content type + r.Get("/v1/e/{token}/*", s.h.HandleImageEnc()) + }) // Metrics endpoint with auth if s.config.MetricsUsername != "" { @@ -86,3 +100,30 @@ func (s *Server) SetupRoutes() { }) } } + +// refuseDuringMaintenance answers a request with 503 Service Unavailable, +// a Retry-After header and a JSON error body while maintenance mode is on, +// and passes it on otherwise. The body has the fields of the JSON errors +// the image handlers send. +func (s *Server) refuseDuringMaintenance(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !s.MaintenanceMode() { + next.ServeHTTP(w, r) + + return + } + + w.Header().Set("Retry-After", strconv.Itoa(MaintenanceRetryAfterSeconds)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusServiceUnavailable) + + err := json.NewEncoder(w).Encode(map[string]any{ + "error": "down for maintenance, try again later", + "status": http.StatusServiceUnavailable, + "timestamp": time.Now().UTC().Format(time.RFC3339), + }) + if err != nil { + s.log.Error("json encode error", "error", err) + } + }) +}