From 0c6ccad8ddaa8166fc56c550f369dfc59be49dfd Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 29 Jul 2024 17:31:44 -0400 Subject: [PATCH 1/5] Bump MSRV to 1.79 It's what's in c9s right now, and I want to make use of a newer API. Signed-off-by: Colin Walters --- cli/Cargo.toml | 3 ++- lib/Cargo.toml | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/cli/Cargo.toml b/cli/Cargo.toml index ff2af2411..6f6d841f9 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -6,7 +6,8 @@ license = "MIT OR Apache-2.0" repository = "https://github.com/containers/bootc" readme = "README.md" publish = false -rust-version = "1.63.0" +# For now don't bump this above what is currently shipped in RHEL9. +rust-version = "1.75.0" default-run = "bootc" # See https://github.com/coreos/cargo-vendor-filterer diff --git a/lib/Cargo.toml b/lib/Cargo.toml index 2b50795f3..aa071c5f0 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -6,6 +6,8 @@ name = "bootc-lib" readme = "README.md" repository = "https://github.com/containers/bootc" version = "0.1.14" +# For now don't bump this above what is currently shipped in RHEL9; +# also keep in sync with the version in cli. rust-version = "1.75.0" build = "build.rs" From bbd63314a38827227c0db3a3fa49c58fe72cf4f1 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 26 Jul 2024 14:56:41 -0400 Subject: [PATCH 2/5] utils: Allow non-static str for spinner I have a use case for dynamic strings, this just requires duplicating the string for the progress bar. Signed-off-by: Colin Walters --- lib/src/utils.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/utils.rs b/lib/src/utils.rs index 1b1d0cd9d..5ceae99dd 100644 --- a/lib/src/utils.rs +++ b/lib/src/utils.rs @@ -132,14 +132,14 @@ pub(crate) fn medium_visibility_warning(s: &str) { /// with an automatic spinner to show that we're not blocked. /// Note that generally the called function should not output /// anything to stdout as this will interfere with the spinner. -pub(crate) async fn async_task_with_spinner(msg: &'static str, f: F) -> T +pub(crate) async fn async_task_with_spinner(msg: &str, f: F) -> T where F: Future, { let pb = indicatif::ProgressBar::new_spinner(); let style = indicatif::ProgressStyle::default_bar(); pb.set_style(style.template("{spinner} {msg}").unwrap()); - pb.set_message(msg); + pb.set_message(msg.to_string()); pb.enable_steady_tick(Duration::from_millis(150)); // We need to handle the case where we aren't connected to // a tty, so indicatif would show nothing by default. From 3424b314ece09130b9cd17f7f0a8f3164d666749 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 29 Jul 2024 14:33:07 -0400 Subject: [PATCH 3/5] lib: Enable mount API for rustix Will be used by future changes. Signed-off-by: Colin Walters --- lib/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Cargo.toml b/lib/Cargo.toml index aa071c5f0..c2639b64a 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -32,7 +32,7 @@ liboverdrop = "0.1.0" libsystemd = "0.7" openssl = "^0.10.64" regex = "1.10.4" -rustix = { "version" = "0.38.34", features = ["thread", "fs", "system", "process"] } +rustix = { "version" = "0.38.34", features = ["thread", "fs", "system", "process", "mount"] } schemars = { version = "0.8.17", features = ["chrono"] } serde = { workspace = true, features = ["derive"] } serde_ignored = "0.1.10" From 42412fa8db9bdfddf289100a4cc2cabc8c1c72ec Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 29 Jul 2024 14:34:43 -0400 Subject: [PATCH 4/5] tests: Add generic post-install verification Prep for more work. Signed-off-by: Colin Walters --- tests-integration/src/install.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests-integration/src/install.rs b/tests-integration/src/install.rs index 73b9f30e7..87a0b6dcf 100644 --- a/tests-integration/src/install.rs +++ b/tests-integration/src/install.rs @@ -2,6 +2,7 @@ use std::path::Path; use std::{os::fd::AsRawFd, path::PathBuf}; use anyhow::Result; +use camino::Utf8Path; use cap_std_ext::cap_std; use cap_std_ext::cap_std::fs::Dir; use fn_error_context::context; @@ -53,6 +54,12 @@ fn find_deployment_root() -> Result { anyhow::bail!("Failed to find deployment root") } +// Hook relatively cheap post-install tests here +fn generic_post_install_verification() -> Result<()> { + assert!(Utf8Path::new("/ostree/repo").try_exists()?); + Ok(()) +} + #[context("Install tests")] pub(crate) fn run_alongside(image: &str, mut testargs: libtest_mimic::Arguments) -> Result<()> { // Force all of these tests to be serial because they mutate global state @@ -88,6 +95,8 @@ pub(crate) fn run_alongside(image: &str, mut testargs: libtest_mimic::Arguments) std::fs::write(&tmp_keys, b"ssh-ed25519 ABC0123 testcase@example.com")?; cmd!(sh, "sudo {BASE_ARGS...} {target_args...} -v {tmp_keys}:/test_authorized_keys {image} bootc install to-filesystem {generic_inst_args...} --acknowledge-destructive --karg=foo=bar --replace=alongside --root-ssh-authorized-keys=/test_authorized_keys /target").run()?; + generic_post_install_verification()?; + // Test kargs injected via CLI cmd!( sh, @@ -120,6 +129,7 @@ pub(crate) fn run_alongside(image: &str, mut testargs: libtest_mimic::Arguments) let sh = &xshell::Shell::new()?; reset_root(sh)?; cmd!(sh, "sudo {BASE_ARGS...} {target_args...} {image} bootc install to-existing-root --acknowledge-destructive {generic_inst_args...}").run()?; + generic_post_install_verification()?; let root = &Dir::open_ambient_dir("/ostree", cap_std::ambient_authority()).unwrap(); let mut path = PathBuf::from("."); crate::selinux::verify_selinux_recurse(root, &mut path, false)?; @@ -131,6 +141,7 @@ pub(crate) fn run_alongside(image: &str, mut testargs: libtest_mimic::Arguments) let empty = sh.create_temp_dir()?; let empty = empty.path().to_str().unwrap(); cmd!(sh, "sudo {BASE_ARGS...} {target_args...} -v {empty}:/usr/lib/bootc/install {image} bootc install to-existing-root {generic_inst_args...}").run()?; + generic_post_install_verification()?; Ok(()) }), ]; From 81556a43be8fe649e7c47ccd2e63d4a67ece2c03 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 29 Jul 2024 16:08:57 -0400 Subject: [PATCH 5/5] utils: Capture stderr, add async We want to capture stderr by default in these methods so we provide useful errors. Also add an async variant. Signed-off-by: Colin Walters --- lib/src/utils.rs | 117 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 108 insertions(+), 9 deletions(-) diff --git a/lib/src/utils.rs b/lib/src/utils.rs index 5ceae99dd..326faa6a4 100644 --- a/lib/src/utils.rs +++ b/lib/src/utils.rs @@ -1,5 +1,5 @@ use std::future::Future; -use std::io::Write; +use std::io::{Read, Seek, Write}; use std::os::fd::BorrowedFd; use std::process::Command; use std::time::Duration; @@ -15,17 +15,79 @@ pub(crate) trait CommandRunExt { fn run(&mut self) -> Result<()>; } +/// Helpers intended for [`std::process::ExitStatus`]. +pub(crate) trait ExitStatusExt { + /// If the exit status signals it was not successful, return an error. + /// Note that we intentionally *don't* include the command string + /// in the output; we leave it to the caller to add that if they want, + /// as it may be verbose. + fn check_status(&mut self, stderr: std::fs::File) -> Result<()>; +} + +/// Parse the last chunk (e.g. 1024 bytes) from the provided file, +/// ensure it's UTF-8, and return that value. This function is infallible; +/// if the file cannot be read for some reason, a copy of a static string +/// is returned. +fn last_utf8_content_from_file(mut f: std::fs::File) -> String { + // u16 since we truncate to just the trailing bytes here + // to avoid pathological error messages + const MAX_STDERR_BYTES: u16 = 1024; + let size = f + .metadata() + .map_err(|e| { + tracing::warn!("failed to fstat: {e}"); + }) + .map(|m| m.len().try_into().unwrap_or(u16::MAX)) + .unwrap_or(0); + let size = size.min(MAX_STDERR_BYTES); + let seek_offset = -(size as i32); + let mut stderr_buf = Vec::with_capacity(size.into()); + // We should never fail to seek()+read() really, but let's be conservative + let r = match f + .seek(std::io::SeekFrom::End(seek_offset.into())) + .and_then(|_| f.read_to_end(&mut stderr_buf)) + { + Ok(_) => String::from_utf8_lossy(&stderr_buf), + Err(e) => { + tracing::warn!("failed seek+read: {e}"); + "".into() + } + }; + (&*r).to_owned() +} + +impl ExitStatusExt for std::process::ExitStatus { + fn check_status(&mut self, stderr: std::fs::File) -> Result<()> { + let stderr_buf = last_utf8_content_from_file(stderr); + if self.success() { + return Ok(()); + } + anyhow::bail!(format!("Subprocess failed: {self:?}\n{stderr_buf}")) + } +} + impl CommandRunExt for Command { /// Synchronously execute the child, and return an error if the child exited unsuccessfully. fn run(&mut self) -> Result<()> { - let st = self.status()?; - if !st.success() { - // Note that we intentionally *don't* include the command string - // in the output; we leave it to the caller to add that if they want, - // as it may be verbose. - anyhow::bail!(format!("Subprocess failed: {st:?}")) - } - Ok(()) + let stderr = tempfile::tempfile()?; + self.stderr(stderr.try_clone()?); + self.status()?.check_status(stderr) + } +} + +/// Helpers intended for [`tokio::process::Command`]. +#[allow(dead_code)] +pub(crate) trait AsyncCommandRunExt { + async fn run(&mut self) -> Result<()>; +} + +impl AsyncCommandRunExt for tokio::process::Command { + /// Asynchronously execute the child, and return an error if the child exited unsuccessfully. + /// + async fn run(&mut self) -> Result<()> { + let stderr = tempfile::tempfile()?; + self.stderr(stderr.try_clone()?); + self.status().await?.check_status(stderr) } } @@ -212,6 +274,43 @@ fn test_sigpolicy_from_opts() { #[test] fn command_run_ext() { + // The basics Command::new("true").run().unwrap(); assert!(Command::new("false").run().is_err()); + + // Verify we capture stderr + let e = Command::new("/bin/sh") + .args(["-c", "echo expected-this-oops-message 1>&2; exit 1"]) + .run() + .err() + .unwrap(); + similar_asserts::assert_eq!( + e.to_string(), + "Subprocess failed: ExitStatus(unix_wait_status(256))\nexpected-this-oops-message\n" + ); + + // Ignoring invalid UTF-8 + let e = Command::new("/bin/sh") + .args([ + "-c", + r"echo -e 'expected\xf5\x80\x80\x80\x80-foo\xc0bar\xc0\xc0' 1>&2; exit 1", + ]) + .run() + .err() + .unwrap(); + similar_asserts::assert_eq!( + e.to_string(), + "Subprocess failed: ExitStatus(unix_wait_status(256))\nexpected�����-foo�bar��\n" + ); +} + +#[tokio::test] +async fn async_command_run_ext() { + use tokio::process::Command as AsyncCommand; + let mut success = AsyncCommand::new("true"); + let mut fail = AsyncCommand::new("false"); + // Run these in parallel just because we can + let (success, fail) = tokio::join!(success.run(), fail.run(),); + success.unwrap(); + assert!(fail.is_err()); }