Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5/n] [test-utils] turn redactor into the builder pattern #6786

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 16 additions & 39 deletions dev-tools/omdb/tests/test_all_output.rs
Original file line number Diff line number Diff line change
@@ -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<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>(
@@ -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();

@@ -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();
}
4 changes: 2 additions & 2 deletions dev-tools/reconfigurator-cli/tests/test_basic.rs
Original file line number Diff line number Diff line change
@@ -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);
}
170 changes: 91 additions & 79 deletions test-utils/src/dev/test_cmds.rs
Original file line number Diff line number Diff line change
@@ -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.
Comment on lines +183 to +186
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bikeshed 2/2]:

Suggested change
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.
pub fn 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had redact at first, but especially now that the config knobs are shorter (e.g. uuids, basic) I wanted to draw a distinction between the knobs and the actual process of redaction.

// 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: <REDACTED DURATION>ms, path: ....<REDACTED_TP>....., \
@@ -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