Skip to content

Commit

Permalink
[5/n] [test-utils] turn redactor into the builder pattern (#6786)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sunshowers
sunshowers authored Oct 9, 2024
1 parent 93d6974 commit beaba46
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 120 deletions.
55 changes: 16 additions & 39 deletions dev-tools/omdb/tests/test_all_output.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -234,7 +235,7 @@ async fn test_omdb_success_cases(cptestctx: &ControlPlaneTestContext) {
},
&cmd_path,
args,
Some(&redactions),
&redactor,
)
.await;
}
Expand Down Expand Up @@ -444,14 +445,7 @@ async fn do_run<F>(
) 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<F>(
Expand All @@ -462,30 +456,23 @@ async fn do_run_no_redactions<F>(
) 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<F>(
output: &mut String,
modexec: F,
cmd_path: &Path,
args: &[&str],
extra_redactions: Option<&ExtraRedactions<'_>>,
redactor: &Redactor<'_>,
) where
F: FnOnce(Exec) -> Exec + Send + 'static,
{
write!(
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::<Vec<_>>()
args.iter().map(|r| redactor.do_redact(r)).collect::<Vec<_>>()
)
.unwrap();

Expand Down Expand Up @@ -521,21 +508,11 @@ async fn do_run_extra<F>(
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();
}
Expand Down
4 changes: 2 additions & 2 deletions dev-tools/reconfigurator-cli/tests/test_basic.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}
Expand Down
170 changes: 91 additions & 79 deletions test-utils/src/dev/test_cmds.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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: <REDACTED DURATION>ms, path: ....<REDACTED_TP>....., \
Expand Down Expand Up @@ -347,7 +359,7 @@ mod tests {
for time in times {
let input = format!("{:?}", time);
assert_eq!(
redact_variable(&input),
Redactor::default().do_redact(&input),
"<REDACTED_TIMESTAMP>",
"Failed to redact {:?}",
time
Expand Down

0 comments on commit beaba46

Please sign in to comment.