From 95db98b7d867a567166816e4043a865c39e1c4bb Mon Sep 17 00:00:00 2001 From: GyulyVGC Date: Fri, 7 Aug 2026 14:00:19 +0200 Subject: [PATCH] client: edit a container's /etc/hosts through its host-side file instead of docker exec --- members/nullnet-client/src/control_channel.rs | 90 +++------- members/nullnet-client/src/host_mappings.rs | 154 ++++++++++++++---- .../src/services/service_info.rs | 6 + 3 files changed, 146 insertions(+), 104 deletions(-) diff --git a/members/nullnet-client/src/control_channel.rs b/members/nullnet-client/src/control_channel.rs index 4d080e2..c203fce 100644 --- a/members/nullnet-client/src/control_channel.rs +++ b/members/nullnet-client/src/control_channel.rs @@ -2,7 +2,9 @@ use crate::commands::{RtNetLinkHandle, configure_access_port, dnat, egress, remo use crate::ebpf::{FirewallPeers, FirewallVxlanPorts, NetId}; use crate::egress_policy::{PolicyVerdicts, flush_container_conntrack}; use crate::egress_state::{EgressRecord, EgressState}; -use crate::host_mappings::{HOSTS_MARKER, HostMappingsState, hosts_file_lock}; +use crate::host_mappings::{ + HOSTS_MARKER, HostMappingsState, edit_container_hosts, hosts_file_lock, +}; use crate::nfqueue::BridgeIpCache; use crate::peers::peer::{Peers, VethKey}; use crate::triggers::TriggersState; @@ -899,41 +901,16 @@ fn add_host_mapping(hm: &HostMapping, docker_container: Option<&str>) -> Result< if let Some(container) = docker_container { // container-targeted: the resolver that needs this name lives inside // the container, so write only there and leave the host's file alone. - let cat = std::process::Command::new("docker") - .args(["exec", container, "cat", path]) - .output() - .handle_err(location!())?; - // Bail rather than write on a failed read: `output()` is Ok even when - // the exec itself failed (container gone, docker hiccup), and treating - // the empty stdout as the file's contents would truncate a live - // `/etc/hosts` down to this one entry. - if !cat.status.success() { - Err(format!( - "reading {path} in '{container}' failed: {}", - String::from_utf8_lossy(&cat.stderr).trim() - )) - .handle_err(location!())?; - } - let content = upsert_hosts_entry(&String::from_utf8_lossy(&cat.stdout), &hm.name, &entry); - let mut child = std::process::Command::new("docker") - .args([ - "exec", - "-i", - container, - "sh", - "-c", - &format!("cat > {path}"), - ]) - .stdin(std::process::Stdio::piped()) - .spawn() - .handle_err(location!())?; - if let Some(mut stdin) = child.stdin.take() { - use std::io::Write; - stdin - .write_all(content.as_bytes()) - .handle_err(location!())?; - } - let _ = child.wait(); + // Edited from the host side (see `read_container_hosts`) so a paused + // container is served just as well as a running one. + // + // Bail rather than write on a failed read: treating a missing file as + // empty content would truncate a live `/etc/hosts` down to this one + // entry. + edit_container_hosts(container, |current| { + upsert_hosts_entry(current, &hm.name, &entry) + }) + .handle_err(location!())?; } else { // host-targeted: upsert into the host's /etc/hosts let content = std::fs::read_to_string(path).handle_err(location!())?; @@ -967,38 +944,15 @@ fn remove_host_mapping(hm: &HostMapping, docker_container: Option<&str>) -> Resu if let Some(container) = docker_container { // container-targeted: setup only wrote inside the container, so the - // matching removal is container-only too. - let cat = std::process::Command::new("docker") - .args(["exec", container, "cat", path]) - .output() - .handle_err(location!())?; - if !cat.status.success() { - Err(format!( - "reading {path} in '{container}' failed: {}", - String::from_utf8_lossy(&cat.stderr).trim() - )) - .handle_err(location!())?; - } - let content = remove_hosts_entry(&String::from_utf8_lossy(&cat.stdout), &hm.name, &hm.ip); - let mut child = std::process::Command::new("docker") - .args([ - "exec", - "-i", - container, - "sh", - "-c", - &format!("cat > {path}"), - ]) - .stdin(std::process::Stdio::piped()) - .spawn() - .handle_err(location!())?; - if let Some(mut stdin) = child.stdin.take() { - use std::io::Write; - stdin - .write_all(content.as_bytes()) - .handle_err(location!())?; - } - let _ = child.wait(); + // matching removal is container-only too. Host-side, so this still + // works when the container has already been paused — the teardown and + // the `docker pause` that follows it race, and losing that race used to + // strand the entry, leaving the name pointing at a dead overlay IP for + // as long as the container kept being resumed rather than restarted. + edit_container_hosts(container, |current| { + remove_hosts_entry(current, &hm.name, &hm.ip) + }) + .handle_err(location!())?; } else { // host-targeted: drop this net's line from the host file let content = std::fs::read_to_string(path).handle_err(location!())?; diff --git a/members/nullnet-client/src/host_mappings.rs b/members/nullnet-client/src/host_mappings.rs index 7c9e909..dab57bc 100644 --- a/members/nullnet-client/src/host_mappings.rs +++ b/members/nullnet-client/src/host_mappings.rs @@ -1,5 +1,6 @@ use nullnet_grpc_lib::nullnet_grpc::HostMapping; use std::collections::HashMap; +use std::path::{Path, PathBuf}; use std::process::Command; use std::sync::{Arc, LazyLock, Mutex, PoisonError}; @@ -93,14 +94,10 @@ pub fn purge_stale_mappings() { } drop(guard); - for container in running_containers() { + for container in all_containers() { let lock = hosts_file_lock(Some(&container)); let _guard = lock.lock().unwrap_or_else(PoisonError::into_inner); - let Some(content) = container_hosts(&container) else { - continue; - }; - let cleaned = strip_marked(&content); - if cleaned != content && write_container_hosts(&container, &cleaned) { + if let Ok(true) = edit_container_hosts(&container, strip_marked) { swept += 1; } } @@ -117,9 +114,14 @@ fn strip_marked(content: &str) -> String { kept.join("\n") + "\n" } -fn running_containers() -> Vec { +/// Every container docker knows about, running or not. +/// +/// `-a` rather than `-q` alone: a paused container is listed either way, but a +/// stopped one only with `-a`, and both are reachable now that we edit the +/// host-side file instead of exec'ing into the container. +fn all_containers() -> Vec { let Ok(out) = Command::new("docker") - .args(["ps", "-q", "--no-trunc"]) + .args(["ps", "-aq", "--no-trunc"]) .output() else { return vec![]; // no docker on this node @@ -135,40 +137,120 @@ fn running_containers() -> Vec { .collect() } -/// `None` when the read failed — never an empty string, which would truncate -/// the file on write-back (the same trap `add_host_mapping` guards against). -fn container_hosts(container: &str) -> Option { +/// Host-side path of the file docker bind-mounts at the container's +/// `/etc/hosts`, straight from docker rather than assembled from a hardcoded +/// data-root (which a custom `data-root` or a snap install would break). +/// +/// `docker inspect` answers for paused and stopped containers alike — the whole +/// point of going through the file: `docker exec` refuses on a paused container +/// ("is paused, unpause the container before exec"), so the exec-based version +/// of this silently skipped exactly the containers whose entries most needed +/// removing. +/// +/// `None` when docker has no path for it (a container sharing the host's +/// network namespace has no bind-mounted hosts file at all). +fn container_hosts_path(container: &str) -> Option { let out = Command::new("docker") - .args(["exec", container, "cat", HOSTS_PATH]) + .args(["inspect", "-f", "{{.HostsPath}}", container]) .output() .ok()?; - out.status - .success() - .then(|| String::from_utf8_lossy(&out.stdout).into_owned()) + if !out.status.success() { + return None; + } + let path = String::from_utf8_lossy(&out.stdout).trim().to_string(); + (!path.is_empty()).then(|| PathBuf::from(path)) } -fn write_container_hosts(container: &str, content: &str) -> bool { - use std::io::Write; - let Ok(mut child) = Command::new("docker") - .args([ - "exec", - "-i", - container, - "sh", - "-c", - &format!("cat > {HOSTS_PATH}"), - ]) - .stdin(std::process::Stdio::piped()) - .spawn() - else { - return false; - }; - if let Some(mut stdin) = child.stdin.take() - && stdin.write_all(content.as_bytes()).is_err() - { - return false; +/// Read-modify-write a container's hosts file from the host side, reporting +/// whether the contents actually changed. +/// +/// One resolved path for both halves, so a container replaced under the same +/// name mid-operation cannot have us read one file and write another. A failed +/// read is an error rather than empty contents — treating a missing file as +/// empty would truncate a live `/etc/hosts` down to whatever `edit` returns. +/// +/// The write is in place, deliberately: the container sees this file through a +/// bind mount on the *inode*, so the usual write-temp-then-rename would leave +/// it looking at the old, now-detached file with every change silently +/// invisible. +pub fn edit_container_hosts( + container: &str, + edit: impl FnOnce(&str) -> String, +) -> Result { + let path = container_hosts_path(container) + .ok_or_else(|| format!("no hosts file for container '{container}'"))?; + edit_hosts_file(&path, edit) +} + +/// The file half of [`edit_container_hosts`], split out so the read-modify-write +/// — and in particular its refusal to write anything when the read failed — is +/// testable without a running docker. +fn edit_hosts_file(path: &Path, edit: impl FnOnce(&str) -> String) -> Result { + let current = std::fs::read_to_string(path) + .map_err(|e| format!("reading {} failed: {e}", path.display()))?; + let updated = edit(¤t); + if updated == current { + return Ok(false); + } + std::fs::write(path, &updated) + .map_err(|e| format!("writing {} failed: {e}", path.display()))?; + Ok(true) +} + +#[cfg(test)] +mod edit_tests { + use super::{HOSTS_MARKER, edit_hosts_file, strip_marked}; + use std::path::PathBuf; + + fn temp_file(name: &str, content: &str) -> PathBuf { + let path = std::env::temp_dir().join(format!("nullnet-hosts-test-{name}")); + std::fs::write(&path, content).unwrap(); + path + } + + #[test] + fn reports_no_change_and_leaves_the_file_alone() { + let original = "127.0.0.1 localhost\n"; + let path = temp_file("noop", original); + + assert_eq!(edit_hosts_file(&path, ToString::to_string), Ok(false)); + assert_eq!(std::fs::read_to_string(&path).unwrap(), original); + let _ = std::fs::remove_file(&path); + } + + #[test] + fn writes_in_place_and_reports_the_change() { + let path = temp_file( + "change", + &format!("127.0.0.1 localhost\n10.0.0.2 api {HOSTS_MARKER}\n"), + ); + // Same inode before and after: the container sees this file through a + // bind mount, so replacing it would detach their view. + let before = std::fs::metadata(&path).unwrap(); + + assert_eq!(edit_hosts_file(&path, strip_marked), Ok(true)); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + "127.0.0.1 localhost\n" + ); + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + assert_eq!(before.ino(), std::fs::metadata(&path).unwrap().ino()); + } + let _ = std::fs::remove_file(&path); + } + + /// A missing file must be an error, never "empty contents" — treating it as + /// empty would write the edit of nothing over a live `/etc/hosts`. + #[test] + fn a_failed_read_writes_nothing() { + let path = std::env::temp_dir().join("nullnet-hosts-test-absent"); + let _ = std::fs::remove_file(&path); + + assert!(edit_hosts_file(&path, |_| "clobbered\n".to_string()).is_err()); + assert!(!path.exists(), "must not create the file it failed to read"); } - child.wait().map(|s| s.success()).unwrap_or(false) } #[cfg(test)] diff --git a/members/nullnet-server/src/services/service_info.rs b/members/nullnet-server/src/services/service_info.rs index 501925c..4465acc 100644 --- a/members/nullnet-server/src/services/service_info.rs +++ b/members/nullnet-server/src/services/service_info.rs @@ -491,6 +491,12 @@ impl RegisteredServiceInfo { ) .await; // Invariant: a non-pinned Docker-backed replica with no clients is paused. + // Sent immediately, not deferred behind the teardown ack: the flag flips + // here, and `replica_suspended` gates the resume, so any gap between flag + // and pause is a window where an arriving request "resumes" a container + // that was never paused and the late pause then freezes a live client out. + // Ordering the pause is unnecessary anyway — every teardown step works on + // a paused container (hosts edits go through the host-side file). replica.reconcile_suspend(orchestrator, pinned).await; } return;