From 56716fc8667ab82036de344c7ac1fdd5a794e775 Mon Sep 17 00:00:00 2001 From: Ashutosh More Date: Sat, 12 Sep 2026 15:52:57 +0530 Subject: [PATCH] fix(dns): pair retained values before position in the set plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `plan_set` paired existing records to `--data` values purely by position, so a value that was staying in the set could be paired with a different record and re-created. v3 keys record identity on (name, type, data), so that create fails with DUPLICATE_RECORD — and `apply_replace` keeps the old record when its create fails, while the surplus `Delete` later in the plan still runs. Narrowing `www A {1.2.3.4, 5.6.7.8}` to just 5.6.7.8 therefore deleted 5.6.7.8 and left 1.2.3.4 behind — the exact inverse of what was asked. A pure reorder of the same values failed both replaces as duplicates. Pair each desired value with the record already holding it first, then pair whatever is left by position as before. `plan_set` now takes (record_id, current value) so it can see content. --- rust/src/dns/set/mod.rs | 18 ++-- rust/src/dns/set/outcome.rs | 13 ++- rust/src/dns/set/plan.rs | 173 +++++++++++++++++++++++++++++++----- 3 files changed, 173 insertions(+), 31 deletions(-) diff --git a/rust/src/dns/set/mod.rs b/rust/src/dns/set/mod.rs index 544d029e..e2d315fb 100644 --- a/rust/src/dns/set/mod.rs +++ b/rust/src/dns/set/mod.rs @@ -8,8 +8,8 @@ use crate::scopes::DOMAINS_DNS_UPDATE; use super::records::verify_with_list_action; use super::records::{ - RecordOptions, RecordWriteArgs, fetch_records, validate_caa_fields, validate_svcb_fields, - validate_tlsa_fields, + RecordOptions, RecordWriteArgs, fetch_records, record_value, validate_caa_fields, + validate_svcb_fields, validate_tlsa_fields, }; mod outcome; @@ -140,12 +140,20 @@ pub(super) fn command() -> RuntimeCommandSpec { .with_fix(fix) .into_cli_error()); } - let existing_ids: Vec = existing + // (record_id, current value) per existing record — `plan_set` pairs a + // desired value with the record already holding it before falling back + // to position, so a retained value is never re-created. + let existing_pairs: Vec<(String, String)> = existing .iter() - .filter_map(|r| r.record_id.clone()) + .filter_map(|r| { + Some(( + r.record_id.clone()?, + record_value(r).unwrap_or_default().to_owned(), + )) + }) .collect(); - let plan = plan_set(&existing_ids, &data); + let plan = plan_set(&existing_pairs, &data); if ctx.dry_run() { return Ok(CommandResult::new(dry_run_set_preview( &domain, diff --git a/rust/src/dns/set/outcome.rs b/rust/src/dns/set/outcome.rs index 31bc99d2..80cd896d 100644 --- a/rust/src/dns/set/outcome.rs +++ b/rust/src/dns/set/outcome.rs @@ -206,7 +206,11 @@ mod tests { #[test] fn dry_run_set_preview_matches_the_plan_it_would_execute() { let plan = plan_set( - &["r1".to_string(), "r2".to_string(), "r3".to_string()], + &[ + ("r1".to_string(), "1.1.1.1".to_string()), + ("r2".to_string(), "2.2.2.2".to_string()), + ("r3".to_string(), "3.3.3.3".to_string()), + ], &["9.9.9.9".to_string(), "8.8.8.8".to_string()], ); let preview = dry_run_set_preview("example.com", "A", "www", &plan); @@ -225,7 +229,10 @@ mod tests { /// catch, since it never goes through the projection). #[test] fn dry_run_set_preview_survives_default_field_projection() { - let plan = plan_set(&["r1".to_string()], &["9.9.9.9".to_string()]); + let plan = plan_set( + &[("r1".to_string(), "1.1.1.1".to_string())], + &["9.9.9.9".to_string()], + ); let preview = dry_run_set_preview("example.com", "A", "www", &plan); let default_fields = "domain,type,name,replaced,created,deleted,action,plan"; let projected = cli_engine::output::filter_fields(&preview, default_fields); @@ -245,7 +252,7 @@ mod tests { #[test] fn dry_run_set_preview_renders_plan_as_a_nested_table() { let plan = plan_set( - &["r1".to_string()], + &[("r1".to_string(), "1.1.1.1".to_string())], &["9.9.9.9".to_string(), "8.8.8.8".to_string()], ); let preview = dry_run_set_preview("example.com", "A", "www", &plan); diff --git a/rust/src/dns/set/plan.rs b/rust/src/dns/set/plan.rs index 5a1a6958..e2ea72c7 100644 --- a/rust/src/dns/set/plan.rs +++ b/rust/src/dns/set/plan.rs @@ -1,4 +1,4 @@ -//! Reconcile planning for `dns set` — deciding, from the existing record ids +//! Reconcile planning for `dns set` — deciding, from the existing records //! and desired `--data` values, which per-record ops to run. /// One reconcile action for `dns set` over v3's per-record endpoints. @@ -20,27 +20,63 @@ pub(super) enum SetAction { Create { data: String }, } -/// Reconcile the existing records for a type+name (their ids, in list order) with -/// the desired `--data` values: pair up the overlap (`Replace`), `Delete` -/// the surplus existing, `Create` the surplus desired. Pure so the plan — the -/// least-destructive way to emulate a set-replace over per-record v3 ops — is -/// unit-testable. -pub(super) fn plan_set(existing_ids: &[String], desired: &[String]) -> Vec { - let overlap = existing_ids.len().min(desired.len()); - let mut actions = Vec::with_capacity(existing_ids.len().max(desired.len())); - for i in 0..overlap { - actions.push(SetAction::Replace { - record_id: existing_ids[i].clone(), - data: desired[i].clone(), - }); +/// Reconcile the existing records for a type+name — `(record_id, current value)` +/// in list order — with the desired `--data` values: pair them up (`Replace`), +/// `Delete` the surplus existing, `Create` the surplus desired. Pure so the +/// plan — the least-destructive way to emulate a set-replace over per-record v3 +/// ops — is unit-testable. +/// +/// A desired value that some existing record already holds is paired with *that* +/// record before anything is paired by position. v3 keys record identity on +/// (name, type, data), so creating a value another record still holds fails with +/// `DUPLICATE_RECORD`. Under a purely positional pairing that is not a harmless +/// no-op: [`super::write::apply_replace`] keeps the old record when its create +/// fails, while the surplus `Delete` later in the plan still runs — so narrowing +/// `www A {1.2.3.4, 5.6.7.8}` to just `5.6.7.8` would delete `5.6.7.8` and leave +/// `1.2.3.4` behind, the exact inverse of what was asked. Pairing retained values +/// first means a value that stays in the set is never re-created. +pub(super) fn plan_set(existing: &[(String, String)], desired: &[String]) -> Vec { + let mut paired: Vec> = vec![None; desired.len()]; + let mut taken = vec![false; existing.len()]; + + // Pass 1: pair by content, wherever the holder sits in the list. + for (d, want) in desired.iter().enumerate() { + if let Some(e) = (0..existing.len()).find(|&e| !taken[e] && &existing[e].1 == want) { + taken[e] = true; + paired[d] = Some(e); + } + } + + // Pass 2: whatever is left pairs by position, as before. + let mut free: Vec = (0..existing.len()).filter(|&e| !taken[e]).rev().collect(); + for slot in paired.iter_mut().filter(|s| s.is_none()) { + let Some(e) = free.pop() else { break }; + taken[e] = true; + *slot = Some(e); } - for id in &existing_ids[overlap..] { - actions.push(SetAction::Delete { - record_id: id.clone(), - }); + + let mut actions = Vec::with_capacity(existing.len().max(desired.len())); + for (d, want) in desired.iter().enumerate() { + if let Some(e) = paired[d] { + actions.push(SetAction::Replace { + record_id: existing[e].0.clone(), + data: want.clone(), + }); + } + } + // Deletes stay ahead of creates: freeing a surplus record's value is what + // lets a create of that same value succeed. + for (e, (id, _)) in existing.iter().enumerate() { + if !taken[e] { + actions.push(SetAction::Delete { + record_id: id.clone(), + }); + } } - for d in &desired[overlap..] { - actions.push(SetAction::Create { data: d.clone() }); + for (d, want) in desired.iter().enumerate() { + if paired[d].is_none() { + actions.push(SetAction::Create { data: want.clone() }); + } } actions } @@ -49,10 +85,18 @@ pub(super) fn plan_set(existing_ids: &[String], desired: &[String]) -> Vec Vec<(String, String)> { + pairs + .iter() + .map(|(id, value)| ((*id).to_string(), (*value).to_string())) + .collect() + } + #[test] fn plan_set_reuses_overlap_deletes_extra_creates_shortfall() { - // 3 existing, 2 desired → reuse 2 ids, delete the 3rd. - let existing = vec!["r1".to_string(), "r2".to_string(), "r3".to_string()]; + // 3 existing, 2 desired, no value in common → reuse 2 ids, delete the 3rd. + let existing = ids(&[("r1", "1.1.1.1"), ("r2", "2.2.2.2"), ("r3", "3.3.3.3")]); let desired = vec!["9.9.9.9".to_string(), "8.8.8.8".to_string()]; assert_eq!( plan_set(&existing, &desired), @@ -73,7 +117,7 @@ mod tests { // 1 existing, 3 desired → reuse 1 id, create 2. assert_eq!( plan_set( - &["r1".to_string()], + &ids(&[("r1", "z")]), &["a".to_string(), "b".to_string(), "c".to_string()] ), vec![ @@ -91,4 +135,87 @@ mod tests { vec![SetAction::Create { data: "x".into() }] ); } + + #[test] + fn plan_set_pairs_a_retained_value_with_the_record_that_holds_it() { + // Narrowing `www A {1.2.3.4, 5.6.7.8}` down to just 5.6.7.8 must pair the + // desired value with r2, which already holds it, and delete r1. Pairing + // positionally instead (Replace{r1 → 5.6.7.8}, Delete{r2}) makes the + // create collide with r2's still-live duplicate, so `apply_replace` keeps + // r1 while the surplus delete still removes r2 — leaving the zone holding + // exactly the value the user asked to drop. + let existing = ids(&[("r1", "1.2.3.4"), ("r2", "5.6.7.8")]); + assert_eq!( + plan_set(&existing, &["5.6.7.8".to_string()]), + vec![ + SetAction::Replace { + record_id: "r2".into(), + data: "5.6.7.8".into() + }, + SetAction::Delete { + record_id: "r1".into() + }, + ] + ); + } + + #[test] + fn plan_set_reordering_the_same_values_pairs_each_to_itself() { + // A pure reorder is a no-op set. Every pairing must land on the record + // that already holds the value, so `apply_replace` short-circuits each + // one instead of issuing two creates that both fail as duplicates. + let existing = ids(&[("r1", "1.2.3.4"), ("r2", "5.6.7.8")]); + assert_eq!( + plan_set(&existing, &["5.6.7.8".to_string(), "1.2.3.4".to_string()]), + vec![ + SetAction::Replace { + record_id: "r2".into(), + data: "5.6.7.8".into() + }, + SetAction::Replace { + record_id: "r1".into(), + data: "1.2.3.4".into() + }, + ] + ); + } + + #[test] + fn plan_set_keeps_a_retained_value_and_still_replaces_the_rest() { + // Mixed case: 5.6.7.8 stays (pair with r2), 1.2.3.4 goes, 9.9.9.9 is new. + // The freed id r1 is reused positionally rather than deleted-and-created. + let existing = ids(&[("r1", "1.2.3.4"), ("r2", "5.6.7.8")]); + assert_eq!( + plan_set(&existing, &["5.6.7.8".to_string(), "9.9.9.9".to_string()]), + vec![ + SetAction::Replace { + record_id: "r2".into(), + data: "5.6.7.8".into() + }, + SetAction::Replace { + record_id: "r1".into(), + data: "9.9.9.9".into() + }, + ] + ); + } + + #[test] + fn plan_set_duplicate_desired_values_pair_distinct_records() { + // Two identical --data values must not both pair with the same record. + let existing = ids(&[("r1", "1.2.3.4"), ("r2", "1.2.3.4")]); + assert_eq!( + plan_set(&existing, &["1.2.3.4".to_string(), "1.2.3.4".to_string()]), + vec![ + SetAction::Replace { + record_id: "r1".into(), + data: "1.2.3.4".into() + }, + SetAction::Replace { + record_id: "r2".into(), + data: "1.2.3.4".into() + }, + ] + ); + } }