From 8741843d2e884783eed1c00bbc2293e526f3e80d Mon Sep 17 00:00:00 2001 From: Diego Imbert <70353967+diegoimbert@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:45:47 +0200 Subject: [PATCH] feat: external instance cluster for data tables and Ducklake catalogs (#11197) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix pg_dump stuck on version 17 on nix * fix(datatables): refuse a malformed role annotation instead of ignoring it `-- Role operator`, `-- role operator;` and `-- role operator -- why` all failed the annotation parser's exact-match rule, so the query fell through to the data table's default role and ran, silently, under a login the author did not choose. Naming a role exists precisely to not do that. A leading comment whose first word is `role` is now an annotation attempt: the keyword matches case-insensitively, one trailing `;` is tolerated, and anything else is an error naming the line. Only callers that already know the target is a `datatable://` reference ever run this, so ordinary SQL keeps its comments. Also bumps the dev shell's postgres client to 18 — it trailed the server the dev database runs, which takes out every data table export, clone and fork-with-data. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): refuse a malformed role query string instead of ignoring it `?Role=analytics`, `?role=` and `?x=1&role=…` all fell through the reference parser's exact-match rule, so the connection resolved to the data table's default role and ran under a login the caller never asked for — the URI half of the same trap as a malformed `-- role` annotation. The key now matches case-insensitively, and anything else in the query string is an error naming it; `role` is the only parameter a reference takes. Callers that only need the entry keep a lenient `datatable_ref_name`, since they never act on the role. The DuckDB `ATTACH` parser propagates it rather than attaching under the default. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): carry the role annotation into the row_to_json retry The retry rebuilds its SQL from `pruneComments(code)`, so the leading comment block never reached the second attempt — and with it the `-- role ` line that decides which login the query runs as. The retry connected as the data table's default role instead, so a query the first attempt was denied could succeed on the second, reported as "recovered with the row_to_json fix". Carry the leading comment block over. The retry itself is unchanged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * chore(datatables): don't mount the roles UI until the ACL editor lands Enforcement ships first. The permissions drawer is what turns roles on, and the catalog section is what creates them — both are only useful once there is a way to grant a role the privileges it needs, which arrives with the ACL editor. Left mounted they would offer a feature whose other half does not exist. The two components are complete and reviewed; only their call sites here are commented out, with a note pointing the follow-up PRs at them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): honour `-- role: x`, and fix the DuckDB attach test Two review findings, both real. `attach_datatable_parses_name_and_role` never compiled: `parse_attach_datatable` returns `Result>` now and one call site kept a single `unwrap`. Its `?Role=analytics` case also asserted a refusal, contradicting the parser in the same commit, which matches the key case-insensitively. Replaced with the cases that are genuinely malformed, and a positive one for the cased key. `-- role: analytics` fell through to the default role — the silent fallback the strict parser exists to remove, for the spelling most likely to be typed. The keyword now accepts an optional colon, attached or spaced, while a word that merely starts with it (`rolebased`) is still not an attempt. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): clone a fork's pointer instead of failing after the copy Forking a fork with cloning left an orphan database. The preflight resolves the pointer and sees the governing entry, so both endpoints ran and filled the new database; `apply_forked_datatable` then refused the inherited pointer and rolled the fork back, stranding a registered `wm_fork_*` that no entry names and whose name blocks the retry. Refusing earlier would have been the smaller change, but forking a fork and cloning worked before pointers existed, so it would trade an orphan for a regression. Resolve what the pointer names and write the terminal entry the clone needs: the whole `database` object rather than a patch of its `resource_path`, since a pointer has none, and `reference` removed with it. Also accepts `-- role=x` and `-- Role = x`, two more spellings that fell through to the default role. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): refuse to roll back the catalog while roles exist The down migration dropped the table and left every role behind: live Postgres logins whose passwords only that table carried, so after a revert Windmill could neither use, disable nor delete them, and re-applying could not recreate them because the names were taken. Cleaning up here is not possible either — dropping a role means reassigning what it owns in every instance database, and a migration runs in one — so it now refuses while the catalog is non-empty and says to delete the roles through instance settings, which does the cluster work. Also enforces the instance-only invariant the resolved-pointer clone relies on rather than only asserting it in a comment. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * refactor(datatables): settle clonability in one place, before anything is created A clone is three stages a workspace apart — `create_pg_database`, then `import_pg_database`, then `apply_forked_datatable` inside the fork transaction. Only the third can roll back, and `CREATE DATABASE` is not transactional, so any refusal that lives there strands a registered `wm_fork_*` that no entry names and whose name blocks the retry. That orphan has now been fixed three times, most recently reintroduced by a guard added one commit ago. Patching each new refusal into the first endpoint is not the fix; having two places that can refuse is. `ensure_datatable_is_clonable` now answers every reason a copy can be refused and returns what it resolved, and the stage that writes the entry only does the work. Also takes an ACCESS EXCLUSIVE lock before the rollback guard counts, so a role created concurrently cannot slip between the check and the drop. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): let a retried clone reclaim its own leftover database A clone creates its target database one request before it copies into it, and the fork that would name it is written a request after that. Any failure in between — a pg_dump error, a bad restore, a dropped connection, the source's roles changing mid-flow — left a registered `wm_fork_*` that no entry names, and every retry then failed on its name. This predates data table roles. `create_pg_database` now reclaims such a leftover before creating: only a `wm_fork_*` database Windmill registered as a data table database and that no data table or ducklake entry names, in any workspace, archived ones included. The drop never terminates connections, so a clone still copying into it makes the reclaim fail instead of being cut off. It is limited to callers who administer the source — reaching it is not enough, since on a data table without roles every member reaches it — and anyone else gets the refusal an existing database always got. Co-Authored-By: Claude Opus 5 (1M context) * Revert "fix(datatables): let a retried clone reclaim its own leftover database" This reverts commit 7dd3275a10. The reclaim tied the caller to the source they administer, but not to the database it dropped. Between another workspace's import and its final fork request, that workspace's target is full, registered, unnamed and has no open connection, so an admin of any instance data table could name it and have it dropped and recreated empty. The victim's fork would then commit pointing at the empty copy. Safe reclaim needs durable clone ownership and serialization with the request that names the database; until then the leftover stays, as it did before this PR. Co-Authored-By: Claude Opus 5 (1M context) * docs(datatables): record the stale clone database as a known limitation A clone is three requests and `CREATE DATABASE` is not transactional, so a failure after the first leaves a registered `wm_fork_*` behind, as it did before data table roles. Accepted for this PR: it is harmless to data and goes away once the clone is a single server-side operation. The comment also records why the obvious fix is wrong: reclaiming the leftover on retry, without durable clone ownership, can drop another workspace's fully copied database between its import and its final fork request. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): bounce the streams reading a data table when it is deleted Deleting a governing data table, or the workspace that holds it, only collected the fork pointers it stranded, for the warning. A Postgres trigger or capture already streaming through one of those pointers kept the replication connection it opened while the pointer still resolved, so it went on dispatching the governing database's rows after the fork lost access — until its connection happened to restart. The governing workspace's own streams on a deleted entry did the same. Both deletion paths now bounce the affected listeners inside their own transaction, through the helper a permission change already uses, so a listener that reconnects re-resolves the entry and finds it gone. The helper is split so a caller can pass the (workspace, local name) pairs it already holds. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): keep the fork schema baseline, and bounce streams on every removal Three fixes from review. `edit_datatable_config` took `forked_from` wholesale from the stored entry, so the fork schema diff's save of an advanced baseline was silently discarded and an applied change was offered again. Whether an entry carries a clone stamp is still carried from the store, since that is what marks its database droppable, but the baseline inside it is now taken from the request. The stranded-pointer warning and the stream bounce ran over the optional `deleted_datatables` hint, which the settings-sync CLI never sends, so removing a governing data table through `wmill` bounced nothing. Removals are now derived from the stored configuration against the saved one. `delete_workspace` read the pointers to bounce before its transaction, so a fork committing a pointer during the deletion was missed. The read now happens inside the transaction, after the workspace row is deleted: a fork's insert key-share locks that row through its parent foreign key, so it is either seen or fails on the missing parent. Co-Authored-By: Claude Opus 5 (1M context) * refactor(datatables): keep Postgres triggers and data table roles apart A replication stream reads every row of every table whatever the data table's roles grant, and its listener checks access only when it connects. Rather than chase every way access can change and bounce the streams each one affects, a data table now carries one or the other: - a Postgres trigger or capture cannot be created on, or connect to, a data table under roles; - roles cannot be turned on while an enabled trigger or a live capture reads the data table, its own or a fork's through its pointer. The refusal names each one to disable. This removes the stream bounces on roles edits and on data table and workspace deletion, and the trigger gate that admitted admins. The fork schema baseline fix from the same review round is kept. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): refuse a Postgres trigger on a data table under roles when it is saved Creating or editing a trigger that points at a data table under roles was accepted, and its listener then retried the refused connection every 30 seconds forever. The save is now refused, and a trigger that reaches such a data table anyway (re-enabled, or cloned into a fork) is disabled by its listener with the reason, as a missing replication slot is. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): disable a data table role before deleting it Deleting a role reassigns and drops what it owns in each registered database on its own connection, and each of those passes commits as it goes. A database failing part-way left the role enabled in the catalog and able to log in, but already stripped in the databases reached before it. The role is now disabled in its own commit first, so a failed delete leaves a disabled role to retry. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): serialize roles going on with a stream starting Turning roles on looked for enabled triggers and live captures once, without a lock anything starting a stream also took. A trigger enabled in that window could have its listener connect before roles committed, and a healthy listener never checks again. Both transitions now serialize on one advisory lock: roles going on hold it exclusive while they look, and trigger create, edit and enable, and capture setup and ping hold it shared while they commit. Either the look sees the stream, or the listener connects after roles are committed and refuses. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): wait out live listeners, and resolve stored names containing `?` Turning roles on counted a trigger as gone once disabled, and a capture once its client stopped pinging, but the listener keeps its replication connection until its next heartbeat notices. A trigger or capture whose listener pinged in the last 15 seconds, the window a server holds a listener for, now still counts as streaming. Data table names could contain `?` before they were restricted, and such entries are still stored. Splitting `?role=` off a reference misread them: `a?b` became `a` with an unknown parameter, and the clone checks looked at a different entry than the one copied. An entry stored under the whole reference is now looked up first, in the Postgres executor, DuckDB ATTACH and the clone checks. Agent workers cannot read the workspace and keep the strict parse, which refuses such a name rather than misreading it. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): warn when a settings sync strands fork pointers A settings save reported the fork pointers left resolving to nothing only for the names in `deleted_datatables`, which `wmill sync push` never sends. The save now works out what it removed from the locked entries, and the CLI prints the stranded pointers it returns. Also correct the replication helper's contract: no role or admin check makes a replication connection safe, so a data table under roles is refused outright rather than gated as an admin operation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): refuse a save that drops a data table's roles through an undeclared rename A data table's roles follow its entry only through a declared rename. A settings sync sends the whole map and never declares one, so renaming a data table under roles there read as a delete and a new entry on the same database: the new entry carried no roles, and every caller connected as admin. Such a save is now refused, naming both entries. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): no entry without roles may newly reach a database under roles The previous guard only caught a new name replacing an entry under roles. A whole-map save could also repoint an existing entry without roles at that database, or another workspace could point one there, and every caller of that entry would connect as admin. The rule is now stated on the saved entries: one that carries no roles and newly points at an instance database any entry under roles uses, in this workspace or another, is refused. A declared rename carries its roles and passes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * feat(datatables): move data table role catalog and resolution to the enterprise edition Roles are an Enterprise Edition feature. The catalog, the Postgres logins, CONNECT convergence, tenant evaluation and the role half of connection resolution move to windmill-ee-private. Every public function keeps its path and signature and forwards through datatable_roles_oss, which re-exports the enterprise implementation or, without it, refuses. Without the enterprise edition a data table under roles, or a caller naming a role, is refused a connection rather than resolved as admin, and the reach and admin-access checks refuse one under roles. A data table not under roles resolves as before in every edition, and an instance database keeps the CONNECT grants it was created with. The catalog lock, the stream lock, the tenant cascades and the permissions stripping stay in OSS: they only restrict. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * feat(datatables): move the data table permissions endpoints to the enterprise edition The permissions read, save and usable-roles handlers move to windmill-ee-private; the routes stay registered and, without the enterprise edition, answer that data table roles are an Enterprise Edition feature. ensure_governs_datatable and ensure_reaches_datatable keep their paths: the first refuses, the second passes a data table not under roles. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * feat(datatables): move the data table role catalog endpoints to the enterprise edition The superadmin list, create, update and delete handlers move to windmill-ee-private. The routes stay registered and, without the enterprise edition, refuse after authentication. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * test(datatables): run the roles tests on the enterprise edition, refusals without it Each test that exercises roles runs with private and enterprise. Two tests run without them: every roles route answers the Enterprise refusal, and a data table saved under roles, or a named role, is refused a connection while one not under roles resolves as before. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * feat(datatables): gate the roles UI mount sites on an enterprise license Both mount sites are still commented out; the gate travels with them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * test(datatables): run the tenant matcher test on the enterprise edition The matcher it covers is enterprise code now, so without the enterprise edition the test hit the stub and failed the default windmill-common run. It runs with private and enterprise, and a counterpart without them asserts that no tenant list covers anyone, the wildcard and a workspace admin included. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * chore: update ee-repo-ref to a1873dbb67f2302b85ff5362f8387b48eccdb607 This commit updates the EE repository reference after PR #783 was merged in windmill-ee-private. Previous ee-repo-ref: 5c853e2c20eca6b748415fc0d6862a6ebfb5fec4 New ee-repo-ref: a1873dbb67f2302b85ff5362f8387b48eccdb607 Automated by sync-ee-ref workflow. * fix(datatables): refuse roles while a same-workspace alias reaches the database Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): add an ACL editor for data table roles Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): data table roles in the DB manager and raw apps Co-Authored-By: Claude Opus 5 (1M context) * fix: never add a role to the reference of a data table whose name contains '?' Co-Authored-By: Claude Opus 5 (1M context) * fix: read the roles of a data table whose name contains '?' The generated client leaves a '?' in a path param unencoded, so the lookup 404'd and the raw-app picker blocked Start on such a data table. Co-Authored-By: Claude Opus 5 (1M context) * fix: take every pooled connection before the ACL apply locks Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: refresh grant options only after the ACL apply validates its plan Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix(datatables): refuse a reference naming both a legacy data table and a role When a workspace stores both `sales` and a legacy `sales?role=analytics`, the reference resolved to the legacy entry without a role, so browsing `sales` as `analytics` reached another data table. Co-Authored-By: Claude Opus 5 (1M context) * fix: declare the default role in migrations written for a data table whose name contains '?' Such a data table connects as its default role without naming it, so the migrations the manager wrote for it declared no role and ran as admin. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): let CE migrations connect as an explicitly named admin Co-Authored-By: Claude Opus 5 (1M context) * fix: add only missing grant options before an ACL apply, never default privileges Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix(datatables): serialize roles going on with aliases saved from other workspaces Co-Authored-By: Claude Opus 5 (1M context) * docs(datatables): note that legacy names with ? cannot be migrated Co-Authored-By: Claude Opus 5 (1M context) * fix: run one data table ACL apply at a time per server before it connects Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * feat(datatables): set up an external instance cluster for data tables Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): send external cluster passwords as SCRAM verifiers Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): scope external cluster credential readers to the crate Co-Authored-By: Claude Opus 5 (1M context) * [ee] feat(datatables): external_instance data tables on the external cluster Co-Authored-By: Claude Opus 5 (1M context) * fix: hold the ACL connection to the database that was authorized Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix(datatables): compare the external cluster settings under a row lock before storing setup Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): only drop external databases Windmill marked, and check use under the lock Co-Authored-By: Claude Opus 5 (1M context) * fix: build the ACL connection from the authorized data table entry Resolving the settings again could land on a resource with the same database name on another server, which the later entry checks never see. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: check ACL read reach against the entry it connects from Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * feat(datatables): put a data table's connection under Postgres roles A data table backed by the instance database resolved to exactly one Postgres connection, `custom_instance_user`, for everyone who could reach it at all. There was no way to say this job reads, that one writes, this one never sees the salaries table. A data table role is now a real Postgres login on the cluster, defined once for the instance by a superadmin and named exactly as they named it. A script that declares `-- role analytics` connects as `analytics`, and Postgres decides what it may touch — grants are ordinary SQL. Windmill answers only "may this caller ask for this role", from the tenant lists on the data table entry: `u/alice`, `g/analysts`, `f/finance` or `*`. A data table with no `permissions` block behaves exactly as before. Everything that opens a connection on someone's behalf goes through one chokepoint, `get_datatable_resource_from_db`, which takes the identity explicitly and fails closed when there is none. The role logs in as itself — never `SET ROLE`, which a script could `RESET ROLE` its way out of. A fork's data table entry becomes a pointer at the workspace that governs it rather than a copy of it. The settings clone used to hand a fork a byte-identical entry naming the parent's database, which a fork admin could edit to grant themselves `admin` there; a pointer has nothing local to edit, and its tenants are evaluated as a member of the governing workspace, by email. `permissions` is stripped from the workspace export and ignored on import: tenants name principals of one workspace, and a settings push is not where an access decision should be made. Operations that see the whole database whatever the roles grant stay with the governing workspace's admins: editing the roles, a migration that declares none, and opening a replication stream for a Postgres trigger or capture. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): gate the paths that reach a whole database as admin Auditing what still resolved through the unchecked resolver turned up three that act for a caller and hand back the admin connection: `resolve_pg_source_checked` (behind schema export, the full-schema read, database creation, import and the forked-database drop), the connection test, and the schema snapshot a fork clone takes of its parent. On a data table under roles each let any workspace member — or a fork admin who is nobody in the governing workspace — read or copy the whole database whatever its roles grant. All three now require admin reach on the governing workspace. A dump taken under a restricted role would be a silently truncated copy rather than an error, so refusing is the only right answer for the copy paths. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): confine roles to the instance database, and stop a fork reaching the parent's bookkeeping A data table role is a login on Windmill's own Postgres. Nothing stopped a workspace admin putting a *resource-backed* data table under roles, at which point the executor dialled the host that resource names — one the admin chose — with the role's real cluster password, and `CONNECT` is granted to every registered instance database. Both ends now refuse: the permissions endpoint rejects the save, and the chokepoint refuses to substitute credentials on a non-instance entry rather than trusting the record it read. Two more places reached the governing database without answering to it. The initial-migration generator returned a `pg_dump` of the whole schema to any member. And the migration rename/delete cascade followed a fork's pointer into the parent, so a fork admin renaming or removing their own local entry relabelled or wiped the parent's `_wm_migrations` — after which the parent re-runs every migration from zero. The remote half is now skipped when the entry resolves into another workspace, which is also just correct: a fork renaming what it calls a data table changes nothing about the data table. Also: revoking a tenant now bounces the replication streams of every workspace holding an entry that resolves here, not only the governing one, so a fork's trigger stops rather than living on inside its open connection; the instance role catalog and the governing workspace's tenant lists are no longer returned to someone who cannot edit them; and the tenant rename dedup collapses non-adjacent duplicates, per role rather than once any role changed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): fail loudly where a role or a pointer can be left half-recorded Three ways the feature could end up in a state nobody could see or undo. Creating a role writes the cluster first and the catalog second, but the catalog write was an `UPDATE` that matched nothing when the instance Postgres settings row was absent — leaving a live login with a password nobody recorded: invisible to the catalog, un-recreatable because the name is taken, and un-deletable because there is no entry to delete. It now errors, so the operation is retryable once the row is restored. Deleting a workspace only nulls the fork lineage; the data table entries pointing at it are left resolving to nothing. Sweeping them is not an option — turning a pointer back into a copy would hand each fork the database outright — so the delete now names the data tables it stranded, and resolving one says which workspace is missing rather than reporting a data table this workspace never had. `InstanceDatatableRole` derived `Debug` while holding a Postgres password; it is now hand-written so `{:?}` on the catalog cannot put a live credential in a log line. Adds the two branches the reviews found unpinned: a caller who is not a member of the governing workspace at all, and `NoIdentity` — the compatibility path for an agent worker that predates this and sends no job id. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): unbreak two operator messages and two comments that described other code The two strings this branch added for states an operator hits once — the catalog write that matched nothing, and the delete that stranded a pointer — were collapsed from their multi-line form with the indentation left in, so both rendered with a fourteen-space gap mid-sentence. `list_datatables` claimed to report a chain it cannot follow and then dropped it; it does drop it, and the comment now says why that is the right place to stay quiet. The non-superadmin check in `edit_datatable_config` was introduced as also covering references, which it does not and need not: `reference` is overwritten from the stored entry for every caller before the check runs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): serialize role catalog mutations, and state each helper's authorization contract The catalog is one JSON document, so create, rename, enable and delete are all read-modify-write. Two concurrent creates read the same snapshot, both succeed in the cluster, and the second write drops the first — leaving a live Postgres login with a password nobody recorded, which is the exact state the delete path exists to prevent. Every mutation now runs in one transaction holding an advisory lock across the read, the cluster DDL and the write, so a lost update cannot happen and a failure rolls the whole thing back. The DDL helpers take that transaction rather than the pool, which is what makes the lock cover them. Their statements moved off `sqlx::raw_sql`: the simple protocol is only needed for genuinely multi-statement SQL, and its future is not `Send`, which an axum handler holding the transaction requires. Each of these is one statement anyway. The new cross-crate surface now says what callers must do. `read_role_catalog` returns plaintext credentials; `create`/`rename`/`set_login`/`drop_instance_role` and `converge_connect_grants` mutate cluster-wide state; `read_datatable_entry` reads a workspace's raw config. All of them are superadmin-gated by their current handlers, but nothing said so at the definition, which is where the next caller looks. Also: the roles table reloads after a failed login toggle instead of leaving it claiming a flip that did not land; the rename affordance is the design-system `Button`, not a raw one; and `resolve_datatable_pg_as_caller` drops a `role` parameter no caller ever filled — browsing resolves as the data table's default until the database manager grows a picker. Why role passwords stay a plain `String` while the instance user's password beside them is a `StringOrSecretRef`, asked three times across reviews: that one is a secret ref because an operator supplies it and may want it from their own backend, while these are minted here and never entered by anyone, so there is nothing for a ref to point at. Encrypting generated secrets at rest is a separate change that would take the replication password with it. Now said at the field. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): give the role catalog its own row, out of reach of the config machinery Putting it inside `custom_instance_pg_databases` was the wrong call, and it cost two ways. The catalog serializes a generated Postgres password per role, and that row is the operator-facing instance config, so the passwords reached `get_instance_config` and its YAML editor — a live cluster credential in a response body, a UI field and any log of either. Worse in the other direction: `to_settings_map` strips the catalog, so a full-row upsert of that key writes the row back without it and the catalog is gone, while the cluster keeps every login it described. `custom_instance_replication_pwd` is the precedent and says exactly why — a generated secret, written only by the server, never operator-authored, hidden so the config machinery cannot read, rewrite or drop it. The catalog is the same thing, so it now has the same shape: `datatable_roles`, in `HIDDEN_SETTINGS`, `PROTECTED_SETTINGS` and the agent-worker denylist. No redaction to keep in step with three code paths, and no way for a neighbouring write to take it out. Two races on the same shared documents. `edit_datatable_config` read the stored data tables outside its transaction and then wrote the whole `datatable` document, so a permissions save committing in between was silently rolled back; it now reads under `FOR UPDATE`. And `set_datatable_permissions` validated role ids against the catalog before opening its transaction, so a deletion in between let it write a deleted role back — including as the default, which every later job then fails on; it now holds the catalog lock and the settings row across validation and write. Completes the authorization contracts the previous commit claimed but did not finish: `read_datatable_entry` (which it named and missed), `resolve_governing_datatable`, whose whole job is to answer for a workspace the caller may not belong to, and `converge_connect_grants_with`, which had not inherited its wrapper's. Also the generic Python SDK reference: `_format_py_params` learned the bare `*` last time, but `extract_py_functions` is a second formatter and still rendered `datatable(name, role)`, so code written from that page passed a keyword-only argument positionally. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): make the concurrency test pin the handlers, and the contracts describe what is enforced The concurrency test reimplemented the read-modify-write inline, so deleting the lock from all three handlers left it green — it pinned Postgres, not the code it was written for. It now drives `create_datatable_role` twice concurrently and asserts the catalog kept both names. Checked the way the last one should have been: removing the lock from the handler makes it fail with "wmtest_a_… is a live cluster login the catalog forgot". The contracts added last commit were stricter than this PR's own callers, which is worse than none — the next reader sees a rule already broken and learns to ignore it. `read_role_catalog` said superadmin-only while two of its four callers are open to any workspace member, and `converge_connect_grants` said superadmin while `set_datatable_permissions` reaches it as a workspace admin. Both were fine on substance: the rule that actually holds is about the credential never reaching a response, log, audit record or export, not about who may call. They now say that. `read_datatable_entry` gets the same treatment rather than the one the earlier message claimed for it: it is the primitive every resolution goes through, so it is deliberately open, and what must not escape is `permissions` — it names the governing workspace's users, groups and folders. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): close the last ways a role or a pointer can be left pointing at nothing The raw settings readers hand back whatever is in the row, so moving the catalog into its own `global_settings` key protected the config machinery and left `GET /settings/global/datatable_roles` and the settings listing returning every live password. Both now filter that one key. The neighbouring `custom_instance_replication_pwd` has the same shape and is not touched here: it predates this and widening the fix to it is a decision about an operator workflow, not a consequence of this change. Three ways a save could leave something resolving to nothing: A permissioned data table could be moved to a PostgreSQL resource. The block was carried across as a server-owned field, the runtime refuses roles on a resource-backed table, so the save succeeded and every job afterwards failed. Refused instead — turning roles off first is one step, and it keeps discarding an access decision something somebody chose. Renaming a governing data table left every fork pointing at the old name: the data table disappears from their pickers and their jobs stop, with nothing in the renaming workspace to suggest why. The rename now follows into the pointers in the same transaction. Deleting one cannot be followed the same way, so it is reported instead — the response names what it stranded, the way deleting a workspace does, and the fork's own error already says which workspace is gone. Also: `ensure_instance_db_grant_options_unchecked` claimed superadmin while the permissions handler reaches it as a workspace admin (the same class fixed last commit, one instance missed); the role entry kept an `instance_config_schema` derive it no longer needs; `write_role_catalog` was the one writer of that table not stamping `updated_at`; and the concurrency test dropped its roles only on success — a failing run is exactly the one that creates them without recording them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * refactor(datatables): put the role catalog in its own table, not in global_settings Five findings across three rounds were all the same choice. A set of live Postgres credentials was living in `global_settings`, which has generic read, list, write, config-export and CLI round-trip paths that know nothing about what they carry: the passwords reached the instance config and its YAML editor, a full-row upsert of a neighbouring key erased the catalog, `GET /settings/global/{key}` and the settings listing returned them raw, and this round the redaction that fixed the last two turned `wmill instance push` into something that wipes every password — a fix breaking the assumption the previous fix made. `POST /settings/global/datatable_roles` could also empty it outside the lock. The approved plan offered a table or `global_settings`, so this is the other option it already allowed rather than a new design. `datatable_role` is a table: no generic settings path can read it, list it, export it, write it or round-trip it, so none of the five needs a guard. The redaction, the hidden/protected/agent-denylist entries and the JSON document all go with it. One row per role also removes the read-modify-write the concurrency work was about: two concurrent creates are two inserts, and the unique index on `name` is what settles a collision. The advisory lock stays for the one window rows do not cover — `CREATE ROLE` is invisible to another transaction until commit, so without it both creates pass their `pg_roles` check. Also from this round: rename mappings are checked against the configuration they claim to describe, since fork pointers are rewritten from them — a caller could otherwise submit `main -> missing` against an unchanged config and repoint every fork of `main` at a name nothing has, and `A -> B` plus `B -> C` moved what pointed at `A` all the way to `C`. And the warning naming forks a delete stranded reached the response but not the screen: both the data table settings save and the workspace delete now show it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): validate a rename against the save it describes, and re-check under the locks Three from the round, all about deciding on state that could already have moved. A permission save resolved the data table and checked it was instance-backed before taking any lock, then wrote under one. A config save committing in between could move the table onto a PostgreSQL resource — recreating exactly what the transition guard refuses — or rename it, in which case the write targeted a key that no longer existed and reported success having changed nothing. It now re-resolves and re-checks on the locked state. Rename validation checked that the source existed before and the target existed after, which still accepts `main -> decoy` against a save that keeps both: every fork of `main` then follows onto a different data table, silently, because it keeps resolving. The rule is now the actual old-to-new key transition — a source may only survive if another rename took its name, and a target may only pre-exist if another rename freed it. That also stops two sources sharing one target, and it admits a swap, which the previous guard refused: `datatables` is keyed by name, so a swap cannot be done one save at a time, and refusing it was a regression against main. The pointer cascade now runs in two passes through a temporary name, the way the migration cascade one layer down already handles the same shape, so `A -> B` with `B -> C` moves each pointer once from what it named before the save. The tenant mutators say what they are for: they write an access decision for any workspace named, with an arbitrary mutation, and exist for the transaction that frees or renames a principal. Editing a decision on purpose belongs in the permissions endpoint. Carried in the same change: the stranded-fork list is a field rather than a phrase to grep out of a success string; the pointer cascade matches with `EXISTS` instead of a `LIKE` over the whole document, so a workspace whose pointers name something else is not rewritten to a byte-identical value under an exclusive lock; and `InstanceDatatableRole` drops the serde derives left over from the JSON document, one of which would emit `pwd`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): cascade on the leave route that is used, gate migrations before the admin connection, and drop a role atomically The tenant cascade on leaving went onto `/users/leave`. The UI and the generated client call `/workspaces/leave` — a different handler in a different crate with the same name — which deleted the membership and left `u/` in the tenant lists. Leaving and rejoining therefore restored the access the leave was supposed to end, and a later account taking the username would have inherited it. The regression test drives the route the client actually calls; without the fix it fails with "leaving kept the tenant". The migration endpoints authorized too late. `run_datatable_migrations` opened the data table's admin connection, created `_wm_migrations` and read it before reaching the per-migration role check — so with nothing pending, nothing was checked at all. Rollback returned before its check when nothing was applied, and the status endpoint had none. All three now ask, before any connection is opened, whether the caller can reach the data table as any role at all; which role a given migration runs as is still decided per migration, and by the executor after that. Deleting a role committed the cluster drop and the catalog row, then swept the tenant lists in separate transactions. A sweep failing part-way left workspaces naming a role nothing can connect as, while the retry answered `NotFound` because the catalog entry was already gone. The sweep now runs in the same transaction, so the drop, the row and every tenant list commit together. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): refuse to copy a data table that is under roles pg_dump carries no roles and the import runs with --no-privileges, so a copied data table arrives owned by the admin connection with no GRANT for any role. The settings clone brings `permissions` across, so the fork's tenants pass Windmill's check, connect as the role they were given, and are denied by Postgres on everything: an entry that reads as configured and answers nothing. Refuse the copy — in the import endpoint before any data moves, and in the fork path the CLI takes. Replaying the source's owners and ACLs into the clone is what lifts this, and is a change of its own. Dropping `permissions` from the copy instead would be the unsafe half, since the copy holds the parent's rows. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): refuse the clone's database too, not only its data A clone is two endpoints: `create_pg_database` then `import_pg_database`. Only the second refused a data table under roles, so a fork asking to clone one created and registered an empty `wm_fork_…` instance database and then failed — and nothing collects it, since `drop_forked_datatable_databases` only drops entries carrying `forked_from` and no entry names this one. Refuse in both, so the clone stops before a database exists. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * nit worker error msg * fix pg_dump stuck on version 17 on nix * fix(datatables): refuse a malformed role annotation instead of ignoring it `-- Role operator`, `-- role operator;` and `-- role operator -- why` all failed the annotation parser's exact-match rule, so the query fell through to the data table's default role and ran, silently, under a login the author did not choose. Naming a role exists precisely to not do that. A leading comment whose first word is `role` is now an annotation attempt: the keyword matches case-insensitively, one trailing `;` is tolerated, and anything else is an error naming the line. Only callers that already know the target is a `datatable://` reference ever run this, so ordinary SQL keeps its comments. Also bumps the dev shell's postgres client to 18 — it trailed the server the dev database runs, which takes out every data table export, clone and fork-with-data. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): refuse a malformed role query string instead of ignoring it `?Role=analytics`, `?role=` and `?x=1&role=…` all fell through the reference parser's exact-match rule, so the connection resolved to the data table's default role and ran under a login the caller never asked for — the URI half of the same trap as a malformed `-- role` annotation. The key now matches case-insensitively, and anything else in the query string is an error naming it; `role` is the only parameter a reference takes. Callers that only need the entry keep a lenient `datatable_ref_name`, since they never act on the role. The DuckDB `ATTACH` parser propagates it rather than attaching under the default. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR * fix(datatables): carry the role annotation into the row_to_json retry The retry rebuilds its SQL from `pruneComments(code)`, so the leading comment block never reached the second attempt — and with it the `-- role ` line that decides which login the query runs as. The retry connected as the data table's default role instead, so a query the first attempt was denied could succeed on the second, reported as "recovered with the row_to_json fix". Carry the leading comment block over. The retry itself is unchanged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * chore(datatables): don't mount the roles UI until the ACL editor lands Enforcement ships first. The permissions drawer is what turns roles on, and the catalog section is what creates them — both are only useful once there is a way to grant a role the privileges it needs, which arrives with the ACL editor. Left mounted they would offer a feature whose other half does not exist. The two components are complete and reviewed; only their call sites here are commented out, with a note pointing the follow-up PRs at them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): honour `-- role: x`, and fix the DuckDB attach test Two review findings, both real. `attach_datatable_parses_name_and_role` never compiled: `parse_attach_datatable` returns `Result>` now and one call site kept a single `unwrap`. Its `?Role=analytics` case also asserted a refusal, contradicting the parser in the same commit, which matches the key case-insensitively. Replaced with the cases that are genuinely malformed, and a positive one for the cased key. `-- role: analytics` fell through to the default role — the silent fallback the strict parser exists to remove, for the spelling most likely to be typed. The keyword now accepts an optional colon, attached or spaced, while a word that merely starts with it (`rolebased`) is still not an attempt. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): clone a fork's pointer instead of failing after the copy Forking a fork with cloning left an orphan database. The preflight resolves the pointer and sees the governing entry, so both endpoints ran and filled the new database; `apply_forked_datatable` then refused the inherited pointer and rolled the fork back, stranding a registered `wm_fork_*` that no entry names and whose name blocks the retry. Refusing earlier would have been the smaller change, but forking a fork and cloning worked before pointers existed, so it would trade an orphan for a regression. Resolve what the pointer names and write the terminal entry the clone needs: the whole `database` object rather than a patch of its `resource_path`, since a pointer has none, and `reference` removed with it. Also accepts `-- role=x` and `-- Role = x`, two more spellings that fell through to the default role. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): refuse to roll back the catalog while roles exist The down migration dropped the table and left every role behind: live Postgres logins whose passwords only that table carried, so after a revert Windmill could neither use, disable nor delete them, and re-applying could not recreate them because the names were taken. Cleaning up here is not possible either — dropping a role means reassigning what it owns in every instance database, and a migration runs in one — so it now refuses while the catalog is non-empty and says to delete the roles through instance settings, which does the cluster work. Also enforces the instance-only invariant the resolved-pointer clone relies on rather than only asserting it in a comment. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * refactor(datatables): settle clonability in one place, before anything is created A clone is three stages a workspace apart — `create_pg_database`, then `import_pg_database`, then `apply_forked_datatable` inside the fork transaction. Only the third can roll back, and `CREATE DATABASE` is not transactional, so any refusal that lives there strands a registered `wm_fork_*` that no entry names and whose name blocks the retry. That orphan has now been fixed three times, most recently reintroduced by a guard added one commit ago. Patching each new refusal into the first endpoint is not the fix; having two places that can refuse is. `ensure_datatable_is_clonable` now answers every reason a copy can be refused and returns what it resolved, and the stage that writes the entry only does the work. Also takes an ACCESS EXCLUSIVE lock before the rollback guard counts, so a role created concurrently cannot slip between the check and the drop. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): let a retried clone reclaim its own leftover database A clone creates its target database one request before it copies into it, and the fork that would name it is written a request after that. Any failure in between — a pg_dump error, a bad restore, a dropped connection, the source's roles changing mid-flow — left a registered `wm_fork_*` that no entry names, and every retry then failed on its name. This predates data table roles. `create_pg_database` now reclaims such a leftover before creating: only a `wm_fork_*` database Windmill registered as a data table database and that no data table or ducklake entry names, in any workspace, archived ones included. The drop never terminates connections, so a clone still copying into it makes the reclaim fail instead of being cut off. It is limited to callers who administer the source — reaching it is not enough, since on a data table without roles every member reaches it — and anyone else gets the refusal an existing database always got. Co-Authored-By: Claude Opus 5 (1M context) * Revert "fix(datatables): let a retried clone reclaim its own leftover database" This reverts commit 7dd3275a10. The reclaim tied the caller to the source they administer, but not to the database it dropped. Between another workspace's import and its final fork request, that workspace's target is full, registered, unnamed and has no open connection, so an admin of any instance data table could name it and have it dropped and recreated empty. The victim's fork would then commit pointing at the empty copy. Safe reclaim needs durable clone ownership and serialization with the request that names the database; until then the leftover stays, as it did before this PR. Co-Authored-By: Claude Opus 5 (1M context) * docs(datatables): record the stale clone database as a known limitation A clone is three requests and `CREATE DATABASE` is not transactional, so a failure after the first leaves a registered `wm_fork_*` behind, as it did before data table roles. Accepted for this PR: it is harmless to data and goes away once the clone is a single server-side operation. The comment also records why the obvious fix is wrong: reclaiming the leftover on retry, without durable clone ownership, can drop another workspace's fully copied database between its import and its final fork request. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): bounce the streams reading a data table when it is deleted Deleting a governing data table, or the workspace that holds it, only collected the fork pointers it stranded, for the warning. A Postgres trigger or capture already streaming through one of those pointers kept the replication connection it opened while the pointer still resolved, so it went on dispatching the governing database's rows after the fork lost access — until its connection happened to restart. The governing workspace's own streams on a deleted entry did the same. Both deletion paths now bounce the affected listeners inside their own transaction, through the helper a permission change already uses, so a listener that reconnects re-resolves the entry and finds it gone. The helper is split so a caller can pass the (workspace, local name) pairs it already holds. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): keep the fork schema baseline, and bounce streams on every removal Three fixes from review. `edit_datatable_config` took `forked_from` wholesale from the stored entry, so the fork schema diff's save of an advanced baseline was silently discarded and an applied change was offered again. Whether an entry carries a clone stamp is still carried from the store, since that is what marks its database droppable, but the baseline inside it is now taken from the request. The stranded-pointer warning and the stream bounce ran over the optional `deleted_datatables` hint, which the settings-sync CLI never sends, so removing a governing data table through `wmill` bounced nothing. Removals are now derived from the stored configuration against the saved one. `delete_workspace` read the pointers to bounce before its transaction, so a fork committing a pointer during the deletion was missed. The read now happens inside the transaction, after the workspace row is deleted: a fork's insert key-share locks that row through its parent foreign key, so it is either seen or fails on the missing parent. Co-Authored-By: Claude Opus 5 (1M context) * refactor(datatables): keep Postgres triggers and data table roles apart A replication stream reads every row of every table whatever the data table's roles grant, and its listener checks access only when it connects. Rather than chase every way access can change and bounce the streams each one affects, a data table now carries one or the other: - a Postgres trigger or capture cannot be created on, or connect to, a data table under roles; - roles cannot be turned on while an enabled trigger or a live capture reads the data table, its own or a fork's through its pointer. The refusal names each one to disable. This removes the stream bounces on roles edits and on data table and workspace deletion, and the trigger gate that admitted admins. The fork schema baseline fix from the same review round is kept. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): refuse a Postgres trigger on a data table under roles when it is saved Creating or editing a trigger that points at a data table under roles was accepted, and its listener then retried the refused connection every 30 seconds forever. The save is now refused, and a trigger that reaches such a data table anyway (re-enabled, or cloned into a fork) is disabled by its listener with the reason, as a missing replication slot is. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): disable a data table role before deleting it Deleting a role reassigns and drops what it owns in each registered database on its own connection, and each of those passes commits as it goes. A database failing part-way left the role enabled in the catalog and able to log in, but already stripped in the databases reached before it. The role is now disabled in its own commit first, so a failed delete leaves a disabled role to retry. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): serialize roles going on with a stream starting Turning roles on looked for enabled triggers and live captures once, without a lock anything starting a stream also took. A trigger enabled in that window could have its listener connect before roles committed, and a healthy listener never checks again. Both transitions now serialize on one advisory lock: roles going on hold it exclusive while they look, and trigger create, edit and enable, and capture setup and ping hold it shared while they commit. Either the look sees the stream, or the listener connects after roles are committed and refuses. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): wait out live listeners, and resolve stored names containing `?` Turning roles on counted a trigger as gone once disabled, and a capture once its client stopped pinging, but the listener keeps its replication connection until its next heartbeat notices. A trigger or capture whose listener pinged in the last 15 seconds, the window a server holds a listener for, now still counts as streaming. Data table names could contain `?` before they were restricted, and such entries are still stored. Splitting `?role=` off a reference misread them: `a?b` became `a` with an unknown parameter, and the clone checks looked at a different entry than the one copied. An entry stored under the whole reference is now looked up first, in the Postgres executor, DuckDB ATTACH and the clone checks. Agent workers cannot read the workspace and keep the strict parse, which refuses such a name rather than misreading it. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): warn when a settings sync strands fork pointers A settings save reported the fork pointers left resolving to nothing only for the names in `deleted_datatables`, which `wmill sync push` never sends. The save now works out what it removed from the locked entries, and the CLI prints the stranded pointers it returns. Also correct the replication helper's contract: no role or admin check makes a replication connection safe, so a data table under roles is refused outright rather than gated as an admin operation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): refuse a save that drops a data table's roles through an undeclared rename A data table's roles follow its entry only through a declared rename. A settings sync sends the whole map and never declares one, so renaming a data table under roles there read as a delete and a new entry on the same database: the new entry carried no roles, and every caller connected as admin. Such a save is now refused, naming both entries. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * fix(datatables): no entry without roles may newly reach a database under roles The previous guard only caught a new name replacing an entry under roles. A whole-map save could also repoint an existing entry without roles at that database, or another workspace could point one there, and every caller of that entry would connect as admin. The rule is now stated on the saved entries: one that carries no roles and newly points at an instance database any entry under roles uses, in this workspace or another, is refused. A declared rename carries its roles and passes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * feat(datatables): move data table role catalog and resolution to the enterprise edition Roles are an Enterprise Edition feature. The catalog, the Postgres logins, CONNECT convergence, tenant evaluation and the role half of connection resolution move to windmill-ee-private. Every public function keeps its path and signature and forwards through datatable_roles_oss, which re-exports the enterprise implementation or, without it, refuses. Without the enterprise edition a data table under roles, or a caller naming a role, is refused a connection rather than resolved as admin, and the reach and admin-access checks refuse one under roles. A data table not under roles resolves as before in every edition, and an instance database keeps the CONNECT grants it was created with. The catalog lock, the stream lock, the tenant cascades and the permissions stripping stay in OSS: they only restrict. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * feat(datatables): move the data table permissions endpoints to the enterprise edition The permissions read, save and usable-roles handlers move to windmill-ee-private; the routes stay registered and, without the enterprise edition, answer that data table roles are an Enterprise Edition feature. ensure_governs_datatable and ensure_reaches_datatable keep their paths: the first refuses, the second passes a data table not under roles. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * feat(datatables): move the data table role catalog endpoints to the enterprise edition The superadmin list, create, update and delete handlers move to windmill-ee-private. The routes stay registered and, without the enterprise edition, refuse after authentication. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * test(datatables): run the roles tests on the enterprise edition, refusals without it Each test that exercises roles runs with private and enterprise. Two tests run without them: every roles route answers the Enterprise refusal, and a data table saved under roles, or a named role, is refused a connection while one not under roles resolves as before. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * feat(datatables): gate the roles UI mount sites on an enterprise license Both mount sites are still commented out; the gate travels with them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * test(datatables): run the tenant matcher test on the enterprise edition The matcher it covers is enterprise code now, so without the enterprise edition the test hit the stub and failed the default windmill-common run. It runs with private and enterprise, and a counterpart without them asserts that no tenant list covers anyone, the wildcard and a workspace admin included. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb * chore: update ee-repo-ref to a1873dbb67f2302b85ff5362f8387b48eccdb607 This commit updates the EE repository reference after PR #783 was merged in windmill-ee-private. Previous ee-repo-ref: 5c853e2c20eca6b748415fc0d6862a6ebfb5fec4 New ee-repo-ref: a1873dbb67f2302b85ff5362f8387b48eccdb607 Automated by sync-ee-ref workflow. * fix(datatables): refuse roles while a same-workspace alias reaches the database Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): let CE migrations connect as an explicitly named admin Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): serialize roles going on with aliases saved from other workspaces Co-Authored-By: Claude Opus 5 (1M context) * docs(datatables): note that legacy names with ? cannot be migrated Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): add an ACL editor for data table roles Co-Authored-By: Claude Opus 5 (1M context) * fix: take every pooled connection before the ACL apply locks Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: refresh grant options only after the ACL apply validates its plan Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: add only missing grant options before an ACL apply, never default privileges Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: run one data table ACL apply at a time per server before it connects Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: hold the ACL connection to the database that was authorized Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: build the ACL connection from the authorized data table entry Resolving the settings again could land on a resource with the same database name on another server, which the later entry checks never see. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: check ACL read reach against the entry it connects from Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * feat(datatables): Ducklake catalogs on the external instance cluster Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): never grant CREATEROLE to custom_instance_user on the external cluster Co-Authored-By: Claude Opus 5 (1M context) * docs(datatables): state the authorization contract of external database usage lookups Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): drop a DuckDB data table secret once its ATTACH has used it Co-Authored-By: Claude Opus 5 (1M context) * perf(datatables): resolve a workspace's data tables per pointer hop, not per entry Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): hold the parent's settings while a fork points at its data tables Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): refuse repointing the external cluster while it is in use, and keep verify-ca working for pg_dump Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): protect external databases pending fork cleanup, and describe Ducklake usage in the API Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): per-cluster data table role catalogs, with roles on the external instance cluster Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): write the external cluster setting under the lifecycle lock, and check fork targets are registered Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): register external fork catalogs under the lifecycle lock, and keep certificate verification in DuckDB attaches Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): keep certificate verification when DuckDB attaches an external data table Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): keep certificate verification when DuckDB attaches an external data table Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): refuse fork cleanup of an external database another workspace uses Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): refuse rolling back while external data tables are under roles, and type external_instance in the CLI Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): stop counting storage-only fork cleanup rows as uses of an external database Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): bind fork database copies to their workspace, and count every use before dropping one Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): authenticate instance database setup before writing its status, and keep a fork reservation across it Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): create external databases only on a cluster setup succeeded on, and document the registry reader Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): keep only the most recently used DuckDB root certificate files Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): migrate fork reservations on workspace rename, and lock the parent's data tables for the whole fork Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): take the fork data table lock once, before the external cluster's Co-Authored-By: Claude Opus 5 (1M context) * docs(datatables): describe the external instance cluster and how to run one locally Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): keep fork reservations private, drop a cleaned-up entry with its database, and serialize cleanup with settings saves Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): hold the fork lock across a fork import, and carry the reservation inside the setup write Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): check the external cluster setting on its own transaction, and gate the registry probe Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): add an ACL editor for data table roles Co-Authored-By: Claude Opus 5 (1M context) * fix: take every pooled connection before the ACL apply locks Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: refresh grant options only after the ACL apply validates its plan Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: add only missing grant options before an ACL apply, never default privileges Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: run one data table ACL apply at a time per server before it connects Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: hold the ACL connection to the database that was authorized Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: build the ACL connection from the authorized data table entry Resolving the settings again could land on a resource with the same database name on another server, which the later entry checks never see. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * fix: check ACL read reach against the entry it connects from Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BRoYE5ZeAVvrDYdfhDAYXb * chore: update ee-repo-ref to 7e338e4dabf91689bfd7fb0333c6534040b17b59 This commit updates the EE repository reference after PR #787 was merged in windmill-ee-private. Previous ee-repo-ref: 0edd40979cf36bfba59323f3f6a0811ae1369cf5 New ee-repo-ref: 7e338e4dabf91689bfd7fb0333c6534040b17b59 Automated by sync-ee-ref workflow. * fix(datatables): keep DuckDB root certificate files in the job directory Co-Authored-By: Claude Opus 5 (1M context) * chore: drop the unused json import from the settings crate Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): drop the serde_json::json import left unused by the fork setup write Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): accept external_instance data tables in the settings form type Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): serialize fork database reservations, cleanup, setup and Ducklake saves on the database - A fork copy is created and registered under its workspace's fork lock, and refused once the workspace is archived; a rename re-migrates reservations under that lock after archiving. - Ducklake saves lock every instance database they newly name, as data table saves do. - Instance database setup holds the database's lock until its entry is written. - Fork import also holds the database's lock across the restore. - Cleanup re-reads the entry under its locks before dropping anything. - Non-superadmins no longer see fork copies reserved for workspaces they are not in. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): state the authorization contract of the external cluster status and write check Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): lock the fork copy at finalization, and order cleanup's registry write after its settings row - Fork finalization holds the copy's database lock from its availability check to the commit. - Cleanup removes the registry entry in its own transaction, in a task of its own, instead of on a second connection; a rename migrates reservations after its settings rewrites, so both take the settings rows before the registry. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): hold the fork lock across a rename's settings copy Fork cleanup of the old id could otherwise drop a copy the renamed workspace goes on using. Also states the authorization contract of the instance database drop helpers. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): refuse moving the external cluster to another host or port while it holds databases Every settings writer goes through the same check as removal: the login, TLS and maintenance database may still change, the cluster may not. Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): clone a data table under roles with its owners and grants (#11120) Claude-Session: https://claude.ai/code/session_01UbrtwiYNfayrmqouBJHwGV Co-authored-by: Claude Opus 5 (1M context) * fix: open the raw app data table drawer when the workspace has none Selecting the first data table of an empty list passed undefined to the name check, which threw instead of opening the drawer on no data table. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): offer cloning a data table under roles where its grants can be replayed The server clones such a data table and replays the source's owners and grants, which only the Enterprise Edition does, so the fork wizard hid both clone options everywhere instead of on a build that cannot replay them. Co-Authored-By: Claude Opus 5 (1M context) * docs: name the placeholder the empty raw app data drawer renders Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): replace a job directory file at the DuckDB root certificate path instead of trusting it Co-Authored-By: Claude Opus 5 (1M context) * test: pin the enterprise refusal the role pickers read as 'not under roles' The server's sentence and the frontend's copy of it were coupled by nothing, so rewording either one turned every role picker on a community build into a failed lookup. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): type Ducklake catalogs on the external instance cluster in the settings form Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): read a roles answer only for the workspace it was asked in A fork and its parent each have their own roles on a data table of the same name, so an answer stamped with the name alone settled the role from the workspace the editor was acting on before. Also derive the AI table creation flag from the data replaced into the editor: data naming no data table left the flag on from before. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): keep an instance database a settings save is waiting to name Cleanup for a database whose setup failed took the lock first, read no user, and dropped it while a save blocked on that same lock was about to commit a reference to it. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): check the workspace stamp in the default database selector too Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): read what a save racing instance-database cleanup committed A transaction blocked on the lock may still roll back, so keeping the database for it stranded one whose name then blocks every retry: it is let through and its outcome read instead. The waiter query also matches this database's locks only, since pg_locks spans the cluster. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): tell a waiting request apart from the workspaces using a database Both callers render what cleanup returns as the workspaces that keep the database, so a waiting request's pid read as one of them. Each now words that case itself, and the give-up comment names where the kept name actually goes. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): take the cleanup lock on a connection the pool cannot reclaim A session lock outlives the future holding it, so a cancellation between taking it and releasing it handed a locked session back to the pool, where every later settings save waits on it. Detached, the connection closes when it is dropped and the server releases the lock with the session. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): close the cleanup connection on drop instead of detaching it Detaching released the pool permit while the session stayed alive, so concurrent cleanups waiting on their locks could open as many connections as they liked. Closing on drop covers the same cancellation and keeps them counted. Co-Authored-By: Claude Opus 5 (1M context) * chore: update ee-repo-ref to 179d2454a063ee818a9387a1eafdd354a217a16c This commit updates the EE repository reference after PR #802 was merged in windmill-ee-private. Previous ee-repo-ref: c2e43f5b5ff753d339b70e232fd17b6ffecb054d New ee-repo-ref: 179d2454a063ee818a9387a1eafdd354a217a16c Automated by sync-ee-ref workflow. * chore: update ee-repo-ref to fd5b8af748f2c985b13e18d9ea30894f3bd7e9a3 This commit updates the EE repository reference after PR #798 was merged in windmill-ee-private. Previous ee-repo-ref: 3145e422d61d580f0a82804f075285c112879da0 New ee-repo-ref: fd5b8af748f2c985b13e18d9ea30894f3bd7e9a3 Automated by sync-ee-ref workflow. * fix(datatables): classify a fork import's target from the resolution that built its connection A second read could see the entry flipped to a resource and skip the reservation check while the connection already built still reached the instance cluster. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): run external database lifecycle on the caller's transaction, one lock order for fork cleanup Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): check a fork copy's reservation on the locked transaction's connection Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): make and drop fork copies of external data tables on the external cluster Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): refuse a fork whose external copy something already names Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): check an external fork copy's uses before removing its entry, so child fork pointers still count Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): migrate external cluster fork reservations on workspace rename Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): configure the external instance cluster and pick its databases from the UI Instance settings gets an External Postgres tab: the cluster's connection, the setup run with its report, and the databases Windmill created there. Data table and Ducklake settings offer the external_instance kind, with a picker over those databases. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): keep the external cluster form out of the instance settings the page bulk-saves The form seeded its key into the settings store after the page snapshotted them, so merely opening the tab made the page send a setting the validator refuses, failing an admin's unrelated save. The form is local state now, and only the setup writes the key. The database picker also treats a superadmin-only listing as authoritative: a workspace admin, who cannot list, no longer sees a saved database as missing. Co-Authored-By: Claude Opus 5 (1M context) * fix(tests): app paths are validated, so a guest app cannot be renamed to one with a space Path validation on apps landed after this test, which still expected a space to be accepted and created its fixture on a ':' path. The scopable-path guard it exists for is kept by planting that path on the row, which is now the only way an app can hold one. Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): one Managed Postgres tab, and a switch for Windmill's own database The instance settings tab covers both substrates Windmill administers: the external cluster, which can now be disabled once nothing sits on it, and Windmill's own database, which an operator can turn off so the cluster is the only one a workspace may newly name. A save that names it then refuses, as the create endpoint does. Workspace settings name them the way an admin meets them: Postgres Resource, and Managed instance, qualified as Internal or External only while both are on offer. Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): offer the external cluster in the add-data-table wizard, and give Ducklake its room back The wizard gets the external cluster as a fourth substrate: pick a database it already holds or name a new one, which the run creates before writing the entry, so Try again does not trip on a database the last attempt made. Ducklake's maintenance column moves into the settings popover next to the extra args, which the wider catalog select had squeezed the name box out of. A setting a component saves itself now also moves the page's baseline, or Discard would restore what it replaced and a later save would send that stale value back. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): only a database this wizard run made makes its retry idempotent A retry skipped the create whenever the name was registered, which let a run adopt a database another superadmin had made and share their data without the warning the existing-database branch shows. The run tracks what it created instead, and reports it so Discard can say the database is still there. Creating one now refreshes the registry the next run's default name and its validation read, and that default counts the external cluster's databases rather than the instance's. Row ownership carries the substrate: "instance" and "external_instance" can hold the same name, and a row repointed between them while a run probes must not read as that run's own. Also fixes the merge leftovers in the DuckDB executor's tests, which cargo check never compiles: the new PgDatabase field and the attach helper's job directory. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): shorten the picked managed kind, and say what an empty database list means The closed select has room for the qualifier, not the whole name, so a picked managed kind reads "Managed (Internal)". An external entry gets the tooltip its internal counterpart has, and a database picker with nothing in it says to type a name rather than "No items found". Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): keep the wizard's external-database bookkeeping optional and parked The run deps gained a required field, which every existing caller of runSetup -- the sibling test suite included -- does not pass. It defaults to empty instead, the parked payload carries it across a Supabase redirect as it claims to, and a resumed run counts it as something left behind. Two regressions pin the create: made on a first attempt, skipped only for a database this run made. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): review an external-cluster database as one, not as a Postgres resource The review step fell through to the resource branch for the new provider, so it announced a connection already in the workspace for a database that does not exist yet. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): show the server's refusal instead of an object in the toast Saving data table settings passed the whole error to the toast, which rendered the request and response as JSON, and the instance-database wizard replaced it with "check console". Both surface the message the API sent, through the helper that exists for it. Co-Authored-By: Claude Opus 5 (1M context) * chore: point at the EE commit dropping the unused import Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): define a role on the cluster its data table sits on The permissions drawer created and listed roles without naming a cluster, so a role defined from an external data table became a login on Windmill's own, and the refresh then replaced the correct catalog with the internal one. The drawer passes the data table's cluster to both. The wizard also read a database it had just created as a name collision, which blocked returning to the failed attempt and finishing under the same name. And the merge had left the CONNECT-grant pass and its log in both the caller and the callee. Co-Authored-By: Claude Opus 5 (1M context) * feat(datatables): manage the external cluster's role catalog, and name the cluster in the copy The roles drawer offered one catalog, so the external cluster's roles could only be reached through a data table sitting on it — and became unreachable once the last one was gone, while the cluster could not be unset until they were dropped. Opened from the page it now offers the clusters the instance has; a data table's own drawer still pins its cluster, and its copy names that cluster rather than "the instance". A DuckDB attach also decides whether to verify certificates the way every other Postgres connection does, so a resource carrying a root certificate is no longer downgraded to require here alone. Co-Authored-By: Claude Opus 5 (1M context) * fix(datatables): bind the roles list to the cluster it was read for Switching catalogs left the previous cluster's rows on screen and applied whichever response landed last, so a slower read could seat one cluster's logins under the other's heading while every control acted on the wrong id — invisible where a name exists on both. The switch clears the rows, a token discards a response the selection has moved past, and the controls stay inert while a catalog is being read. The drop confirmation also names the cluster whose databases it reaches. Co-Authored-By: Claude Opus 5 (1M context) * chore: update ee-repo-ref to dfe7b8b9204d21e0264fbea1c6f6eedf9e738d56 This commit updates the EE repository reference after PR #810 was merged in windmill-ee-private. Previous ee-repo-ref: 30db1b33bda0446f5fc5dfe353fbb226d57a26d4 New ee-repo-ref: dfe7b8b9204d21e0264fbea1c6f6eedf9e738d56 Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: windmill-internal-app[bot] Co-authored-by: Ruben Fiszel --- AGENTS.md | 4 + ...d368347c59f36fd87df6dc7996152ccb84af0.json | 38 -- ...9ad2db940c2455ab19cc95e5198baf96d5629.json | 29 - ...df2a63695f8cbf8af648da5a0a77a5b9d02ba.json | 17 - ...12373638be391f884f81dc387ffc465badac6.json | 20 - ...8dc45020a2a553d8874c49f9eafedea5a9d40.json | 24 - backend/Cargo.lock | 1 + backend/Cargo.toml | 1 + backend/ee-repo-ref.txt | 2 +- ...0917133824_datatable_role_cluster.down.sql | 22 + ...260917133824_datatable_role_cluster.up.sql | 8 + backend/tests/instance_config.rs | 46 ++ .../tests/datatable_roles.rs | 12 +- backend/windmill-api-settings/src/lib.rs | 249 +++++++- .../src/datatable_acl.rs | 78 ++- .../src/datatable_clone.rs | 100 +++- .../src/datatable_migrations.rs | 6 +- .../windmill-api-workspaces/src/workspaces.rs | 533 ++++++++++++++---- .../src/workspaces_extra.rs | 184 +++++- backend/windmill-api/openapi.yaml | 177 +++++- backend/windmill-common/Cargo.toml | 1 + .../windmill-common/src/datatable_roles.rs | 171 ++++-- .../src/datatable_roles_oss.rs | 64 ++- .../src/external_instance_pg.rs | 456 +++++++++++++++ .../src/external_instance_pg_oss.rs | 82 +++ .../windmill-common/src/global_settings.rs | 8 + .../windmill-common/src/instance_config.rs | 103 ++-- backend/windmill-common/src/lib.rs | 153 ++++- backend/windmill-common/src/workspaces.rs | 197 ++++++- .../windmill-worker/src/duckdb_executor.rs | 285 ++++++++-- cli/src/commands/datatable/datatable.ts | 6 +- docs/external-instance-datatables.md | 88 +++ .../src/lib/components/InstanceSetting.svelte | 22 +- .../lib/components/InstanceSettings.svelte | 13 + .../src/lib/components/instanceSettings.ts | 31 + .../ExternalInstancePgSettings.svelte | 471 ++++++++++++++++ .../InstancePgSettings.svelte | 65 +++ .../AddDataTableWizard.svelte | 153 ++++- .../CustomInstanceDbWizardModal.svelte | 5 +- .../DataTablePermissionsButton.svelte | 20 +- .../DataTableRolesSection.svelte | 71 ++- .../DataTableSettings.svelte | 137 ++++- .../workspaceSettings/DucklakeSettings.svelte | 399 +++++++------ .../ExternalInstanceDbSelect.svelte | 117 ++++ .../InstanceRolesButton.svelte | 8 +- .../addDataTableModel.test.ts | 48 +- .../workspaceSettings/addDataTableModel.ts | 99 +++- .../workspaceSettings/datatableTelemetry.ts | 2 +- .../workspaceSettings/utils.svelte.ts | 24 + .../workspaceSettings/wizardParking.ts | 3 + 50 files changed, 4121 insertions(+), 732 deletions(-) delete mode 100644 backend/.sqlx/query-71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0.json delete mode 100644 backend/.sqlx/query-79799b5a2e499df6c28e286c42b9ad2db940c2455ab19cc95e5198baf96d5629.json delete mode 100644 backend/.sqlx/query-86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba.json delete mode 100644 backend/.sqlx/query-b9842d2d8abf382bd82d8fa1de012373638be391f884f81dc387ffc465badac6.json delete mode 100644 backend/.sqlx/query-d48ca62c86b1af7a9dd2450c1c28dc45020a2a553d8874c49f9eafedea5a9d40.json create mode 100644 backend/migrations/20260917133824_datatable_role_cluster.down.sql create mode 100644 backend/migrations/20260917133824_datatable_role_cluster.up.sql create mode 100644 backend/windmill-common/src/external_instance_pg.rs create mode 100644 backend/windmill-common/src/external_instance_pg_oss.rs create mode 100644 docs/external-instance-datatables.md create mode 100644 frontend/src/lib/components/instanceSettings/ExternalInstancePgSettings.svelte create mode 100644 frontend/src/lib/components/instanceSettings/InstancePgSettings.svelte create mode 100644 frontend/src/lib/components/workspaceSettings/ExternalInstanceDbSelect.svelte diff --git a/AGENTS.md b/AGENTS.md index 858e9329ff..850538f080 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,6 +29,10 @@ Open-source platform for internal tools, workflows, API integrations, background - **Agent workers**: `docs/agent-worker-e2e.md` — building and running one locally. An agent reaches the DB only through the API, so `Connection::Http` paths are never taken by a plain `cargo run`; a normal build cannot start one at all. +- **External instance data tables**: `docs/external-instance-datatables.md` — the cluster Windmill + administers behind `external_instance` data tables and Ducklake catalogs: its invariants (one + lifecycle lock, managed-object markers, the setup gate, per-cluster roles, fork copy ownership) + and how to run one locally - **Enterprise**: `docs/enterprise.md` — EE file conventions and PR workflow - **Operator write rights**: `docs/operator-write-rights.md` — which `operator_settings` flags are enforced rather than cosmetic, and why a right that is granted-unless-withdrawn needs `Option` diff --git a/backend/.sqlx/query-71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0.json b/backend/.sqlx/query-71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0.json deleted file mode 100644 index a03fab7081..0000000000 --- a/backend/.sqlx/query-71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0.json +++ /dev/null @@ -1,38 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT id, name, enabled, pwd FROM datatable_role", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "id", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "name", - "type_info": "Varchar" - }, - { - "ordinal": 2, - "name": "enabled", - "type_info": "Bool" - }, - { - "ordinal": 3, - "name": "pwd", - "type_info": "Text" - } - ], - "parameters": { - "Left": [] - }, - "nullable": [ - false, - false, - false, - true - ] - }, - "hash": "71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0" -} diff --git a/backend/.sqlx/query-79799b5a2e499df6c28e286c42b9ad2db940c2455ab19cc95e5198baf96d5629.json b/backend/.sqlx/query-79799b5a2e499df6c28e286c42b9ad2db940c2455ab19cc95e5198baf96d5629.json deleted file mode 100644 index c36b6fb361..0000000000 --- a/backend/.sqlx/query-79799b5a2e499df6c28e286c42b9ad2db940c2455ab19cc95e5198baf96d5629.json +++ /dev/null @@ -1,29 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"datatable!\"\n FROM workspace_settings ws\n JOIN workspace w ON w.id = ws.workspace_id AND w.deleted = false\n CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt\n WHERE ws.workspace_id <> $1\n AND dt.value->'database'->>'resource_type' = 'instance'\n AND dt.value->'database'->>'resource_path' = $2\n ORDER BY ws.workspace_id, dt.key\n ", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "workspace_id!", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "datatable!", - "type_info": "Text" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - false, - null - ] - }, - "hash": "79799b5a2e499df6c28e286c42b9ad2db940c2455ab19cc95e5198baf96d5629" -} diff --git a/backend/.sqlx/query-86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba.json b/backend/.sqlx/query-86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba.json deleted file mode 100644 index 7627d9828d..0000000000 --- a/backend/.sqlx/query-86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba.json +++ /dev/null @@ -1,17 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "INSERT INTO datatable_role (id, name, enabled, pwd) VALUES ($1, $2, $3, $4)", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Varchar", - "Varchar", - "Bool", - "Text" - ] - }, - "nullable": [] - }, - "hash": "86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba" -} diff --git a/backend/.sqlx/query-b9842d2d8abf382bd82d8fa1de012373638be391f884f81dc387ffc465badac6.json b/backend/.sqlx/query-b9842d2d8abf382bd82d8fa1de012373638be391f884f81dc387ffc465badac6.json deleted file mode 100644 index 4b3a2f33e9..0000000000 --- a/backend/.sqlx/query-b9842d2d8abf382bd82d8fa1de012373638be391f884f81dc387ffc465badac6.json +++ /dev/null @@ -1,20 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT jsonb_object_keys(value->'databases') FROM global_settings\n WHERE name = 'custom_instance_pg_databases'", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "jsonb_object_keys", - "type_info": "Text" - } - ], - "parameters": { - "Left": [] - }, - "nullable": [ - null - ] - }, - "hash": "b9842d2d8abf382bd82d8fa1de012373638be391f884f81dc387ffc465badac6" -} diff --git a/backend/.sqlx/query-d48ca62c86b1af7a9dd2450c1c28dc45020a2a553d8874c49f9eafedea5a9d40.json b/backend/.sqlx/query-d48ca62c86b1af7a9dd2450c1c28dc45020a2a553d8874c49f9eafedea5a9d40.json deleted file mode 100644 index 49875acd78..0000000000 --- a/backend/.sqlx/query-d48ca62c86b1af7a9dd2450c1c28dc45020a2a553d8874c49f9eafedea5a9d40.json +++ /dev/null @@ -1,24 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT dt.key AS \"datatable!\"\n FROM workspace_settings ws\n CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt\n WHERE ws.workspace_id = $1\n AND dt.key <> $2\n AND NOT dt.value ? 'permissions'\n AND dt.value->'database'->>'resource_type' = 'instance'\n AND dt.value->'database'->>'resource_path' = $3\n ORDER BY dt.key", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "datatable!", - "type_info": "Text" - } - ], - "parameters": { - "Left": [ - "Text", - "Text", - "Text" - ] - }, - "nullable": [ - null - ] - }, - "hash": "d48ca62c86b1af7a9dd2450c1c28dc45020a2a553d8874c49f9eafedea5a9d40" -} diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 9182843789..e80d0edf08 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -15660,6 +15660,7 @@ dependencies = [ "pin-project-lite", "pkcs1", "postgres-native-tls 0.5.3", + "postgres-protocol", "prometheus", "quick_cache", "rand 0.9.5", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 77f512a3dd..8571b0b99c 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -626,6 +626,7 @@ wasm-bindgen-test = "^0" convert_case = "0.6.0" getrandom = "0.2" tokio-postgres = {version = "^0.7", features = ["array-impls", "with-serde_json-1", "with-chrono-0_4", "with-uuid-1", "with-bit-vec-0_6"]} +postgres-protocol = "0.6" rust-postgres = { package = "tokio-postgres", git = "https://github.com/MaterializeInc/rust-postgres", rev = "78c1222577bb091d69bc22b1bc7ad01c14675abe"} rust-postgres-native-tls = { package = "postgres-native-tls", git = "https://github.com/MaterializeInc/rust-postgres", features = ["runtime"], rev = "78c1222577bb091d69bc22b1bc7ad01c14675abe" } bit-vec = "=0.6.3" diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 58f9aaf07d..21e445394e 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -9855e1b7a43a0a33e04f8accf1c497af3fd9b139 +dfe7b8b9204d21e0264fbea1c6f6eedf9e738d56 diff --git a/backend/migrations/20260917133824_datatable_role_cluster.down.sql b/backend/migrations/20260917133824_datatable_role_cluster.down.sql new file mode 100644 index 0000000000..379f1e3d34 --- /dev/null +++ b/backend/migrations/20260917133824_datatable_role_cluster.down.sql @@ -0,0 +1,22 @@ +-- Roles on the external cluster are live logins there; dropping the column would forget them. +LOCK TABLE datatable_role; +DO $$ +BEGIN + IF EXISTS (SELECT 1 FROM datatable_role WHERE cluster <> 'instance') THEN + RAISE EXCEPTION 'datatable_role holds roles on the external instance cluster. Delete them in instance settings first.'; + END IF; + -- Before this, only data tables on Windmill's own cluster could be under roles, and a role + -- block left with just `admin` survives deleting every external role. + IF EXISTS ( + SELECT 1 FROM workspace_settings ws, + jsonb_each(CASE WHEN jsonb_typeof(ws.datatable->'datatables') = 'object' + THEN ws.datatable->'datatables' ELSE '{}'::jsonb END) dt + WHERE dt.value->'database'->>'resource_type' = 'external_instance' + AND dt.value ? 'permissions' + ) THEN + RAISE EXCEPTION 'external instance data tables are still under roles. Turn their roles off first.'; + END IF; +END $$; +ALTER TABLE datatable_role DROP CONSTRAINT datatable_role_cluster_name_key; +ALTER TABLE datatable_role ADD CONSTRAINT datatable_role_name_key UNIQUE (name); +ALTER TABLE datatable_role DROP COLUMN cluster; diff --git a/backend/migrations/20260917133824_datatable_role_cluster.up.sql b/backend/migrations/20260917133824_datatable_role_cluster.up.sql new file mode 100644 index 0000000000..d63f06f1f4 --- /dev/null +++ b/backend/migrations/20260917133824_datatable_role_cluster.up.sql @@ -0,0 +1,8 @@ +-- A data table role is a Postgres login on one cluster: Windmill's own ('instance'), or the external +-- instance cluster ('external_instance'). Role names are the cluster's own key, so they are unique +-- per cluster rather than across the instance. +ALTER TABLE datatable_role + ADD COLUMN cluster VARCHAR(20) NOT NULL DEFAULT 'instance' + CHECK (cluster IN ('instance', 'external_instance')); +ALTER TABLE datatable_role DROP CONSTRAINT datatable_role_name_key; +ALTER TABLE datatable_role ADD CONSTRAINT datatable_role_cluster_name_key UNIQUE (cluster, name); diff --git a/backend/tests/instance_config.rs b/backend/tests/instance_config.rs index 26a46d272e..52ebbee7e7 100644 --- a/backend/tests/instance_config.rs +++ b/backend/tests/instance_config.rs @@ -1609,6 +1609,52 @@ async fn declarative_sync_rejects_an_unusable_default_allowed_origins(db: Pool

) { + clear_settings_and_configs(&db).await; + let cluster = |host: &str, password: &str| serde_json::json!({ "host": host, "port": 5432, "user": "wm_admin", "password": password }); + sqlx::query( + "INSERT INTO global_settings (name, value) VALUES + ('external_instance_pg', $1), + ('external_instance_pg_state', '{\"databases\": {\"dt_a\": {\"success\": true}}}')", + ) + .bind(cluster("pg-a.internal", "one")) + .execute(&db) + .await + .unwrap(); + let current = BTreeMap::from([( + "external_instance_pg".to_string(), + cluster("pg-a.internal", "one"), + )]); + let sync = |value: serde_json::Value| { + let desired = BTreeMap::from([("external_instance_pg".to_string(), value)]); + let (db, current) = (db.clone(), current.clone()); + async move { + windmill_common::instance_config::sync_global_settings_declarative( + &db, ¤t, &desired, + ) + .await + } + }; + + let err = sync(cluster("pg-b.internal", "one")) + .await + .expect_err("another host must be refused while dt_a is registered"); + assert!(err.to_string().contains("dt_a"), "got: {err}"); + assert_eq!( + get_global_setting(&db, "external_instance_pg").await, + Some(cluster("pg-a.internal", "one")) + ); + + sync(cluster("PG-A.internal ", "two")) + .await + .expect("a new login on the same cluster must sync"); +} + /// The accent color is interpolated into a stylesheet every user loads, so the operator /// path must refuse anything but `#rrggbb` just like the settings API does. #[sqlx::test(fixtures("base"))] diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 4948a0098a..ddb94f8c2b 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -391,7 +391,11 @@ async fn concurrent_role_creations_both_survive(db: Pool) -> anyhow::R assert_eq!(a.0, 200, "{}", a.1); assert_eq!(b.0, 200, "{}", b.1); - let catalog = windmill_common::datatable_roles::read_role_catalog(&db).await?; + let catalog = windmill_common::datatable_roles::read_role_catalog( + &db, + windmill_common::datatable_roles::DatatableRoleCluster::Instance, + ) + .await?; let recorded: Vec<&str> = catalog.values().map(|r| r.name.as_str()).collect(); for name in &names { assert!( @@ -460,7 +464,11 @@ async fn a_role_delete_that_fails_part_way_leaves_the_role_disabled( let body = resp.text().await?; assert_eq!(status, 400, "{body}"); - let catalog = windmill_common::datatable_roles::read_role_catalog(&db).await?; + let catalog = windmill_common::datatable_roles::read_role_catalog( + &db, + windmill_common::datatable_roles::DatatableRoleCluster::Instance, + ) + .await?; let role = catalog .get(&id) .expect("a failed delete keeps the entry to retry"); diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index ca93d3c682..71cbcc8460 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -42,7 +42,6 @@ use axum::{ routing::{get, post}, Json, Router, }; -use serde_json::json; use serde::{Deserialize, Serialize}; use windmill_ai::ai_cache::bump_instance_ai_config_revision; @@ -61,6 +60,7 @@ use windmill_common::{ ACCENT_COLOR_SETTING, AI_CONFIG_SETTING, APP_WORKSPACED_ROUTE_SETTING, AUTOMATE_USERNAME_CREATION_SETTING, CRITICAL_ALERT_MUTE_UI_SETTING, CUSTOM_TAGS_SETTING, DEFAULT_TAGS_WORKSPACES_SETTING, DISABLE_HUB_SETTING, EMAIL_DOMAIN_SETTING, ENV_SETTINGS, + EXTERNAL_INSTANCE_PG_SETTING, GITHUB_APP_WEBHOOK_BASE_URL_SETTING, HTTP_ROUTE_DEFAULT_ALLOWED_ORIGINS_SETTING, HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, HUB_ACCESSIBLE_URL_SETTING, HUB_BASE_URL_SETTING, INSTANCE_BANNER_SETTING, MAX_RETENTION_OVERRIDE_WORKSPACES, @@ -181,6 +181,22 @@ pub fn global_service() -> Router { "/refresh_custom_instance_user_pwd", post(refresh_custom_instance_user_pwd), ) + .route( + "/external_instance_pg/status", + get(get_external_instance_pg_status), + ) + .route( + "/external_instance_pg/setup", + post(setup_external_instance_pg), + ) + .route( + "/external_instance_pg/databases", + get(list_external_instance_pg_databases), + ) + .route( + "/external_instance_pg/databases/{name}", + post(create_external_instance_pg_database).delete(drop_external_instance_pg_database), + ) .route( "/setup_custom_instance_pg_database/{name}", post(setup_custom_instance_pg_database), @@ -853,6 +869,14 @@ pub async fn set_global_setting_internal( ))); } + if key == EXTERNAL_INSTANCE_PG_SETTING { + return windmill_common::external_instance_pg::write_external_instance_pg_setting( + db, + Some(&value), + ) + .await; + } + run_setting_pre_write_hook(db, &key, &value).await?; match value { @@ -1248,7 +1272,7 @@ async fn set_instance_config( let desired_map = desired.global_settings.to_settings_map(); if !desired_map.is_empty() { let current_map = current.global_settings.to_settings_map(); - let settings_diff = + let mut settings_diff = instance_config::diff_global_settings(¤t_map, &desired_map, ApplyMode::Merge); let ai_config_changed = settings_diff .upserts @@ -1277,8 +1301,15 @@ async fn set_instance_config( } for (key, value) in &settings_diff.upserts { - run_setting_pre_write_hook(&db, key, value).await?; + if key != EXTERNAL_INSTANCE_PG_SETTING { + run_setting_pre_write_hook(&db, key, value).await?; + } } + windmill_common::external_instance_pg::write_external_instance_pg_from_diff( + &db, + &mut settings_diff, + ) + .await?; instance_config::apply_settings_diff(&db, &settings_diff) .await @@ -1643,6 +1674,8 @@ struct CustomInstanceDb { tag: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] used_by_workspaces: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + workspace_id: Option, } #[derive(Deserialize, Debug, Serialize, Default)] @@ -1685,7 +1718,40 @@ async fn list_custom_instance_pg_databases( )) })?; - if windmill_api_auth::is_super_admin_authed(&db, &authed).await? { + if !windmill_api_auth::is_super_admin_authed(&db, &authed).await? { + // A fork copy's name gives away the workspace it was reserved for, so every pending fork on + // the instance would be listed. Kept for members of that workspace, and wherever the + // caller's workspaces use it, e.g. the fork it was finalized into. + let reserved_visible: BTreeSet = sqlx::query_scalar( + r#"SELECT e.k FROM global_settings gs + CROSS JOIN LATERAL jsonb_each(gs.value->'databases') AS e(k, v) + WHERE gs.name = 'custom_instance_pg_databases' AND e.v->>'workspace_id' IS NOT NULL + AND (EXISTS (SELECT 1 FROM usr WHERE usr.email = $1 + AND usr.workspace_id = e.v->>'workspace_id') + OR EXISTS (SELECT 1 FROM usr JOIN workspace_settings ws + ON ws.workspace_id = usr.workspace_id + CROSS JOIN LATERAL jsonb_each( + CASE WHEN jsonb_typeof(ws.datatable->'datatables') = 'object' + THEN ws.datatable->'datatables' ELSE '{}'::jsonb END) dt + WHERE usr.email = $1 + AND dt.value->'database'->>'resource_type' = 'instance' + AND dt.value->'database'->>'resource_path' = e.k))"#, + ) + .bind(&authed.email) + .fetch_all(&db) + .await? + .into_iter() + .collect(); + result.retain(|dbname, entry| { + entry.workspace_id.is_none() || reserved_visible.contains(dbname) + }); + // Which workspace reserved a copy is still only for superadmins. + for entry in result.values_mut() { + entry.workspace_id = None; + } + return Ok(Json(result)); + } + { // Enrich each database with the list of workspaces referencing it through // either a ducklake catalog or a datatable database whose resource_type is // 'instance'. Not stored in DB to avoid drift. @@ -1741,6 +1807,139 @@ async fn refresh_custom_instance_user_pwd( Ok(Json(())) } +async fn get_external_instance_pg_status( + authed: ApiAuthed, + Extension(db): Extension, +) -> JsonResult { + require_super_admin(&db, &authed).await?; + Ok(Json( + windmill_common::external_instance_pg::external_instance_pg_status(&db).await?, + )) +} + +#[derive(Deserialize)] +struct SetupExternalInstancePgBody { + #[serde(default)] + rotate_passwords: bool, +} + +async fn setup_external_instance_pg( + authed: ApiAuthed, + Extension(db): Extension, + Json(body): Json, +) -> JsonResult { + require_super_admin(&db, &authed).await?; + let report = windmill_common::external_instance_pg::setup_external_instance_pg_unchecked( + &db, + body.rotate_passwords, + ) + .await?; + let rotated = body.rotate_passwords.to_string(); + let success = report.success.to_string(); + windmill_audit::audit_oss::audit_log( + &db, + &authed, + "settings.setup_external_instance_pg", + windmill_audit::ActionKind::Update, + "global", + Some(&authed.email), + Some( + [ + ("rotate_passwords", rotated.as_str()), + ("success", success.as_str()), + ] + .into(), + ), + ) + .await?; + Ok(Json(report)) +} + +#[derive(Serialize)] +struct ExternalInstancePgDatabase { + #[serde(flatten)] + status: windmill_common::instance_config::CustomInstanceDb, + used_by_workspaces: Vec, +} + +async fn list_external_instance_pg_databases( + authed: ApiAuthed, + Extension(db): Extension, +) -> JsonResult> { + require_super_admin(&db, &authed).await?; + let databases = windmill_common::external_instance_pg::external_instance_databases(&db).await?; + let mut usages = + windmill_common::external_instance_pg::external_instance_database_usages(&db).await?; + Ok(Json( + databases + .into_iter() + .map(|(name, status)| { + let used_by_workspaces = usages.remove(&name).unwrap_or_default(); + ( + name, + ExternalInstancePgDatabase { + status, + used_by_workspaces: used_by_workspaces.into_iter().collect(), + }, + ) + }) + .collect(), + )) +} + +async fn create_external_instance_pg_database( + authed: ApiAuthed, + Extension(db): Extension, + Path(dbname): Path, + Json(body): Json, +) -> JsonResult<()> { + require_super_admin(&db, &authed).await?; + let tag = body.tag.as_deref().unwrap_or("datatable"); + let mut tx = db.begin().await?; + windmill_common::external_instance_pg::create_external_instance_database_unchecked( + &db, &mut tx, &dbname, tag, None, + ) + .await?; + tx.commit().await?; + windmill_audit::audit_oss::audit_log( + &db, + &authed, + "settings.create_external_instance_pg_database", + windmill_audit::ActionKind::Create, + "global", + Some(&authed.email), + Some([("dbname", dbname.as_str()), ("tag", tag)].into()), + ) + .await?; + Ok(Json(())) +} + +async fn drop_external_instance_pg_database( + authed: ApiAuthed, + Extension(db): Extension, + Path(dbname): Path, +) -> JsonResult<()> { + require_super_admin(&db, &authed).await?; + // A data table naming a dropped database fails on every job, far from the drop that caused it. + let mut tx = db.begin().await?; + windmill_common::external_instance_pg::drop_external_instance_database_unchecked( + &mut tx, &dbname, None, + ) + .await?; + tx.commit().await?; + windmill_audit::audit_oss::audit_log( + &db, + &authed, + "settings.drop_external_instance_pg_database", + windmill_audit::ActionKind::Delete, + "global", + Some(&authed.email), + Some([("dbname", dbname.as_str())].into()), + ) + .await?; + Ok(Json(())) +} + #[derive(Deserialize)] struct SetupCustomInstanceDbBody { tag: Option, @@ -1752,18 +1951,46 @@ async fn setup_custom_instance_pg_database( Path(dbname): Path, Json(body): Json, ) -> JsonResult { + // Before anything is recorded: the status written below replaces the registry entry, and with it + // the workspace a fork copy is reserved for. + require_super_admin(&db, &authed).await?; + windmill_common::workspaces::ensure_instance_pg_available(&db).await?; + // Fork cleanup checks and drops the database and its entry under this lock. Held from before + // the setup creates the database to after its entry is written, neither lands on the other's + // half-done state: a dropped database with its entry written back, or the reverse. + let mut tx = db.begin().await?; + windmill_common::datatable_roles::lock_instance_databases_governance(&mut tx, [dbname.trim()]) + .await?; let mut logs = CustomInstanceDbLogs::default(); let result = setup_custom_instance_pg_database_inner(authed, &db, &dbname, &mut logs).await; let success = result.is_ok(); let error = result.err().map(|e| e.to_string()); - let status = - CustomInstanceDb { logs, success, error, tag: body.tag, used_by_workspaces: vec![] }; + let status = CustomInstanceDb { + logs, + success, + error, + tag: body.tag, + used_by_workspaces: vec![], + workspace_id: None, + }; let status_json = serde_json::to_value(&status).map_err(to_anyhow)?; - // Save that the database was setup successfully - sqlx::query!( - r#"UPDATE global_settings SET value = jsonb_set(value, '{databases}', (COALESCE(value->'databases', '{}'::jsonb) || to_jsonb($1::json))) WHERE name = 'custom_instance_pg_databases'"#, - json!({ dbname: status_json }) - ).execute(&db).await?; + // The fork reservation is carried over inside the write, from whatever the row holds then: a + // rename migrating it while the setup above ran would otherwise be overwritten with the value + // this request started from, stranding the copy under the archived workspace. + let saved = sqlx::query_scalar::<_, serde_json::Value>( + r#"UPDATE global_settings SET value = jsonb_set(value, '{databases}', + COALESCE(value->'databases', '{}'::jsonb) + || jsonb_build_object($1::text, $2::jsonb || jsonb_build_object( + 'workspace_id', value->'databases'->$1::text->'workspace_id'))) + WHERE name = 'custom_instance_pg_databases' + RETURNING value->'databases'->$1::text"#, + ) + .bind(&dbname) + .bind(&status_json) + .fetch_one(&mut *tx) + .await?; + tx.commit().await?; + let status: CustomInstanceDb = serde_json::from_value(saved).map_err(to_anyhow)?; Ok(Json(status)) } diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 04dc3d4e80..ebab6214ce 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -6,7 +6,7 @@ * LICENSE-AGPL for a copy of the license. */ -//! Ownership and grants on the objects of an instance data table. +//! Ownership and grants on the objects of a data table on a cluster Windmill manages. //! //! [`datatable_permissions`](crate::datatable_permissions) decides who may connect as which role; //! this decides what each role may then touch. Every change is a real `GRANT`, `REVOKE`, @@ -33,7 +33,7 @@ use windmill_audit::audit_oss::audit_log; use windmill_audit::ActionKind; use windmill_common::datatable_roles::{ lock_role_catalog, quote_ident, read_role_catalog, read_role_catalog_tx, DatatableRoleCatalog, - ADMIN_DATATABLE_ROLE, CUSTOM_INSTANCE_USER, + DatatableRoleCluster, ADMIN_DATATABLE_ROLE, CUSTOM_INSTANCE_USER, }; use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; use windmill_common::workspaces::{resolve_governing_datatable, DataTable, GoverningDatatable}; @@ -297,22 +297,22 @@ fn role_names(catalog: &DatatableRoleCatalog) -> Vec { names } -fn ensure_instance(governing: &GoverningDatatable) -> Result<()> { - if governing.is_instance() { - return Ok(()); - } - Err(Error::BadRequest(format!( - "Data table '{}' is backed by a Postgres resource, so its access is managed on that \ - server directly. Only a data table on the Windmill instance's own database has data \ - table roles to grant to.", - governing.name - ))) +/// The cluster whose roles the data table's grants name. +fn ensure_managed(governing: &GoverningDatatable) -> Result { + governing.role_cluster().ok_or_else(|| { + Error::BadRequest(format!( + "Data table '{}' is backed by a Postgres resource, so its access is managed on that \ + server directly. Only a data table on a database Windmill manages has data table \ + roles to grant to.", + governing.name + )) + }) } /// The data table's `admin` connection, and the notices Postgres sends on it. /// -/// Authorization: connects as `custom_instance_user` with the instance's own credentials and checks -/// nothing. Callers MUST have authorized the request first — a request about to be refused must +/// Authorization: connects as `custom_instance_user` with the cluster's stored credentials and +/// checks nothing. Callers MUST have authorized the request first — a request about to be refused must /// not get as far as this connection. async fn connect_as_admin_unchecked( db: &DB, @@ -322,21 +322,33 @@ async fn connect_as_admin_unchecked( mpsc::UnboundedReceiver, String, )> { - ensure_instance(governing)?; + let cluster = ensure_managed(governing)?; // Built from the authorized entry, never by resolving the settings again: a save in between // could point the entry at a resource on another server and back, and this connection would // then alter a database the later checks of the entry never see. - let mut pg = PgDatabase::parse_uri(&windmill_common::get_database_url().await?.as_str().await)?; - pg.dbname = governing + let dbname = governing .datatable .database .as_ref() .expect("a governing entry owns a database") .resource_path .clone(); - pg.user = Some(CUSTOM_INSTANCE_USER.to_string()); - pg.password = Some(windmill_common::utils::get_custom_pg_instance_password(db).await?); - let dbname = pg.dbname.clone(); + let pg = match cluster { + DatatableRoleCluster::Instance => { + let mut pg = + PgDatabase::parse_uri(&windmill_common::get_database_url().await?.as_str().await)?; + pg.dbname = dbname.clone(); + pg.user = Some(CUSTOM_INSTANCE_USER.to_string()); + pg.password = Some(windmill_common::utils::get_custom_pg_instance_password(db).await?); + pg + } + DatatableRoleCluster::ExternalInstance => { + windmill_common::external_instance_pg::external_instance_connection_unchecked( + db, &dbname, false, + ) + .await? + } + }; let (client, notices) = connect_with_notices(db, &pg).await?; Ok((client, notices, dbname)) } @@ -1085,12 +1097,12 @@ async fn get_datatable_acl( let target: AclTarget = query.try_into()?; let governing = resolve_governing_datatable(&db, &w_id, &datatable_name).await?; ensure_reaches_governing_datatable(&db, &w_id, &datatable_name, &governing, &authed).await?; - ensure_instance(&governing)?; + let cluster = ensure_managed(&governing)?; let editable = ensure_governs_datatable(&db, &authed, &w_id, &governing) .await .is_ok(); let roles = if editable { - role_names(&read_role_catalog(&db).await?) + role_names(&read_role_catalog(&db, cluster).await?) } else { vec![] }; @@ -1376,7 +1388,7 @@ async fn authorize_acl_change( ) -> Result { let governing = resolve_governing_datatable(db, w_id, datatable_name).await?; ensure_governs_datatable(db, authed, w_id, &governing).await?; - ensure_instance(&governing)?; + ensure_managed(&governing)?; Ok(governing) } @@ -1386,8 +1398,14 @@ static APPLY_SLOT: tokio::sync::Semaphore = tokio::sync::Semaphore::const_new(1) /// provisioned before data table roles gave `custom_instance_user` none. Adds that option to its /// database and `public` privileges, and nothing else: default privileges are left alone, since a /// schema's change of owner is planned against them. Best-effort, as a grant it fails to enable is -/// refused when it runs. -async fn ensure_grant_options(client: &tokio_postgres::Client, db: &DB, dbname: &str) { +/// refused when it runs. An external instance database was created with the options, so one +/// missing there is someone's deliberate revoke and is left alone. +async fn ensure_grant_options( + client: &tokio_postgres::Client, + db: &DB, + cluster: DatatableRoleCluster, + dbname: &str, +) { let held = client .query_one( "SELECT has_database_privilege(current_database(), 'CONNECT WITH GRANT OPTION') @@ -1399,7 +1417,7 @@ async fn ensure_grant_options(client: &tokio_postgres::Client, db: &DB, dbname: ) .await .is_ok_and(|row| row.get::<_, bool>(0)); - if held { + if held || cluster != DatatableRoleCluster::Instance { return; } if let Err(e) = grant_options_as_server(db, dbname).await { @@ -1625,7 +1643,8 @@ async fn plan_datatable_acl( ) -> JsonResult { crate::datatable_acl_oss::ensure_datatable_acl_available()?; let governing = authorize_acl_change(&db, &authed, &w_id, &datatable_name).await?; - let catalog = read_role_catalog(&db).await?; + let cluster = ensure_managed(&governing)?; + let catalog = read_role_catalog(&db, cluster).await?; let (client, _notices, dbname) = connect_as_admin_unchecked(&db, &governing).await?; Ok(Json( build_plan(&client, &dbname, &catalog, &req.target, &req.change).await?, @@ -1649,6 +1668,7 @@ async fn apply_datatable_acl( // connection could wait forever on a pool that concurrent applies, queued on the same locks, // have exhausted. let governing = authorize_acl_change(&db, &authed, &w_id, &datatable_name).await?; + let cluster = ensure_managed(&governing)?; // Applies queue on an instance-wide lock while each holds a direct connection to the instance's // Postgres; unbounded, the queue alone could exhaust its connection limit. One at a time per // server, and the ones waiting hold no connection at all. @@ -1657,7 +1677,7 @@ async fn apply_datatable_acl( .await .map_err(|e| Error::internal_err(format!("ACL apply slot closed: {e}")))?; let (mut client, mut notices, dbname) = connect_as_admin_unchecked(&db, &governing).await?; - ensure_grant_options(&client, &db, &dbname).await; + ensure_grant_options(&client, &db, cluster, &dbname).await; // Held until the change is committed: a role renamed or dropped meanwhile would change what // the plan names, and a settings save could move the entry onto another database. Taken in the @@ -1672,7 +1692,7 @@ async fn apply_datatable_acl( .fetch_optional(&mut *tx) .await? .flatten(); - let catalog = read_role_catalog_tx(&mut tx).await?; + let catalog = read_role_catalog_tx(&mut tx, cluster).await?; let plan = build_plan(&client, &dbname, &catalog, &req.target, &req.change).await?; if !entry_unchanged(&governing, entry_now) || &plan.statements != confirmed { diff --git a/backend/windmill-api-workspaces/src/datatable_clone.rs b/backend/windmill-api-workspaces/src/datatable_clone.rs index a57a2216e5..6083b7b619 100644 --- a/backend/windmill-api-workspaces/src/datatable_clone.rs +++ b/backend/windmill-api-workspaces/src/datatable_clone.rs @@ -11,8 +11,8 @@ //! The fork request makes its copies before it writes the fork: each is created, filled and — for a //! data table under roles — given the source's owners and grants. What each copy was made from //! stays in the request ([`MadeCopy`]), so the fork's transaction checks it against the source as it -//! is then, and a fork that fails drops the instance copies it made ([`drop_copies_after`]) and names -//! the others, which live on servers of the workspace's own. +//! is then, and a fork that fails drops the copies it made on a cluster Windmill manages +//! ([`drop_copies_after`]) and names the others, which live on servers of the workspace's own. use std::collections::BTreeSet; @@ -22,8 +22,8 @@ use windmill_common::error::{pg_error_message, Error, Result}; use windmill_common::utils::require_admin; use windmill_common::worker::CLOUD_HOSTED; use windmill_common::workspaces::{ - get_datatable_resource_from_db_unchecked, DataTableDatabase, DataTableForkBehavior, - GoverningDatatable, + get_datatable_resource_from_db_unchecked, DataTableCatalogResourceType, DataTableDatabase, + DataTableForkBehavior, GoverningDatatable, }; use windmill_common::{PgDatabase, DB}; @@ -312,27 +312,38 @@ pub(crate) async fn drop_copies_after(db: &DB, copies: Vec, error: Err } async fn drop_copy(db: &DB, source_database: &DataTableDatabase, dbname: &str) -> Result<()> { - if source_database.resource_type - == windmill_common::workspaces::DataTableCatalogResourceType::Instance - { - match windmill_common::drop_unused_instance_database(db, dbname).await? { - windmill_common::Cleanup::Dropped => Ok(()), - windmill_common::Cleanup::InUse(users) => Err(Error::BadRequest(format!( - "kept, since workspaces {} now use it", - users.join(", ") - ))), - windmill_common::Cleanup::Waiter(pid) => Err(Error::BadRequest(format!( - "kept, since a request (pid {pid}) is still waiting to name it" - ))), + match source_database.resource_type { + DataTableCatalogResourceType::Instance => { + match windmill_common::drop_unused_instance_database(db, dbname).await? { + windmill_common::Cleanup::Dropped => Ok(()), + windmill_common::Cleanup::InUse(users) => Err(Error::BadRequest(format!( + "kept, since workspaces {} now use it", + users.join(", ") + ))), + windmill_common::Cleanup::Waiter(pid) => Err(Error::BadRequest(format!( + "kept, since a request (pid {pid}) is still waiting to name it" + ))), + } } - } else { + // Refused, and the copy kept, when anything names it by then. + DataTableCatalogResourceType::ExternalInstance => { + let mut tx = db.begin().await?; + windmill_common::external_instance_pg::drop_external_instance_database_unchecked( + &mut tx, dbname, None, + ) + .await?; + tx.commit().await?; + Ok(()) + } + DataTableCatalogResourceType::Postgresql => { // On a server of the workspace's own, where a resource edited meanwhile can already name it // and nothing locks such an edit: dropping it could take someone's data. - Err(Error::BadRequest( - "kept on its PostgreSQL server, where it may already be in use; drop it there once it \ - is not, before forking the same data table under this id again" - .to_string(), - )) + Err(Error::BadRequest( + "kept on its PostgreSQL server, where it may already be in use; drop it there once \ + it is not, before forking the same data table under this id again" + .to_string(), + )) + } } } @@ -349,10 +360,10 @@ async fn make_copy( request.name )) })?; - let is_instance = governing.is_instance(); + let managed = source_database.resource_type.is_windmill_managed(); // Resolved from the snapshot, not read again: the connection the copy is made on is then // exactly what these rows describe, which the fork's own clone of them is checked against. - let (server, connection): (PgDatabase, Option) = if is_instance { + let (server, connection): (PgDatabase, Option) = if managed { let server = serde_json::from_value( get_datatable_resource_from_db_unchecked(db, parent_w_id, request.name).await?, ) @@ -383,22 +394,44 @@ async fn make_copy( // Dumped before the database exists, so a source that cannot be read leaves nothing behind. // Ownership never carries over: the restore runs as the target's connection user. Grants do, - // except on the instance, where the replay below is what reproduces them. + // except on a cluster Windmill manages, which plants its own and where the replay below is what + // reproduces the roles'. let dump = pg_dump_database( &server, PgDumpOptions { schema_only: request.behavior == DataTableForkBehavior::SchemaOnly, no_owner: true, - no_acl: is_instance, + no_acl: managed, ..Default::default() }, ) .await?; - if is_instance { - windmill_common::create_custom_instance_database(db, request.dbname, "datatable").await?; - } else { - create_database_on_server(db, &server, request.dbname).await?; + match source_database.resource_type { + DataTableCatalogResourceType::Instance => { + windmill_common::create_custom_instance_database( + db, + request.dbname, + "datatable", + Some(parent_w_id), + ) + .await? + } + DataTableCatalogResourceType::ExternalInstance => { + let mut tx = db.begin().await?; + windmill_common::external_instance_pg::create_external_instance_database_unchecked( + db, + &mut tx, + request.dbname, + "datatable", + Some(parent_w_id), + ) + .await?; + tx.commit().await?; + } + DataTableCatalogResourceType::Postgresql => { + create_database_on_server(db, &server, request.dbname).await? + } } let target = PgDatabase { dbname: request.dbname.to_string(), ..server.clone() }; @@ -481,11 +514,16 @@ async fn fill( let replayed = now.datatable.permissions.is_some(); if replayed { crate::datatable_replay_oss::ensure_replay()?; - let catalog = read_role_catalog_tx(&mut tx).await?; + // The copy is on the source's cluster, so its roles come from that cluster's catalog. + let cluster = governing + .role_cluster() + .ok_or_else(|| Error::internal_err("a replayed clone is on a managed database"))?; + let catalog = read_role_catalog_tx(&mut tx, cluster).await?; // The replay leaves `CONNECT` to the catalog, and creating the database only tried to set // it: a copy `PUBLIC` could still connect to would admit logins the source turns away. windmill_common::datatable_roles::converge_connect_grants_with( db, + cluster, &target.dbname, &catalog, ) diff --git a/backend/windmill-api-workspaces/src/datatable_migrations.rs b/backend/windmill-api-workspaces/src/datatable_migrations.rs index df1fac203b..f762fda175 100644 --- a/backend/windmill-api-workspaces/src/datatable_migrations.rs +++ b/backend/windmill-api-workspaces/src/datatable_migrations.rs @@ -11,7 +11,7 @@ //! to keep that file focused on core workspace configuration. use crate::workspaces::{ - is_instance_datatable, pg_dump_database, strip_unreplayable_dump_lines, ItemComparison, + managed_datatable_kind, pg_dump_database, strip_unreplayable_dump_lines, ItemComparison, PgDumpOptions, }; @@ -1669,7 +1669,9 @@ async fn generate_initial_datatable_migration( // without what a replay elsewhere cannot run: the replaying user owns none of this // database's objects, and the grants Windmill plants in an instance database (`ALTER // DEFAULT PRIVILEGES FOR ROLE ...`) fail even replaying onto the same server. - let no_acl = is_instance_datatable(&db, &w_id, &datatable_name).await?; + let no_acl = managed_datatable_kind(&db, &w_id, &datatable_name) + .await? + .is_some(); let dump_file = pg_dump_database( &pg_db, PgDumpOptions { diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 185fe17b25..458e63a973 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -2300,7 +2300,8 @@ struct DataTableTables { schemas: TableListMap, #[serde(skip_serializing_if = "Option::is_none")] error: Option, - /// On the instance database: the only kind that can be under roles or have its access edited. + /// On a database Windmill manages, on its own cluster or the external one: the only kinds that + /// can be under roles or have their access edited. instance: bool, permissioned: bool, /// The roles this caller may connect as, by name; empty when not under roles. @@ -2557,7 +2558,7 @@ async fn list_one_datatable_tables( }; let result: Result<()> = async { let governing = resolve_governing_datatable(db, w_id, &entry.datatable_name).await?; - entry.instance = governing.is_instance(); + entry.instance = governing.role_cluster().is_some(); let usable = crate::datatable_permissions_oss::usable_datatable_roles(db, authed, w_id, &governing) .await?; @@ -3082,6 +3083,25 @@ pub(crate) async fn resolve_pg_source_checked( w_id: &str, source: &str, ) -> Result { + Ok( + resolve_pg_source_checked_with_kind(db, user_db, authed, w_id, source) + .await? + .0, + ) +} + +/// [`resolve_pg_source_checked`], also reporting the kind of Windmill-managed database behind the +/// source, `None` for a user resource. Callers that guard a managed connection MUST take the kind +/// from here rather than ask separately: between two reads a save can flip the entry, leaving the guards of one kind +/// applied to the connection of the other. +pub(crate) async fn resolve_pg_source_checked_with_kind( + db: &DB, + user_db: &UserDB, + authed: &ApiAuthed, + w_id: &str, + source: &str, +) -> Result<(PgDatabase, Option)> { + let mut managed_kind = None; let db_resource = if let Some(name) = source.strip_prefix("datatable://") { windmill_common::workspaces::ensure_datatable_admin_access( db, @@ -3090,7 +3110,13 @@ pub(crate) async fn resolve_pg_source_checked( &DatatableAccess::Authed(authed.to_authed_ref()), ) .await?; - get_datatable_resource_from_db_unchecked(db, w_id, name).await? + let (connection, kind) = + windmill_common::workspaces::get_datatable_connection_and_kind_unchecked( + db, w_id, name, + ) + .await?; + managed_kind = kind; + connection } else if let Some(path) = source.strip_prefix("$res:") { let db_with_authed = windmill_common::db::DbWithOptAuthed::from_authed( authed, @@ -3123,27 +3149,37 @@ pub(crate) async fn resolve_pg_source_checked( ))); }; - serde_json::from_value(db_resource) - .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e))) + let pg: PgDatabase = serde_json::from_value(db_resource) + .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e)))?; + Ok((pg, managed_kind)) } -/// Whether the data table `name` is backed by the Windmill instance's own PostgreSQL -/// rather than a user resource. -pub(crate) async fn is_instance_datatable(db: &DB, w_id: &str, name: &str) -> Result { +/// The kind of the database backing the data table `name` when Windmill manages it (on its own +/// cluster or the external one), `None` when it is a user resource. +pub(crate) async fn managed_datatable_kind( + db: &DB, + w_id: &str, + name: &str, +) -> Result> { // Resolved rather than read: a pointer entry owns no database of its own, so only the entry it - // lands on can answer. A name that resolves to nothing keeps the historical `false`. + // lands on can answer. A name that resolves to nothing keeps the historical `None`. Ok(resolve_governing_datatable(db, w_id, name) .await .ok() .and_then(|g| g.datatable.database) - .is_some_and(|d| d.resource_type == DataTableCatalogResourceType::Instance)) + .map(|d| d.resource_type) + .filter(|kind| kind.is_windmill_managed())) } /// Same, for the `datatable://` / `$res:` form the import endpoints take. -async fn is_instance_datatable_source(db: &DB, w_id: &str, source: &str) -> Result { +async fn managed_datatable_source_kind( + db: &DB, + w_id: &str, + source: &str, +) -> Result> { match source.strip_prefix("datatable://") { - Some(name) => is_instance_datatable(db, w_id, name).await, - None => Ok(false), + Some(name) => managed_datatable_kind(db, w_id, name).await, + None => Ok(None), } } @@ -3242,13 +3278,7 @@ pub(crate) async fn pg_dump_database( if let Some(ref password) = pg_db.password { cmd.env("PGPASSWORD", password); } - - if let Some(ref sslmode) = pg_db.sslmode { - cmd.env("PGSSLMODE", sslmode); - } - if let Some(options) = pg_db.non_empty_options() { - cmd.env("PGOPTIONS", options); - } + let _root_cert = apply_pg_tls_env(&mut cmd, pg_db)?; let output = cmd .output() @@ -3366,7 +3396,7 @@ async fn comment_out_unsupported_settings( /// A psql invocation against `pg_db`, carrying the connection settings the CLI reads /// from the environment. -fn psql_command(pg_db: &PgDatabase) -> tokio::process::Command { +fn psql_command(pg_db: &PgDatabase) -> Result<(tokio::process::Command, Option)> { let mut cmd = tokio::process::Command::new("psql"); cmd.arg("--host") .arg(&pg_db.host) @@ -3383,13 +3413,92 @@ fn psql_command(pg_db: &PgDatabase) -> tokio::process::Command { if let Some(ref password) = pg_db.password { cmd.env("PGPASSWORD", password); } + let root_cert = apply_pg_tls_env(&mut cmd, pg_db)?; + Ok((cmd, root_cert)) +} + +/// Give libpq the TLS settings `PgDatabase::connect` applies. The returned file holds the root +/// certificate `PGSSLROOTCERT` names, so it must outlive the command. +fn apply_pg_tls_env( + cmd: &mut tokio::process::Command, + pg_db: &PgDatabase, +) -> Result> { if let Some(ref sslmode) = pg_db.sslmode { cmd.env("PGSSLMODE", sslmode); } if let Some(options) = pg_db.non_empty_options() { cmd.env("PGOPTIONS", options); } - cmd + if let Some(pem) = pg_db + .root_certificate_pem + .as_deref() + .filter(|p| !p.is_empty()) + { + let file = DumpFile::new()?; + std::fs::write(&file.path, pem) + .map_err(|e| Error::internal_err(format!("Failed to write root certificate: {e}")))?; + cmd.env("PGSSLROOTCERT", &file.path); + return Ok(Some(file)); + } + // Only a connection that asked to be verified against the system trust store. Without a file, + // libpq's own default would look for `~/.postgresql/root.crt` and refuse a verify-* mode. libpq + // takes the special `system` value with verify-full only, so verify-ca needs the bundle itself. + if pg_db.accept_invalid_certs == Some(false) { + match pg_db.sslmode.as_deref() { + Some("verify-full") => { + cmd.env("PGSSLROOTCERT", "system"); + } + Some("verify-ca") => { + if let Some(bundle) = windmill_common::system_ca_bundle() { + cmd.env("PGSSLROOTCERT", bundle); + } + } + _ => {} + } + } + Ok(None) +} + + +#[cfg(test)] +mod pg_tls_env_tests { + use super::apply_pg_tls_env; + use windmill_common::PgDatabase; + + fn root_cert_env(sslmode: &str) -> Option { + let pg_db = PgDatabase { + host: "db".to_string(), + user: None, + password: None, + port: None, + sslmode: Some(sslmode.to_string()), + dbname: "d".to_string(), + root_certificate_pem: None, + accept_invalid_certs: Some(false), + use_iam_auth: None, + region: None, + options: None, + }; + let mut cmd = tokio::process::Command::new("psql"); + apply_pg_tls_env(&mut cmd, &pg_db).unwrap(); + cmd.as_std() + .get_envs() + .find(|(k, _)| *k == "PGSSLROOTCERT") + .and_then(|(_, v)| v.map(|v| v.to_os_string())) + } + + #[test] + fn system_roots_only_through_verify_full() { + assert_eq!( + root_cert_env("verify-full").as_deref(), + Some("system".as_ref()) + ); + // libpq refuses `sslrootcert=system` with verify-ca, which would fail every dump and restore. + assert_ne!( + root_cert_env("verify-ca").as_deref(), + Some("system".as_ref()) + ); + } } /// GUC names the server backing `pg_db` knows about. @@ -3399,7 +3508,8 @@ fn psql_command(pg_db: &PgDatabase) -> tokio::process::Command { /// and an unset mode, where `PgDatabase::connect` would hand a TLS-only server a /// plaintext socket and fail before the import ever starts. async fn server_setting_names(pg_db: &PgDatabase) -> Result> { - let output = psql_command(pg_db) + let (mut cmd, _root_cert) = psql_command(pg_db)?; + let output = cmd .arg("--tuples-only") .arg("--no-align") .arg("--command") @@ -3436,7 +3546,8 @@ pub(crate) async fn pg_import_dump(target_db: &PgDatabase, dump_file: &DumpFile) let supported_settings = server_setting_names(target_db).await?; comment_out_unsupported_settings(dump_file, &supported_settings).await?; - let output = psql_command(target_db) + let (mut cmd, _root_cert) = psql_command(target_db)?; + let output = cmd .arg("--set") .arg("ON_ERROR_STOP=1") .arg("--single-transaction") @@ -3494,9 +3605,39 @@ async fn create_pg_database( } } - if is_instance_datatable_source(&db, &w_id, &req.source).await? { - windmill_common::create_custom_instance_database(&db, &req.target_dbname, "datatable") + if let Some(source_kind) = managed_datatable_source_kind(&db, &w_id, &req.source).await? { + // Held until the copy is registered, as a rename migrates reservations to the new id under + // it once the old one is archived: a copy registered after that would be reserved for an + // id nothing answers on. + let mut tx = db.begin().await?; + windmill_common::workspaces::lock_fork_datatables(&mut tx, &w_id).await?; + let live = sqlx::query_scalar::<_, bool>("SELECT NOT deleted FROM workspace WHERE id = $1") + .bind(&w_id) + .fetch_optional(&mut *tx) + .await? + .unwrap_or(false); + if !live { + return Err(Error::BadRequest(format!("Workspace '{w_id}' is archived"))); + } + if source_kind == DataTableCatalogResourceType::ExternalInstance { + windmill_common::external_instance_pg::create_external_instance_database_unchecked( + &db, + &mut tx, + &req.target_dbname, + "datatable", + Some(&w_id), + ) .await?; + } else { + windmill_common::create_custom_instance_database( + &db, + &req.target_dbname, + "datatable", + Some(&w_id), + ) + .await?; + } + tx.commit().await?; } else { let source_pg = resolve_pg_source_checked(&db, &user_db, &authed, &w_id, &req.source).await?; @@ -3583,12 +3724,12 @@ pub(crate) async fn ensure_datatable_is_clonable( let governing = resolve_governing_datatable(db, w_id, name).await?; // The copy has to name a database of its own. A resource-backed entry reached through a // pointer names one this workspace does not own, so there is nothing here to repoint. - let is_instance = governing + let is_managed = governing .datatable .database .as_ref() - .is_some_and(|d| d.resource_type == DataTableCatalogResourceType::Instance); - if governing.workspace_id != w_id && !is_instance { + .is_some_and(|d| d.resource_type.is_windmill_managed()); + if governing.workspace_id != w_id && !is_managed { return Err(Error::BadRequest(format!( "Data table '{name}' points at a resource-backed data table in another workspace \ and cannot be copied; fork it from the workspace that owns it." @@ -3642,9 +3783,11 @@ async fn import_pg_database( } let schema_only = req.fork_behavior == DataTableForkBehavior::SchemaOnly; - let source_pg = resolve_pg_source_checked(&db, &user_db, &authed, &w_id, &req.source).await?; - let mut target_pg = - resolve_pg_source_checked(&db, &user_db, &authed, &w_id, &req.target).await?; + let mut fork_lock: Option> = None; + let (source_pg, source_kind) = + resolve_pg_source_checked_with_kind(&db, &user_db, &authed, &w_id, &req.source).await?; + let (mut target_pg, target_kind) = + resolve_pg_source_checked_with_kind(&db, &user_db, &authed, &w_id, &req.target).await?; if let Some(ref override_dbname) = req.target_dbname_override { if !windmill_api_auth::is_super_admin_authed(&db, &authed).await? { @@ -3654,6 +3797,29 @@ async fn import_pg_database( .to_string(), )); } + // The kind the connection above was built from, never a second read: an entry flipped + // to a resource between the two would keep the managed connection here and lose the + // check that decides which copy it may fill. + if let Some(kind) = target_kind { + // Held until the restore is done: fork finalization takes the first, and every + // save newly naming a database, in any workspace, the second. Nothing may start + // using this database while `psql` is still filling it. + let mut tx = db.begin().await?; + windmill_common::workspaces::lock_fork_datatables(&mut tx, &w_id).await?; + windmill_common::datatable_roles::lock_instance_databases_governance( + &mut tx, + [override_dbname.as_str()], + ) + .await?; + windmill_common::ensure_fork_database_available_to( + &mut tx, + kind, + override_dbname, + &w_id, + ) + .await?; + fork_lock = Some(tx); + } } target_pg.dbname = override_dbname.clone(); } @@ -3663,8 +3829,7 @@ async fn import_pg_database( // what it creates it owns. Grants do, except around an instance data table — Windmill // plants `custom_instance_user` grants in one, which nothing else can replay. Elsewhere // the ACLs are user intent (`REVOKE ... FROM PUBLIC`) and dropping them widens access. - let no_acl = is_instance_datatable_source(&db, &w_id, &req.target).await? - || is_instance_datatable_source(&db, &w_id, &req.source).await?; + let no_acl = target_kind.is_some() || source_kind.is_some(); let dump_file = pg_dump_database( &source_pg, @@ -3672,6 +3837,9 @@ async fn import_pg_database( ) .await?; pg_import_dump(&target_pg, &dump_file).await?; + if let Some(tx) = fork_lock { + tx.commit().await?; + } Ok(format!( "Imported from '{}' into '{}'", @@ -3772,39 +3940,76 @@ async fn edit_ducklake_config( ) .await?; - let old_ducklakes = sqlx::query_scalar!( - r#" - SELECT ws.ducklake->'ducklakes' AS ducklake_name - FROM workspace_settings ws - WHERE ws.workspace_id = $1 - "#, - &w_id + // Under the row lock the save writes with, taken before the database locks below as fork + // cleanup takes the two. + let old_ducklakes = sqlx::query_scalar::<_, Option>( + "SELECT ws.ducklake->'ducklakes' FROM workspace_settings ws + WHERE ws.workspace_id = $1 FOR UPDATE", ) + .bind(&w_id) .fetch_one(&mut *tx) .await? .unwrap_or(serde_json::Value::Null); let old_ducklakes: HashMap = serde_json::from_value(old_ducklakes).unwrap_or_default(); - // Check that non-superadmins are not abusing Instance databases - if !is_superadmin { - for (name, dl) in new_config.settings.ducklakes.iter() { - if dl.catalog.resource_type == DucklakeCatalogResourceType::Instance { - let old_dl = old_ducklakes.get(name); - if old_dl.is_none() - || old_dl.unwrap().catalog.resource_type - != DucklakeCatalogResourceType::Instance - || old_dl.unwrap().catalog.resource_path != dl.catalog.resource_path - { - return Err(Error::BadRequest( - "Only superadmins can create or modify ducklakes with Instance databases" - .to_string(), - )); - } - } + // Check that non-superadmins are not abusing Instance databases. An unchanged catalog is left + // alone either way, so a downgraded instance can still save lakes that already name an + // external instance database. + for (name, dl) in new_config.settings.ducklakes.iter() { + let kind = &dl.catalog.resource_type; + if !matches!( + kind, + DucklakeCatalogResourceType::Instance | DucklakeCatalogResourceType::ExternalInstance + ) { + continue; + } + let unchanged = old_ducklakes.get(name).is_some_and(|old| { + &old.catalog.resource_type == kind + && old.catalog.resource_path == dl.catalog.resource_path + }); + if unchanged { + continue; + } + // Before the registration check, whose refusal would otherwise tell a workspace admin + // which databases exist on the cluster. + if !is_superadmin { + return Err(Error::BadRequest( + "Only superadmins can create or modify ducklakes with Instance databases" + .to_string(), + )); + } + if *kind == DucklakeCatalogResourceType::ExternalInstance { + windmill_common::external_instance_pg::ensure_external_instance_available()?; + windmill_common::external_instance_pg::ensure_external_instance_database_registered( + &mut tx, + &dl.catalog.resource_path, + ) + .await?; + } else { + windmill_common::workspaces::ensure_instance_pg_available(&mut *tx).await?; } } + // Fork cleanup decides nothing uses an instance database under this lock, so a catalog newly + // put on one must not commit between its check and its drop. + windmill_common::datatable_roles::lock_instance_databases_governance( + &mut *tx, + new_config + .settings + .ducklakes + .iter() + .filter(|(name, dl)| { + dl.catalog.resource_type == DucklakeCatalogResourceType::Instance + && old_ducklakes.get(name.as_str()).is_none_or(|old| { + old.catalog.resource_type != DucklakeCatalogResourceType::Instance + || old.catalog.resource_path != dl.catalog.resource_path + }) + }) + .map(|(_, dl)| dl.catalog.resource_path.as_str()), + ) + .await?; + let config: serde_json::Value = serde_json::to_value(&new_config.settings) .map_err(|err| Error::internal_err(err.to_string()))?; @@ -3860,6 +4065,8 @@ async fn edit_datatable_config( let is_superadmin = require_super_admin(&db, &authed).await.is_ok(); let mut tx = db.begin().await?; + // Ahead of the settings row, as fork cleanup of this workspace takes the two. + windmill_common::workspaces::lock_fork_datatables(&mut tx, &w_id).await?; windmill_common::lock_instance_databases( &mut tx, new_config.settings.datatables.values().filter_map(|dt| { @@ -3983,6 +4190,7 @@ async fn edit_datatable_config( // so these line up with the `datatable_configured` adoption counts. created_substrates.push(match dt.database.as_ref().map(|d| d.resource_type) { Some(DataTableCatalogResourceType::Instance) => "instance", + Some(DataTableCatalogResourceType::ExternalInstance) => "external_instance", Some(DataTableCatalogResourceType::Postgresql) => "postgresql", None => "reference", }); @@ -4018,18 +4226,20 @@ async fn edit_datatable_config( ))); } // Carrying the block onto a resource-backed entry would produce a data table the chokepoint - // refuses on every job — a save that succeeds and breaks everything afterwards. Refuse it - // instead: turning roles off first is one step, and it keeps discarding an access decision - // something somebody chose rather than a side effect of moving a database. + // refuses on every job — a save that succeeds and breaks everything afterwards — and onto + // the other managed cluster, one whose role ids name nothing in that cluster's catalog. + // Refuse it instead: turning roles off first is one step, and it keeps discarding an access + // decision something somebody chose rather than a side effect of moving a database. + let old_kind = old.and_then(|old| old.database.as_ref()).map(|d| d.resource_type); if dt.permissions.is_some() && dt .database .as_ref() - .is_some_and(|d| d.resource_type != DataTableCatalogResourceType::Instance) + .is_some_and(|d| Some(d.resource_type) != old_kind) { return Err(Error::BadRequest(format!( - "Data table '{name}' is under roles, which only a data table on the instance \ - database can be. Turn its roles off before moving it to a PostgreSQL resource." + "Data table '{name}' is under roles, which belong to the cluster its database is \ + on. Turn its roles off before moving it to another kind of database." ))); } // A pointer names no database of its own, so the form's empty `database` is correct there. @@ -4054,35 +4264,52 @@ async fn edit_datatable_config( // Check that non-superadmins are not abusing Instance databases, which reach a database this // workspace does not own. Pointing an entry at another workspace's data table is not checked // here because it cannot be requested at all: `reference` is overwritten from the stored entry - // above, for every caller. + // above, for every caller. An unchanged entry is left alone either way, so a downgraded + // instance can still save settings that already name an external instance database. // // Compared against the entry the carried fields came from, not the one stored under the same // name: otherwise swapping two names keeps each database in place while moving a clone's // `governed_by` off the copy it governs. - if !is_superadmin { - for (name, dt) in new_config.settings.datatables.iter() { - let old_dt = old_datatables.get( + for (name, dt) in new_config.settings.datatables.iter() { + let Some(database) = dt + .database + .as_ref() + .filter(|d| d.resource_type.is_windmill_managed()) + else { + continue; + }; + let unchanged = old_datatables + .get( rename_src .get(name.as_str()) .copied() .unwrap_or(name.as_str()), - ); - if dt - .database - .as_ref() - .is_some_and(|d| d.resource_type == DataTableCatalogResourceType::Instance) - { - let unchanged = old_dt.and_then(|o| o.database.as_ref()).is_some_and(|o| { - o.resource_type == DataTableCatalogResourceType::Instance - && Some(&o.resource_path) == dt.database.as_ref().map(|d| &d.resource_path) - }); - if !unchanged { - return Err(Error::BadRequest( - "Only superadmins can create or modify data tables with Instance databases" - .to_string(), - )); - } - } + ) + .and_then(|o| o.database.as_ref()) + .is_some_and(|o| { + o.resource_type == database.resource_type + && o.resource_path == database.resource_path + }); + if unchanged { + continue; + } + // Before the registration check, whose refusal would otherwise tell a workspace admin + // which databases exist on the cluster. + if !is_superadmin { + return Err(Error::BadRequest( + "Only superadmins can create or modify data tables with Instance databases" + .to_string(), + )); + } + if database.resource_type == DataTableCatalogResourceType::ExternalInstance { + windmill_common::external_instance_pg::ensure_external_instance_available()?; + windmill_common::external_instance_pg::ensure_external_instance_database_registered( + &mut tx, + &database.resource_path, + ) + .await?; + } else { + windmill_common::workspaces::ensure_instance_pg_available(&mut *tx).await?; } } @@ -4102,7 +4329,7 @@ async fn edit_datatable_config( // entry through a declared rename alone, and a settings sync never declares one, so an entry // without roles that newly points at such a database — a name added, or an existing one // repointed — would answer everyone there as `admin`. That holds whichever workspace governs it. - let newly_pointed: Vec<(&String, &str)> = new_config + let newly_pointed: Vec<(&String, DataTableCatalogResourceType, &str)> = new_config .settings .datatables .iter() @@ -4112,7 +4339,7 @@ async fn edit_datatable_config( let db = dt .database .as_ref() - .filter(|d| d.resource_type == DataTableCatalogResourceType::Instance)?; + .filter(|d| d.resource_type.is_windmill_managed())?; let lookup = rename_src .get(name.as_str()) .copied() @@ -4124,38 +4351,72 @@ async fn edit_datatable_config( old_db.resource_type != db.resource_type || old_db.resource_path != db.resource_path }); - repointed.then_some((name, db.resource_path.as_str())) + repointed.then_some((name, db.resource_type, db.resource_path.as_str())) }) .collect(); // Another workspace turning roles on for the same database holds only its own settings row, so - // without this the scan below could read past its uncommitted write. + // without this the scan below could read past its uncommitted write. Every managed database + // this save newly names is locked, not just the ones the scan is about: fork cleanup takes the + // same lock to decide nothing uses the database it is dropping. + let newly_named: std::collections::BTreeSet<&str> = new_config + .settings + .datatables + .iter() + .filter_map(|(name, dt)| { + let db = dt + .database + .as_ref() + .filter(|d| d.resource_type == DataTableCatalogResourceType::Instance)?; + let lookup = rename_src + .get(name.as_str()) + .copied() + .unwrap_or(name.as_str()); + old_datatables + .get(lookup) + .and_then(|old| old.database.as_ref()) + .is_none_or(|old_db| { + old_db.resource_type != db.resource_type + || old_db.resource_path != db.resource_path + }) + .then_some(db.resource_path.as_str()) + }) + .collect(); windmill_common::datatable_roles::lock_instance_databases_governance( &mut *tx, - newly_pointed.iter().map(|(_, dbname)| *dbname), + newly_pointed + .iter() + .map(|(_, _, dbname)| *dbname) + .chain(newly_named.iter().copied()), ) .await?; - let governed_elsewhere: Vec = if newly_pointed.is_empty() { + let governed_elsewhere: Vec<(String, String)> = if newly_pointed.is_empty() { vec![] } else { - sqlx::query_scalar( - "SELECT DISTINCT dt.value->'database'->>'resource_path' FROM workspace_settings ws + sqlx::query_as( + "SELECT DISTINCT dt.value->'database'->>'resource_type', + dt.value->'database'->>'resource_path' + FROM workspace_settings ws CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt WHERE ws.workspace_id <> $1 AND (dt.value ? 'permissions' OR dt.value ? 'governed_by') - AND dt.value->'database'->>'resource_type' = 'instance'", + AND dt.value->'database'->>'resource_type' IN ('instance', 'external_instance')", ) .bind(&w_id) .fetch_all(&mut *tx) .await? }; - for (name, dbname) in newly_pointed { + for (name, kind, dbname) in newly_pointed { let governed_here = old_datatables.values().any(|old| { (old.permissions.is_some() || old.governed_by.is_some()) - && old.database.as_ref().is_some_and(|d| { - d.resource_type == DataTableCatalogResourceType::Instance - && d.resource_path == dbname - }) + && old + .database + .as_ref() + .is_some_and(|d| d.resource_type == kind && d.resource_path == dbname) }); - if governed_here || governed_elsewhere.iter().any(|g| g == dbname) { + if governed_here + || governed_elsewhere + .iter() + .any(|(k, p)| k == kind.as_ref() && p == dbname) + { return Err(Error::BadRequest(format!( "Data table '{name}' would point at database '{dbname}', which a data table under \ roles uses, without carrying those roles: everyone reaching '{name}' would connect \ @@ -8429,13 +8690,14 @@ async fn point_kept_datatables_at_parent( if dt.reference.is_some() { continue; } - // Only instance databases. A resource-backed data table names a resource, and the settings - // clone gave the fork its own copy of that resource in its own workspace — pointing at the - // parent's entry would silently move the fork onto the parent's resource instead. + // Only instance databases, on either cluster. A resource-backed data table names a + // resource, and the settings clone gave the fork its own copy of that resource in its own + // workspace — pointing at the parent's entry would silently move the fork onto the + // parent's resource instead. if dt .database .as_ref() - .is_none_or(|d| d.resource_type != DataTableCatalogResourceType::Instance) + .is_none_or(|d| !d.resource_type.is_windmill_managed()) { continue; } @@ -8609,11 +8871,39 @@ async fn apply_forked_datatable( })?, }; - if database.resource_type == DataTableCatalogResourceType::Instance { + if database.resource_type == DataTableCatalogResourceType::ExternalInstance { + windmill_common::external_instance_pg::ensure_external_instance_database_registered( + tx, + &fdt.new_dbname, + ) + .await?; + } + if database.resource_type.is_windmill_managed() { + // Held until the fork commits, as every save newly naming a database takes it: none may + // claim the copy between the check below and this fork's entry landing on it. + windmill_common::datatable_roles::lock_instance_databases_governance( + &mut **tx, + [fdt.new_dbname.as_str()], + ) + .await?; + } + if database.resource_type.is_windmill_managed() + && !windmill_api_auth::is_super_admin_authed(db, authed).await? + { + windmill_common::ensure_fork_database_available_to( + &mut **tx, + database.resource_type, + &fdt.new_dbname, + parent_w_id, + ) + .await?; + } + if database.resource_type.is_windmill_managed() { // The whole `database` object, not just its `resource_path`: a pointer entry has none to - // patch. `reference` goes with it — exactly one of the two may be set. + // patch. `reference` goes with it — exactly one of the two may be set. The copy was created + // on the same cluster as its source, so it keeps the source's kind. let new_database = serde_json::json!({ - "resource_type": "instance", + "resource_type": database.resource_type, "resource_path": &fdt.new_dbname, }); sqlx::query( @@ -9184,6 +9474,11 @@ async fn write_workspace_fork( .bind(&nw.id) .execute(&mut *tx) .await?; + // Before the settings clone reads the parent's data tables: a pointer this fork ends up with + // must not be written after cleanup of the parent decided that nothing points at its copies. + // Also before the external cluster's lifecycle lock, which finalizing an external copy takes: + // fork cleanup takes the two in this order. + windmill_common::workspaces::lock_fork_datatables(&mut tx, &parent_workspace_id).await?; if nw.is_dev_workspace { // The checks above ran outside a transaction, so the parent's eligibility and the chain's @@ -9328,6 +9623,32 @@ async fn write_workspace_fork( ))); } } + // Saves name an external database under the external cluster's lifecycle lock instead. + let external_copies: Vec<&str> = copies + .iter() + .filter(|c| { + c.source_database.resource_type == DataTableCatalogResourceType::ExternalInstance + }) + .map(|c| c.dbname.as_str()) + .collect(); + if !external_copies.is_empty() { + windmill_common::external_instance_pg::lock_external_instance_pg_state(&mut tx).await?; + } + for dbname in &external_copies { + let uses = windmill_common::workspaces::managed_database_uses( + &mut tx, + DataTableCatalogResourceType::ExternalInstance, + dbname, + None, + ) + .await?; + if !uses.is_empty() { + return Err(Error::BadRequest(format!( + "Database '{dbname}' copied for this fork is already used by {}; fork again", + uses.join(", ") + ))); + } + } // Update forked datatable settings to point to new databases for fdt in &nw.forked_datatables { diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index 9039bda2ef..8ca98f66d1 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -56,6 +56,11 @@ pub(crate) async fn change_workspace_id( let mut tx = db.begin().await?; + // The settings copy below carries every data table entry to the new id, which fork cleanup of + // the old id cannot see until this commits: without the lock it could drop a copy the renamed + // workspace goes on using. Before the pairing lock, as forking takes the two in that order. + windmill_common::workspaces::lock_fork_datatables(&mut tx, &old_id).await?; + // A rename rewrites the workspace's dev flag and reparents its children, so it decides on the // same state the pairing handlers do: without this lock a concurrent create/attach could commit // an active dev workspace under the shell this rename is about to archive. Both ids, since the @@ -869,6 +874,10 @@ pub(crate) async fn change_workspace_id( } } + // After every workspace_settings write above: fork cleanup locks a settings row before the + // registry, so taking the registry first here would deadlock with it. + migrate_fork_reservations(&mut tx, &old_id, &rw.new_id).await?; + // Audit log in the same transaction as the workspace changes audit_log( &mut *tx, @@ -937,6 +946,14 @@ pub(crate) async fn change_workspace_id( let (_schedules_count, canceled_count, _deleted_tokens_count) = archive_workspace_impl(&db, &old_id, &authed.username, None).await?; + // The old id stays live between the commit above and the archive, and a fork copy created for + // it in that window registers under it. Creation checks the workspace is live under the fork + // lock, so once this has run under it, no copy can be reserved for the old id any more. + let mut tx = db.begin().await?; + windmill_common::workspaces::lock_fork_datatables(&mut tx, &old_id).await?; + migrate_fork_reservations(&mut tx, &old_id, &rw.new_id).await?; + tx.commit().await?; + info!( "Workspace id change completed: moved {} to {}, archived old workspace", old_id, rw.new_id @@ -948,6 +965,50 @@ pub(crate) async fn change_workspace_id( )) } +/// A fork copy reserved for the old id would otherwise be unreachable: its creator cannot import +/// into it or finish its fork under the new id, and nothing else would ever drop it. +async fn migrate_fork_reservations( + tx: &mut Transaction<'_, Postgres>, + old_id: &str, + new_id: &str, +) -> Result<()> { + const MIGRATE: &str = r#"UPDATE global_settings SET value = jsonb_set(value, '{databases}', ( + SELECT COALESCE(jsonb_object_agg(k, CASE WHEN v->>'workspace_id' = $1 + THEN jsonb_set(v, '{workspace_id}', to_jsonb($2::text)) ELSE v END), '{}'::jsonb) + FROM jsonb_each(COALESCE(value->'databases', '{}'::jsonb)) AS e(k, v) + )) + WHERE name = $3"#; + sqlx::query(MIGRATE) + .bind(old_id) + .bind(new_id) + .bind("custom_instance_pg_databases") + .execute(&mut **tx) + .await?; + // External copies are registered in the cluster state, which its writers rewrite whole under + // the lifecycle lock. Only taken when there is something to move, as setup can hold it for as + // long as the cluster takes to answer. + let state = windmill_common::global_settings::EXTERNAL_INSTANCE_PG_STATE_SETTING; + let reserved = sqlx::query_scalar::<_, bool>( + "SELECT EXISTS (SELECT 1 FROM global_settings, jsonb_each(value->'databases') AS e(k, v) + WHERE name = $1 AND jsonb_typeof(value->'databases') = 'object' + AND v->>'workspace_id' = $2)", + ) + .bind(state) + .bind(old_id) + .fetch_one(&mut **tx) + .await?; + if reserved { + windmill_common::external_instance_pg::lock_external_instance_pg_state(tx).await?; + sqlx::query(MIGRATE) + .bind(old_id) + .bind(new_id) + .bind(state) + .execute(&mut **tx) + .await?; + } + Ok(()) +} + #[derive(Deserialize)] pub(crate) struct DeleteWorkspaceQuery { pub(crate) only_delete_forks: Option, @@ -1430,9 +1491,7 @@ pub async fn drop_forked_datatable_databases( _ => continue, }; - if database.resource_type - == windmill_common::workspaces::DataTableCatalogResourceType::Instance - { + if database.resource_type.is_windmill_managed() { let db_to_drop = &database.resource_path; if !db_to_drop.starts_with("wm_fork_") { errors.push(format!( @@ -1441,7 +1500,112 @@ pub async fn drop_forked_datatable_databases( )); continue; } - if let Err(e) = windmill_common::drop_custom_instance_database(&db, db_to_drop).await { + // The fork's own entry is what is going away; anything else still reaching the copy, + // a child fork's pointer at this entry included, keeps it. The lock keeps a child fork + // from gaining such a pointer before the drop. + // A task of its own, so a client going away cannot stop it between dropping the + // database and committing the entry's removal. + let dropped = tokio::spawn({ + let (db, w_id, dt_name, db_to_drop) = ( + db.clone(), + w_id.clone(), + dt_name.clone(), + db_to_drop.clone(), + ); + let resource_type = database.resource_type; + async move { + let mut tx = db.begin().await?; + // The three locks a settings save takes, in its order: this workspace's data + // tables, its settings row, and the database itself. Without them a save could + // rename this entry, or point another one here, either side of the check below. + windmill_common::workspaces::lock_fork_datatables(&mut tx, &w_id).await?; + // The snapshot above was read unlocked: a save committing since could have + // repointed this entry, and the entry is removed below whatever it names by then. + let current = sqlx::query_scalar::<_, Option>( + "SELECT datatable->'datatables'->$2 FROM workspace_settings + WHERE workspace_id = $1 FOR UPDATE", + ) + .bind(&w_id) + .bind(&dt_name) + .fetch_optional(&mut *tx) + .await? + .flatten() + .and_then(|v| serde_json::from_value::(v).ok()); + if !current.is_some_and(|dt| { + dt.forked_from.is_some() + && dt.database.is_some_and(|d| { + d.resource_type == resource_type && d.resource_path == db_to_drop + }) + }) { + return Err(Error::BadRequest( + "the data table changed while it was being cleaned up".to_string(), + )); + } + let external = resource_type + == windmill_common::workspaces::DataTableCatalogResourceType::ExternalInstance; + if external { + // Before the governance lock, in the order settings saves and fork + // finalization take the two. + windmill_common::external_instance_pg::lock_external_instance_pg_state( + &mut tx, + ) + .await?; + } + windmill_common::datatable_roles::lock_instance_databases_governance( + &mut tx, + [db_to_drop.as_str()], + ) + .await?; + // Before the entry is removed: child fork pointers are found through it. + let uses = windmill_common::workspaces::managed_database_uses( + &mut tx, + resource_type, + &db_to_drop, + Some((w_id.as_str(), dt_name.as_str())), + ) + .await?; + if !uses.is_empty() { + return Err(Error::BadRequest(format!( + "it is still used by {}", + uses.join(", ") + ))); + } + // The entry goes with the database: a fork this one is cloned into afterwards + // must not inherit a pointer at a data table whose database is gone. + sqlx::query( + "UPDATE workspace_settings SET datatable = datatable #- ARRAY['datatables', $2] + WHERE workspace_id = $1", + ) + .bind(&w_id) + .bind(&dt_name) + .execute(&mut *tx) + .await?; + if external { + // Checks the uses of the database itself, and unregisters it. + windmill_common::external_instance_pg::drop_external_instance_database_unchecked( + &mut tx, + &db_to_drop, + Some((w_id.as_str(), dt_name.as_str())), + ) + .await?; + } else { + windmill_common::drop_custom_instance_database_keep_entry(&db, &db_to_drop) + .await?; + sqlx::query( + "UPDATE global_settings SET value = value #- ARRAY['databases', $1] + WHERE name = 'custom_instance_pg_databases'", + ) + .bind(&db_to_drop) + .execute(&mut *tx) + .await?; + } + tx.commit().await?; + Ok::<_, Error>(()) + } + }) + .await + .unwrap_or_else(|e| Err(Error::internal_err(format!("cleanup task failed: {e}")))); + if let Err(e) = dropped { errors.push(format!( "Could not drop instance database '{}' for datatable://{}: {}", db_to_drop, dt_name, e @@ -1808,7 +1972,17 @@ async fn resolve_fork_catalog_pg( "ducklake://{ducklake_name}: malformed registry catalog identity `{catalog}`" )) })?; - let catalog_resource = if resource_type == "instance" { + let catalog_resource = if resource_type == "external_instance" { + serde_json::to_value( + windmill_common::external_instance_pg::external_instance_connection_unchecked( + db, + resource_path, + false, + ) + .await?, + ) + .map_err(|e| Error::internal_err(format!("serializing pg creds: {e}")))? + } else if resource_type == "instance" { let mut pg_creds = windmill_common::PgDatabase::parse_uri( &windmill_common::get_database_url().await?.as_str().await, )?; diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 4a1ce62bad..cf373241d4 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -1572,6 +1572,104 @@ paths: schema: type: object + /settings/external_instance_pg/status: + get: + summary: Returns whether the external instance cluster is configured and how its last setup went + operationId: getExternalInstancePgStatus + tags: + - setting + responses: + "200": + description: external instance cluster status + content: + application/json: + schema: + $ref: "#/components/schemas/ExternalInstancePgStatus" + + /settings/external_instance_pg/setup: + post: + summary: Sets up the external instance cluster with its saved admin login, optionally rotating the passwords Windmill manages on it (enterprise edition only) + operationId: setupExternalInstancePg + tags: + - setting + requestBody: + required: true + content: + application/json: + schema: + type: object + properties: + rotate_passwords: + type: boolean + responses: + "200": + description: the setup report, also stored as the last setup + content: + application/json: + schema: + $ref: "#/components/schemas/ExternalInstancePgSetupReport" + + /settings/external_instance_pg/databases: + get: + summary: Lists the databases Windmill created on the external instance cluster, with the workspaces whose data tables, Ducklake catalogs or pending fork cleanups use each + operationId: listExternalInstancePgDatabases + tags: + - setting + responses: + "200": + description: databases by name + content: + application/json: + schema: + type: object + additionalProperties: + $ref: "#/components/schemas/CustomInstanceDb" + + /settings/external_instance_pg/databases/{name}: + post: + summary: Creates a database on the external instance cluster (enterprise edition only) + operationId: createExternalInstancePgDatabase + tags: + - setting + parameters: + - name: name + in: path + required: true + schema: + type: string + requestBody: + required: true + content: + application/json: + schema: + type: object + properties: + tag: + $ref: "#/components/schemas/CustomInstanceDbTag" + responses: + "200": + description: database created + content: + application/json: + schema: {} + delete: + summary: Drops a database Windmill created on the external instance cluster, refused while a data table, Ducklake catalog or pending fork cleanup uses it + operationId: dropExternalInstancePgDatabase + tags: + - setting + parameters: + - name: name + in: path + required: true + schema: + type: string + responses: + "200": + description: database dropped + content: + application/json: + schema: {} + /settings/list_custom_instance_pg_databases: post: summary: Returns the set-up statuses of custom instance pg databases @@ -1590,13 +1688,19 @@ paths: /settings/datatable_roles: get: - summary: list the instance's data table roles + summary: list the data table roles of one Windmill-managed Postgres cluster operationId: listInstanceDatatableRoles tags: - setting + parameters: + - in: query + name: cluster + required: false + schema: + $ref: "#/components/schemas/DatatableRoleCluster" responses: "200": - description: the instance role catalog + description: the cluster's role catalog content: application/json: schema: @@ -1604,7 +1708,7 @@ paths: items: $ref: "#/components/schemas/InstanceDatatableRole" post: - summary: create a data table role on the instance's Postgres cluster + summary: create a data table role on a Windmill-managed Postgres cluster operationId: createInstanceDatatableRole tags: - setting @@ -1618,6 +1722,8 @@ paths: properties: name: type: string + cluster: + $ref: "#/components/schemas/DatatableRoleCluster" responses: "200": description: the created role @@ -5351,7 +5457,7 @@ paths: type: string resource_type: type: string - enum: [postgres, instance] + enum: [postgres, instance, external_instance] resource_path: type: string governing_workspace_id: @@ -34048,9 +34154,55 @@ components: - ducklake - datatable + ExternalInstancePgSetupStep: + type: object + required: [name, status, message] + properties: + name: + type: string + status: + type: string + enum: [ok, warning, error] + message: + type: string + + ExternalInstancePgSetupReport: + type: object + required: [success, finished_at, steps] + properties: + success: + type: boolean + description: no step failed; warnings leave it true + finished_at: + type: string + format: date-time + steps: + type: array + items: + $ref: "#/components/schemas/ExternalInstancePgSetupStep" + + ExternalInstancePgStatus: + type: object + required: [configured, database_count] + properties: + configured: + type: boolean + database_count: + type: integer + last_setup: + $ref: "#/components/schemas/ExternalInstancePgSetupReport" + + DatatableRoleCluster: + type: string + description: >- + The Windmill-managed Postgres cluster a data table role is a login on: Windmill's own + (behind `instance` data tables) or the external instance cluster (behind + `external_instance` ones). Defaults to `instance`. + enum: [instance, external_instance] + InstanceDatatableRole: type: object - required: [id, name, enabled] + required: [id, name, enabled, cluster] properties: id: type: string @@ -34058,6 +34210,8 @@ components: type: string enabled: type: boolean + cluster: + $ref: "#/components/schemas/DatatableRoleCluster" DatatableRoleTenants: type: object @@ -34079,8 +34233,10 @@ components: supported: type: boolean description: >- - Whether this data table can be put under roles at all. Only one backed by the - instance database can: a role is a login on that cluster. + Whether this data table can be put under roles at all. Only one on a database Windmill + manages can: a role is a login on that database's cluster. + cluster: + $ref: "#/components/schemas/DatatableRoleCluster" permissioned: type: boolean default_role: @@ -34392,7 +34548,10 @@ components: type: array items: type: string - description: Workspaces that reference this database via a ducklake catalog or datatable database with resource_type 'instance'. Computed at request time, not persisted. + description: Workspaces that reference this database through a ducklake catalog or a datatable database of the kind being listed — 'instance' for the instance databases endpoint, 'external_instance' for the external cluster one. Computed at request time, not persisted, and only returned to superadmins. + workspace_id: + type: string + description: The workspace a member created this database for as a fork copy. Only that workspace can import into it or point a fork at it. NewSqsTrigger: type: object @@ -36400,6 +36559,7 @@ components: - postgresql - mysql - instance + - external_instance resource_path: type: string required: @@ -36463,6 +36623,7 @@ components: enum: - postgresql - instance + - external_instance resource_path: type: string required: diff --git a/backend/windmill-common/Cargo.toml b/backend/windmill-common/Cargo.toml index e95fadfe66..96fa7ceb63 100644 --- a/backend/windmill-common/Cargo.toml +++ b/backend/windmill-common/Cargo.toml @@ -76,6 +76,7 @@ bitflags.workspace = true once_cell.workspace = true phf.workspace = true tokio-postgres.workspace = true +postgres-protocol.workspace = true postgres-native-tls.workspace = true native-tls.workspace = true diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index a9195055ac..f355e733cb 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -6,22 +6,67 @@ * LICENSE-AGPL for a copy of the license. */ -//! The instance's data table role catalog. +//! The instance's data table role catalogs. //! -//! A data table role is a real Postgres login role on the Windmill cluster, named exactly as the -//! user named it, shared by every instance database. Windmill decides who may ask for a role (the -//! per-data-table tenant lists in [`crate::workspaces`]); Postgres decides what the role may then -//! touch. The catalog here is only the first half's vocabulary plus the cluster provisioning. +//! A data table role is a real Postgres login role on one cluster — Windmill's own, or the external +//! instance cluster — named exactly as the user named it, shared by every database Windmill manages +//! on that cluster. Each cluster has its own catalog: a role exists where it was created and nowhere +//! else. Windmill decides who may ask for a role (the per-data-table tenant lists in +//! [`crate::workspaces`]); Postgres decides what the role may then touch. The catalog here is only +//! the first half's vocabulary plus the cluster provisioning. //! //! Entries are keyed by a generated id so a rename moves nothing else: tenants name the id. use std::collections::BTreeMap; +use serde::{Deserialize, Serialize}; + use crate::{ error::{Error, Result}, + workspaces::DataTableCatalogResourceType, DB, }; +/// The cluster a role catalog belongs to. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum DatatableRoleCluster { + /// Windmill's own Postgres, behind `instance` data tables. + #[default] + Instance, + /// The external instance cluster, behind `external_instance` data tables. + ExternalInstance, +} + +impl DatatableRoleCluster { + pub fn as_str(self) -> &'static str { + match self { + Self::Instance => "instance", + Self::ExternalInstance => "external_instance", + } + } + + pub fn parse(value: &str) -> Result { + match value { + "instance" => Ok(Self::Instance), + "external_instance" => Ok(Self::ExternalInstance), + other => Err(Error::BadRequest(format!( + "Unknown data table role cluster '{other}': expected instance or external_instance" + ))), + } + } + + /// The cluster whose roles a data table on `kind` can use. `None` for a resource-backed one, + /// which is never under roles. + pub fn of(kind: DataTableCatalogResourceType) -> Option { + match kind { + DataTableCatalogResourceType::Instance => Some(Self::Instance), + DataTableCatalogResourceType::ExternalInstance => Some(Self::ExternalInstance), + DataTableCatalogResourceType::Postgresql => None, + } + } +} + /// The connection every data table resolved to before roles existed (`custom_instance_user`). It /// owns every pre-existing object, so it is a reserved name rather than a catalog entry: never /// created, renamed or dropped. @@ -164,28 +209,42 @@ pub async fn lock_instance_databases_governance<'a>( /// need the names — but callers MUST NOT let `pwd` reach a response, a log line, an audit record /// or an export. Nothing about who may call it: the credential is the whole risk, and `Debug` is /// hand-written to redact it for the same reason. -pub async fn read_role_catalog(db: &DB) -> Result { - crate::datatable_roles_oss::read_role_catalog(db).await +pub async fn read_role_catalog( + db: &DB, + cluster: DatatableRoleCluster, +) -> Result { + crate::datatable_roles_oss::read_role_catalog(db, cluster).await } /// As [`read_role_catalog`], reading inside the caller's transaction so the value is the one /// [`lock_role_catalog`] is protecting. Same disclosure contract. pub async fn read_role_catalog_tx( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + cluster: DatatableRoleCluster, ) -> Result { - crate::datatable_roles_oss::read_role_catalog_tx(tx).await + crate::datatable_roles_oss::read_role_catalog_tx(tx, cluster).await } -/// Record a role, in the caller's transaction so it commits with the `CREATE ROLE` it describes. +/// The cluster a role belongs to, or `None` if no role has this id. +pub async fn role_cluster( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + id: &str, +) -> Result> { + crate::datatable_roles_oss::role_cluster(tx, id).await +} + +/// Record a role, in the caller's transaction. On Windmill's own cluster that commits it with the +/// `CREATE ROLE` it describes; on the external cluster the role already exists by then. /// /// Authorization: writes a generated Postgres credential. Callers MUST restrict this to superadmin /// paths and MUST hold [`lock_role_catalog`] on `tx`. pub async fn insert_role_catalog_entry( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, id: &str, + cluster: DatatableRoleCluster, role: &InstanceDatatableRole, ) -> Result<()> { - crate::datatable_roles_oss::insert_role_catalog_entry(tx, id, role).await + crate::datatable_roles_oss::insert_role_catalog_entry(tx, id, cluster, role).await } /// Update a role's recorded name, login flag and password. Same contract as @@ -215,7 +274,7 @@ pub fn role_id_by_name<'a>(catalog: &'a DatatableRoleCatalog, name: &str) -> Res .find(|(_, role)| role.name == name) .ok_or_else(|| { Error::NotFound(format!( - "'{name}' is not a data table role of this instance. Defined roles: {}.", + "'{name}' is not a data table role of this database's cluster. Defined roles: {}.", catalog .values() .map(|r| r.name.as_str()) @@ -231,70 +290,90 @@ pub fn role_id_by_name<'a>(catalog: &'a DatatableRoleCatalog, name: &str) -> Res Ok(entry.0.as_str()) } -/// Every instance database the registry knows about. Role provisioning has to reach all of them: -/// a role that cannot `CONNECT` to a database is refused by Postgres before any grant matters. +/// Every database Windmill manages on `cluster`. Role provisioning has to reach all of them: a role +/// that cannot `CONNECT` to a database is refused by Postgres before any grant matters. /// -/// Authorization: checks nothing, and names every instance database across all workspaces. Callers +/// Authorization: checks nothing, and names every managed database across all workspaces. Callers /// MUST be superadmin-gated or keep the names server-side; never return them to a workspace caller. -pub async fn registered_instance_databases(db: &DB) -> Result> { - crate::datatable_roles_oss::registered_instance_databases(db).await +pub async fn registered_instance_databases( + db: &DB, + cluster: DatatableRoleCluster, +) -> Result> { + crate::datatable_roles_oss::registered_instance_databases(db, cluster).await } -/// `CONNECT` on `dbname` for every enabled role, and none for `PUBLIC`. Run at role creation, at -/// database creation, and lazily whenever an instance data table is administered, so a database -/// provisioned before a role existed is repaired rather than left silently unreachable. +/// `CONNECT` on `dbname` for every enabled role of `cluster`, and none for `PUBLIC`. Run at role +/// creation, at database creation, and lazily whenever a managed data table is administered, so a +/// database provisioned before a role existed is repaired rather than left silently unreachable. /// /// Authorization: rewrites a database's ACL with the server's own credentials and checks nothing. /// Callers MUST have authorized administration of `dbname` — superadmin, or an admin of the /// workspace governing a data table on it. -pub async fn converge_connect_grants(db: &DB, dbname: &str) -> Result<()> { - crate::datatable_roles_oss::converge_connect_grants(db, dbname).await +pub async fn converge_connect_grants( + db: &DB, + cluster: DatatableRoleCluster, + dbname: &str, +) -> Result<()> { + crate::datatable_roles_oss::converge_connect_grants(db, cluster, dbname).await } -/// As [`converge_connect_grants`], with a catalog the caller already read. Same contract. +/// As [`converge_connect_grants`], with the catalog of `cluster` the caller already read. Same +/// contract. pub async fn converge_connect_grants_with( db: &DB, + cluster: DatatableRoleCluster, dbname: &str, catalog: &DatatableRoleCatalog, ) -> Result<()> { - crate::datatable_roles_oss::converge_connect_grants_with(db, dbname, catalog).await + crate::datatable_roles_oss::converge_connect_grants_with(db, cluster, dbname, catalog).await } -/// `CREATE ROLE LOGIN PASSWORD ...; GRANT TO custom_instance_user`, and `CONNECT` on -/// every registered database. No privileges beyond that — an admin grants them through SQL or the -/// ACL editor. +/// `CREATE ROLE LOGIN PASSWORD ...; GRANT TO custom_instance_user` on `cluster`. No +/// privileges beyond that — an admin grants them through SQL or the ACL editor. +/// +/// On Windmill's own cluster the DDL runs on `tx`, so it commits with the catalog row. The external +/// cluster is another server: the role is created there before `tx` commits, and callers MUST drop +/// it again ([`drop_datatable_role`]) if `tx` then fails to commit. /// /// Authorization: creates a cluster-wide Postgres login. Callers MUST restrict this to superadmin -/// paths, and MUST hold [`lock_role_catalog`] on the same transaction. -pub async fn create_instance_role( +/// paths, and MUST hold [`lock_role_catalog`] on `tx`. +pub async fn create_datatable_role( + db: &DB, tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + cluster: DatatableRoleCluster, name: &str, password: &str, ) -> Result<()> { - crate::datatable_roles_oss::create_instance_role(tx, name, password).await + crate::datatable_roles_oss::create_datatable_role(db, tx, cluster, name, password).await } /// Authorization: alters a cluster-wide Postgres login. Callers MUST restrict this to superadmin -/// paths, and MUST hold [`lock_role_catalog`] on the same transaction. -pub async fn set_instance_role_login( +/// paths, and MUST hold [`lock_role_catalog`] on `tx`. +pub async fn set_datatable_role_login( + db: &DB, tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + cluster: DatatableRoleCluster, name: &str, enabled: bool, ) -> Result<()> { - crate::datatable_roles_oss::set_instance_role_login(tx, name, enabled).await + crate::datatable_roles_oss::set_datatable_role_login(db, tx, cluster, name, enabled).await } -/// A rename discards an md5-hashed password, so the caller has to hand over a fresh one. +/// A rename discards an md5-hashed password, so the caller has to hand over a fresh one. On the +/// external cluster the rename lands before `tx` commits, and callers MUST rename it back if `tx` +/// then fails to commit. /// /// Authorization: renames a cluster-wide Postgres login. Callers MUST restrict this to superadmin -/// paths, and MUST hold [`lock_role_catalog`] on the same transaction. -pub async fn rename_instance_role( +/// paths, and MUST hold [`lock_role_catalog`] on `tx`. +pub async fn rename_datatable_role( + db: &DB, tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + cluster: DatatableRoleCluster, from: &str, to: &str, password: &str, ) -> Result<()> { - crate::datatable_roles_oss::rename_instance_role(tx, from, to, password).await + crate::datatable_roles_oss::rename_datatable_role(db, tx, cluster, from, to, password).await } /// A role owning anything in any database blocks its own `DROP ROLE`, and both its objects and the @@ -302,8 +381,9 @@ pub async fn rename_instance_role( /// registry. An unreachable database aborts the whole delete: dropping the role while one database /// still holds objects owned by it leaves those objects owned by a numeric OID nobody can name. /// -/// Each pass runs as the instance's own Postgres user rather than `custom_instance_user`, which -/// owns the databases and can therefore revoke a grant whoever made it. `custom_instance_user` +/// Each pass runs as the cluster's administrator rather than `custom_instance_user`: on Windmill's +/// own cluster the instance's Postgres user, on the external one its configured admin login. Both +/// own the databases and can therefore revoke a grant whoever made it. `custom_instance_user` /// could only undo what it granted itself, so a privilege planted by an operator in psql — the /// ordinary way privileges reach a role — would survive and block the drop. /// @@ -311,16 +391,19 @@ pub async fn rename_instance_role( /// MUST restrict this to superadmin paths, and MUST hold [`lock_role_catalog`] on `tx`. /// /// The per-database passes open their own connections and cannot join `tx`; the lock is what keeps -/// a concurrent mutation out while they run. Only the final `DROP ROLE` is on `tx`, so it commits -/// or rolls back with the catalog write that forgets the role. Those passes commit as they go, so -/// callers MUST have disabled the role in an earlier committed transaction: a failure part-way -/// then leaves a disabled role to retry, not an enabled one already stripped in some databases. -pub async fn drop_instance_role( +/// a concurrent mutation out while they run. On Windmill's own cluster only the final `DROP ROLE` +/// is on `tx`, so it commits or rolls back with the catalog write that forgets the role; on the +/// external cluster it runs there, and tolerates a role already gone so a retry after a failed +/// commit can finish. The passes commit as they go, so callers MUST have disabled the role in an +/// earlier committed transaction: a failure part-way then leaves a disabled role to retry, not an +/// enabled one already stripped in some databases. +pub async fn drop_datatable_role( db: &DB, tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + cluster: DatatableRoleCluster, name: &str, ) -> Result<()> { - crate::datatable_roles_oss::drop_instance_role(db, tx, name).await + crate::datatable_roles_oss::drop_datatable_role(db, tx, cluster, name).await } #[cfg(test)] diff --git a/backend/windmill-common/src/datatable_roles_oss.rs b/backend/windmill-common/src/datatable_roles_oss.rs index 2a3f9dda48..a97a936827 100644 --- a/backend/windmill-common/src/datatable_roles_oss.rs +++ b/backend/windmill-common/src/datatable_roles_oss.rs @@ -24,12 +24,12 @@ pub fn datatable_roles_unavailable() -> Error { #[cfg(all(feature = "private", feature = "enterprise"))] pub(crate) use crate::datatable_roles_ee::{ can_use_datatable_role, can_use_datatable_role_in_governing_workspace, converge_connect_grants, - converge_connect_grants_with, create_instance_role, delete_role_catalog_entry, - drop_instance_role, ensure_can_use_datatable_role, ensure_datatable_admin_access, + converge_connect_grants_with, create_datatable_role, delete_role_catalog_entry, + drop_datatable_role, ensure_can_use_datatable_role, ensure_datatable_admin_access, ensure_instance_db_grant_options_unchecked, forget_datatable_role_everywhere, insert_role_catalog_entry, read_role_catalog, read_role_catalog_tx, - registered_instance_databases, rename_instance_role, resolve_datatable_role_connection, - set_instance_role_login, update_role_catalog_entry, + registered_instance_databases, rename_datatable_role, resolve_datatable_role_connection, + role_cluster, set_datatable_role_login, update_role_catalog_entry, }; #[cfg(not(all(feature = "private", feature = "enterprise")))] @@ -39,7 +39,7 @@ pub(crate) use ce::*; mod ce { use super::datatable_roles_unavailable as unavailable; use crate::{ - datatable_roles::{DatatableRoleCatalog, InstanceDatatableRole}, + datatable_roles::{DatatableRoleCatalog, DatatableRoleCluster, InstanceDatatableRole}, db::AuthedRef, error::Result, workspaces::{ @@ -50,17 +50,31 @@ mod ce { type Tx<'a> = sqlx::Transaction<'a, sqlx::Postgres>; - pub(crate) async fn read_role_catalog(_db: &DB) -> Result { + pub(crate) async fn read_role_catalog( + _db: &DB, + _cluster: DatatableRoleCluster, + ) -> Result { Err(unavailable()) } - pub(crate) async fn read_role_catalog_tx(_tx: &mut Tx<'_>) -> Result { + pub(crate) async fn read_role_catalog_tx( + _tx: &mut Tx<'_>, + _cluster: DatatableRoleCluster, + ) -> Result { + Err(unavailable()) + } + + pub(crate) async fn role_cluster( + _tx: &mut Tx<'_>, + _id: &str, + ) -> Result> { Err(unavailable()) } pub(crate) async fn insert_role_catalog_entry( _tx: &mut Tx<'_>, _id: &str, + _cluster: DatatableRoleCluster, _role: &InstanceDatatableRole, ) -> Result<()> { Err(unavailable()) @@ -78,43 +92,57 @@ mod ce { Err(unavailable()) } - pub(crate) async fn registered_instance_databases(_db: &DB) -> Result> { + pub(crate) async fn registered_instance_databases( + _db: &DB, + _cluster: DatatableRoleCluster, + ) -> Result> { Err(unavailable()) } - /// Nothing to converge: with no roles to admit, an instance database keeps the `CONNECT` - /// grants it was created with, `PUBLIC`'s included, as it did before roles existed. - pub(crate) async fn converge_connect_grants(_db: &DB, _dbname: &str) -> Result<()> { + /// Nothing to converge: with no roles to admit, a managed database keeps the `CONNECT` grants + /// it was created with, as it did before roles existed. + pub(crate) async fn converge_connect_grants( + _db: &DB, + _cluster: DatatableRoleCluster, + _dbname: &str, + ) -> Result<()> { Ok(()) } /// As [`converge_connect_grants`]. pub(crate) async fn converge_connect_grants_with( _db: &DB, + _cluster: DatatableRoleCluster, _dbname: &str, _catalog: &DatatableRoleCatalog, ) -> Result<()> { Ok(()) } - pub(crate) async fn create_instance_role( + pub(crate) async fn create_datatable_role( + _db: &DB, _tx: &mut Tx<'_>, + _cluster: DatatableRoleCluster, _name: &str, _password: &str, ) -> Result<()> { Err(unavailable()) } - pub(crate) async fn set_instance_role_login( + pub(crate) async fn set_datatable_role_login( + _db: &DB, _tx: &mut Tx<'_>, + _cluster: DatatableRoleCluster, _name: &str, _enabled: bool, ) -> Result<()> { Err(unavailable()) } - pub(crate) async fn rename_instance_role( + pub(crate) async fn rename_datatable_role( + _db: &DB, _tx: &mut Tx<'_>, + _cluster: DatatableRoleCluster, _from: &str, _to: &str, _password: &str, @@ -122,12 +150,18 @@ mod ce { Err(unavailable()) } - pub(crate) async fn drop_instance_role(_db: &DB, _tx: &mut Tx<'_>, _name: &str) -> Result<()> { + pub(crate) async fn drop_datatable_role( + _db: &DB, + _tx: &mut Tx<'_>, + _cluster: DatatableRoleCluster, + _name: &str, + ) -> Result<()> { Err(unavailable()) } pub(crate) async fn ensure_instance_db_grant_options_unchecked( _db: &DB, + _cluster: DatatableRoleCluster, _dbname: &str, ) -> Result<()> { Err(unavailable()) diff --git a/backend/windmill-common/src/external_instance_pg.rs b/backend/windmill-common/src/external_instance_pg.rs new file mode 100644 index 0000000000..c0b6203e40 --- /dev/null +++ b/backend/windmill-common/src/external_instance_pg.rs @@ -0,0 +1,456 @@ +/* + * Author: Ruben Fiszel + * Copyright: Windmill Labs, Inc 2022 + * This file and its contents are licensed under the AGPLv3 License. + * Please see the included NOTICE for copyright information and + * LICENSE-AGPL for a copy of the license. + */ + +//! The external Postgres cluster behind `external_instance` data tables and Ducklake catalogs. +//! +//! Windmill administers that cluster itself, logged in as the user in +//! [`EXTERNAL_INSTANCE_PG_SETTING`]. It creates `custom_instance_user` and +//! `custom_instance_replication_user` there, with passwords it generates and keeps in +//! [`EXTERNAL_INSTANCE_PG_STATE_SETTING`]. They share their names with the roles on Windmill's own +//! cluster, but they are different roles with different passwords. +//! +//! The cluster may hold data Windmill did not create. Two Windmill instances sharing one is not +//! supported: each would keep resetting the passwords the other depends on. + +use std::collections::{BTreeMap, BTreeSet}; + +use serde::{Deserialize, Serialize}; + +use crate::{ + error::{Error, Result}, + global_settings::{EXTERNAL_INSTANCE_PG_SETTING, EXTERNAL_INSTANCE_PG_STATE_SETTING}, + instance_config::{CustomInstanceDb, ExternalInstancePg}, + DB, +}; + +/// What Windmill keeps about the external cluster. Server-managed and hidden: never part of the +/// instance config, never readable by an agent worker. No `Debug`: it carries live passwords. +#[derive(Serialize, Deserialize, Clone, Default)] +pub struct ExternalInstancePgState { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub user_pwd: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub replication_pwd: Option, + /// The databases Windmill created on the cluster. It only ever drops one of these. + #[serde(default)] + pub databases: BTreeMap, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub last_setup: Option, + /// The cluster ([`external_instance_pg_address`]) the last successful setup converged. Databases + /// are only created on a cluster setup succeeded on: the passwords above exist as soon as setup + /// first runs, whether or not the cluster accepted them. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub set_up_for: Option, +} + +/// What identifies the cluster a configuration points at. Other fields (admin login, sslmode) can +/// change without it becoming another cluster. +pub fn external_instance_pg_address(config: &ExternalInstancePg) -> String { + format!( + "{}:{}", + config.host.trim().to_lowercase(), + config.port.unwrap_or(5432) + ) +} + +#[derive(Serialize, Deserialize, Clone, Debug)] +pub struct ExternalInstancePgSetupReport { + /// No step failed. Warnings leave it true. + pub success: bool, + pub finished_at: chrono::DateTime, + pub steps: Vec, +} + +#[derive(Serialize, Deserialize, Clone, Debug)] +pub struct ExternalInstancePgSetupStep { + pub name: String, + pub status: SetupStepStatus, + pub message: String, +} + +#[derive(Serialize, Deserialize, Clone, Copy, Debug, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +pub enum SetupStepStatus { + Ok, + Warning, + Error, +} + +/// The status the settings page shows without running anything. +#[derive(Serialize, Debug)] +pub struct ExternalInstancePgStatus { + pub configured: bool, + pub database_count: usize, + #[serde(skip_serializing_if = "Option::is_none")] + pub last_setup: Option, +} + +/// Authorization: returns the cluster's admin password and checks nothing. Callers MUST be +/// superadmin or an internal server path. +pub(crate) async fn read_external_instance_pg_config<'c>( + executor: impl sqlx::PgExecutor<'c>, +) -> Result> { + let value = sqlx::query_scalar!( + "SELECT value FROM global_settings WHERE name = $1", + EXTERNAL_INSTANCE_PG_SETTING + ) + .fetch_optional(executor) + .await?; + value + .map(|v| { + serde_json::from_value(v).map_err(|e| { + Error::internal_err(format!("reading {EXTERNAL_INSTANCE_PG_SETTING}: {e}")) + }) + }) + .transpose() +} + +/// Authorization: returns the passwords Windmill generated on the cluster and checks nothing. +/// Callers MUST be superadmin or an internal server path. +pub(crate) async fn read_external_instance_pg_state<'c>( + executor: impl sqlx::PgExecutor<'c>, +) -> Result { + let value = sqlx::query_scalar!( + "SELECT value FROM global_settings WHERE name = $1", + EXTERNAL_INSTANCE_PG_STATE_SETTING + ) + .fetch_optional(executor) + .await?; + match value { + None => Ok(ExternalInstancePgState::default()), + Some(v) => serde_json::from_value(v).map_err(|e| { + Error::internal_err(format!("reading {EXTERNAL_INSTANCE_PG_STATE_SETTING}: {e}")) + }), + } +} + +/// Authorization: reads the hidden cluster state and checks nothing. Callers MUST be superadmin. +pub async fn external_instance_pg_status(db: &DB) -> Result { + let configured = read_external_instance_pg_config(db).await?.is_some(); + let state = read_external_instance_pg_state(db).await?; + Ok(ExternalInstancePgStatus { + configured, + database_count: state.databases.len(), + last_setup: state.last_setup, + }) +} + +/// The databases Windmill created on the external cluster, without the passwords kept beside them. +/// +/// Authorization: names every database across all workspaces, and the workspace each fork copy is +/// reserved for, and checks nothing. Callers MUST be superadmin or an internal authorization or +/// lifecycle path that does not return the names to a workspace caller. +pub async fn external_instance_databases(db: &DB) -> Result> { + Ok(read_external_instance_pg_state(db).await?.databases) +} + +/// The workspaces whose data tables or Ducklake catalogs name each database on the external cluster, +/// and the forks whose Ducklake metadata schemas there are still waiting to be dropped: those rows +/// outlive a settings change, and cleanup cannot drop a schema in a database that is gone. A row +/// whose schema is already dropped only waits on object storage, which needs no database. +/// +/// Authorization: reads every workspace's settings and checks nothing. Callers MUST be superadmin +/// or an internal lifecycle path. +pub async fn external_instance_database_usages<'c>( + db: impl sqlx::PgExecutor<'c>, +) -> Result>> { + let rows = sqlx::query_as::<_, (String, String)>( + "SELECT ws.workspace_id, entry->'database'->>'resource_path' + FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each( + CASE WHEN jsonb_typeof(ws.datatable->'datatables') = 'object' + THEN ws.datatable->'datatables' + ELSE '{}'::jsonb END + ) AS dt(k, entry) + WHERE entry->'database'->>'resource_type' = 'external_instance' + AND entry->'database'->>'resource_path' IS NOT NULL + UNION ALL + SELECT ws.workspace_id, entry->'catalog'->>'resource_path' + FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each( + CASE WHEN jsonb_typeof(ws.ducklake->'ducklakes') = 'object' + THEN ws.ducklake->'ducklakes' + ELSE '{}'::jsonb END + ) AS dl(k, entry) + WHERE entry->'catalog'->>'resource_type' = 'external_instance' + AND entry->'catalog'->>'resource_path' IS NOT NULL + UNION ALL + SELECT workspace_id, substring(catalog FROM length('external_instance:') + 1) + FROM fork_ducklake_namespace + WHERE catalog LIKE 'external\\_instance:%' AND NOT schema_dropped", + ) + .fetch_all(db) + .await?; + let mut usages: BTreeMap> = BTreeMap::new(); + for (workspace_id, dbname) in rows { + usages.entry(dbname).or_default().insert(workspace_id); + } + Ok(usages) +} + +/// Refuse to unset the cluster while Windmill still has databases or data table roles on it, or a +/// workspace still points at one: every data table there would stop resolving, and every role +/// would be a login nothing can drop any more. Allowed on every edition, so a +/// downgraded instance can still clear a setting it no longer uses. +async fn ensure_external_instance_pg_removable(conn: &mut sqlx::PgConnection) -> Result<()> { + ensure_external_instance_pg_unused(conn, &format!("removing {EXTERNAL_INSTANCE_PG_SETTING}")).await +} + +/// Refuse while Windmill has databases or data table roles on the cluster, or a workspace points +/// at one of its databases. `before` finishes the sentence saying what to do first. +async fn ensure_external_instance_pg_unused( + conn: &mut sqlx::PgConnection, + before: &str, +) -> Result<()> { + let state = read_external_instance_pg_state(&mut *conn).await?; + let usages = external_instance_database_usages(&mut *conn).await?; + let roles = sqlx::query_scalar::<_, String>( + "SELECT name FROM datatable_role WHERE cluster = 'external_instance' ORDER BY name", + ) + .fetch_all(&mut *conn) + .await?; + if state.databases.is_empty() && usages.is_empty() && roles.is_empty() { + return Ok(()); + } + let mut held = vec![]; + if !(state.databases.is_empty() && usages.is_empty()) { + let names = state + .databases + .keys() + .chain(usages.keys()) + .collect::>() + .into_iter() + .cloned() + .collect::>() + .join(", "); + held.push(format!("databases in use ({names})")); + } + if !roles.is_empty() { + held.push(format!("data table roles ({})", roles.join(", "))); + } + Err(Error::BadRequest(format!( + "The external instance cluster still holds {}. Drop them and repoint the data tables and \ + Ducklake catalogs using them before {before}.", + held.join(" and ") + ))) +} + +/// Refuse a workspace setting that newly names an `external_instance` database on an edition +/// without them. +pub fn ensure_external_instance_available() -> Result<()> { + crate::external_instance_pg_oss::ensure_external_instance_available() +} + +/// The connection an `external_instance` database resolves to: `custom_instance_user`, or the +/// replication user, on the external cluster. +/// +/// Authorization: returns live credentials and checks nothing. Callers MUST have authorized access +/// to the data table that names `dbname`. +pub async fn external_instance_connection_unchecked( + db: &DB, + dbname: &str, + replication: bool, +) -> Result { + crate::external_instance_pg_oss::external_instance_connection_unchecked(db, dbname, replication) + .await +} + +/// Create `dbname` on the external cluster and register it. Refuses a name already taken there, +/// whoever took it. +/// +/// Runs on `tx`, which it takes [`lock_external_instance_pg_state`] on: the registration lands when +/// the caller commits. Take that lock before any database governance lock, as saves do. +/// +/// Authorization: checks nothing. Callers MUST be superadmin, or be cloning a data table they may +/// fork into a `wm_fork_` database. +pub async fn create_external_instance_database_unchecked( + db: &DB, + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + dbname: &str, + tag: &str, + for_workspace: Option<&str>, +) -> Result<()> { + crate::external_instance_pg_oss::create_external_instance_database_unchecked( + db, + tx, + dbname, + tag, + for_workspace, + ) + .await +} + +/// Drop `dbname` from the external cluster: only a database Windmill registered creating, and still +/// carries the mark it set there. Refused while anything uses it +/// ([`crate::workspaces::managed_database_uses`]), except the `exempt` data table entry: the fork +/// copy being cleaned up. +/// +/// Runs on `tx`, like [`create_external_instance_database_unchecked`]: the database is gone at +/// once, its registry entry when the caller commits. +/// +/// Authorization: checks nothing. Callers MUST be superadmin, or be deleting the fork that owns +/// this `wm_fork_` database. +pub async fn drop_external_instance_database_unchecked( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + dbname: &str, + exempt: Option<(&str, &str)>, +) -> Result<()> { + crate::external_instance_pg_oss::drop_external_instance_database_unchecked(tx, dbname, exempt) + .await +} + +/// Serializes everything that changes which databases exist on the external cluster, or which data +/// tables name them: setup, creates, drops, and data table saves. Held until `tx` ends. +pub async fn lock_external_instance_pg_state( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, +) -> Result<()> { + sqlx::query("SELECT pg_advisory_xact_lock(hashtext($1))") + .bind(EXTERNAL_INSTANCE_PG_STATE_SETTING) + .execute(&mut **tx) + .await?; + Ok(()) +} + +/// Refuse a data table naming `dbname` unless Windmill created it on the external cluster. Takes +/// the lock drops take, so none can remove the database before `tx`, which saves the data table, +/// commits. +/// +/// Authorization: its refusal says whether Windmill created a database of that name, which is +/// instance-wide knowledge. Callers MUST have authorized the caller as superadmin first. +pub async fn ensure_external_instance_database_registered( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + dbname: &str, +) -> Result<()> { + lock_external_instance_pg_state(tx).await?; + if read_external_instance_pg_state(&mut **tx) + .await? + .databases + .contains_key(dbname) + { + return Ok(()); + } + Err(Error::BadRequest(format!( + "Windmill did not create a database named '{dbname}' on the external instance cluster. \ + Create it from the instance settings first." + ))) +} + +/// Write [`EXTERNAL_INSTANCE_PG_SETTING`]: `None`, null or an empty string unsets it. Every writer +/// of global settings goes through this for that key — the per-key and bulk endpoints as well as +/// the declarative sync — instead of writing the row itself. +/// +/// The checks and the write share one transaction holding [`lock_external_instance_pg_state`]. A +/// check taken outside it could pass while a database create still reads the old cluster, which +/// would then register a database there after the setting names another one. +/// +/// Authorization: checks nothing. Callers MUST be superadmin, or the declarative instance config +/// sync, which applies what the operator deployed. +pub async fn write_external_instance_pg_setting( + db: &DB, + value: Option<&serde_json::Value>, +) -> Result<()> { + let value = match value { + None | Some(serde_json::Value::Null) => None, + Some(serde_json::Value::String(s)) if s.trim().is_empty() => None, + Some(value) => Some(value), + }; + let mut tx = db.begin().await?; + lock_external_instance_pg_state(&mut tx).await?; + // Every check runs on this transaction's own connection: it holds the advisory lock, and + // taking a second connection from the pool while other writers queue on that lock is how a + // small pool deadlocks. + match value { + None => { + ensure_external_instance_pg_removable(&mut tx).await?; + sqlx::query("DELETE FROM global_settings WHERE name = $1") + .bind(EXTERNAL_INSTANCE_PG_SETTING) + .execute(&mut *tx) + .await?; + } + Some(value) => { + crate::external_instance_pg_oss::validate_external_instance_pg_setting(value)?; + ensure_external_instance_pg_not_repointed(&mut tx, value).await?; + sqlx::query( + "INSERT INTO global_settings (name, value) VALUES ($1, $2) + ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value, updated_at = now()", + ) + .bind(EXTERNAL_INSTANCE_PG_SETTING) + .bind(value) + .execute(&mut *tx) + .await?; + } + } + tx.commit().await?; + tracing::info!( + "{} global setting {EXTERNAL_INSTANCE_PG_SETTING}", + if value.is_some() { "Set" } else { "Unset" } + ); + Ok(()) +} + +/// [`write_external_instance_pg_setting`] for a settings diff: writes the key if the diff touches +/// it, and takes it out of the diff so the generic apply does not write it again. +/// +/// Authorization: checks nothing. Callers MUST be superadmin, or the declarative instance config +/// sync, which applies what the operator deployed. +pub async fn write_external_instance_pg_from_diff( + db: &DB, + diff: &mut crate::instance_config::SettingsDiff, +) -> Result<()> { + if let Some(value) = diff.upserts.remove(EXTERNAL_INSTANCE_PG_SETTING) { + write_external_instance_pg_setting(db, Some(&value)).await?; + } + if let Some(i) = diff + .deletes + .iter() + .position(|k| k == EXTERNAL_INSTANCE_PG_SETTING) + { + diff.deletes.remove(i); + write_external_instance_pg_setting(db, None).await?; + } + Ok(()) +} + +/// Refuse pointing the setting at another host or port while databases or data table roles live on +/// the current one. Data tables name databases, and the role catalog names logins, not clusters, so +/// both would silently resolve to whatever the new cluster holds under the same names. Other fields +/// (admin login, sslmode) may change freely. +async fn ensure_external_instance_pg_not_repointed( + conn: &mut sqlx::PgConnection, + value: &serde_json::Value, +) -> Result<()> { + let Some(current) = read_external_instance_pg_config(&mut *conn).await? else { + return Ok(()); + }; + let Ok(desired) = serde_json::from_value::(value.clone()) else { + return Ok(()); + }; + if external_instance_pg_address(¤t) == external_instance_pg_address(&desired) { + return Ok(()); + } + ensure_external_instance_pg_unused( + conn, + &format!("pointing {EXTERNAL_INSTANCE_PG_SETTING} at another cluster"), + ) + .await +} + +/// Converge the external cluster on the configured login: check what it can do, create or update +/// Windmill's two roles with the stored passwords, and report anything that would get in the way. +/// With `rotate_passwords`, generate new passwords first. Safe to run again; running it again is +/// how a failed rotation is repaired. +/// +/// Authorization: administers the external cluster with its admin credentials and checks nothing. +/// Callers MUST be superadmin. +pub async fn setup_external_instance_pg_unchecked( + db: &DB, + rotate_passwords: bool, +) -> Result { + crate::external_instance_pg_oss::setup_external_instance_pg_unchecked(db, rotate_passwords) + .await +} diff --git a/backend/windmill-common/src/external_instance_pg_oss.rs b/backend/windmill-common/src/external_instance_pg_oss.rs new file mode 100644 index 0000000000..93b6fc63dd --- /dev/null +++ b/backend/windmill-common/src/external_instance_pg_oss.rs @@ -0,0 +1,82 @@ +/* + * Author: Ruben Fiszel + * Copyright: Windmill Labs, Inc 2022 + * This file and its contents are licensed under the AGPLv3 License. + * Please see the included NOTICE for copyright information and + * LICENSE-AGPL for a copy of the license. + */ + +//! Where the external instance cluster comes from: the enterprise implementation, or a refusal. +//! `private` alone is not that edition: community builds carry it. + +use crate::error::Error; + +pub fn external_instance_pg_unavailable() -> Error { + Error::BadRequest( + "External instance databases are a Windmill Enterprise Edition feature".to_string(), + ) +} + +#[cfg(all(feature = "private", feature = "enterprise"))] +pub(crate) use crate::external_instance_pg_ee::{ + create_external_instance_database_unchecked, drop_external_instance_database_unchecked, + external_instance_connection_unchecked, setup_external_instance_pg_unchecked, + validate_external_instance_pg_setting, +}; + +#[cfg(all(feature = "private", feature = "enterprise"))] +pub(crate) fn ensure_external_instance_available() -> crate::error::Result<()> { + Ok(()) +} + +#[cfg(not(all(feature = "private", feature = "enterprise")))] +pub(crate) use ce::*; + +#[cfg(not(all(feature = "private", feature = "enterprise")))] +mod ce { + use super::external_instance_pg_unavailable as unavailable; + use crate::{ + error::Result, external_instance_pg::ExternalInstancePgSetupReport, PgDatabase, DB, + }; + + pub(crate) fn validate_external_instance_pg_setting(_value: &serde_json::Value) -> Result<()> { + Err(unavailable()) + } + + pub(crate) fn ensure_external_instance_available() -> Result<()> { + Err(unavailable()) + } + + pub(crate) async fn setup_external_instance_pg_unchecked( + _db: &DB, + _rotate_passwords: bool, + ) -> Result { + Err(unavailable()) + } + + pub(crate) async fn external_instance_connection_unchecked( + _db: &DB, + _dbname: &str, + _replication: bool, + ) -> Result { + Err(unavailable()) + } + + pub(crate) async fn create_external_instance_database_unchecked( + _db: &DB, + _tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + _dbname: &str, + _tag: &str, + _for_workspace: Option<&str>, + ) -> Result<()> { + Err(unavailable()) + } + + pub(crate) async fn drop_external_instance_database_unchecked( + _tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + _dbname: &str, + _exempt: Option<(&str, &str)>, + ) -> Result<()> { + Err(unavailable()) + } +} diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index 7306c5c59a..9137dc364e 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/backend/windmill-common/src/global_settings.rs @@ -57,6 +57,11 @@ pub const SAML_METADATA_SETTING: &str = "saml_metadata"; pub const SMTP_SETTING: &str = "smtp_settings"; pub const TEAMS_SETTING: &str = "teams"; pub const INDEXER_SETTING: &str = "indexer_settings"; +pub const EXTERNAL_INSTANCE_PG_SETTING: &str = "external_instance_pg"; +/// Turns off Windmill's own Postgres as a data table and Ducklake substrate. Absent means on, +/// which is what every instance that predates the setting expects. +pub const INSTANCE_PG_DISABLED_SETTING: &str = "instance_pg_disabled"; +pub const EXTERNAL_INSTANCE_PG_STATE_SETTING: &str = "external_instance_pg_state"; pub const TIMEOUT_WAIT_RESULT_SETTING: &str = "timeout_wait_result"; pub const UNIQUE_ID_SETTING: &str = "uid"; @@ -448,6 +453,9 @@ pub const AGENT_WORKER_BLOCKED_SETTINGS: &[&str] = &[ // resolve datatable connections through the dedicated datatable endpoints, never these. "custom_instance_pg_databases", "custom_instance_replication_pwd", + // The external cluster's admin login, and the passwords Windmill generated on it. + EXTERNAL_INSTANCE_PG_SETTING, + EXTERNAL_INSTANCE_PG_STATE_SETTING, ]; /// Whether an agent worker may read the given global setting over HTTP. diff --git a/backend/windmill-common/src/instance_config.rs b/backend/windmill-common/src/instance_config.rs index 264916d522..fd790547b6 100644 --- a/backend/windmill-common/src/instance_config.rs +++ b/backend/windmill-common/src/instance_config.rs @@ -354,6 +354,8 @@ pub struct GlobalSettings { pub ducklake_settings: Option, #[serde(skip_serializing_if = "Option::is_none")] pub custom_instance_pg_databases: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub external_instance_pg: Option, // Opaque settings (EE-private structs or no clear schema) #[serde(skip_serializing_if = "Option::is_none")] @@ -815,6 +817,9 @@ pub struct CustomInstanceDb { pub error: Option, #[serde(skip_serializing_if = "Option::is_none")] pub tag: Option, + /// The workspace a member created this fork copy for. Absent when a superadmin created it. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub workspace_id: Option, } /// Setup log entries for a custom instance database. @@ -841,6 +846,36 @@ pub struct CustomInstanceDbLogs { pub user_connect: String, } +// --------------------------------------------------------------------------- +// External instance PG cluster +// --------------------------------------------------------------------------- + +/// The external Postgres cluster Windmill manages for `external_instance` data tables and Ducklake +/// catalogs. `user` logs in as the cluster's administrator: it needs `CREATEDB` and `CREATEROLE`. +/// `dbname` is only where that login connects to run cluster-wide statements. +/// +/// Every field defaults rather than being required: this deserializes as part of the whole +/// instance config, and one malformed row must not make every other setting unreadable. The +/// write path and every use reject an incomplete value instead. +#[derive(Deserialize, Serialize, Clone, Debug, Default)] +#[cfg_attr(feature = "instance_config_schema", derive(schemars::JsonSchema))] +pub struct ExternalInstancePg { + #[serde(default)] + pub host: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub port: Option, + #[serde(default)] + pub user: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub password: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub dbname: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub sslmode: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub root_certificate_pem: Option, +} + // --------------------------------------------------------------------------- // Autoscaling (worker config) // --------------------------------------------------------------------------- @@ -977,6 +1012,7 @@ pub const PROTECTED_SETTINGS: &[&str] = &[ "ducklake_settings", "custom_instance_pg_databases", "custom_instance_replication_pwd", + "external_instance_pg_state", "uid", "rsa_keys", "jwt_secret", @@ -1002,6 +1038,8 @@ pub const HIDDEN_SETTINGS: &[&str] = &[ // Server-only (written by setup/refresh via direct SQL), never operator-authored — // hidden so the config machinery can't read, rewrite, or drop it. "custom_instance_replication_pwd", + // Same for the passwords and database registry Windmill keeps for the external cluster. + "external_instance_pg_state", ]; /// Top-level settings whose entire value is sensitive and must be fully redacted in logs. @@ -1013,6 +1051,7 @@ const SENSITIVE_SETTINGS: &[&str] = &[ "license_key", "ducklake_user_pg_pwd", "custom_instance_replication_pwd", + "external_instance_pg_state", "pip_index_url", "pip_extra_index_url", "npm_config_registry", @@ -1038,6 +1077,7 @@ const NESTED_SENSITIVE_FIELDS: &[(&str, &[&str])] = &[ &["secret_key", "serviceAccountKey", "accessKey"], ), ("custom_instance_pg_databases", &["user_pwd"]), + ("external_instance_pg", &["password"]), ("github_enterprise_app", &["private_key"]), ]; @@ -1379,7 +1419,8 @@ pub async fn sync_global_settings_declarative( crate::global_settings::parse_max_token_expiration_days(desired.get(max_expiration_key)) .map_err(|e| anyhow::anyhow!("{max_expiration_key}: {e}"))?; - let diff = diff_global_settings(current, desired, ApplyMode::Replace); + let mut diff = diff_global_settings(current, desired, ApplyMode::Replace); + crate::external_instance_pg::write_external_instance_pg_from_diff(db, &mut diff).await?; apply_settings_diff(db, &diff).await?; Ok(()) @@ -1512,6 +1553,10 @@ pub fn resolve_env_refs(settings: &mut GlobalSettings) -> Result<(), String> { resolve_env_option(&mut pg.user_pwd)?; } + if let Some(pg) = &mut settings.external_instance_pg { + resolve_env_option(&mut pg.password)?; + } + Ok(()) } @@ -2495,39 +2540,33 @@ mod tests { } #[test] - fn custom_instance_replication_pwd_is_isolated_from_config() { - // The replication-role password is server-only: written by setup/refresh via direct - // SQL, never operator-authored. It must stay out of the declarative config surface - // (hidden on read) and be undeletable, so config sync can't read, rewrite, or drop it. - assert!(HIDDEN_SETTINGS.contains(&"custom_instance_replication_pwd")); - assert!(PROTECTED_SETTINGS.contains(&"custom_instance_replication_pwd")); - assert!(SENSITIVE_SETTINGS.contains(&"custom_instance_replication_pwd")); + fn server_generated_db_passwords_are_isolated_from_config() { + // These hold passwords the server generates: written by setup/refresh via direct SQL, + // never operator-authored. They must stay out of the declarative config surface + // (hidden on read) and be undeletable, so config sync can't read, rewrite, or drop them. + for key in [ + "custom_instance_replication_pwd", + "external_instance_pg_state", + ] { + assert!(HIDDEN_SETTINGS.contains(&key), "{key}"); + assert!(PROTECTED_SETTINGS.contains(&key), "{key}"); + assert!(SENSITIVE_SETTINGS.contains(&key), "{key}"); - // A stray desired value (e.g. flattened into `extra`) is ignored, not upserted. - let mut desired = BTreeMap::new(); - desired.insert( - "custom_instance_replication_pwd".to_string(), - serde_json::json!("attacker-set"), - ); - let diff = diff_global_settings(&BTreeMap::new(), &desired, ApplyMode::Merge); - assert!( - diff.upserts.is_empty(), - "hidden setting must not be upserted" - ); + // A stray desired value (e.g. flattened into `extra`) is ignored, not upserted. + let mut desired = BTreeMap::new(); + desired.insert(key.to_string(), serde_json::json!("attacker-set")); + let diff = diff_global_settings(&BTreeMap::new(), &desired, ApplyMode::Merge); + assert!(diff.upserts.is_empty(), "{key} must not be upserted"); - // A current value is never deleted by a Replace that omits it. - let mut current = BTreeMap::new(); - current.insert( - "custom_instance_replication_pwd".to_string(), - serde_json::json!("live"), - ); - let diff = diff_global_settings(¤t, &BTreeMap::new(), ApplyMode::Replace); - assert!( - !diff - .deletes - .contains(&"custom_instance_replication_pwd".to_string()), - "hidden setting must not be deleted" - ); + // A current value is never deleted by a Replace that omits it. + let mut current = BTreeMap::new(); + current.insert(key.to_string(), serde_json::json!("live")); + let diff = diff_global_settings(¤t, &BTreeMap::new(), ApplyMode::Replace); + assert!( + !diff.deletes.contains(&key.to_string()), + "{key} must not be deleted" + ); + } } #[test] diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index 47a7ea5b2a..0a1b0974d5 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -58,6 +58,10 @@ pub mod ee_oss; pub mod email_ee; pub mod email_oss; pub mod error; +pub mod external_instance_pg; +#[cfg(all(feature = "private", feature = "enterprise"))] +mod external_instance_pg_ee; +pub mod external_instance_pg_oss; pub mod external_ip; #[cfg(feature = "private")] pub mod feature_usage_ee; @@ -1115,7 +1119,13 @@ impl PgDatabase { if err_str.contains("password authentication failed for user") && err_str.contains("custom_instance_user") { - if let Some(db) = main_db { + // The external instance cluster has a `custom_instance_user` of its own, whose + // password setup manages. Rotating the local one would break every instance + // data table and fix nothing. + let local = PgDatabase::parse_uri(&get_database_url().await?.as_str().await)?; + let on_local_cluster = local.host == self.host + && local.port.unwrap_or(5432) == self.port.unwrap_or(5432); + if let Some(db) = main_db.filter(|_| on_local_cluster) { tracing::warn!( "custom_instance_user password auth failed, refreshing and retrying..." ); @@ -1665,13 +1675,41 @@ pub async fn instance_database_users( } /// Drop a custom instance database: validate, terminate connections, DROP DATABASE, remove from global_settings. +/// +/// Authorization: drops any instance database but Windmill's own and checks nothing. Callers MUST +/// be superadmin, or have established the caller may drop this one — a fork's owner cleaning up +/// its own copy that nothing else uses. pub async fn drop_custom_instance_database(db: &DB, dbname: &str) -> error::Result<()> { drop_custom_instance_database_on(&mut *db.acquire().await?, dbname).await } +/// [`drop_custom_instance_database`] leaving its registry entry, for a caller holding row locks in +/// a transaction: the registry write has to go through that transaction, as waiting on another +/// connection for a lock the transaction's own peers hold is a deadlock Postgres cannot see. Same +/// authorization contract. +pub async fn drop_custom_instance_database_keep_entry(db: &DB, dbname: &str) -> error::Result<()> { + drop_instance_database_keep_entry_on(&mut *db.acquire().await?, dbname).await +} + async fn drop_custom_instance_database_on( conn: &mut sqlx::PgConnection, dbname: &str, +) -> error::Result<()> { + let dbname = dbname.trim(); + drop_instance_database_keep_entry_on(&mut *conn, dbname).await?; + // Always remove from global_settings + sqlx::query!( + r#"UPDATE global_settings SET value = value #- ARRAY['databases', $1] WHERE name = 'custom_instance_pg_databases'"#, + dbname + ) + .execute(&mut *conn) + .await?; + Ok(()) +} + +async fn drop_instance_database_keep_entry_on( + conn: &mut sqlx::PgConnection, + dbname: &str, ) -> error::Result<()> { let dbname = dbname.trim(); validate_dbname(dbname)?; @@ -1718,14 +1756,6 @@ async fn drop_custom_instance_database_on( tracing::info!("Database '{}' does not exist, skipping drop", dbname); } - // Always remove from global_settings - sqlx::query!( - r#"UPDATE global_settings SET value = value #- ARRAY['databases', $1] WHERE name = 'custom_instance_pg_databases'"#, - dbname - ) - .execute(&mut *conn) - .await?; - Ok(()) } @@ -1750,26 +1780,30 @@ pub(crate) fn instance_db_grants(dbname: &str) -> String { ) } -/// Re-apply [`instance_db_grants`] to an instance database provisioned before data table roles -/// existed, whose grants carry no grant option. Connects as the instance's own Postgres user — -/// the database and `public` schema owner — since only it can hand out an option it holds. +/// Re-apply [`instance_db_grants`] to a managed database provisioned before data table roles +/// existed, whose grants carry no grant option. Connects as the cluster's administrator — the +/// database and `public` schema owner — since only it can hand out an option it holds. /// -/// Authorization: reaches an instance database with the server's own credentials and checks +/// Authorization: reaches a managed database with the server's own credentials and checks /// nothing. Callers MUST have authorized administration of `dbname` — superadmin, or an admin of /// the workspace governing a data table on it. pub async fn ensure_instance_db_grant_options_unchecked( db: &DB, + cluster: crate::datatable_roles::DatatableRoleCluster, dbname: &str, ) -> error::Result<()> { - crate::datatable_roles_oss::ensure_instance_db_grant_options_unchecked(db, dbname).await + crate::datatable_roles_oss::ensure_instance_db_grant_options_unchecked(db, cluster, dbname) + .await } /// Create a custom instance database: CREATE DATABASE, grant permissions, register in global_settings. -/// The `tag` is stored in global_settings metadata (e.g. "datatable" or "ducklake"). +/// The `tag` is stored in global_settings metadata (e.g. "datatable" or "ducklake"). `for_workspace` +/// is the workspace a member creates a fork copy for; see [`ensure_fork_database_available_to`]. pub async fn create_custom_instance_database( db: &DB, dbname: &str, tag: &str, + for_workspace: Option<&str>, ) -> error::Result<()> { let dbname = dbname.trim(); validate_dbname(dbname)?; @@ -1799,7 +1833,7 @@ pub async fn create_custom_instance_database( // Nothing names a database that failed past this point, and its name blocks the retry: drop it // rather than leave it behind. - if let Err(e) = finish_custom_instance_database(db, dbname, tag).await { + if let Err(e) = finish_custom_instance_database(db, dbname, tag, for_workspace).await { match drop_unused_instance_database(db, dbname).await { Ok(Cleanup::InUse(users)) => tracing::warn!( "Kept '{dbname}' after failing to set it up: workspaces {} use it", @@ -1820,7 +1854,13 @@ pub async fn create_custom_instance_database( // A data table role can only reach a database it may CONNECT to, and PUBLIC's default CONNECT // would otherwise let every role in regardless of what this instance defines. Best-effort: a // failure here leaves the database usable as `admin`, and the next role change repairs it. - if let Err(e) = crate::datatable_roles::converge_connect_grants(db, dbname).await { + if let Err(e) = crate::datatable_roles::converge_connect_grants( + db, + crate::datatable_roles::DatatableRoleCluster::Instance, + dbname, + ) + .await + { tracing::warn!("Could not set CONNECT grants on instance database '{dbname}': {e}"); } @@ -1829,7 +1869,12 @@ pub async fn create_custom_instance_database( } /// Grant `custom_instance_user` its privileges on a database just created, and register it. -async fn finish_custom_instance_database(db: &DB, dbname: &str, tag: &str) -> error::Result<()> { +async fn finish_custom_instance_database( + db: &DB, + dbname: &str, + tag: &str, + for_workspace: Option<&str>, +) -> error::Result<()> { // Grant permissions to custom_instance_user let wmill_pg_creds = PgDatabase::parse_uri(&get_database_url().await?.as_str().await)?; let new_pg_creds = PgDatabase { dbname: dbname.to_string(), ..wmill_pg_creds }; @@ -1856,7 +1901,8 @@ async fn finish_custom_instance_database(db: &DB, dbname: &str, tag: &str) -> er }, "success": true, "error": null, - "tag": tag + "tag": tag, + "workspace_id": for_workspace, }); sqlx::query!( r#"UPDATE global_settings SET value = jsonb_set(value, '{databases}', (COALESCE(value->'databases', '{}'::jsonb) || to_jsonb($1::json))) WHERE name = 'custom_instance_pg_databases'"#, @@ -1864,6 +1910,75 @@ async fn finish_custom_instance_database(db: &DB, dbname: &str, tag: &str) -> er ) .execute(db) .await?; + + Ok(()) +} + +/// The system's CA bundle file, for libpq clients that cannot take `sslrootcert=system`: that value +/// needs libpq 16, and verify-full only. +pub fn system_ca_bundle() -> Option { + std::env::var_os("SSL_CERT_FILE") + .map(std::path::PathBuf::from) + .into_iter() + .chain( + [ + "/etc/ssl/certs/ca-certificates.crt", + "/etc/pki/tls/certs/ca-bundle.crt", + "/etc/ssl/cert.pem", + "/etc/ssl/ca-bundle.pem", + ] + .map(std::path::PathBuf::from), + ) + .find(|path| path.is_file()) +} + +/// Refuse a workspace member writing a fork copy into, or pointing a fork at, the managed database +/// `dbname` of `kind`, unless `w_id` created it for that ([`create_custom_instance_database`], or +/// its external instance counterpart) and nothing uses it yet. The `wm_fork_` prefix is no +/// authorization: every database of a cluster answers to the same `custom_instance_user`, so a name +/// is all it takes to reach another workspace's copy. +/// +/// Runs on `conn`: its callers hold a transaction with the fork lock while they check, and a +/// second connection taken from the pool under it is how a small pool deadlocks. +/// +/// Authorization: reads the global registries and every workspace's settings, and names other +/// workspaces in its refusal. Callers MUST have authorized `w_id` for the caller first — a member +/// of it forking or importing there — and MUST NOT call it on a workspace the caller is not in. +pub async fn ensure_fork_database_available_to( + conn: &mut sqlx::PgConnection, + kind: workspaces::DataTableCatalogResourceType, + dbname: &str, + w_id: &str, +) -> error::Result<()> { + let created_for = match kind { + workspaces::DataTableCatalogResourceType::ExternalInstance => { + external_instance_pg::read_external_instance_pg_state(&mut *conn) + .await? + .databases + .remove(dbname) + .and_then(|entry| entry.workspace_id) + } + _ => sqlx::query_scalar::<_, Option>( + "SELECT value->'databases'->$1->>'workspace_id' FROM global_settings + WHERE name = 'custom_instance_pg_databases'", + ) + .bind(dbname) + .fetch_optional(&mut *conn) + .await? + .flatten(), + }; + if created_for.as_deref() != Some(w_id) { + return Err(Error::BadRequest(format!( + "Database '{dbname}' was not created for a fork of workspace '{w_id}'" + ))); + } + let uses = workspaces::managed_database_uses(conn, kind, dbname, None).await?; + if !uses.is_empty() { + return Err(Error::BadRequest(format!( + "Database '{dbname}' is already in use: {}", + uses.join(", ") + ))); + } Ok(()) } diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index b285332b7d..724743dce9 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1542,6 +1542,47 @@ pub enum DataTableCatalogResourceType { #[strum(serialize = "postgres")] Postgresql, Instance, + /// On the external instance cluster ([`crate::external_instance_pg`]). Enterprise Edition. + #[serde(rename = "external_instance")] + #[strum(serialize = "external_instance")] + ExternalInstance, +} + +impl DataTableCatalogResourceType { + /// A database Windmill created and administers, on its own cluster or the external one, as + /// opposed to one a user brought as a resource. + pub fn is_windmill_managed(self) -> bool { + matches!(self, Self::Instance | Self::ExternalInstance) + } +} + +/// Refuse a new use of Windmill's own Postgres as a data table or Ducklake substrate where an +/// operator turned it off, and on the managed cloud, which never had it. Entries already on it +/// keep resolving: this gates what a save may newly name, not what runs. +pub async fn ensure_instance_pg_available<'c>(executor: impl sqlx::PgExecutor<'c>) -> Result<()> { + if *crate::worker::CLOUD_HOSTED { + return Err(Error::BadRequest( + "Windmill's own database cannot back a data table or Ducklake catalog on Windmill Cloud" + .to_string(), + )); + } + let disabled = sqlx::query_scalar::<_, Option>( + "SELECT value FROM global_settings WHERE name = $1", + ) + .bind(crate::global_settings::INSTANCE_PG_DISABLED_SETTING) + .fetch_optional(executor) + .await? + .flatten() + .is_some_and(|v| v.as_bool().unwrap_or(false)); + if disabled { + return Err(Error::BadRequest( + "Windmill's own database is disabled as a data table and Ducklake substrate on this \ + instance. Use the external instance cluster, or turn it back on in the instance \ + settings." + .to_string(), + )); + } + Ok(()) } /// Build a self-teaching error for an unresolved `datatable://` reference. @@ -1621,14 +1662,82 @@ pub struct GoverningDatatable { pub governor: Option, } +/// Everything still using the Windmill-managed database `dbname`, one description per use: data +/// table entries naming it, fork entries pointing at those, Ducklake catalogs on it, and fork +/// Ducklake metadata schemas there that cleanup has not dropped yet. `exempt` is the one data table +/// entry, `(workspace_id, name)`, the caller is about to stop using it through; pointers at that +/// entry still count, since dropping the database would leave them resolving to nothing. +/// +/// Authorization: reads every workspace's settings and checks nothing. Callers MUST only turn the +/// answer into a refusal for someone allowed to administer `dbname`. +pub async fn managed_database_uses( + conn: &mut sqlx::PgConnection, + kind: DataTableCatalogResourceType, + dbname: &str, + exempt: Option<(&str, &str)>, +) -> Result> { + let (exempt_workspace, exempt_name) = exempt.unzip(); + Ok(sqlx::query_scalar::<_, String>( + "WITH entries AS ( + SELECT ws.workspace_id::text AS workspace_id, dt.key AS name, dt.value + FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each( + CASE WHEN jsonb_typeof(ws.datatable->'datatables') = 'object' + THEN ws.datatable->'datatables' ELSE '{}'::jsonb END) dt + ), naming AS ( + SELECT workspace_id, name FROM entries + WHERE value->'database'->>'resource_type' = $1 + AND value->'database'->>'resource_path' = $2 + ) + SELECT format('data table ''%s'' in workspace ''%s''', name, workspace_id) FROM naming + WHERE $3::text IS NULL OR NOT (workspace_id = $3 AND name = $4) + UNION ALL + SELECT format('data table ''%s'' in workspace ''%s'', which points at the one in ''%s''', + e.name, e.workspace_id, n.workspace_id) + FROM entries e JOIN naming n + ON e.value->'reference'->>'workspace_id' = n.workspace_id + AND e.value->'reference'->>'datatable' = n.name + UNION ALL + SELECT format('Ducklake ''%s'' in workspace ''%s''', dl.key, ws.workspace_id) + FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each( + CASE WHEN jsonb_typeof(ws.ducklake->'ducklakes') = 'object' + THEN ws.ducklake->'ducklakes' ELSE '{}'::jsonb END) dl + WHERE dl.value->'catalog'->>'resource_type' = $1 + AND dl.value->'catalog'->>'resource_path' = $2 + UNION ALL + SELECT format('the Ducklake namespace of fork ''%s'', not cleaned up yet', workspace_id) + FROM fork_ducklake_namespace + WHERE catalog = $1 || ':' || $2 AND NOT schema_dropped + ORDER BY 1", + ) + .bind(kind.as_ref()) + .bind(dbname) + .bind(exempt_workspace) + .bind(exempt_name) + .fetch_all(&mut *conn) + .await?) +} + +/// Held by fork cleanup of `w_id`'s data tables and by forking `w_id`, which can hand the new fork +/// pointers at them, so a pointer cannot appear between cleanup's check and its drop. +pub async fn lock_fork_datatables(conn: &mut sqlx::PgConnection, w_id: &str) -> Result<()> { + sqlx::query("SELECT pg_advisory_xact_lock(hashtext('fork_datatables:' || $1))") + .bind(w_id) + .execute(&mut *conn) + .await?; + Ok(()) +} + impl GoverningDatatable { - /// Backed by the Windmill instance's own Postgres, which is the only substrate data table - /// roles apply to. - pub fn is_instance(&self) -> bool { + /// The Windmill-managed cluster whose data table roles this entry can use. `None` for a + /// resource-backed one: roles are logins Windmill creates, and it creates none on a host a + /// workspace admin chose. + pub fn role_cluster(&self) -> Option { self.datatable .database .as_ref() - .is_some_and(|d| d.resource_type == DataTableCatalogResourceType::Instance) + .and_then(|d| crate::datatable_roles::DatatableRoleCluster::of(d.resource_type)) } /// The workspace whose admins administer the data table and whose members its tenants are. @@ -1776,20 +1885,19 @@ pub async fn resolve_workspace_governing_datatables( clone, )), (None, Some(governed_by)) => { - let (governor_ws, governor_name) = - (governed_by.workspace_id.clone(), governed_by.datatable.clone()); + let (governor_ws, governor_name) = ( + governed_by.workspace_id.clone(), + governed_by.datatable.clone(), + ); let clone = clone.or(Some((ws, name, datatable))); next.push((i, governor_ws, governor_name, clone)); } (None, None) => resolved.push(( i, match clone { - None => GoverningDatatable { - workspace_id: ws, - name, - datatable, - governor: None, - }, + None => { + GoverningDatatable { workspace_id: ws, name, datatable, governor: None } + } Some((clone_ws, clone_name, mut clone_datatable)) => { clone_datatable.permissions = datatable.permissions; GoverningDatatable { @@ -1830,7 +1938,8 @@ pub async fn resolve_workspace_governing_datatables( } /// Build the `admin` connection for a governing entry: `custom_instance_user` for an instance -/// database, the user's own resource for a BYO-postgres one. +/// database, on Windmill's cluster or the external one; the user's own resource for a BYO-postgres +/// one. async fn resolve_datatable_connection_unchecked( db: &DB, governing: &GoverningDatatable, @@ -1841,7 +1950,16 @@ async fn resolve_datatable_connection_unchecked( .database .as_ref() .expect("a governing entry owns a database"); - if database.resource_type == DataTableCatalogResourceType::Instance { + if database.resource_type == DataTableCatalogResourceType::ExternalInstance { + let pg_creds = crate::external_instance_pg::external_instance_connection_unchecked( + db, + &database.resource_path, + replication, + ) + .await?; + serde_json::to_value(&pg_creds) + .map_err(|e| Error::internal_err(format!("Error serializing pg creds: {}", e))) + } else if database.resource_type == DataTableCatalogResourceType::Instance { let mut pg_creds = PgDatabase::parse_uri(&get_database_url().await?.as_str().await)?; pg_creds.dbname = database.resource_path.clone(); if replication { @@ -1879,8 +1997,31 @@ pub async fn get_datatable_resource_from_db_unchecked( w_id: &str, name: &str, ) -> Result { + Ok(get_datatable_connection_and_kind_unchecked(db, w_id, name) + .await? + .0) +} + +/// As [`get_datatable_resource_from_db_unchecked`], also reporting the kind of database Windmill +/// manages behind it, `None` for a user resource. One resolution answers both: a caller reading the +/// kind separately can be handed one kind's connection and the other kind's checks by a save +/// landing between the two, and the entry a pointer lands on is another workspace's to change. +/// +/// Same authorization contract: the connection reaches every role. +pub async fn get_datatable_connection_and_kind_unchecked( + db: &DB, + w_id: &str, + name: &str, +) -> Result<(serde_json::Value, Option)> { let governing = resolve_governing_datatable(db, w_id, name).await?; - resolve_datatable_connection_unchecked(db, &governing, false).await + let kind = governing + .datatable + .database + .as_ref() + .map(|d| d.resource_type) + .filter(|kind| kind.is_windmill_managed()); + let connection = resolve_datatable_connection_unchecked(db, &governing, false).await?; + Ok((connection, kind)) } /// Same as [`get_datatable_resource_from_db_unchecked`] but for postgres trigger @@ -2385,6 +2526,10 @@ pub enum DucklakeCatalogResourceType { Postgresql, Mysql, Instance, + /// On the external instance cluster ([`crate::external_instance_pg`]). Enterprise Edition. + #[serde(rename = "external_instance")] + #[strum(serialize = "external_instance")] + ExternalInstance, } #[derive(Deserialize, Serialize)] @@ -2912,7 +3057,16 @@ async fn ducklake_conn_data( let ducklake = serde_json::from_value::(ducklake)?; let catalog_resource = - if ducklake.catalog.resource_type == DucklakeCatalogResourceType::Instance { + if ducklake.catalog.resource_type == DucklakeCatalogResourceType::ExternalInstance { + let pg_creds = crate::external_instance_pg::external_instance_connection_unchecked( + db, + &ducklake.catalog.resource_path, + false, + ) + .await?; + serde_json::to_value(&pg_creds) + .map_err(|e| Error::internal_err(format!("Error serializing pg creds: {}", e)))? + } else if ducklake.catalog.resource_type == DucklakeCatalogResourceType::Instance { let mut pg_creds = PgDatabase::parse_uri(&get_database_url().await?.as_str().await)?; pg_creds.dbname = ducklake.catalog.resource_path.clone(); pg_creds.user = Some("custom_instance_user".to_string()); @@ -3293,6 +3447,14 @@ async fn register_fork_ducklake_namespace( { return Ok(()); } + let mut tx = db.begin().await?; + // A row naming an external database counts as a use of it. Written under the lock a drop takes, + // and only while the database is still registered, so a drop cannot slip in between the + // settings this attach resolved and the row that protects the database. + if let Some(dbname) = catalog.strip_prefix("external_instance:") { + crate::external_instance_pg::ensure_external_instance_database_registered(&mut tx, dbname) + .await?; + } sqlx::query!( "INSERT INTO fork_ducklake_namespace (workspace_id, ducklake_name, metadata_schema, catalog, storage, storage_ref, data_path) @@ -3307,9 +3469,10 @@ async fn register_fork_ducklake_namespace( &storage_ref, data_path, ) - .execute(db) + .execute(&mut *tx) .await .map_err(|e| Error::internal_err(format!("registering fork ducklake namespace: {e:#}")))?; + tx.commit().await?; let mut locations = FORK_DUCKLAKE_REGISTERED .get(w_id) .filter(|(_, exp)| *exp > now) diff --git a/backend/windmill-worker/src/duckdb_executor.rs b/backend/windmill-worker/src/duckdb_executor.rs index 943703472f..d3bd976f21 100644 --- a/backend/windmill-worker/src/duckdb_executor.rs +++ b/backend/windmill-worker/src/duckdb_executor.rs @@ -1475,6 +1475,7 @@ pub async fn do_duckdb( &job.id, client, &mut hidden_passwords, + job_dir, ) .await?, ); @@ -1490,13 +1491,19 @@ pub async fn do_duckdb( &mut hidden_passwords, &job.workspace_id, materialize.as_ref().map(|(_, m)| m.asset_path.as_str()), + job_dir, ) .await? { probe_blocks.extend(q); - } else if let Some(q) = - transform_attach_datatable(&query_block, conn, &mut hidden_passwords, job) - .await? + } else if let Some(q) = transform_attach_datatable( + &query_block, + conn, + &mut hidden_passwords, + job, + job_dir, + ) + .await? { probe_blocks.extend(q); } else { @@ -1552,6 +1559,7 @@ pub async fn do_duckdb( &job.id, client, &mut hidden_passwords, + job_dir, ) .await?, ); @@ -1567,13 +1575,19 @@ pub async fn do_duckdb( &mut hidden_passwords, &job.workspace_id, materialize.as_ref().map(|(_, m)| m.asset_path.as_str()), + job_dir, ) .await? { v.extend(ducklake_query); - } else if let Some(datatable_query) = - transform_attach_datatable(&query_block, conn, &mut hidden_passwords, job) - .await? + } else if let Some(datatable_query) = transform_attach_datatable( + &query_block, + conn, + &mut hidden_passwords, + job, + job_dir, + ) + .await? { v.extend(datatable_query); } else { @@ -2240,11 +2254,80 @@ fn parse_attach_db_resource<'a>(query: &'a str) -> Option Result { +/// The verification a DuckDB postgres attach keeps, as its libpq `sslmode` and `sslrootcert`. +/// +/// Attaches have always turned verify-ca and verify-full into `require`, which resources rely on. +/// A connection that explicitly refuses invalid certificates — the external instance cluster's — +/// keeps its mode instead: under `require` its shared password would go to whichever server +/// answers. DuckDB's libpq takes one root file, so it gets the system bundle plus the configured +/// certificate, written in the job directory: a resource's certificate is workspace-controlled, so +/// a file per distinct one has to go with the job rather than pile up on the worker. +fn pg_attach_verification<'a>( + res: &'a PgDatabase, + job_dir: &str, +) -> Result> { + let mode = match res.sslmode.as_deref() { + // The rule every other Postgres connection uses, so a resource that verifies elsewhere is + // not quietly downgraded here. + Some(mode @ ("verify-ca" | "verify-full")) if !res.verify_mode_skips_verification() => mode, + _ => return Ok(None), + }; + let bundle = windmill_common::system_ca_bundle() + .map(std::fs::read_to_string) + .transpose() + .map_err(|e| Error::ExecutionErr(format!("Failed to read the system CA bundle: {e}")))? + .unwrap_or_default(); + let pem = res.root_certificate_pem.as_deref().unwrap_or_default(); + if bundle.is_empty() && pem.is_empty() { + return Err(Error::ExecutionErr(format!( + "sslmode {mode} needs a root certificate, and this worker has no system CA bundle" + ))); + } + let roots = format!("{bundle}\n{pem}\n"); + use sha2::Digest; + let path = std::path::Path::new(job_dir).join(format!( + "pg_roots_{}.pem", + hex::encode(&sha2::Sha256::digest(roots.as_bytes())[..8]) + )); + // The job's modules are written in this directory first, so whatever already sits at this path + // may be caller-supplied: trusting it would let the caller pick the CA. Replace it, and + // `create_new` refuses to write through anything recreated there. + let write_roots = || -> std::io::Result<()> { + match std::fs::remove_file(&path) { + Err(e) if e.kind() != std::io::ErrorKind::NotFound => return Err(e), + _ => {} + } + use std::io::Write; + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&path)? + .write_all(roots.as_bytes()) + }; + write_roots() + .map_err(|e| Error::ExecutionErr(format!("Failed to write root certificates: {e}")))?; + Ok(Some((mode, path))) +} + +fn pg_attach_uri(res: &PgDatabase, job_dir: &str) -> Result { + let uri = res.to_uri(); + let Some((mode, roots)) = pg_attach_verification(res, job_dir)? else { + return Ok(uri); + }; + let base = uri.strip_suffix("?sslmode=require").ok_or_else(|| { + Error::internal_err("unexpected sslmode in a postgres connection URI".to_string()) + })?; + Ok(format!( + "{base}?sslmode={mode}&sslrootcert={}", + urlencoding::encode(&roots.to_string_lossy()) + )) +} + +fn format_attach_db_conn_str(db_resource: Value, db_type: &str, job_dir: &str) -> Result { let s = match db_type.to_lowercase().as_str() { "postgres" | "postgresql" => { let res: PgDatabase = serde_json::from_value(db_resource)?; - res.to_uri() + pg_attach_uri(&res, job_dir)? } #[cfg(feature = "mysql")] "mysql" => { @@ -2316,6 +2399,7 @@ async fn transform_attach_db_resource_query( job_id: &Uuid, client: &AuthedClient, hidden_passwords: &mut Arc>>, + job_dir: &str, ) -> Result> { let db_resource: Value = client .get_resource_value_interpolated(parsed.resource_path, Some(job_id.to_string())) @@ -2323,8 +2407,14 @@ async fn transform_attach_db_resource_query( if let Some(pwd) = db_resource.get("password").and_then(|p| p.as_str()) { hidden_passwords.lock().unwrap().push(pwd.to_string()); } - db_resource_to_attach_statements(db_resource, parsed.name, parsed.db_type, parsed.extra_args) - .await + db_resource_to_attach_statements( + db_resource, + parsed.name, + parsed.db_type, + parsed.extra_args, + job_dir, + ) + .await } async fn db_resource_to_attach_statements( @@ -2332,11 +2422,12 @@ async fn db_resource_to_attach_statements( ident_name: &str, db_type: &str, extra_args: Option<&str>, + job_dir: &str, ) -> Result> { // Escape single quotes: the connection string is built from resource fields // (host/db/user/password) and embedded in a single-quoted DuckDB literal, so an // unescaped quote in any field would otherwise break out of the ATTACH statement. - let conn_str = format_attach_db_conn_str(db_resource, db_type)?.replace('\'', "''"); + let conn_str = format_attach_db_conn_str(db_resource, db_type, job_dir)?.replace('\'', "''"); let attach_str = format!( "ATTACH '{}' as {} (TYPE {}{});", conn_str, @@ -2359,6 +2450,7 @@ async fn transform_attach_ducklake( hidden_passwords: &mut Arc>>, w_id: &str, materialize_target: Option<&str>, + job_dir: &str, ) -> Result>> { lazy_static::lazy_static! { static ref RE: regex::Regex = regex::Regex::new(r"(?i)ATTACH\s*'ducklake(://[^':]+)?'\s*AS\s+([^ ;]+)\s*(\([^)]*\))?").unwrap(); @@ -2391,7 +2483,9 @@ async fn transform_attach_ducklake( format!(", {}", user_extra_args) }; let db_type = match ducklake.catalog.resource_type { - DucklakeCatalogResourceType::Instance => "postgres", + DucklakeCatalogResourceType::Instance | DucklakeCatalogResourceType::ExternalInstance => { + "postgres" + } _ => ducklake.catalog.resource_type.as_ref(), }; @@ -2407,7 +2501,7 @@ async fn transform_attach_ducklake( // single-quoted DuckDB literals below, so an unescaped quote in a resource // field would break out of the ATTACH statement. let db_conn_str = - format_attach_db_conn_str(ducklake.catalog_resource, db_type)?.replace('\'', "''"); + format_attach_db_conn_str(ducklake.catalog_resource, db_type, job_dir)?.replace('\'', "''"); let storage = ducklake .storage .storage @@ -2462,6 +2556,7 @@ async fn transform_attach_ducklake( defer, materialize_target, hidden_passwords, + job_dir, )?); } Ok(Some(statements)) @@ -2495,6 +2590,7 @@ fn fork_defer_statements( defer: &windmill_common::workspaces::DucklakeForkDefer, materialize_target: Option<&str>, hidden_passwords: &mut Arc>>, + job_dir: &str, ) -> Result> { let mut stmts = vec![]; if defer.ancestors.is_empty() { @@ -2507,12 +2603,13 @@ fn fork_defer_statements( hidden_passwords.lock().unwrap().push(pwd.to_string()); } let db_type = match a.catalog.resource_type { - DucklakeCatalogResourceType::Instance => "postgres", + DucklakeCatalogResourceType::Instance + | DucklakeCatalogResourceType::ExternalInstance => "postgres", _ => a.catalog.resource_type.as_ref(), }; stmts.push(get_attach_db_install_str(db_type)?.to_string()); - let conn_str = - format_attach_db_conn_str(a.catalog_resource.clone(), db_type)?.replace('\'', "''"); + let conn_str = format_attach_db_conn_str(a.catalog_resource.clone(), db_type, job_dir)? + .replace('\'', "''"); let storage = a .storage .storage @@ -2631,6 +2728,7 @@ async fn transform_attach_datatable( conn: &Connection, hidden_passwords: &mut Arc>>, job: &MiniPulledJob, + job_dir: &str, ) -> Result>> { let Some(attached) = parse_attach_datatable(query) else { return Ok(None); @@ -2673,6 +2771,7 @@ async fn transform_attach_datatable( Ok(Some(pg_secret_attach_statements( db_resource, attached.alias, + job_dir, )?)) } @@ -2693,17 +2792,32 @@ fn datatable_secret_name(alias: &str) -> String { /// ATTACH a datatable's postgres database through a DuckDB TEMPORARY SECRET holding /// the connection parameters; only sslmode and options ride in the ATTACH string. -fn pg_secret_attach_statements(db_resource: Value, alias_name: &str) -> Result> { +fn pg_secret_attach_statements( + db_resource: Value, + alias_name: &str, + job_dir: &str, +) -> Result> { let res: PgDatabase = serde_json::from_value(db_resource)?; // Escape single quotes: each field is embedded in a single-quoted DuckDB literal, // so an unescaped quote would break out of the CREATE SECRET statement. let esc = |s: &str| s.replace('\'', "''"); // The postgres secret type has no sslmode parameter, so it goes in the ATTACH // string; only the libpq values PgDatabase::to_uri collapses to are forwarded. - let sslmode = match res.sslmode.as_deref() { - Some("disable") => "disable", - Some("require") | Some("verify-ca") | Some("verify-full") => "require", - _ => "prefer", + let sslmode = match pg_attach_verification(&res, job_dir)? { + // A libpq keyword/value string: the path is quoted for libpq, then for the DuckDB literal. + Some((mode, roots)) => format!( + "{mode} sslrootcert=''{}''", + roots + .to_string_lossy() + .replace('\\', "\\\\") + .replace('\'', "\\''") + ), + None => match res.sslmode.as_deref() { + Some("disable") => "disable", + Some("require") | Some("verify-ca") | Some("verify-full") => "require", + _ => "prefer", + } + .to_string(), }; // Nor an options parameter. The value is quoted for libpq's keyword/value syntax first, // then escaped for the DuckDB literal around it. @@ -2805,6 +2919,76 @@ pub struct Arg { mod tests { use super::*; + #[test] + fn pg_attach_keeps_verification_only_when_required() { + let job_dir = std::env::temp_dir().join(format!("wm-test-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&job_dir).unwrap(); + let job_dir = job_dir.to_string_lossy().to_string(); + let pg = |sslmode: &str, accept_invalid_certs: Option| PgDatabase { + host: "db.internal".to_string(), + user: Some("custom_instance_user".to_string()), + password: Some("pw".to_string()), + port: None, + sslmode: Some(sslmode.to_string()), + dbname: "dt".to_string(), + root_certificate_pem: Some("-----BEGIN CERTIFICATE-----test".to_string()), + accept_invalid_certs, + use_iam_auth: None, + region: None, + options: None, + }; + let uri = pg_attach_uri(&pg("verify-full", Some(false)), &job_dir).unwrap(); + assert!(uri.contains("?sslmode=verify-full&sslrootcert="), "{uri}"); + let root = urlencoding::decode(uri.split("sslrootcert=").nth(1).unwrap()).unwrap(); + assert!(std::fs::read_to_string(root.as_ref()) + .unwrap() + .contains("-----BEGIN CERTIFICATE-----test")); + // A file a job module planted under the same name is not trusted. + std::fs::write(root.as_ref(), "-----BEGIN CERTIFICATE-----planted").unwrap(); + assert_eq!( + pg_attach_uri(&pg("verify-full", Some(false)), &job_dir).unwrap(), + uri + ); + assert!(!std::fs::read_to_string(root.as_ref()) + .unwrap() + .contains("planted")); + // Every certificate a job attaches keeps its own file: one attach must not evict another's. + for i in 0..40 { + let mut other = pg("verify-full", Some(false)); + other.root_certificate_pem = Some(format!("-----BEGIN CERTIFICATE-----{i}")); + let other = pg_attach_uri(&other, &job_dir).unwrap(); + let path = urlencoding::decode(other.split("sslrootcert=").nth(1).unwrap()).unwrap(); + assert!(std::path::Path::new(path.as_ref()).is_file(), "{path}"); + } + assert!( + std::path::Path::new(root.as_ref()).is_file(), + "the first file is still there" + ); + let external = serde_json::to_value(pg("verify-full", Some(false))).unwrap(); + let attach = &pg_secret_attach_statements(external, "dt", &job_dir).unwrap()[3]; + assert!( + attach.starts_with(&format!( + "ATTACH 'sslmode=verify-full sslrootcert=''{}''", + root + )), + "{attach}" + ); + // A resource carrying a root certificate verifies without opting in, as it does on every + // other Postgres path; one carrying neither keeps the historical downgrade. + assert!(pg_attach_uri(&pg("verify-full", None), &job_dir) + .unwrap() + .contains("?sslmode=verify-full&sslrootcert=")); + let mut bare = pg("verify-full", None); + bare.root_certificate_pem = None; + assert!(pg_attach_uri(&bare, &job_dir) + .unwrap() + .ends_with("?sslmode=require")); + assert!(pg_attach_uri(&pg("require", Some(false)), &job_dir) + .unwrap() + .ends_with("?sslmode=require")); + std::fs::remove_dir_all(&job_dir).unwrap(); + } + #[test] fn attach_datatable_parses_name_and_role() { let reference_of = |q: &str| parse_attach_datatable(q).unwrap().reference; @@ -2956,7 +3140,7 @@ mod tests { let mut defer = test_fork_defer(vec![("orders", false)], vec![]); defer.ancestors[0].extra_args = Some("ENCRYPTED true".to_string()); let mut hp = Arc::new(Mutex::new(vec![])); - let stmts = fork_defer_statements("lake", "dl", &defer, None, &mut hp).unwrap(); + let stmts = fork_defer_statements("lake", "dl", &defer, None, &mut hp, "/tmp").unwrap(); let attach = stmts .iter() .find(|s| s.starts_with("ATTACH IF NOT EXISTS")) @@ -2980,7 +3164,7 @@ mod tests { vec![], ); let mut hp = Arc::new(Mutex::new(vec![])); - let stmts = fork_defer_statements("lake", "dl", &defer, None, &mut hp).unwrap(); + let stmts = fork_defer_statements("lake", "dl", &defer, None, &mut hp, "/tmp").unwrap(); let joined = stmts.join("\n"); assert!( joined.contains( @@ -3012,7 +3196,7 @@ mod tests { fn test_fork_defer_statements_shape() { let defer = test_fork_defer(vec![("orders", false), ("dim", true)], vec![]); let mut hp = Arc::new(Mutex::new(vec![])); - let stmts = fork_defer_statements("lake", "dl", &defer, None, &mut hp).unwrap(); + let stmts = fork_defer_statements("lake", "dl", &defer, None, &mut hp, "/tmp").unwrap(); let joined = stmts.join("\n"); // Ancestor attach: read-only, idempotent, never auto-migrating or auto-creating. assert!(joined.contains("ATTACH IF NOT EXISTS"), "{joined}"); @@ -3038,9 +3222,15 @@ mod tests { // Target currently a defer view → skip its CREATE, drop the view (+ companion). let defer = test_fork_defer(vec![("orders", false)], vec!["orders", "orders_current"]); let mut hp = Arc::new(Mutex::new(vec![])); - let stmts = - fork_defer_statements("lake", "_wm_target", &defer, Some("lake/orders"), &mut hp) - .unwrap(); + let stmts = fork_defer_statements( + "lake", + "_wm_target", + &defer, + Some("lake/orders"), + &mut hp, + "/tmp", + ) + .unwrap(); let joined = stmts.join("\n"); assert!(!joined.contains("CREATE VIEW"), "{joined}"); assert!( @@ -3055,15 +3245,22 @@ mod tests { // Target already a real table (NOT in fork_views, e.g. after a failed re-run whose // status can't be trusted) → no DROP VIEW, or the job would wedge on a type mismatch. let defer = test_fork_defer(vec![("orders", false)], vec![]); - let stmts = - fork_defer_statements("lake", "_wm_target", &defer, Some("lake/orders"), &mut hp) - .unwrap(); + let stmts = fork_defer_statements( + "lake", + "_wm_target", + &defer, + Some("lake/orders"), + &mut hp, + "/tmp", + ) + .unwrap(); assert!(!stmts.join("\n").contains("DROP VIEW"), "{stmts:?}"); // Target in a different lake → this lake's defer views are untouched. let defer = test_fork_defer(vec![("orders", false)], vec!["orders"]); let stmts = - fork_defer_statements("lake", "dl", &defer, Some("other/orders"), &mut hp).unwrap(); + fork_defer_statements("lake", "dl", &defer, Some("other/orders"), &mut hp, "/tmp") + .unwrap(); let joined = stmts.join("\n"); assert!( joined.contains("CREATE VIEW IF NOT EXISTS dl.\"orders\""), @@ -3076,7 +3273,7 @@ mod tests { fn test_fork_defer_statements_schema_qualified() { let defer = test_fork_defer(vec![("staging.raw", false)], vec![]); let mut hp = Arc::new(Mutex::new(vec![])); - let stmts = fork_defer_statements("lake", "dl", &defer, None, &mut hp).unwrap(); + let stmts = fork_defer_statements("lake", "dl", &defer, None, &mut hp, "/tmp").unwrap(); let joined = stmts.join("\n"); assert!( joined.contains("CREATE SCHEMA IF NOT EXISTS dl.\"staging\";"), @@ -3902,7 +4099,7 @@ mod tests { "dbname": "mydb", "sslmode": "require" }); - let result = format_attach_db_conn_str(db_resource, "postgres").unwrap(); + let result = format_attach_db_conn_str(db_resource, "postgres", "/tmp").unwrap(); // Should be in URI format: postgres://user:password@host:port/dbname?sslmode=require assert!(result.starts_with("postgres://")); assert!(result.contains("admin:secret123@localhost:5432/mydb")); @@ -3915,7 +4112,7 @@ mod tests { "host": "db.example.com", "dbname": "production" }); - let result = format_attach_db_conn_str(db_resource, "postgres").unwrap(); + let result = format_attach_db_conn_str(db_resource, "postgres", "/tmp").unwrap(); // Should be in URI format with defaults: postgres://postgres:@host:5432/dbname?sslmode=prefer assert!(result.starts_with("postgres://")); assert!(result.contains("@db.example.com:5432/production")); @@ -3928,7 +4125,7 @@ mod tests { "host": "localhost", "dbname": "test" }); - let result = format_attach_db_conn_str(db_resource, "postgresql").unwrap(); + let result = format_attach_db_conn_str(db_resource, "postgresql", "/tmp").unwrap(); // Should be in URI format (postgresql is treated the same as postgres) assert!(result.starts_with("postgres://")); assert!(result.contains("@localhost:5432/test")); @@ -3945,7 +4142,7 @@ mod tests { "dbname": "wm_datatables", "sslmode": "require" }); - let stmts = pg_secret_attach_statements(db_resource, "dt").unwrap(); + let stmts = pg_secret_attach_statements(db_resource, "dt", "/tmp").unwrap(); assert_eq!(stmts[0], "INSTALL postgres;"); assert_eq!(stmts[1], "LOAD postgres;"); let secret_name = datatable_secret_name("dt"); @@ -3976,7 +4173,7 @@ mod tests { if let Some(s) = input { db_resource["sslmode"] = json!(s); } - let stmts = pg_secret_attach_statements(db_resource, "dt").unwrap(); + let stmts = pg_secret_attach_statements(db_resource, "dt", "/tmp").unwrap(); assert!( stmts[3].starts_with(&format!("ATTACH 'sslmode={expected}'")), "sslmode {input:?} → {}", @@ -3988,7 +4185,7 @@ mod tests { #[test] fn test_pg_secret_attach_statements_options() { let db_resource = json!({ "host": "h", "dbname": "d", "options": r"-c search_path='a\b'" }); - let stmts = pg_secret_attach_statements(db_resource, "dt").unwrap(); + let stmts = pg_secret_attach_statements(db_resource, "dt", "/tmp").unwrap(); let secret_name = datatable_secret_name("dt"); assert_eq!( stmts[3], @@ -4012,7 +4209,7 @@ mod tests { let db_resource = json!({ "project_id": "my-gcp-project" }); - let result = format_attach_db_conn_str(db_resource, "bigquery").unwrap(); + let result = format_attach_db_conn_str(db_resource, "bigquery", "/tmp").unwrap(); assert_eq!(result, "project=my-gcp-project"); } @@ -4021,7 +4218,7 @@ mod tests { let db_resource = json!({ "other_field": "value" }); - let result = format_attach_db_conn_str(db_resource, "bigquery"); + let result = format_attach_db_conn_str(db_resource, "bigquery", "/tmp"); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("project_id")); } @@ -4029,7 +4226,7 @@ mod tests { #[test] fn test_format_attach_db_conn_str_unsupported_type() { let db_resource = json!({}); - let result = format_attach_db_conn_str(db_resource, "oracle"); + let result = format_attach_db_conn_str(db_resource, "oracle", "/tmp"); assert!(result.is_err()); assert!(result .unwrap_err() @@ -4043,7 +4240,7 @@ mod tests { "host": "localhost", "dbname": "test" }); - let result = format_attach_db_conn_str(db_resource, "POSTGRES").unwrap(); + let result = format_attach_db_conn_str(db_resource, "POSTGRES", "/tmp").unwrap(); // Should be in URI format assert!(result.starts_with("postgres://")); assert!(result.contains("@localhost:5432/test")); @@ -4060,7 +4257,7 @@ mod tests { "database": "app_db", "ssl": true }); - let result = format_attach_db_conn_str(db_resource, "mysql").unwrap(); + let result = format_attach_db_conn_str(db_resource, "mysql", "/tmp").unwrap(); assert!(result.contains("database=app_db")); assert!(result.contains("host=mysql.example.com")); assert!(result.contains("ssl_mode=required")); @@ -4077,7 +4274,7 @@ mod tests { "database": "test", "ssl": false }); - let result = format_attach_db_conn_str(db_resource, "mysql").unwrap(); + let result = format_attach_db_conn_str(db_resource, "mysql", "/tmp").unwrap(); assert!(result.contains("ssl_mode=disabled")); } diff --git a/cli/src/commands/datatable/datatable.ts b/cli/src/commands/datatable/datatable.ts index 6609d08c50..46ee7b4c39 100644 --- a/cli/src/commands/datatable/datatable.ts +++ b/cli/src/commands/datatable/datatable.ts @@ -111,6 +111,8 @@ const migrateCommand = new Command() ) .action(migrateDown as any); +type DataTableResourceType = "postgresql" | "instance" | "external_instance"; + async function create( opts: GlobalOptions & { resource?: string; force?: boolean }, name?: string, @@ -139,12 +141,12 @@ async function create( const datatables: Record< string, - { database: { resource_type: "postgresql" | "instance"; resource_path?: string } } + { database: { resource_type: DataTableResourceType; resource_path?: string } } > = {}; for (const d of existing) { datatables[d.name] = { database: { - resource_type: d.resource_type as "postgresql" | "instance", + resource_type: d.resource_type as DataTableResourceType, resource_path: d.resource_path ?? undefined, }, }; diff --git a/docs/external-instance-datatables.md b/docs/external-instance-datatables.md new file mode 100644 index 0000000000..acfb93b90a --- /dev/null +++ b/docs/external-instance-datatables.md @@ -0,0 +1,88 @@ +# External instance data tables + +A data table is backed by one of three things: a Postgres resource a workspace brings +(`postgresql`), a database on Windmill's own cluster (`instance`), or a database on a separate +cluster Windmill administers (`external_instance`, Enterprise Edition). The third is what this +document covers; Ducklake catalogs take the same three shapes. + +Windmill administers the external cluster the way it administers its own: it creates and drops +databases there, owns `custom_instance_user` and `custom_instance_replication_user`, and creates +the data table roles of that cluster. It logs in as the admin in the `external_instance_pg` +instance setting, and keeps what it generates in the hidden `external_instance_pg_state` setting. + +## Code + +| Where | What | +|---|---| +| `windmill-common/src/external_instance_pg.rs` | Setting, state, usage accounting, the lifecycle lock, the OSS forwarders | +| `windmill-common/src/external_instance_pg_ee.rs` | Setup, database create and drop, the admin connection | +| `windmill-common/src/datatable_roles.rs` | Per-cluster role catalogs (`DatatableRoleCluster`) | +| `windmill-common/src/workspaces.rs` | Resolution (`resolve_datatable_connection_unchecked`), `managed_database_uses` | +| `windmill-api-settings/src/lib.rs` | `/settings/external_instance_pg/*`, `/settings/datatable_roles` | + +## What holds it together + +- **One lifecycle lock.** `lock_external_instance_pg_state` serializes everything that changes + which databases exist on the cluster or which entries name them: setup, create, drop, data table + and Ducklake saves, external role DDL, and writes to the setting itself. Anything reading the + configuration to reach the cluster reads it under that lock, so a database is never created on + one cluster and registered while the setting names another. +- **Windmill only touches what it made.** Databases it creates carry a comment, and a drop + requires it. The two managed roles and every data table role carry their own comment, and setup + refuses a `custom_instance_user` without it rather than resetting the password of someone else's + role. +- **Creation needs a successful setup.** `set_up_for` records the `host:port` the last successful + setup converged. Creating a database on a cluster that setup has not succeeded on is refused. +- **Nothing is dropped from under a user.** `managed_database_uses` lists every data table naming + a database, every fork pointing at those, every Ducklake catalog on it, and every fork Ducklake + metadata schema still to be dropped. Fork cleanup exempts exactly the entry it is cleaning up. +- **Fork copies belong to a workspace.** `wm_fork_*` is a name, not an authorization: every + database of a cluster answers to the same `custom_instance_user`. The registry records the + workspace a copy was created for, and a member can only import into or fork onto a copy of their + own workspace. +- **Roles are per cluster.** `datatable_role.cluster` splits the catalog, so the same role name can + exist on both clusters. Role names are unique per cluster, as they are in Postgres. + +## Running one locally + +```bash +docker run -d --name wm-external-pg -e POSTGRES_PASSWORD=external -p 5497:5432 postgres:18 \ + -c wal_level=logical +psql "postgresql://postgres:external@127.0.0.1:5497/postgres" \ + -c "CREATE ROLE wm_admin LOGIN PASSWORD 'adminpw' CREATEDB CREATEROLE REPLICATION" +``` + +A non-superuser admin with `CREATEDB` and `CREATEROLE` is the realistic case: managed Postgres +gives nothing more. `REPLICATION` is only needed for Postgres triggers on external data tables. + +Then, as superadmin (`$T` is a token): + +```bash +api=http://localhost:8000/api +curl -s -X POST $api/settings/global/external_instance_pg -H "Authorization: Bearer $T" \ + -H 'Content-Type: application/json' \ + --data '{"value":{"host":"127.0.0.1","port":5497,"user":"wm_admin","password":"adminpw","sslmode":"disable"}}' +curl -s -X POST $api/settings/external_instance_pg/setup -H "Authorization: Bearer $T" \ + -H 'Content-Type: application/json' --data '{}' # report per step +curl -s -X POST $api/settings/external_instance_pg/databases/dt_demo -H "Authorization: Bearer $T" \ + -H 'Content-Type: application/json' --data '{}' +curl -s -X POST $api/w/admins/workspaces/edit_datatable_config -H "Authorization: Bearer $T" \ + -H 'Content-Type: application/json' \ + --data '{"settings":{"datatables":{"demo":{"database":{"resource_type":"external_instance","resource_path":"dt_demo"}}}}}' +``` + +`sslmode` defaults to `verify-full`; `disable` is for a local container only. With `verify-full` +against a server with a private CA, put the CA in `root_certificate_pem` — `pg_dump`, `psql` and +DuckDB attaches all verify against the system trust store plus that certificate. + +Jobs then reach it as any data table: `ATTACH 'datatable://demo' AS d` from DuckDB, or +`datatable://demo` as the database of a PostgreSQL script, with `-- role ` to connect as a +data table role of that cluster. + +Worth knowing while testing: + +- A worker needs the `postgresql` and `duckdb` tags for those jobs + (`update config set config = jsonb_set(config, '{worker_tags}', …) where name = 'worker__default'`). +- DuckDB jobs load `libwindmill_duckdb_ffi_internal.so` by name, so a binary built into its own + `CARGO_TARGET_DIR` needs that library on `LD_LIBRARY_PATH`. +- Setup holds the lifecycle lock for its whole run, so a settings save during it waits. diff --git a/frontend/src/lib/components/InstanceSetting.svelte b/frontend/src/lib/components/InstanceSetting.svelte index 0da63ffbb0..d6740c6c9b 100644 --- a/frontend/src/lib/components/InstanceSetting.svelte +++ b/frontend/src/lib/components/InstanceSetting.svelte @@ -23,6 +23,8 @@ import RetentionPeriodOverrides from './instanceSettings/RetentionPeriodOverrides.svelte' import SmtpSettings from './instanceSettings/SmtpSettings.svelte' import SecretBackendConfig from './instanceSettings/SecretBackendConfig.svelte' + import ExternalInstancePgSettings from './instanceSettings/ExternalInstancePgSettings.svelte' + import InstancePgSettings from './instanceSettings/InstancePgSettings.svelte' import GhesAppSettings from './instanceSettings/GhesAppSettings.svelte' import WebhookBaseUrlSetting from './instanceSettings/WebhookBaseUrlSetting.svelte' import WsConnectivityTest from './instanceSettings/WsConnectivityTest.svelte' @@ -43,6 +45,7 @@ openSmtpSettings?: () => void oauths?: Record warning?: string + markSettingSaved?: (key: string) => void } let { @@ -52,7 +55,8 @@ loading = true, openSmtpSettings, oauths, - warning + warning, + markSettingSaved }: Props = $props() const dispatch = createEventDispatcher() @@ -782,10 +786,10 @@ />

Comma-separated host/IP patterns the proxy still traces but for which it skips - upstream TLS certificate verification. Use for internal endpoints with - self-signed or otherwise untrusted certificates — unlike NO_PROXY above, these - requests stay traced. Same matching as NO_PROXY (example.com matches - subdomains; .example.com matches subdomains only). + upstream TLS certificate verification. Use for internal endpoints with self-signed + or otherwise untrusted certificates — unlike NO_PROXY above, these requests stay + traced. Same matching as NO_PROXY (example.com matches subdomains; + .example.com matches subdomains only).

@@ -865,6 +869,14 @@ {:else if setting.fieldType == 'secret_backend'} + {:else if setting.fieldType == 'external_instance_pg'} + + {:else if setting.fieldType == 'instance_pg'} + {:else if setting.fieldType == 'github_enterprise_app'} {:else if setting.fieldType == 'webhook_base_url'} diff --git a/frontend/src/lib/components/InstanceSettings.svelte b/frontend/src/lib/components/InstanceSettings.svelte index 0c27a3c7c4..1476b9ad87 100644 --- a/frontend/src/lib/components/InstanceSettings.svelte +++ b/frontend/src/lib/components/InstanceSettings.svelte @@ -60,6 +60,14 @@ let requirePreexistingUserForOauth: boolean = $state(false) let initialValues: Record = $state({}) + + /// A setting its own component writes (it has to persist before acting on the cluster) is + /// already saved, so the baseline must move with it: otherwise Discard restores the value it + /// replaced, and a later bulk save sends that stale one back. + function markSettingSaved(key: string) { + initialValues[key] = + $values[key] === undefined ? undefined : JSON.parse(JSON.stringify($values[key])) + } let baseUrlIsFallback = $state(false) // Per-instance OAuth providers (Snowflake, ServiceNow, …): instance name // keyed by provider, used to build their per-instance connect_config URLs. @@ -730,6 +738,7 @@ secret_backend: ['token', 'client_secret', 'secret_access_key'], object_store_cache_config: ['secret_key', 'serviceAccountKey', 'accessKey'], custom_instance_pg_databases: ['user_pwd'], + external_instance_pg: ['password'], rsa_keys: ['private_key'], github_enterprise_app: ['private_key'] } @@ -1194,6 +1203,7 @@ {loading} {setting} {values} + {markSettingSaved} {version} {oauths} /> @@ -1213,6 +1223,7 @@ {loading} {setting} {values} + {markSettingSaved} {version} {oauths} warning={setting.key === 'base_url' && baseUrlIsFallback @@ -1228,6 +1239,7 @@ {@const licenseKeySetting = settings['Core'].find((s) => s.key === 'license_key')} {#if licenseKeySetting} closeDrawer?.()} {loading} @@ -1253,6 +1265,7 @@ {loading} {setting} {values} + {markSettingSaved} {version} {oauths} /> diff --git a/frontend/src/lib/components/instanceSettings.ts b/frontend/src/lib/components/instanceSettings.ts index 17d5ec9f6e..9701b74e30 100644 --- a/frontend/src/lib/components/instanceSettings.ts +++ b/frontend/src/lib/components/instanceSettings.ts @@ -68,6 +68,8 @@ export interface Setting { | 'otel' | 'otel_tracing_proxy' | 'secret_backend' + | 'external_instance_pg' + | 'instance_pg' | 'github_enterprise_app' | 'webhook_base_url' | 'ws_connectivity' @@ -599,6 +601,25 @@ export const settings: Record = { (Number.isInteger(Number(v)) && Number(v) >= 0 && Number(v) <= 3650) } ], + 'Managed Postgres': [ + { + label: 'External instance', + description: + 'A Postgres cluster Windmill administers for data tables and Ducklake catalogs, instead of its own database. It creates the databases there and manages the roles jobs connect as.', + key: 'external_instance_pg', + fieldType: 'external_instance_pg', + storage: 'setting', + ee_only: '' + }, + { + label: 'Windmill instance', + description: + "Windmill's own database as a data table and Ducklake substrate. On unless turned off here, and never available on cloud.", + key: 'instance_pg_disabled', + fieldType: 'instance_pg', + storage: 'setting' + } + ], 'Object Storage': [ { label: 'Instance object storage', @@ -1309,6 +1330,14 @@ export const instanceSettingsNavigationGroups = [ aiId: 'instance-settings-object-storage', aiDescription: 'Instance object storage settings', isEE: true + }, + { + id: 'managed_postgres', + label: 'Managed Postgres', + aiId: 'instance-settings-managed-postgres', + aiDescription: + 'Postgres substrates Windmill administers for data tables and Ducklake catalogs: its own database and an external cluster', + isEE: true } ] }, @@ -1428,6 +1457,7 @@ export const tabToCategoryMap: Record = { telemetry: 'Telemetry', secret_storage: 'Secret Storage', object_storage: 'Object Storage', + managed_postgres: 'Managed Postgres', jobs: 'Jobs', private_hub: 'Private Hub', github_enterprise_app: 'GitHub App', @@ -1464,6 +1494,7 @@ export const categoryToTabMap: Record = { Telemetry: 'telemetry', 'Secret Storage': 'secret_storage', 'Object Storage': 'object_storage', + 'Managed Postgres': 'managed_postgres', Jobs: 'jobs', 'Private Hub': 'private_hub', 'GitHub App': 'github_enterprise_app', diff --git a/frontend/src/lib/components/instanceSettings/ExternalInstancePgSettings.svelte b/frontend/src/lib/components/instanceSettings/ExternalInstancePgSettings.svelte new file mode 100644 index 0000000000..97b5494f44 --- /dev/null +++ b/frontend/src/lib/components/instanceSettings/ExternalInstancePgSettings.svelte @@ -0,0 +1,471 @@ + + +
+ {#if !$enterpriseLicense} + + {/if} + + + Windmill creates the databases for data tables and Ducklake catalogs on this cluster, and + manages the roles they connect as. The admin login below needs CREATEDB + and CREATEROLE, and is never handed to a job. + + +
+
+ + + +
+ +
+
+ Admin password + +
+ +
+ + SSL mode + + {#snippet text()} + Anything below verify-ca sends the managed roles' passwords to whichever server + answers. Windmill trusts the system roots plus the certificate below. + {/snippet} + + + + +
+ +
+ + + + {#if status} +
+ {#if status.configured} + + Configured · {status.database_count} database{status.database_count === 1 ? '' : 's'} + {:else} + + Not configured yet + {/if} +
+ {/if} +
+ + {#if loadError} + {loadError} + {/if} + + {#if status?.last_setup} + {@const report = status.last_setup} +
+
+ Last setup + {new Date(report.finished_at).toLocaleString()} +
+
+ {#each report.steps as step} +
+
+ {#if step.status === 'ok'} + + {:else if step.status === 'warning'} + + {:else} + + {/if} +
+
+ {step.name} + {step.message} +
+
+ {/each} +
+
+ {/if} + + {#if status?.configured} +
+ Databases on the cluster + + Only databases created here can back a data table or a Ducklake catalog, and one in use + cannot be dropped. + + + + + Name + Tag + Used by + + + + + {#each databaseEntries as [name, db]} + + {name} + {db.tag ?? '-'} + + {db.used_by_workspaces?.length ? db.used_by_workspaces.join(', ') : '-'} + + +
+
+ + + {:else} + + + No database yet. Create one below, then pick it in a workspace's data table or + Ducklake settings. + + + {/each} + + +
+ +
+ dataTable.database.resource_type, (resource_type) => { @@ -451,12 +537,20 @@ } } } + transformInputSelectedText={shortManagedInstanceLabel} id="database-type-select" - class="w-28" + class="w-36" />
- {#if dataTable.database.resource_type !== 'instance'} + {#if dataTable.database.resource_type === 'external_instance'} + + {:else if dataTable.database.resource_type !== 'instance'} diff --git a/frontend/src/lib/components/workspaceSettings/DucklakeSettings.svelte b/frontend/src/lib/components/workspaceSettings/DucklakeSettings.svelte index a0d29af434..f6929e0779 100644 --- a/frontend/src/lib/components/workspaceSettings/DucklakeSettings.svelte +++ b/frontend/src/lib/components/workspaceSettings/DucklakeSettings.svelte @@ -13,7 +13,7 @@ ducklakes: { name: string catalog: { - resource_type: 'postgresql' | 'mysql' | 'instance' + resource_type: 'postgresql' | 'mysql' | 'instance' | 'external_instance' resource_path?: string // Name of the database when resource_type is instance } storage: { @@ -74,7 +74,6 @@ GitFork, Plus, SettingsIcon, - Wrench } from 'lucide-svelte' import Button from '../common/button/Button.svelte' @@ -91,7 +90,7 @@ import { ScheduleService, SettingService, WorkspaceService } from '$lib/gen' import type { GetSettingsResponse } from '$lib/gen' - import { enterpriseLicense, userWorkspaces, workspaceStore } from '$lib/stores' + import { enterpriseLicense, superadmin, userWorkspaces, workspaceStore } from '$lib/stores' import { base } from '$app/paths' import Toggle from '../Toggle.svelte' import { sendUserToast } from '$lib/toast' @@ -105,9 +104,16 @@ import Popover from '../meltComponents/Popover.svelte' import TextInput from '../text_input/TextInput.svelte' import { slide } from 'svelte/transition' - import { isCustomInstanceDbEnabled, getUnusedInstanceDbName } from './utils.svelte' + import { + isCustomInstanceDbEnabled, + managedInstanceLabels, + shortManagedInstanceLabel, + getUnusedInstanceDbName + } from './utils.svelte' import { resource } from 'runed' + import { isCloudHosted } from '$lib/cloud' import CustomInstanceDbSelect from './CustomInstanceDbSelect.svelte' + import ExternalInstanceDbSelect from './ExternalInstanceDbSelect.svelte' import Label from '../Label.svelte' type Props = { @@ -140,8 +146,14 @@ ducklakeSettings.ducklakes.push({ name, catalog: { - resource_type: $isCustomInstanceDbEnabled ? 'instance' : 'postgresql', - resource_path: $isCustomInstanceDbEnabled ? defaultInstanceDbName() : undefined + // A new entry starts on a kind this instance actually offers, so it is never born + // on one the save would refuse. + resource_type: instanceAvailable + ? 'instance' + : externalInstanceConfigured && $superadmin + ? 'external_instance' + : 'postgresql', + resource_path: instanceAvailable ? defaultInstanceDbName() : undefined }, storage: { storage: undefined, @@ -170,6 +182,77 @@ const customInstanceDbs = resource([() => $workspaceStore], SettingService.listCustomInstanceDbs) + // Superadmin-only endpoints, and the kind is theirs to pick: a workspace admin never loads + // them and sees the option disabled instead of an empty picker. + const externalInstanceStatus = resource([() => $superadmin], ([isSuperadmin]) => + isSuperadmin ? SettingService.getExternalInstancePgStatus() : Promise.resolve(undefined) + ) + const externalInstanceDbs = resource([() => $superadmin], ([isSuperadmin]) => + isSuperadmin ? SettingService.listExternalInstancePgDatabases() : Promise.resolve({}) + ) + let externalInstanceConfigured = $derived(externalInstanceStatus.current?.configured === true) + // Superadmin-only like the ones above, and absent means on. + const instancePgDisabled = resource([() => $superadmin], ([isSuperadmin]) => + isSuperadmin + ? SettingService.getGlobal({ key: 'instance_pg_disabled' }).catch(() => undefined) + : Promise.resolve(undefined) + ) + // Both substrates answer only to a superadmin, so nobody else can be told whether one is on + // offer: they see a managed kind only where an entry already sits on it. + let instancePossible = $derived( + !!$superadmin && !isCloudHosted() && !instancePgDisabled.current + ) + let instanceAvailable = $derived(instancePossible) + + // A kind already saved stays listed whatever the instance offers now. + // Qualified per form, not per row: see DataTableSettings. + let anyInstanceLake = $derived( + ducklakeSettings.ducklakes.some((d) => d.catalog.resource_type === 'instance') + ) + let anyExternalLake = $derived( + ducklakeSettings.ducklakes.some((d) => d.catalog.resource_type === 'external_instance') + ) + function catalogItems(current: string | undefined) { + const showInstance = instancePossible || current === 'instance' + const showExternal = + (externalInstanceConfigured && !!$superadmin) || current === 'external_instance' + const labels = managedInstanceLabels( + instancePossible || anyInstanceLake, + (externalInstanceConfigured && !!$superadmin) || anyExternalLake + ) + const items: { value: string; label: string; disabled?: boolean; subtitle?: string }[] = [ + { value: 'postgresql', label: 'Postgres Resource' }, + { value: 'mysql', label: 'MySQL Resource' } + ] + if (showInstance) { + items.push({ + value: 'instance', + label: labels.instance, + disabled: !instanceAvailable, + subtitle: instanceAvailable + ? undefined + : !$superadmin + ? 'Superadmin only' + : isCloudHosted() + ? 'Not available on cloud' + : "Windmill's database is disabled" + }) + } + if (showExternal) { + items.push({ + value: 'external_instance', + label: labels.external, + disabled: !externalInstanceConfigured || !$superadmin, + subtitle: !$superadmin + ? 'Superadmin only' + : externalInstanceConfigured + ? undefined + : 'No external cluster configured' + }) + } + return items + } + async function onSave() { try { if ( @@ -245,15 +328,13 @@ secondaryStorageNames.refresh() }) - let tableHeadNames = ['Name', 'Catalog', 'Workspace storage', 'Maintenance', '', ''] as const + let tableHeadNames = ['Name', 'Catalog', 'Workspace storage', '', ''] as const let tableHeadTooltips: Partial> = { Name: "Ducklakes are referenced in DuckDB scripts with the ATTACH 'ducklake://name' AS dl; syntax", Catalog: 'Ducklake needs an SQL database to store metadata about the data', 'Workspace storage': - 'Where the data is actually stored, in parquet format. You need to configure a workspace storage first', - Maintenance: - 'Scheduled snapshot expiry, small-file compaction and orphaned-file cleanup, run as jobs on a managed per-lake schedule (EE)' + 'Where the data is actually stored, in parquet format. You need to configure a workspace storage first' } let confirmationModal = createAsyncConfirmationModal() @@ -283,10 +364,9 @@ This workspace is a fork, and these settings are its own copy. Lakes marked isolated read the parent's tables through defer views and write to a fork-scoped namespace that is cleaned up when the fork is deleted. Lakes marked - shared with parent read and write the parent's physical - lake directly — editing their catalog or storage here repoints the shared lake for this - fork's jobs. The choice is made per lake when the fork is created and cannot be changed - here. + shared with parent read and write the parent's physical lake + directly — editing their catalog or storage here repoints the shared lake for this fork's jobs. + The choice is made per lake when the fork is created and cannot be changed here.
{/if} @@ -359,8 +439,8 @@ isolated - Writes go to a fork-scoped namespace; reads of tables not yet materialized in - this fork defer to the parent. Deleting the fork cleans the namespace up. + Writes go to a fork-scoped namespace; reads of tables not yet materialized in this + fork defer to the parent. Deleting the fork cleans the namespace up. {/if}
@@ -373,17 +453,13 @@ Use Windmill's PostgreSQL instance as a catalog + {:else if ducklake.catalog.resource_type === 'external_instance'} + + Use a database Windmill manages on the external PostgreSQL cluster as a catalog + {/if} (value = i)} + placeholder="Search or create..." + showPlaceholderOnOpen + {items} + id="external-instance-db-select" + disabled={!$superadmin} + noItemsMsg="Start typing to create a new database" + > + {#snippet endSnippet({ item })} + {@render sharedWorkspacesWarning(item.value)} + {/snippet} + + {#if value} +
+ {#if unknownName} + + {:else if $superadmin} + {@render sharedWorkspacesWarning(value)} +
+ {/if} +
+ {/if} +
+ +{#snippet sharedWorkspacesWarning(dbname: string)} + {@const others = otherWorkspaces(dbname)} + {#if others.length > 0} + + + {#snippet text()} + This database is also used by workspace{others.length > 1 ? 's' : ''} + {others.join(', ')}. Any data written here will be shared + with {others.length > 1 ? 'them' : 'it'}. + {/snippet} + + {/if} +{/snippet} diff --git a/frontend/src/lib/components/workspaceSettings/InstanceRolesButton.svelte b/frontend/src/lib/components/workspaceSettings/InstanceRolesButton.svelte index 9932495fce..6072cdad3f 100644 --- a/frontend/src/lib/components/workspaceSettings/InstanceRolesButton.svelte +++ b/frontend/src/lib/components/workspaceSettings/InstanceRolesButton.svelte @@ -2,14 +2,18 @@ import { Badge, Button, Drawer, DrawerContent } from '../common' import { Users } from 'lucide-svelte' import DataTableRolesSection from './DataTableRolesSection.svelte' + import type { DatatableRoleCluster } from '$lib/gen' let { hideTrigger = false, + cluster, onChanged, unavailable }: { /** Mount the drawer without its button, for a caller that opens it with `open()`. */ hideTrigger?: boolean + /** Whose catalog to manage. A role is a login on one cluster. */ + cluster?: DatatableRoleCluster /** Called after every change to the instance roles. */ onChanged?: () => void /** Why the roles cannot be managed here: the button stays, disabled with this reason, so the @@ -49,13 +53,13 @@ drawer?.closeDrawer()} - tooltip="A data table role is a real Postgres login on this instance, shared by every instance database. A job that names one connects as it, and Postgres decides what it may touch. Which people may use a role on a given data table, and what it may do there, is set per data table, in its roles drawer." + tooltip="A data table role is a real Postgres login on the cluster it belongs to, shared by every database Windmill manages there. A job that names one connects as it, and Postgres decides what it may touch. Which people may use a role on a given data table, and what it may do there, is set per data table, in its roles drawer." > {#snippet titleExtra()} Beta {/snippet} {#key openCount} - + {/key} diff --git a/frontend/src/lib/components/workspaceSettings/addDataTableModel.test.ts b/frontend/src/lib/components/workspaceSettings/addDataTableModel.test.ts index cb42d73075..9461a7b04b 100644 --- a/frontend/src/lib/components/workspaceSettings/addDataTableModel.test.ts +++ b/frontend/src/lib/components/workspaceSettings/addDataTableModel.test.ts @@ -23,6 +23,7 @@ const getSettingsMock = vi.fn() const editDataTableConfigMock = vi.fn() const testDataTableConnectionMock = vi.fn() const setupCustomInstanceDbMock = vi.fn() +const createExternalInstanceDbMock = vi.fn() vi.mock('$lib/gen', () => ({ VariableService: { existsVariable: (...a: any[]) => existsVariableMock(...a), @@ -36,7 +37,10 @@ vi.mock('$lib/gen', () => ({ createResource: vi.fn(), updateResource: vi.fn() }, - SettingService: { setupCustomInstanceDb: (...a: any[]) => setupCustomInstanceDbMock(...a) }, + SettingService: { + setupCustomInstanceDb: (...a: any[]) => setupCustomInstanceDbMock(...a), + createExternalInstancePgDatabase: (...a: any[]) => createExternalInstanceDbMock(...a) + }, WorkspaceService: { getSettings: (...a: any[]) => getSettingsMock(...a), editDataTableConfig: (...a: any[]) => editDataTableConfigMock(...a), @@ -239,6 +243,48 @@ describe('runSetup rolling the instance row back', () => { }) }) +// A database on the external cluster belongs to whoever created it, and the endpoint refuses a +// name it already holds. Skipping the create on any registered name is how a run ends up +// attached to somebody else's data without the sharing warning the existing-database branch +// shows -- so only the databases this run made may make a retry skip it. +describe('runSetup creating a database on the external cluster', () => { + function creatingExternal(): WizardState { + const state = newWizardState({ name: 'main', projectName: 'x', folder: 'f/team' }) + state.provider = 'external_instance' + state.external = { mode: 'create', dbName: 'dt_new' } + return state + } + const externalDeps = (createdExternalDbs: string[] = []) => + ({ + workspace: 'w', + onProgress: () => {}, + claims: noClaims, + username: 'alice', + createdProjects: [], + createdExternalDbs + }) as any + + beforeEach(() => { + vi.clearAllMocks() + getSettingsMock.mockResolvedValue({ datatable: { datatables: {} } }) + editDataTableConfigMock.mockResolvedValue(undefined) + testDataTableConnectionMock.mockResolvedValue({ can_create_table: true }) + }) + + it('creates the database on a first attempt', async () => { + const result = await runSetup(creatingExternal(), externalDeps()) + expect(result.ok).toBe(true) + expect(createExternalInstanceDbMock).toHaveBeenCalledTimes(1) + expect(result.createdExternalDbs).toContain('dt_new') + }) + + it('skips the create only for a database it made itself', async () => { + const result = await runSetup(creatingExternal(), externalDeps(['dt_new'])) + expect(result.ok).toBe(true) + expect(createExternalInstanceDbMock).not.toHaveBeenCalled() + }) +}) + // The fields are the connection; a connection string is a way of writing one down. Reading the // resource back out of the string is what let a URI grammar gap change what got saved. describe('newResourceParts', () => { diff --git a/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts b/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts index 2625786991..073d496d0c 100644 --- a/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts +++ b/frontend/src/lib/components/workspaceSettings/addDataTableModel.ts @@ -42,7 +42,7 @@ import { type SupabaseProject } from './supabaseProvisioning' -export type Provider = 'supabase' | 'instance' | 'resource' +export type Provider = 'supabase' | 'instance' | 'external_instance' | 'resource' export type WizardState = { step: 1 | 2 | 3 @@ -61,6 +61,8 @@ export type WizardState = { connectionMode: SupabaseConnectionMode } instance: { mode: 'existing' | 'create'; dbName: string | undefined } + /** Same shape as `instance`, on the Postgres cluster Windmill administers elsewhere. */ + external: { mode: 'existing' | 'create'; dbName: string | undefined } /** * One list: the workspace's Postgres resources, plus the one about to exist. A * connection string is not an alternative to a resource, it is how one is written -- @@ -107,6 +109,7 @@ export function newWizardState(defaults: { connectionMode: 'session' }, instance: { mode: 'create', dbName: undefined }, + external: { mode: 'create', dbName: undefined }, own: { resourcePath: undefined, creating: false, @@ -137,6 +140,7 @@ export function intentComplete(state: WizardState): boolean { : !!state.supabase.project && !!state.supabase.password } if (state.provider === 'instance') return !!state.instance.dbName?.trim() + if (state.provider === 'external_instance') return !!state.external.dbName?.trim() if (!state.own.creating) return !!state.own.resourcePath // Text that will not parse leaves the fields on their last good values, which is what makes // it correctable -- but the connection on screen is then not the one they describe, and @@ -307,6 +311,7 @@ export type RunStepKey = | 'create_project' | 'wait_healthy' | 'save_credentials' + | 'create_external' | 'setup_instance' | 'check' @@ -331,6 +336,13 @@ export function plan(state: WizardState): { key: RunStepKey; title: string }[] { key: 'setup_instance', title: `Setting up ${state.instance.dbName} in the Windmill database` }) + } else if (state.provider === 'external_instance') { + if (state.external.mode === 'create') { + steps.push({ + key: 'create_external', + title: `Creating ${state.external.dbName} on the external cluster` + }) + } } else if (state.own.creating) { steps.push({ key: 'save_credentials', title: `Saving the connection to ${path}` }) } @@ -362,6 +374,16 @@ export type RunDeps = { * is really there, since the name is also recorded when a create could not be confirmed. */ createdProjects: CreatedProject[] + /** + * The databases earlier attempts in this session created on the external cluster. Only these + * make a retry skip the create: any other registered name is somebody else's database, and + * attaching to it silently would share their data without the warning the existing-database + * branch shows. + */ + createdExternalDbs?: string[] + /** Called once a database lands on the external cluster, so the caller's registry — which + * names the next run's default and validates it — is not a page-load-old view. */ + onExternalDbsChanged?: () => Promise /** * What earlier attempts in this session wrote, and this one may therefore write over again. * The pre-flight checks the names are free, but the Supabase branch then spends minutes @@ -391,6 +413,8 @@ export type RunResult = { * later attempt may write over it. */ createdProjects: CreatedProject[] + /** The external-cluster databases this session created, kept for the same reason. */ + createdExternalDbs: string[] /** What this run holds now, for the next attempt to be given back. */ claims: Claims } @@ -410,6 +434,17 @@ async function exists(kind: 'variable' | 'resource', workspace: string, path: st : ResourceService.existsResource({ workspace, path }) } +/** + * What identifies the database a row points at. The kind belongs in it: `instance` and + * `external_instance` are different databases that may carry the same name, so a row repointed + * from one to the other while this run probes must not read as the row this run wrote. + */ +function rowMark(database: { resource_type?: string; resource_path?: string } | undefined) { + return database?.resource_path === undefined + ? undefined + : `${database.resource_type ?? ''}:${database.resource_path}` +} + /** * Adds the data table to the workspace config, once everything it points at exists. * `edit_datatable_config` replaces the whole map, so the rest is read back and sent with @@ -419,16 +454,16 @@ async function writeRow( deps: RunDeps, claims: Claims, name: string, - database: { resource_type: 'postgresql' | 'instance'; resource_path: string } + database: { + resource_type: 'postgresql' | 'instance' | 'external_instance' + resource_path: string + } ): Promise { const settings = await WorkspaceService.getSettings({ workspace: deps.workspace }) const datatables: Record = { ...(settings.datatable?.datatables ?? {}) } // Free when the pre-flight looked, taken by the time we write: repointing it here would // silently hand another admin's data table a database they never chose. - if ( - datatables[name] && - !stillOurs(claims, 'row', name, datatables[name]?.database?.resource_path) - ) { + if (datatables[name] && !stillOurs(claims, 'row', name, rowMark(datatables[name]?.database))) { throw new Error( `A data table called ${name} was created while this setup was running. Choose another name and try again.` ) @@ -438,7 +473,7 @@ async function writeRow( workspace: deps.workspace, requestBody: { settings: { datatables }, renames: [], deleted_datatables: [] } }) - return claim(claims, 'row', name, database.resource_path) + return claim(claims, 'row', name, rowMark(database)!) } /** @@ -455,7 +490,7 @@ async function removeRow(deps: RunDeps, claims: Claims, name: string): Promise { if (!createdProjects.some((p) => p.path === at)) @@ -595,7 +631,8 @@ export async function runSetup(state: WizardState, deps: RunDeps): Promise p.path === path) const instanceName = state.instance.dbName?.trim() ?? '' + const externalName = state.external.dbName?.trim() ?? '' let project = state.supabase.project let resourcePath = @@ -740,6 +778,18 @@ export async function runSetup(state: WizardState, deps: RunDeps): Promise