From beaba469815d4f9cd062d7da3ae6bb9e6b5bd924 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 8 Oct 2024 21:56:11 -0700 Subject: [PATCH] [5/n] [test-utils] turn redactor into the builder pattern (#6786) In an upcoming PR I want to redact everything except UUIDs. The most convenient way to do that is to use the builder pattern for the redactor. --- dev-tools/omdb/tests/test_all_output.rs | 55 ++---- .../reconfigurator-cli/tests/test_basic.rs | 4 +- test-utils/src/dev/test_cmds.rs | 170 ++++++++++-------- 3 files changed, 109 insertions(+), 120 deletions(-) diff --git a/dev-tools/omdb/tests/test_all_output.rs b/dev-tools/omdb/tests/test_all_output.rs index be14d07765..57594fcca5 100644 --- a/dev-tools/omdb/tests/test_all_output.rs +++ b/dev-tools/omdb/tests/test_all_output.rs @@ -16,9 +16,8 @@ use nexus_types::deployment::Blueprint; use nexus_types::deployment::SledFilter; use nexus_types::deployment::UnstableReconfiguratorState; use omicron_test_utils::dev::test_cmds::path_to_executable; -use omicron_test_utils::dev::test_cmds::redact_extra; use omicron_test_utils::dev::test_cmds::run_command; -use omicron_test_utils::dev::test_cmds::ExtraRedactions; +use omicron_test_utils::dev::test_cmds::Redactor; use slog_error_chain::InlineErrorChain; use std::fmt::Write; use std::net::IpAddr; @@ -203,19 +202,21 @@ async fn test_omdb_success_cases(cptestctx: &ControlPlaneTestContext) { // ControlPlaneTestContext. ]; - let mut redactions = ExtraRedactions::new(); - redactions - .variable_length("tmp_path", tmppath.as_str()) - .fixed_length("blueprint_id", &initial_blueprint_id) - .variable_length( + let mut redactor = Redactor::default(); + redactor + .extra_variable_length("tmp_path", tmppath.as_str()) + .extra_fixed_length("blueprint_id", &initial_blueprint_id) + .extra_variable_length( "cockroachdb_fingerprint", &initial_blueprint.cockroachdb_fingerprint, ); + let crdb_version = initial_blueprint.cockroachdb_setting_preserve_downgrade.to_string(); if initial_blueprint.cockroachdb_setting_preserve_downgrade.is_set() { - redactions.variable_length("cockroachdb_version", &crdb_version); + redactor.extra_variable_length("cockroachdb_version", &crdb_version); } + for args in invocations { println!("running commands with args: {:?}", args); let p = postgres_url.to_string(); @@ -234,7 +235,7 @@ async fn test_omdb_success_cases(cptestctx: &ControlPlaneTestContext) { }, &cmd_path, args, - Some(&redactions), + &redactor, ) .await; } @@ -444,14 +445,7 @@ async fn do_run( ) where F: FnOnce(Exec) -> Exec + Send + 'static, { - do_run_extra( - output, - modexec, - cmd_path, - args, - Some(&ExtraRedactions::new()), - ) - .await; + do_run_extra(output, modexec, cmd_path, args, &Redactor::default()).await; } async fn do_run_no_redactions( @@ -462,7 +456,7 @@ async fn do_run_no_redactions( ) where F: FnOnce(Exec) -> Exec + Send + 'static, { - do_run_extra(output, modexec, cmd_path, args, None).await; + do_run_extra(output, modexec, cmd_path, args, &Redactor::noop()).await; } async fn do_run_extra( @@ -470,7 +464,7 @@ async fn do_run_extra( modexec: F, cmd_path: &Path, args: &[&str], - extra_redactions: Option<&ExtraRedactions<'_>>, + redactor: &Redactor<'_>, ) where F: FnOnce(Exec) -> Exec + Send + 'static, { @@ -478,14 +472,7 @@ async fn do_run_extra( output, "EXECUTING COMMAND: {} {:?}\n", cmd_path.file_name().expect("missing command").to_string_lossy(), - args.iter() - .map(|r| { - extra_redactions.map_or_else( - || r.to_string(), - |redactions| redact_extra(r, redactions), - ) - }) - .collect::>() + args.iter().map(|r| redactor.do_redact(r)).collect::>() ) .unwrap(); @@ -521,21 +508,11 @@ async fn do_run_extra( write!(output, "termination: {:?}\n", exit_status).unwrap(); write!(output, "---------------------------------------------\n").unwrap(); write!(output, "stdout:\n").unwrap(); - - if let Some(extra_redactions) = extra_redactions { - output.push_str(&redact_extra(&stdout_text, extra_redactions)); - } else { - output.push_str(&stdout_text); - } + output.push_str(&redactor.do_redact(&stdout_text)); write!(output, "---------------------------------------------\n").unwrap(); write!(output, "stderr:\n").unwrap(); - - if let Some(extra_redactions) = extra_redactions { - output.push_str(&redact_extra(&stderr_text, extra_redactions)); - } else { - output.push_str(&stderr_text); - } + output.push_str(&redactor.do_redact(&stderr_text)); write!(output, "=============================================\n").unwrap(); } diff --git a/dev-tools/reconfigurator-cli/tests/test_basic.rs b/dev-tools/reconfigurator-cli/tests/test_basic.rs index 2f45158d30..c0f52e72de 100644 --- a/dev-tools/reconfigurator-cli/tests/test_basic.rs +++ b/dev-tools/reconfigurator-cli/tests/test_basic.rs @@ -19,8 +19,8 @@ use omicron_test_utils::dev::poll::wait_for_condition; use omicron_test_utils::dev::poll::CondCheckError; use omicron_test_utils::dev::test_cmds::assert_exit_code; use omicron_test_utils::dev::test_cmds::path_to_executable; -use omicron_test_utils::dev::test_cmds::redact_variable; use omicron_test_utils::dev::test_cmds::run_command; +use omicron_test_utils::dev::test_cmds::Redactor; use omicron_test_utils::dev::test_cmds::EXIT_SUCCESS; use omicron_uuid_kinds::SledUuid; use slog::debug; @@ -43,7 +43,7 @@ fn test_basic() { let exec = Exec::cmd(path_to_cli()).arg("tests/input/cmds.txt"); let (exit_status, stdout_text, stderr_text) = run_command(exec); assert_exit_code(exit_status, EXIT_SUCCESS, &stderr_text); - let stdout_text = redact_variable(&stdout_text); + let stdout_text = Redactor::default().do_redact(&stdout_text); assert_contents("tests/output/cmd-stdout", &stdout_text); assert_contents("tests/output/cmd-stderr", &stderr_text); } diff --git a/test-utils/src/dev/test_cmds.rs b/test-utils/src/dev/test_cmds.rs index 94c558d82e..220efa4628 100644 --- a/test-utils/src/dev/test_cmds.rs +++ b/test-utils/src/dev/test_cmds.rs @@ -125,7 +125,83 @@ pub fn error_for_enoent() -> String { /// invocation to invocation (e.g., assigned TCP port numbers, timestamps) /// /// This allows use to use expectorate to verify the shape of the CLI output. -pub fn redact_variable(input: &str) -> String { +#[derive(Clone, Debug)] +pub struct Redactor<'a> { + basic: bool, + uuids: bool, + extra: Vec<(&'a str, String)>, +} + +impl Default for Redactor<'_> { + fn default() -> Self { + Self { basic: true, uuids: true, extra: Vec::new() } + } +} + +impl<'a> Redactor<'a> { + /// Create a new redactor that does not do any redactions. + pub fn noop() -> Self { + Self { basic: false, uuids: false, extra: Vec::new() } + } + + pub fn basic(&mut self, basic: bool) -> &mut Self { + self.basic = basic; + self + } + + pub fn uuids(&mut self, uuids: bool) -> &mut Self { + self.uuids = uuids; + self + } + + pub fn extra_fixed_length( + &mut self, + name: &str, + text_to_redact: &'a str, + ) -> &mut Self { + // Use the same number of chars as the number of bytes in + // text_to_redact. We're almost entirely in ASCII-land so they're the + // same, and getting the length right is nice but doesn't matter for + // correctness. + // + // A technically more correct impl would use unicode-width, but ehhh. + let replacement = fill_redaction_text(name, text_to_redact.len()); + self.extra.push((text_to_redact, replacement)); + self + } + + pub fn extra_variable_length( + &mut self, + name: &str, + text_to_redact: &'a str, + ) -> &mut Self { + let replacement = format!("<{}_REDACTED>", name.to_uppercase()); + self.extra.push((text_to_redact, replacement)); + self + } + + pub fn do_redact(&self, input: &str) -> String { + // Perform extra redactions at the beginning, not the end. This is because + // some of the built-in redactions in redact_variable might match a + // substring of something that should be handled by extra_redactions (e.g. + // a temporary path). + let mut s = input.to_owned(); + for (name, replacement) in &self.extra { + s = s.replace(name, replacement); + } + + if self.basic { + s = redact_basic(&s); + } + if self.uuids { + s = redact_uuids(&s); + } + + s + } +} + +fn redact_basic(input: &str) -> String { // Replace TCP port numbers. We include the localhost // characters to avoid catching any random sequence of numbers. let s = regex::Regex::new(r"\[::1\]:\d{4,5}") @@ -141,19 +217,6 @@ pub fn redact_variable(input: &str) -> String { .replace_all(&s, "127.0.0.1:REDACTED_PORT") .to_string(); - // Replace uuids. - // - // The length of a UUID is 32 nibbles for the hex encoding of a u128 + 4 - // dashes = 36. - const UUID_LEN: usize = 36; - let s = regex::Regex::new( - "[a-zA-Z0-9]{8}-[a-zA-Z0-9]{4}-[a-zA-Z0-9]{4}-\ - [a-zA-Z0-9]{4}-[a-zA-Z0-9]{12}", - ) - .unwrap() - .replace_all(&s, fill_redaction_text("uuid", UUID_LEN)) - .to_string(); - // Replace timestamps. // // Format: RFC 3339 (ISO 8601) @@ -213,63 +276,14 @@ pub fn redact_variable(input: &str) -> String { s } -/// Redact text from a string, allowing for extra redactions to be specified. -pub fn redact_extra( - input: &str, - extra_redactions: &ExtraRedactions<'_>, -) -> String { - // Perform extra redactions at the beginning, not the end. This is because - // some of the built-in redactions in redact_variable might match a - // substring of something that should be handled by extra_redactions (e.g. - // a temporary path). - let mut s = input.to_owned(); - for (name, replacement) in &extra_redactions.redactions { - s = s.replace(name, replacement); - } - redact_variable(&s) -} - -/// Represents a list of extra redactions for [`redact_variable`]. -/// -/// Extra redactions are applied in-order, before any builtin redactions. -#[derive(Clone, Debug, Default)] -pub struct ExtraRedactions<'a> { - // A pair of redaction and replacement strings. - redactions: Vec<(&'a str, String)>, -} - -impl<'a> ExtraRedactions<'a> { - pub fn new() -> Self { - Self { redactions: Vec::new() } - } - - pub fn fixed_length( - &mut self, - name: &str, - text_to_redact: &'a str, - ) -> &mut Self { - // Use the same number of chars as the number of bytes in - // text_to_redact. We're almost entirely in ASCII-land so they're the - // same, and getting the length right is nice but doesn't matter for - // correctness. - // - // A technically more correct impl would use unicode-width, but ehhh. - let replacement = fill_redaction_text(name, text_to_redact.len()); - self.redactions.push((text_to_redact, replacement)); - self - } - - pub fn variable_length( - &mut self, - name: &str, - text_to_redact: &'a str, - ) -> &mut Self { - let gen = format!("<{}_REDACTED>", name.to_uppercase()); - let replacement = gen.to_string(); - - self.redactions.push((text_to_redact, replacement)); - self - } +fn redact_uuids(input: &str) -> String { + // The length of a UUID is 32 nibbles for the hex encoding of a u128 + 4 + // dashes = 36. + const UUID_LEN: usize = 36; + regex::Regex::new(r"[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}") + .unwrap() + .replace_all(&input, fill_redaction_text("uuid", UUID_LEN)) + .to_string() } fn fill_redaction_text(name: &str, text_to_redact_len: usize) -> String { @@ -309,13 +323,11 @@ mod tests { let input = "time: 123ms, path: /var/tmp/tmp.456ms123s, \ path2: /short, \ path3: /variable-length/path"; - let actual = redact_extra( - input, - ExtraRedactions::new() - .fixed_length("tp", "/var/tmp/tmp.456ms123s") - .fixed_length("short_redact", "/short") - .variable_length("variable", "/variable-length/path"), - ); + let actual = Redactor::default() + .extra_fixed_length("tp", "/var/tmp/tmp.456ms123s") + .extra_fixed_length("short_redact", "/short") + .extra_variable_length("variable", "/variable-length/path") + .do_redact(input); assert_eq!( actual, "time: ms, path: ........., \ @@ -347,7 +359,7 @@ mod tests { for time in times { let input = format!("{:?}", time); assert_eq!( - redact_variable(&input), + Redactor::default().do_redact(&input), "", "Failed to redact {:?}", time