ostree-ext/store: don't include filtered files in metadata

Filtered files are only determined at the time we import a layer.
So if that layer is already imported, we won't have that information
available. That in turn means that the metadata is state-dependent,
which in turn means that the commit digest is not reproducible.

We still want to provide the filtered files warning though. Just make
this information part of the LayeredImageState object instead. The
obvious downside of that is that now we only get that warning the first
time the layer is imported and it's no longer part of the commit object
itself.

One way to make this more sticky is to attach it to the individual
layers' commits instead, and then the merge commit can coalesce them.

Related: #1346

Author: jlebon

lib/src/deploy.rs

@@ -376,9 +376,7 @@ pub(crate) async fn prepare_for_pull(
 
 #[context("Pulling")]
 pub(crate) async fn pull_from_prepared(
-    repo: &ostree::Repo,
     imgref: &ImageReference,
-    target_imgref: Option<&OstreeImageReference>,
     quiet: bool,
     prog: ProgressWriter,
     mut prepared_image: PreparedImportMeta,
@@ -424,11 +422,9 @@ pub(crate) async fn pull_from_prepared(
     let import = import?;
     let imgref_canonicalized = imgref.clone().canonicalize()?;
     tracing::debug!("Canonicalized image reference: {imgref_canonicalized:#}");
-    let ostree_imgref = &OstreeImageReference::from(imgref_canonicalized);
-    let wrote_imgref = target_imgref.as_ref().unwrap_or(&ostree_imgref);
 
     if let Some(msg) =
-        ostree_container::store::image_filtered_content_warning(repo, &wrote_imgref.imgref)
+        ostree_container::store::image_filtered_content_warning(&import.filtered_files)
             .context("Image content warning")?
     {
         crate::journal::journal_print(libsystemd::logging::Priority::Notice, &msg);
@@ -446,15 +442,9 @@ pub(crate) async fn pull(
 ) -> Result<Box<ImageState>> {
     match prepare_for_pull(repo, imgref, target_imgref).await? {
         PreparedPullResult::AlreadyPresent(existing) => Ok(existing),
-        PreparedPullResult::Ready(prepared_image_meta) => Ok(pull_from_prepared(
-            repo,
-            imgref,
-            target_imgref,
-            quiet,
-            prog,
-            prepared_image_meta,
-        )
-        .await?),
+        PreparedPullResult::Ready(prepared_image_meta) => {
+            Ok(pull_from_prepared(imgref, quiet, prog, prepared_image_meta).await?)
+        }
     }
 }
 

lib/src/install.rs

@@ -793,22 +793,15 @@ async fn install_container(
     let repo = &sysroot.repo();
     repo.set_disable_fsync(true);
 
-    let pulled_image =
-        match prepare_for_pull(repo, &spec_imgref, Some(&state.target_imgref)).await? {
-            PreparedPullResult::AlreadyPresent(existing) => existing,
-            PreparedPullResult::Ready(image_meta) => {
-                check_disk_space(root_setup.physical_root.as_fd(), &image_meta, &spec_imgref)?;
-                pull_from_prepared(
-                    repo,
-                    &spec_imgref,
-                    Some(&state.target_imgref),
-                    false,
-                    ProgressWriter::default(),
-                    image_meta,
-                )
-                .await?
-            }
-        };
+    let pulled_image = match prepare_for_pull(repo, &spec_imgref, Some(&state.target_imgref))
+        .await?
+    {
+        PreparedPullResult::AlreadyPresent(existing) => existing,
+        PreparedPullResult::Ready(image_meta) => {
+            check_disk_space(root_setup.physical_root.as_fd(), &image_meta, &spec_imgref)?;
+            pull_from_prepared(&spec_imgref, false, ProgressWriter::default(), image_meta).await?
+        }
+    };
 
     repo.set_disable_fsync(false);
 

ostree-ext/src/cli.rs

@@ -906,7 +906,7 @@ async fn container_store(
     }
     let import = import?;
     if let Some(msg) =
-        ostree_container::store::image_filtered_content_warning(repo, &imgref.imgref)?
+        ostree_container::store::image_filtered_content_warning(&import.filtered_files)?
     {
         eprintln!("{msg}")
     }
@@ -1263,7 +1263,6 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
                         ostree::Sysroot::new_default()
                     };
                     sysroot.load(gio::Cancellable::NONE)?;
-                    let repo = &sysroot.repo();
                     let kargs = karg.as_deref();
                     let kargs = kargs.map(|v| {
                         let r: Vec<_> = v.iter().map(|s| s.as_str()).collect();
@@ -1321,10 +1320,8 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
                         Some(options),
                     )
                     .await?;
-                    let wrote_imgref = target_imgref.as_ref().unwrap_or(&imgref);
                     if let Some(msg) = ostree_container::store::image_filtered_content_warning(
-                        repo,
-                        &wrote_imgref.imgref,
+                        &state.filtered_files,
                     )? {
                         eprintln!("{msg}")
                     }

ostree-ext/src/container/store.rs

@@ -58,9 +58,7 @@ pub(crate) const META_MANIFEST_DIGEST: &str = "ostree.manifest-digest";
 const META_MANIFEST: &str = "ostree.manifest";
 /// The key injected into the merge commit with the image configuration serialized as JSON.
 const META_CONFIG: &str = "ostree.container.image-config";
-/// Value of type `a{sa{su}}` containing number of filtered out files
-pub const META_FILTERED: &str = "ostree.tar-filtered";
-/// The type used to store content filtering information with `META_FILTERED`.
+/// The type used to store content filtering information.
 pub type MetaFilteredData = HashMap<String, HashMap<String, u32>>;
 
 /// The ref prefixes which point to ostree deployments.  (TODO: Add an official API for this)
@@ -138,6 +136,8 @@ pub struct LayeredImageState {
     /// in the future we should probably instead just proxy a signature object
     /// instead, but this is sufficient for now.
     pub verify_text: Option<String>,
+    /// Files that were filtered out during the import.
+    pub filtered_files: Option<MetaFilteredData>,
 }
 
 impl LayeredImageState {
@@ -969,7 +969,7 @@ impl ImageImporter {
         let ostree_ref = ref_for_image(&target_imgref.imgref)?;
 
         let mut layer_commits = Vec::new();
-        let mut layer_filtered_content: MetaFilteredData = HashMap::new();
+        let mut layer_filtered_content: Option<MetaFilteredData> = None;
         let have_derived_layers = !import.layers.is_empty();
         tracing::debug!("Processing layers: {}", import.layers.len());
         for layer in import.layers {
@@ -1024,7 +1024,9 @@ impl ImageImporter {
                         write!(msg, "...and {n_rest} more").unwrap();
                     }
                     tracing::debug!("Found filtered toplevels: {msg}");
-                    layer_filtered_content.insert(layer.layer.digest().to_string(), filtered_owned);
+                    layer_filtered_content
+                        .get_or_insert_default()
+                        .insert(layer.layer.digest().to_string(), filtered_owned);
                 } else {
                     tracing::debug!("No filtered content");
                 }
@@ -1064,8 +1066,6 @@ impl ImageImporter {
             "ostree.importer.version",
             env!("CARGO_PKG_VERSION").to_variant(),
         );
-        let filtered = layer_filtered_content.to_variant();
-        metadata.insert(META_FILTERED, filtered);
         let metadata = metadata.to_variant();
 
         let timestamp = timestamp_of_manifest_or_config(&import.manifest, &import.config)
@@ -1191,6 +1191,7 @@ impl ImageImporter {
         .await?;
         // We can at least avoid re-verifying the base commit.
         state.verify_text = import.verify_text;
+        state.filtered_files = layer_filtered_content;
         Ok(state)
     }
 }
@@ -1354,6 +1355,7 @@ pub fn query_image_commit(repo: &ostree::Repo, commit: &str) -> Result<Box<Layer
         cached_update,
         // we can't cross-reference with a remote here
         verify_text: None,
+        filtered_files: None,
     });
     tracing::debug!("Wrote merge commit {}", state.merge_commit);
     Ok(state)
@@ -1726,36 +1728,26 @@ pub fn count_layer_references(repo: &ostree::Repo) -> Result<u32> {
     Ok(n as u32)
 }
 
-/// Given an image, if it has any non-ostree compatible content, return a suitable
-/// warning message.
+/// Generate a suitable warning message from given list of filtered files, if any.
 pub fn image_filtered_content_warning(
-    repo: &ostree::Repo,
-    image: &ImageReference,
+    filtered_files: &Option<MetaFilteredData>,
 ) -> Result<Option<String>> {
     use std::fmt::Write;
 
-    let ostree_ref = ref_for_image(image)?;
-    let rev = repo.require_rev(&ostree_ref)?;
-    let commit_obj = repo.load_commit(rev.as_str())?.0;
-    let commit_meta = &glib::VariantDict::new(Some(&commit_obj.child_value(0)));
-
-    let r = commit_meta
-        .lookup::<MetaFilteredData>(META_FILTERED)?
-        .filter(|v| !v.is_empty())
-        .map(|v| {
-            let mut filtered = HashMap::<&String, u32>::new();
-            for paths in v.values() {
-                for (k, v) in paths {
-                    let e = filtered.entry(k).or_default();
-                    *e += v;
-                }
+    let r = filtered_files.as_ref().map(|v| {
+        let mut filtered = BTreeMap::<&String, u32>::new();
+        for paths in v.values() {
+            for (k, v) in paths {
+                let e = filtered.entry(k).or_default();
+                *e += v;
             }
-            let mut buf = "Image contains non-ostree compatible file paths:".to_string();
-            for (k, v) in filtered {
-                write!(buf, " {k}: {v}").unwrap();
-            }
-            buf
-        });
+        }
+        let mut buf = "Image contains non-ostree compatible file paths:".to_string();
+        for (k, v) in filtered {
+            write!(buf, " {k}: {v}").unwrap();
+        }
+        buf
+    });
     Ok(r)
 }
 

ostree-ext/src/fixture.rs

@@ -939,6 +939,8 @@ impl Fixture {
         temprootd.create_dir_with("usr/bin", &db)?;
         temprootd.write("usr/bin/newderivedfile", "newderivedfile v0")?;
         temprootd.write("usr/bin/newderivedfile3", "newderivedfile3 v0")?;
+        temprootd.create_dir_with("run", &db)?;
+        temprootd.write("run/filtered", "data")?;
         crate::integrationtest::generate_derived_oci(derived_path, temproot, tag)?;
         Ok(())
     }

ostree-ext/tests/it/main.rs

@@ -1031,7 +1031,7 @@ async fn test_container_chunked() -> Result<()> {
     assert_eq!(store::list_images(fixture.destrepo()).unwrap().len(), 1);
 
     assert!(
-        store::image_filtered_content_warning(fixture.destrepo(), &imgref.imgref)
+        store::image_filtered_content_warning(&import.filtered_files)
             .unwrap()
             .is_none()
     );
@@ -1122,11 +1122,32 @@ r usr/bin/bash bash-v0
 
     // We want to test explicit layer pruning
     imp.disable_gc();
-    let _import = imp.import(prep).await.unwrap();
+    let import = imp.import(prep).await.unwrap();
     assert_eq!(store::list_images(fixture.destrepo()).unwrap().len(), 2);
 
+    assert_eq!(
+        store::image_filtered_content_warning(&import.filtered_files)
+            .unwrap()
+            .unwrap(),
+        "Image contains non-ostree compatible file paths: filtered: 1"
+    );
+
+    // redo it but with the layers already imported and sanity-check that the
+    // merge commit is the same
+    let merge_commit = import.merge_commit;
+    store::remove_image(fixture.destrepo(), &derived_imgref.imgref).unwrap();
+    let mut imp =
+        store::ImageImporter::new(fixture.destrepo(), &derived_imgref, Default::default()).await?;
+    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.context("reimport").unwrap();
+    assert_eq!(import.merge_commit, merge_commit);
+
+    // but this time we don't get the warning because we didn't reimport the derived layer
     assert!(
-        store::image_filtered_content_warning(fixture.destrepo(), &derived_imgref.imgref)
+        store::image_filtered_content_warning(&import.filtered_files)
             .unwrap()
             .is_none()
     );
@@ -1225,7 +1246,7 @@ async fn test_container_var_content() -> Result<()> {
         .query_exists(gio::Cancellable::NONE));
 
     assert!(
-        store::image_filtered_content_warning(fixture.destrepo(), &derived_imgref.imgref)
+        store::image_filtered_content_warning(&import.filtered_files)
             .unwrap()
             .is_none()
     );