From 660614dafe2fada4db4268f69a46d46990145b42 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 9 Feb 2024 16:35:28 -0500 Subject: [PATCH 1/2] tar: Generalize a TarImportConfig I plan to add another variant of this behavior in the future, and having a proper structure is better than a single `bool`. --- lib/src/cli.rs | 3 ++- lib/src/tar/write.rs | 46 +++++++++++++++++++++++++++----------------- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/lib/src/cli.rs b/lib/src/cli.rs index f1e62c2d..42bdb60a 100644 --- a/lib/src/cli.rs +++ b/lib/src/cli.rs @@ -849,7 +849,8 @@ async fn testing(opts: &TestingOpts) -> Result<()> { TestingOpts::Run => crate::integrationtest::run_tests(), TestingOpts::RunIMA => crate::integrationtest::test_ima(), TestingOpts::FilterTar => { - crate::tar::filter_tar(std::io::stdin(), std::io::stdout(), false).map(|_| {}) + crate::tar::filter_tar(std::io::stdin(), std::io::stdout(), &Default::default()) + .map(|_| {}) } } } diff --git a/lib/src/tar/write.rs b/lib/src/tar/write.rs index 62691697..9ad97139 100644 --- a/lib/src/tar/write.rs +++ b/lib/src/tar/write.rs @@ -109,10 +109,15 @@ enum NormalizedPathResult<'a> { Normal(Utf8PathBuf), } -fn normalize_validate_path( - path: &Utf8Path, +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub(crate) struct TarImportConfig { allow_nonusr: bool, -) -> Result> { +} + +fn normalize_validate_path<'a>( + path: &'a Utf8Path, + config: &'_ TarImportConfig, +) -> Result> { // This converts e.g. `foo//bar/./baz` into `foo/bar/baz`. let mut components = path .components() @@ -152,7 +157,7 @@ fn normalize_validate_path( o if EXCLUDED_TOPLEVEL_PATHS.contains(&o) => { return Ok(NormalizedPathResult::Filtered(part)); } - _ if allow_nonusr => ret.push(part), + _ if config.allow_nonusr => ret.push(part), _ => { return Ok(NormalizedPathResult::Filtered(part)); } @@ -180,7 +185,7 @@ fn normalize_validate_path( pub(crate) fn filter_tar( src: impl std::io::Read, dest: impl std::io::Write, - allow_nonusr: bool, + config: &TarImportConfig, ) -> Result> { let src = std::io::BufReader::new(src); let mut src = tar::Archive::new(src); @@ -190,7 +195,7 @@ pub(crate) fn filter_tar( let ents = src.entries()?; - tracing::debug!("Filtering tar; allow_nonusr={allow_nonusr}"); + tracing::debug!("Filtering tar; config={config:?}"); // Lookaside data for dealing with hardlinked files into /sysroot; see below. let mut changed_sysroot_objects = HashMap::new(); @@ -259,7 +264,7 @@ pub(crate) fn filter_tar( } } - let normalized = match normalize_validate_path(path, allow_nonusr)? { + let normalized = match normalize_validate_path(path, config)? { NormalizedPathResult::Filtered(path) => { tracing::trace!("Filtered: {path}"); if let Some(v) = filtered.get_mut(path) { @@ -282,15 +287,16 @@ pub(crate) fn filter_tar( async fn filter_tar_async( src: impl AsyncRead + Send + 'static, mut dest: impl AsyncWrite + Send + Unpin, - allow_nonusr: bool, + config: &TarImportConfig, ) -> Result> { let (tx_buf, mut rx_buf) = tokio::io::duplex(8192); // The source must be moved to the heap so we know it is stable for passing to the worker thread let src = Box::pin(src); + let config = config.clone(); let tar_transformer = tokio::task::spawn_blocking(move || { let mut src = tokio_util::io::SyncIoBridge::new(src); let dest = tokio_util::io::SyncIoBridge::new(tx_buf); - let r = filter_tar(&mut src, dest, allow_nonusr); + let r = filter_tar(&mut src, dest, &config); // Pass ownership of the input stream back to the caller - see below. (r, src) }); @@ -365,7 +371,9 @@ pub async fn write_tar( let mut child_stdout = r.stdout.take().unwrap(); let mut child_stderr = r.stderr.take().unwrap(); // Copy the filtered tar stream to child stdin - let filtered_result = filter_tar_async(src, child_stdin, options.allow_nonusr); + let mut import_config = TarImportConfig::default(); + import_config.allow_nonusr = options.allow_nonusr; + let filtered_result = filter_tar_async(src, child_stdin, &import_config); let output_copier = async move { // Gather stdout/stderr to buffers let mut child_stdout_buf = String::new(); @@ -421,6 +429,8 @@ mod tests { #[test] fn test_normalize_path() { + let imp_default = &TarImportConfig::default(); + let allow_nonusr = &TarImportConfig { allow_nonusr: true }; let valid_all = &[ ("/usr/bin/blah", "./usr/bin/blah"), ("usr/bin/blah", "./usr/bin/blah"), @@ -431,8 +441,8 @@ mod tests { ]; let valid_nonusr = &[("boot", "./boot"), ("opt/puppet/blah", "./opt/puppet/blah")]; for &(k, v) in valid_all { - let r = normalize_validate_path(k.into(), false).unwrap(); - let r2 = normalize_validate_path(k.into(), true).unwrap(); + let r = normalize_validate_path(k.into(), imp_default).unwrap(); + let r2 = normalize_validate_path(k.into(), allow_nonusr).unwrap(); assert_eq!(r, r2); match r { NormalizedPathResult::Normal(r) => assert_eq!(r, v), @@ -440,12 +450,12 @@ mod tests { } } for &(k, v) in valid_nonusr { - let strict = normalize_validate_path(k.into(), false).unwrap(); + let strict = normalize_validate_path(k.into(), imp_default).unwrap(); assert!( matches!(strict, NormalizedPathResult::Filtered(_)), "Incorrect filter for {k}" ); - let nonusr = normalize_validate_path(k.into(), true).unwrap(); + let nonusr = normalize_validate_path(k.into(), allow_nonusr).unwrap(); match nonusr { NormalizedPathResult::Normal(r) => assert_eq!(r, v), NormalizedPathResult::Filtered(o) => panic!("Should not have filtered {o}"), @@ -453,7 +463,7 @@ mod tests { } let filtered = &["/run/blah", "/sys/foo", "/dev/somedev"]; for &k in filtered { - match normalize_validate_path(k.into(), true).unwrap() { + match normalize_validate_path(k.into(), imp_default).unwrap() { NormalizedPathResult::Filtered(_) => {} NormalizedPathResult::Normal(_) => { panic!("{} should be filtered", k) @@ -462,8 +472,8 @@ mod tests { } let errs = &["usr/foo/../../bar"]; for &k in errs { - assert!(normalize_validate_path(k.into(), true).is_err()); - assert!(normalize_validate_path(k.into(), false).is_err()); + assert!(normalize_validate_path(k.into(), allow_nonusr).is_err()); + assert!(normalize_validate_path(k.into(), imp_default).is_err()); } } @@ -481,7 +491,7 @@ mod tests { let _ = rootfs_tar.into_inner()?; let mut dest = Vec::new(); let src = tokio::io::BufReader::new(tokio::fs::File::open(rootfs_tar_path).await?); - filter_tar_async(src, &mut dest, false).await?; + filter_tar_async(src, &mut dest, &Default::default()).await?; let dest = dest.as_slice(); let mut final_tar = tar::Archive::new(Cursor::new(dest)); let destdir = &tempd.path().join("destdir"); From 4bcc315f49d8b739704b069bede94c71cb8ceb1e Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 9 Feb 2024 16:43:26 -0500 Subject: [PATCH 2/2] store: If ostree >= 2024.3, retain content in `/var` This is intended to pair with https://github.com/ostreedev/ostree/pull/3166 If we detect a new enough ostree version, then by default don't remap content in `/var`, assuming that ostree itself will handle it. In order to unit test this (without depending on the ostree version that happens to be on the host) add an API to the importer which allows overriding the version. --- lib/src/container/store.rs | 9 +++++++++ lib/src/fixture.rs | 4 ++++ lib/src/tar/write.rs | 31 +++++++++++++++++++++++++++---- lib/tests/it/main.rs | 24 ++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 4 deletions(-) diff --git a/lib/src/container/store.rs b/lib/src/container/store.rs index a2d35972..ca6d1899 100644 --- a/lib/src/container/store.rs +++ b/lib/src/container/store.rs @@ -166,6 +166,8 @@ pub struct ImageImporter { disable_gc: bool, // If true, don't prune unused image layers /// If true, require the image has the bootable flag require_bootable: bool, + /// If true, we have ostree v2024.3 or newer. + ostree_v2024_3: bool, pub(crate) proxy_img: OpenedImage, layer_progress: Option>, @@ -471,6 +473,7 @@ impl ImageImporter { proxy_img, target_imgref: None, no_imgref: false, + ostree_v2024_3: ostree::check_version(2024, 3), disable_gc: false, require_bootable: false, imgref: imgref.clone(), @@ -496,6 +499,11 @@ impl ImageImporter { self.require_bootable = true; } + /// Override the ostree version being targeted + pub fn set_ostree_version(&mut self, year: u32, v: u32) { + self.ostree_v2024_3 = (year > 2024) || (year == 2024 && v >= 3) + } + /// Do not prune image layers. pub fn disable_gc(&mut self) { self.disable_gc = true; @@ -864,6 +872,7 @@ impl ImageImporter { base: Some(base_commit.clone()), selinux: true, allow_nonusr: root_is_transient, + retain_var: self.ostree_v2024_3, }; let r = crate::tar::write_tar(&self.repo, blob, layer.ostree_ref.as_str(), Some(opts)); diff --git a/lib/src/fixture.rs b/lib/src/fixture.rs index 3322d04b..cc001993 100644 --- a/lib/src/fixture.rs +++ b/lib/src/fixture.rs @@ -439,6 +439,10 @@ impl Fixture { pub fn clear_destrepo(&self) -> Result<()> { self.destrepo() .set_ref_immediate(None, self.testref(), None, gio::Cancellable::NONE)?; + for (r, _) in self.destrepo().list_refs(None, gio::Cancellable::NONE)? { + self.destrepo() + .set_ref_immediate(None, &r, None, gio::Cancellable::NONE)?; + } self.destrepo() .prune(ostree::RepoPruneFlags::REFS_ONLY, 0, gio::Cancellable::NONE)?; Ok(()) diff --git a/lib/src/tar/write.rs b/lib/src/tar/write.rs index 9ad97139..82c887ec 100644 --- a/lib/src/tar/write.rs +++ b/lib/src/tar/write.rs @@ -68,6 +68,9 @@ pub struct WriteTarOptions { pub selinux: bool, /// Allow content not in /usr; this should be paired with ostree rootfs.transient = true pub allow_nonusr: bool, + /// If true, do not move content in /var to /usr/share/factory/var. This should be used + /// with ostree v2024.3 or newer. + pub retain_var: bool, } /// The result of writing a tar stream. @@ -112,6 +115,7 @@ enum NormalizedPathResult<'a> { #[derive(Debug, Clone, PartialEq, Eq, Default)] pub(crate) struct TarImportConfig { allow_nonusr: bool, + remap_factory_var: bool, } fn normalize_validate_path<'a>( @@ -150,9 +154,13 @@ fn normalize_validate_path<'a>( "etc" => { ret.push("usr/etc"); } - // Content in /var will get copied by a systemd tmpfiles.d unit "var" => { - ret.push("usr/share/factory/var"); + // Content in /var will get copied by a systemd tmpfiles.d unit + if config.remap_factory_var { + ret.push("usr/share/factory/var"); + } else { + ret.push(part) + } } o if EXCLUDED_TOPLEVEL_PATHS.contains(&o) => { return Ok(NormalizedPathResult::Filtered(part)); @@ -373,6 +381,7 @@ pub async fn write_tar( // Copy the filtered tar stream to child stdin let mut import_config = TarImportConfig::default(); import_config.allow_nonusr = options.allow_nonusr; + import_config.remap_factory_var = !options.retain_var; let filtered_result = filter_tar_async(src, child_stdin, &import_config); let output_copier = async move { // Gather stdout/stderr to buffers @@ -429,8 +438,18 @@ mod tests { #[test] fn test_normalize_path() { - let imp_default = &TarImportConfig::default(); - let allow_nonusr = &TarImportConfig { allow_nonusr: true }; + let imp_default = &TarImportConfig { + allow_nonusr: false, + remap_factory_var: true, + }; + let allow_nonusr = &TarImportConfig { + allow_nonusr: true, + remap_factory_var: true, + }; + let composefs_and_new_ostree = &TarImportConfig { + allow_nonusr: true, + remap_factory_var: false, + }; let valid_all = &[ ("/usr/bin/blah", "./usr/bin/blah"), ("usr/bin/blah", "./usr/bin/blah"), @@ -475,6 +494,10 @@ mod tests { assert!(normalize_validate_path(k.into(), allow_nonusr).is_err()); assert!(normalize_validate_path(k.into(), imp_default).is_err()); } + assert!(matches!( + normalize_validate_path("var/lib/foo".into(), composefs_and_new_ostree).unwrap(), + NormalizedPathResult::Normal(_) + )); } #[tokio::test] diff --git a/lib/tests/it/main.rs b/lib/tests/it/main.rs index 3a0bfdab..2c4380f9 100644 --- a/lib/tests/it/main.rs +++ b/lib/tests/it/main.rs @@ -935,6 +935,7 @@ async fn test_container_var_content() -> Result<()> { }; let mut imp = store::ImageImporter::new(fixture.destrepo(), &derived_imgref, Default::default()).await?; + imp.set_ostree_version(2023, 11); let prep = match imp.prepare().await.unwrap() { store::PrepareResult::AlreadyPresent(_) => panic!("should not be already imported"), store::PrepareResult::Ready(r) => r, @@ -963,6 +964,29 @@ async fn test_container_var_content() -> Result<()> { .is_none() ); + // Reset things + fixture.clear_destrepo()?; + + let mut imp = + store::ImageImporter::new(fixture.destrepo(), &derived_imgref, Default::default()).await?; + imp.set_ostree_version(2024, 3); + let prep = match imp.prepare().await.unwrap() { + store::PrepareResult::AlreadyPresent(_) => panic!("should not be already imported"), + store::PrepareResult::Ready(r) => r, + }; + let import = imp.import(prep).await.unwrap(); + let ostree_root = fixture + .destrepo() + .read_commit(&import.merge_commit, gio::Cancellable::NONE)? + .0; + let varfile = ostree_root + .child("usr/share/factory/var/lib/foo") + .downcast::() + .unwrap(); + assert!(!varfile.query_exists(gio::Cancellable::NONE)); + assert!(ostree_root + .child("var/lib/foo") + .query_exists(gio::Cancellable::NONE)); Ok(()) }