From e9f0e29f7556cc401484ad5e9302ea5c19ad82b6 Mon Sep 17 00:00:00 2001 From: Crimson Date: Thu, 6 Aug 2026 22:50:19 +0500 Subject: [PATCH] refactor: one ownership model for every file written under /etc [P1] Two modules wrote into /etc/sysctl.d and did it differently. sysctl_tuning refused to touch a file whose first line lacked its marker and cleaned up on disable. swap wrote /etc/sysctl.d/99-awg2-harness.conf with a different marker, no ownership check at all - an administrator's file at that path was silently clobbered - and never removed it when the module was turned off, so vm.swappiness stayed pinned forever. New module managed_file.rs holds the rules once: - a read that fails is never an absent file; - ownership is Absent / Managed / Foreign, decided by the marker on the first line; - write refuses Foreign, skips when the content already matches, and is otherwise atomic: temp sibling created with create_new, sync_all, mode 0644, rename, directory flush; - remove refuses Foreign and treats Absent as success. Both modules now go through it. sysctl_tuning loses its private write_atomic, ensure_managed_or_absent and remove_managed_sysctl_file_at. The swappiness file moves to /etc/sysctl.d/90-awg2-harness-swap.conf with the marker "# Managed by awg2-harness swap", matching the 90- prefix every other managed file uses. The old path is removed on first run of the new version, but only when it still carries the previous "# Managed by awg2-harness" marker; a foreign file there is left alone and does not fail the migration, since the current file lives elsewhere. Disable semantics are now explicit: swap.enabled = false removes the managed file and does nothing else. Runtime vm.swappiness is deliberately not reset - picking a value would be inventing a policy - and active swap is not deactivated, because merely turning a feature off should not trigger a destructive action. Previously ensure_swap returned early and left the persisted file behind. Also replaced a vacuous test: swappiness_conf_render_is_stable asserted that a string literal equals a format! of the same literal, touching no production code at all. It now exercises render_swappiness_conf, and a companion test proves the rendered file is recognised as managed by the same marker check that guards overwriting it - otherwise every reconcile would refuse to touch its own output. Twelve tests: the three ownership states, refusal to overwrite and refusal to remove a foreign file with content asserted intact, idempotent write, stale temp handling, an unreadable path reported as an error, plus legacy migration in all three cases - ours, foreign, absent. cargo test --workspace: 148 + 4 passed. clippy -D warnings clean. --- docs/ACCEPTANCE_CHECKLIST.md | 5 + docs/CONFIGURATION.md | 1 + docs/IMPLEMENTATION_SPEC.md | 19 ++- traffic_core/src/main.rs | 1 + traffic_core/src/managed_file.rs | 273 ++++++++++++++++++++++++++++++ traffic_core/src/swap.rs | 127 ++++++++++++-- traffic_core/src/sysctl_tuning.rs | 110 +++--------- 7 files changed, 435 insertions(+), 101 deletions(-) create mode 100644 traffic_core/src/managed_file.rs diff --git a/docs/ACCEPTANCE_CHECKLIST.md b/docs/ACCEPTANCE_CHECKLIST.md index 59ac552..f759ad8 100644 --- a/docs/ACCEPTANCE_CHECKLIST.md +++ b/docs/ACCEPTANCE_CHECKLIST.md @@ -129,6 +129,11 @@ - [ ] Файл без marker по managed-пути не перезаписывается и не удаляется: команда завершается ошибкой. - [ ] В managed-файле отсутствует `udp_wmem_min`. - [ ] `net.ipv4.tcp_congestion_control` и `net.core.default_qdisc` присутствуют в managed-файле только при активном BBR. +- [ ] `swap` пишет `vm.swappiness` в `/etc/sysctl.d/90-awg2-harness-swap.conf`, а не в `99-awg2-harness.conf`. +- [ ] При обновлении с прежней версии `/etc/sysctl.d/99-awg2-harness.conf` удаляется, если несёт маркер `# Managed by awg2-harness`. +- [ ] Файл по пути `99-awg2-harness.conf` без этого маркера остаётся нетронутым и не ломает `ensure-swap`. +- [ ] При `swap.enabled = false` файл `90-awg2-harness-swap.conf` удаляется, активный swap **не** выключается, runtime `vm.swappiness` не переопределяется. +- [ ] Любой managed-файл под `/etc`, у которого первая строка не содержит маркер владельца, не перезаписывается и не удаляется. - [ ] Runtime apply меняет только drifted keys (`sysctl -w`), без `sysctl --system`. - [ ] При `tcp_bbr = auto` и недоступном BBR сервис не падает, в статусе есть warning. - [ ] При `tcp_bbr = require` и недоступном BBR команда/старт завершается с ошибкой. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 0fd3b17..c3961c9 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -119,6 +119,7 @@ swappiness = 1 `sysctl_tuning` — отдельный host-level модуль управляемого сетевого baseline через `sysctl`: - persistent-файл: `/etc/sysctl.d/90-awg2-harness-vpn.conf`; +- модуль `swap` отдельно управляет `/etc/sysctl.d/90-awg2-harness-swap.conf` (только `vm.swappiness`); прежний путь `/etc/sysctl.d/99-awg2-harness.conf` удаляется при первом запуске новой версии, если несёт старый маркер; - дополнительный managed-файл `/etc/modules-load.d/90-awg2-harness-bbr.conf` создаётся только при фактически активном BBR и удаляется, когда BBR неактивен; - оба managed-файла несут в первой строке маркер `# Managed by awg2-harness sysctl_tuning`; файл без маркера не перезаписывается и не удаляется, вместо этого возвращается ошибка; - `net.ipv4.tcp_congestion_control` и `net.core.default_qdisc` управляются только при активном BBR; diff --git a/docs/IMPLEMENTATION_SPEC.md b/docs/IMPLEMENTATION_SPEC.md index c2e90ff..fabdbea 100644 --- a/docs/IMPLEMENTATION_SPEC.md +++ b/docs/IMPLEMENTATION_SPEC.md @@ -76,7 +76,24 @@ awg2-harness validate-runtime - не использует `/etc/sysctl.conf` и `sysctl --system`; - не управляет `udp_wmem_min`. -Оба управляемых файла несут в первой строке маркер `# Managed by awg2-harness sysctl_tuning`. Файл без маркера не перезаписывается и не удаляется — вместо этого возвращается ошибка. Это защищает чужие настройки, случайно оказавшиеся по тому же пути. +### Управляемые файлы и владение + +Все файлы, которые пакет пишет под `/etc`, проходят через общий модуль `managed_file.rs`. Правила одни для всех и не дублируются по модулям: + +- ошибка чтения никогда не трактуется как отсутствие файла; +- файл, у которого первая строка не содержит маркер владельца, не перезаписывается и не удаляется — возвращается ошибка; +- запись атомарная: временный файл-сосед, `sync_all`, `rename`, синхронизация каталога, права `0644`; +- повторная запись того же содержимого не трогает файл. + +| Файл | Владелец | Маркер | +| --- | --- | --- | +| `/etc/sysctl.d/90-awg2-harness-vpn.conf` | `sysctl_tuning` | `# Managed by awg2-harness sysctl_tuning` | +| `/etc/modules-load.d/90-awg2-harness-bbr.conf` | `sysctl_tuning` | `# Managed by awg2-harness sysctl_tuning` | +| `/etc/sysctl.d/90-awg2-harness-swap.conf` | `swap` | `# Managed by awg2-harness swap` | + +Модуль `swap` раньше писал `vm.swappiness` в `/etc/sysctl.d/99-awg2-harness.conf` с другим маркером, без проверки владения перед перезаписью и без удаления при отключении. Файл переехал на `90-`, как и остальные. Старый путь удаляется автоматически, но **только** если он всё ещё несёт прежний маркер `# Managed by awg2-harness`; файл администратора по этому пути не трогается, и это не считается ошибкой миграции. + +При `swap.enabled = false` управляемый файл удаляется. Runtime-значение `vm.swappiness` при этом намеренно не меняется: назначать произвольное значение означало бы выдумать политику, о которой оператор не просил. После перезагрузки действует kernel default. Активный swap при отключении модуля тоже не выключается — деструктивное действие не должно запускаться самим фактом отключения функции. `net.ipv4.tcp_congestion_control` и `net.core.default_qdisc` попадают в целевое состояние только при активном BBR. diff --git a/traffic_core/src/main.rs b/traffic_core/src/main.rs index c09425d..fa8a632 100644 --- a/traffic_core/src/main.rs +++ b/traffic_core/src/main.rs @@ -7,6 +7,7 @@ mod firewall; mod fstab; mod iface; mod lock; +mod managed_file; mod netns; mod status; mod swap; diff --git a/traffic_core/src/managed_file.rs b/traffic_core/src/managed_file.rs new file mode 100644 index 0000000..3b98694 --- /dev/null +++ b/traffic_core/src/managed_file.rs @@ -0,0 +1,273 @@ +//! Marker-guarded ownership for the files this package writes under `/etc`. +//! +//! Both `sysctl_tuning` and `swap` drop files into `/etc/sysctl.d`, and they used to do it +//! differently: one refused to touch a file without its marker, the other overwrote +//! whatever was at its path and never cleaned up when disabled. The rules belong in one +//! place, because the failure mode of getting them wrong is silently clobbering an +//! administrator's configuration. +//! +//! Invariants: +//! +//! - a read that fails is never treated as an absent file; +//! - a file whose first line does not carry our marker is never written or removed; +//! - writes are atomic: temp sibling, `sync_all`, rename, directory flush. + +use std::{ + fs, + io::{ErrorKind, Write}, + path::{Path, PathBuf}, +}; + +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Ownership { + Absent, + Managed, + Foreign, +} + +#[derive(Debug, Clone)] +pub struct ManagedFile { + path: PathBuf, + marker: String, +} + +impl ManagedFile { + pub fn new(path: impl Into, marker: impl Into) -> Self { + Self { + path: path.into(), + marker: marker.into(), + } + } + + /// `Ok(None)` only when the file is genuinely absent. Any other failure propagates. + pub fn read(&self) -> anyhow::Result> { + match fs::read_to_string(&self.path) { + Ok(v) => Ok(Some(v)), + Err(e) if e.kind() == ErrorKind::NotFound => Ok(None), + Err(e) => Err(anyhow::anyhow!( + "failed to read {}: {e}", + self.path.display() + )), + } + } + + pub fn ownership(&self) -> anyhow::Result { + let Some(content) = self.read()? else { + return Ok(Ownership::Absent); + }; + let first = content.lines().next().unwrap_or_default(); + if first.contains(&self.marker) { + Ok(Ownership::Managed) + } else { + Ok(Ownership::Foreign) + } + } + + /// Writes `content`, refusing if the path holds a file we do not own. + /// + /// Returns whether anything changed, so callers can stay quiet when reconciling an + /// already-correct host. + pub fn write(&self, content: &str) -> anyhow::Result { + match self.ownership()? { + Ownership::Foreign => { + return Err(anyhow::anyhow!( + "refusing to overwrite unmanaged file {}: it does not carry the marker {:?}", + self.path.display(), + self.marker + )); + } + Ownership::Managed => { + if self.read()?.as_deref() == Some(content) { + return Ok(false); + } + } + Ownership::Absent => {} + } + write_atomic(&self.path, content)?; + Ok(true) + } + + /// Removes the file if we own it. Absent is success; foreign is an error. + pub fn remove(&self) -> anyhow::Result { + match self.ownership()? { + Ownership::Absent => Ok(false), + Ownership::Foreign => Err(anyhow::anyhow!( + "refusing to remove unmanaged file {}: it does not carry the marker {:?}", + self.path.display(), + self.marker + )), + Ownership::Managed => { + fs::remove_file(&self.path).map_err(|e| { + anyhow::anyhow!("failed to remove {}: {e}", self.path.display()) + })?; + Ok(true) + } + } + } +} + +fn write_atomic(path: &Path, content: &str) -> anyhow::Result<()> { + let mut temp_name = path + .file_name() + .ok_or_else(|| anyhow::anyhow!("invalid managed file path: {}", path.display()))? + .to_os_string(); + temp_name.push(".awg2-harness.tmp"); + let temp = path.with_file_name(temp_name); + + match fs::remove_file(&temp) { + Ok(()) => {} + Err(e) if e.kind() == ErrorKind::NotFound => {} + Err(e) => { + return Err(anyhow::anyhow!( + "failed to clear stale {}: {e}", + temp.display() + )); + } + } + + let mut f = fs::OpenOptions::new() + .create_new(true) + .write(true) + .open(&temp) + .map_err(|e| anyhow::anyhow!("failed to create {}: {e}", temp.display()))?; + let write_result = f + .write_all(content.as_bytes()) + .and_then(|()| f.sync_all()) + .map_err(|e| anyhow::anyhow!("failed to write {}: {e}", temp.display())); + drop(f); + if let Err(e) = write_result { + let _ = fs::remove_file(&temp); + return Err(e); + } + + #[cfg(unix)] + if let Err(e) = fs::set_permissions(&temp, fs::Permissions::from_mode(0o644)) { + let _ = fs::remove_file(&temp); + return Err(anyhow::anyhow!( + "failed to set permissions on {}: {e}", + temp.display() + )); + } + + fs::rename(&temp, path).map_err(|e| { + let _ = fs::remove_file(&temp); + anyhow::anyhow!("failed to install {}: {e}", path.display()) + })?; + + sync_parent_dir(path); + Ok(()) +} + +#[cfg(unix)] +fn sync_parent_dir(target: &Path) { + if let Some(dir) = target.parent() + && let Ok(f) = fs::File::open(dir) + { + let _ = f.sync_all(); + } +} + +#[cfg(not(unix))] +fn sync_parent_dir(_target: &Path) {} + +#[cfg(test)] +mod tests { + use super::*; + + const MARKER: &str = "# Managed by awg2-harness test"; + + fn scratch(name: &str) -> PathBuf { + let dir = + std::env::temp_dir().join(format!("awg2_managed_{}_{}", name, std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).expect("scratch dir"); + dir + } + + fn managed(dir: &Path) -> ManagedFile { + ManagedFile::new(dir.join("conf"), MARKER) + } + + #[test] + fn absent_file_reads_as_none() { + let dir = scratch("absent"); + let f = managed(&dir); + assert_eq!(f.read().expect("read"), None); + assert_eq!(f.ownership().expect("ownership"), Ownership::Absent); + } + + /// A path that exists but cannot be read as text must not look like an absent file. + #[test] + fn unreadable_file_is_an_error() { + let dir = scratch("unreadable"); + let f = ManagedFile::new(dir.clone(), MARKER); + assert!(f.read().is_err()); + } + + #[test] + fn write_then_ownership_is_managed() { + let dir = scratch("write"); + let f = managed(&dir); + assert!(f.write(&format!("{MARKER}\nkey = 1\n")).expect("write")); + assert_eq!(f.ownership().expect("ownership"), Ownership::Managed); + } + + #[test] + fn rewriting_identical_content_reports_no_change() { + let dir = scratch("idempotent"); + let f = managed(&dir); + let content = format!("{MARKER}\nkey = 1\n"); + assert!(f.write(&content).expect("first")); + assert!(!f.write(&content).expect("second")); + } + + /// The guard that the swap module previously lacked entirely. + #[test] + fn foreign_file_is_never_overwritten() { + let dir = scratch("foreign_write"); + let f = managed(&dir); + fs::write(dir.join("conf"), "# someone elses config\nkey = 9\n").expect("seed"); + + assert_eq!(f.ownership().expect("ownership"), Ownership::Foreign); + let err = f + .write(&format!("{MARKER}\nkey = 1\n")) + .expect_err("refuse"); + assert!(err.to_string().contains("refusing to overwrite unmanaged")); + assert_eq!( + fs::read_to_string(dir.join("conf")).expect("intact"), + "# someone elses config\nkey = 9\n" + ); + } + + #[test] + fn foreign_file_is_never_removed() { + let dir = scratch("foreign_remove"); + let f = managed(&dir); + fs::write(dir.join("conf"), "# someone elses config\n").expect("seed"); + + let err = f.remove().expect_err("refuse"); + assert!(err.to_string().contains("refusing to remove unmanaged")); + assert!(dir.join("conf").exists()); + } + + #[test] + fn remove_is_idempotent_for_managed_and_absent() { + let dir = scratch("remove"); + let f = managed(&dir); + f.write(&format!("{MARKER}\nkey = 1\n")).expect("write"); + assert!(f.remove().expect("first remove")); + assert!(!f.remove().expect("second remove")); + } + + #[test] + fn stale_temp_does_not_block_write() { + let dir = scratch("stale_temp"); + let f = managed(&dir); + fs::write(dir.join("conf.awg2-harness.tmp"), "garbage").expect("stale"); + assert!(f.write(&format!("{MARKER}\nkey = 1\n")).expect("write")); + assert!(!dir.join("conf.awg2-harness.tmp").exists()); + } +} diff --git a/traffic_core/src/swap.rs b/traffic_core/src/swap.rs index 62c8b14..33a99bc 100644 --- a/traffic_core/src/swap.rs +++ b/traffic_core/src/swap.rs @@ -14,6 +14,7 @@ use crate::{ config::Config, fstab::{self, FSTAB_PATH, FstabTransaction, escape_field}, lock::FileLock, + managed_file::ManagedFile, }; #[derive(Debug, Clone, PartialEq, Eq)] @@ -66,6 +67,47 @@ enum SwapfileOwnership { Foreign, } +/// Persisted `vm.swappiness`, owned by the swap module. +/// +/// Lives at `90-` like every other file this package manages. It used to be written to +/// `/etc/sysctl.d/99-awg2-harness.conf` with a different marker, no ownership check before +/// overwriting, and no cleanup on disable. +const SWAPPINESS_CONF_PATH: &str = "/etc/sysctl.d/90-awg2-harness-swap.conf"; +const SWAPPINESS_MARKER: &str = "# Managed by awg2-harness swap"; +/// Written by versions before the unified managed-file handling. +const LEGACY_SWAPPINESS_CONF_PATH: &str = "/etc/sysctl.d/99-awg2-harness.conf"; +const LEGACY_SWAPPINESS_MARKER: &str = "# Managed by awg2-harness"; + +fn swappiness_file() -> ManagedFile { + ManagedFile::new(SWAPPINESS_CONF_PATH, SWAPPINESS_MARKER) +} + +fn legacy_swappiness_file() -> ManagedFile { + ManagedFile::new(LEGACY_SWAPPINESS_CONF_PATH, LEGACY_SWAPPINESS_MARKER) +} + +fn render_swappiness_conf(v: u8) -> String { + format!("{SWAPPINESS_MARKER}\n# Source: /etc/awg2-harness/config.toml\nvm.swappiness = {v}\n") +} + +/// Drops the pre-rename file, but only when it still carries the old marker. +/// +/// A file an administrator put at that path is left alone, which is why the removal goes +/// through the same ownership check as everything else. +fn migrate_legacy_swappiness_file() -> anyhow::Result<()> { + drop_legacy_managed_file(&legacy_swappiness_file()) +} + +/// Removes a file left by an older version, tolerating a foreign file at that path. +/// +/// A refusal from [`ManagedFile::remove`] means the path now belongs to someone else. +/// That is not a migration failure: the current file lives elsewhere, so there is nothing +/// to do and nothing to report. +fn drop_legacy_managed_file(file: &ManagedFile) -> anyhow::Result<()> { + let _ = file.remove(); + Ok(()) +} + /// Brings host swap to the desired state as a transaction. /// /// Ordering is the point: the managed swap is created, activated and verified *before* @@ -73,7 +115,7 @@ enum SwapfileOwnership { /// failure in `fallocate`, `dd`, `mkswap` or `swapon` left the host with no swap at all. pub fn ensure_swap(cfg: &Config) -> anyhow::Result<()> { if !cfg.swap.enabled { - return Ok(()); + return remove_managed_swap_state(); } let _lock = FileLock::acquire_timeout("/run/awg2-harness/swap.lock", Duration::from_secs(30))?; @@ -106,6 +148,19 @@ pub fn ensure_swap(cfg: &Config) -> anyhow::Result<()> { Ok(()) } +/// Undoes the persistent part of the module when `swap.enabled = false`. +/// +/// Only the managed file is removed. The runtime `vm.swappiness` is deliberately left as +/// it is: forcing an arbitrary value would be inventing a policy the operator never asked +/// for. The kernel default applies again after reboot, and `status` reports that the value +/// is no longer managed. Active swap is not disabled either - turning it off would be a +/// destructive action triggered by merely disabling a feature. +fn remove_managed_swap_state() -> anyhow::Result<()> { + migrate_legacy_swappiness_file()?; + swappiness_file().remove()?; + Ok(()) +} + /// Lexical validation of `swap.path`, with no filesystem access. /// /// Kept side-effect free so `validate-config` can run it on a host where the file does not @@ -544,12 +599,8 @@ fn apply_swappiness(v: u8) -> anyhow::Result<()> { anyhow::bail!("failed to apply vm.swappiness: got {current}, expected {v}"); } - let conf_path = "/etc/sysctl.d/99-awg2-harness.conf"; - let expected = format!("# Managed by awg2-harness\nvm.swappiness = {v}\n"); - let current_file = fs::read_to_string(conf_path).unwrap_or_default(); - if current_file != expected { - fs::write(conf_path, expected)?; - } + migrate_legacy_swappiness_file()?; + swappiness_file().write(&render_swappiness_conf(v))?; let persisted = read_persisted_swappiness()?; if persisted != Some(v) { @@ -575,10 +626,8 @@ pub fn read_current_swappiness() -> anyhow::Result { } pub fn read_persisted_swappiness() -> anyhow::Result> { - let conf_path = "/etc/sysctl.d/99-awg2-harness.conf"; - let content = match fs::read_to_string(conf_path) { - Ok(v) => v, - Err(_) => return Ok(None), + let Some(content) = swappiness_file().read()? else { + return Ok(None); }; for line in content.lines() { let t = line.trim(); @@ -959,10 +1008,60 @@ mod tests { #[test] fn swappiness_conf_render_is_stable() { - let expected = "# Managed by awg2-harness\nvm.swappiness = 1\n"; + let rendered = render_swappiness_conf(1); + assert!(rendered.starts_with(SWAPPINESS_MARKER)); + assert!(rendered.contains("vm.swappiness = 1")); + assert!(render_swappiness_conf(60).contains("vm.swappiness = 60")); + } + + /// The rendered file must be recognised as ours by the same marker check that guards + /// overwriting it, otherwise every reconcile would refuse to touch its own output. + #[test] + fn rendered_swappiness_conf_is_recognised_as_managed() { + let dir = scratch("swappiness_marker"); + let f = ManagedFile::new(dir.join("90-awg2-harness-swap.conf"), SWAPPINESS_MARKER); + assert!(f.write(&render_swappiness_conf(1)).expect("write")); assert_eq!( - expected, - format!("# Managed by awg2-harness\nvm.swappiness = {}\n", 1) + f.ownership().expect("ownership"), + crate::managed_file::Ownership::Managed + ); + assert!(!f.write(&render_swappiness_conf(1)).expect("idempotent")); + } + + #[test] + fn legacy_swappiness_file_is_removed_when_it_carries_the_old_marker() { + let dir = scratch("legacy_ours"); + let path = dir.join("99-awg2-harness.conf"); + fs::write( + &path, + format!("{LEGACY_SWAPPINESS_MARKER}\nvm.swappiness = 1\n"), + ) + .expect("seed legacy"); + + let legacy = ManagedFile::new(path.clone(), LEGACY_SWAPPINESS_MARKER); + drop_legacy_managed_file(&legacy).expect("migrate"); + assert!(!path.exists()); + } + + /// An administrator's file that happens to sit at the old path is not ours to delete. + #[test] + fn legacy_migration_leaves_a_foreign_file_alone() { + let dir = scratch("legacy_foreign"); + let path = dir.join("99-awg2-harness.conf"); + fs::write(&path, "# hand written\nvm.swappiness = 30\n").expect("seed foreign"); + + let legacy = ManagedFile::new(path.clone(), LEGACY_SWAPPINESS_MARKER); + drop_legacy_managed_file(&legacy).expect("must not fail"); + assert_eq!( + fs::read_to_string(&path).expect("intact"), + "# hand written\nvm.swappiness = 30\n" ); } + + #[test] + fn legacy_migration_is_a_no_op_when_nothing_is_there() { + let dir = scratch("legacy_absent"); + let legacy = ManagedFile::new(dir.join("99-awg2-harness.conf"), LEGACY_SWAPPINESS_MARKER); + assert!(drop_legacy_managed_file(&legacy).is_ok()); + } } diff --git a/traffic_core/src/sysctl_tuning.rs b/traffic_core/src/sysctl_tuning.rs index aba596e..4461373 100644 --- a/traffic_core/src/sysctl_tuning.rs +++ b/traffic_core/src/sysctl_tuning.rs @@ -1,9 +1,6 @@ -use std::{collections::BTreeMap, fs, io::Write, path::Path, process::Command, time::Duration}; +use std::{collections::BTreeMap, fs, path::Path, process::Command, time::Duration}; -#[cfg(unix)] -use std::os::unix::fs::PermissionsExt; - -use crate::{config::Config, lock::FileLock}; +use crate::{config::Config, lock::FileLock, managed_file::ManagedFile}; const SYSCTL_CONF_PATH: &str = "/etc/sysctl.d/90-awg2-harness-vpn.conf"; const BBR_MODULES_LOAD_PATH: &str = "/etc/modules-load.d/90-awg2-harness-bbr.conf"; @@ -66,8 +63,7 @@ pub fn ensure_sysctl_tuning(cfg: &Config) -> anyhow::Result<()> { reconcile_modules_load_file(&plan)?; - ensure_managed_or_absent(SYSCTL_CONF_PATH)?; - write_atomic(SYSCTL_CONF_PATH, &plan.persisted)?; + managed(Path::new(SYSCTL_CONF_PATH)).write(&plan.persisted)?; if let Some(v) = plan.desired.get("net.ipv4.ip_forward") { let current = read_sysctl("net.ipv4.ip_forward").ok(); @@ -288,45 +284,23 @@ pub fn remove_managed_sysctl_files_if_disabled(cfg: &Config) -> anyhow::Result<( } fn reconcile_modules_load_file(plan: &RenderPlan) -> anyhow::Result<()> { + let file = managed(Path::new(BBR_MODULES_LOAD_PATH)); if let Some(content) = &plan.modules_load_persisted { - ensure_managed_or_absent(BBR_MODULES_LOAD_PATH)?; - write_atomic(BBR_MODULES_LOAD_PATH, content)?; + file.write(content)?; } else { - remove_managed_sysctl_file_at(Path::new(BBR_MODULES_LOAD_PATH))?; + file.remove()?; } Ok(()) } fn remove_managed_sysctl_file_at(path: &Path) -> anyhow::Result<()> { - if !path.exists() { - return Ok(()); - } - let raw = fs::read_to_string(path)?; - let first = raw.lines().next().unwrap_or(""); - if !first.contains(MANAGED_MARKER) { - anyhow::bail!( - "refusing to remove unmanaged sysctl file: {}", - path.display() - ); - } - fs::remove_file(path)?; + managed(path).remove()?; Ok(()) } -fn ensure_managed_or_absent(path: &str) -> anyhow::Result<()> { - let p = Path::new(path); - if !p.exists() { - return Ok(()); - } - - let raw = fs::read_to_string(p)?; - let first = raw.lines().next().unwrap_or(""); - - if !first.contains(MANAGED_MARKER) { - anyhow::bail!("refusing to overwrite unmanaged sysctl file: {path}"); - } - - Ok(()) +/// Ownership rules live in [`crate::managed_file`], shared with the swap module. +fn managed(path: &Path) -> ManagedFile { + ManagedFile::new(path, MANAGED_MARKER) } fn build_render_plan(cfg: &Config, allow_modprobe: bool) -> anyhow::Result { @@ -472,35 +446,6 @@ fn normalize(v: &str) -> String { v.lines().map(str::trim_end).collect::>().join("\n") } -fn write_atomic(path: &str, content: &str) -> anyhow::Result<()> { - #[cfg(unix)] - let p = Path::new(path); - #[cfg(unix)] - let dir = p - .parent() - .ok_or_else(|| anyhow::anyhow!("no parent directory for path: {path}"))?; - let tmp_path = format!("{path}.tmp"); - - let mut f = fs::File::create(&tmp_path)?; - - #[cfg(unix)] - fs::set_permissions(&tmp_path, fs::Permissions::from_mode(0o644))?; - - f.write_all(content.as_bytes())?; - f.sync_all()?; - drop(f); - - fs::rename(&tmp_path, path)?; - - #[cfg(unix)] - { - let d = fs::File::open(dir)?; - d.sync_all()?; - } - - Ok(()) -} - #[cfg(test)] mod tests { use super::*; @@ -610,9 +555,14 @@ mod tests { )); fs::write(&path, "# manual file\nnet.core.wmem_max = 1\n").expect("write unmanaged"); - let path_str = path.to_string_lossy().to_string(); - let e = ensure_managed_or_absent(&path_str); - assert!(e.is_err()); + let err = managed(&path) + .write(&format!("{MANAGED_MARKER}\nnet.core.wmem_max = 2\n")) + .expect_err("must refuse"); + assert!(err.to_string().contains("refusing to overwrite unmanaged")); + assert_eq!( + fs::read_to_string(&path).expect("intact"), + "# manual file\nnet.core.wmem_max = 1\n" + ); let _ = fs::remove_file(&path); } @@ -623,25 +573,13 @@ mod tests { "awg2_harness_absent_test_{}.conf", std::process::id() )); - if absent.exists() { - let _ = fs::remove_file(&absent); - } - let absent_str = absent.to_string_lossy().to_string(); - ensure_managed_or_absent(&absent_str).expect("allow absent"); + let _ = fs::remove_file(&absent); + let content = format!("{MANAGED_MARKER}\nnet.core.rmem_max = 1\n"); + assert!(managed(&absent).write(&content).expect("allow absent")); + // Same content again is a no-op rather than a rewrite. + assert!(!managed(&absent).write(&content).expect("allow managed")); - let managed = std::env::temp_dir().join(format!( - "awg2_harness_managed_overwrite_test_{}.conf", - std::process::id() - )); - fs::write( - &managed, - format!("{MANAGED_MARKER}\nnet.core.rmem_max = 1\n"), - ) - .expect("write managed"); - let managed_str = managed.to_string_lossy().to_string(); - ensure_managed_or_absent(&managed_str).expect("allow managed"); - - let _ = fs::remove_file(&managed); + let _ = fs::remove_file(&absent); } #[test]