fix(worker): address docker review items (hot-reload, nsjail socket gate, async teardown)

- P2: hot-reload docker_image_storage_size_mb — handle the setting in process_notify_event (main.rs), not just initial_load, so the instance-settings UI change takes effect on running workers without a restart. - P2: don't let a mounted host /var/run/docker.sock suppress per-job podman under nsjail — the bash nsjail proto never mounts the host socket into the jail, so it's unreachable there; only count the unix socket when !nsjail (DOCKER_HOST tcp still counts in all modes since the network crosses the jail). - P1: make PerJobPodman teardown non-blocking — offload `podman system reset` + service kill + rmdir to spawn_blocking (bounded by `timeout 30`) instead of blocking the Tokio worker thread in Drop; run inline only when off-runtime. - Docs: note that the per-job podman daemon runs outside nsjail (Docker-API bind mounts can reach worker-visible paths — trusted-tenant capability, prefer dedicated docker workers on shared fleets), and that mixed base/full fleets must route docker jobs to a *-full WORKER_TAGS=docker group rather than the default tag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-08-18 16:02:10 +00:00 · 2026-06-04 20:04:42 +00:00
parent a1efca2fb5
commit f1b6746e0e
3 changed files with 86 additions and 34 deletions
@@ -222,6 +222,19 @@ Go to http://localhost - default credentials: `admin@windmill.dev` / `changeme`
 > notes in
 > [docker-compose.yml](./docker-compose.yml). To use an external/host Docker daemon
 > instead (legacy), provide `DOCKER_HOST` or mount `/var/run/docker.sock`.
+>
+> **Mixed fleets:** `docker` is a default tag, so a base/slim default worker (no podman)
+> can pick up a docker job and fail it ("no Docker daemon… use a `*-full` image"). If not
+> all your default workers use a `*-full` image, route docker jobs to a dedicated
+> `*-full` group with `WORKER_TAGS=docker` rather than relying on the default tag.
+>
+> **Security:** the per-job podman daemon runs outside the worker's nsjail sandbox (only
+> its socket is exposed to the script), so it does **not** extend nsjail's filesystem
+> isolation to containers — a `# docker` script can bind-mount worker-visible paths (e.g.
+> other concurrent job dirs, caches) via the Docker API. Rootless uid-mapping limits this
+> to the unprivileged worker user (a big improvement over privileged dind), but treat
+> docker-capable workers as a trusted-tenant capability and prefer dedicated/per-workspace
+> docker workers on shared multi-tenant fleets.

 **Using an external database**: Set `DATABASE_URL` in `.env` to point to your managed Postgres (AWS RDS, GCP Cloud SQL, Azure, Neon, etc.) and set db replicas to 0.

@@ -43,11 +43,11 @@ use windmill_common::{
        CRITICAL_ALERTS_ON_DB_OVERSIZE_SETTING, CRITICAL_ALERTS_ON_TOKEN_EXPIRY_SETTING,
        CRITICAL_ALERT_MUTE_UI_SETTING, CRITICAL_ERROR_CHANNELS_SETTING, CUSTOM_TAGS_SETTING,
        DEFAULT_TAGS_PER_WORKSPACE_SETTING, DEFAULT_TAGS_WORKSPACES_SETTING,
-        DISABLE_PASSWORD_LOGIN_SETTING, EMAIL_DOMAIN_SETTING, ENV_SETTINGS,
-        EXPOSE_DEBUG_METRICS_SETTING, EXPOSE_METRICS_SETTING, EXTRA_PIP_INDEX_URL_SETTING,
-        FORK_WORKSPACE_TAG_APPEND_FORK_SUFFIX_SETTING, HTTP_ROUTE_WORKSPACED_ROUTE_SETTING,
-        HUB_API_SECRET_SETTING, HUB_BASE_URL_SETTING, INDEXER_SETTING,
-        INSTANCE_EVENTS_WEBHOOK_SETTING, INSTANCE_PYTHON_VERSION_SETTING,
+        DISABLE_PASSWORD_LOGIN_SETTING, DOCKER_IMAGE_STORAGE_SIZE_MB_SETTING, EMAIL_DOMAIN_SETTING,
+        ENV_SETTINGS, EXPOSE_DEBUG_METRICS_SETTING, EXPOSE_METRICS_SETTING,
+        EXTRA_PIP_INDEX_URL_SETTING, FORK_WORKSPACE_TAG_APPEND_FORK_SUFFIX_SETTING,
+        HTTP_ROUTE_WORKSPACED_ROUTE_SETTING, HUB_API_SECRET_SETTING, HUB_BASE_URL_SETTING,
+        INDEXER_SETTING, INSTANCE_EVENTS_WEBHOOK_SETTING, INSTANCE_PYTHON_VERSION_SETTING,
        JOB_DEFAULT_TIMEOUT_SECS_SETTING, JOB_ISOLATION_SETTING, JWT_SECRET_SETTING,
        KEEP_JOB_DIR_SETTING, LICENSE_KEY_SETTING, MAVEN_REPOS_SETTING, MAVEN_SETTINGS_XML_SETTING,
        MONITOR_LOGS_ON_OBJECT_STORE_SETTING, NO_DEFAULT_MAVEN_SETTING,
@@ -128,11 +128,12 @@ use crate::monitor::{
    reload_audit_log_retention_days_setting, reload_base_url_setting,
    reload_bun_install_min_release_age_setting, reload_bunfig_install_scopes_setting,
    reload_critical_alert_mute_ui_setting, reload_critical_alerts_on_token_expiry_setting,
-    reload_critical_error_channels_setting, reload_extra_pip_index_url_setting,
-    reload_http_route_workspaced_route_setting, reload_hub_api_secret_setting,
-    reload_hub_base_url_setting, reload_instance_events_webhook_setting,
-    reload_job_default_timeout_setting, reload_job_isolation_setting, reload_jwt_secret_setting,
-    reload_license_key, reload_npm_config_registry_setting, reload_nsjail_tmp_backing_setting,
+    reload_critical_error_channels_setting, reload_docker_image_storage_size_setting,
+    reload_extra_pip_index_url_setting, reload_http_route_workspaced_route_setting,
+    reload_hub_api_secret_setting, reload_hub_base_url_setting,
+    reload_instance_events_webhook_setting, reload_job_default_timeout_setting,
+    reload_job_isolation_setting, reload_jwt_secret_setting, reload_license_key,
+    reload_npm_config_registry_setting, reload_nsjail_tmp_backing_setting,
    reload_nsjail_tmpfs_size_setting, reload_otel_tracing_proxy_setting,
    reload_pip_index_url_setting, reload_retention_period_setting, reload_scim_token_setting,
    reload_smtp_config, reload_store_audit_logs_s3_setting, reload_uv_exclude_newer_setting,
@@ -1821,6 +1822,9 @@ async fn process_notify_event(
                JOB_ISOLATION_SETTING => reload_job_isolation_setting(conn).await,
                NSJAIL_TMPFS_SIZE_MB_SETTING => reload_nsjail_tmpfs_size_setting(conn).await,
                NSJAIL_TMP_BACKING_SETTING => reload_nsjail_tmp_backing_setting(conn).await,
+                DOCKER_IMAGE_STORAGE_SIZE_MB_SETTING => {
+                    reload_docker_image_storage_size_setting(conn).await
+                }
                #[cfg(feature = "parquet")]
                OBJECT_STORE_CONFIG_SETTING => {
                    if !disable_s3_store {
@@ -200,9 +200,15 @@ exit $exit_status
    // the end of the function, so no container it spawns can outlive the job, in any
    // sandbox mode). Requires podman in the image (the *-full images) — else
    // start_per_job_podman returns a clear error.
+    // A provided Docker daemon (legacy path) wins over per-job podman. DOCKER_HOST
+    // counts in all modes (a tcp:// dind stays reachable through the jail since the
+    // sandbox doesn't isolate the network). A mounted /var/run/docker.sock only
+    // counts when NOT under nsjail: the bash nsjail proto never bind-mounts the host
+    // socket into the jail, so under nsjail it is unreachable and we must fall back
+    // to the per-job podman socket (which IS mounted in) instead of failing the job.
    #[cfg(feature = "dind")]
    let docker_daemon_provided = std::env::var("DOCKER_HOST").is_ok()
-        || std::path::Path::new("/var/run/docker.sock").exists();
+        || (!nsjail && std::path::Path::new("/var/run/docker.sock").exists());
    #[cfg(feature = "dind")]
    let per_job_podman: Option<PerJobPodman> = if annotation.docker && !docker_daemon_provided {
        Some(start_per_job_podman(job_dir, job.id, &job.workspace_id, conn).await?)
@@ -466,6 +472,37 @@ struct PerJobPodman {
    monitor: Option<tokio::task::JoinHandle<()>>,
 }

+// Blocking teardown of a per-job podman instance: `system reset` removes ALL
+// containers AND images in the per-job store and, crucially, deletes the
+// subuid-owned overlay layers a plain `rm -rf` (or `rm -af`, which only touches
+// containers) cannot — preventing a storage leak. Wrapped in `timeout` so a wedged
+// runtime/storage can't hang the caller indefinitely.
+#[cfg(feature = "dind")]
+fn teardown_per_job_podman(dir: String, service: Option<std::process::Child>) {
+    let storage = format!("{dir}/storage");
+    let runroot = format!("{dir}/runroot");
+    let _ = std::process::Command::new("timeout")
+        .args([
+            "30",
+            "podman",
+            "--root",
+            &storage,
+            "--runroot",
+            &runroot,
+            "system",
+            "reset",
+            "--force",
+        ])
+        .stdout(std::process::Stdio::null())
+        .stderr(std::process::Stdio::null())
+        .status();
+    if let Some(mut child) = service {
+        let _ = child.kill();
+        let _ = child.wait();
+    }
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
 #[cfg(feature = "dind")]
 impl Drop for PerJobPodman {
    fn drop(&mut self) {
@@ -473,30 +510,17 @@ impl Drop for PerJobPodman {
        if let Some(monitor) = self.monitor.take() {
            monitor.abort();
        }
-        // Drop is synchronous: use blocking std::process for guaranteed teardown.
-        // `system reset` removes ALL containers AND images in the per-job store and,
-        // crucially, deletes the subuid-owned overlay layers a plain `rm -rf` (or
-        // `rm -af`, which only touches containers) cannot — preventing a storage leak.
-        let storage = format!("{}/storage", self.dir);
-        let runroot = format!("{}/runroot", self.dir);
-        let _ = std::process::Command::new("podman")
-            .args([
-                "--root",
-                &storage,
-                "--runroot",
-                &runroot,
-                "system",
-                "reset",
-                "--force",
-            ])
-            .stdout(std::process::Stdio::null())
-            .stderr(std::process::Stdio::null())
-            .status();
-        if let Some(mut child) = self.service.take() {
-            let _ = child.kill();
-            let _ = child.wait();
+        let dir = std::mem::take(&mut self.dir);
+        let service = self.service.take();
+        // Teardown shells out (`podman system reset`) and can do non-trivial I/O, so
+        // offload it to the blocking pool rather than stalling the Tokio worker thread
+        // this Drop runs on. If we're not on a runtime (e.g. tests), run inline.
+        match tokio::runtime::Handle::try_current() {
+            Ok(handle) => {
+                handle.spawn_blocking(move || teardown_per_job_podman(dir, service));
+            }
+            Err(_) => teardown_per_job_podman(dir, service),
        }
-        let _ = std::fs::remove_dir_all(&self.dir);
    }
 }

@@ -612,6 +636,17 @@ fn spawn_docker_storage_monitor(
 // storage, returning the host socket path. Inherits the worker's container
 // config (CONTAINERS_CONF etc.) but overrides storage so it is job-scoped. A
 // background monitor enforces a soft size cap on the image store.
+//
+// SECURITY: the daemon runs OUTSIDE the nsjail (only its socket is mounted in), so
+// it does NOT extend nsjail's filesystem isolation to docker containers. A `# docker`
+// script controls this daemon over the Docker API and can bind-mount any path the
+// worker user can read (e.g. `docker run -v /tmp/windmill/...`), reaching other
+// concurrent job dirs / caches that nsjail deliberately hid. Rootless uid-mapping
+// caps the blast radius to the unprivileged worker user (no root-owned secrets), and
+// this is a large improvement over the privileged dind it replaces — but the
+// per-job-podman path is NOT a full filesystem sandbox the way pure nsjail is. Treat
+// docker-capable workers as a trusted-tenant capability; on shared multi-tenant
+// fleets prefer per-workspace/dedicated docker workers so this stays intra-tenant.
 #[cfg(feature = "dind")]
 async fn start_per_job_podman(
    job_dir: &str,