diff --git a/crates/etc-merge/src/lib.rs b/crates/etc-merge/src/lib.rs index 1ea0d1f72..9858788fe 100644 --- a/crates/etc-merge/src/lib.rs +++ b/crates/etc-merge/src/lib.rs @@ -78,6 +78,12 @@ fn stat_eq_ignore_mtime(this: &Stat, other: &Stat) -> bool { return true; } +#[derive(Debug)] +pub struct UnmergablePaths { + pub path: PathBuf, + pub reason: String, +} + /// Represents the differences between two directory trees. #[derive(Debug)] pub struct Diff { @@ -88,6 +94,8 @@ pub struct Diff { modified: Vec, /// Paths that exist in the pristine /etc but not in the current one removed: Vec, + /// Paths that are unmergable + pub unmergable_paths: Vec, } fn collect_all_files( @@ -170,6 +178,47 @@ fn get_deletions( Ok(()) } +fn check_if_mergable( + new: &Directory, + current_inode: &Inode, + current_path: &PathBuf, + diff: &mut Diff, +) { + match current_inode { + // If currently 'file' is a directory, make sure it's not a regular file + // new_etc as well, else we can't merge + Inode::Directory(..) => { + let new_dir = new.get_directory(¤t_path.as_os_str()); + + if matches!(new_dir, Err(ImageError::NotADirectory(..))) { + diff.unmergable_paths.push(UnmergablePaths { + path: current_path.clone(), + reason: format!( + "Directory '{}' now defaults to a file in new etc", + current_path.display() + ), + }); + } + } + + // If currently 'file' is not a directory, make sure it's not a directory in the + // new_etc either, else we can't merge + Inode::Leaf(..) => { + let new_dir = new.get_directory(¤t_path.as_os_str()); + + if matches!(new_dir, Ok(..)) { + diff.unmergable_paths.push(UnmergablePaths { + path: current_path.clone(), + reason: format!( + "File '{}' now defaults to a directory in new etc", + current_path.display() + ), + }); + } + } + } +} + // 1. Files in the currently booted deployment’s /etc which were modified from the default /usr/etc (of the same deployment) are retained. // // 2. Files in the currently booted deployment’s /etc which were not modified from the default /usr/etc (of the same deployment) @@ -190,6 +239,7 @@ fn get_modifications( current: &Directory, pristine_leaves: &[Leaf], current_leaves: &[Leaf], + new_leaves: &[Leaf], new: &Directory, mut current_path: PathBuf, diff: &mut Diff, @@ -216,26 +266,45 @@ fn get_modifications( &curr_dir, pristine_leaves, current_leaves, + new_leaves, new, current_path.clone(), diff, )?; - // This directory or its contents were modified/added - // Check if the new directory was deleted from new_etc - // If it was, we want to add the directory back - if new.get_directory_opt(¤t_path.as_os_str())?.is_none() { - if diff.added.len() != total_added { - diff.added.insert(total_added, current_path.clone()); - } else if diff.modified.len() != total_modified { - diff.modified.insert(total_modified, current_path.clone()); + match new.get_directory(¤t_path.as_os_str()) { + Ok(..) => { + // Directory exists in both current and new etc. + // Modifications/additions within this directory are handled recursively. + // No additional action needed here. + } + + Err(ImageError::NotFound(..)) | Err(ImageError::NotADirectory(..)) => { + // This directory was deleted in new_etc + // If it was modified in the current etc, we want this back + // + // Was a directory in current etc, but is now + // a file/symlink in the new etc + if diff.added.len() != total_added { + diff.added.insert(total_added, current_path.clone()); + } else if diff.modified.len() != total_modified { + diff.modified.insert(total_added, current_path.clone()); + } } + + Err(e) => Err(e)?, + } + + if diff.modified.len() != total_modified || diff.added.len() != total_added + { + check_if_mergable(new, inode, ¤t_path, diff); } } Err(ImageError::NotFound(..)) => { // Dir not found in original /etc, dir was added diff.added.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); // Also add every file inside that dir collect_all_files(&curr_dir, current_path.clone(), &mut diff.added); @@ -245,9 +314,10 @@ fn get_modifications( // Some directory was changed to a file/symlink // This should be counted in the diff, but we don't really merge this diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } - Err(e) => Err(e)?, + Err(e) => Err(e).with_context(|| format!("Opening pristine {path:?}"))?, } } @@ -255,8 +325,10 @@ fn get_modifications( Ok(old_leaf_id) => { let leaf = ¤t_leaves[leaf_id.0]; let old_leaf = &pristine_leaves[old_leaf_id.0]; + if !stat_eq_ignore_mtime(&old_leaf.stat, &leaf.stat) { diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); current_path.pop(); continue; } @@ -266,6 +338,7 @@ fn get_modifications( if old_meta.content_hash != current_meta.content_hash { // File modified in some way diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } } @@ -273,12 +346,14 @@ fn get_modifications( if old_link != current_link { // Symlink modified in some way diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } } (Symlink(..), Regular(..)) | (Regular(..), Symlink(..)) => { // File changed to symlink or vice-versa diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } (a, b) => { @@ -290,14 +365,16 @@ fn get_modifications( Err(ImageError::IsADirectory(..)) => { // A directory was changed to a file diff.modified.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } Err(ImageError::NotFound(..)) => { // File not found in original /etc, file was added diff.added.push(current_path.clone()); + check_if_mergable(new, inode, ¤t_path, diff); } - Err(e) => Err(e).context(format!("{path:?}"))?, + Err(e) => Err(e).with_context(|| format!("Opening pristine {path:?}"))?, }, } @@ -385,6 +462,7 @@ pub fn compute_diff( added: vec![], modified: vec![], removed: vec![], + unmergable_paths: vec![], }; get_modifications( @@ -392,6 +470,7 @@ pub fn compute_diff( ¤t_etc_files.root, &pristine_etc_files.leaves, ¤t_etc_files.leaves, + &new_etc_files.leaves, &new_etc_files.root, PathBuf::new(), &mut diff, @@ -407,6 +486,19 @@ pub fn compute_diff( Ok(diff) } +pub fn print_unmergable_paths(diff: &Diff, writer: &mut impl Write) { + use owo_colors::OwoColorize; + + for unmergable in &diff.unmergable_paths { + let _ = writeln!( + writer, + "{} {}", + ModificationType::Unmergable.magenta(), + unmergable.reason + ); + } +} + /// Prints a colorized summary of differences to standard output. pub fn print_diff(diff: &Diff, writer: &mut impl Write) { use owo_colors::OwoColorize; @@ -422,6 +514,8 @@ pub fn print_diff(diff: &Diff, writer: &mut impl Write) { for removed in &diff.removed { let _ = writeln!(writer, "{} {removed:?}", ModificationType::Removed.red()); } + + print_unmergable_paths(diff, writer); } #[context("Collecting xattrs")] @@ -589,6 +683,7 @@ enum ModificationType { Added, Modified, Removed, + Unmergable, } impl std::fmt::Display for ModificationType { @@ -603,6 +698,7 @@ impl ModificationType { ModificationType::Added => "+", ModificationType::Modified => "~", ModificationType::Removed => "-", + ModificationType::Unmergable => "*", } } } @@ -613,23 +709,35 @@ fn create_dir_with_perms( stat: &Stat, new_inode: Option<&Inode>, ) -> anyhow::Result<()> { - // The new directory is not present in the new_etc, so we create it, else we only copy the - // metadata - if new_inode.is_none() { - // Here we use `create_dir_all` to create every parent as we will set the permissions later - // on. Due to the fact that we have an ordered (sorted) list of directories and directory - // entries and we have a DFS traversal, we will always have directory creation starting from - // the parent anyway. - // - // The exception being, if a directory is modified in the current_etc, and a new directory - // is added inside the modified directory, say `dir/prems` has its permissions modified and - // `dir/prems/new` is the new directory created. Since we handle added files/directories first, - // we will create the directories `perms/new` with directory `new` also getting its - // permissions set, but `perms` will not. `perms` will have its permissions set up when we - // handle the modified directories. - new_etc_fd - .create_dir_all(&dir_name) - .context(format!("Failed to create dir {dir_name:?}"))?; + match new_inode { + Some(inode) => match inode { + Inode::Directory(..) => { /* no-op */ } + + Inode::Leaf(..) => { + anyhow::bail!( + "Modified config directory {dir_name:?} newly defaults to file. Cannot merge" + ) + } + }, + + // The new directory is not present in the new_etc, so we create it, else we only copy the + // metadata + None => { + // Here we use `create_dir_all` to create every parent as we will set the permissions later + // on. Due to the fact that we have an ordered (sorted) list of directories and directory + // entries and we have a DFS traversal, we will always have directory creation starting from + // the parent anyway. + // + // The exception being, if a directory is modified in the current_etc, and a new directory + // is added inside the modified directory, say `dir/prems` has its permissions modified and + // `dir/prems/new` is the new directory created. Since we handle added files/directories first, + // we will create the directories `perms/new` with directory `new` also getting its + // permissions set, but `perms` will not. `perms` will have its permissions set up when we + // handle the modified directories. + new_etc_fd + .create_dir_all(&dir_name) + .context(format!("Failed to create dir {dir_name:?}"))?; + } } new_etc_fd @@ -730,12 +838,14 @@ fn merge_modified_files( file, current_inode.stat(current_leaves), new_inode, - )?; + ) + .context("Merging directory")?; } Inode::Leaf(leaf_id, _) => { let leaf = ¤t_leaves[leaf_id.0]; - merge_leaf(current_etc_fd, new_etc_fd, leaf, new_inode, file)? + merge_leaf(current_etc_fd, new_etc_fd, leaf, new_inode, file) + .context("Merging leaf")? } }; } @@ -755,7 +865,7 @@ fn merge_modified_files( } }, - Err(e) => Err(e)?, + Err(e) => Err(e).with_context(|| format!("Opening {file:?} in new etc"))?, }; } diff --git a/crates/lib/src/bootc_composefs/finalize.rs b/crates/lib/src/bootc_composefs/finalize.rs index 9e0f5e3d4..23dcf8a72 100644 --- a/crates/lib/src/bootc_composefs/finalize.rs +++ b/crates/lib/src/bootc_composefs/finalize.rs @@ -14,12 +14,17 @@ use cap_std_ext::cap_std::{ambient_authority, fs::Dir}; use cap_std_ext::dirext::CapStdExtDirExt; use composefs::generic_tree::{FileSystem, Stat}; use composefs_ctl::composefs; -use etc_merge::{compute_diff, merge, print_diff, traverse_etc}; +use etc_merge::{Diff, compute_diff, merge, print_diff, traverse_etc}; use rustix::fs::fsync; use fn_error_context::context; -pub(crate) async fn get_etc_diff(storage: &Storage, booted_cfs: &BootedComposefs) -> Result<()> { +pub(crate) async fn get_etc_diff( + storage: &Storage, + booted_cfs: &BootedComposefs, + new_etc: Option<&Dir>, + print: bool, +) -> Result { let host = get_composefs_status(storage, booted_cfs).await?; let booted_composefs = host.require_composefs_booted()?; @@ -37,16 +42,22 @@ pub(crate) async fn get_etc_diff(storage: &Storage, booted_cfs: &BootedComposefs Dir::open_ambient_dir(erofs_tmp_mnt.dir.path().join("etc"), ambient_authority())?; let current_etc = Dir::open_ambient_dir("/etc", ambient_authority())?; - let (pristine_files, current_files, _) = traverse_etc(&pristine_etc, ¤t_etc, None)?; + let (pristine_files, current_files, new_etc_files) = + traverse_etc(&pristine_etc, ¤t_etc, new_etc)?; + let diff = compute_diff( &pristine_files, ¤t_files, - &FileSystem::new(Stat::uninitialized()), + &new_etc_files + .as_ref() + .unwrap_or(&FileSystem::new(Stat::uninitialized())), )?; - print_diff(&diff, &mut std::io::stdout()); + if print { + print_diff(&diff, &mut std::io::stdout()); + } - Ok(()) + Ok(diff) } pub(crate) async fn composefs_backend_finalize( diff --git a/crates/lib/src/bootc_composefs/update.rs b/crates/lib/src/bootc_composefs/update.rs index 1f48021c7..90e7af130 100644 --- a/crates/lib/src/bootc_composefs/update.rs +++ b/crates/lib/src/bootc_composefs/update.rs @@ -7,10 +7,12 @@ use composefs_ctl::composefs; use composefs_ctl::composefs_boot; use composefs_ctl::composefs_oci; use composefs_oci::image::create_filesystem; +use etc_merge::print_unmergable_paths; use fn_error_context::context; use ocidir::cap_std::ambient_authority; use ostree_ext::container::ManifestDiff; +use crate::bootc_composefs::finalize::get_etc_diff; use crate::bootc_composefs::gc::GCOpts; use crate::spec::BootloaderKind; use crate::{ @@ -288,6 +290,18 @@ pub(crate) async fn do_upgrade( .context("Failed to mount composefs image")?, )?; + // Check if three way etc merge is possible without conflicts + let new_etc = mounted_fs + .open_dir("etc") + .context("Opening deployment's etc")?; + + let diff = get_etc_diff(storage, booted_cfs, Some(&new_etc), false).await?; + + if !diff.unmergable_paths.is_empty() { + print_unmergable_paths(&diff, &mut std::io::stderr()); + anyhow::bail!("Merge conflicts found in etc"); + } + let boot_type = BootType::from(entry); let boot_digest = match boot_type { @@ -307,14 +321,16 @@ pub(crate) async fn do_upgrade( )?, }; + let staged_state = StagedDeployment { + depl_id: id.to_hex(), + finalization_locked: opts.download_only, + }; + write_composefs_state( &Utf8PathBuf::from("/sysroot"), &id, imgref, - Some(StagedDeployment { - depl_id: id.to_hex(), - finalization_locked: opts.download_only, - }), + Some(staged_state), boot_type, boot_digest, &manifest_digest, @@ -393,16 +409,20 @@ pub(crate) async fn upgrade_composefs( start_finalize_stated_svc()?; - // Make the staged deployment not download_only - let new_staged = StagedDeployment { - depl_id: staged.require_composefs()?.verity.clone(), - finalization_locked: false, - }; - let staged_depl_dir = Dir::open_ambient_dir(COMPOSEFS_TRANSIENT_STATE_DIR, ambient_authority()) .context("Opening transient state directory")?; + let current = staged_depl_dir + .read_to_string(COMPOSEFS_STAGED_DEPLOYMENT_FNAME) + .context("Reading staged file")?; + + let mut new_staged: StagedDeployment = + serde_json::from_str(¤t).context("Deserialzing staged file")?; + + // Make the staged deployment not download_only + new_staged.finalization_locked = false; + staged_depl_dir .atomic_replace_with( COMPOSEFS_STAGED_DEPLOYMENT_FNAME, diff --git a/crates/lib/src/cli.rs b/crates/lib/src/cli.rs index b70eb44ce..d66c0d246 100644 --- a/crates/lib/src/cli.rs +++ b/crates/lib/src/cli.rs @@ -2455,7 +2455,8 @@ async fn run_from_opt(opt: Opt) -> Result<()> { anyhow::bail!("ConfigDiff is only supported for composefs backend") } BootedStorageKind::Composefs(booted_cfs) => { - get_etc_diff(storage, &booted_cfs).await + get_etc_diff(storage, &booted_cfs, None, true).await?; + Ok(()) } } } diff --git a/crates/lib/src/store/mod.rs b/crates/lib/src/store/mod.rs index 78926d173..d7274d868 100644 --- a/crates/lib/src/store/mod.rs +++ b/crates/lib/src/store/mod.rs @@ -249,7 +249,6 @@ impl<'a> BootedOstree<'a> { } /// Represents a composefs-based boot environment -#[allow(dead_code)] pub struct BootedComposefs { pub repo: Arc, pub cmdline: &'static ComposefsCmdline, diff --git a/tmt/plans/integration.fmf b/tmt/plans/integration.fmf index 43e3f2cbc..e032999f6 100644 --- a/tmt/plans/integration.fmf +++ b/tmt/plans/integration.fmf @@ -285,4 +285,11 @@ execute: how: fmf test: - /tmt/tests/tests/test-45-composefs-corruped-state-resilience + +/plan-46-etc-merge-conflict: + summary: Verify etc merge conflicts are caught during upgrade, not finalization + discover: + how: fmf + test: + - /tmt/tests/tests/test-46-etc-merge-conflict # END GENERATED PLANS diff --git a/tmt/tests/booted/test-etc-merge-conflict.nu b/tmt/tests/booted/test-etc-merge-conflict.nu new file mode 100644 index 000000000..09dcdb8d4 --- /dev/null +++ b/tmt/tests/booted/test-etc-merge-conflict.nu @@ -0,0 +1,78 @@ +# number: 46 +# tmt: +# summary: Verify etc merge conflicts are caught during upgrade, not finalization +# duration: 15m +# +# Verifies that file-to-directory and directory-to-file type changes between +# the current /etc and the new image's /etc are detected during `bootc switch` +# (the upgrade phase) and cause it to fail immediately, rather than silently +# staging and failing during finalization at reboot. +# +# No reboot needed: the upgrade is expected to fail before staging. +use std assert +use tap.nu + +if not (tap is_composefs) { + exit 0 +} + +tap begin "etc merge conflict detection during upgrade" + +def main [] { + bootc image copy-to-storage + + # Test case 1: File in current /etc conflicts with directory in new image's /etc. + # Create a file on the live system; the new image will have a directory at + # the same path. + "local modification" | save /etc/test-etc-merge-conflict + + let td = mktemp -d + cd $td + + let dockerfile = ' +FROM localhost/bootc as base +RUN mkdir -p /etc/test-etc-merge-conflict && \ + echo "inner" > /etc/test-etc-merge-conflict/inner.conf +' + (tap make_uki_containerfile $dockerfile) | save Dockerfile + podman build -t localhost/bootc-etc-conflict . + + let result = do { bootc switch --transport containers-storage localhost/bootc-etc-conflict } | complete + print $"exit_code: ($result.exit_code)" + print $"stderr: ($result.stderr)" + + assert ($result.exit_code != 0) "bootc switch should fail: locally-added file conflicts with directory in new etc" + assert ($result.stderr | str contains "Merge conflicts found in etc") $"Expected 'Merge conflicts found in etc' in stderr, got: ($result.stderr)" + + # Verify no deployment was staged - the failure must happen during upgrade, + # not during finalization at reboot. + assert ((bootc status --json | from json | get status.staged) == null) "No deployment should be staged after merge conflict failure" + + # Test case 2: Directory in current /etc conflicts with file in new image's /etc. + # Create a directory with a file inside on the live system; the new image + # will have a regular file at the same path. + mkdir /etc/test-etc-merge-dir + "local config" | save /etc/test-etc-merge-dir/local.conf + + let td2 = mktemp -d + cd $td2 + + let dockerfile2 = ' +FROM localhost/bootc as base +RUN echo "now a file" > /etc/test-etc-merge-dir +' + (tap make_uki_containerfile $dockerfile2) | save Dockerfile + podman build -t localhost/bootc-etc-conflict-2 . + + let result2 = do { bootc switch --transport containers-storage localhost/bootc-etc-conflict-2 } | complete + print $"exit_code: ($result2.exit_code)" + print $"stderr: ($result2.stderr)" + + assert ($result2.exit_code != 0) "bootc switch should fail: locally-added directory conflicts with file in new etc" + assert ($result2.stderr | str contains "Merge conflicts found in etc") $"Expected 'Merge conflicts found in etc' in stderr, got: ($result2.stderr)" + + # Again verify nothing was staged + assert ((bootc status --json | from json | get status.staged) == null) "No deployment should be staged after directory-to-file conflict failure" + + tap ok +} diff --git a/tmt/tests/tests.fmf b/tmt/tests/tests.fmf index a8b5b91f8..0c12b5d4a 100644 --- a/tmt/tests/tests.fmf +++ b/tmt/tests/tests.fmf @@ -178,3 +178,8 @@ check: summary: Test composefs backend resilience to state corruption duration: 30m test: nu booted/test-composefs-corruped-state-resilience.nu + +/test-46-etc-merge-conflict: + summary: Verify etc merge conflicts are caught during upgrade, not finalization + duration: 15m + test: nu booted/test-etc-merge-conflict.nu