From 7c16d482fd1d75116481ac32b39c801881916f5e Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 17 Sep 2026 16:34:14 +0200 Subject: [PATCH 1/2] fix(datatables): refuse fork cleanup of an external database another workspace uses Co-Authored-By: Claude Opus 5 (1M context) --- backend/ee-repo-ref.txt | 2 +- backend/windmill-api-settings/src/lib.rs | 2 +- .../windmill-api-workspaces/src/workspaces_extra.rs | 5 ++++- backend/windmill-common/src/external_instance_pg.rs | 13 +++++++++---- .../windmill-common/src/external_instance_pg_oss.rs | 2 +- 5 files changed, 16 insertions(+), 8 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 7532fcaed6..6f1b5b251b 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -f920b69b68d0ce2e907774c75573ffd4d9f916ca \ No newline at end of file +3bb6bfdadf005e6729dd17e5dcace9ac24bc244f \ No newline at end of file diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 3bf04d1ef7..5fee9ee99a 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -1887,7 +1887,7 @@ async fn drop_external_instance_pg_database( require_super_admin(&db, &authed).await?; // A data table naming a dropped database fails on every job, far from the drop that caused it. windmill_common::external_instance_pg::drop_external_instance_database_unchecked( - &db, &dbname, true, + &db, &dbname, None, ) .await?; windmill_audit::audit_oss::audit_log( diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index 975c178982..abb2e60739 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -1421,8 +1421,11 @@ pub async fn drop_forked_datatable_databases( let dropped = if database.resource_type == windmill_common::workspaces::DataTableCatalogResourceType::ExternalInstance { + // Its own entry still names the copy; another workspace's never should. windmill_common::external_instance_pg::drop_external_instance_database_unchecked( - &db, db_to_drop, false, + &db, + db_to_drop, + Some(&w_id), ) .await } else { diff --git a/backend/windmill-common/src/external_instance_pg.rs b/backend/windmill-common/src/external_instance_pg.rs index 957d908ffc..38773fc5d7 100644 --- a/backend/windmill-common/src/external_instance_pg.rs +++ b/backend/windmill-common/src/external_instance_pg.rs @@ -215,19 +215,20 @@ pub async fn create_external_instance_database_unchecked( } /// Drop `dbname` from the external cluster: only a database Windmill registered creating, and still -/// carries the mark it set there. With `refuse_if_used`, refuse while a data table names it. +/// carries the mark it set there. Refused while a data table names it, except one in +/// `usage_allowed_in`: the fork whose own copy is being cleaned up. /// /// 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( db: &DB, dbname: &str, - refuse_if_used: bool, + usage_allowed_in: Option<&str>, ) -> Result<()> { crate::external_instance_pg_oss::drop_external_instance_database_unchecked( db, dbname, - refuse_if_used, + usage_allowed_in, ) .await } @@ -273,7 +274,8 @@ pub async fn ensure_external_instance_database_registered( /// 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. +/// 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>, @@ -316,6 +318,9 @@ pub async fn write_external_instance_pg_setting( /// [`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, diff --git a/backend/windmill-common/src/external_instance_pg_oss.rs b/backend/windmill-common/src/external_instance_pg_oss.rs index 7f6168878e..a04a8c9cd6 100644 --- a/backend/windmill-common/src/external_instance_pg_oss.rs +++ b/backend/windmill-common/src/external_instance_pg_oss.rs @@ -73,7 +73,7 @@ mod ce { pub(crate) async fn drop_external_instance_database_unchecked( _db: &DB, _dbname: &str, - _refuse_if_used: bool, + _usage_allowed_in: Option<&str>, ) -> Result<()> { Err(unavailable()) } From 48f026eb0f363afcad29444d96e330366ddbb9ff Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 17 Sep 2026 16:35:45 +0200 Subject: [PATCH 2/2] 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) --- .../20260917133824_datatable_role_cluster.down.sql | 11 +++++++++++ cli/src/commands/datatable/datatable.ts | 6 ++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/backend/migrations/20260917133824_datatable_role_cluster.down.sql b/backend/migrations/20260917133824_datatable_role_cluster.down.sql index 2fb7bcc8a8..379f1e3d34 100644 --- a/backend/migrations/20260917133824_datatable_role_cluster.down.sql +++ b/backend/migrations/20260917133824_datatable_role_cluster.down.sql @@ -5,6 +5,17 @@ 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); 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, }, };