diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 0dda76149..9e52c3054 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -2381,12 +2381,14 @@ jobs: # between buckets: its login rate limiter lives in memory and each bucket is # sized to one fresh limiter's budget (see test/e2e/buckets.sh). The restart # also means each later bucket runs on data the earlier ones left in Postgres. + # A final serial bucket migrates the Deployment strategy and exercises a + # PostgreSQL outage/recovery against that populated installation. e2e-postgres: name: e2e postgres / amd64 (kind) needs: [changes, build-images] if: (needs.changes.outputs.e2e == 'true') || contains(fromJSON(needs.changes.outputs.ci-jobs || '[]'), 'e2e-postgres') runs-on: ubuntu-latest - timeout-minutes: 75 # Boots a kind cluster with PostgreSQL, installs the chart, runs three buckets. + timeout-minutes: 75 # Boots PostgreSQL and runs auth, roles, RBAC, then availability. permissions: contents: read env: @@ -2444,6 +2446,13 @@ jobs: GAMEPLANE_E2E_REUSE_CLUSTER=1 \ GAMEPLANE_E2E_CLUSTER="$CLUSTER" \ go test -tags=e2e -timeout 10m -v -parallel 4 -run "$RUN_REGEX" ./... + - name: run go e2e (PostgreSQL singleton upgrade and outage recovery) + run: | + RUN_REGEX="$(./test/e2e/buckets.sh regex postgres-availability)" + cd test/e2e && \ + GAMEPLANE_E2E_REUSE_CLUSTER=1 \ + GAMEPLANE_E2E_CLUSTER="$CLUSTER" \ + go test -tags=e2e -timeout 15m -v -parallel 1 -run "$RUN_REGEX" ./... - name: dump cluster state on failure (e2e-postgres) if: failure() uses: ./.github/actions/dump-cluster-state @@ -2854,6 +2863,10 @@ jobs: if (m) bucketSet.add(m[1]); if (/^e2e multicluster \//.test(n)) bucketSet.add('multicluster'); if (/^e2e upgrade \//.test(n)) bucketSet.add('upgrade'); + if (/^e2e postgres \//.test(n) && Array.isArray(j.steps)) { + const availability = j.steps.find((s) => s.name === 'run go e2e (PostgreSQL singleton upgrade and outage recovery)'); + if (availability && availability.conclusion === 'success') bucketSet.add('postgres-availability'); + } // The ratelimit bucket runs as a trailing step inside the // api-auth job leg (deliberately, so it drains the shared // login buckets last), not as its own job — so it never diff --git a/api/cmd/health.go b/api/cmd/health.go new file mode 100644 index 000000000..56bae3836 --- /dev/null +++ b/api/cmd/health.go @@ -0,0 +1,39 @@ +package main + +import ( + "context" + "net/http" + "time" + + "github.com/go-chi/chi/v5" + + "github.com/GameplanePanel/gameplane/api/internal/db" +) + +// mountHealthRoutes separates process liveness from database availability. +// Only one readiness ping can wait for a connection at a time, particularly +// important for SQLite's single-connection pool. Concurrent probes fail closed +// immediately instead of queueing more database work. No background work or +// cached success can outlive the two-second request budget. +func mountHealthRoutes(r chi.Router, store *db.Store) { + r.Get("/healthz", func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("ok")) + }) + pingSlot := make(chan struct{}, 1) + r.Get("/readyz", func(w http.ResponseWriter, req *http.Request) { + select { + case pingSlot <- struct{}{}: + defer func() { <-pingSlot }() + default: + http.Error(w, "not ready", http.StatusServiceUnavailable) + return + } + ctx, cancel := context.WithTimeout(req.Context(), 2*time.Second) + defer cancel() + if err := store.DB.PingContext(ctx); err != nil { + http.Error(w, "not ready", http.StatusServiceUnavailable) + return + } + _, _ = w.Write([]byte("ok")) + }) +} diff --git a/api/cmd/health_test.go b/api/cmd/health_test.go new file mode 100644 index 000000000..b51d2b225 --- /dev/null +++ b/api/cmd/health_test.go @@ -0,0 +1,100 @@ +package main + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/go-chi/chi/v5" + + "github.com/GameplanePanel/gameplane/api/internal/db" +) + +func TestHealthRoutesDatabaseUnavailable(t *testing.T) { + store, err := db.Open(t.Context(), "sqlite", ":memory:") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = store.Close() }) + r := chi.NewRouter() + mountHealthRoutes(r, store) + + check := func(path string, want int, body string) { + t.Helper() + rr := httptest.NewRecorder() + r.ServeHTTP(rr, httptest.NewRequestWithContext(t.Context(), http.MethodGet, path, nil)) + if rr.Code != want || rr.Body.String() != body { + t.Fatalf("%s: status=%d body=%q, want %d %q", path, rr.Code, rr.Body.String(), want, body) + } + } + check("/readyz", http.StatusOK, "ok") + if err := store.Close(); err != nil { + t.Fatal(err) + } + check("/readyz", http.StatusServiceUnavailable, "not ready\n") + check("/healthz", http.StatusOK, "ok") +} + +// Holding SQLite's only connection reproduces pool starvation without a fake +// driver. Readiness must time out, cap concurrent pings, and recover when the +// connection is released; liveness must never wait for that connection. +func TestHealthRoutesBoundPingAndRecover(t *testing.T) { + store, err := db.Open(t.Context(), "sqlite", ":memory:") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = store.Close() }) + conn, err := store.DB.Conn(t.Context()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = conn.Close() }() + r := chi.NewRouter() + mountHealthRoutes(r, store) + ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second) + defer cancel() + done := make(chan *httptest.ResponseRecorder, 1) + started := time.Now() + go func() { + rr := httptest.NewRecorder() + r.ServeHTTP(rr, httptest.NewRequestWithContext(ctx, http.MethodGet, "/readyz", nil)) + done <- rr + }() + for store.DB.Stats().WaitCount == 0 { + select { + case <-ctx.Done(): + t.Fatal("readiness did not attempt the database ping") + case <-time.After(time.Millisecond): + } + } + for _, path := range []string{"/readyz", "/healthz"} { + rr := httptest.NewRecorder() + before := time.Now() + r.ServeHTTP(rr, httptest.NewRequestWithContext(ctx, http.MethodGet, path, nil)) + want := http.StatusOK + if path == "/readyz" { + want = http.StatusServiceUnavailable + } + if rr.Code != want || time.Since(before) > 500*time.Millisecond { + t.Fatalf("%s blocked behind the ping or returned %d", path, rr.Code) + } + } + select { + case rr := <-done: + if rr.Code != http.StatusServiceUnavailable || time.Since(started) > 2500*time.Millisecond { + t.Fatalf("ping did not fail within its two-second budget: status=%d elapsed=%s", rr.Code, time.Since(started)) + } + case <-ctx.Done(): + t.Fatal("readiness exceeded its own deadline") + } + if err := conn.Close(); err != nil { + t.Fatal(err) + } + rr := httptest.NewRecorder() + r.ServeHTTP(rr, httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/readyz", nil)) + if rr.Code != http.StatusOK { + t.Fatalf("readiness did not recover: %d %s", rr.Code, rr.Body.String()) + } +} diff --git a/api/cmd/main.go b/api/cmd/main.go index c199bc30a..b1bd15b53 100644 --- a/api/cmd/main.go +++ b/api/cmd/main.go @@ -313,9 +313,7 @@ func main() { r.Use(bodyLimit(1 << 20)) // 1 MiB default; upload proxy raises its own ceiling r.Use(audit.Middleware(auditor)) - r.Get("/healthz", func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte("ok")) - }) + mountHealthRoutes(r, store) // Prometheus metrics are served by the separate metrics listener below, // never on this public router. diff --git a/api/internal/auth/sessions.go b/api/internal/auth/sessions.go index 875a65e6b..5141ce5ac 100644 --- a/api/internal/auth/sessions.go +++ b/api/internal/auth/sessions.go @@ -4,6 +4,7 @@ import ( "context" "crypto/rand" "crypto/sha256" + "database/sql" "encoding/base64" "encoding/hex" "errors" @@ -23,6 +24,11 @@ const ( sessionTTL = 12 * time.Hour ) +var ( + errSessionInvalid = errors.New("session invalid") + errSessionExpired = errors.New("session expired") +) + type ctxKey int const userCtxKey ctxKey = iota @@ -136,7 +142,13 @@ func (s *SessionStore) Authenticate(next http.Handler) http.Handler { } u, csrf, err := s.lookup(req.Context(), cookie.Value) if err != nil { - httperr.WriteMessage(w, req, messages.New("server.common.unauthenticated", nil)) + if errors.Is(err, sql.ErrNoRows) || errors.Is(err, errSessionInvalid) || errors.Is(err, errSessionExpired) { + httperr.WriteMessage(w, req, messages.New("server.common.unauthenticated", nil)) + } else { + // Store failures deny access but do not imply logout. Never put + // database, account, cookie or permission details in the response. + httperr.WriteMessage(w, req, messages.New("server.generic.503", nil)) + } return } if isMutating(req.Method) { @@ -169,11 +181,6 @@ func (s *SessionStore) lookup(ctx context.Context, token string) (*User, string, if err != nil { return nil, "", err } - perms, perr := s.LoadPerms(ctx, u.ID) - if perr != nil { - return nil, "", perr - } - u.Perms = perms exp, perr := time.Parse(time.RFC3339, expires) if perr != nil { // A corrupt expires_at row is indistinguishable from an expired @@ -181,12 +188,17 @@ func (s *SessionStore) lookup(ctx context.Context, token string) (*User, string, // user log in again. Log so it's visible in ops. slog.Warn("session expires_at parse", "err", perr, "value", expires) _, _ = s.db.DB.ExecContext(ctx, `DELETE FROM sessions WHERE token = ?`, digest) - return nil, "", errors.New("session invalid") + return nil, "", errSessionInvalid } if time.Now().After(exp) { _, _ = s.db.DB.ExecContext(ctx, `DELETE FROM sessions WHERE token = ?`, digest) - return nil, "", errors.New("session expired") + return nil, "", errSessionExpired } + perms, perr := s.LoadPerms(ctx, u.ID) + if perr != nil { + return nil, "", perr + } + u.Perms = perms return &u, csrf, nil } diff --git a/api/internal/auth/sessions_availability_test.go b/api/internal/auth/sessions_availability_test.go new file mode 100644 index 000000000..36eb8d72d --- /dev/null +++ b/api/internal/auth/sessions_availability_test.go @@ -0,0 +1,87 @@ +package auth + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +func TestAuthenticateStoreFailureDoesNotInvalidateSession(t *testing.T) { + s := newAuthDB(t) + seedUser(t, s, "alice", "pw", "admin") + store := NewSessionStore(s) + token, _, err := store.Create(t.Context(), 1) + if err != nil { + t.Fatal(err) + } + h := store.Authenticate(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + request := func(cookie string, want int) { + t.Helper() + rr := httptest.NewRecorder() + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/protected", nil) + if cookie != "" { + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: cookie}) + } + h.ServeHTTP(rr, req) + if rr.Code != want { + t.Fatalf("status=%d body=%q, want %d", rr.Code, rr.Body.String(), want) + } + if len(rr.Result().Cookies()) != 0 { + t.Fatal("authentication failure changed the client's session cookies") + } + if want == http.StatusServiceUnavailable && rr.Body.String() != "Service Unavailable\n" { + t.Fatalf("store error was not generic: %q", rr.Body.String()) + } + } + + request(token, http.StatusNoContent) + // A permission-query failure after a successful session lookup must deny + // access without being mistaken for an expired/unknown session. + if _, err := s.DB.ExecContext(t.Context(), "ALTER TABLE role_permissions RENAME TO unavailable_permissions"); err != nil { + t.Fatal(err) + } + request(token, http.StatusServiceUnavailable) + request("", http.StatusUnauthorized) + request("unknown-token", http.StatusUnauthorized) + if _, err := s.DB.ExecContext(t.Context(), "ALTER TABLE unavailable_permissions RENAME TO role_permissions"); err != nil { + t.Fatal(err) + } + request(token, http.StatusNoContent) + if err := s.Close(); err != nil { + t.Fatal(err) + } + request(token, http.StatusServiceUnavailable) + request("unknown-token", http.StatusServiceUnavailable) + request("", http.StatusUnauthorized) +} + +func TestAuthenticateInvalidSessionRemainsUnauthorized(t *testing.T) { + for _, expires := range []string{"invalid-timestamp", time.Now().Add(-time.Hour).UTC().Format(time.RFC3339)} { + t.Run(expires, func(t *testing.T) { + s := newAuthDB(t) + seedUser(t, s, "alice", "pw", "admin") + store := NewSessionStore(s) + token, _, err := store.Create(t.Context(), 1) + if err != nil { + t.Fatal(err) + } + if _, err := s.DB.ExecContext(t.Context(), "UPDATE sessions SET expires_at = ?", expires); err != nil { + t.Fatal(err) + } + h := store.Authenticate(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Error("invalid session reached protected handler") + })) + rr := httptest.NewRecorder() + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/protected", nil) + req.AddCookie(&http.Cookie{Name: sessionCookie, Value: token}) + h.ServeHTTP(rr, req) + if rr.Code != http.StatusUnauthorized || strings.TrimSpace(rr.Body.String()) != "unauthenticated" { + t.Fatalf("invalid session returned %d %q", rr.Code, rr.Body.String()) + } + }) + } +} diff --git a/api/internal/db/db.go b/api/internal/db/db.go index 999a7efe6..ffed7bc42 100644 --- a/api/internal/db/db.go +++ b/api/internal/db/db.go @@ -84,7 +84,7 @@ func (s *Store) Close() error { return s.DB.Close() } // user-management access (role permission edits, primary-role changes, // user deletion). Hold it from the last-user-manager check through the // commit of the change so two requests can't both pass the check. It -// covers one API process; the SQLite install runs a single API replica. +// covers one API process; every database driver requires a single API replica. func (s *Store) LockUserManagement() (unlock func()) { s.userMgmtMu.Lock() return s.userMgmtMu.Unlock diff --git a/api/specs.md b/api/specs.md index 8bec9011e..ed31635f1 100644 --- a/api/specs.md +++ b/api/specs.md @@ -104,6 +104,7 @@ The HTTP server listens on `:8000` (configurable) with these route groups. Prome - `/auth/oidc/start` (legacy) — GET: single helm-provider start - `/auth/oidc/callback` (legacy) — GET: single helm-provider callback - `/healthz` — GET: liveness probe +- `/readyz` — GET: database readiness, a `PingContext` with a two-second deadline. At most one ping runs at once; concurrent probes return a generic 503 without queueing more database work. Success returns `ok`; unavailable or busy returns only `not ready`. The chart probes every five seconds with a three-second timeout; three consecutive failed probes remove readiness, tolerating brief SQLite connection contention. `/healthz` remains independent of the database, so outages do not trigger liveness restarts. **Protected (authenticated + RBAC):** - All six `/fleet/*` reads share a per-authenticated-user token bucket: 60 requests/minute sustained, burst 10. Exhausted requests receive 429 with `Retry-After: 1` before cluster discovery or resource listing. Sessions and routes share the account's budget; users behind the same IP do not. This per-process limit bounds request frequency without removing owner/collaborator scans; multiple API replicas have independent budgets. @@ -537,7 +538,9 @@ Verify from `/api/go.mod`. ### Database driver selection - **Production (default):** `modernc.org/sqlite` — file-based, WAL mode, tested -- **Experimental:** PostgreSQL via `jackc/pgx/v5` — compile with `-tags=postgres`. Works end to end; `api/internal/db`'s tests run against PostgreSQL in the `api (postgres)` CI job, but there is no Postgres e2e/upgrade coverage yet +- **Experimental:** PostgreSQL via `jackc/pgx/v5` — compile with `-tags=postgres`. The `api (postgres)` CI job covers the store. The `e2e postgres` job runs API auth/roles/RBAC plus the `postgres-availability` bucket: populated RollingUpdate-to-Recreate migration, singleton Pod observation, audit integrity, and database outage/recovery without restarting the API or replacing the session. +- Every enabled API install requires exactly one replica, for both drivers and both combined/standalone modes. Helm renders `Recreate` with `rollingUpdate: null`; the combined-install pre-upgrade hook clears an existing rolling-update strategy for either driver. Recreate prevents ordinary rollout overlap, but cannot fence a process on a partitioned node after force deletion. HA requires database-level serialization of audit/user-management transactions and coordinated background work/rate limiting before multiple writers are supported. +- Protected-route session lookup distinguishes missing, expired and corrupt sessions (generic 401) from database/session-permission lookup failures (generic 503). Both deny access. Operational failures do not delete sessions or clear cookies; access recovers with the same cookie when the store recovers. Missing-cookie responses remain 401 without consulting storage, and login privacy is unchanged. - Driver selected at startup via `--db-driver` (sqlite|postgres) + `--db-dsn` - SQLite DSNs automatically receive a `busy_timeout(5000)` pragma if one is not already specified, allowing multiple processes (e.g., the API and bootstrap-admin) sharing the same file to wait for locks instead of failing immediately - Migrations run automatically on startup (`store.Migrate(ctx)`): the driver's legacy set (`migrations/sqlite/` or `migrations/postgres/`, 001-012, same filenames and resulting schema) then the shared set (`migrations/common/`, 013+, one portable file per migration), in version order; a version present in both sets is a startup error. Versions are recorded in `schema_migrations` by bare filename, so SQLite installs see the same versions as before the split diff --git a/charts/gameplane/templates/api.yaml b/charts/gameplane/templates/api.yaml index 4f3c2bda3..adf54a0d7 100644 --- a/charts/gameplane/templates/api.yaml +++ b/charts/gameplane/templates/api.yaml @@ -226,22 +226,18 @@ spec: replicas: {{ .Values.api.replicas }} selector: matchLabels: { app.kubernetes.io/name: gameplane-api } - {{- if or .Values.api.standalone (eq .Values.api.db.driver "sqlite") }} - # Standalone also avoids overlapping API processes with PostgreSQL: generated - # keys use a ReadWriteOnce PVC, and audit/user-management locks are per-process. - # Recreate strategy for SQLite: the database is a single-writer store - # on a ReadWriteOnce PVC. RollingUpdate would start a new pod before - # terminating the old one, allowing two processes to compete for the - # database file. The legacy adoption code would race the old writer, - # risking corruption. Recreate ensures the old pod is fully gone first. + # Every mode and database driver requires one API process: audit-chain and + # user-management locks are per-process. Recreate accepts a short outage to + # stop the old writer before starting its replacement. It is not fencing + # against forced Pod deletion or a partitioned node. strategy: type: Recreate # Explicit null, not omission: it states that this Deployment has no # rolling-update config, so nothing can merge one back in. # # It is NOT sufficient on its own to fix an upgrade from a release - # that predates this stanza (<= 0.2.0-beta.5). Those shipped no - # strategy at all, so the apiserver defaulted them to RollingUpdate + # that predates this stanza (including older PostgreSQL installs). Those + # shipped no strategy, so the apiserver defaulted them to RollingUpdate # and materialized spec.strategy.rollingUpdate on the live object; # under server-side apply Helm does not own that field, so a null # here does not remove it and the apiserver rejects the Deployment @@ -250,7 +246,6 @@ spec: # runs pre-upgrade. Both halves are load-bearing; the e2e upgrade # test covers them. rollingUpdate: null - {{- end }} template: metadata: labels: @@ -503,7 +498,9 @@ spec: - { name: http, containerPort: 8000 } - { name: metrics, containerPort: {{ .Values.api.metricsPort | default 9090 }} } livenessProbe: { httpGet: { path: /healthz, port: http }, initialDelaySeconds: 15 } - readinessProbe: { httpGet: { path: /healthz, port: http }, initialDelaySeconds: 5 } + # The DB ping has a 2s deadline. Tolerate brief contention on SQLite's + # single connection before removing the only API endpoint. + readinessProbe: { httpGet: { path: /readyz, port: http }, initialDelaySeconds: 5, timeoutSeconds: 3, periodSeconds: 5, failureThreshold: 3 } resources: {{ toYaml .Values.api.resources | nindent 12 }} securityContext: allowPrivilegeEscalation: false diff --git a/charts/gameplane/templates/crd-apply-hook.yaml b/charts/gameplane/templates/crd-apply-hook.yaml index 4ad3925cb..d02015347 100644 --- a/charts/gameplane/templates/crd-apply-hook.yaml +++ b/charts/gameplane/templates/crd-apply-hook.yaml @@ -108,15 +108,15 @@ subjects: - kind: ServiceAccount name: {{ .Release.Name }}-crd-apply namespace: {{ .Release.Namespace }} -{{- if and (eq (include "gameplane.apiEnabled" .) "true") (eq .Values.api.db.driver "sqlite") .Release.IsUpgrade }} +{{- if and (eq (include "gameplane.apiEnabled" .) "true") .Release.IsUpgrade }} --- # One-time workload migration, riding along with this hook because it needs # exactly the same thing: a kubectl that runs BEFORE Helm applies the release. # -# Releases <= 0.2.0-beta.5 shipped no `strategy` on the api Deployment, so the -# apiserver defaulted it to RollingUpdate and materialized a +# Older releases (including combined PostgreSQL installs) shipped no `strategy` +# on the api Deployment, so the apiserver defaulted it to RollingUpdate with a # spec.strategy.rollingUpdate block. The chart now sets `type: Recreate` (the -# SQLite store is single-writer on an RWO volume), and the apiserver rejects +# API's audit/user-management locks require one process), and the apiserver rejects # the two together: # # spec.strategy.rollingUpdate: Forbidden: may not be specified when @@ -202,7 +202,10 @@ spec: runAsUser: 65532 fsGroup: 65532 seccompProfile: { type: RuntimeDefault } - {{- if and (eq (include "gameplane.apiEnabled" .) "true") (eq .Values.api.db.driver "sqlite") .Release.IsUpgrade }} + {{- /* The target may be enabled for the first time during this upgrade. + Only an existing Deployment needs migration; a lookup error fails + rendering, while NotFound safely skips the patch. */}} + {{- if and (eq (include "gameplane.apiEnabled" .) "true") .Release.IsUpgrade (lookup "apps/v1" "Deployment" .Release.Namespace "gameplane-api") }} initContainers: # Clear the stale spec.strategy.rollingUpdate left by releases that # predate the Recreate strategy — see the Role above for why Helm diff --git a/charts/gameplane/templates/validate.yaml b/charts/gameplane/templates/validate.yaml index 79b5e17b6..6c43de459 100644 --- a/charts/gameplane/templates/validate.yaml +++ b/charts/gameplane/templates/validate.yaml @@ -1,3 +1,6 @@ +{{- if and (eq (include "gameplane.apiEnabled" .) "true") (ne (toString .Values.api.replicas) "1") }} +{{- fail "api requires api.replicas=1; multiple API processes are not supported" }} +{{- end }} {{- if .Values.api.standalone }} {{- if ne (include "gameplane.apiEnabled" .) "true" }} {{- fail "api.standalone requires api.enabled=true" }} @@ -13,9 +16,6 @@ {{- if eq (include "gameplane.operatorEnabled" .) "true" }} {{- fail "api.standalone requires operator.enabled=false; install operators in the remote game clusters" }} {{- end }} -{{- if ne (int .Values.api.replicas) 1 }} -{{- fail "api.standalone requires api.replicas=1 for the persistent panel key and database" }} -{{- end }} {{- if (.Values.gateway | default dict).enabled }} {{- fail "api.standalone cannot host gateway.enabled; install the gateway in the remote game cluster" }} {{- end }} diff --git a/charts/gameplane/values.yaml b/charts/gameplane/values.yaml index 8acb5ba60..583c02e40 100644 --- a/charts/gameplane/values.yaml +++ b/charts/gameplane/values.yaml @@ -159,7 +159,10 @@ api: # Requires operator.enabled=false, which excludes bundled native CRDs. # Standalone mode also skips the CRD apply hook and its RBAC. standalone: false - replicas: 1 # Keep at 1. SQLite is single-writer on a RWO volume; with db.driver=postgres the user-management lock and audit hash chain are still per-process + # Must be 1 in every enabled API install, including PostgreSQL. Audit-chain + # and user-management locks are per-process. Recreate rollouts stop the old + # API before starting its replacement and accept a brief panel outage. + replicas: 1 nodeSelector: {} tolerations: [] affinity: {} diff --git a/charts/tests/check_standalone.py b/charts/tests/check_standalone.py index 4caea9cc6..e30ea1211 100644 --- a/charts/tests/check_standalone.py +++ b/charts/tests/check_standalone.py @@ -42,8 +42,41 @@ def check_packaged_crds(example): assert not any("crd-apply" in obj["metadata"]["name"] for obj in objects), objects +def check_api_availability(): + """All API modes stay singleton, migrate old strategies, and probe the DB.""" + for standalone in (False, True): + for driver in ("sqlite", "postgres"): + values = {"operator": {"enabled": not standalone}, + "api": {"standalone": standalone, "db": {"driver": driver}}} + for upgrade in (False, True): + objects = render(values, upgrade=upgrade) + deployment = one(objects, "Deployment", "gameplane-api") + assert deployment["spec"]["replicas"] == 1 + assert deployment["spec"]["strategy"] == {"type": "Recreate", "rollingUpdate": None} + container = next(c for c in deployment["spec"]["template"]["spec"]["containers"] if c["name"] == "api") + assert container["livenessProbe"]["httpGet"]["path"] == "/healthz" + assert container["readinessProbe"]["httpGet"]["path"] == "/readyz" + assert container["readinessProbe"]["failureThreshold"] == 3 + assert container["readinessProbe"]["periodSeconds"] == 5 + assert container["readinessProbe"]["timeoutSeconds"] > 2 + if upgrade and not standalone: + role = one(objects, "Role", "gameplane-strategy-migrate") + assert role["rules"][0]["resourceNames"] == ["gameplane-api"] + job = one(objects, "Job", "gameplane-crd-apply") + # Client-side rendering has no live Deployment. Enabling + # the API on an older operator-only release must skip the + # migration instead of PATCHing an absent resource. + assert not any(c["name"] == "migrate-api-strategy" for c in job["spec"]["template"]["spec"].get("initContainers", [])) + for replicas in (0, 2, -1, 1.5): + values["api"]["replicas"] = replicas + render(values, failure="api requires api.replicas=1") + # A remote installation with no API has no singleton requirement. + render({"api": {"enabled": False, "replicas": 0}}) + + def main(): """Check deployment roles, standalone key custody, and source/archive CRDs.""" + check_api_availability() defaults = render(include_crds=True) assert_crds(defaults, EXPECTED_CRDS) one(defaults, "Deployment", "gameplane-operator") @@ -139,7 +172,7 @@ def main(): render({"api": {"standalone": True}}, failure="api.standalone requires operator.enabled=false") render({"operator": {"enabled": False}, "api": {"enabled": False, "standalone": True}}, failure="api.standalone requires api.enabled=true") - render({"operator": {"enabled": False}, "api": {"standalone": True, "replicas": 2}}, failure="api.standalone requires api.replicas=1") + render({"operator": {"enabled": False}, "api": {"standalone": True, "replicas": 2}}, failure="api requires api.replicas=1") example = yaml.safe_load((Path(__file__).parents[1] / "gameplane/examples/standalone-panel-values.yaml").read_text()) for upgrade in (False, True): objects = render(example, include_crds=True, upgrade=upgrade) diff --git a/docs/install.md b/docs/install.md index d5050f049..c18725623 100644 --- a/docs/install.md +++ b/docs/install.md @@ -945,16 +945,46 @@ and the existing database is used instead — no data loss. Nothing else needs to happen; the upgrade proceeds normally. -### SQLite upgrades (brief downtime) - -When using the SQLite database driver, the API Deployment uses a `Recreate` -upgrade strategy: the old pod is fully terminated before the new one starts. -This ensures no two API processes try to write the same SQLite database file -(which is a single-writer store on a ReadWriteOnce PVC). As a result, -SQLite-backed installs experience a few seconds of dashboard downtime during -an upgrade. Combined PostgreSQL installs use rolling updates, but the driver -remains experimental and the API should still run as a single replica (see -`api.replicas` in `values.yaml`). Standalone installs use `Recreate` with either -database driver: generated keys use a ReadWriteOnce volume, and audit-chain and -user-management locks remain per-process. A shared provisioned key does not make -multiple API replicas supported. +### API upgrades (brief downtime) + +Every API Deployment uses `Recreate`: the old pod terminates before its +replacement starts. This includes combined and standalone installations with +either SQLite or PostgreSQL. The chart rejects `api.replicas` other than `1`. +Audit-chain and user-management locks are per-process; PostgreSQL and a shared +standalone key do not make multiple API writers supported. Plan a brief panel +outage on upgrade. Existing games and their operator continue independently. + +Upgrading an older combined PostgreSQL release now changes its default +RollingUpdate strategy to Recreate. The existing pre-upgrade CRD hook also +migrates that Deployment, removing the stale `rollingUpdate` field before Helm +applies the new strategy. Keep `crds.autoApply.enabled=true`. If you manage +CRDs and upgrade hooks externally, apply this atomic patch before upgrading: + +```sh +kubectl --namespace gameplane-system patch deployment gameplane-api --type=merge \ + --patch '{"spec":{"strategy":{"type":"Recreate","rollingUpdate":null}}}' +``` + +Recreate controls normal Kubernetes rollouts; it does not fence a writer on a +partitioned node after forced Pod deletion. Stop or fence that old process +before allowing a replacement to access the database. Multi-replica API HA is +a separate feature requiring database-level transaction serialization for +audit append/prune and user-management invariants, plus coordinated background +work and rate limiting. + +### API database readiness + +`/healthz` reports process liveness. `/readyz` pings the database with a +two-second deadline and returns 503 when it cannot obtain a connection or +complete the ping. Only one ping can be in flight; concurrent checks fail +without queueing more work in SQLite's one-connection pool. Neither endpoint +returns database addresses, credentials or account details. + +The chart checks readiness every five seconds, allows three seconds per probe, +and marks the Pod unready after three consecutive failures (normally within +about seventeen seconds of a database outage, plus Kubernetes endpoint +propagation). This tolerates brief SQLite connection contention. Liveness +stays on `/healthz`, so an external outage does not restart a healthy API. +Readiness recovers after the database becomes reachable again. Session-store +errors return a generic 503 without clearing the session; invalid or expired +sessions remain 401. Clients can retry after recovery using the same cookie. diff --git a/docs/standalone-panel.md b/docs/standalone-panel.md index c405b996b..808fcef49 100644 --- a/docs/standalone-panel.md +++ b/docs/standalone-panel.md @@ -420,6 +420,10 @@ restart; a remote operator upgrade may roll game pods. For a panel-only Helm release, repeat the installation command with the updated chart, matching images, and your standalone values file. Keep `api.replicas=1` and the existing key storage, including when using PostgreSQL. +All Helm API deployments use Recreate and accept a brief panel outage during +rollout. This does not fence a process on a partitioned node after force +deletion. See [API upgrades and readiness](install.md#api-upgrades-brief-downtime) +for the singleton boundary and the separate database readiness probe. ## Troubleshooting diff --git a/test/e2e/api_availability_e2e_test.go b/test/e2e/api_availability_e2e_test.go new file mode 100644 index 000000000..ff35c4336 --- /dev/null +++ b/test/e2e/api_availability_e2e_test.go @@ -0,0 +1,366 @@ +//go:build e2e + +package e2e + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "os/exec" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/watch" +) + +// This bucket owns the PostgreSQL fixture after the normal API buckets finish. +// It deliberately changes a live Deployment's strategy, upgrades the release, +// and stops the database; it must never run concurrently with other tests. +func TestUpgrade_PostgresAvailability(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 12*time.Minute) + defer cancel() + const ns = "gameplane-system" + deployments := envInstance.K8s.AppsV1().Deployments(ns) + dep, err := deployments.Get(ctx, "gameplane-api", metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(strings.Join(dep.Spec.Template.Spec.Containers[0].Args, " "), "--db-driver=postgres") { + t.Fatal("postgres-availability requires the PostgreSQL E2E fixture") + } + envInstance.BootstrapAdmin(t, adminUsername, adminPassword) + admin := envInstance.APIClient(t, adminUsername, adminPassword) + defer admin.Close() + _, _, userID := envInstance.CreateUser(t, admin, "viewer", "e2e-availability") + + // Model a pre-fix combined PostgreSQL installation, including the stale + // rollingUpdate block that must be removed before Helm applies Recreate. + _, err = deployments.Patch(ctx, dep.Name, types.MergePatchType, + []byte(`{"spec":{"strategy":{"type":"RollingUpdate","rollingUpdate":{"maxSurge":1,"maxUnavailable":0}}}}`), metav1.PatchOptions{}) + if err != nil { + t.Fatal(err) + } + finishWatch := watchSingletonAPI(ctx, t) + chart, err := findRepoRel("charts/gameplane") + if err != nil { + t.Fatal(err) + } + // Changing a real container env value forces a Pod replacement even when + // the working-tree image tag is the same as the fixture's installed tag. + logLevel := "debug" + for _, variable := range dep.Spec.Template.Spec.Containers[0].Env { + if variable.Name == "GAMEPLANE_LOG_LEVEL" && variable.Value == "debug" { + logLevel = "info" + } + } + upgrade := exec.CommandContext(ctx, "helm", "upgrade", "gameplane", chart, + "--kube-context", envInstance.Context, "--namespace", ns, "--reuse-values", + "--set-string", "api.logLevel="+logLevel, "--wait", "--timeout", "4m") + finishWrites := writeUsersDuringAPIUpgrade(ctx, t, admin, userID) + if out, err := upgrade.CombinedOutput(); err != nil { + t.Fatalf("PostgreSQL strategy migration failed: %v\n%s", err, out) + } + finishWrites() + finishWatch() + dep, err = deployments.Get(ctx, dep.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if dep.Spec.Strategy.Type != appsv1.RecreateDeploymentStrategyType || dep.Spec.Strategy.RollingUpdate != nil || *dep.Spec.Replicas != 1 { + t.Fatalf("unsafe API strategy after upgrade: %+v replicas=%v", dep.Spec.Strategy, dep.Spec.Replicas) + } + + // A direct Pod forward stays reachable after readiness removes the Pod + // from Service endpoints, so the outage assertions test the API itself. + pods, err := envInstance.K8s.CoreV1().Pods(ns).List(ctx, metav1.ListOptions{LabelSelector: "app.kubernetes.io/name=gameplane-api"}) + if err != nil || len(pods.Items) != 1 { + t.Fatalf("expected one API Pod after upgrade: %v pods=%v", err, pods) + } + pod := pods.Items[0] + port, stop := envInstance.PortForward(t, ns, "pod/"+pod.Name, 8000) + defer stop() + // Reuse the pre-upgrade cookie and CSRF token; do not log in again. + client := &APIClient{BaseURL: fmt.Sprintf("http://127.0.0.1:%d", port), HTTP: admin.HTTP, CSRF: admin.CSRF} + assertAPIStatus(t, client, "/readyz", http.StatusOK) + assertAPIStatus(t, client, "/users/me", http.StatusOK) + resp, body, err := client.Patch("/users/"+userID, map[string]string{"displayName": "after singleton upgrade"}) + if err == nil { + _ = resp.Body.Close() + } + if err != nil || resp.StatusCode != http.StatusOK { + t.Fatalf("pre-upgrade user could not be updated: %v %s", err, body) + } + resp, body, err = client.Get("/admin/audit/verify") + if err == nil { + _ = resp.Body.Close() + } + var auditResult struct { + OK bool `json:"ok"` + Checked int64 `json:"checked"` + } + if err != nil || resp.StatusCode != http.StatusOK || json.Unmarshal(body, &auditResult) != nil || !auditResult.OK || auditResult.Checked == 0 { + t.Fatalf("audit integrity after PostgreSQL upgrade: %v %s", err, body) + } + + postgres := envInstance.K8s.AppsV1().StatefulSets(ns) + scale, err := postgres.GetScale(ctx, "gameplane-postgres", metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + replicas := scale.Spec.Replicas + restore := func() { + restoreCtx, stopRestore := context.WithTimeout(context.Background(), time.Minute) + defer stopRestore() + current, getErr := postgres.GetScale(restoreCtx, "gameplane-postgres", metav1.GetOptions{}) + if getErr != nil { + t.Errorf("read PostgreSQL scale for recovery: %v", getErr) + return + } + current.Spec.Replicas = replicas + if _, updateErr := postgres.UpdateScale(restoreCtx, "gameplane-postgres", current, metav1.UpdateOptions{}); updateErr != nil { + t.Errorf("restore PostgreSQL: %v", updateErr) + } + } + t.Cleanup(restore) + scale.Spec.Replicas = 0 + if _, err := postgres.UpdateScale(ctx, "gameplane-postgres", scale, metav1.UpdateOptions{}); err != nil { + t.Fatal(err) + } + envInstance.Eventually(t, 45*time.Second, func() (bool, string) { + resp, body, err := client.DoContext(ctx, http.MethodGet, "/readyz", nil) + if err == nil { + _ = resp.Body.Close() + } + return err == nil && resp.StatusCode == http.StatusServiceUnavailable && string(body) == "not ready\n", fmt.Sprintf("readiness: %v %s", err, body) + }) + assertAPIStatus(t, client, "/healthz", http.StatusOK) + body = assertAPIStatus(t, client, "/users/me", http.StatusServiceUnavailable) + if string(body) != "Service Unavailable\n" { + t.Fatalf("session failure exposed detail: %q", body) + } + // Stay down longer than the liveness failure window (3 x 10 seconds). + // Neither failed readiness nor session lookup should restart this Pod. + envInstance.Consistently(t, 35*time.Second, time.Second, func() (bool, string) { + current, err := envInstance.K8s.CoreV1().Pods(ns).Get(ctx, pod.Name, metav1.GetOptions{}) + if err != nil { + return false, err.Error() + } + if current.UID != pod.UID || apiRestartCount(current) != apiRestartCount(&pod) { + return false, "API restarted during database outage" + } + resp, body, err := client.DoContext(ctx, http.MethodGet, "/healthz", nil) + if err == nil { + _ = resp.Body.Close() + } + return err == nil && resp.StatusCode == http.StatusOK, fmt.Sprintf("liveness: %v %s", err, body) + }) + current, err := envInstance.K8s.CoreV1().Pods(ns).Get(ctx, pod.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + for _, condition := range current.Status.Conditions { + if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue { + t.Fatal("API remained Ready while PostgreSQL was unavailable") + } + } + restore() + envInstance.Eventually(t, 90*time.Second, func() (bool, string) { + resp, body, err := client.DoContext(ctx, http.MethodGet, "/readyz", nil) + if err == nil { + _ = resp.Body.Close() + } + return err == nil && resp.StatusCode == http.StatusOK, fmt.Sprintf("recovery readiness: %v %s", err, body) + }) + assertAPIStatus(t, client, "/users/me", http.StatusOK) + current, err = envInstance.K8s.CoreV1().Pods(ns).Get(ctx, pod.Name, metav1.GetOptions{}) + if err != nil || current.UID != pod.UID || apiRestartCount(current) != apiRestartCount(&pod) { + t.Fatalf("recovery restarted the API: %v", err) + } + // An unknown credential remains a true unauthorized response after recovery. + anonymous := &APIClient{BaseURL: client.BaseURL, HTTP: &http.Client{Timeout: 5 * time.Second}} + assertAPIStatus(t, anonymous, "/users/me", http.StatusUnauthorized) + request, err := http.NewRequestWithContext(ctx, http.MethodGet, client.BaseURL+"/users/me", nil) + if err != nil { + t.Fatal(err) + } + request.AddCookie(&http.Cookie{Name: "gameplane_session", Value: "unknown-session"}) + resp, err = anonymous.HTTP.Do(request) + if err != nil { + t.Fatal(err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusUnauthorized { + t.Fatalf("unknown session after recovery returned %d", resp.StatusCode) + } + // A release can enable the API for the first time on upgrade, or recover + // a deleted Deployment. The pre-upgrade migration must skip an absent API. + foreground := metav1.DeletePropagationForeground + if err := deployments.Delete(ctx, "gameplane-api", metav1.DeleteOptions{PropagationPolicy: &foreground}); err != nil { + t.Fatal(err) + } + envInstance.Eventually(t, time.Minute, func() (bool, string) { + _, err := deployments.Get(ctx, "gameplane-api", metav1.GetOptions{}) + return apierrors.IsNotFound(err), fmt.Sprintf("waiting for absent API Deployment: %v", err) + }) + recreate := exec.CommandContext(ctx, "helm", "upgrade", "gameplane", chart, + "--kube-context", envInstance.Context, "--namespace", ns, "--reuse-values", + "--set", "api.enabled=true", "--wait", "--timeout", "4m") + if out, err := recreate.CombinedOutput(); err != nil { + t.Fatalf("upgrade with no existing API Deployment failed: %v\n%s", err, out) + } + recreated, err := deployments.Get(ctx, "gameplane-api", metav1.GetOptions{}) + if err != nil || recreated.Status.ReadyReplicas != 1 || recreated.Spec.Strategy.Type != appsv1.RecreateDeploymentStrategyType { + t.Fatalf("missing API did not recover as a ready singleton: %v %+v", err, recreated) + } +} + +func assertAPIStatus(t *testing.T, client *APIClient, path string, want int) []byte { + t.Helper() + resp, body, err := client.DoContext(t.Context(), http.MethodGet, path, nil) + if err == nil { + _ = resp.Body.Close() + } + if err != nil || resp.StatusCode != want { + t.Fatalf("%s: want %d: %v %s", path, want, err, body) + } + return body +} + +func apiRestartCount(pod *corev1.Pod) int32 { + for _, container := range pod.Status.ContainerStatuses { + if container.Name == "api" { + return container.RestartCount + } + } + return 0 +} + +// Keep concurrent audited mutations arriving while the old API shuts down. +// The accepted Recreate outage can close its port-forward; session persistence +// and a mutation through the replacement are checked separately above. +func writeUsersDuringAPIUpgrade(ctx context.Context, t *testing.T, client *APIClient, userID string) func() { + t.Helper() + ctx, cancel := context.WithCancel(ctx) + t.Cleanup(cancel) + var workers sync.WaitGroup + var successes atomic.Int64 + for worker := range 2 { + workers.Go(func() { + for { + select { + case <-ctx.Done(): + return + default: + } + resp, _, err := client.DoContext(ctx, http.MethodPatch, "/users/"+userID, map[string]string{"displayName": fmt.Sprintf("upgrade writer %d", worker)}) + if err != nil { + return // The old Pod's port-forward ends with the old process. + } + _ = resp.Body.Close() + if resp.StatusCode == http.StatusOK { + successes.Add(1) + } + select { + case <-ctx.Done(): + return + case <-time.After(time.Second): + } + } + }) + } + envInstance.Eventually(t, 10*time.Second, func() (bool, string) { + return successes.Load() >= 2, "concurrent mutations have not succeeded before upgrade" + }) + return func() { + cancel() + workers.Wait() + t.Logf("%d successful concurrent user mutations before the old API stopped", successes.Load()) + } +} + +// Watch from the list's resourceVersion so even a brief overlapping writer is +// observed. A terminating Pod still counts while its API container is Running. +func watchSingletonAPI(ctx context.Context, t *testing.T) func() { + t.Helper() + pods := envInstance.K8s.CoreV1().Pods("gameplane-system") + options := metav1.ListOptions{LabelSelector: "app.kubernetes.io/name=gameplane-api"} + initial, err := pods.List(ctx, options) + if err != nil || len(initial.Items) != 1 { + t.Fatalf("singleton baseline: %v pods=%v", err, initial) + } + oldUID := initial.Items[0].UID + options.ResourceVersion = initial.ResourceVersion + events, err := pods.Watch(ctx, options) + if err != nil { + t.Fatal(err) + } + t.Cleanup(events.Stop) + done := make(chan error, 1) + replaced := make(chan struct{}, 1) + stop := make(chan struct{}) + go func() { + states := map[types.UID]corev1.Pod{oldUID: initial.Items[0]} + for { + select { + case <-stop: + done <- nil + return + case <-ctx.Done(): + done <- ctx.Err() + return + case event, ok := <-events.ResultChan(): + pod, isPod := event.Object.(*corev1.Pod) + if !ok || event.Type == watch.Error || !isPod { + done <- fmt.Errorf("API Pod watch ended without proving singleton rollout: %v", event.Type) + return + } + if event.Type == watch.Deleted { + delete(states, pod.UID) + } else { + states[pod.UID] = *pod + } + running := 0 + for _, state := range states { + for _, container := range state.Status.ContainerStatuses { + if container.Name == "api" && container.State.Running != nil { + running++ + if state.UID != oldUID { + select { + case replaced <- struct{}{}: + default: + } + } + } + } + } + if running > 1 { + done <- fmt.Errorf("%d API writers ran concurrently during upgrade", running) + return + } + } + } + }() + return func() { + t.Helper() + select { + case err := <-done: + t.Fatalf("singleton rollout observation: %v", err) + case <-replaced: + case <-ctx.Done(): + t.Fatal("no replacement API Pod observed") + } + close(stop) + if err := <-done; err != nil { + t.Fatal(err) + } + } +} diff --git a/test/e2e/buckets.sh b/test/e2e/buckets.sh index b803e2f42..9768b72bb 100755 --- a/test/e2e/buckets.sh +++ b/test/e2e/buckets.sh @@ -376,6 +376,13 @@ TestUpgrade_FromPreviousRelease EOF } +# Runs alone after the normal PostgreSQL API buckets: upgrades the populated +# release and stops/restores its database. No parallel tests may share it. +bucket_postgres_availability() { cat <<'EOF' +TestUpgrade_PostgresAvailability +EOF +} + # One sequential lifecycle test that runs `helm upgrade --reuse-values` (1m # report interval, dashboard token, public summary, no per-source ingest # limit) on its own bucket cluster, so it must not share a cluster with other @@ -415,7 +422,7 @@ EOF unbucketed() { :; } bucket_names() { - printf '%s\n' operator api-auth api-roles api-rbac api-agent api-mods ratelimit bot-fast bot-heavy multicluster upgrade telemetry capture-overhead optional-components management-placement + printf '%s\n' operator api-auth api-roles api-rbac api-agent api-mods ratelimit bot-fast bot-heavy multicluster upgrade postgres-availability telemetry capture-overhead optional-components management-placement } list_bucket() { @@ -431,6 +438,7 @@ list_bucket() { bot-heavy) bucket_bot_heavy ;; multicluster) bucket_multicluster ;; upgrade) bucket_upgrade ;; + postgres-availability) bucket_postgres_availability ;; telemetry) bucket_telemetry ;; capture-overhead) bucket_capture_overhead ;; optional-components) bucket_optional_components ;; diff --git a/test/e2e/env.go b/test/e2e/env.go index 35e3da008..abd376d19 100644 --- a/test/e2e/env.go +++ b/test/e2e/env.go @@ -571,6 +571,11 @@ func (c *APIClient) Close() { // marshalled as JSON and the Content-Type header is set. CSRF header is // attached for mutating methods. func (c *APIClient) Do(method, path string, body any) (*http.Response, []byte, error) { + return c.DoContext(context.Background(), method, path, body) +} + +// DoContext performs an authenticated request within the caller's lifetime. +func (c *APIClient) DoContext(ctx context.Context, method, path string, body any) (*http.Response, []byte, error) { var br io.Reader if body != nil { b, err := json.Marshal(body) @@ -579,7 +584,7 @@ func (c *APIClient) Do(method, path string, body any) (*http.Response, []byte, e } br = bytes.NewReader(b) } - req, err := http.NewRequestWithContext(context.Background(), method, c.BaseURL+path, br) + req, err := http.NewRequestWithContext(ctx, method, c.BaseURL+path, br) if err != nil { return nil, nil, fmt.Errorf("new request: %w", err) } diff --git a/test/e2e/helm_install_e2e_test.go b/test/e2e/helm_install_e2e_test.go index ffa62e6aa..4c6ba7b7e 100644 --- a/test/e2e/helm_install_e2e_test.go +++ b/test/e2e/helm_install_e2e_test.go @@ -17,6 +17,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/yaml" ) @@ -221,12 +222,58 @@ func TestHelmInstall_APIHealthz(t *testing.T) { // metricsProbeScript runs in a transient curl pod. It prints the public // API port's status code for /metrics, flags a Prometheus body there, and // reports whether the dedicated metrics port serves Prometheus text. -const metricsProbeScript = `code=$(curl -s -o /dev/null -w '%{http_code}' --max-time 5 http://gameplane-api/metrics) +const metricsProbeScript = `code=$(curl -sS -o /tmp/public-metrics -w '%{http_code}' --max-time 5 http://gameplane-api/metrics) || exit 1 echo "public-status=$code" -if curl -s --max-time 5 http://gameplane-api/metrics | grep -q '^# HELP'; then echo "public-body=metrics"; fi -if curl -fsS --max-time 5 http://gameplane-api:9090/metrics | grep -q '^# HELP go_goroutines'; then echo "metrics-port=ok"; fi +if grep -q '^# HELP' /tmp/public-metrics; then echo "public-body=metrics"; fi +if curl -fsS -o /tmp/dedicated-metrics --max-time 5 http://gameplane-api:9090/metrics; then + if grep -q '^# HELP go_goroutines' /tmp/dedicated-metrics; then + echo "metrics-port=ok" + else + echo "metrics-port=unexpected-body" + head -n 10 /tmp/dedicated-metrics + fi +fi exit 0` +// Read completed Pod logs instead of attaching after its command has started. +// A fast probe can print its verdict before kubectl run establishes the stream. +func metricsProbeOutput(ctx context.Context, name string) (string, error) { + const ns = "gameplane-system" + noToken := false + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, AutomountServiceAccountToken: &noToken, + Containers: []corev1.Container{{Name: "probe", Image: "curlimages/curl:8.10.1", Command: []string{"sh", "-c", metricsProbeScript}}}, + }} + pods := envInstance.K8s.CoreV1().Pods(ns) + if _, err := pods.Create(ctx, pod, metav1.CreateOptions{}); err != nil { + return "", fmt.Errorf("create metrics probe: %w", err) + } + defer func() { + cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) + defer cancel() + _ = pods.Delete(cleanupCtx, name, metav1.DeleteOptions{}) + }() + var phase corev1.PodPhase + if err := wait.PollUntilContextCancel(ctx, 250*time.Millisecond, true, func(ctx context.Context) (bool, error) { + current, err := pods.Get(ctx, name, metav1.GetOptions{}) + if err != nil { + return false, err + } + phase = current.Status.Phase + return phase == corev1.PodSucceeded || phase == corev1.PodFailed, nil + }); err != nil { + return "", fmt.Errorf("wait for metrics probe (phase %s): %w", phase, err) + } + out, err := pods.GetLogs(name, &corev1.PodLogOptions{Container: "probe"}).DoRaw(ctx) + if err != nil { + return "", fmt.Errorf("read metrics probe logs: %w", err) + } + if phase != corev1.PodSucceeded { + return string(out), fmt.Errorf("metrics probe pod finished in phase %s", phase) + } + return string(out), nil +} + // TestHelmInstall_MetricsNotOnPublicPort — the API's public port (the // Service port the Ingress and the web front end route to) does not serve // Prometheus metrics, and the dedicated metrics port that the ServiceMonitor @@ -240,15 +287,9 @@ func TestHelmInstall_MetricsNotOnPublicPort(t *testing.T) { // Random suffix so an Eventually retry doesn't collide with a // not-yet-cleaned-up pod from the previous tick. name := fmt.Sprintf("metrics-probe-%d", time.Now().UnixNano()) - o, err := envInstance.Kubectl( - t.Context(), - "run", "-n", "gameplane-system", - "--rm", "--restart=Never", "--attach", - "--image=curlimages/curl:8.10.1", - name, - "--command", "--", - "sh", "-c", metricsProbeScript, - ) + ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second) + defer cancel() + o, err := metricsProbeOutput(ctx, name) if err != nil { return false, fmt.Sprintf("metrics probe pod failed: %v\n%s", err, o) } diff --git a/test/e2e/specs.md b/test/e2e/specs.md index 2954c54b1..6a85a30c2 100644 --- a/test/e2e/specs.md +++ b/test/e2e/specs.md @@ -91,7 +91,7 @@ This is a name-matching contract, not a structural one: ## Conventions (already normative via `CLAUDE.md`, restated here for the module's own record) -- **`t.Parallel()`**: tests call it unless shared infrastructure requires serialization. The `ratelimit` bucket deliberately drains the shared login limiter; gateway parity temporarily changes the shared remote Helm release and trust. These are documented exceptions. +- **`t.Parallel()`**: tests call it unless shared infrastructure requires serialization. The `ratelimit` bucket deliberately drains the shared login limiter; gateway parity temporarily changes the shared remote Helm release and trust; `postgres-availability` (`TestUpgrade_PostgresAvailability`) upgrades the shared release and interrupts PostgreSQL. These are documented exceptions. - **Per-test unique resource names**: CRs, namespaces, and users a test creates get a name unique to that test run (commonly time-based, e.g. `CreateUser`'s `fmt.Sprintf("%s-%d", prefix, time.Now().UnixNano())`) so parallel tests never collide on a shared cluster. - **`ociPushMu`**: a package-level mutex guarding tests that push through the shared, fixed-name `oras-push` Job (`module_e2e_test.go`, `module_verify_e2e_test.go`). Because the Job's name and namespace are fixed fixtures, two tests racing to apply/wait on it would stomp each other; `ociPushMu.Lock()`/`Unlock()` serializes them. - **`ensureResticRepo(t)`**: guards tests that run a backup against the shared restic repository fixture (`backup_e2e_test.go`, `backupschedule_e2e_test.go`, `failure_paths_e2e_test.go`) — restic repo initialization is not safe to race. @@ -183,6 +183,7 @@ This module *is* a test tier; it carries no unit tests of its own and no `.testc - **`buckets.sh verify`** — the "e2e bucket coverage" CI job, structural proof every test is in exactly one bucket. - **CI's per-bucket jobs** — `operator`, `api-auth`, `api-roles`, `api-rbac`, `api-agent`, `api-mods`, `ratelimit`, `bot-fast`, `multicluster`, `upgrade` all run against a real kind cluster per job; `bot-heavy` is bucketed but never scheduled (see Non-goals). +- **`postgres-availability`** runs alone at the end of the `e2e postgres` job. `TestUpgrade_PostgresAvailability` creates a user/session before migrating a live combined PostgreSQL Deployment from RollingUpdate to Recreate, watches API containers for overlapping writers, verifies the populated audit chain, then scales the database fixture down/up. A direct API Pod connection verifies failing readiness, healthy liveness, generic session 503s, no API restart, and recovery with the original session. It must not run in parallel with other tests because it upgrades the shared release and interrupts storage. - **`joincoverage.sh`/`joincoverage_test.sh`** — reports which games/protocols the bot suite actually reaches JoinDepth on, cross-referenced with `internal/specs.md`'s JoinDepth model. ## References diff --git a/test/e2e/upgrade_e2e_test.go b/test/e2e/upgrade_e2e_test.go index 65d9007b9..7a7dcfefb 100644 --- a/test/e2e/upgrade_e2e_test.go +++ b/test/e2e/upgrade_e2e_test.go @@ -371,7 +371,7 @@ func TestUpgrade_FromPreviousRelease(t *testing.T) { // --timeout without --wait: hooks block `helm install` until they finish // regardless of --wait, so the CRD-apply Job has completed and the schema - // is settled when this returns. operator.replicas=0 / api.replicas=0 + // is settled when this returns. operator.replicas=0 / api.enabled=false // because only the hook matters here. gamesNamespace is fresh so the // install cannot race the uninstalled release's games Namespace deletion. chartStamp := manifestCRDStamp(t, "gameplane.local_gameservers.yaml") @@ -386,7 +386,7 @@ func TestUpgrade_FromPreviousRelease(t *testing.T) { "--set", "operator.agentImage=gameplane-test/agent:"+tag, "--set", "operator.leaderElect=false", "--set", "operator.replicas=0", - "--set", "api.replicas=0", + "--set", "api.enabled=false", "--set", "gamesNamespace=e2e-crd-reinstall-games", "--set", "defaultModuleSource.enabled=false", "--timeout", "3m", diff --git a/web/nginx.conf.template b/web/nginx.conf.template index b20b738da..2aff04eab 100644 --- a/web/nginx.conf.template +++ b/web/nginx.conf.template @@ -111,6 +111,12 @@ server { return 200 "ok\n"; } + # The database readiness slot belongs to kubelet probes on the API's + # internal port. Public requests must never compete for that slot. + location = /readyz { + return 404; + } + location / { # 1) Real file on disk (index.html, favicon, manifest, …) → serve it. # 2) Otherwise fall through to the @backend named location, which diff --git a/web/specs.md b/web/specs.md index d24b3f964..38779d645 100644 --- a/web/specs.md +++ b/web/specs.md @@ -42,6 +42,8 @@ The Gameplane dashboard is a React SPA providing a UI layer over the Gameplane A **API-only:** The dashboard never speaks directly to the operator or agent. All reads/writes flow through the Kubernetes API server and the REST/WebSocket API gateway (`api/`). +**Internal readiness:** Dashboard nginx returns 404 for `/readyz`, including query strings and normalized paths, before SPA routing or API proxying. Kubelet probes reach the API directly; public traffic cannot consume its database readiness slot. The nginx proxy smoke test checks rejection against a live upstream and confirms ordinary proxying still works. + **Pre-auth privacy:** The login page and any unauthenticated screen must not leak internal state: no hostnames, cluster names, server counts, version strings, user lists, or "user not found" errors. See docs/security.md "pre-auth privacy" and CLAUDE.md rule 3. **Cluster-bound WebSocket:** Server streams carry the open server's explicit cluster and namespace. Reconnects retain that target; leaving the server view closes its streams. Changing an infrastructure selection cannot retarget a stream. Pod logs and PTY attach use the target Kubernetes client; remote agent streams use the configured authenticated gateway and fail closed when unavailable. @@ -1087,6 +1089,7 @@ Each namespace is an object of typed functions building and fetching URLs: ## Internationalization (spec 019) - **Provider.** `src/lib/i18n/I18nProvider.tsx` wraps the whole app inside `QueryClientProvider` in `src/main.tsx`, so `/login` and `/share/*` are covered. It renders react-intl `IntlProvider` and react-aria `I18nProvider` with the same locale, and exposes `useLanguage()` (`language`, `setLanguage`, `direction`). Outside a provider `useLanguage()` and `useFormat()` fall back to English. +- **Template picker.** `PickTemplate` in `src/routes/CreateServer.tsx` uses `useOptionalIntl()` for its existing messages: it follows the active locale inside the provider and falls back to English when rendered without one. - **App wiring** (`AppI18nProvider`, same file). `src/main.tsx` mounts `AppI18nProvider` rather than the bare provider. It supplies the two server inputs to resolution: the user's saved language, read from the cached `["me"]` profile (`preferences.language`) without starting a fetch, so the route guards stay the only `/users/me` caller and the value updates when that cache entry changes; and the instance default from `GET /auth/locale` (`Auth.locale()`, query key `["auth-locale"]`, `staleTime: Infinity`, no retry). A failed locale request leaves the instance default unset, so resolution falls through to `en`. `Users.setLanguage(tag | null)` (`PUT /users/me/language`) is the write path for the saved choice; the picker that calls it is not yet built. - **Resolution order** (`src/lib/i18n/resolve.ts`): user choice, then browser choice (storage), then each `navigator.languages` entry (regional, then base language, so `fr-CA` resolves to `fr`), then the instance default, then `en`. Candidates that are not listed languages are skipped. Listing comes only from the generated manifest. - **Storage key.** `localStorage["gameplane-language"]` holds `{choice, applied}`. `choice` is the viewer's pick on this browser; `applied` is the language last rendered. Every access is guarded, and failures mean "no preference". diff --git a/web/test-mod-upload-proxy.py b/web/test-mod-upload-proxy.py index 07a73c6f7..e8644a86b 100644 --- a/web/test-mod-upload-proxy.py +++ b/web/test-mod-upload-proxy.py @@ -18,6 +18,14 @@ class UploadSink(BaseHTTPRequestHandler): received = [] + get_requests = [] + + def do_GET(self): + self.get_requests.append(self.path) + self.send_response(200) + self.send_header("Content-Length", "2") + self.end_headers() + self.wfile.write(b"ok") def do_POST(self): length = int(self.headers["Content-Length"]) @@ -72,6 +80,24 @@ def main(): subprocess.run(["docker", "logs", name], check=False) raise time.sleep(0.2) + # A live upstream and working ordinary proxy are positive controls: + # only the kubelet's database readiness path must stop at nginx. + with urllib.request.urlopen(f"http://127.0.0.1:{sink.server_port}/readyz", timeout=5) as response: + assert response.status == 200 + with urllib.request.urlopen("http://127.0.0.1:8080/healthz", timeout=5) as response: + assert response.status == 200 + for path in ("/readyz", "/readyz?probe=public", "/%72eadyz", "//readyz"): + for accept in ("application/json", "text/html", "*/*"): + request = urllib.request.Request("http://127.0.0.1:8080" + path, + headers={"Accept": accept}) + try: + urllib.request.urlopen(request, timeout=5) + except urllib.error.HTTPError as error: + assert error.code == 404, (path, accept, error.code) + error.close() + else: + raise AssertionError(("public readiness request succeeded", path, accept)) + assert UploadSink.get_requests == ["/readyz", "/healthz"], UploadSink.get_requests upload = Path(directory) / "default-limit.jar" with upload.open("wb") as stream: stream.truncate(file_mib * MIB)