Skip to content

Parse any replica URL scheme, not just s3:// - #26

Open
distronode-com wants to merge 1 commit into
Calnode:mainfrom
distronode-com:fix/replica-url-scheme
Open

Parse any replica URL scheme, not just s3://#26
distronode-com wants to merge 1 commit into
Calnode:mainfrom
distronode-com:fix/replica-url-scheme

Conversation

@distronode-com

Copy link
Copy Markdown

A gcs:// replica URL displays as gcs: in Settings → Storage

GetStorageSettings trimmed a literal "s3://" off LITESTREAM_REPLICA_URL and then cut
at the first /. For any other scheme Litestream supports, nothing was trimmed and the
first / found was the one inside ://, so the bucket name came out as the scheme.

Measured against the old expression rather than inferred:

LITESTREAM_REPLICA_URL old backups_bucket
s3://my-bucket/calnode my-bucket (the only case that worked)
gcs://my-bucket/calnode gcs:
abs://my-container/calnode abs:
file:///var/lib/calnode file:

We hit this on a live GCS deployment: the admin page reported the backup bucket as gcs:.

The fix

A small replicaBucket() helper strips any leading <scheme>:// and takes the authority
up to the first path separator. No new dependency, no net/url round-trip.

  • s3://, gcs://, abs:// → the bucket or container name.
  • file:///var/lib/calnode and a bare path → "". A file replica has no bucket, and
    saying so beats inventing one; the page already renders an em dash for an empty value.
  • "" stays "", because backups_configured is computed from the raw value and ""
    here has to keep meaning "nothing to show".

One thing beyond the reported bug

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. Happy to split this into its own commit if you would rather review it
separately.

Testing

go test ./... green (26 packages); gofmt -l . empty; go vet ./... clean.

  • TestReplicaBucket — a table over s3, gcs, abs, file, a bare path, the empty string, a
    scheme with no bucket, and an unknown scheme.
  • TestGetStorageSettings_reportsTheBucketForEveryReplicaScheme — the same cases through
    GET /v1/settings/storage, pinning backups_bucket and backups_configured at the
    boundary the bug was reported from. There were no tests for this endpoint before.

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 <scheme>:// 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).

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes in this run: full diff of PR #26 (scheme-agnostic replica URL bucket parse for settings display and recording storage).

  • replicaBucket helper — Strips any leading <scheme>:// then takes the authority up to the first /, so gcs:// / abs:// no longer report the scheme as the bucket.
  • Shared call sitesGetStorageSettings and recordingStorage() both use the helper; file:// / empty still yield no bucket and keep the existing empty-bucket guard.
  • Tests — Table unit coverage plus a GET /v1/settings/storage boundary test that would have failed on the old gcs: bug. Local run of those tests passed.

Pullfrog  | View workflow run | Using Grok𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant