twoliter: ensure krane tmpfile is deleted

When using named tempfiles or tempdirs, the `tempfile` crate relies on
Rust calling `drop()` in order to delete the created temporary files.

We were using a static reference to a tempfile of the krane binary,
which is not guaranteed to drop when the program completes. This caused
twoliter to leak somewhat large krane binary tempfiles.

This commit moves away from a static reference to the tempfile, instead
improving ergonomics around ImageTool owning the tempfile reference.
This allows the Rust compiler to more-reliably drop() the reference on
exit.

Cargo.toml

@@ -85,6 +85,7 @@ chrono = { version = "0.4", default-features = false }
 clap = "4"
 coldsnap = { version = "0.6", default-features = false }
 daemonize = "0.5"
+derivative = "2"
 duct = "0.13"
 env_logger = "0.11"
 fastrand = "2"

tools/krane/Cargo.toml

@@ -11,6 +11,7 @@ anyhow.workspace = true
 flate2.workspace = true
 lazy_static.workspace = true
 tempfile.workspace = true
+tokio.workspace = true
 
 [build-dependencies]
 flate2.workspace = true

tools/krane/src/lib.rs

@@ -2,15 +2,13 @@ use anyhow::Result;
 use flate2::read::GzDecoder;
 use std::fs::{File, Permissions};
 use std::os::unix::fs::PermissionsExt;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 
 use tempfile::TempDir;
 
 const COMPRESSED_KRANE_BIN: &[u8] = include_bytes!(env!("KRANE_GZ_PATH"));
 
-lazy_static::lazy_static! {
-    pub static ref KRANE: Krane = Krane::seal().unwrap();
-}
+pub type KraneError = anyhow::Error;
 
 #[derive(Debug)]
 pub struct Krane {
@@ -20,9 +18,11 @@ pub struct Krane {
 }
 
 impl Krane {
-    fn seal() -> Result<Krane> {
+    pub fn new() -> Result<Krane> {
         let tmp_dir = TempDir::new()?;
+
         let path = tmp_dir.path().join("krane");
+        Self::write_archive(&path)?;
 
         let mut krane_file = File::create(&path)?;
         let permissions = Permissions::from_mode(0o755);
@@ -38,6 +38,17 @@ impl Krane {
         })
     }
 
+    pub fn write_archive(path: impl AsRef<Path>) -> Result<()> {
+        let mut krane_file = File::create(path.as_ref())?;
+        let permissions = Permissions::from_mode(0o755);
+        krane_file.set_permissions(permissions)?;
+
+        let mut krane_reader = GzDecoder::new(COMPRESSED_KRANE_BIN);
+
+        std::io::copy(&mut krane_reader, &mut krane_file)?;
+        Ok(())
+    }
+
     pub fn path(&self) -> &PathBuf {
         &self.path
     }
@@ -50,7 +61,7 @@ mod test {
 
     #[test]
     fn test_krane_runs() {
-        let status = Command::new(KRANE.path())
+        let status = Command::new(Krane::new().unwrap().path())
             .arg("--help")
             .output()
             .expect("failed to run krane");

tools/oci-cli-wrapper/src/crane.rs

@@ -1,7 +1,10 @@
+use std::borrow::Borrow;
+use std::fmt::Debug;
 use std::fs::File;
 use std::path::Path;
 
 use async_trait::async_trait;
+use krane_bundle::Krane;
 use snafu::ResultExt;
 use tar::Archive as TarArchive;
 use tempfile::TempDir;
@@ -10,12 +13,27 @@ use crate::{
     cli::CommandLine, error, ConfigView, DockerArchitecture, ImageToolImpl, ImageView, Result,
 };
 
+// allow CraneCLI to be initialized with an Arc or an owned Krane
+pub trait CraneBinary: Borrow<Krane> + Debug + Send + Sync + 'static {}
+impl<T: Borrow<Krane> + Debug + Send + Sync + 'static> CraneBinary for T {}
+
 #[derive(Debug)]
-pub struct CraneCLI {
+pub struct CraneCLI<C: CraneBinary> {
     pub(crate) cli: CommandLine,
+    // Hold a reference to a tempfile which is deleted when `_krane` is dropped
+    _crane: C,
 }
 
-impl CraneCLI {
+impl<C: CraneBinary> CraneCLI<C> {
+    pub fn new(crane: C) -> Self {
+        Self {
+            cli: CommandLine {
+                path: crane.borrow().path().to_path_buf(),
+            },
+            _crane: crane,
+        }
+    }
+
     /// Enables verbose logging of crane if debug logging is enabled.
     fn crane_cmd<'a>(cmd: &[&'a str]) -> Vec<&'a str> {
         if log::max_level() >= log::LevelFilter::Debug {
@@ -27,7 +45,7 @@ impl CraneCLI {
 }
 
 #[async_trait]
-impl ImageToolImpl for CraneCLI {
+impl<K: CraneBinary> ImageToolImpl for CraneCLI<K> {
     async fn pull_oci_image(&self, path: &Path, uri: &str) -> Result<()> {
         let archive_path = path.to_string_lossy();
         self.cli

tools/oci-cli-wrapper/src/lib.rs

@@ -12,36 +12,44 @@
 //!     metadata. In addition, in order to operate with OCI image format, the containerd-snapshotter
 //!     feature has to be enabled in the docker daemon
 use std::fmt::{Display, Formatter};
+use std::sync::Arc;
 use std::{collections::HashMap, path::Path};
 
 use async_trait::async_trait;
-use cli::CommandLine;
-use crane::CraneCLI;
-use krane_bundle::KRANE;
+use crane::{CraneBinary, CraneCLI};
+use error::CraneInitializeSnafu;
+use krane_bundle::Krane;
 use olpc_cjson::CanonicalFormatter;
 use serde::{Deserialize, Serialize};
 use snafu::ResultExt;
 
 mod cli;
 mod crane;
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct ImageTool {
-    image_tool_impl: Box<dyn ImageToolImpl>,
+    image_tool_impl: Arc<dyn ImageToolImpl>,
 }
 
 impl ImageTool {
-    /// Uses the builtin `krane` provided by the `tools/krane` crate.
-    pub fn from_builtin_krane() -> Self {
-        let image_tool_impl = Box::new(CraneCLI {
-            cli: CommandLine {
-                path: KRANE.path().to_path_buf(),
-            },
-        });
+    /// Creates a new `ImageTool` using the given Krane.
+    ///
+    /// Can be called with an owned Krane or Arc<Krane>.
+    pub fn from_krane<K: CraneBinary>(krane: K) -> Self {
+        let image_tool_impl = Arc::new(CraneCLI::new(krane));
         Self { image_tool_impl }
     }
 
-    pub fn new(image_tool_impl: Box<dyn ImageToolImpl>) -> Self {
+    /// Uses the builtin `krane` provided by the `tools/krane` crate.
+    ///
+    /// Each call to this creates a new temporary file with the `krane` binary. Use
+    /// [`ImageTool::from_krane`] to share references to e.g. an `Arc<Krane>`.
+    pub fn from_new_krane() -> Result<Self> {
+        let image_tool_impl = Arc::new(CraneCLI::new(Krane::new().context(CraneInitializeSnafu)?));
+        Ok(Self { image_tool_impl })
+    }
+
+    pub fn new(image_tool_impl: Arc<dyn ImageToolImpl>) -> Self {
         Self { image_tool_impl }
     }
 
@@ -179,6 +187,9 @@ pub mod error {
         #[snafu(display("Failed to create temporary directory for crane push: {source}"))]
         CraneTemp { source: std::io::Error },
 
+        #[snafu(display("Failed to initialize crane: {source}"))]
+        CraneInitialize { source: krane_bundle::KraneError },
+
         #[snafu(display("Failed to create temporary directory for docker save: {source}"))]
         DockerTemp { source: std::io::Error },
 
@@ -220,3 +231,41 @@ pub mod error {
         Unsupported { name: String },
     }
 }
+
+#[cfg(test)]
+mod test {
+    use std::sync::Arc;
+
+    use super::*;
+
+    #[tokio::test]
+    async fn test_owned_krane_cleans_up() {
+        // Ensure we can make an image tool with an owned or Arc Krane object.
+        // Using an Arc would allow multiple `ImageTools` to refer to the same `krane` binary on
+        // disk.
+
+        let krane = Krane::new().unwrap();
+        let bin_path = krane.path().to_path_buf();
+
+        let owned_image_tool = ImageTool::from_krane(krane);
+        assert!(tokio::fs::try_exists(&bin_path).await.unwrap());
+
+        drop(owned_image_tool);
+        assert!(!tokio::fs::try_exists(&bin_path).await.unwrap());
+    }
+
+    #[tokio::test]
+    async fn test_shared_krane_cleans_up() {
+        let krane = Arc::new(Krane::new().unwrap());
+        let bin_path = krane.path().to_path_buf();
+
+        let image_tool = ImageTool::from_krane(Arc::clone(&krane));
+        assert!(tokio::fs::try_exists(&bin_path).await.unwrap());
+
+        drop(image_tool);
+        assert!(tokio::fs::try_exists(&bin_path).await.unwrap());
+
+        drop(krane);
+        assert!(!tokio::fs::try_exists(&bin_path).await.unwrap());
+    }
+}

tools/pubsys/src/kit/publish_kit/mod.rs

@@ -31,7 +31,7 @@ pub(crate) struct PublishKitArgs {
 }
 
 pub(crate) async fn run(args: &Args, publish_kit_args: &PublishKitArgs) -> Result<()> {
-    let image_tool = ImageTool::from_builtin_krane();
+    let image_tool = ImageTool::from_new_krane().context(error::ImageToolInitializationSnafu)?;
 
     // If a lock file exists, use that, otherwise use Infra.toml
     let infra_config = InfraConfig::from_path_or_lock(&args.infra_config_path, false)
@@ -137,6 +137,11 @@ mod error {
         #[snafu(display("Error reading config: {}", source))]
         Config { source: pubsys_config::Error },
 
+        #[snafu(display("Failed to initialize oci image tool: {source}"))]
+        ImageToolInitialization {
+            source: oci_cli_wrapper::error::Error,
+        },
+
         #[snafu(display("Could not convert {} to docker architecture: {}", arch, source))]
         InvalidArchitecture {
             source: oci_cli_wrapper::error::Error,

twoliter/Cargo.toml

@@ -17,6 +17,7 @@ async-trait.workspace = true
 base64.workspace = true
 buildsys-config.workspace = true
 clap = { workspace = true, features = ["derive", "env", "std"] }
+derivative.workspace = true
 env_logger.workspace = true
 filetime.workspace = true
 flate2.workspace = true

twoliter/src/project/lock/mod.rs

@@ -19,7 +19,6 @@ use crate::project::{Project, ValidIdentifier};
 use crate::schema_version::SchemaVersion;
 use anyhow::{bail, ensure, Context, Result};
 use image::{ImageResolver, LockedImage};
-use oci_cli_wrapper::ImageTool;
 use olpc_cjson::CanonicalFormatter as CanonicalJsonFormatter;
 use semver::Version;
 use serde::{Deserialize, Serialize};
@@ -103,10 +102,9 @@ impl LockedSDK {
         };
 
         debug!(?sdk, "Resolving workspace SDK");
-        let image_tool = ImageTool::from_builtin_krane();
         ImageResolver::from_image(&sdk)?
             .skip_metadata_retrieval() // SDKs don't have metadata
-            .resolve(&image_tool)
+            .resolve(project.oci_image_tool())
             .await
             .map(|(sdk, _)| Some(Self(sdk)))
     }
@@ -203,7 +201,6 @@ impl Lock {
     /// Fetches all external kits defined in a Twoliter.lock to the build directory
     #[instrument(level = "trace", skip_all)]
     pub(crate) async fn fetch(&self, project: &Project<Locked>, arch: &str) -> Result<()> {
-        let image_tool = ImageTool::from_builtin_krane();
         let target_dir = project.external_kits_dir();
         create_dir_all(&target_dir).await.context(format!(
             "failed to create external-kits directory at {}",
@@ -218,7 +215,7 @@ impl Lock {
             let image = project.as_project_image(image)?;
             let resolver = ImageResolver::from_image(&image)?;
             resolver
-                .extract(&image_tool, &project.external_kits_dir(), arch)
+                .extract(project.oci_image_tool(), &project.external_kits_dir(), arch)
                 .await?;
         }
 
@@ -257,7 +254,6 @@ impl Lock {
     async fn resolve(project: &Project<Unlocked>) -> Result<Self> {
         let mut known: HashMap<(ValidIdentifier, ValidIdentifier), Version> = HashMap::new();
         let mut locked: Vec<LockedImage> = Vec::new();
-        let image_tool = ImageTool::from_builtin_krane();
         let mut remaining = project.direct_kit_deps()?;
 
         let mut sdk_set = HashSet::new();
@@ -292,7 +288,8 @@ impl Lock {
                     image.version().clone(),
                 );
                 let image_resolver = ImageResolver::from_image(image)?;
-                let (locked_image, metadata) = image_resolver.resolve(&image_tool).await?;
+                let (locked_image, metadata) =
+                    image_resolver.resolve(project.oci_image_tool()).await?;
                 let metadata = metadata.context(format!(
                     "failed to validate kit image with name {} from vendor {}",
                     locked_image.name, locked_image.vendor
@@ -322,7 +319,7 @@ impl Lock {
         debug!(?sdk, "Resolving workspace SDK");
         let (sdk, _metadata) = ImageResolver::from_image(sdk)?
             .skip_metadata_retrieval() // SDKs don't have metadata
-            .resolve(&image_tool)
+            .resolve(project.oci_image_tool())
             .await?;
 
         Ok(Self {