From a913ab0d5a451dd21f099c4f22de8ee366269879 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 13 Sep 2022 16:05:53 -0400 Subject: [PATCH 1/3] container/commit: Auto-clean `/var/{tmp,cache}`, `/tmp`, `/run` The original command here was just scoped to `/var`, but we also don't want content in `/run`. Extend the tooling to handle that and the other two temporary directories. Also, let's be a bit nicer here and auto-clean empty directories in `/var`. I was testing out the https://github.com/coreos/coreos-layering-examples/blob/main/tailscale/Dockerfile example and today we have this: ``` drwxr-xr-x root/root 0 2022-09-13 18:53 run/ drwxr-xr-x root/root 0 2022-09-13 18:51 run/rpm-ostree/ drwxr-xr-x root/root 0 2022-09-13 18:53 run/rpm-ostree/lock/ drwxr-xr-x root/root 0 2022-09-13 18:51 run/systemd/ drwxr-xr-x root/root 0 2022-09-13 18:51 run/systemd/resolve/ -rwx------ root/root 0 2022-09-13 18:51 run/systemd/resolve/stub-resolv.conf ... drwxr-xr-x root/root 0 2022-09-13 18:53 var/ drwxr-xr-x root/root 0 2022-09-13 18:53 var/cache/ drwx------ root/root 0 2022-09-13 18:53 var/cache/ldconfig/ -rw------- root/root 22000 2022-09-13 18:53 var/cache/ldconfig/aux-cache drwxr-xr-x root/root 0 2022-09-08 23:10 var/cache/tailscale/ drwxr-xr-x root/root 0 2022-09-13 18:53 var/tmp/ ``` In this set, we can auto-clean the leftover locking directories rpm-ostree (erroneously) leaves in `/run`, as well as `/var/cache/ldconfig`. --- lib/src/commit.rs | 118 +++++++++++++++++++++++++++++++++------------- 1 file changed, 84 insertions(+), 34 deletions(-) diff --git a/lib/src/commit.rs b/lib/src/commit.rs index 50107ffc..855baa89 100644 --- a/lib/src/commit.rs +++ b/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 { + 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. +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(()) } From 14402640007b70a026aa75c5e3623a8154b9c0c9 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 15 Sep 2022 10:12:47 -0400 Subject: [PATCH 2/3] Make `commit` module public I think it makes sense for us to use this in rpm-ostree directly too at build time for example. --- lib/src/commit.rs | 2 +- lib/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/commit.rs b/lib/src/commit.rs index 855baa89..7f639f42 100644 --- a/lib/src/commit.rs +++ b/lib/src/commit.rs @@ -49,7 +49,7 @@ fn process_dir_recurse(root: &Dir, path: &Utf8Path, error_count: &mut i32) -> Re /// Given a root filesystem, clean out empty directories and warn about /// files in /var. /run, /tmp, and /var/tmp have their contents recursively cleaned. -fn prepare_ostree_commit_in(root: &Dir) -> Result<()> { +pub fn prepare_ostree_commit_in(root: &Dir) -> Result<()> { let mut error_count = 0; for path in FORCE_CLEAN_PATHS { if let Some(subdir) = root.open_dir_optional(path)? { diff --git a/lib/src/lib.rs b/lib/src/lib.rs index fa1ee020..759d373f 100644 --- a/lib/src/lib.rs +++ b/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")] From 972a1349d7643de7dd61d953d4b924b8e069bebd Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 15 Sep 2022 10:05:25 -0400 Subject: [PATCH 3/3] ci: Add a flow that tests `ostree container commit` To verify our changes there too. --- .github/workflows/rust.yml | 18 ++++++++++++++++++ ci/container-build-integration.sh | 20 ++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100755 ci/container-build-integration.sh diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 70074085..69afc132 100644 --- a/.github/workflows/rust.yml +++ b/.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 diff --git a/ci/container-build-integration.sh b/ci/container-build-integration.sh new file mode 100755 index 00000000..08ef8b5c --- /dev/null +++ b/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