diff --git a/crates/private-server/src/fns/ansible_variables.rs b/crates/private-server/src/fns/ansible_variables.rs index b69766ef7..0d22c38e2 100644 --- a/crates/private-server/src/fns/ansible_variables.rs +++ b/crates/private-server/src/fns/ansible_variables.rs @@ -161,6 +161,10 @@ pub struct HistoryEntry { /// The plain value it set or removed. #[schema(value_type = Option)] pub value: Option, + /// Whether its entry holds the plain value it set or removed, so a value of + /// null is told from one the entry never recorded, as an entry written before + /// entries held values does not. + pub value_recorded: bool, /// The plain value a set replaced. #[schema(value_type = Option)] pub replaced: Option, @@ -555,7 +559,9 @@ async fn history_entry( let secret = flag("secret"); let replaced = plain("replaced"); let replaced_secret = flag("replaced_secret"); - let value = plain("value"); + // A plain value is the entry's own, null included, where the entry holds one. + let value = (!secret).then(|| detail.get("value").cloned()).flatten(); + let value_recorded = value.is_some(); let put_away_secret = match change { Change::Set => replaced_secret, Change::Removed => secret, @@ -565,7 +571,7 @@ async fn history_entry( } else { match change { Change::Set => replaced.is_some(), - Change::Removed => value.is_some(), + Change::Removed => value_recorded, } }; Ok(HistoryEntry { @@ -575,6 +581,7 @@ async fn history_entry( change, secret, value, + value_recorded, replaced, replaced_secret, restored_from: detail @@ -811,10 +818,16 @@ pub async fn restore( Change::Removed => change.value, }; let value = put_away.ok_or_else(|| { - AppError::BadRequest(format!( - "change {} replaced nothing, so there is nothing to put back", - args.entry_id - )) + AppError::BadRequest(match change.change { + Change::Set => format!( + "change {} replaced nothing, so there is nothing to put back", + args.entry_id + ), + Change::Removed => format!( + "change {} does not record the value it removed, so there is nothing to put back", + args.entry_id + ), + }) })?; (value, false) } diff --git a/crates/private-server/tests/it/ansible_variables.rs b/crates/private-server/tests/it/ansible_variables.rs index 2ed0dc049..b5956de45 100644 --- a/crates/private-server/tests/it/ansible_variables.rs +++ b/crates/private-server/tests/it/ansible_variables.rs @@ -1273,18 +1273,36 @@ async fn a_secret_change_is_undone_while_its_value_is_held() { } /// A scope's history holds every change to a variable set there, removed ones -/// among them and however long ago within the log, and nothing of another -/// scope's, though the group's record names its environments' too. +/// among them: one set again since, and one removed long enough ago to be off +/// the list of recent removals. It holds nothing of another scope's, though the +/// group's record names its environments' variables too. // spec: ANSVAR#presentation #[tokio::test(flavor = "multi_thread")] async fn a_scopes_history_holds_its_removed_variables_and_no_other_scopes() { commons_tests::server::run(async move |mut conn, _public, private| { let group = insert_group(&mut conn, "kamaka").await; - insert_application(&mut conn, group, "kamaka-central", "tamanu-central", None).await; + let (_, machine) = insert_application_on_its_own_machine( + &mut conn, + group, + "kamaka-central", + "tamanu-central", + None, + None, + ) + .await; let environment = json!({ "server_group_id": group, "rank": "dev" }); let group_scope = json!({ "server_group_id": group }); + let machine_scope = json!({ "machine_id": machine }); set_var(&private, environment.clone(), "retries", json!(3), false).await; set_var(&private, environment.clone(), "kept", json!(true), false).await; + set_var(&private, machine_scope.clone(), "mine", json!(1), false).await; + post( + &private, + "/api/ansible_variables/remove", + named(&environment, "retries"), + ) + .await; + set_var(&private, environment.clone(), "retries", json!(4), false).await; set_var( &private, group_scope.clone(), @@ -1293,26 +1311,38 @@ async fn a_scopes_history_holds_its_removed_variables_and_no_other_scopes() { false, ) .await; - post( - &private, - "/api/ansible_variables/remove", - named(&environment, "retries"), - ) - .await; - // Removed long enough ago to be off the list of recent removals. + + // A removal a month old, in a week laid out as provisioning lays them + // out: the log is never updated, so it is written there in the first place. conn.batch_execute( - "UPDATE audit_entries SET recorded_at = recorded_at - interval '30 days' - WHERE action = 'inventory_variable_removed'", + "CREATE TABLE audit_entries_old PARTITION OF audit_entries \ + FOR VALUES FROM (now() - interval '40 days') TO (now() - interval '20 days') \ + PARTITION BY LIST (retention); \ + CREATE TABLE audit_entries_old_standard PARTITION OF audit_entries_old FOR VALUES IN ('standard'); \ + CREATE TABLE audit_entries_old_year PARTITION OF audit_entries_old FOR VALUES IN ('year'); \ + INSERT INTO audit_entries (recorded_at, retention, actor_kind, actor_id, actor_name, \ + action, records, detail, outcome, settled_at) \ + SELECT recorded_at - interval '30 days', retention, actor_kind, actor_id, actor_name, action, \ + (SELECT jsonb_agg(CASE WHEN r->>'name' = 'retries' \ + THEN r || jsonb_build_object('id', replace(r->>'id', ':retries', ':ancient'), 'name', 'ancient') \ + ELSE r END) FROM jsonb_array_elements(records) AS r), \ + detail || '{\"value\": \"old\"}', outcome, settled_at - interval '30 days' \ + FROM audit_entries WHERE action = 'inventory_variable_removed'", ) .await .unwrap(); + let recent = post( &private, "/api/ansible_variables/removed", environment.clone(), ) .await; - assert_eq!(recent, json!([])); + assert_eq!( + recent, + json!([]), + "one is set again, the other removed too long ago" + ); let history = post( &private, @@ -1333,18 +1363,26 @@ async fn a_scopes_history_holds_its_removed_variables_and_no_other_scopes() { .collect(); assert_eq!( changes, - [("kept", "set"), ("retries", "set"), ("retries", "removed")], + [ + ("retries", "set"), + ("retries", "removed"), + ("kept", "set"), + ("retries", "set"), + ("ancient", "removed"), + ], "newest first, the group's own changes left out" ); - let removal = &history[2]; - assert_eq!(removal["value"], 3); - assert_eq!(removal["undoable"], true); + assert_eq!(history[1]["value"], 3); + let ancient = &history[4]; + assert_eq!(ancient["value"], "old"); + assert_eq!(ancient["undoable"], true); - let mut restore = named(&environment, "retries"); - restore["entry_id"] = removal["entry_id"].clone(); + let mut restore = named(&environment, "ancient"); + restore["entry_id"] = ancient["entry_id"].clone(); post(&private, "/api/ansible_variables/restore", restore).await; let body = read_served(&private, json!({ "server_group_id": group })).await; - assert_eq!(body["vars"]["retries"], 3); + assert_eq!(body["vars"]["ancient"], "old"); + assert_eq!(body["vars"]["retries"], 4); let at_group = post( &private, @@ -1354,6 +1392,129 @@ async fn a_scopes_history_holds_its_removed_variables_and_no_other_scopes() { .await; assert_eq!(at_group.as_array().unwrap().len(), 1); assert_eq!(at_group[0]["name"], "timezone"); + let at_machine = post( + &private, + "/api/ansible_variables/scope_history", + machine_scope, + ) + .await; + assert_eq!(at_machine.as_array().unwrap().len(), 1); + assert_eq!(at_machine[0]["name"], "mine"); + }) + .await +} + +/// A group's recent removals are found among its own entries, so a busy +/// environment's removals, which name the group too, do not crowd them out. +// spec: ANSVAR#presentation +#[tokio::test(flavor = "multi_thread")] +async fn a_busy_environment_does_not_crowd_out_its_groups_removals() { + commons_tests::server::run(async move |mut conn, _public, private| { + let group = insert_group(&mut conn, "kamaka").await; + insert_application(&mut conn, group, "kamaka-central", "tamanu-central", None).await; + let environment = json!({ "server_group_id": group, "rank": "dev" }); + let group_scope = json!({ "server_group_id": group }); + set_var( + &private, + group_scope.clone(), + "timezone", + json!("UTC"), + false, + ) + .await; + post( + &private, + "/api/ansible_variables/remove", + named(&group_scope, "timezone"), + ) + .await; + set_var(&private, environment.clone(), "retries", json!(3), false).await; + post( + &private, + "/api/ansible_variables/remove", + named(&environment, "retries"), + ) + .await; + // More of the environment's removals since than a page of the group's + // entries holds. + conn.batch_execute( + "INSERT INTO audit_entries (recorded_at, retention, actor_kind, actor_id, actor_name, \ + action, records, detail, outcome, settled_at) \ + SELECT recorded_at + n * interval '1 millisecond', retention, actor_kind, actor_id, \ + actor_name, action, records, detail, outcome, settled_at \ + FROM audit_entries, generate_series(1, 600) AS n \ + WHERE action = 'inventory_variable_removed' \ + AND records @> '[{\"name\": \"retries\"}]'", + ) + .await + .unwrap(); + + let at_group = post(&private, "/api/ansible_variables/removed", group_scope).await; + assert_eq!(at_group.as_array().unwrap().len(), 1); + assert_eq!(at_group[0]["name"], "timezone"); + let at_environment = post(&private, "/api/ansible_variables/removed", environment).await; + assert_eq!(at_environment.as_array().unwrap().len(), 1); + assert_eq!(at_environment[0]["name"], "retries"); + }) + .await +} + +/// A change whose entry never recorded its value is told from one that +/// recorded a value of null: the first has nothing to put back, the second puts +/// back null. +// spec: ANSVAR#history +#[tokio::test(flavor = "multi_thread")] +async fn a_change_without_its_value_is_told_from_one_of_null() { + commons_tests::server::run(async move |mut conn, _public, private| { + let group = insert_group(&mut conn, "kamaka").await; + insert_application(&mut conn, group, "kamaka-central", "tamanu-central", None).await; + let environment = json!({ "server_group_id": group, "rank": "dev" }); + set_var(&private, environment.clone(), "proxy", json!(null), false).await; + post( + &private, + "/api/ansible_variables/remove", + named(&environment, "proxy"), + ) + .await; + // The same removal as an entry holding no value, as one written before + // entries held them. + conn.batch_execute( + "INSERT INTO audit_entries (recorded_at, retention, actor_kind, actor_id, actor_name, \ + action, records, detail, outcome, settled_at) \ + SELECT recorded_at + interval '1 millisecond', retention, actor_kind, actor_id, \ + actor_name, action, records, detail - 'value', outcome, settled_at \ + FROM audit_entries WHERE action = 'inventory_variable_removed'", + ) + .await + .unwrap(); + + let history = post( + &private, + "/api/ansible_variables/history", + named(&environment, "proxy"), + ) + .await; + let [unrecorded, of_null, _set] = history.as_array().unwrap().as_slice() else { + panic!("three changes: {history}"); + }; + assert_eq!(unrecorded["value_recorded"], false); + assert_eq!(unrecorded["undoable"], false); + assert_eq!(of_null["value_recorded"], true); + assert_eq!(of_null["value"], Value::Null); + assert_eq!(of_null["undoable"], true); + + let mut restore = named(&environment, "proxy"); + restore["entry_id"] = unrecorded["entry_id"].clone(); + private + .post("/api/ansible_variables/restore") + .json(&restore) + .await + .assert_status_bad_request(); + restore["entry_id"] = of_null["entry_id"].clone(); + post(&private, "/api/ansible_variables/restore", restore).await; + let body = read_served(&private, json!({ "server_group_id": group })).await; + assert_eq!(body["vars"]["proxy"], Value::Null); + assert!(body["vars"].as_object().unwrap().contains_key("proxy")); }) .await } diff --git a/private-web/e2e/ansible.spec.ts b/private-web/e2e/ansible.spec.ts index aedac4016..043c11585 100644 --- a/private-web/e2e/ansible.spec.ts +++ b/private-web/e2e/ansible.spec.ts @@ -4,6 +4,7 @@ import { resetSeededTables, seedAnsibleCatalogue, seedAnsibleVariable, + seedAuditEntry, seedMachine, seedMaintenanceWindow, seedServer, @@ -416,6 +417,70 @@ test.describe("group Ansible", () => { await expect(scope.getByTestId("removed-variables")).toHaveCount(0); }); + // A scope's history names each variable changed there, a removal among them + // even once the variable is set again and off the list of removals, and + // puts it back. Another scope's changes are not in it. + // spec: ANSVAR#presentation + test("puts back a removed variable from its scope's history", async ({ page, sql }) => { + const group = await seedServerGroup(sql, { name: "kamaka", tags: {} }); + await seedServer(sql, { + name: "kamaka-prod-central", + type: "tamanu-central", + rank: "production", + groupId: group.id, + tags: {}, + }); + + await page.goto(`/fleet/groups/${group.id}`); + const production = page + .getByTestId("group-ansible") + .getByTestId("environment-production"); + const scope = production.getByTestId("scope-environment"); + await setVariable(production.getByTestId("scope-group"), "timezone", "Pacific/Fiji"); + await setVariable(scope, "log_level", "info"); + await setVariable(scope, "region", "nz"); + const logLevel = scope.getByTestId("var").filter({ hasText: "log_level" }); + await expect(scope.getByTestId("var").filter({ hasText: "region" })).toHaveCount(1); + await scope.getByTestId("remove-log_level").click(); + await expect(logLevel).toHaveCount(0); + await page.keyboard.press("Escape"); + await setVariable(scope, "log_level", "debug"); + await expect(logLevel).toContainText('"debug"'); + await expect(scope.getByTestId("removed-variables")).toHaveCount(0); + // A removal whose entry holds no value, as one written before entries + // held them, names no value and offers nothing to put back. + await seedAuditEntry(sql, { + action: "inventory_variable_removed", + minutesAgo: 60, + records: [ + { + kind: "variable", + id: `inv-vars-production-${group.id}:proxy`, + name: "proxy", + }, + { kind: "group", id: group.id, name: "kamaka" }, + ], + detail: { rank: "production", secret: false }, + }); + + await page.getByRole("button", { name: "History of production", exact: true }).click(); + const history = page.getByTestId("variable-history"); + const entries = history.getByTestId("history-entry"); + await expect(entries).toHaveCount(5); + await expect(entries.nth(0)).toContainText('log_level Set to "debug"'); + await expect(entries.nth(1)).toContainText('log_level Removed "info"'); + await expect(entries.nth(2)).toContainText('region Set to "nz"'); + await expect(entries.nth(3)).toContainText('log_level Set to "info"'); + await expect(entries.nth(4)).toContainText("proxy Removed"); + await expect(entries.nth(4)).not.toContainText("null"); + await expect(entries.nth(4).getByTestId("history-restore")).toHaveCount(0); + await expect(history).not.toContainText("timezone"); + + await entries.nth(1).getByTestId("history-restore").click(); + await expect(page.getByTestId("variable-history")).toHaveCount(0); + await expect(logLevel).toContainText('"info"'); + }); + // The name completes from the catalogue, showing each variable's type, and // the value is entered in a form shaped by it. A variable limited to other // scopes is not offered. diff --git a/private-web/openapi.json b/private-web/openapi.json index 6fb3227c9..82a0680e9 100644 --- a/private-web/openapi.json +++ b/private-web/openapi.json @@ -5171,6 +5171,10 @@ }, "value": { "description": "The plain value it set or removed." + }, + "value_recorded": { + "description": "Whether its entry holds the plain value it set or removed, so a value of\nnull is told from one the entry never recorded, as an entry written before\nentries held values does not.", + "type": "boolean" } }, "required": [ @@ -5179,6 +5183,7 @@ "actor", "change", "secret", + "value_recorded", "replaced_secret", "undoable" ], diff --git a/private-web/src/api-types.ts b/private-web/src/api-types.ts index 2c5071e64..0aced77e4 100644 --- a/private-web/src/api-types.ts +++ b/private-web/src/api-types.ts @@ -8729,6 +8729,12 @@ export interface components { undoable: boolean; /** @description The plain value it set or removed. */ value?: unknown; + /** + * @description Whether its entry holds the plain value it set or removed, so a value of + * null is told from one the entry never recorded, as an entry written before + * entries held values does not. + */ + value_recorded: boolean; }; /** @description The window a run declared, and the end to give it. */ HoldArgs: { diff --git a/private-web/src/components/ansible/VariableHistory.tsx b/private-web/src/components/ansible/VariableHistory.tsx index 10682ac28..f2cd58e48 100644 --- a/private-web/src/components/ansible/VariableHistory.tsx +++ b/private-web/src/components/ansible/VariableHistory.tsx @@ -138,9 +138,14 @@ export default function VariableHistory({ function describe(entry: VariableHistoryEntry): string { if (entry.change === "removed") { - return entry.secret ? "Removed, secret" : `Removed ${show(entry.value)}`; + if (entry.secret) return "Removed, secret"; + return entry.value_recorded ? `Removed ${show(entry.value)}` : "Removed"; } - const set = entry.secret ? "Set, secret" : `Set to ${show(entry.value)}`; + const set = entry.secret + ? "Set, secret" + : entry.value_recorded + ? `Set to ${show(entry.value)}` + : "Set"; if (entry.replaced_secret) return `${set}, over a secret`; if (entry.replaced !== undefined && entry.replaced !== null) return `${set}, was ${show(entry.replaced)}`;