From 089cdba66800b49469f18508bd8bf3b83dd2415a Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Thu, 5 Nov 2026 11:54:05 +0100 Subject: [PATCH 1/2] fix: Don't stack tmpfs mounts on repeated NodePublishVolume The kubelet calls NodePublishVolume again for volumes it already published, for example after a kubelet restart, when it reconstructs mounted volumes as uncertain and retries the mount. In privileged mode every call mounted a new tmpfs over the target path. Each extra layer hides the files below it from anything that opens the path later, and costs NodeUnpublishVolume one failed attempt, because it unmounts once and then hits EBUSY removing the directory. The CSI spec requires NodePublishVolume to be idempotent. Reusing the mount means a repeated publish rewrites the existing files, so they are now opened with truncate. Without it, a regenerated key or certificate shorter than the previous one kept the old trailing bytes. This already affected unprivileged mode, which never mounts a tmpfs. A repeated publish still regenerates the secret data, and a running container sees the files rewritten in place. --- CHANGELOG.md | 3 + rust/operator-binary/src/csi_server/node.rs | 76 +++++++++++++++++++-- 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7557f73f..dbaed878 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ All notable changes to this project will be documented in this file. `serviceAccount.create=false` now requires `serviceAccount.name`; it used to fall back to the namespace default ServiceAccount, which lacks the operator ClusterRole ([#736]). - `CreateVolume` no longer returns gRPC codes that make external-provisioner retry indefinitely ([#743]). +- In privileged mode, `NodePublishVolume` reuses an existing tmpfs mount instead of mounting another one on top, so a repeated publish (for example after a kubelet restart) no longer stacks mounts, which kept Pods in `Terminating` for minutes ([#770]). +- `NodePublishVolume` truncates the secret files it rewrites, so a repeated publish no longer leaves trailing bytes of a longer previous file behind ([#770]). - Report the certificate expiry for the `certManager` backend, so that Pods are restarted before their certificate expires and pick up the renewed one. Previously no expiry was reported at all, so a Pod kept the certificate it was given at startup and eventually ran on an expired one @@ -39,6 +41,7 @@ All notable changes to this project will be documented in this file. [#762]: https://github.com/stackabletech/secret-operator/pull/762 [#763]: https://github.com/stackabletech/secret-operator/pull/763 [#766]: https://github.com/stackabletech/secret-operator/pull/766 +[#770]: https://github.com/stackabletech/secret-operator/pull/770 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index 822a7a06..b2a3cfea 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -1,6 +1,6 @@ use std::{ fs::Permissions, - os::unix::prelude::PermissionsExt, + os::unix::prelude::{MetadataExt, PermissionsExt}, path::{Component, Path, PathBuf}, }; @@ -66,6 +66,12 @@ enum PublishError { path: PathBuf, }, + #[snafu(display("failed to read metadata of {path:?}"))] + ReadMetadata { + source: std::io::Error, + path: PathBuf, + }, + #[snafu(display("failed to mount volume mount directory {path:?}"))] Mount { source: std::io::Error, @@ -122,6 +128,7 @@ impl From for Status { Status::new(source.grpc_code(), full_msg) } PublishError::CreateDir { .. } => Status::unavailable(full_msg), + PublishError::ReadMetadata { .. } => Status::unavailable(full_msg), PublishError::Mount { .. } => Status::unavailable(full_msg), PublishError::FormatData { .. } => Status::unavailable(full_msg), PublishError::SetDirPermissions { .. } => Status::unavailable(full_msg), @@ -194,11 +201,21 @@ impl SecretProvisionerNode { }, } if self.privileged { - Mount::builder() - .fstype("tmpfs") - .flags(MountFlags::NODEV | MountFlags::NOEXEC | MountFlags::NOSUID) - .mount("", target_path) - .context(publish_error::MountSnafu { path: target_path })?; + // NodePublishVolume must be idempotent: the kubelet calls it again for volumes it has + // already published, for example after a kubelet restart. Mounting on every call would + // stack a new empty tmpfs over the previous one. The container keeps the lowest layer, + // and `clean_secret_dir` unmounts only one layer per NodeUnpublishVolume, so removing + // the directory fails with EBUSY until the kubelet has retried once per layer. + // And that is the reason we check here whether a mount already exists. + if is_mount_point(target_path).await? { + tracing::info!(volume.path = %target_path.display(), "Volume path is already mounted, reusing the mount"); + } else { + Mount::builder() + .fstype("tmpfs") + .flags(MountFlags::NODEV | MountFlags::NOEXEC | MountFlags::NOSUID) + .mount("", target_path) + .context(publish_error::MountSnafu { path: target_path })?; + } } else { tracing::info!("Running in unprivileged mode, not creating mount for secret volume"); } @@ -231,6 +248,10 @@ impl SecretProvisionerNode { let mut opts = OpenOptions::new(); opts.create(true) .write(true) + // A repeated publish rewrites existing files, and regenerated keys and + // certificates vary in length. Without truncating, a shorter file keeps the + // trailing bytes of the previous one. Not good. So we truncate. + .truncate(true) // User: root/secret-operator // Group: Controlled by Pod.securityContext.fsGroup, the actual application // (when running as unprivileged user) @@ -373,6 +394,23 @@ impl SecretProvisionerNode { } } +/// Returns whether `path` is the root of a mount, by comparing its device with its parent's. +/// +/// Bind mounts within the same filesystem are not detected. +/// That is fine for the tmpfs mounts created by [`SecretProvisionerNode::prepare_secret_dir`]. +async fn is_mount_point(path: &Path) -> Result { + let parent = path.parent().unwrap_or(path); + let device = tokio::fs::metadata(path) + .await + .context(publish_error::ReadMetadataSnafu { path })? + .dev(); + let parent_device = tokio::fs::metadata(parent) + .await + .context(publish_error::ReadMetadataSnafu { path: parent })? + .dev(); + Ok(device != parent_device) +} + // Most of the services are not yet implemented, most of them will never be, because they are // not needed for this use case. // The main two services are publish_volume und unpublish_volume, which get called whenever a @@ -496,3 +534,29 @@ impl Node for SecretProvisionerNode { })) } } + +#[cfg(test)] +mod tests { + use std::path::Path; + + use super::is_mount_point; + + #[tokio::test] + async fn plain_directory_is_not_mount_point() { + let dir = tempfile::tempdir().expect("temporary directory can be created"); + assert!( + !is_mount_point(dir.path()) + .await + .expect("metadata of temporary directory is readable") + ); + } + + #[tokio::test] + async fn proc_is_mount_point() { + assert!( + is_mount_point(Path::new("/proc")) + .await + .expect("metadata of /proc is readable") + ); + } +} From 3d97bc6272f0f388252ac8904b8522816b41d9ec Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Mon, 9 Nov 2026 11:26:42 +0100 Subject: [PATCH 2/2] Update rust/operator-binary/src/csi_server/node.rs Co-authored-by: Techassi --- rust/operator-binary/src/csi_server/node.rs | 25 ++++++++++++--------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index b2a3cfea..0a6a90c4 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -399,16 +399,21 @@ impl SecretProvisionerNode { /// Bind mounts within the same filesystem are not detected. /// That is fine for the tmpfs mounts created by [`SecretProvisionerNode::prepare_secret_dir`]. async fn is_mount_point(path: &Path) -> Result { - let parent = path.parent().unwrap_or(path); - let device = tokio::fs::metadata(path) - .await - .context(publish_error::ReadMetadataSnafu { path })? - .dev(); - let parent_device = tokio::fs::metadata(parent) - .await - .context(publish_error::ReadMetadataSnafu { path: parent })? - .dev(); - Ok(device != parent_device) + match path.parent() { + Some(parent) => { + let device = tokio::fs::metadata(path) + .await + .context(publish_error::ReadMetadataSnafu { path })? + .dev(); + let parent_device = tokio::fs::metadata(parent) + .await + .context(publish_error::ReadMetadataSnafu { path: parent })? + .dev(); + Ok(device != parent_device) + } + // If the path and its parent is the same, we can always return false. + None => Ok(false), + } } // Most of the services are not yet implemented, most of them will never be, because they are