From d9c1fa3e2177c1fb61c9fe4f20c3c2be5bf9bd02 Mon Sep 17 00:00:00 2001 From: Sean Dean <254259913+distronode-com@users.noreply.github.com> Date: Fri, 4 Sep 2026 04:29:23 -0400 Subject: [PATCH] fix(settings): read the bucket from any replica scheme, not just s3:// MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Storage settings page trimmed a literal "s3://" off LITESTREAM_REPLICA_URL and then cut at the first '/'. For every other scheme Litestream supports, nothing was trimmed and the first '/' found was the one inside "://", so the bucket name came out as the scheme: gcs://my-bucket/calnode -> "gcs:" abs://my-container/calnode -> "abs:" file:///var/lib/calnode -> "file:" s3://my-bucket/calnode -> "my-bucket" (the only case that worked) Measured against the old expression rather than inferred. Hit on a live GCS deployment, where the admin page reported the backup bucket as "gcs:". Replaced with replicaBucket(), which strips any leading :// and takes the authority up to the first path separator. A file:// replica and a bare path yield "" — a file replica has no bucket, and saying so beats inventing one. The empty string stays empty, because backups_configured is computed from the raw value and "" here has to keep meaning "nothing to show". recordingStorage() in livekit_recording.go carried a byte-identical copy of the same parse and now shares the helper. There the consequence was worse than a display bug: with credentials present, a gcs:// or abs:// replica would have sent meeting recordings to a bucket literally named "gcs:". Behaviour for the already-correct cases is unchanged, including the bucket == "" guard that keeps recording storage reporting not-ready for a file replica. Tests: a table over every scheme (s3, gcs, abs, file, bare path, empty, scheme only, unknown scheme) against the parser, plus a GET /v1/settings/storage test that pins backups_bucket and backups_configured at the boundary the bug was reported from. Verified: gofmt -l . empty, go vet ./... clean, go test ./... exit 0 (26 packages). --- internal/handler/livekit_recording.go | 5 +- internal/handler/storage_settings.go | 28 ++++++++-- .../handler/storage_settings_internal_test.go | 39 ++++++++++++++ internal/handler/storage_settings_test.go | 54 +++++++++++++++++++ 4 files changed, 117 insertions(+), 9 deletions(-) create mode 100644 internal/handler/storage_settings_internal_test.go create mode 100644 internal/handler/storage_settings_test.go diff --git a/internal/handler/livekit_recording.go b/internal/handler/livekit_recording.go index 33eafb4..1cf78bb 100644 --- a/internal/handler/livekit_recording.go +++ b/internal/handler/livekit_recording.go @@ -39,10 +39,7 @@ func recordingStorage() (livekit.S3Config, bool) { if replica == "" || key == "" || secret == "" { return livekit.S3Config{}, false } - bucket := strings.TrimPrefix(replica, "s3://") // s3://bucket/path → bucket - if i := strings.IndexByte(bucket, '/'); i >= 0 { - bucket = bucket[:i] - } + bucket := replicaBucket(replica) // s3://bucket/path → bucket, and the same for gcs/abs if bucket == "" { return livekit.S3Config{}, false } diff --git a/internal/handler/storage_settings.go b/internal/handler/storage_settings.go index 9254e6f..ce0fdec 100644 --- a/internal/handler/storage_settings.go +++ b/internal/handler/storage_settings.go @@ -11,20 +11,38 @@ import ( // backups (configured via environment, so read-only here) and meeting recordings (which reuse // the same bucket under a recordings/ prefix). The only editable knob is the recording toggle. +// replicaBucket extracts the bucket (or Azure container) name from a LITESTREAM_REPLICA_URL. +// +// Litestream replicates to more than S3 — `gcs://`, `abs://` and `file://` are all valid — so +// the scheme is stripped generically rather than by trimming a literal "s3://". Trimming only +// s3 left every other scheme's URL intact, and the first '/' then found was the one inside +// "://": a `gcs://my-bucket/calnode` replica reported its bucket as "gcs:". +// +// Whatever is left after the scheme is the authority, up to the first path separator. For +// `file:///var/lib/calnode` that is empty, which is the honest answer — a file replica has no +// bucket — and for the empty string it stays empty, so callers can still tell "not configured" +// from "configured with no bucket". +func replicaBucket(replica string) string { + rest := replica + if i := strings.Index(rest, "://"); i >= 0 { + rest = rest[i+len("://"):] + } + if i := strings.IndexByte(rest, '/'); i >= 0 { + rest = rest[:i] + } + return rest +} + // GetStorageSettings handles GET /v1/settings/storage (admin). func (h *Handler) GetStorageSettings(w http.ResponseWriter, r *http.Request) { if _, ok := h.requireAdmin(w, r); !ok { return } replica := os.Getenv("LITESTREAM_REPLICA_URL") - bucket := strings.TrimPrefix(replica, "s3://") - if i := strings.IndexByte(bucket, '/'); i >= 0 { - bucket = bucket[:i] - } _, recReady := recordingStorage() h.writeJSON(w, http.StatusOK, map[string]any{ "backups_configured": replica != "", - "backups_bucket": bucket, + "backups_bucket": replicaBucket(replica), "backups_endpoint": os.Getenv("LITESTREAM_ENDPOINT"), "recordings_enabled": h.recordingsEnabled(r.Context()), "recordings_storage_ready": recReady, diff --git a/internal/handler/storage_settings_internal_test.go b/internal/handler/storage_settings_internal_test.go new file mode 100644 index 0000000..2ee3300 --- /dev/null +++ b/internal/handler/storage_settings_internal_test.go @@ -0,0 +1,39 @@ +package handler + +import "testing" + +// TestReplicaBucket covers every replica scheme Litestream accepts, because the Storage +// settings page used to trim a literal "s3://" and reported a `gcs://my-bucket/calnode` +// replica's bucket as "gcs:" — the first '/' it found was the one inside "://". +func TestReplicaBucket(t *testing.T) { + cases := []struct { + name string + replica string + want string + }{ + {"s3", "s3://my-bucket/calnode", "my-bucket"}, + {"gcs", "gcs://my-bucket/calnode", "my-bucket"}, + {"azure blob storage", "abs://my-container/calnode", "my-container"}, + {"s3 with no path", "s3://my-bucket", "my-bucket"}, + {"gcs with no path", "gcs://my-bucket", "my-bucket"}, + {"deep path", "gcs://my-bucket/calnode/db/replica", "my-bucket"}, + {"dots and dashes in the bucket", "s3://calnode.backups-eu/db", "calnode.backups-eu"}, + // A file replica has no bucket, and saying so is more useful than inventing one. + // The settings page renders an em dash for the empty value. + {"file, absolute path", "file:///var/lib/calnode/backups", ""}, + {"bare absolute path", "/var/lib/calnode/backups", ""}, + // Not configured must stay empty: the page reports backups_configured separately, + // from the raw value, so "" here has to mean "nothing to show" and nothing else. + {"empty", "", ""}, + // Degenerate but well-defined rather than crashing or guessing. + {"scheme only", "gcs://", ""}, + {"unknown scheme", "wat://some-bucket/path", "some-bucket"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := replicaBucket(c.replica); got != c.want { + t.Errorf("replicaBucket(%q) = %q; want %q", c.replica, got, c.want) + } + }) + } +} diff --git a/internal/handler/storage_settings_test.go b/internal/handler/storage_settings_test.go new file mode 100644 index 0000000..e878ed4 --- /dev/null +++ b/internal/handler/storage_settings_test.go @@ -0,0 +1,54 @@ +package handler_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" +) + +// TestGetStorageSettings_reportsTheBucketForEveryReplicaScheme pins the bug at the boundary +// it was reported from: the admin Storage page showed "gcs:" as the bucket name for a live +// GCS deployment. The parser has its own table test; this one proves the page is wired to it. +func TestGetStorageSettings_reportsTheBucketForEveryReplicaScheme(t *testing.T) { + cases := []struct { + name string + replica string + wantBucket string + wantConfig bool + }{ + {"gcs", "gcs://my-bucket/calnode", "my-bucket", true}, + {"s3", "s3://my-bucket/calnode", "my-bucket", true}, + {"azure blob storage", "abs://my-container/calnode", "my-container", true}, + // Configured, but with no bucket to name: the page renders an em dash. + {"file", "file:///var/lib/calnode/backups", "", true}, + {"unset", "", "", false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + h, _, ownerKey, _ := setupWorkspaceWithDB(t) + t.Setenv("LITESTREAM_REPLICA_URL", c.replica) + + req := authReq(http.MethodGet, "/v1/settings/storage", "", ownerKey) + rec := httptest.NewRecorder() + h.RequireAuth(h.GetStorageSettings)(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("GET storage settings: got %d; want 200 — %s", rec.Code, rec.Body.String()) + } + + var got struct { + BackupsConfigured bool `json:"backups_configured"` + BackupsBucket string `json:"backups_bucket"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode response: %v — %s", err, rec.Body.String()) + } + if got.BackupsBucket != c.wantBucket { + t.Errorf("backups_bucket for %q: got %q; want %q", c.replica, got.BackupsBucket, c.wantBucket) + } + if got.BackupsConfigured != c.wantConfig { + t.Errorf("backups_configured for %q: got %v; want %v", c.replica, got.BackupsConfigured, c.wantConfig) + } + }) + } +}