diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 5a19b73d1c..4dff91e5d2 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -15845,6 +15845,7 @@ dependencies = [ "reqwest 0.13.4", "serde", "serde_json", + "serial_test", "sqlx", "tempfile", "tokio", diff --git a/backend/src/main.rs b/backend/src/main.rs index f84812151d..118795bd17 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -1944,6 +1944,13 @@ async fn process_notify_event( if let Err(e) = reload_license_key(&db.into()).await { tracing::error!("Failed to reload license key: {e:#}"); } + // The worker-group cache override is Enterprise-only, and nothing else + // re-reads the plan for it: the periodic settings pass runs ahead of + // reload_license_key, so it would see the plan this event just replaced. + #[cfg(feature = "parquet")] + if worker_mode { + crate::monitor::reload_cache_object_store_override_with_retry(db).await; + } } DEFAULT_TAGS_PER_WORKSPACE_SETTING => { if let Err(e) = load_tag_per_workspace_enabled(db).await { diff --git a/backend/src/monitor.rs b/backend/src/monitor.rs index 9aae2cca06..867d2c8520 100644 --- a/backend/src/monitor.rs +++ b/backend/src/monitor.rs @@ -398,6 +398,8 @@ pub async fn initial_load( additional_python_paths: None, pip_local_dependencies: None, native_mode, + // an agent worker never reads its group's config, only its token + object_store_cache_config: None, })); } } @@ -5294,6 +5296,7 @@ pub async fn reload_worker_config(db: &DB, tx: KillpillSender, kill_if_change: b .dedicated_workers .as_ref() .is_some_and(|dws| !dws.is_empty()); + if **wc != config || has_dedicated { if kill_if_change { if has_dedicated @@ -5341,6 +5344,37 @@ pub async fn reload_worker_config(db: &DB, tx: KillpillSender, kill_if_change: b store_pull_query(&config).await; WORKER_CONFIG.store(std::sync::Arc::new(config)); } + + // After the store, so a retry that wakes mid-build reads the config being applied + // rather than the one it replaced. Unconditional rather than gated on the value + // changing, so that a pass triggered by anything else — a license-plan change, most + // of all — still re-evaluates the entitlement. + #[cfg(feature = "parquet")] + reload_cache_object_store_override_with_retry(db).await; + } +} + +/// Apply this worker group's dependency-cache object store, retrying once shortly after a build +/// that failed for a reason that may pass — the periodic settings reload behind it is 12h apart, +/// which is a long time for a whole group to cache nothing but locally. +#[cfg(feature = "parquet")] +pub async fn reload_cache_object_store_override_with_retry(db: &DB) { + let settings = WORKER_CONFIG.load().object_store_cache_config.clone(); + if matches!( + windmill_object_store::reload_cache_object_store_override(db, settings).await, + ObjectStoreReload::Later + ) { + let db = db.clone(); + tokio::spawn(async move { + tokio::time::sleep(Duration::from_secs(10)).await; + if windmill_object_store::cache_object_store_override_failed().await { + // Re-read rather than reuse: the group config may have changed while we slept, + // and installing the settings this retry was born with would pin the worker to a + // store the group no longer asks for. + let settings = WORKER_CONFIG.load().object_store_cache_config.clone(); + windmill_object_store::reload_cache_object_store_override(&db, settings).await; + } + }); } } diff --git a/backend/windmill-api-configs/src/lib.rs b/backend/windmill-api-configs/src/lib.rs index 7ad99996d5..7598021f00 100644 --- a/backend/windmill-api-configs/src/lib.rs +++ b/backend/windmill-api-configs/src/lib.rs @@ -59,14 +59,116 @@ struct Config { config: serde_json::Value, } +/// Credential-bearing fields across the `ObjectSettings` variants, which are flattened into a +/// single object by the `type` tag. +const OBJECT_STORE_SECRET_KEYS: &[&str] = + &["access_key", "secret_key", "accessKey", "serviceAccountKey"]; + +/// What an obfuscated read shows a caller who may not see the real credential. +const OBJECT_STORE_SECRET_MASK: &str = "*****"; + +/// Blank the secrets in one worker-group config, in place. +/// +/// Worker-group configs are instance-global and expose `env_vars_static` and the bucket +/// credentials of `object_store_cache_config`; a job token (capped at workspace admin) gets this +/// view even when its identity is a superadmin, as does a devops user who is not an instance +/// admin. See `is_instance_admin` (GHSA-hfh4-cx4h-3fcr). Every route that returns a worker-group +/// config must go through here — a single unobfuscated read hands over the whole bucket. +fn obfuscate_worker_config(config: &mut serde_json::Value) { + let Some(config) = config.as_object_mut() else { + return; + }; + if let Some(env_vars) = config + .get_mut("env_vars_static") + .and_then(|v| v.as_object_mut()) + { + for (_, value) in env_vars.iter_mut() { + // the value is a string, so to_string() it and take -2 to drop the quotes + *value = serde_json::json!("*".repeat(value.to_string().len().saturating_sub(2))); + } + } + if let Some(store) = config + .get_mut("object_store_cache_config") + .and_then(|v| v.as_object_mut()) + { + for key in OBJECT_STORE_SECRET_KEYS { + if let Some(secret) = store.get_mut(*key) { + *secret = serde_json::json!(OBJECT_STORE_SECRET_MASK); + } + } + } +} + +/// Put back the credentials behind [`OBJECT_STORE_SECRET_MASK`]. A devops user who is not an +/// instance admin edits the group from the obfuscated view, so a plain save would otherwise +/// store the mask as the secret and take the group's dependency cache offline — silently, since +/// a worker that cannot build its override just falls back to caching on local disk. +async fn restore_masked_object_store_secrets( + db: &DB, + name: &str, + config: &mut serde_json::Value, +) -> error::Result<()> { + if !has_masked_object_store_secret(config) { + return Ok(()); + } + let stored = sqlx::query_as!( + Config, + "SELECT name, config FROM config WHERE name = $1", + name + ) + .fetch_optional(db) + .await? + .map(|c| c.config); + restore_object_store_secrets(config, stored.as_ref()); + Ok(()) +} + +fn has_masked_object_store_secret(config: &serde_json::Value) -> bool { + let Some(store) = config.get("object_store_cache_config") else { + return false; + }; + OBJECT_STORE_SECRET_KEYS + .iter() + .any(|k| store.get(k).and_then(|v| v.as_str()) == Some(OBJECT_STORE_SECRET_MASK)) +} + +/// The half of [`restore_masked_object_store_secrets`] after the read. +fn restore_object_store_secrets( + config: &mut serde_json::Value, + stored: Option<&serde_json::Value>, +) { + let stored = stored + .and_then(|c| c.get("object_store_cache_config")) + .and_then(|v| v.as_object()) + .cloned(); + let Some(store) = config + .get_mut("object_store_cache_config") + .and_then(|v| v.as_object_mut()) + else { + return; + }; + for key in OBJECT_STORE_SECRET_KEYS { + if store.get(*key).and_then(|v| v.as_str()) != Some(OBJECT_STORE_SECRET_MASK) { + continue; + } + match stored.as_ref().and_then(|s| s.get(*key)) { + Some(secret) => store.insert(key.to_string(), secret.clone()), + // Nothing to put back: drop the mask rather than store it. + None => store.remove(*key), + }; + } +} + async fn list_worker_groups( authed: ApiAuthed, Extension(db): Extension, ) -> error::JsonResult> { - let mut configs_raw = - sqlx::query_as!(Config, "SELECT name, config FROM config WHERE name LIKE 'worker__%'") - .fetch_all(&db) - .await?; + let mut configs_raw = sqlx::query_as!( + Config, + "SELECT name, config FROM config WHERE name LIKE 'worker__%'" + ) + .fetch_all(&db) + .await?; // Remove the 'worker__' prefix from all config names for config in configs_raw.iter_mut() { if let Some(name) = &config.name { @@ -75,44 +177,12 @@ async fn list_worker_groups( } } } - // Worker-group configs are instance-global and expose env_vars_static (may hold - // secrets); a job token (capped at workspace admin) gets the obfuscated view even - // when its identity is a superadmin. See is_instance_admin (GHSA-hfh4-cx4h-3fcr). - let configs = if !is_instance_admin(&authed) { - let mut obfuscated_configs: Vec = vec![]; - for config in configs_raw { - let config_value_opt = config.config.as_object().map(|obj| obj.to_owned()); - if let Some(mut config_value) = config_value_opt { - if let Some(env_var_map) = config_value - .get("env_vars_static") - .map(|obj| obj.as_object()) - .flatten() - { - let mut new_env_var_map: serde_json::Map = - serde_json::Map::new(); - for (key, value) in env_var_map { - new_env_var_map.insert( - key.to_owned(), - // we know the value is a string here, so we to_string() it and take -2 to remove the quotes - serde_json::json!("*".repeat(value.to_string().len() - 2)), - ); - } - config_value.insert( - "env_vars_static".to_string(), - serde_json::Value::Object(new_env_var_map), - ); - } - obfuscated_configs.push(Config { - name: config.name, - config: serde_json::Value::Object(config_value), - }) - } + if !is_instance_admin(&authed) { + for config in configs_raw.iter_mut() { + obfuscate_worker_config(&mut config.config); } - obfuscated_configs - } else { - configs_raw - }; - Ok(Json(configs)) + } + Ok(Json(configs_raw)) } async fn get_config( @@ -122,10 +192,20 @@ async fn get_config( ) -> error::JsonResult> { require_devops_role(&db, &authed).await?; - let config = sqlx::query_as!(Config, "SELECT name, config FROM config WHERE name = $1", name) - .fetch_optional(&db) - .await? - .map(|c| c.config); + let mut config = sqlx::query_as!( + Config, + "SELECT name, config FROM config WHERE name = $1", + name + ) + .fetch_optional(&db) + .await? + .map(|c| c.config); + + if !is_instance_admin(&authed) { + if let Some(config) = config.as_mut() { + obfuscate_worker_config(config); + } + } Ok(Json(config)) } @@ -134,10 +214,14 @@ async fn update_config( Path(name): Path, Extension(db): Extension, authed: ApiAuthed, - Json(config): Json, + Json(mut config): Json, ) -> error::Result { require_devops_role(&db, &authed).await?; + if name.starts_with("worker__") { + restore_masked_object_store_secrets(&db, &name, &mut config).await?; + } + #[cfg(not(feature = "enterprise"))] let config = if name.starts_with("worker__") { // In CE, only allow setting worker_tags, cache_clear, init_bash, and native_mode @@ -321,9 +405,14 @@ async fn list_configs( Extension(db): Extension, ) -> error::JsonResult> { require_devops_role(&db, &authed).await?; - let configs = sqlx::query_as!(Config, "SELECT name, config FROM config") + let mut configs = sqlx::query_as!(Config, "SELECT name, config FROM config") .fetch_all(&db) .await?; + if !is_instance_admin(&authed) { + for config in configs.iter_mut() { + obfuscate_worker_config(&mut config.config); + } + } Ok(Json(configs)) } @@ -414,3 +503,49 @@ async fn list_all_dedicated_with_deps( Ok(Json(result)) } + +#[cfg(test)] +mod tests { + use super::*; + + /// The mask an obfuscated read hands out must never be storable as the credential itself: + /// a devops user who is not an instance admin edits the group from that view, and a worker + /// that cannot build its override degrades to a local-only cache without failing a job, so + /// the breakage would go unnoticed. + #[test] + fn masked_secrets_survive_a_save_from_the_obfuscated_view() { + let stored = serde_json::json!({ + "object_store_cache_config": { + "type": "S3", "bucket": "cache", "access_key": "AKIA", "secret_key": "s3cr3t" + }, + "env_vars_static": { "TOKEN": "hunter2" }, + }); + + let mut shown = stored.clone(); + obfuscate_worker_config(&mut shown); + let store = &shown["object_store_cache_config"]; + assert_eq!(store["secret_key"], OBJECT_STORE_SECRET_MASK); + assert_eq!(store["access_key"], OBJECT_STORE_SECRET_MASK); + assert_eq!(store["bucket"], "cache"); + assert_ne!(shown["env_vars_static"]["TOKEN"], "hunter2"); + + let mut saved = shown.clone(); + saved["object_store_cache_config"]["bucket"] = serde_json::json!("other"); + restore_object_store_secrets(&mut saved, Some(&stored)); + let store = &saved["object_store_cache_config"]; + assert_eq!(store["secret_key"], "s3cr3t"); + assert_eq!(store["access_key"], "AKIA"); + assert_eq!(store["bucket"], "other"); + } + + #[test] + fn a_mask_with_nothing_behind_it_is_dropped_rather_than_stored() { + let mut saved = serde_json::json!({ + "object_store_cache_config": { "type": "S3", "secret_key": OBJECT_STORE_SECRET_MASK } + }); + restore_object_store_secrets(&mut saved, None); + assert!(saved["object_store_cache_config"] + .get("secret_key") + .is_none()); + } +} diff --git a/backend/windmill-common/src/instance_config.rs b/backend/windmill-common/src/instance_config.rs index 7ea6dbe5ae..c3060e9a89 100644 --- a/backend/windmill-common/src/instance_config.rs +++ b/backend/windmill-common/src/instance_config.rs @@ -916,6 +916,14 @@ pub struct WorkerGroupConfig { pub autoscaling: Option, #[serde(skip_serializing_if = "Option::is_none")] pub native_mode: Option, + /// Object store this group's dependency cache uses instead of the instance one. Same shape + /// as the instance `object_store_cache_config`. + #[serde(skip_serializing_if = "Option::is_none")] + #[cfg_attr( + feature = "instance_config_schema", + schemars(schema_with = "opaque_json_schema") + )] + pub object_store_cache_config: Option, /// Catch-all for fields not yet covered by typed fields. #[serde(flatten)] diff --git a/backend/windmill-common/src/worker.rs b/backend/windmill-common/src/worker.rs index 84284759c6..ebb0a723ea 100644 --- a/backend/windmill-common/src/worker.rs +++ b/backend/windmill-common/src/worker.rs @@ -392,6 +392,7 @@ lazy_static::lazy_static! { pip_local_dependencies: Default::default(), env_vars: Default::default(), native_mode: false, + object_store_cache_config: Default::default(), }); pub static ref WORKER_PULL_QUERIES: arc_swap::ArcSwap> = arc_swap::ArcSwap::from_pointee(vec![]); @@ -2343,6 +2344,7 @@ pub async fn load_worker_config( .or_else(|| load_additional_python_paths_from_env()), env_vars: resolved_env_vars, native_mode, + object_store_cache_config: config.object_store_cache_config, }) } @@ -2432,6 +2434,7 @@ pub struct WorkerConfigOpt { pub env_vars_static: Option>, pub env_vars_allowlist: Option>, pub native_mode: Option, + pub object_store_cache_config: Option, } impl Default for WorkerConfigOpt { @@ -2450,6 +2453,7 @@ impl Default for WorkerConfigOpt { env_vars_static: Default::default(), env_vars_allowlist: Default::default(), native_mode: Default::default(), + object_store_cache_config: Default::default(), } } } @@ -2468,12 +2472,18 @@ pub struct WorkerConfig { pub pip_local_dependencies: Option>, pub env_vars: HashMap, pub native_mode: bool, + /// Object store this group's dependency cache uses instead of the instance one, as stored + /// in the group config. Raw JSON: `windmill-common` cannot depend on the object store crate + /// that parses it, and comparing the raw value is what tells a reload the store changed. + pub object_store_cache_config: Option, } impl std::fmt::Debug for WorkerConfig { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "WorkerConfig {{ worker_tags: {:?}, priority_tags_sorted: {:?}, dedicated_worker: {:?}, dedicated_workers: {:?}, init_bash: {:?}, periodic_script_bash: {:?}, periodic_script_interval_seconds: {:?}, cache_clear: {:?}, additional_python_paths: {:?}, pip_local_dependencies: {:?}, env_vars: {:?}, native_mode: {:?} }}", - self.worker_tags, self.priority_tags_sorted, self.dedicated_worker, self.dedicated_workers, self.init_bash, self.periodic_script_bash, self.periodic_script_interval_seconds, self.cache_clear, self.additional_python_paths, self.pip_local_dependencies, self.env_vars.iter().map(|(k, v)| format!("{}: {}{} ({} chars)", k, &v[..3.min(v.len())], "***", v.len())).collect::>().join(", "), self.native_mode) + write!(f, "WorkerConfig {{ worker_tags: {:?}, priority_tags_sorted: {:?}, dedicated_worker: {:?}, dedicated_workers: {:?}, init_bash: {:?}, periodic_script_bash: {:?}, periodic_script_interval_seconds: {:?}, cache_clear: {:?}, additional_python_paths: {:?}, pip_local_dependencies: {:?}, env_vars: {:?}, native_mode: {:?}, object_store_cache_config: {} }}", + self.worker_tags, self.priority_tags_sorted, self.dedicated_worker, self.dedicated_workers, self.init_bash, self.periodic_script_bash, self.periodic_script_interval_seconds, self.cache_clear, self.additional_python_paths, self.pip_local_dependencies, self.env_vars.iter().map(|(k, v)| format!("{}: {}{} ({} chars)", k, &v[..3.min(v.len())], "***", v.len())).collect::>().join(", "), self.native_mode, + // holds bucket credentials + self.object_store_cache_config.as_ref().map(|_| "***").unwrap_or("None")) } } diff --git a/backend/windmill-object-store/Cargo.toml b/backend/windmill-object-store/Cargo.toml index 3602007d8c..90af4f7e7c 100644 --- a/backend/windmill-object-store/Cargo.toml +++ b/backend/windmill-object-store/Cargo.toml @@ -58,3 +58,4 @@ aws-credential-types = { workspace = true, optional = true } tempfile.workspace = true tokio = { workspace = true, features = ["rt", "macros"] } object_store.workspace = true +serial_test = "3" diff --git a/backend/windmill-object-store/src/lib.rs b/backend/windmill-object-store/src/lib.rs index cc588d485f..c403625e00 100644 --- a/backend/windmill-object-store/src/lib.rs +++ b/backend/windmill-object-store/src/lib.rs @@ -162,37 +162,211 @@ impl From> for ExpirableObjectStore { #[cfg(feature = "parquet")] lazy_static::lazy_static! { pub static ref OBJECT_STORE_SETTINGS: Arc>> = Arc::new(RwLock::new(None)); + + /// Worker-group override of the store backing the *dependency cache* only: venvs, language + /// bundles and compiled binaries, which a worker both writes and reads back itself. + /// Everything the server also reads — job results, logs, codebases, app assets — stays on + /// [`OBJECT_STORE_SETTINGS`], which a worker-local redirect would make unreachable. + static ref CACHE_OBJECT_STORE_OVERRIDE: Arc>> = Arc::new(RwLock::new(None)); + + /// The config [`CACHE_OBJECT_STORE_OVERRIDE`] was built from, so a rebuild that fails for a + /// config already being served can keep serving it. Locked after the store, never before. + static ref CACHE_OVERRIDE_APPLIED: Arc>> = Arc::new(RwLock::new(None)); + + /// Held across a whole [`reload_cache_object_store_override`], build included, so that the + /// override's flag, store and applied config only ever move together. + static ref CACHE_OVERRIDE_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::new(()); +} + +/// Whether a worker-group cache override is configured, held apart from the store it built so +/// that a configured-but-broken override reads as "no cache store" instead of silently falling +/// back to the instance bucket the operator redirected away from. +#[cfg(feature = "parquet")] +static CACHE_OBJECT_STORE_OVERRIDDEN: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + +/// Bumped by every [`reload_cache_object_store_override`] at entry. Builds are slow and several +/// callers race — a config change, the retry behind a failed one, a license-plan change — and the +/// lock alone would only order them by arrival, so a reload that lost its claim while waiting +/// drops out rather than installing a store the group has already moved off. +#[cfg(feature = "parquet")] +static CACHE_OVERRIDE_GENERATION: std::sync::atomic::AtomicU64 = + std::sync::atomic::AtomicU64::new(0); + +#[cfg(feature = "parquet")] +async fn resolve_object_store( + settings_lock: &RwLock>, +) -> Option> { + let settings = settings_lock.read().await; + let Some(s) = settings.as_ref() else { + return None; + }; + match &s.refresh { + Some(refresh) if refresh.refresh_needed() => { + let refresh = refresh.clone(); + let refreshed_from = s.store.clone(); + drop(settings); + let new_store = refresh.refresh().await?; + let mut settings = settings_lock.write().await; + match settings.as_ref() { + // A reload may have installed a different store while the credentials were + // being minted; that one reflects newer config, so the refresh is stale. + Some(current) if !Arc::ptr_eq(¤t.store, &refreshed_from) => { + Some(current.store.clone()) + } + Some(_) => { + let arc = new_store.store.clone(); + *settings = Some(new_store); + Some(arc) + } + // Cleared while refreshing. + None => None, + } + } + _ => Some(s.store.clone()), + } } #[cfg(feature = "parquet")] pub async fn get_object_store() -> Option> { - let settings = OBJECT_STORE_SETTINGS.read().await; - if let Some(s) = settings.as_ref() { - match &s.refresh { - Some(refresh) => { - if refresh.refresh_needed() { - let refresh = refresh.clone(); - drop(settings); - let new_store = refresh.refresh().await; - if let Some(new_store) = new_store { - let mut s3_cache_settings = OBJECT_STORE_SETTINGS.write().await; - let arc = new_store.store.clone(); - *s3_cache_settings = Some(new_store); - return Some(arc); - } else { - return None; - } - } else { - return Some(s.store.clone()); - } - } - None => { - return Some(s.store.clone()); - } - } - } else { - return None; + resolve_object_store(&OBJECT_STORE_SETTINGS).await +} + +/// The store the dependency cache reads and writes: the worker group's override when it has one, +/// the instance object store otherwise. Anything the server must also reach goes through +/// [`get_object_store`] instead. +#[cfg(feature = "parquet")] +pub async fn get_cache_object_store() -> Option> { + if CACHE_OBJECT_STORE_OVERRIDDEN.load(std::sync::atomic::Ordering::Relaxed) { + return resolve_object_store(&CACHE_OBJECT_STORE_OVERRIDE).await; } + resolve_object_store(&OBJECT_STORE_SETTINGS).await +} + +/// True when an override is configured but has no usable store. The caller's short retry is the +/// fast path back; this is the backstop, and the full settings reload it rides on is 12h apart by +/// default (`SETTINGS_RELOAD_PERIOD_SECS`), so an outage outlasting the retry keeps the group's +/// dependency cache local until then or until someone edits the group config. +#[cfg(feature = "parquet")] +pub async fn cache_object_store_override_failed() -> bool { + CACHE_OBJECT_STORE_OVERRIDDEN.load(std::sync::atomic::Ordering::Relaxed) + && CACHE_OBJECT_STORE_OVERRIDE.read().await.is_none() +} + +/// Apply the `object_store_cache_config` of this worker's group. `None` (or JSON null) drops the +/// override and returns the worker to the instance object store. +/// +/// Returns [`ObjectStoreReload::Later`] when the store did not build for a reason that may pass — +/// the caller is expected to retry shortly, as `initial_load` does for the instance store. +#[cfg(feature = "parquet")] +pub async fn reload_cache_object_store_override( + db: &windmill_common::DB, + settings: Option, +) -> ObjectStoreReload { + use std::sync::atomic::Ordering; + use windmill_common::ee_oss::{get_license_plan, LicensePlan}; + + // Claim a generation, then take the lock: every state transition below happens inside one + // critical section, and a caller that lost its claim while waiting drops out rather than + // installing what the group has already moved off. + let generation = CACHE_OVERRIDE_GENERATION.fetch_add(1, Ordering::SeqCst) + 1; + let _transition = CACHE_OVERRIDE_LOCK.lock().await; + if CACHE_OVERRIDE_GENERATION.load(Ordering::SeqCst) != generation { + return ObjectStoreReload::Never; + } + + // DISABLE_S3_STORE turns off the instance object store for this process; a group override + // must not be a way back in. + let store_disabled = std::env::var("DISABLE_S3_STORE") + .ok() + .is_some_and(|x| x == "1" || x == "true"); + + let Some(settings) = settings.filter(|v| !v.is_null() && !store_disabled) else { + if CACHE_OBJECT_STORE_OVERRIDDEN.swap(false, Ordering::Relaxed) { + clear_cache_object_store_override().await; + tracing::info!( + "Worker group object store cache override removed, falling back to the instance object store" + ); + } + return ObjectStoreReload::Never; + }; + + // Enterprise-only, so anything else — Community, including a CE build reaching this through + // the config-as-code API, and Pro — must not get a store, and a plan that stops being + // Enterprise must drop one loaded while it still was. + if !matches!(get_license_plan().await, LicensePlan::Enterprise) { + tracing::error!( + "Object store cache override requires an enterprise license, ignoring it for this worker group" + ); + if CACHE_OBJECT_STORE_OVERRIDDEN.swap(false, Ordering::Relaxed) { + clear_cache_object_store_override().await; + } + return ObjectStoreReload::Never; + } + + apply_cache_object_store_override(db, settings).await +} + +/// The half of [`reload_cache_object_store_override`] past the entitlement gate: build the store +/// and commit it. Split out so the commit rules are testable without a license plan. +#[cfg(feature = "parquet")] +async fn apply_cache_object_store_override( + db: &windmill_common::DB, + settings: serde_json::Value, +) -> ObjectStoreReload { + use std::sync::atomic::Ordering; + + // Claim the override before building it: until a store is in place the dependency cache + // must stay local-only rather than reach for the instance bucket. + CACHE_OBJECT_STORE_OVERRIDDEN.store(true, Ordering::Relaxed); + + let (store, reload) = match serde_json::from_value::(settings.clone()) { + Ok(setting) => match build_object_store_from_settings(setting, Some(db)).await { + Ok(store) => (Some(store), ObjectStoreReload::Never), + Err(e) => { + tracing::error!( + "Error building the worker group object store cache override, the dependency cache stays local to this worker until it builds: {e:?}" + ); + (None, ObjectStoreReload::Later) + } + }, + // A malformed config will read the same on every retry. + Err(e) => { + tracing::error!( + "Error parsing the worker group object store cache override, the dependency cache stays local to this worker: {e:?}" + ); + (None, ObjectStoreReload::Never) + } + }; + + let mut current = CACHE_OBJECT_STORE_OVERRIDE.write().await; + match store { + Some(store) => { + *current = Some(store); + *CACHE_OVERRIDE_APPLIED.write().await = Some(settings); + tracing::info!( + "Dependency cache of this worker group now uses its own object store, not the instance one" + ); + } + // A rebuild that failed for the config already being served leaves that store in place: + // the group is entitled to it, and dropping it would take the whole group's cache local + // over a transient error. A *different* config failing must still clear, or the worker + // would keep writing to the bucket the operator redirected it away from. + None if current.is_some() + && CACHE_OVERRIDE_APPLIED.read().await.as_ref() == Some(&settings) => {} + None => { + *current = None; + *CACHE_OVERRIDE_APPLIED.write().await = None; + } + } + reload +} + +/// Drop the override store and the config it was built from, in that lock order. +#[cfg(feature = "parquet")] +async fn clear_cache_object_store_override() { + *CACHE_OBJECT_STORE_OVERRIDE.write().await = None; + *CACHE_OVERRIDE_APPLIED.write().await = None; } #[cfg(feature = "parquet")] @@ -2361,10 +2535,100 @@ mod tests { .contains("Error building filesystem object store")); } + /// A worker group override that is configured but has no usable store must leave the + /// dependency cache with no object store at all. Falling back to the instance one would + /// write the group's cache into the bucket the operator redirected it away from. + // Serialized with the other test that swaps OBJECT_STORE_SETTINGS: the store is + // process-global and CI runs this binary with --test-threads=10. + #[cfg(feature = "parquet")] + #[tokio::test] + #[serial_test::serial(object_store_settings)] + async fn test_get_cache_object_store_override() { + use object_store::{path::Path, ObjectStore, PutPayload}; + use std::sync::atomic::Ordering; + + async fn marker_of(store: &Arc) -> String { + let bytes = store.get(&Path::from("marker")).await.unwrap(); + String::from_utf8(bytes.bytes().await.unwrap().to_vec()).unwrap() + } + + let instance_dir = tempfile::tempdir().unwrap(); + let instance = build_filesystem_client(instance_dir.path().to_str().unwrap()).unwrap(); + instance + .put(&Path::from("marker"), PutPayload::from("instance")) + .await + .unwrap(); + + let group_dir = tempfile::tempdir().unwrap(); + let group = build_filesystem_client(group_dir.path().to_str().unwrap()).unwrap(); + group + .put(&Path::from("marker"), PutPayload::from("group")) + .await + .unwrap(); + + *OBJECT_STORE_SETTINGS.write().await = Some(ExpirableObjectStore::from(instance)); + + let store = get_cache_object_store().await.unwrap(); + assert_eq!(marker_of(&store).await, "instance"); + assert!(!cache_object_store_override_failed().await); + + CACHE_OBJECT_STORE_OVERRIDDEN.store(true, Ordering::Relaxed); + *CACHE_OBJECT_STORE_OVERRIDE.write().await = Some(ExpirableObjectStore::from(group)); + let store = get_cache_object_store().await.unwrap(); + assert_eq!(marker_of(&store).await, "group"); + + // Configured but unbuilt, as a failed reload leaves it: no store at all, rather than the + // instance bucket the operator redirected the group away from. + *CACHE_OBJECT_STORE_OVERRIDE.write().await = None; + assert!(get_cache_object_store().await.is_none()); + assert!(cache_object_store_override_failed().await); + + // The teardown branch returns before the pool is used, so a lazy one is enough. + let db = sqlx::postgres::PgPool::connect_lazy("postgres://localhost/unused").unwrap(); + reload_cache_object_store_override(&db, None).await; + let store = get_cache_object_store().await.unwrap(); + assert_eq!(marker_of(&store).await, "instance"); + assert!(!cache_object_store_override_failed().await); + + *OBJECT_STORE_SETTINGS.write().await = None; + } + + /// A rebuild is triggered by any edit to the group config, not only by editing the store, so + /// a build that fails for the config already installed must leave it alone — otherwise a + /// renamed worker tag plus one flaky token mint takes the whole group's cache local. A + /// *different* config failing still has to clear it. + #[cfg(feature = "parquet")] + #[tokio::test] + #[serial_test::serial(object_store_settings)] + async fn test_failed_rebuild_keeps_the_store_serving_the_same_config() { + let db = sqlx::postgres::PgPool::connect_lazy("postgres://localhost/unused").unwrap(); + let dir = tempfile::tempdir().unwrap(); + let settings = serde_json::json!({ + "type": "Filesystem", "root_path": dir.path().to_str().unwrap() + }); + + apply_cache_object_store_override(&db, settings.clone()).await; + assert!(get_cache_object_store().await.is_some()); + + // Same config, now unbuildable: the store it already produced stays. + dir.close().unwrap(); + apply_cache_object_store_override(&db, settings).await; + assert!(get_cache_object_store().await.is_some()); + + // A different config that will not build must not leave the old bucket in place. + let moved = serde_json::json!({ "type": "Filesystem", "root_path": "/proc/nonexistent" }); + apply_cache_object_store_override(&db, moved).await; + assert!(get_cache_object_store().await.is_none()); + assert!(cache_object_store_override_failed().await); + + reload_cache_object_store_override(&db, None).await; + } + // --- get_logs_from_store test --- #[cfg(feature = "parquet")] #[tokio::test] + #[serial_test::serial(object_store_settings)] async fn test_get_logs_from_store_with_filesystem() { use futures::StreamExt; use object_store::{path::Path, ObjectStore, PutPayload}; diff --git a/backend/windmill-worker/src/global_cache.rs b/backend/windmill-worker/src/global_cache.rs index 5e9666722a..3f6205f100 100644 --- a/backend/windmill-worker/src/global_cache.rs +++ b/backend/windmill-worker/src/global_cache.rs @@ -147,14 +147,17 @@ pub fn extract_tar(tar: bytes::Bytes, folder: &str) -> error::Result<()> { Ok(()) } -/// Two-tier cache load: check local disk first, then fall back to instance object store. +/// Two-tier cache load: check local disk first, then fall back to the shared object store. +/// +/// "Shared" is the worker group's own store when its config overrides one, the instance store +/// otherwise — see [`windmill_object_store::get_cache_object_store`]. /// Returns `(hit, log_message)`. pub async fn load_cache(bin_path: &str, _remote_path: &str, is_dir: bool) -> (bool, String) { if tokio::fs::metadata(&bin_path).await.is_ok() { (true, format!("loaded from local cache: {}\n", bin_path)) } else { #[cfg(all(feature = "enterprise", feature = "parquet"))] - if let Some(os) = windmill_object_store::get_object_store().await { + if let Some(os) = windmill_object_store::get_cache_object_store().await { let started = std::time::Instant::now(); if let Ok(mut x) = windmill_object_store::attempt_fetch_bytes(os, _remote_path).await { @@ -200,13 +203,15 @@ pub async fn load_cache(bin_path: &str, _remote_path: &str, is_dir: bool) -> (bo } } -/// Whether this worker can push to the instance object store at all — the features are +/// Whether this worker can push to the shared object store at all — the features are /// compiled in and a store is loaded. False on builds without them, where `save_cache` /// only ever writes to the worker's own disk. pub async fn object_store_available() -> bool { #[cfg(all(feature = "enterprise", feature = "parquet"))] { - windmill_object_store::get_object_store().await.is_some() + windmill_object_store::get_cache_object_store() + .await + .is_some() } #[cfg(not(all(feature = "enterprise", feature = "parquet")))] { @@ -214,14 +219,14 @@ pub async fn object_store_available() -> bool { } } -/// Whether a binary/bundle is in the instance object store, ignoring the local cache. +/// Whether a binary/bundle is in the shared object store, ignoring the local cache. /// /// The deploy-time prebuild asks this rather than [`exists_in_cache`]: a copy on the /// building worker's own disk is exactly the state the prebuild exists to fix, so /// answering from it would latch a failed upload into a permanent skip. pub async fn exists_in_object_store(_remote_path: &str) -> bool { #[cfg(all(feature = "enterprise", feature = "parquet"))] - if let Some(os) = windmill_object_store::get_object_store().await { + if let Some(os) = windmill_object_store::get_cache_object_store().await { return os .head(&windmill_object_store::object_store_reexports::Path::from( _remote_path, @@ -241,18 +246,18 @@ pub async fn ensure_pushed_to_object_store(remote_path: &str) -> error::Result<( return Ok(()); } Err(error::Error::ExecutionErr(format!( - "the binary was built but did not reach the instance object store at {remote_path}, \ + "the binary was built but did not reach the object store at {remote_path}, \ so no other worker can load it" ))) } -/// Check whether a binary/bundle exists in local cache or instance object store. +/// Check whether a binary/bundle exists in local cache or the shared object store. pub async fn exists_in_cache(bin_path: &str, _remote_path: &str) -> bool { if tokio::fs::metadata(&bin_path).await.is_ok() { return true; } else { #[cfg(all(feature = "enterprise", feature = "parquet"))] - if let Some(os) = windmill_object_store::get_object_store().await { + if let Some(os) = windmill_object_store::get_cache_object_store().await { return os .get(&windmill_object_store::object_store_reexports::Path::from( _remote_path, @@ -264,7 +269,7 @@ pub async fn exists_in_cache(bin_path: &str, _remote_path: &str) -> bool { } } -/// Two-tier cache write: upload to instance object store, then copy to local disk. +/// Two-tier cache write: upload to the shared object store, then copy to local disk. pub async fn save_cache( local_cache_path: &str, _remote_cache_path: &str, @@ -275,7 +280,7 @@ pub async fn save_cache( let mut _cached_to_s3 = false; #[cfg(all(feature = "enterprise", feature = "parquet"))] - if let Some(os) = windmill_object_store::get_object_store().await { + if let Some(os) = windmill_object_store::get_cache_object_store().await { use windmill_object_store::object_store_reexports::Path; let file_to_cache = if is_dir { let tar_path = format!( diff --git a/backend/windmill-worker/src/python_executor.rs b/backend/windmill-worker/src/python_executor.rs index 18808672fc..9f363a7f90 100644 --- a/backend/windmill-worker/src/python_executor.rs +++ b/backend/windmill-worker/src/python_executor.rs @@ -91,10 +91,10 @@ struct PiptarUploadTask { #[cfg(all(feature = "enterprise", feature = "parquet"))] async fn handle_piptar_uploads(mut rx: tokio::sync::mpsc::UnboundedReceiver) { use crate::global_cache::build_tar_and_push; - use windmill_object_store::get_object_store; + use windmill_object_store::get_cache_object_store; while let Some(task) = rx.recv().await { - if let Some(os) = get_object_store().await { + if let Some(os) = get_cache_object_store().await { match build_tar_and_push(os, task.venv_path.clone(), task.cache_dir, None, false).await { Ok(()) => { @@ -144,7 +144,7 @@ pub fn has_relative_imports(content: &str) -> bool { use crate::global_cache::pull_from_tar; #[cfg(all(feature = "enterprise", feature = "parquet"))] -use windmill_object_store::OBJECT_STORE_SETTINGS; +use windmill_object_store::get_cache_object_store; use crate::{ common::{ @@ -2449,7 +2449,7 @@ pub async fn handle_python_reqs( } #[cfg(all(feature = "enterprise", feature = "parquet"))] - if OBJECT_STORE_SETTINGS.read().await.is_none() { + if get_cache_object_store().await.is_none() { (s3_pull, s3_push) = (false, false); } @@ -2907,7 +2907,7 @@ pub async fn handle_python_reqs( #[cfg(all(feature = "enterprise", feature = "parquet"))] if is_not_pro { - if let Some(os) = windmill_object_store::get_object_store().await { + if let Some(os) = windmill_object_store::get_cache_object_store().await { tokio::select! { // Cancel was called on the job _ = kill_rx.recv() => return Err(Error::from(anyhow::anyhow!("S3 pull was canceled"))), diff --git a/backend/windmill-worker/src/universal_pkg_installer.rs b/backend/windmill-worker/src/universal_pkg_installer.rs index f36a5e6fc9..1126c94d17 100644 --- a/backend/windmill-worker/src/universal_pkg_installer.rs +++ b/backend/windmill-worker/src/universal_pkg_installer.rs @@ -333,7 +333,7 @@ pub async fn par_install_language_dependencies_all_at_once< mark_success(path.clone(), job_id, w_id).await; #[cfg(all(feature = "enterprise", feature = "parquet"))] { - if let Some(os) = windmill_object_store::get_object_store().await { + if let Some(os) = windmill_object_store::get_cache_object_store().await { let language_name = _language_name.to_owned(); tokio::spawn(async move { if let Err(e) = crate::global_cache::build_tar_and_push( @@ -790,7 +790,7 @@ async fn try_install_one_detached<'a, T: Clone + std::marker::Send + Sync + 'a + #[cfg(all(feature = "enterprise", feature = "parquet"))] let s3_pull_future = if is_not_pro { - if let Some(os) = windmill_object_store::get_object_store().await { + if let Some(os) = windmill_object_store::get_cache_object_store().await { Some(crate::global_cache::pull_from_tar( os, dep.path.clone(), @@ -893,7 +893,7 @@ async fn try_install_one_detached<'a, T: Clone + std::marker::Send + Sync + 'a + #[cfg(all(feature = "enterprise", feature = "parquet"))] { - if let Some(os) = windmill_object_store::get_object_store().await { + if let Some(os) = windmill_object_store::get_cache_object_store().await { let language_name = _language_name.to_string(); let platform_agnostic = _platform_agnostic; let path = dep.path.clone(); @@ -954,8 +954,7 @@ async fn print_success( } #[cfg(all(feature = "enterprise", feature = "parquet"))] - if windmill_object_store::OBJECT_STORE_SETTINGS - .read() + if windmill_object_store::get_cache_object_store() .await .is_none() { diff --git a/backend/windmill-worker/src/worker_lockfiles.rs b/backend/windmill-worker/src/worker_lockfiles.rs index 9bba9a99e6..1f3b9f1d56 100644 --- a/backend/windmill-worker/src/worker_lockfiles.rs +++ b/backend/windmill-worker/src/worker_lockfiles.rs @@ -174,7 +174,7 @@ async fn handle_build_binary_job( if !crate::global_cache::object_store_available().await { return Ok(to_raw_value_owned(json!({ "status": "skipped", - "reason": "this worker cannot reach the instance object store, so the binary \ + "reason": "this worker cannot reach an object store, so the binary \ would not be shared with other workers", }))); } diff --git a/frontend/src/lib/components/ObjectStoreConfigSettings.svelte b/frontend/src/lib/components/ObjectStoreConfigSettings.svelte index 14658a4228..9ddd7f7fdc 100644 --- a/frontend/src/lib/components/ObjectStoreConfigSettings.svelte +++ b/frontend/src/lib/components/ObjectStoreConfigSettings.svelte @@ -1,3 +1,42 @@ + +