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) + } + }) + } +}