From a34274e65363e53470495309c5a4dd1789e4cf9b Mon Sep 17 00:00:00 2001 From: Daniel Nash <38335330+dannash100@users.noreply.github.com> Date: Thu, 22 Oct 2026 10:49:15 +1300 Subject: [PATCH 1/5] share migrate defaults --- README.md | 4 +-- src/controllers/canopy/intent.rs | 48 +++++++++++++++++++++----------- tests/migration.rs | 4 +-- 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 4dca6a7f..aced68c1 100644 --- a/README.md +++ b/README.md @@ -111,8 +111,8 @@ Defines a continuously-refreshed replica of a PostgreSQL database restored from | `postgresExtraConfig` | `string` | No | — | Extra lines appended to `postgresql.conf` (e.g. `shared_preload_libraries`). | | `notifications` | `[]NotificationConfig` | No | `[]` | Notification targets called on restore events. | | `persistentSchemas` | `[]string` | No | — | List of schema names to migrate from the previous restore to the new restore on each switchover. See [Persistent schemas](#persistent-schemas) below for the migration time budget and what happens on timeout. | -| `preMigrateDropSchemas` | `[]string` | No | — | Schemas dropped from the restore before its migration Job runs. Views over a table a migration alters block the DDL, so a deployment that drops and regenerates such schemas around a real upgrade names them here and the test starts from the same place. `public`, `information_schema` and `pg_*` are refused. Set from the intent's `pre_migrate_drop_schemas` param, which the `upgrade` intent defaults to `reporting`. | -| `preMigrateSql` | `string` | No | — | SQL run against the restore after the schema drops and before its migration Job, for starting-state changes a drop can't express. Set from the intent's `pre_migrate_sql` param, which the `upgrade` intent defaults to calling Tamanu's `forget_server_identity()` where the snapshot has it and soft-deleting the `S1:` encrypted values in `local_system_secrets` and `local_system_facts`, so the copy forgets the source server and the upgrade's config key check passes without the source deployment's key. | +| `preMigrateDropSchemas` | `[]string` | No | — | Schemas dropped from the restore before its migration Job runs. Views over a table a migration alters block the DDL, so a deployment that drops and regenerates such schemas around a real upgrade names them here and the test starts from the same place. `public`, `information_schema` and `pg_*` are refused. Set from the intent's `pre_migrate_drop_schemas` param, which the `upgrade` and `reporting-schema` intents default to `reporting`. | +| `preMigrateSql` | `string` | No | — | SQL run against the restore after the schema drops and before its migration Job, for starting-state changes a drop can't express. Set from the intent's `pre_migrate_sql` param, which the `upgrade` and `reporting-schema` intents default to calling Tamanu's `forget_server_identity()` where the snapshot has it and soft-deleting the `S1:` encrypted values in `local_system_secrets` and `local_system_facts`, so the copy forgets the source server and the upgrade's config key check passes without the source deployment's key. | | `redaction` | `RedactionSpec` | No | — | If set, apply a Tamanu/dbt-shaped masking manifest to the restored data via the `postgresql_anonymizer` extension before switchover. See [RedactionSpec](#redactionspec) below. | | `migrateTo` | `MigrationTarget` | No | — | Apply a Tamanu version's schema migrations to each restore of this replica. `{ version, versionId }`. Set by the canopy worklist syncer by two routes: for the `upgrade` intent from the target version canopy names on the worklist entry, whose outcome is reported back as a version-readiness signal; and for an `analytics` replica from its operator-chosen `migrate_to` param, keeping a persistent upgraded query replica, whose outcome is not reported. A restore carrying this is built read-write, since migrations are DDL. | | `builderImage` | `string` | No | — | Image that builds a Tamanu reporting schema against each migrated restore. Set by the canopy worklist syncer from the `reporting-schema` intent's `builder_image` param. pgro hands the image a database, a version and a group and takes back the SQL it emits over a callback; how a schema is made is the image's business. Its presence is what sends the restore through a build before switchover. | diff --git a/src/controllers/canopy/intent.rs b/src/controllers/canopy/intent.rs index 85728097..8b9f2930 100644 --- a/src/controllers/canopy/intent.rs +++ b/src/controllers/canopy/intent.rs @@ -146,9 +146,9 @@ const DEFAULT_ANALYTICS_MINIMUM_TTL_SECS: i64 = 7200; const DEFAULT_ANALYTICS_SWITCHOVER_GRACE_SECS: i64 = 120; /// Every tamanu deployment carries the reporting schema, and a real upgrade /// drops and regenerates it because its views over altered tables block the -/// DDL. An `upgrade` replica that left this unset would fail on something the +/// DDL. A migrating replica that left this unset would fail on something the /// deployment it models never meets. -const DEFAULT_UPGRADE_DROP_SCHEMAS: &str = "reporting"; +const DEFAULT_PRE_MIGRATE_DROP_SCHEMAS: &str = "reporting"; /// Tamanu's upgrade refuses to start when a live `S1:` encrypted value in these /// tables doesn't decrypt with the configured key, and the migration Job only @@ -156,7 +156,7 @@ const DEFAULT_UPGRADE_DROP_SCHEMAS: &str = "reporting"; /// `forget_server_identity()`, where the snapshot has it, also clears the /// plaintext facts that would point the copy at the source's central; it runs /// only once the transaction declares the database a copy. -pub const DEFAULT_UPGRADE_PRE_MIGRATE_SQL: &str = r#"DO $$ +pub const DEFAULT_PRE_MIGRATE_SQL: &str = r#"DO $$ DECLARE t text; BEGIN IF to_regprocedure('public.forget_server_identity()') IS NOT NULL THEN @@ -190,15 +190,15 @@ fn upgrade_param_schema() -> ParamSchema { ParamSchema(HashMap::from([ ( params::PRE_MIGRATE_DROP_SCHEMAS.to_string(), - param(ParamType::Text, Some(json!(DEFAULT_UPGRADE_DROP_SCHEMAS))), - ), - ( - params::PRE_MIGRATE_SQL.to_string(), param( ParamType::Text, - Some(json!(DEFAULT_UPGRADE_PRE_MIGRATE_SQL)), + Some(json!(DEFAULT_PRE_MIGRATE_DROP_SCHEMAS)), ), ), + ( + params::PRE_MIGRATE_SQL.to_string(), + param(ParamType::Text, Some(json!(DEFAULT_PRE_MIGRATE_SQL))), + ), ])) } @@ -213,11 +213,14 @@ fn reporting_schema_param_schema() -> ParamSchema { ), ( params::PRE_MIGRATE_DROP_SCHEMAS.to_string(), - param(ParamType::Text, None), + param( + ParamType::Text, + Some(json!(DEFAULT_PRE_MIGRATE_DROP_SCHEMAS)), + ), ), ( params::PRE_MIGRATE_SQL.to_string(), - param(ParamType::Text, None), + param(ParamType::Text, Some(json!(DEFAULT_PRE_MIGRATE_SQL))), ), ])) } @@ -839,7 +842,7 @@ mod tests { #[test] fn pre_migrate_drop_schemas_parses_on_every_migrating_intent() { - for intent in ["analytics", "upgrade"] { + for intent in ["analytics", "upgrade", "reporting-schema"] { let spec = config_for(intent).unwrap().to_replica_spec( &entry( intent, @@ -889,20 +892,20 @@ mod tests { #[test] fn upgrade_default_forgets_identity_and_hides_secrets() { assert!( - DEFAULT_UPGRADE_PRE_MIGRATE_SQL.contains("PERFORM public.forget_server_identity();"), + DEFAULT_PRE_MIGRATE_SQL.contains("PERFORM public.forget_server_identity();"), "the default calls Tamanu's function when the snapshot has it" ); assert!( - DEFAULT_UPGRADE_PRE_MIGRATE_SQL.contains( + DEFAULT_PRE_MIGRATE_SQL.contains( "PERFORM set_config('tamanu.this_is_a_copy', 'true', true);\n PERFORM public.forget_server_identity();" ), "the function refuses unless the same transaction declares a copy" ); assert!( - DEFAULT_UPGRADE_PRE_MIGRATE_SQL.contains("SET deleted_at = now()"), + DEFAULT_PRE_MIGRATE_SQL.contains("SET deleted_at = now()"), "the encrypted values the job's key can't read are always hidden" ); - assert!(!DEFAULT_UPGRADE_PRE_MIGRATE_SQL.contains("RETURN;")); + assert!(!DEFAULT_PRE_MIGRATE_SQL.contains("RETURN;")); } #[test] @@ -1016,7 +1019,7 @@ mod tests { // read, so the default hides them. assert_eq!( upgrade_params.get(params::PRE_MIGRATE_SQL).unwrap().default, - Some(json!(DEFAULT_UPGRADE_PRE_MIGRATE_SQL)) + Some(json!(DEFAULT_PRE_MIGRATE_SQL)) ); let drop_schemas = upgrade_params .get(params::PRE_MIGRATE_DROP_SCHEMAS) @@ -1048,6 +1051,19 @@ mod tests { build_params.get(params::BUILDER_IMAGE).unwrap().type_, ParamType::Text ); + // A build restores a real site, carrying its reporting views and its + // encrypted secrets, and migrates it just as an upgrade test does. + assert_eq!( + build_params.get(params::PRE_MIGRATE_SQL).unwrap().default, + upgrade_params.get(params::PRE_MIGRATE_SQL).unwrap().default, + ); + assert_eq!( + build_params + .get(params::PRE_MIGRATE_DROP_SCHEMAS) + .unwrap() + .default, + Some(json!("reporting")) + ); assert!(build.description.is_some()); let analytics = &ds[2]; diff --git a/tests/migration.rs b/tests/migration.rs index abdb62e4..e0a04ed5 100644 --- a/tests/migration.rs +++ b/tests/migration.rs @@ -1,7 +1,7 @@ use k8s_openapi::api::{batch::v1::Job, core::v1::Secret}; use kube::{Api, api::PostParams}; use postgres_restore_operator::{ - controllers::canopy::intent::DEFAULT_UPGRADE_PRE_MIGRATE_SQL, + controllers::canopy::intent::DEFAULT_PRE_MIGRATE_SQL, types::{MigrationTarget, PostgresPhysicalReplica, PostgresPhysicalRestore, RestorePhase}, }; use tokio::time::{sleep, timeout}; @@ -52,7 +52,7 @@ async fn migration_target_drives_a_migration_job() { // which is the shape that blocks a migration's DDL on a real deployment. replica.spec.pre_migrate_drop_schemas = Some(vec!["reporting".to_string(), "public".to_string()]); - replica.spec.pre_migrate_sql = Some(DEFAULT_UPGRADE_PRE_MIGRATE_SQL.to_string()); + replica.spec.pre_migrate_sql = Some(DEFAULT_PRE_MIGRATE_SQL.to_string()); replica.metadata.namespace = Some(ns.into()); replicas .create(&PostParams::default(), &replica) From ef1f11532e2223a384271fbc696af1014a493209 Mon Sep 17 00:00:00 2001 From: Daniel Nash <38335330+dannash100@users.noreply.github.com> Date: Thu, 22 Oct 2026 10:49:15 +1300 Subject: [PATCH 2/5] report build log --- Cargo.lock | 19 +++ Cargo.toml | 1 + src/controllers/jobs.rs | 191 +++++++++++++++++++++++- src/controllers/replica/schema_build.rs | 126 +++++++++++++++- src/controllers/restore/migration.rs | 190 ++--------------------- 5 files changed, 345 insertions(+), 182 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 151fe35b..18a98e4d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3388,6 +3388,7 @@ dependencies = [ "serde_yaml", "sha2 0.11.0", "socket2", + "strip-ansi-escapes", "tempfile", "thiserror 2.0.20", "tokio", @@ -4427,6 +4428,15 @@ dependencies = [ "unicode-properties", ] +[[package]] +name = "strip-ansi-escapes" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2a8f8038e7e7969abb3f1b7c2a811225e9296da208539e0f79c5251d6cac0025" +dependencies = [ + "vte", +] + [[package]] name = "strsim" version = "0.11.1" @@ -5187,6 +5197,15 @@ dependencies = [ "libc", ] +[[package]] +name = "vte" +version = "0.14.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "231fdcd7ef3037e8330d8e17e61011a2c244126acc0a982f4040ac3f9f0bc077" +dependencies = [ + "memchr", +] + [[package]] name = "wait-timeout" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index 7ae82e98..1a82530c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,7 @@ serde_json = "1.0.150" serde_yaml = "0.9.34" sha2 = "0.11.0" socket2 = "0.6.2" +strip-ansi-escapes = "0.2.1" thiserror = "2.0.18" tokio = { version = "1.50.0", features = ["full"] } tokio-postgres = { version = "0.7.18", features = ["with-serde_json-1"] } diff --git a/src/controllers/jobs.rs b/src/controllers/jobs.rs index 8ee42dae..5c02f72a 100644 --- a/src/controllers/jobs.rs +++ b/src/controllers/jobs.rs @@ -6,8 +6,13 @@ use std::{ use k8s_openapi::api::{ batch::v1::Job, - core::v1::{EnvVar, EnvVarSource, SecretKeySelector}, + core::v1::{EnvVar, EnvVarSource, Pod, SecretKeySelector}, }; +use kube::{ + Api, Client, ResourceExt, + api::{ListParams, LogParams}, +}; +use tracing::warn; /// Status of a Kubernetes Job. #[derive(Debug)] @@ -107,6 +112,101 @@ pub fn env_literal(name: &str, value: &str) -> EnvVar { } } +/// The tail of a Job container's log, as its pod still holds it. +/// +/// Best effort: a log that cannot be read leaves the caller without a cause and +/// the reconcile carries on. +pub async fn container_log( + client: &Client, + namespace: &str, + job_name: &str, + container: &str, +) -> Option { + let pods: Api = Api::namespaced(client.clone(), namespace); + let list = pods + .list(&ListParams::default().labels(&format!("job-name={job_name}"))) + .await + .map_err(|e| warn!(job = job_name, error = %e, "listing job pods failed")) + .ok()?; + let pod = list.items.first()?; + + pods.logs( + &pod.name_any(), + &LogParams { + container: Some(container.to_string()), + tail_lines: Some(200), + ..Default::default() + }, + ) + .await + .map_err(|e| warn!(job = job_name, error = %e, "reading job log failed")) + .ok() +} + +/// The tail of a failed Job's log, bounded so a status field stays readable. +/// +/// A thrown error prints its message first and the failing SQL after it, so a +/// long statement pushes the message out of the tail. The first line naming an +/// error is kept ahead of the tail when the tail doesn't already hold it. +pub fn log_tail(log: &str) -> String { + const KEEP_LINES: usize = 40; + const MAX_BYTES: usize = 2000; + const MAX_HEADLINE_BYTES: usize = 500; + + let lines: Vec<&str> = log.lines().collect(); + let tail_start = lines.len().saturating_sub(KEEP_LINES); + let tail = lines[tail_start..].join("\n"); + + let headline = lines + .iter() + .position(|line| is_error_headline(line)) + .filter(|&at| at < tail_start || tail.len() > MAX_BYTES) + .map(|at| truncate_to(lines[at].trim(), MAX_HEADLINE_BYTES)); + + let budget = match &headline { + Some(headline) => MAX_BYTES.saturating_sub(headline.len() + "\n…\n".len()), + None => MAX_BYTES, + }; + let kept = if tail.len() <= budget { + tail.as_str() + } else { + let cut = (tail.len() - budget..tail.len()) + .find(|at| tail.is_char_boundary(*at)) + .unwrap_or(tail.len()); + &tail[cut..] + }; + + match headline { + Some(headline) if !kept.contains(headline.as_str()) => format!("{headline}\n…\n{kept}"), + _ => kept.to_string(), + } +} + +fn is_error_headline(line: &str) -> bool { + let line = line.trim_start(); + [ + "Error:", + "ERROR:", + "error:", + "Error [", + "DatabaseError", + "SequelizeDatabaseError", + ] + .iter() + .any(|marker| line.starts_with(marker) || line.contains(&format!(" {marker}"))) +} + +fn truncate_to(text: &str, max_bytes: usize) -> String { + if text.len() <= max_bytes { + return text.to_string(); + } + let end = (0..=max_bytes) + .rev() + .find(|at| text.is_char_boundary(*at)) + .unwrap_or(0); + text[..end].to_string() +} + #[cfg(test)] mod tests { use k8s_openapi::api::batch::v1::{JobSpec, JobStatus as K8sJobStatus}; @@ -232,3 +332,92 @@ mod tests { assert!(env.value_from.is_none()); } } + +#[cfg(test)] +mod log_tail_tests { + use super::log_tail; + + #[test] + fn the_log_is_kept_whole() { + let log = concat!( + "running 1721000002-addBaz.ts\n", + "error: insert or update on table \"encounters\" violates foreign key constraint \"fk_patient\"\n", + "DETAIL: Key (patient_id)=(abc-123) is not present in table \"patients\".\n", + "HINT: add the patient first\n", + ); + + let out = log_tail(log); + + assert!(out.contains("violates foreign key constraint"), "{out}"); + assert!(out.contains("Key (patient_id)=(abc-123)"), "{out}"); + assert!(out.contains("HINT: add the patient first"), "{out}"); + } + + #[test] + fn only_the_last_lines_are_kept() { + let log = (0..100) + .map(|i| format!("applying migration {i}")) + .collect::>() + .join("\n"); + + let out = log_tail(&log); + + assert!(out.starts_with("applying migration 60"), "{out}"); + assert!(out.ends_with("applying migration 99"), "{out}"); + assert!(!out.contains("applying migration 59"), "{out}"); + } + + #[test] + fn the_tail_is_capped() { + let log = (0..60) + .map(|i| format!("applying migration {i} {}", "x".repeat(200))) + .collect::>() + .join("\n"); + + let out = log_tail(&log); + + assert!(out.len() <= 2000, "{} bytes", out.len()); + assert!(out.ends_with("xxx"), "the end of the log is what is kept"); + } + + #[test] + fn the_error_survives_a_long_statement_after_it() { + let mut log = String::from( + "info: Applying migration: 1781222400001-makeIdDeterministic\n\ + error: DatabaseError [SequelizeDatabaseError]: index \"idx_example\" does not exist\n\ + \x20 at Query.run (query.js:50:25)\n", + ); + for i in 0..60 { + log.push_str(&format!( + " sql: 'INSERT INTO logs.changes line {i} {}',\n", + "x".repeat(80) + )); + } + + let out = log_tail(&log); + + assert!(out.len() <= 2000, "{} bytes", out.len()); + assert!( + out.starts_with( + "error: DatabaseError [SequelizeDatabaseError]: index \"idx_example\" does not exist" + ), + "{out}" + ); + assert!( + out.contains("line 59"), + "the end of the log is still kept: {out}" + ); + } + + #[test] + fn a_headline_already_in_the_tail_is_not_repeated() { + let log = "running 1-foo\nError: boom\nat x\n"; + + assert_eq!(log_tail(log), log.trim_end()); + } + + #[test] + fn an_empty_log_is_nothing() { + assert_eq!(log_tail(""), ""); + } +} diff --git a/src/controllers/replica/schema_build.rs b/src/controllers/replica/schema_build.rs index dc2838a5..627376ca 100644 --- a/src/controllers/replica/schema_build.rs +++ b/src/controllers/replica/schema_build.rs @@ -51,6 +51,8 @@ const PULL_SECRET_NAME: &str = "schema-build-pull"; const BUILD_DEADLINE_SECONDS: i64 = 30 * 60; +const BUILD_CONTAINER: &str = "build"; + /// How long a finished build Job is left for an operator to read, and for the /// reconcile that records it to read its receipt. An operator that was down /// while it expired reads no receipt and builds again. @@ -267,7 +269,7 @@ fn build_schema_build_job( }] }), containers: vec![Container { - name: "build".to_string(), + name: BUILD_CONTAINER.to_string(), image: Some(image.to_string()), security_context: Some(SecurityContext { allow_privilege_escalation: Some(false), @@ -603,6 +605,10 @@ pub(super) async fn reconcile_schema_build( Ok(false) } BuildOutcome::Failed => { + // Read before the Job is deleted, which takes its pod and the only + // record of why the build failed. + let log = jobs::container_log(client, namespace, &job_name, BUILD_CONTAINER).await; + let secrets = build_secrets(client, namespace, replica).await; record_schema_build( client, namespace, @@ -610,7 +616,7 @@ pub(super) async fn reconcile_schema_build( Some(&job_name), SchemaBuildResult { built: false, - error: Some("the build job failed".to_string()), + error: Some(build_failure(log.as_deref(), &secrets)), total_elapsed_seconds: build.elapsed_seconds, schema_bytes: build.delivered_bytes, artifacts: Vec::new(), @@ -626,6 +632,54 @@ pub(super) async fn reconcile_schema_build( } } +/// The values the build pod is handed that its log must not carry into the +/// result: the database password and the callback token. Best effort, as the +/// log itself is. +async fn build_secrets( + client: &Client, + namespace: &str, + replica: &PostgresPhysicalReplica, +) -> Vec { + let mut secrets = Vec::new(); + + let creds: Api = Api::namespaced(client.clone(), namespace); + match creds.get(&replica.creds_secret_name()).await { + Ok(secret) => secrets.extend(postgres::read_secret_field(&secret, "password").ok()), + Err(err) => { + warn!(replica = %replica.name_any(), "reading the build credentials failed: {err}") + } + } + + match build_token(client, namespace, &replica.name_any()).await { + Ok(token) => secrets.extend(token), + Err(err) => warn!(replica = %replica.name_any(), "reading the build token failed: {err}"), + } + + secrets +} + +const BUILD_FAILED: &str = "the build job failed"; + +/// What a failed build records: the tail of its log, with colour codes +/// stripped and every given secret masked, so canopy shows why it failed. +fn build_failure(log: Option<&str>, secrets: &[String]) -> String { + let Some(log) = log else { + return BUILD_FAILED.to_string(); + }; + + let mut log = String::from_utf8_lossy(&strip_ansi_escapes::strip(log)).into_owned(); + for secret in secrets.iter().filter(|secret| !secret.is_empty()) { + log = log.replace(secret.as_str(), "[redacted]"); + } + + let tail = jobs::log_tail(&log); + if tail.is_empty() { + BUILD_FAILED.to_string() + } else { + format!("{BUILD_FAILED}:\n{tail}") + } +} + /// Put a taken schema back for the next reconcile. fn keep_schema(ctx: &Arc, namespace: &str, replica_name: &str, sql: &Bytes) { ctx.schema_build_results.store( @@ -1537,4 +1591,72 @@ mod tests { ); assert_eq!(result.schema_bytes, Some(36)); } + + #[test] + fn a_failed_build_carries_the_end_of_its_log() { + let log = concat!( + "\x1b[94mRunning: dbt run --profiles-dir config --target replica\x1b[0m\n", + "Database Error in model ds__encounters (models/datasets/ds__encounters.sql)\n", + " column \"triage_score\" does not exist\n", + "Done. PASS=277 WARN=0 ERROR=6 SKIP=0 TOTAL=283\n", + "\x1b[91mError: the build failed\x1b[0m\n", + ); + + let error = build_failure(Some(log), &[]); + + assert!( + error.starts_with("the build job failed:\nRunning: dbt run"), + "{error}" + ); + assert!(error.contains("column \"triage_score\" does not exist")); + assert!(error.ends_with("Error: the build failed"), "{error}"); + assert!( + !error.contains('\x1b'), + "colour codes are stripped: {error}" + ); + } + + #[test] + fn a_failed_build_masks_what_it_was_handed() { + let log = concat!( + "connecting to postgresql://analytics:hunter2-long@kamaka-restore:5432/tamanu\n", + "POST /api/v1/schema-build-results/pgro/kamaka/tok-123 answered 403\n", + ); + + let error = build_failure( + Some(log), + &[ + "hunter2-long".to_string(), + "tok-123".to_string(), + String::new(), + ], + ); + + assert!(!error.contains("hunter2-long"), "{error}"); + assert!(!error.contains("tok-123"), "{error}"); + assert!( + error.contains("analytics:[redacted]@kamaka-restore"), + "{error}" + ); + assert!(error.contains("kamaka/[redacted] answered 403"), "{error}"); + } + + #[test] + fn a_failed_build_log_is_bounded() { + let log = (0..500) + .map(|i| format!("dbt line {i} {}", "x".repeat(100))) + .collect::>() + .join("\n"); + + let error = build_failure(Some(&log), &[]); + + assert!(error.len() <= 2000 + "the build job failed:\n".len()); + assert!(error.ends_with("xxx"), "the end of the log is kept"); + } + + #[test] + fn a_failed_build_without_a_log_says_only_that() { + assert_eq!(build_failure(None, &[]), "the build job failed"); + assert_eq!(build_failure(Some("\n"), &[]), "the build job failed"); + } } diff --git a/src/controllers/restore/migration.rs b/src/controllers/restore/migration.rs index c282088b..9e527f5c 100644 --- a/src/controllers/restore/migration.rs +++ b/src/controllers/restore/migration.rs @@ -12,13 +12,13 @@ use std::{collections::BTreeMap, time::Duration}; use k8s_openapi::{ api::{ batch::v1::{Job, JobSpec}, - core::v1::{Container, Pod, PodSpec, PodTemplateSpec, ResourceRequirements, Secret}, + core::v1::{Container, PodSpec, PodTemplateSpec, ResourceRequirements, Secret}, }, apimachinery::pkg::api::resource::Quantity, }; use kube::{ Api, ResourceExt, - api::{ListParams, LogParams, ObjectMeta, PostParams}, + api::{ObjectMeta, PostParams}, runtime::controller::Action, }; use tracing::{info, warn}; @@ -462,7 +462,13 @@ async fn read_result( job_failed: bool, ) -> Result { let log = if job_failed { - migration_job_log(ctx, namespace, &migration_job_name(restore_name)).await + crate::controllers::jobs::container_log( + &ctx.client, + namespace, + &migration_job_name(restore_name), + MIGRATION_CONTAINER, + ) + .await } else { None }; @@ -598,32 +604,6 @@ pub(super) fn result_from_batch( } } -/// The tail of the migrate container's log, as the pod still holds it. -/// -/// Best effort: a log that cannot be read leaves the result without a cause and -/// the reconcile carries on. -async fn migration_job_log(ctx: &Context, namespace: &str, job_name: &str) -> Option { - let pods: Api = Api::namespaced(ctx.client.clone(), namespace); - let list = pods - .list(&ListParams::default().labels(&format!("job-name={job_name}"))) - .await - .map_err(|e| warn!(job = job_name, error = %e, "listing migration job pods failed")) - .ok()?; - let pod = list.items.first()?; - - pods.logs( - &pod.name_any(), - &LogParams { - container: Some(MIGRATION_CONTAINER.to_string()), - tail_lines: Some(200), - ..Default::default() - }, - ) - .await - .map_err(|e| warn!(job = job_name, error = %e, "reading migration job log failed")) - .ok() -} - /// The cause of a failed migration: tamanu's own structured error where the /// version records one, otherwise the tail of the job's log. fn migration_error(stats: Option<&serde_json::Value>, log: Option<&str>) -> Option { @@ -669,161 +649,13 @@ fn migration_error(stats: Option<&serde_json::Value>, log: Option<&str>) -> Opti } } - let tail = migration_log_tail(log?); + let tail = crate::controllers::jobs::log_tail(log?); (!tail.is_empty()).then_some(tail) } -/// The tail of a migration job's log, bounded so a status field stays readable. -/// -/// A thrown error prints its message first and the failing SQL after it, so a -/// long statement pushes the message out of the tail. The first line naming an -/// error is kept ahead of the tail when the tail doesn't already hold it. -fn migration_log_tail(log: &str) -> String { - const KEEP_LINES: usize = 40; - const MAX_BYTES: usize = 2000; - const MAX_HEADLINE_BYTES: usize = 500; - - let lines: Vec<&str> = log.lines().collect(); - let tail_start = lines.len().saturating_sub(KEEP_LINES); - let tail = lines[tail_start..].join("\n"); - - let headline = lines - .iter() - .position(|line| is_error_headline(line)) - .filter(|&at| at < tail_start || tail.len() > MAX_BYTES) - .map(|at| truncate_to(lines[at].trim(), MAX_HEADLINE_BYTES)); - - let budget = match &headline { - Some(headline) => MAX_BYTES.saturating_sub(headline.len() + "\n…\n".len()), - None => MAX_BYTES, - }; - let kept = if tail.len() <= budget { - tail.as_str() - } else { - let cut = (tail.len() - budget..tail.len()) - .find(|at| tail.is_char_boundary(*at)) - .unwrap_or(tail.len()); - &tail[cut..] - }; - - match headline { - Some(headline) if !kept.contains(headline.as_str()) => format!("{headline}\n…\n{kept}"), - _ => kept.to_string(), - } -} - -fn is_error_headline(line: &str) -> bool { - let line = line.trim_start(); - [ - "Error:", - "ERROR:", - "error:", - "Error [", - "DatabaseError", - "SequelizeDatabaseError", - ] - .iter() - .any(|marker| line.starts_with(marker) || line.contains(&format!(" {marker}"))) -} - -fn truncate_to(text: &str, max_bytes: usize) -> String { - if text.len() <= max_bytes { - return text.to_string(); - } - let end = (0..=max_bytes) - .rev() - .find(|at| text.is_char_boundary(*at)) - .unwrap_or(0); - text[..end].to_string() -} - #[cfg(test)] mod error_tests { - use super::{migration_error, migration_log_tail}; - - #[test] - fn the_log_is_kept_whole() { - let log = concat!( - "running 1721000002-addBaz.ts\n", - "error: insert or update on table \"encounters\" violates foreign key constraint \"fk_patient\"\n", - "DETAIL: Key (patient_id)=(abc-123) is not present in table \"patients\".\n", - "HINT: add the patient first\n", - ); - - let out = migration_log_tail(log); - - assert!(out.contains("violates foreign key constraint"), "{out}"); - assert!(out.contains("Key (patient_id)=(abc-123)"), "{out}"); - assert!(out.contains("HINT: add the patient first"), "{out}"); - } - - #[test] - fn only_the_last_lines_are_kept() { - let log = (0..100) - .map(|i| format!("applying migration {i}")) - .collect::>() - .join("\n"); - - let out = migration_log_tail(&log); - - assert!(out.starts_with("applying migration 60"), "{out}"); - assert!(out.ends_with("applying migration 99"), "{out}"); - assert!(!out.contains("applying migration 59"), "{out}"); - } - - #[test] - fn the_tail_is_capped() { - let log = (0..60) - .map(|i| format!("applying migration {i} {}", "x".repeat(200))) - .collect::>() - .join("\n"); - - let out = migration_log_tail(&log); - - assert!(out.len() <= 2000, "{} bytes", out.len()); - assert!(out.ends_with("xxx"), "the end of the log is what is kept"); - } - - #[test] - fn the_error_survives_a_long_statement_after_it() { - let mut log = String::from( - "info: Applying migration: 1781222400001-makeIdDeterministic\n\ - error: DatabaseError [SequelizeDatabaseError]: index \"idx_example\" does not exist\n\ - \x20 at Query.run (query.js:50:25)\n", - ); - for i in 0..60 { - log.push_str(&format!( - " sql: 'INSERT INTO logs.changes line {i} {}',\n", - "x".repeat(80) - )); - } - - let out = migration_log_tail(&log); - - assert!(out.len() <= 2000, "{} bytes", out.len()); - assert!( - out.starts_with( - "error: DatabaseError [SequelizeDatabaseError]: index \"idx_example\" does not exist" - ), - "{out}" - ); - assert!( - out.contains("line 59"), - "the end of the log is still kept: {out}" - ); - } - - #[test] - fn a_headline_already_in_the_tail_is_not_repeated() { - let log = "running 1-foo\nError: boom\nat x\n"; - - assert_eq!(migration_log_tail(log), log.trim_end()); - } - - #[test] - fn an_empty_log_is_nothing() { - assert_eq!(migration_log_tail(""), ""); - } + use super::migration_error; #[test] fn structured_stats_render_as_one_line() { From cdc17fa67881728e02a586fbaab31e022cc369b2 Mon Sep 17 00:00:00 2001 From: Daniel Nash <38335330+dannash100@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:06:24 +1300 Subject: [PATCH 3/5] mask encoded secrets --- Cargo.lock | 1 + Cargo.toml | 1 + src/controllers/jobs.rs | 21 +++++++++++++++++++++ src/controllers/replica/schema_build.rs | 20 +++++++++++++++++++- 4 files changed, 42 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 18a98e4d..9364feb7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3378,6 +3378,7 @@ dependencies = [ "kube", "kube_quantity", "mimalloc", + "percent-encoding", "prometheus", "rand 0.10.3", "reqwest", diff --git a/Cargo.toml b/Cargo.toml index 1a82530c..ed7045ce 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,7 @@ k8s-openapi = { version = "0.28.0", features = ["latest", "schemars"] } kube = { version = "4.0.0", default-features = false, features = ["runtime", "derive", "client", "rustls-tls", "ws"] } kube_quantity = "0.9.0" mimalloc = { version = "0.1.52", default-features = false } +percent-encoding = "2.3.2" prometheus = "0.14.0" rand = "0.10.1" reqwest = { version = "0.13.4", features = ["json", "socks"] } diff --git a/src/controllers/jobs.rs b/src/controllers/jobs.rs index 5c02f72a..e66e921e 100644 --- a/src/controllers/jobs.rs +++ b/src/controllers/jobs.rs @@ -191,6 +191,10 @@ fn is_error_headline(line: &str) -> bool { "Error [", "DatabaseError", "SequelizeDatabaseError", + "Database Error in", + "Compilation Error in", + "Runtime Error in", + "Failure in model", ] .iter() .any(|marker| line.starts_with(marker) || line.contains(&format!(" {marker}"))) @@ -409,6 +413,23 @@ mod log_tail_tests { ); } + #[test] + fn a_dbt_model_failure_survives_truncation() { + let mut log = String::from( + "Database Error in model ds__encounters (models/datasets/ds__encounters.sql)\n", + ); + for i in 0..200 { + log.push_str(&format!("dbt line {i} {}\n", "x".repeat(80))); + } + + let out = log_tail(&log); + + assert!( + out.starts_with("Database Error in model ds__encounters"), + "{out}" + ); + } + #[test] fn a_headline_already_in_the_tail_is_not_repeated() { let log = "running 1-foo\nError: boom\nat x\n"; diff --git a/src/controllers/replica/schema_build.rs b/src/controllers/replica/schema_build.rs index 627376ca..83584e2d 100644 --- a/src/controllers/replica/schema_build.rs +++ b/src/controllers/replica/schema_build.rs @@ -669,7 +669,12 @@ fn build_failure(log: Option<&str>, secrets: &[String]) -> String { let mut log = String::from_utf8_lossy(&strip_ansi_escapes::strip(log)).into_owned(); for secret in secrets.iter().filter(|secret| !secret.is_empty()) { - log = log.replace(secret.as_str(), "[redacted]"); + let encoded = + percent_encoding::utf8_percent_encode(secret, percent_encoding::NON_ALPHANUMERIC) + .to_string(); + log = log + .replace(secret.as_str(), "[redacted]") + .replace(&encoded, "[redacted]"); } let tail = jobs::log_tail(&log); @@ -1641,6 +1646,19 @@ mod tests { assert!(error.contains("kamaka/[redacted] answered 403"), "{error}"); } + #[test] + fn a_failed_build_masks_a_url_encoded_secret() { + let log = "connecting to postgresql://analytics:p%40ss%2Fw0rd@kamaka-restore:5432/tamanu\n"; + + let error = build_failure(Some(log), &["p@ss/w0rd".to_string()]); + + assert!(!error.contains("p%40ss"), "{error}"); + assert!( + error.contains("analytics:[redacted]@kamaka-restore"), + "{error}" + ); + } + #[test] fn a_failed_build_log_is_bounded() { let log = (0..500) From 37c5751106591437e1bf8b7994cee53546148d4e Mon Sep 17 00:00:00 2001 From: Daniel Nash <38335330+dannash100@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:25:50 +1300 Subject: [PATCH 4/5] mask both job logs --- src/controllers/jobs.rs | 37 ++++++++- src/controllers/replica/schema_build.rs | 99 +++++++++++++++---------- src/controllers/restore/migration.rs | 1 + 3 files changed, 96 insertions(+), 41 deletions(-) diff --git a/src/controllers/jobs.rs b/src/controllers/jobs.rs index e66e921e..a2f52022 100644 --- a/src/controllers/jobs.rs +++ b/src/controllers/jobs.rs @@ -124,7 +124,11 @@ pub async fn container_log( ) -> Option { let pods: Api = Api::namespaced(client.clone(), namespace); let list = pods - .list(&ListParams::default().labels(&format!("job-name={job_name}"))) + .list( + &ListParams::default() + .labels(&format!("job-name={job_name}")) + .limit(1), + ) .await .map_err(|e| warn!(job = job_name, error = %e, "listing job pods failed")) .ok()?; @@ -143,6 +147,37 @@ pub async fn container_log( .ok() } +/// The last part of a Job's log, colour codes stripped and every given secret +/// masked, raw and in the forms a URL carries it, before any of it is recorded. +pub fn redact(log: &str, secrets: &[String]) -> String { + let mut start = log.len().saturating_sub(REDACT_WINDOW); + while !log.is_char_boundary(start) { + start += 1; + } + let mut log = String::from_utf8_lossy(&strip_ansi_escapes::strip(&log[start..])).into_owned(); + for secret in secrets.iter().filter(|secret| !secret.is_empty()) { + log = log.replace(secret.as_str(), REDACTED); + for set in [percent_encoding::NON_ALPHANUMERIC, URL_COMPONENT] { + let encoded = percent_encoding::utf8_percent_encode(secret, set).to_string(); + log = log.replace(&encoded, REDACTED); + } + } + log +} + +const REDACTED: &str = "[redacted]"; + +/// Comfortably more than [`log_tail`] keeps, so a secret straddling its cut is +/// still masked. +const REDACT_WINDOW: usize = 64 * 1024; + +/// What URL encoders leave alone: RFC 3986's unreserved characters. +const URL_COMPONENT: &percent_encoding::AsciiSet = &percent_encoding::NON_ALPHANUMERIC + .remove(b'-') + .remove(b'.') + .remove(b'_') + .remove(b'~'); + /// The tail of a failed Job's log, bounded so a status field stays readable. /// /// A thrown error prints its message first and the failing SQL after it, so a diff --git a/src/controllers/replica/schema_build.rs b/src/controllers/replica/schema_build.rs index 83584e2d..420ef7f2 100644 --- a/src/controllers/replica/schema_build.rs +++ b/src/controllers/replica/schema_build.rs @@ -608,7 +608,10 @@ pub(super) async fn reconcile_schema_build( // Read before the Job is deleted, which takes its pod and the only // record of why the build failed. let log = jobs::container_log(client, namespace, &job_name, BUILD_CONTAINER).await; - let secrets = build_secrets(client, namespace, replica).await; + let secrets = match log { + Some(_) => build_secrets(client, namespace, replica).await, + None => Some(Vec::new()), + }; record_schema_build( client, namespace, @@ -616,7 +619,7 @@ pub(super) async fn reconcile_schema_build( Some(&job_name), SchemaBuildResult { built: false, - error: Some(build_failure(log.as_deref(), &secrets)), + error: Some(build_failure(log.as_deref(), secrets.as_deref())), total_elapsed_seconds: build.elapsed_seconds, schema_bytes: build.delivered_bytes, artifacts: Vec::new(), @@ -633,49 +636,46 @@ pub(super) async fn reconcile_schema_build( } /// The values the build pod is handed that its log must not carry into the -/// result: the database password and the callback token. Best effort, as the -/// log itself is. +/// result: the database password and the callback token. `None` when either +/// could not be read, so the log is withheld rather than recorded unmasked. async fn build_secrets( client: &Client, namespace: &str, replica: &PostgresPhysicalReplica, -) -> Vec { - let mut secrets = Vec::new(); - +) -> Option> { let creds: Api = Api::namespaced(client.clone(), namespace); - match creds.get(&replica.creds_secret_name()).await { - Ok(secret) => secrets.extend(postgres::read_secret_field(&secret, "password").ok()), - Err(err) => { - warn!(replica = %replica.name_any(), "reading the build credentials failed: {err}") - } - } + let password = creds + .get(&replica.creds_secret_name()) + .await + .map_err( + |err| warn!(replica = %replica.name_any(), "reading the build credentials failed: {err}"), + ) + .ok() + .and_then(|secret| postgres::read_secret_field(&secret, "password").ok())?; - match build_token(client, namespace, &replica.name_any()).await { - Ok(token) => secrets.extend(token), - Err(err) => warn!(replica = %replica.name_any(), "reading the build token failed: {err}"), - } + let token = build_token(client, namespace, &replica.name_any()) + .await + .map_err( + |err| warn!(replica = %replica.name_any(), "reading the build token failed: {err}"), + ) + .ok()?; - secrets + Some(std::iter::once(password).chain(token).collect()) } const BUILD_FAILED: &str = "the build job failed"; -/// What a failed build records: the tail of its log, with colour codes -/// stripped and every given secret masked, so canopy shows why it failed. -fn build_failure(log: Option<&str>, secrets: &[String]) -> String { +/// What a failed build records: the tail of its log, masked, so canopy shows +/// why it failed. With no secrets to mask by, the log is left out. +fn build_failure(log: Option<&str>, secrets: Option<&[String]>) -> String { let Some(log) = log else { return BUILD_FAILED.to_string(); }; + let Some(secrets) = secrets else { + return format!("{BUILD_FAILED} (log withheld: its secrets could not be read to mask it)"); + }; - let mut log = String::from_utf8_lossy(&strip_ansi_escapes::strip(log)).into_owned(); - for secret in secrets.iter().filter(|secret| !secret.is_empty()) { - let encoded = - percent_encoding::utf8_percent_encode(secret, percent_encoding::NON_ALPHANUMERIC) - .to_string(); - log = log - .replace(secret.as_str(), "[redacted]") - .replace(&encoded, "[redacted]"); - } + let log = jobs::redact(log, secrets); let tail = jobs::log_tail(&log); if tail.is_empty() { @@ -1607,7 +1607,7 @@ mod tests { "\x1b[91mError: the build failed\x1b[0m\n", ); - let error = build_failure(Some(log), &[]); + let error = build_failure(Some(log), Some(&[])); assert!( error.starts_with("the build job failed:\nRunning: dbt run"), @@ -1630,11 +1630,11 @@ mod tests { let error = build_failure( Some(log), - &[ + Some(&[ "hunter2-long".to_string(), "tok-123".to_string(), String::new(), - ], + ]), ); assert!(!error.contains("hunter2-long"), "{error}"); @@ -1648,17 +1648,36 @@ mod tests { #[test] fn a_failed_build_masks_a_url_encoded_secret() { - let log = "connecting to postgresql://analytics:p%40ss%2Fw0rd@kamaka-restore:5432/tamanu\n"; + let log = concat!( + "connecting to postgresql://analytics:p%40ss%2Fw0rd@kamaka-restore:5432/tamanu\n", + "connecting to postgresql://analytics:p-s.s%2Fw0rd@kamaka-restore:5432/tamanu\n", + ); - let error = build_failure(Some(log), &["p@ss/w0rd".to_string()]); + let error = build_failure( + Some(log), + Some(&["p@ss/w0rd".to_string(), "p-s.s/w0rd".to_string()]), + ); assert!(!error.contains("p%40ss"), "{error}"); - assert!( - error.contains("analytics:[redacted]@kamaka-restore"), + assert!(!error.contains("p-s.s"), "{error}"); + assert_eq!( + error.matches("analytics:[redacted]@kamaka-restore").count(), + 2, "{error}" ); } + #[test] + fn a_failed_build_log_is_withheld_without_its_secrets() { + let error = build_failure( + Some("connecting to postgresql://analytics:hunter2@x\n"), + None, + ); + + assert!(!error.contains("hunter2"), "{error}"); + assert!(error.starts_with("the build job failed"), "{error}"); + } + #[test] fn a_failed_build_log_is_bounded() { let log = (0..500) @@ -1666,7 +1685,7 @@ mod tests { .collect::>() .join("\n"); - let error = build_failure(Some(&log), &[]); + let error = build_failure(Some(&log), Some(&[])); assert!(error.len() <= 2000 + "the build job failed:\n".len()); assert!(error.ends_with("xxx"), "the end of the log is kept"); @@ -1674,7 +1693,7 @@ mod tests { #[test] fn a_failed_build_without_a_log_says_only_that() { - assert_eq!(build_failure(None, &[]), "the build job failed"); - assert_eq!(build_failure(Some("\n"), &[]), "the build job failed"); + assert_eq!(build_failure(None, Some(&[])), "the build job failed"); + assert_eq!(build_failure(Some("\n"), Some(&[])), "the build job failed"); } } diff --git a/src/controllers/restore/migration.rs b/src/controllers/restore/migration.rs index 9e527f5c..4c0fc052 100644 --- a/src/controllers/restore/migration.rs +++ b/src/controllers/restore/migration.rs @@ -469,6 +469,7 @@ async fn read_result( MIGRATION_CONTAINER, ) .await + .map(|log| crate::controllers::jobs::redact(&log, std::slice::from_ref(&creds.1))) } else { None }; From 53a3e70dfefd943e775274a53236c2b7cb900f1d Mon Sep 17 00:00:00 2001 From: Daniel Nash <38335330+dannash100@users.noreply.github.com> Date: Thu, 22 Oct 2026 11:50:32 +1300 Subject: [PATCH 5/5] test secret handling --- src/controllers/jobs.rs | 26 ++++++- src/controllers/replica/schema_build.rs | 92 +++++++++++++++++++++---- 2 files changed, 105 insertions(+), 13 deletions(-) diff --git a/src/controllers/jobs.rs b/src/controllers/jobs.rs index a2f52022..fdb8ef39 100644 --- a/src/controllers/jobs.rs +++ b/src/controllers/jobs.rs @@ -374,7 +374,7 @@ mod tests { #[cfg(test)] mod log_tail_tests { - use super::log_tail; + use super::{log_tail, redact}; #[test] fn the_log_is_kept_whole() { @@ -476,4 +476,28 @@ mod log_tail_tests { fn an_empty_log_is_nothing() { assert_eq!(log_tail(""), ""); } + + #[test] + fn a_migration_log_has_the_password_masked_and_the_error_kept() { + let log = concat!( + "connecting to postgresql://tamanu:p@ss.w0rd@kamaka-restore:5432/tamanu\n", + "retrying postgresql://tamanu:p%40ss.w0rd@kamaka-restore:5432/tamanu\n", + "error: insert or update on table \"encounters\" violates foreign key constraint \"fk_patient\"\n", + "DETAIL: Key (patient_id)=(abc-123) is not present in table \"patients\".\n", + ); + + let masked = redact(log, &["p@ss.w0rd".to_string()]); + + assert!(!masked.contains("p@ss"), "{masked}"); + assert!(!masked.contains("p%40ss"), "{masked}"); + assert_eq!( + masked.matches("tamanu:[redacted]@kamaka-restore").count(), + 2, + "{masked}" + ); + assert!( + masked.contains("DETAIL: Key (patient_id)=(abc-123)"), + "the postgres error stays whole: {masked}" + ); + } } diff --git a/src/controllers/replica/schema_build.rs b/src/controllers/replica/schema_build.rs index 420ef7f2..31695589 100644 --- a/src/controllers/replica/schema_build.rs +++ b/src/controllers/replica/schema_build.rs @@ -644,22 +644,26 @@ async fn build_secrets( replica: &PostgresPhysicalReplica, ) -> Option> { let creds: Api = Api::namespaced(client.clone(), namespace); - let password = creds + let creds = creds .get(&replica.creds_secret_name()) .await - .map_err( - |err| warn!(replica = %replica.name_any(), "reading the build credentials failed: {err}"), - ) - .ok() - .and_then(|secret| postgres::read_secret_field(&secret, "password").ok())?; + .map_err(Error::Kube); + let token = build_token(client, namespace, &replica.name_any()).await; + secrets_to_mask(creds, token, &replica.name_any()) +} - let token = build_token(client, namespace, &replica.name_any()) - .await - .map_err( - |err| warn!(replica = %replica.name_any(), "reading the build token failed: {err}"), - ) +fn secrets_to_mask( + creds: Result, + token: Result>, + replica: &str, +) -> Option> { + let password = creds + .and_then(|secret| postgres::read_secret_field(&secret, "password")) + .map_err(|err| warn!(replica, "reading the build password failed: {err}")) + .ok()?; + let token = token + .map_err(|err| warn!(replica, "reading the build token failed: {err}")) .ok()?; - Some(std::iter::once(password).chain(token).collect()) } @@ -1696,4 +1700,68 @@ mod tests { assert_eq!(build_failure(None, Some(&[])), "the build job failed"); assert_eq!(build_failure(Some("\n"), Some(&[])), "the build job failed"); } + + fn creds_with(data: &[(&str, &str)]) -> Secret { + Secret { + data: Some( + data.iter() + .map(|(key, value)| { + ( + key.to_string(), + k8s_openapi::ByteString(value.as_bytes().to_vec()), + ) + }) + .collect(), + ), + ..Default::default() + } + } + + #[test] + fn the_password_and_token_are_masked() { + let secrets = secrets_to_mask( + Ok(creds_with(&[("password", "hunter2")])), + Ok(Some("tok-123".to_string())), + "kamaka", + ); + + assert_eq!( + secrets, + Some(vec!["hunter2".to_string(), "tok-123".to_string()]) + ); + } + + #[test] + fn a_build_with_no_token_masks_the_password_alone() { + let secrets = secrets_to_mask( + Ok(creds_with(&[("password", "hunter2")])), + Ok(None), + "kamaka", + ); + + assert_eq!(secrets, Some(vec!["hunter2".to_string()])); + } + + #[test] + fn an_unreadable_secret_withholds_the_log() { + let missing_secret = secrets_to_mask( + Err(Error::MissingField("no such secret".to_string())), + Ok(Some("tok-123".to_string())), + "kamaka", + ); + let missing_password = secrets_to_mask( + Ok(creds_with(&[("username", "analytics")])), + Ok(Some("tok-123".to_string())), + "kamaka", + ); + let unreadable_token = secrets_to_mask( + Ok(creds_with(&[("password", "hunter2")])), + Err(Error::MissingField("no such job".to_string())), + "kamaka", + ); + + assert_eq!(missing_secret, None, "creds secret unreadable"); + assert_eq!(missing_password, None, "password key missing"); + assert_eq!(unreadable_token, None, "token unreadable"); + } }