Merge pull request bootc-dev#602 from cgwalters/support-ostree-var

cgwalters · web-flow · commit 8dca470b7cf4 · 2024-02-11T13:50:27.000-05:00
store: If ostree &gt;= 2024.3, retain content in `/var`
diff --git 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/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<Sender<ImportProgress>>,
@@ -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
@@ -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
@@ -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.
@@ -109,10 +112,16 @@ 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<NormalizedPathResult<'_>> {
+    remap_factory_var: bool,
+}
+
+fn normalize_validate_path<'a>(
+    path: &'a Utf8Path,
+    config: &'_ TarImportConfig,
+) -> Result<NormalizedPathResult<'a>> {
     // This converts e.g. `foo//bar/./baz` into `foo/bar/baz`.
     let mut components = path
         .components()
@@ -145,14 +154,18 @@ fn normalize_validate_path(
                     "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));
                     }
-                    _ if allow_nonusr => ret.push(part),
+                    _ if config.allow_nonusr => ret.push(part),
                     _ => {
                         return Ok(NormalizedPathResult::Filtered(part));
                     }
@@ -180,7 +193,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<BTreeMap<String, u32>> {
     let src = std::io::BufReader::new(src);
     let mut src = tar::Archive::new(src);
@@ -190,7 +203,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 +272,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 +295,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<BTreeMap<String, u32>> {
     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 +379,10 @@ 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;
+    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
         let mut child_stdout_buf = String::new();
@@ -421,6 +438,18 @@ mod tests {
 
     #[test]
     fn test_normalize_path() {
+        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"),
@@ -431,29 +460,29 @@ 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),
                 NormalizedPathResult::Filtered(o) => panic!("Should not have filtered {o}"),
             }
         }
         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}"),
             }
         }
         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,9 +491,13 @@ 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());
         }
+        assert!(matches!(
+            normalize_validate_path("var/lib/foo".into(), composefs_and_new_ostree).unwrap(),
+            NormalizedPathResult::Normal(_)
+        ));
     }
 
     #[tokio::test]
@@ -481,7 +514,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");
diff --git 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::<ostree::RepoFile>()
+        .unwrap();
+    assert!(!varfile.query_exists(gio::Cancellable::NONE));
+    assert!(ostree_root
+        .child("var/lib/foo")
+        .query_exists(gio::Cancellable::NONE));
     Ok(())
 }
 

Original file line number	Diff line number	Diff line change
`@@ -849,7 +849,8 @@ async fn testing(opts: &TestingOpts) -> Result<()> {`
`849`	`849`	`TestingOpts::Run => crate::integrationtest::run_tests(),`
`850`	`850`	`TestingOpts::RunIMA => crate::integrationtest::test_ima(),`
`851`	`851`	`TestingOpts::FilterTar => {`
`852`		`- crate::tar::filter_tar(std::io::stdin(), std::io::stdout(), false).map(\|_\| {})`
	`852`	`+ crate::tar::filter_tar(std::io::stdin(), std::io::stdout(), &Default::default())`
	`853`	`+ .map(\|_\| {})`
`853`	`854`	`}`
`854`	`855`	`}`
`855`	`856`	`}`