Merge pull request #1421 from jlebon/pr/reproducible-pull

Make `ostree container image pull` merge commit reproducible

Author: cgwalters

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

@@ -238,6 +238,10 @@ pub(crate) enum ContainerImageOpts {
         #[clap(value_parser = parse_imgref)]
         imgref: OstreeImageReference,
 
+        /// File to which to write the resulting OSTree commit digest
+        #[clap(long)]
+        ostree_digestfile: Option<Utf8PathBuf>,
+
         #[clap(flatten)]
         proxyopts: ContainerProxyOpts,
 
@@ -863,13 +867,15 @@ async fn container_info(imgref: &OstreeImageReference) -> Result<()> {
 async fn container_store(
     repo: &ostree::Repo,
     imgref: &OstreeImageReference,
+    ostree_digestfile: Option<Utf8PathBuf>,
     proxyopts: ContainerProxyOpts,
     quiet: bool,
     check: Option<Utf8PathBuf>,
 ) -> Result<()> {
     let mut imp = ImageImporter::new(repo, imgref, proxyopts.into()).await?;
     let prep = match imp.prepare().await? {
         PrepareResult::AlreadyPresent(c) => {
+            write_digest_file(ostree_digestfile, &c.merge_commit)?;
             println!("No changes in {} => {}", imgref, c.merge_commit);
             return Ok(());
         }
@@ -906,17 +912,26 @@ 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}")
     }
     if let Some(ref text) = import.verify_text {
         println!("{text}");
     }
+    write_digest_file(ostree_digestfile, &import.merge_commit)?;
     println!("Wrote: {} => {}", imgref, import.merge_commit);
     Ok(())
 }
 
+fn write_digest_file(digestfile: Option<Utf8PathBuf>, digest: &str) -> Result<()> {
+    if let Some(digestfile) = digestfile.as_deref() {
+        let rootfs = Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
+        rootfs.write(digestfile.as_str().trim_start_matches('/'), digest)?;
+    }
+    Ok(())
+}
+
 /// Output the container image history
 async fn container_history(repo: &ostree::Repo, imgref: &ImageReference) -> Result<()> {
     let img = crate::container::store::query_image(repo, imgref)?
@@ -1095,12 +1110,14 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
                 ContainerImageOpts::Pull {
                     repo,
                     imgref,
+                    ostree_digestfile,
                     proxyopts,
                     quiet,
                     check,
                 } => {
                     let repo = parse_repo(&repo)?;
-                    container_store(&repo, &imgref, proxyopts, quiet, check).await
+                    container_store(&repo, &imgref, ostree_digestfile, proxyopts, quiet, check)
+                        .await
                 }
                 ContainerImageOpts::Reexport {
                     repo,
@@ -1263,7 +1280,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 +1337,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");
                 }
@@ -1047,7 +1049,7 @@ impl ImageImporter {
         let _ = self.layer_byte_progress.take();
         let _ = self.layer_progress.take();
 
-        let mut metadata = HashMap::new();
+        let mut metadata = BTreeMap::new();
         metadata.insert(
             META_MANIFEST_DIGEST,
             import.manifest_digest.to_string().to_variant(),
@@ -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()
     );