From f1997d7f78008e083d2e4d6e3f98d8d3f6fffcf8 Mon Sep 17 00:00:00 2001 From: Martti Malmi Date: Tue, 29 Sep 2026 11:31:13 +0300 Subject: [PATCH] fix(gateway): retry pending firewall updates A failed NAT rebuild retained the requested change but waited for another mapping event before trying again. Track pending rebuilds and retry the latest desired state every ten seconds, clearing pending work only after success. Cover encoding failure, kernel rejection, superseding changes, and recovery without another mapping event. --- CHANGELOG.md | 6 ++ docs/design/fips-gateway.md | 6 +- src/bin/fips-gateway.rs | 9 +++ src/gateway/nat.rs | 139 +++++++++++++++++++++++++++++++++++- 4 files changed, 155 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5866b61e..8444ac8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +#### Gateway + +- The gateway retries failed firewall rebuilds every ten seconds without + waiting for another mapping change. Retries apply the latest desired + mappings and port forwards, preserving changes across transient failures. + #### Linux packages - The systemd tarball's `install.sh` restarts fips-dns and fips-gateway after diff --git a/docs/design/fips-gateway.md b/docs/design/fips-gateway.md index d3530135..34d15f70 100644 --- a/docs/design/fips-gateway.md +++ b/docs/design/fips-gateway.md @@ -458,8 +458,10 @@ sequence is: batch, and a batch too large for any send buffer is refused before it is sent. A batch the kernel refuses leaves the previous table in place; the failure is logged, and the gateway keeps its - record of the change, so the next rebuild that succeeds applies - it. + record of the change. Pending changes are retried every ten seconds, + even when no new mapping event arrives. Each retry rebuilds the latest + desired state, so a newer change supersedes an earlier failed one. + Successful rebuilds clear the pending flag; clean tables are not retried. The rustables crate does not expose rule-handle tracking, so incremental update of individual rules is not available. Atomic diff --git a/src/bin/fips-gateway.rs b/src/bin/fips-gateway.rs index 3d2b784d..60173841 100644 --- a/src/bin/fips-gateway.rs +++ b/src/bin/fips-gateway.rs @@ -545,8 +545,17 @@ async fn main() { info!("fips-gateway running"); let mut exit_code = 0; + let mut nat_retry = tokio::time::interval(std::time::Duration::from_secs(10)); + nat_retry.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); loop { tokio::select! { + _ = nat_retry.tick() => { + match nat_mgr.retry_pending() { + Ok(true) => info!("Applied pending NAT rules"), + Ok(false) => {}, + Err(e) => warn!(error = %e, "Failed to retry pending NAT rules"), + } + } Some(event) = event_rx.recv() => { match event { pool::PoolEvent::MappingCreated { virtual_ip, mesh_addr } => { diff --git a/src/gateway/nat.rs b/src/gateway/nat.rs index 7a0d2770..e7073c27 100644 --- a/src/gateway/nat.rs +++ b/src/gateway/nat.rs @@ -174,6 +174,8 @@ pub struct NatManager { mappings: HashMap, /// Inbound port-forward rules. port_forwards: Vec, + /// Desired state has not yet been acknowledged by the kernel. + rebuild_pending: bool, } impl NatManager { @@ -199,6 +201,7 @@ impl NatManager { lan_interface, mappings: HashMap::new(), port_forwards: Vec::new(), + rebuild_pending: false, } } @@ -222,7 +225,7 @@ impl NatManager { /// the nftables table atomically. Pass an empty slice to clear. pub fn set_port_forwards(&mut self, forwards: &[PortForward]) -> Result<(), NatError> { self.port_forwards = forwards.to_vec(); - self.rebuild()?; + self.rebuild_desired()?; info!( count = self.port_forwards.len(), "Applied inbound port forwards" @@ -307,6 +310,18 @@ impl NatManager { self.mappings.len() } + /// Retry a failed rebuild using the latest desired state. + /// + /// Returns whether a pending rebuild was applied. A clean manager does + /// not open a socket or rebuild the table. + pub fn retry_pending(&mut self) -> Result { + if !self.rebuild_pending { + return Ok(false); + } + self.rebuild_desired()?; + Ok(true) + } + /// The objects a rebuild sends, grouped into the batches that carry them. /// /// One batch, always. The kernel applies a batch as a single transaction, @@ -497,12 +512,19 @@ impl NatManager { /// Rebuild, returning the outcome with the time the rebuild took in /// microseconds, so a mapping change can log its cost on either path. - fn timed_rebuild(&self) -> (Result<(), NatError>, u64) { + fn timed_rebuild(&mut self) -> (Result<(), NatError>, u64) { let started = Instant::now(); - let result = self.rebuild(); + let result = self.rebuild_desired(); let elapsed_us = u64::try_from(started.elapsed().as_micros()).unwrap_or(u64::MAX); (result, elapsed_us) } + + fn rebuild_desired(&mut self) -> Result<(), NatError> { + self.rebuild_pending = true; + self.rebuild()?; + self.rebuild_pending = false; + Ok(()) + } } /// Largest batch, in bytes, that the kernel can admit in one send. @@ -883,6 +905,117 @@ mod tests { mgr } + #[test] + fn failed_rebuild_retains_latest_desired_state_for_retry() { + let mut mgr = manager_with_mappings(2); + // Make encoding fail before opening a socket. + mgr.pre_chain = Chain::new(&mgr.table); + assert!(!mgr.retry_pending().unwrap(), "clean retry must not encode"); + + assert!(mgr.add_mapping(vip(3), mesh(3)).is_err()); + assert!(mgr.rebuild_pending); + assert!(mgr.retry_pending().is_err()); + assert!(mgr.rebuild_pending); + assert!(mgr.remove_mapping(vip(3)).is_err()); + assert!(!mgr.mappings.contains_key(&vip(3))); + assert!(mgr.add_mapping(vip(2), mesh(99)).is_err()); + assert_eq!(mgr.mappings[&vip(2)].mesh_addr, mesh(99)); + assert!(mgr.remove_mapping(vip(4)).is_err()); + assert!( + mgr.rebuild_pending, + "an absent mapping must not clear pending work" + ); + } + + #[test] + fn failed_port_forward_rebuild_remains_pending() { + let mut mgr = manager_with_mappings(1); + mgr.pre_chain = Chain::new(&mgr.table); + let forward = PortForward { + proto: Proto::Tcp, + listen_port: 8080, + target: SocketAddrV6::new(Ipv6Addr::LOCALHOST, 80, 0, 0), + }; + assert!(mgr.set_port_forwards(&[forward]).is_err()); + assert_eq!(mgr.port_forwards.len(), 1); + assert!(mgr.rebuild_pending); + assert!(mgr.set_port_forwards(&[]).is_err()); + assert!(mgr.port_forwards.is_empty()); + assert!(mgr.rebuild_pending); + } + + #[test] + #[ignore = "requires CAP_NET_ADMIN and nft in an isolated network namespace"] + fn kernel_rejection_retries_latest_state_without_another_mapping_event() { + let mut mgr = manager_with_mappings(2); + let table_name = format!("{TABLE_NAME}_retry_test_{}", std::process::id()); + mgr.table = Table::new(ProtocolFamily::Inet).with_name(&table_name); + mgr.pre_chain = Chain::new(&mgr.table) + .with_name(PREROUTING_CHAIN) + .with_type(ChainType::Nat) + .with_hook(Hook::new(HookClass::PreRouting, DSTNAT_PRIORITY)); + mgr.post_chain = Chain::new(&mgr.table) + .with_name(POSTROUTING_CHAIN) + .with_type(ChainType::Nat) + .with_hook(Hook::new(HookClass::PostRouting, SRCNAT_PRIORITY)); + mgr.rebuild().unwrap(); + let listing = || { + let output = std::process::Command::new("nft") + .args(["-j", "list", "table", "inet", &table_name]) + .output() + .unwrap(); + assert!(output.status.success()); + output.stdout + }; + let before = listing(); + // Encoding succeeds, but the kernel rejects an overlong chain name. + let invalid = Chain::new(&mgr.table).with_name("x".repeat(300)); + let original = std::mem::replace(&mut mgr.pre_chain, invalid); + assert!(matches!( + mgr.add_mapping(vip(3), mesh(3)), + Err(NatError::Kernel { .. }) + )); + assert!(mgr.remove_mapping(vip(1)).is_err()); + assert!(mgr.remove_mapping(vip(3)).is_err()); + assert!(mgr.add_mapping(vip(2), mesh(99)).is_err()); + assert!(mgr.retry_pending().is_err()); + assert_eq!( + listing(), + before, + "rejection leaves the old kernel table intact" + ); + + mgr.pre_chain = original; + assert!(mgr.retry_pending().unwrap()); + assert!(!mgr.retry_pending().unwrap()); + assert_eq!(mgr.mapping_count(), 1); + assert_eq!(mgr.mappings[&vip(2)].mesh_addr, mesh(99)); + let after = listing(); + assert_ne!(after, before); + let table: serde_json::Value = serde_json::from_slice(&after).unwrap(); + assert_eq!( + table["nftables"] + .as_array() + .unwrap() + .iter() + .filter(|entry| entry.get("rule").is_some()) + .count(), + 3 + ); + assert!(String::from_utf8(after).unwrap().contains("fd02::63")); + + // A successful normal update also clears earlier pending work. + mgr.pre_chain = Chain::new(&mgr.table); + assert!(mgr.add_mapping(vip(4), mesh(4)).is_err()); + mgr.pre_chain = Chain::new(&mgr.table) + .with_name(PREROUTING_CHAIN) + .with_type(ChainType::Nat) + .with_hook(Hook::new(HookClass::PreRouting, DSTNAT_PRIORITY)); + mgr.remove_mapping(vip(4)).unwrap(); + assert!(!mgr.retry_pending().unwrap()); + mgr.cleanup().unwrap(); + } + #[test] fn rebuild_deletes_and_recreates_the_table_inside_one_batch() { let batches = manager_with_mappings(3).rebuild_batches();