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..0a6a90c4 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,28 @@ 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 { + 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 // not needed for this use case. // The main two services are publish_volume und unpublish_volume, which get called whenever a @@ -496,3 +539,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") + ); + } +}