Merge pull request ostreedev#367 from cgwalters/container-commit-auto…

…clean

container/commit: Auto-clean `/var/tmp`, `/tmp`, `/run`

.github/workflows/rust.yml

@@ -160,3 +160,21 @@ jobs:
         run: install ostree-ext-cli /usr/bin && rm -v ostree-ext-cli
       - name: Integration tests
         run: ./ci/priv-integration.sh
+  container-build:
+    name: "Container build"
+    needs: build
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v2
+      - name: Checkout coreos-layering-examples
+        uses: actions/checkout@v3
+        with:
+          repository: coreos/coreos-layering-examples
+          path: coreos-layering-examples
+      - name: Download
+        uses: actions/download-artifact@v2
+        with:
+          name: ostree-ext-cli
+      - name: Integration tests
+        run: ./ci/container-build-integration.sh

ci/container-build-integration.sh

@@ -0,0 +1,20 @@
+#!/bin/bash
+# Verify `ostree container commit`
+set -euo pipefail
+
+image=quay.io/coreos-assembler/fcos:stable
+example=coreos-layering-examples/tailscale
+set -x
+
+mv ostree-ext-cli ${example}
+cd ${example}
+chmod a+x ostree-ext-cli
+sed -ie 's,ostree container commit,ostree-ext-cli container commit,' Dockerfile
+sed -ie 's,^\(FROM .*\),\1\nADD ostree-ext-cli /usr/bin,' Dockerfile
+git diff
+
+docker build -t localhost/fcos-tailscale .
+
+docker run --rm localhost/fcos-tailscale rpm -q tailscale
+
+echo ok container image integration

lib/src/commit.rs

@@ -6,55 +6,75 @@ use crate::container_utils::require_ostree_container;
 use anyhow::Context;
 use anyhow::Result;
 use camino::Utf8Path;
-use camino::Utf8PathBuf;
-use std::fs;
+use cap_std::fs::Dir;
+use cap_std_ext::cap_std;
+use cap_std_ext::dirext::CapStdExtDirExt;
+use std::convert::TryInto;
+use std::path::Path;
 use tokio::task;
 
-/// Check if there are any files that are not directories and error out if
-/// we find any, /var should not contain any files to commit in a container
-/// as it is where we expect user data to reside.
-fn validate_directories_only_recurse(path: &Utf8Path, error_count: &mut i32) -> Result<()> {
-    let context = || format!("Validating file: {:?}", path);
-    for entry in fs::read_dir(path).with_context(context)? {
+/// Directories for which we will always remove all content.
+const FORCE_CLEAN_PATHS: &[&str] = &["run", "tmp", "var/tmp", "var/cache"];
+
+/// Gather count of non-empty directories.  Empty directories are removed.
+fn process_dir_recurse(root: &Dir, path: &Utf8Path, error_count: &mut i32) -> Result<bool> {
+    let context = || format!("Validating: {path}");
+    let mut validated = true;
+    for entry in root.read_dir(path).with_context(context)? {
         let entry = entry?;
-        let path = entry.path();
-        let path: Utf8PathBuf = path.try_into()?;
+        let name = entry.file_name();
+        let name = Path::new(&name);
+        let name: &Utf8Path = name.try_into()?;
+        let path = &path.join(name);
 
-        let metadata = path.symlink_metadata()?;
+        let metadata = root.symlink_metadata(path)?;
 
         if metadata.is_dir() {
-            validate_directories_only_recurse(&path, error_count)?;
+            if !process_dir_recurse(root, path, error_count)? {
+                validated = false;
+            }
         } else {
+            validated = false;
             *error_count += 1;
             if *error_count < 20 {
                 eprintln!("Found file: {:?}", path)
             }
         }
     }
-    Ok(())
+    if validated {
+        root.remove_dir(path).with_context(context)?;
+    }
+    Ok(validated)
 }
 
-fn validate_ostree_compatibility_in(root: &Utf8Path) -> Result<()> {
-    let var_path = root.join("var");
-    println!("Checking /var for files");
+/// Given a root filesystem, clean out empty directories and warn about
+/// files in /var.  /run, /tmp, and /var/tmp have their contents recursively cleaned.
+pub fn prepare_ostree_commit_in(root: &Dir) -> Result<()> {
     let mut error_count = 0;
-    validate_directories_only_recurse(&var_path, &mut error_count)?;
-    if error_count != 0 {
-        anyhow::bail!("Found content in {var_path}");
+    for path in FORCE_CLEAN_PATHS {
+        if let Some(subdir) = root.open_dir_optional(path)? {
+            for entry in subdir.entries()? {
+                let entry = entry?;
+                subdir.remove_all_optional(entry.file_name())?;
+            }
+        }
+    }
+    let var = Utf8Path::new("var");
+    if root.try_exists(var)? && !process_dir_recurse(root, var, &mut error_count)? {
+        anyhow::bail!("Found content in {var}");
     }
     Ok(())
 }
 
-fn validate_ostree_compatibility() -> Result<()> {
-    validate_ostree_compatibility_in(Utf8Path::new("/"))
-}
-
 /// Entrypoint to the commit procedures, initially we just
 /// have one validation but we expect more in the future.
 pub(crate) async fn container_commit() -> Result<()> {
-    require_ostree_container()?;
-
-    task::spawn_blocking(validate_ostree_compatibility).await?
+    task::spawn_blocking(move || {
+        require_ostree_container()?;
+        let rootdir = Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
+        prepare_ostree_commit_in(&rootdir)
+    })
+    .await?
 }
 
 #[cfg(test)]
@@ -63,18 +83,48 @@ mod tests {
 
     #[test]
     fn commit() -> Result<()> {
-        let td = tempfile::tempdir()?;
-        let td = td.path();
-        let td = Utf8Path::from_path(td).unwrap();
+        let td = &cap_tempfile::tempdir(cap_std::ambient_authority())?;
+
+        // Handle the empty case
+        prepare_ostree_commit_in(td).unwrap();
+
+        let var = Utf8Path::new("var");
+        let run = Utf8Path::new("run");
+        let tmp = Utf8Path::new("tmp");
+        let vartmp_foobar = &var.join("tmp/foo/bar");
+        let runsystemd = &run.join("systemd");
+        let resolvstub = &runsystemd.join("resolv.conf");
+
+        for p in [var, run, tmp] {
+            td.create_dir(p)?;
+        }
 
-        let var = td.join("var");
+        td.create_dir_all(vartmp_foobar)?;
+        td.write(vartmp_foobar.join("a"), "somefile")?;
+        td.write(vartmp_foobar.join("b"), "somefile2")?;
+        td.create_dir_all(runsystemd)?;
+        td.write(resolvstub, "stub resolv")?;
+        prepare_ostree_commit_in(td).unwrap();
+        assert!(!td.try_exists(var)?);
+        assert!(td.try_exists(run)?);
+        assert!(!td.try_exists(runsystemd)?);
 
-        std::fs::create_dir(&var)?;
-        validate_ostree_compatibility_in(td).unwrap();
+        let systemd = run.join("systemd");
+        td.create_dir_all(&systemd)?;
+        prepare_ostree_commit_in(td).unwrap();
+        assert!(!td.try_exists(var)?);
 
-        std::fs::write(var.join("foo"), "somefile")?;
+        td.create_dir(&var)?;
+        td.write(var.join("foo"), "somefile")?;
+        assert!(prepare_ostree_commit_in(td).is_err());
+        assert!(td.try_exists(var)?);
 
-        assert!(validate_ostree_compatibility_in(td).is_err());
+        let nested = Utf8Path::new("var/lib/nested");
+        td.create_dir_all(&nested)?;
+        td.write(nested.join("foo"), "test1")?;
+        td.write(nested.join("foo2"), "test2")?;
+        assert!(prepare_ostree_commit_in(td).is_err());
+        assert!(td.try_exists(var)?);
 
         Ok(())
     }

lib/src/lib.rs

@@ -40,7 +40,7 @@ pub mod tar;
 pub mod tokio_util;
 
 pub mod chunking;
-pub(crate) mod commit;
+pub mod commit;
 pub mod objectsource;
 pub(crate) mod objgv;
 #[cfg(feature = "internal-testing-api")]