docs(juicefs): L6 design — scoped meta role, and why the pooler option was wrong here - #2613
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9da6137f99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Reviewed both findings against source. Both confirmed. 1. The migration target and the privilege it needsVerified in
So Step 1 as written points at the target that cannot do the work. Worth noting the implementation PR (#2614) already anticipates this — its For this doc the fix is small: name 2. Cut over before rotating — confirmed
No branch, no variable — every cross-node join authenticates as the superuser. And #2614 creates Follow Steps 1–3 in order and you rotate or expose a credential that is still the only one in use. The scoped role exists but nothing consumes it, so the exposure surface is unchanged while the doc reads as though it has been reduced. That is the worst shape for a security handoff — it retires the operator's attention on a problem that is still live. Sequencing that holds: create the scoped role → grant VerdictThe analysis in this doc is good — particularly the section on why the pooler option was wrong here, which is the kind of negative result that saves the next person from re-deriving it. Both findings are about sequencing and target-naming rather than the design. Holding with #2614 so the pair lands together and in order. |
|
Root-cause + remediation analysis posted on #2614 (the implementation side), since the fix lands there. Two things from it change what this design doc should say: 1. The node-conditional premise does not hold. The doc and the migration both treat "schema That measurement was taken on a node which had not run bootstrap since the seed landed (2026-06-28). So schema presence distinguishes bootstrapped from not-yet-bootstrapped, not metadata-home from gateway. Anything here reasoning from that distinction needs revisiting. 2. The failure window is narrower than "it never applies", and worse for it. On an already-bootstrapped database — B850, where this was developed — the migration works. On a fresh database it is recorded-but-skipped and never applies again. So the control validates where it was built and is absent on any node rebuilt from scratch. For a handoff whose whole subject is retiring a superuser credential, that is worth stating outright: the scoped role would be missing precisely during a rebuild, which is when someone is most likely to reach for the superuser to get things moving. Neither finding changes the design's direction. Scoped role, DML only, no CREATE on the schema, NOLOGIN until a password exists — that is all correct, and the section on why the pooler option was wrong here is the kind of negative result worth keeping. It is the placement and the preconditions that need the edit. Still held with #2614 so the pair lands together and in order. |
…n was wrong here
Records the L6 cross-node design plus the documentation review that overturned my
own first recommendation. The reasoning is the durable part.
Blocker mechanism: supabase-db sits on pmoves_api + pmoves_data, both internal:true.
Docker installs no DNAT for internal-only networks, so published ports are recorded
but never plumbed — b850:5432 measured unreachable from BOTH z890 and nano-1. The
in-repo NATS precedent (docker-compose.yml:2906) already solves this by multi-homing
onto pmoves_external.
CORRECTION — the pooler recommendation was wrong for this deployment:
* Two official sources conflict. Supabase PLATFORM docs say 5432=session /
6543=transaction and that transaction mode does not support prepared statements.
Supavisor PROJECT docs say mode is set by `mode_type` ON THE USER, not by port.
The port convention is hosted-platform behaviour; self-hosted resolves mode from
the user row, and this stack sets POOLER_POOL_MODE=transaction. So "use 5432 for
session mode" does not hold here.
* JuiceFS docs never mention poolers and point to the lib/pq driver, which uses the
extended query protocol (prepared statements) — transaction mode would break it.
* Decisive against the actual deployment: _supavisor.tenants and _supavisor.users
are BOTH 0 rows, on z890 AND B850. The pooler is Up (healthy) answering
/api/health 204s, but has never been provisioned to pool anything — its health
check probes the API, not tenant existence. Exposing it would publish a port that
cannot serve a connection. (Same shape as Archon reporting container-healthy while
ready:false for three days.)
Decision: expose supabase-db multi-homed onto pmoves_external, bound to the TAILNET
interface only — not 0.0.0.0. The NATS precedent defaults to 0.0.0.0 on the grounds
that NATS is credential-guarded; that is weaker for a database, and weaker still
while the admin credential is un-rotated. Supavisor stays a future lane (provision
tenant + session-mode user + verify lib/pq), not a step in this one.
Ordering refinement: scoped role FIRST, then rotate, then expose. Once JuiceFS
authenticates as a single-schema role, the pending admin rotation no longer touches
the mount at all — it shrinks the rotation's blast radius instead of widening it.
The migration SQL is included inline, reviewed and ready. It is not yet committed to
pmoves/supabase/migrations/ because that path is protected and needs an
operator-set KNOWN_ROAD=migrations:handoff:<this file>; an agent must not self-grant
a protected-path bypass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e rotation Two review findings, both confirmed against source and now fixed. 1. TARGET. Step 1 named `make -C pmoves supa-migrate`, which cannot do the work: it connects as -U postgres, and postgres is NOT superuser in the hardened Supabase image (verified on supabase/postgres:17.6.1.108 -- postgres rolsuper=false, supabase_admin rolsuper=true), so it cannot grant on supabase_admin-owned objects. It also applies no seeds and keeps no ledger. The target is `supabase-bootstrap`, which connects as supabase_admin. The target FILE changed too: the DDL is now a seed (supabase/initdb/00_3_juicefs_meta_role.sql), not a migration. As a migration it ran before its own schema existed on a fresh database, guarded, returned cleanly -- and a clean return is exit 0, which apply_dir records as applied. Reproduced and fixed in #2614; ledger semantics written up in pmoves/docs/services/supabase/MIGRATION_WORKFLOW.md. A third mention further down still said supa-migrate after the header was fixed. Caught on a re-grep -- fixing the prominent instance and leaving the buried one is how a doc keeps saying the wrong thing. 2. SEQUENCING. The order read role -> rotate -> expose, framed as though creating the role shrinks the rotation's blast radius. It does not. The role is NOLOGIN and juicefs-cross-node-setup.sh:58 still builds postgres://supabase_admin@... unconditionally, with no branch -- so until the mount is repointed the rotation touches it exactly as before. Cutover is now its own numbered step between the two: grant LOGIN, repoint the script and the in-stack defaults, verify a real read through the new credential. Rotation follows a VERIFIED cutover, never precedes it. Otherwise the steps rotate and expose a credential that is still the only one in use, while the document reads as though the exposure had been reduced. Also added to Verification: presence of the role is not the assertion. The privilege shape is, including that has_schema_privilege(...,'public','USAGE') is true by PUBLIC inheritance rather than by anything this seed grants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9da6137 to
056aa07
Compare
L6 cross-node design, plus the documentation review that overturned my own first recommendation. The reasoning is the durable part — this exists so the pooler idea isn't re-proposed on architectural instinct.
Blocker mechanism
supabase-dbsits onpmoves_api+pmoves_data, bothinternal: true. Docker installs no DNAT for internal-only networks, so published ports are recorded but never plumbed —b850:5432measured unreachable from z890 and nano-1. The in-repo NATS precedent (docker-compose.yml:2906) already solves exactly this by multi-homing ontopmoves_external.JuiceFS documents the requirement plainly: "ensure that all nodes has access to the Metadata Engine." There's no way around exposing something.
I first recommended exposing supavisor rather than Postgres, on the architectural grounds that it's the purpose-built front door. Reviewing official docs and then the live deployment overturned it:
Two official sources conflict, and the one I'd cited doesn't apply here:
5432=session,6543=transaction; "Transaction mode does not support prepared statements"mode_typeon the user" — mode is per-user, not per-portThe port convention is hosted-platform behavior. Self-hosted resolves mode from the user row, and this stack sets
POOLER_POOL_MODE=transaction. So "use 5432 for session mode" does not hold here.JuiceFS's docs never mention poolers and point to the
lib/pqdriver, which uses the extended query protocol (prepared statements) — transaction mode would break it.Decisive against the actual deployment:
The pooler is
Up (healthy)answering/api/health204s — but has never been provisioned to pool anything; its health check probes the API, not tenant existence. Exposing it would publish a port that cannot serve a connection. (Same shape as Archon reporting container-healthywhileready:falsefor three days.)Decision
Expose
supabase-dbmulti-homed ontopmoves_external, bound to the tailnet interface only — not0.0.0.0. The NATS precedent defaults to0.0.0.0on the grounds that NATS is credential-guarded; that's weaker for a database, and weaker still while the admin credential is un-rotated. Supavisor stays a future lane (provision tenant + session-mode user + verifylib/pq), not a step in this one.Ordering refinement
Scoped role → rotate → expose (approved). Once JuiceFS authenticates as a single-schema role, the pending admin rotation no longer touches the mount at all — it shrinks the rotation's blast radius rather than widening it, and it's cheap/reversible where rotation is expensive (~27 consumers, one window).
JuiceFS currently authenticates as
supabase_admin(rolsuper=t, rolcreaterole=t) — the same credential leaked for ~11 days and still un-rotated. Exposing a superuser surface is materially different from exposing a single-schema one.Migration status
The SQL is included inline, reviewed and ready — DML on one schema,
NOLOGINuntil CHIT delivers a password, noCREATE/BYPASSRLS/SUPERUSER/CREATEROLE, idempotent, applied viamake -C pmoves supa-migrate(no raw psql).It is not committed to
pmoves/supabase/migrations/because that path is protected and needs an operator-setKNOWN_ROAD=migrations:handoff:<this file>. An agent must not self-grant a protected-path bypass — that gate is the point.🤖 Generated with Claude Code