diff --git a/src/node/acl.rs b/src/node/acl.rs index 3d56274..c42bd5f 100644 --- a/src/node/acl.rs +++ b/src/node/acl.rs @@ -9,6 +9,7 @@ //! evaluated first, an allowlist match overrides a denylist match for the //! same peer. +use crate::node::reloadable::Reloadable; use crate::node::{Node, NodeError}; use crate::transport::{TransportAddr, TransportId}; use crate::upper::hosts::{DEFAULT_HOSTS_PATH, HostMap, HostMapReloader, file_mtime}; @@ -17,6 +18,7 @@ use serde::Serialize; use std::collections::{BTreeSet, HashSet}; use std::fmt; use std::path::{Path, PathBuf}; +use std::sync::Arc; use std::time::SystemTime; use tracing::{debug, info, warn}; @@ -283,8 +285,16 @@ impl PeerAcl { } /// Tracks peer ACL files and reloads them on mtime changes. +/// +/// Follows the canonical Arc-wrapper template from [`Reloadable`]: the +/// reader-facing [`PeerAcl`] snapshot is published through an +/// [`arc_swap::ArcSwap`] so the authorization hot path reads it without +/// locking, while the reloader's change-detection state (file mtimes, the +/// embedded hosts reloader) is touched only by [`Reloadable::reload`] on the +/// single node tick task. pub struct PeerAclReloader { - acl: PeerAcl, + /// Reader-facing effective ACL snapshot. + acl: arc_swap::ArcSwap, hosts: HostMapReloader, allow_path: PathBuf, deny_path: PathBuf, @@ -328,7 +338,7 @@ impl PeerAclReloader { let acl = PeerAcl::load_files_with_hosts(&allow_path, &deny_path, hosts.hosts()); Self { - acl, + acl: arc_swap::ArcSwap::from(Arc::new(acl)), hosts, allow_path, deny_path, @@ -337,30 +347,34 @@ impl PeerAclReloader { } } - /// Get the current ACL. - pub fn acl(&self) -> &PeerAcl { - &self.acl + /// Acquire a lock-free guard over the current ACL snapshot. + pub fn acl(&self) -> arc_swap::Guard> { + self.load() } /// Return a human-readable snapshot of the loaded ACL state. pub fn status(&self) -> PeerAclStatus { + let acl = self.acl.load(); PeerAclStatus { allow_file: self.allow_path.display().to_string(), deny_file: self.deny_path.display().to_string(), - enforcement_active: !self.acl.is_empty(), - effective_mode: self.acl.effective_mode().to_string(), - default_decision: self.acl.default_decision().to_string(), - allow_all: self.acl.allow_all, - deny_all: self.acl.deny_all, - allow_file_entries: self.acl.allow_file_entries(), - deny_file_entries: self.acl.deny_file_entries(), - allow_entries: self.acl.allow_entries(), - deny_entries: self.acl.deny_entries(), + enforcement_active: !acl.is_empty(), + effective_mode: acl.effective_mode().to_string(), + default_decision: acl.default_decision().to_string(), + allow_all: acl.allow_all, + deny_all: acl.deny_all, + allow_file_entries: acl.allow_file_entries(), + deny_file_entries: acl.deny_file_entries(), + allow_entries: acl.allow_entries(), + deny_entries: acl.deny_entries(), } } +} - /// Check whether ACL or hosts alias sources changed and reload if needed. - pub fn check_reload(&mut self) -> bool { +impl Reloadable for PeerAclReloader { + type Snapshot = PeerAcl; + + async fn reload(&mut self) -> bool { let allow_mtime = file_mtime(&self.allow_path); let deny_mtime = file_mtime(&self.deny_path); let hosts_changed = self.hosts.check_reload(); @@ -374,27 +388,32 @@ impl PeerAclReloader { self.last_allow_mtime = allow_mtime; self.last_deny_mtime = deny_mtime; - self.acl = + let new_acl = PeerAcl::load_files_with_hosts(&self.allow_path, &self.deny_path, self.hosts.hosts()); info!( allow_file = %self.allow_path.display(), deny_file = %self.deny_path.display(), - allow_entries = self.acl.allow.len(), - deny_entries = self.acl.deny.len(), + allow_entries = new_acl.allow.len(), + deny_entries = new_acl.deny.len(), alias_entries = self.hosts.hosts().len(), - allow_all = self.acl.allow_all, - deny_all = self.acl.deny_all, + allow_all = new_acl.allow_all, + deny_all = new_acl.deny_all, "Reloaded peer ACL files" ); + self.acl.store(Arc::new(new_acl)); true } + + fn load(&self) -> arc_swap::Guard> { + self.acl.load() + } } impl Node { /// Reload the peer ACL if the ACL or hosts files changed. - pub(crate) fn reload_peer_acl(&mut self) -> bool { - self.peer_acl.check_reload() + pub(crate) async fn reload_peer_acl(&mut self) -> bool { + self.peer_acl.reload().await } /// Return a control-plane snapshot of the current peer ACL. @@ -747,20 +766,20 @@ mod tests { assert_eq!(acl.check(&test_peer(&npub)), PeerAclDecision::AllowList); } - #[test] - fn test_acl_reloader_detects_change() { + #[tokio::test] + async fn test_acl_reloader_detects_change() { let dir = tempfile::tempdir().unwrap(); let allow = dir.path().join("peers.allow"); let deny = dir.path().join("peers.deny"); let denied = test_npub(); let mut reloader = PeerAclReloader::with_paths(allow.clone(), deny.clone()); - assert!(!reloader.check_reload()); + assert!(!reloader.reload().await); std::thread::sleep(std::time::Duration::from_millis(5)); std::fs::write(&deny, format!("{denied}\n")).unwrap(); - assert!(reloader.check_reload()); + assert!(reloader.reload().await); assert_eq!( reloader .acl() @@ -769,8 +788,8 @@ mod tests { ); } - #[test] - fn test_acl_reloader_detects_allow_file_removal() { + #[tokio::test] + async fn test_acl_reloader_detects_allow_file_removal() { let dir = tempfile::tempdir().unwrap(); let allow = dir.path().join("peers.allow"); let deny = dir.path().join("peers.deny"); @@ -786,7 +805,7 @@ mod tests { std::thread::sleep(std::time::Duration::from_millis(5)); std::fs::remove_file(&allow).unwrap(); - assert!(reloader.check_reload()); + assert!(reloader.reload().await); assert!(reloader.acl().is_empty()); assert_eq!( reloader.acl().check(&test_peer(&allowed)), @@ -892,8 +911,8 @@ mod tests { assert_eq!(acl.check(&peer), PeerAclDecision::AllowList); } - #[test] - fn test_acl_reloader_detects_hosts_change_for_alias_entry() { + #[tokio::test] + async fn test_acl_reloader_detects_hosts_change_for_alias_entry() { let dir = tempfile::tempdir().unwrap(); let allow = dir.path().join("peers.allow"); let deny = dir.path().join("peers.deny"); @@ -909,7 +928,7 @@ mod tests { std::thread::sleep(std::time::Duration::from_millis(5)); std::fs::write(&hosts, format!("node-a {npub}\n")).unwrap(); - assert!(reloader.check_reload()); + assert!(reloader.reload().await); assert_eq!( reloader.acl().allow_file_entries(), vec!["node-a".to_string()] @@ -923,8 +942,8 @@ mod tests { ); } - #[test] - fn test_acl_reloader_detects_hosts_removal_for_alias_entry() { + #[tokio::test] + async fn test_acl_reloader_detects_hosts_removal_for_alias_entry() { let dir = tempfile::tempdir().unwrap(); let allow = dir.path().join("peers.allow"); let deny = dir.path().join("peers.deny"); @@ -944,7 +963,7 @@ mod tests { std::thread::sleep(std::time::Duration::from_millis(5)); std::fs::remove_file(&hosts).unwrap(); - assert!(reloader.check_reload()); + assert!(reloader.reload().await); assert!(reloader.acl().is_empty()); assert_eq!( reloader.acl().check(&test_peer(&npub)), diff --git a/src/node/handlers/rx_loop.rs b/src/node/handlers/rx_loop.rs index f6f1aa8..ac64d30 100644 --- a/src/node/handlers/rx_loop.rs +++ b/src/node/handlers/rx_loop.rs @@ -251,7 +251,15 @@ impl Node { _ = tick.tick() => { self.check_timeouts(); let now_ms = Self::now_ms(); - self.reload_peer_acl(); + self.reload_peer_acl().await; + // The host map hot-reloads on the same tick as the ACL. It + // is polled separately from `reload_peer_acl` because the + // ACL's embedded alias reloader and this snapshot are + // distinct resources; the `path_mtu_lookup` cache and the + // `nostr_discovery` subsystem are deliberately excluded + // from `Reloadable` since neither reloads from a backing + // file (see `node::reloadable`). + self.reload_host_map().await; self.poll_pending_connects().await; self.poll_nostr_discovery().await; self.resend_pending_handshakes(now_ms).await; diff --git a/src/node/mod.rs b/src/node/mod.rs index bac1f36..7f21fb4 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -1093,6 +1093,13 @@ impl Node { self.identity.npub() } + /// Reload the host map if the backing `/etc/fips/hosts` file changed. + /// + /// Returns `true` if a new snapshot was published. + pub(crate) async fn reload_host_map(&mut self) -> bool { + self.host_map.reload().await + } + /// Return a human-readable display name for a NodeAddr. /// /// Lookup order: diff --git a/src/node/reloadable.rs b/src/node/reloadable.rs index 738d613..6013110 100644 --- a/src/node/reloadable.rs +++ b/src/node/reloadable.rs @@ -30,6 +30,25 @@ //! source degrades to an empty/base snapshot plus a warning), so callers have //! nothing to handle. The return flag reports only whether the snapshot was //! replaced, which is all the tick loop needs for logging. +//! +//! # What is and isn't `Reloadable` +//! +//! Two node-owned resources are deliberately *not* `Reloadable` because +//! neither has a "re-read from a backing source" concept: +//! +//! - `path_mtu_lookup` is an event-driven cache (`Arc>`) +//! populated from observed path-MTU discovery traffic, not loaded from a +//! file. There is nothing to poll. (Its read side could adopt the same +//! lock-free `ArcSwap` shape in the future, but that is an optimization, not +//! a reload.) +//! - `nostr_discovery` is an async spawned subsystem, not a snapshot of disk +//! state. +//! +//! Both [`HostMapReloadable`] and the peer ACL reloader currently stat +//! `/etc/fips/hosts` independently each tick (the ACL reloader embeds its own +//! hosts reloader for alias resolution). A single small-file `stat` per tick +//! is cheap, so the duplicate is left in place; collapsing the two onto a +//! single shared mtime observation is a possible future cleanup. use std::sync::Arc; @@ -50,10 +69,8 @@ pub trait Reloadable: Send { /// changed. I/O errors are absorbed internally (degrading to an /// empty/base snapshot with a warning) rather than surfaced. /// - /// Not yet driven from the node tick — the host map snapshot is still - /// taken once at construction and not polled. Wiring the periodic poll - /// into the tick is a follow-up. - #[cfg_attr(not(test), allow(dead_code))] + /// Driven once per node tick from the rx loop, alongside the other + /// reloadable resources. async fn reload(&mut self) -> bool; /// Acquire a lock-free guard over the current snapshot. @@ -73,15 +90,12 @@ pub struct HostMapReloadable { /// Reader-facing effective snapshot (base merged with hosts file). snapshot: arc_swap::ArcSwap, /// Base map from peer-config aliases (never changes). Read only by - /// `reload`, which is not yet driven from the node tick. - #[cfg_attr(not(test), allow(dead_code))] + /// `reload` on the tick task. base: HostMap, /// Path to the operator hosts file. Read only by `reload`. - #[cfg_attr(not(test), allow(dead_code))] path: std::path::PathBuf, /// Last observed modification time of the hosts file (`None` if absent). /// Read only by `reload`. - #[cfg_attr(not(test), allow(dead_code))] last_mtime: Option, } diff --git a/src/node/tests/acl.rs b/src/node/tests/acl.rs index 99e89b1..9b64841 100644 --- a/src/node/tests/acl.rs +++ b/src/node/tests/acl.rs @@ -1,7 +1,9 @@ use super::*; use crate::ReceivedPacket; use crate::node::acl::PeerAclReloader; +use crate::node::reloadable::HostMapReloadable; use crate::node::wire::build_msg2; +use crate::upper::hosts::HostMap; use crate::utils::index::SessionIndex; use std::path::PathBuf; use std::time::Duration; @@ -29,7 +31,7 @@ async fn test_outbound_connect_denied_by_denylist() { let (dir, mut node) = make_acl_node(); let denied = Identity::generate(); std::fs::write(deny_path(&dir), format!("{}\n", denied.npub())).unwrap(); - node.reload_peer_acl(); + node.reload_peer_acl().await; let result = node .initiate_connection( @@ -94,7 +96,7 @@ async fn test_outbound_msg2_denied_after_acl_reload() { let wire_msg2 = build_msg2(our_index_b, our_index_a, &noise_msg2); std::fs::write(deny_path(&dir), format!("{}\n", node_b.npub())).unwrap(); - assert!(node_a.reload_peer_acl()); + assert!(node_a.reload_peer_acl().await); let packet = ReceivedPacket::with_timestamp(transport_id, remote_addr, wire_msg2, 1100); node_a.handle_msg2(packet).await; @@ -141,7 +143,7 @@ async fn test_inbound_msg3_denied_triggers_disconnect() { format!("{}\n", node_a.npub()), ) .unwrap(); - assert!(node_b.reload_peer_acl()); + assert!(node_b.reload_peer_acl().await); let transport_id_a = TransportId::new(1); let transport_id_b = TransportId::new(1); @@ -260,13 +262,39 @@ async fn test_inbound_msg3_denied_triggers_disconnect() { } } +#[tokio::test] +async fn test_host_map_hot_reloads_from_tick() { + let dir = tempfile::tempdir().unwrap(); + let hosts_path = dir.path().join("hosts"); + + let mut node = Node::new(Config::new()).unwrap(); + node.host_map = HostMapReloadable::new(HostMap::new(), hosts_path.clone()); + + let peer = Identity::generate(); + let peer_addr = *PeerIdentity::from_pubkey_full(peer.pubkey_full()).node_addr(); + + // No hosts file yet: the display name is not the alias. + assert_ne!(node.peer_display_name(&peer_addr), "gateway"); + assert!(!node.reload_host_map().await); + + // Write a hosts entry and let the tick-driven reload pick it up. + std::thread::sleep(Duration::from_millis(50)); + std::fs::write(&hosts_path, format!("gateway {}\n", peer.npub())).unwrap(); + + assert!(node.reload_host_map().await); + assert_eq!(node.peer_display_name(&peer_addr), "gateway"); + + // No further change: reload reports nothing replaced. + assert!(!node.reload_host_map().await); +} + #[tokio::test] async fn test_outbound_connect_not_denied_by_allowlist_miss() { let (dir, mut node) = make_acl_node(); let denied = Identity::generate(); let allowed = Identity::generate(); std::fs::write(allow_path(&dir), format!("{}\n", allowed.npub())).unwrap(); - node.reload_peer_acl(); + node.reload_peer_acl().await; let result = node .initiate_connection(