From 957f0422824ee89d23f3b69f0f1eb0809f4c8445 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 30 Sep 2026 13:45:33 +0000 Subject: [PATCH 01/13] Report an unreadable node log in the interop harness as not established count_log_pattern prints unreadable: and fails when a node's logs cannot be read, but only two of its callers checked that. The rest compared the text as a number: the first-rekey check reported "no FMP rekey cutover observed", the second-cycle wait aborted the run on an unbound variable before the log analysis, and the log analysis read the rekey totals as zero. Each now checks the status and reports the count as not established. --- testing/interop/interop-test.sh | 49 +++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/testing/interop/interop-test.sh b/testing/interop/interop-test.sh index 913518fe..857e4b59 100755 --- a/testing/interop/interop-test.sh +++ b/testing/interop/interop-test.sh @@ -574,14 +574,19 @@ count_node_pattern() { docker logs "${CONTAINER[$node]}" 2>&1 | grep -cE "$pattern" || true } +# Wait until the count of a pattern across all node logs reaches +# min_count. Returns 0 when it does, 1 on timeout, and 2 at once when a +# node's logs cannot be read, since no count is established then; the +# caller reports that when it takes its own count. wait_for_log_pattern_count() { local pattern="$1" min_count="$2" timeout="$3" - local start=$SECONDS - while (( SECONDS - start < timeout )); do - [ "$(count_log_pattern "$pattern")" -ge "$min_count" ] && return 0 + local start=$SECONDS count + while :; do + count="$(count_log_pattern "$pattern")" || return 2 + [ "$count" -ge "$min_count" ] && return 0 + (( SECONDS - start < timeout )) || return 1 sleep "$LOG_POLL_INTERVAL" done - [ "$(count_log_pattern "$pattern")" -ge "$min_count" ] } # One node's bloom-union mesh-size estimate, or "null" when the daemon @@ -866,8 +871,11 @@ PASSED=0; FAILED=0 wait_for_log_pattern_count \ "Rekey cutover complete \(initiator\), K-bit flipped" 1 \ "$FIRST_REKEY_TIMEOUT" || true -fmp_cutovers="$(count_log_pattern 'Rekey cutover complete \(initiator\), K-bit flipped')" -if [ "$fmp_cutovers" -ge 1 ]; then +if ! fmp_cutovers="$(count_log_pattern 'Rekey cutover complete \(initiator\), K-bit flipped')"; then + echo " FAIL FMP rekey cutovers: node logs unreadable ($fmp_cutovers), not established" + FAILED=$((FAILED + 1)) + INTEROP_FAILURES+=("[log] FMP rekey cutovers: node logs unreadable ($fmp_cutovers)") +elif [ "$fmp_cutovers" -ge 1 ]; then echo " PASS FMP rekey initiator cutovers: $fmp_cutovers" PASSED=$((PASSED + 1)) else @@ -897,10 +905,17 @@ echo "Phase 4: Second rekey cycle (waiting up to ${SECOND_REKEY_WAIT}s for the n # the same pre/post cutover-count delta convention as the control window # (Phase 1b), instead of a blind sleep. Bounded by SECOND_REKEY_WAIT so a # stalled rekey falls through to the strict Phase 5/6 assertions. -fmp_cutovers_before="$(count_log_pattern 'Rekey cutover complete \(initiator\), K-bit flipped')" -wait_for_log_pattern_count \ - "Rekey cutover complete \(initiator\), K-bit flipped" \ - "$((fmp_cutovers_before + 1))" "$SECOND_REKEY_WAIT" || true +# An unreadable baseline gives nothing to wait beyond, so do not wait. +# Phase 4 keeps no PASSED/FAILED of its own; the INTEROP_FAILURES entry +# is what turns the run red. +if fmp_cutovers_before="$(count_log_pattern 'Rekey cutover complete \(initiator\), K-bit flipped')"; then + wait_for_log_pattern_count \ + "Rekey cutover complete \(initiator\), K-bit flipped" \ + "$((fmp_cutovers_before + 1))" "$SECOND_REKEY_WAIT" || true +else + echo " FAIL second rekey cycle: node logs unreadable ($fmp_cutovers_before), baseline not established" + INTEROP_FAILURES+=("[log] second rekey cycle: node logs unreadable ($fmp_cutovers_before)") +fi echo "" echo "Phase 5: Post-second-rekey connectivity (reconverge within ${POST_REKEY_TIMEOUT}s)" @@ -1015,16 +1030,22 @@ done # Positive checks — the rekey machinery actually exercised both layers. echo "" echo " -- Rekey machinery exercised --" -fmp_total="$(count_log_pattern 'Rekey cutover complete \(initiator\), K-bit flipped')" -fsp_total="$(count_log_pattern 'FSP rekey cutover complete')" -if [ "$fmp_total" -ge 1 ]; then +# An unreadable log means the harness observed nothing, so it is a FAIL +# on either layer; the FSP WARN below is for a readable zero only. +if ! fmp_total="$(count_log_pattern 'Rekey cutover complete \(initiator\), K-bit flipped')"; then + echo " FAIL FMP rekey cutovers: node logs unreadable ($fmp_total), not established" + FAILED=$((FAILED + 1)) +elif [ "$fmp_total" -ge 1 ]; then echo " PASS FMP rekey cutovers across mesh: $fmp_total" PASSED=$((PASSED + 1)) else echo " FAIL FMP rekey cutovers: $fmp_total (expected >= 1)" FAILED=$((FAILED + 1)) fi -if [ "$fsp_total" -ge 1 ]; then +if ! fsp_total="$(count_log_pattern 'FSP rekey cutover complete')"; then + echo " FAIL FSP rekey cutovers: node logs unreadable ($fsp_total), not established" + FAILED=$((FAILED + 1)) +elif [ "$fsp_total" -ge 1 ]; then echo " PASS FSP rekey cutovers across mesh: $fsp_total" PASSED=$((PASSED + 1)) else From 0277a72b79e6cd0aa2768c2752c314ec201d8977 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 30 Sep 2026 13:46:23 +0000 Subject: [PATCH 02/13] Document that disabling rekey is unsupported and removed in v2 The configuration reference and the mesh-layer design presented node.rekey.enabled: false as an ordinary setting. It is a less-tested path the project does not support, and the option goes away in v2. The flag's behaviour is unchanged. --- docs/design/fips-mesh-layer.md | 5 +++-- docs/reference/configuration.md | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/design/fips-mesh-layer.md b/docs/design/fips-mesh-layer.md index b437fa22..eb7d5b0d 100644 --- a/docs/design/fips-mesh-layer.md +++ b/docs/design/fips-mesh-layer.md @@ -374,8 +374,9 @@ A rekey is initiated when either threshold is reached on the link's current session: `node.rekey.after_secs` (default 120) elapsed since the link came up or last rekeyed, or `node.rekey.after_messages` (default 65536) frames sent. Either side can be the initiator independently. Rekey is on by -default and can be disabled via `node.rekey.enabled: false` (the -configuration tree is documented in +default. `node.rekey.enabled: false` stops this node initiating rekey, but +that configuration is unsupported: it is a less-tested path, and the option +is removed in v2 (the configuration tree is documented in [../reference/configuration.md](../reference/configuration.md)). ### Mechanism diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 9da7f87e..05006695 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -378,7 +378,7 @@ cutover. | Parameter | Type | Default | Description | |-----------|------|---------|-------------| -| `node.rekey.enabled` | bool | `true` | Initiate periodic Noise rekey on links and sessions. A peer-driven session rekey is still answered when this is off, so session keys can still rotate | +| `node.rekey.enabled` | bool | `true` | Initiate periodic Noise rekey on links and sessions. A peer-driven session rekey is still answered when this is off, so session keys can still rotate. Disabling rekey is unsupported: it runs the node on a less-tested path, and the option is removed in v2. | | `node.rekey.after_secs` | u64 | `120` | Initiate rekey after this many seconds on a session | | `node.rekey.after_messages` | u64 | `65536` | Initiate rekey after this many messages sent on a session | @@ -1167,6 +1167,7 @@ node: loss_threshold: 0.05 # MMP loss rate threshold for CE marking (5%) etx_threshold: 3.0 # MMP ETX threshold for CE marking rekey: + # enabled: false is unsupported, less tested, and removed in v2 enabled: true # periodic Noise rekey for forward secrecy after_secs: 120 # rekey interval (seconds) after_messages: 65536 # rekey after N messages sent From 83cb0b49ce1f43b3ee6ec37697436176be69d091 Mon Sep 17 00:00:00 2001 From: Martti Malmi Date: Tue, 29 Sep 2026 11:20:02 +0300 Subject: [PATCH 03/13] transport: make address clones allocation-free Share immutable address bytes across clones. Format socket addresses on the stack so receive-path construction does not add a second allocation, and document copying in the Vec constructor. Cover scoped IPv6 roundtrips and value semantics, and compare clone, socket-construction, and cross-thread clone/drop costs with the former Vec backing. --- CHANGELOG.md | 4 ++ Cargo.toml | 5 ++ benches/transport_addr_clone.rs | 107 ++++++++++++++++++++++++++++++++ src/transport/mod.rs | 36 ++++++++--- src/transport/types.rs | 47 ++++++++++++-- 5 files changed, 185 insertions(+), 14 deletions(-) create mode 100644 benches/transport_addr_clone.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8da6caf9..b9d79280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -215,6 +215,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Transport address clones share immutable bytes instead of allocating a copy. + Socket addresses are formatted on the stack so construction still needs only + one allocation, including scoped IPv6 addresses. + - `Degraded` is now a level rather than a latch. The supervisor's reason set was monotonic, which was correct while no child could recover; with recovery it would have meant "something broke at some point since boot" rather than diff --git a/Cargo.toml b/Cargo.toml index 748e8bb8..dd4d8a53 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -144,3 +144,8 @@ path = "src/bin/fipstop/main.rs" name = "routing_next_hop" path = "benches/routing_next_hop.rs" harness = false + +[[bench]] +name = "transport_addr_clone" +path = "benches/transport_addr_clone.rs" +harness = false diff --git a/benches/transport_addr_clone.rs b/benches/transport_addr_clone.rs new file mode 100644 index 00000000..603e0bcf --- /dev/null +++ b/benches/transport_addr_clone.rs @@ -0,0 +1,107 @@ +//! Compare shared addresses with the former Vec backing, including the receive +//! constructor and a producer cloning while a consumer drops on another thread. +//! The cross-thread case includes bounded-channel overhead in both measurements; +//! it is not an isolated measurement of the atomic reference count. + +use std::hint::black_box; +use std::io::Write; +use std::net::SocketAddr; +use std::sync::{Barrier, mpsc}; +use std::time::{Duration, Instant}; + +use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main}; +use fips::TransportAddr; + +const ADDRESS_BYTES: [usize; 3] = [6, 18, 58]; + +fn former_from_socket_addr(addr: SocketAddr) -> Vec { + let mut buf = Vec::with_capacity(56); + write!(&mut buf, "{addr}").expect("Vec::write_fmt is infallible"); + buf +} + +fn bench_clone(c: &mut Criterion) { + let mut group = c.benchmark_group("transport_addr_clone"); + for len in ADDRESS_BYTES { + let former = vec![0x5a; len]; + let shared = TransportAddr::from_bytes(&former); + group.bench_with_input(BenchmarkId::new("former_vec", len), &former, |b, addr| { + b.iter(|| black_box(addr.clone())) + }); + group.bench_with_input(BenchmarkId::new("shared", len), &shared, |b, addr| { + b.iter(|| black_box(addr.clone())) + }); + } + group.finish(); +} + +fn bench_socket_addr_and_clone(c: &mut Criterion) { + let mut group = c.benchmark_group("transport_addr_socket_and_clone"); + for (name, text) in [ + ("ipv4", "192.168.1.1:2121"), + ("ipv6", "[2001:db8::1]:2121"), + ( + "scoped_ipv6", + "[ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff%4294967295]:65535", + ), + ] { + let socket: SocketAddr = text.parse().unwrap(); + group.bench_with_input(BenchmarkId::new("former_vec", name), &socket, |b, addr| { + b.iter(|| { + let addr = former_from_socket_addr(black_box(*addr)); + black_box((addr.clone(), addr)) + }) + }); + group.bench_with_input(BenchmarkId::new("shared", name), &socket, |b, addr| { + b.iter(|| { + let addr = TransportAddr::from_socket_addr(black_box(*addr)); + black_box((addr.clone(), addr)) + }) + }); + } + group.finish(); +} + +fn cross_thread_clone_drop(addr: &T, iterations: u64) -> Duration { + let (tx, rx) = mpsc::sync_channel::(256); + let ready = Barrier::new(2); + std::thread::scope(|scope| { + let consumer = scope.spawn(|| { + ready.wait(); + for value in rx { + drop(black_box(value)); + } + }); + // Thread startup is outside the timed interval; completion is included. + ready.wait(); + let start = Instant::now(); + for _ in 0..iterations { + tx.send(black_box(addr.clone())).unwrap(); + } + drop(tx); + consumer.join().unwrap(); + start.elapsed() + }) +} + +fn bench_cross_thread(c: &mut Criterion) { + let mut group = c.benchmark_group("transport_addr_cross_thread"); + for len in ADDRESS_BYTES { + let former = vec![0x5a; len]; + let shared = TransportAddr::from_bytes(&former); + group.bench_with_input(BenchmarkId::new("former_vec", len), &former, |b, addr| { + b.iter_custom(|iterations| cross_thread_clone_drop(addr, iterations)) + }); + group.bench_with_input(BenchmarkId::new("shared", len), &shared, |b, addr| { + b.iter_custom(|iterations| cross_thread_clone_drop(addr, iterations)) + }); + } + group.finish(); +} + +criterion_group! { + name = benches; + config = Criterion::default().sample_size(50); + targets = bench_clone, bench_socket_addr_and_clone, bench_cross_thread +} +criterion_main!(benches); diff --git a/src/transport/mod.rs b/src/transport/mod.rs index 81a25fee..3b0e02ab 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -483,9 +483,12 @@ impl TransportAddr { /// Create a UDP/TCP transport address directly from a socket address. pub fn from_socket_addr(addr: std::net::SocketAddr) -> Self { use std::io::Write; - let mut buf = Vec::with_capacity(56); - write!(&mut buf, "{addr}").expect("Vec::write_fmt is infallible"); - Self::new(buf) + // The longest form is 58 bytes: [39-byte IPv6%10-digit scope]:65535. + let mut buf = [0u8; 64]; + let mut cursor = &mut buf[..]; + write!(&mut cursor, "{addr}").expect("64 bytes is enough for any SocketAddr"); + let len = 64 - cursor.len(); + Self::from_bytes(&buf[..len]) } } @@ -1400,11 +1403,28 @@ mod tests { #[test] fn test_transport_addr_from_socket_addr() { - let addr = TransportAddr::from_socket_addr("127.0.0.1:2121".parse().unwrap()); - assert_eq!(addr.as_str(), Some("127.0.0.1:2121")); - - let addr = TransportAddr::from_socket_addr("[::1]:2121".parse().unwrap()); - assert_eq!(addr.as_str(), Some("[::1]:2121")); + for text in [ + "0.0.0.0:0", + "127.0.0.1:2121", + "255.255.255.255:65535", + "[::1]:2121", + "[2001:db8::1]:2121", + "[::ffff:255.255.255.255]:65535", + "[fe80::1%4294967295]:65535", + "[ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff%4294967295]:65535", + ] { + let socket: std::net::SocketAddr = text.parse().unwrap(); + let addr = TransportAddr::from_socket_addr(socket); + assert_eq!(addr.as_str(), Some(text)); + assert_eq!(addr.as_bytes(), socket.to_string().as_bytes()); + assert_eq!( + addr.as_str() + .unwrap() + .parse::() + .unwrap(), + socket + ); + } } #[test] diff --git a/src/transport/types.rs b/src/transport/types.rs index 4e95f4ce..7557279c 100644 --- a/src/transport/types.rs +++ b/src/transport/types.rs @@ -8,6 +8,7 @@ //! via `pub use types::*`, so existing `crate::transport::{LinkId, ...}` //! imports are unaffected. +use alloc::{string::String, sync::Arc, vec::Vec}; use core::fmt; use core::time::Duration; @@ -91,23 +92,28 @@ impl fmt::Display for LinkDirection { /// Each transport type interprets this differently: /// - UDP/TCP: "host:port" (IP address or DNS hostname) /// - Ethernet: MAC address (6 bytes) +/// +/// The immutable bytes are shared across clones. #[derive(Clone, PartialEq, Eq, Hash)] -pub struct TransportAddr(Vec); +pub struct TransportAddr(Arc<[u8]>); impl TransportAddr { /// Create a transport address from raw bytes. + /// + /// Copies the bytes into shared storage; the vector's allocation is not + /// reused. Prefer [`Self::from_bytes`] when a byte slice is already available. pub fn new(bytes: Vec) -> Self { - Self(bytes) + Self(bytes.into()) } /// Create a transport address from a byte slice. pub fn from_bytes(bytes: &[u8]) -> Self { - Self(bytes.to_vec()) + Self(Arc::from(bytes)) } /// Create a transport address from a string. pub fn from_string(s: &str) -> Self { - Self(s.as_bytes().to_vec()) + Self::from_bytes(s.as_bytes()) } /// Get the raw bytes. @@ -158,7 +164,7 @@ impl fmt::Display for TransportAddr { Ok(()) } None => { - for byte in &self.0 { + for byte in self.0.iter() { write!(f, "{:02x}", byte)?; } Ok(()) @@ -175,7 +181,36 @@ impl From<&str> for TransportAddr { impl From for TransportAddr { fn from(s: String) -> Self { - Self(s.into_bytes()) + Self::from_string(&s) + } +} + +#[cfg(test)] +mod tests { + use super::TransportAddr; + + #[test] + fn transport_addr_clone_shares_immutable_bytes() { + let addr = TransportAddr::from_string("192.168.1.1:2121"); + let cloned = addr.clone(); + + assert_eq!(addr, cloned); + assert_eq!(addr.as_bytes().as_ptr(), cloned.as_bytes().as_ptr()); + } + + #[test] + fn transport_addr_equality_and_hash_use_byte_values() { + // Only tests use std; the transport primitives remain alloc-only. + use std::collections::HashSet; + + let original = TransportAddr::from_string("192.168.1.1:2121"); + let same_value = TransportAddr::from_bytes(b"192.168.1.1:2121"); + let mut addrs = HashSet::new(); + + assert_ne!(original.as_bytes().as_ptr(), same_value.as_bytes().as_ptr()); + assert_eq!(original, same_value); + addrs.insert(original); + assert!(addrs.contains(&same_value)); } } From c5d62a63871dd1a7c4861d3f783bd749c9ee10f2 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 30 Sep 2026 13:49:05 +0000 Subject: [PATCH 04/13] Restart fips-dns and fips-gateway after a tarball upgrade install.sh stopped fips before checking fips-dns. Both fips-dns and fips-gateway require fips.service, so stopping fips had already stopped them: the script saw fips-dns inactive and restarted only fips. .fips resolution stayed down and the gateway stayed stopped until started by hand. The script now records which of the three units were active before stopping any, and starts each of those again. A new tarball-install suite runs install.sh as an upgrade under real systemd, with stub binaries, in both local and GitHub CI. --- .github/workflows/ci.yml | 30 +++ packaging/systemd/install.sh | 43 ++-- testing/README.md | 8 + testing/ci-local.sh | 27 +++ testing/tarball-install/test.sh | 367 ++++++++++++++++++++++++++++++++ 5 files changed, 453 insertions(+), 22 deletions(-) create mode 100755 testing/tarball-install/test.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b60ae603..f936aead 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -474,6 +474,36 @@ jobs: timeout-minutes: 5 run: bash testing/openwrt/maintainer-scripts-test.sh +# ───────────────────────────────────────────────────────────────────────────── +# Job 2f – systemd tarball upgrade scenarios +# +# Runs the tarball's install.sh twice in a systemd container, the second time +# as an upgrade, with fips, fips-dns and fips-gateway in a known state, and +# checks that the units running before the upgrade are running after it. The +# binaries are stubs, so like the OpenWrt job it needs no FIPS build and no +# shared test image, and has a job of its own. +# +# The leg keeps `suite:` because testing/check-ci-parity.sh matches it against +# TARBALL_INSTALL_SUITES in ci-local.sh; the step below does not read it. +# ───────────────────────────────────────────────────────────────────────────── + tarball-install: + name: systemd tarball (${{ matrix.suite }}) + runs-on: ubuntu-latest + if: ${{ !inputs.skip_integration }} + + strategy: + fail-fast: false + matrix: + include: + - suite: tarball-install + + steps: + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 + + - name: Run the systemd tarball upgrade scenarios + timeout-minutes: 10 + run: bash testing/tarball-install/test.sh + # ───────────────────────────────────────────────────────────────────────────── # Job 3 – Integration tests (static mesh + chaos simulation) # diff --git a/packaging/systemd/install.sh b/packaging/systemd/install.sh index f71513b7..89e78111 100755 --- a/packaging/systemd/install.sh +++ b/packaging/systemd/install.sh @@ -112,19 +112,20 @@ fi # --- Install systemd units --- -was_active=false -if systemctl is-active --quiet fips.service 2>/dev/null; then - was_active=true - echo "Stopping running fips service..." - systemctl stop fips.service -fi - -dns_was_active=false -if systemctl is-active --quiet fips-dns.service 2>/dev/null; then - dns_was_active=true - echo "Stopping running fips-dns service..." - systemctl stop fips-dns.service -fi +# fips-dns and fips-gateway carry Requires=fips.service, so stopping fips +# stops them too. Record all three before stopping any of them. +declare -A was_active=() +for unit in fips fips-dns fips-gateway; do + if systemctl is-active --quiet "${unit}.service" 2>/dev/null; then + was_active[$unit]=true + fi +done +for unit in fips-gateway fips-dns fips; do + if [ "${was_active[$unit]:-}" = true ]; then + echo "Stopping running ${unit} service..." + systemctl stop "${unit}.service" + fi +done install -m 0644 "${SCRIPT_DIR}/fips.service" "${SYSTEMD_DIR}/fips.service" install -m 0644 "${SCRIPT_DIR}/fips-dns.service" "${SYSTEMD_DIR}/fips-dns.service" @@ -164,15 +165,13 @@ systemctl enable fips.service systemctl enable fips-dns.service echo "Services enabled (will start on boot)." -# Restart if they were running before -if $was_active; then - echo "Restarting fips service..." - systemctl start fips.service -fi -if $dns_was_active; then - echo "Restarting fips-dns service..." - systemctl start fips-dns.service -fi +# Restart each unit that was running before +for unit in fips fips-dns fips-gateway; do + if [ "${was_active[$unit]:-}" = true ]; then + echo "Restarting ${unit} service..." + systemctl start "${unit}.service" + fi +done echo "" echo "=== Installation complete ===" diff --git a/testing/README.md b/testing/README.md index 99772520..13d176ca 100644 --- a/testing/README.md +++ b/testing/README.md @@ -101,6 +101,14 @@ what the real `build-apk.sh` and `build-ipk.sh` package. No router, opkg or apk-tools is involved. Part of both CI runners as `openwrt-scripts`. +### [tarball-install/](tarball-install/) -- systemd Tarball Upgrade + +Runs the systemd tarball's `install.sh` twice in a Debian 12 systemd +container, the second time as an upgrade, with stub binaries. Checks +that the units running before the upgrade (fips, fips-dns and +fips-gateway, in three combinations) are the ones running after it. +Part of both CI runners as `tarball-install`. + ### [native-api/](native-api/) -- Native Datagram API Checks the experimental native datagram API: a client process opens a diff --git a/testing/ci-local.sh b/testing/ci-local.sh index cdaa413d..7ff15dc8 100755 --- a/testing/ci-local.sh +++ b/testing/ci-local.sh @@ -208,6 +208,7 @@ CHAOS_SUITES=( # testing/chaos/scripts/chaos.sh bloom-storm. GATEWAY_SUITES=(gateway) OPENWRT_SUITES=(openwrt-scripts) +TARBALL_INSTALL_SUITES=(tarball-install) SIDECAR_SUITES=(sidecar) FIREWALL_SUITES=(firewall) NAT_SUITES=(cone symmetric lan) @@ -248,6 +249,9 @@ list_suites() { echo " OpenWrt packaging:" for s in "${OPENWRT_SUITES[@]}"; do echo " $s"; done echo "" + echo " systemd tarball:" + for s in "${TARBALL_INSTALL_SUITES[@]}"; do echo " $s"; done + echo "" echo " Firewall baseline:" for s in "${FIREWALL_SUITES[@]}"; do echo " $s"; done echo "" @@ -1207,6 +1211,15 @@ run_integration() { return fi + # Also ahead of the build context, for the same reason: the tarball + # install scenarios use stub binaries and their own systemd image. + if [[ -z "$ONLY_SUITE" ]]; then + run_tarball_install + elif [[ "$ONLY_SUITE" == "tarball-install" ]]; then + run_tarball_install + return + fi + # Populate THIS run's build context, then install the binaries into it. # Everything but the binaries is copied from the tracked context directory; # the binaries are installed fresh, and a previous run's are deliberately @@ -1370,6 +1383,8 @@ run_suite() { run_gateway ;; openwrt-scripts) run_openwrt_scripts ;; + tarball-install) + run_tarball_install ;; firewall) run_firewall ;; nat-cone|nat-symmetric|nat-lan) @@ -1466,6 +1481,18 @@ run_openwrt_scripts() { return $rc } +# The systemd tarball's install.sh, run as an upgrade under real systemd with +# stub binaries: the units that were running before the upgrade must be the +# ones running after it, including fips-dns and fips-gateway, which stop with +# fips through Requires=. +run_tarball_install() { + local rc=0 + info "[tarball-install] Running the systemd tarball upgrade scenarios" + bash "$SCRIPT_DIR/tarball-install/test.sh" || rc=$? + record "tarball-install" $rc + return $rc +} + run_ci_parity() { local rc=0 info "[ci-parity] Comparing the local suite set against the GitHub matrix" diff --git a/testing/tarball-install/test.sh b/testing/tarball-install/test.sh new file mode 100755 index 00000000..e3474095 --- /dev/null +++ b/testing/tarball-install/test.sh @@ -0,0 +1,367 @@ +#!/bin/bash +# Test the systemd tarball's install.sh as an upgrade, under real systemd. +# +# fips-dns.service and fips-gateway.service carry Requires=fips.service, so +# stopping fips stops both. An upgrade that checks which units are running +# only after stopping fips sees the other two as already stopped, and starts +# fips alone again. This suite installs the tarball once, puts the units in a +# known state, runs install.sh a second time (the upgrade), and checks that +# exactly the units that were running before are running after. +# +# The tarball is staged from the real packaging/systemd/install.sh and unit +# files, and the real packaging/common configuration, the same file set +# build-tarball.sh ships. The binaries and DNS helpers are stubs: the fips and +# fips-gateway stubs sleep, so their units stay active, and the DNS helpers +# exit 0. What is under test is install.sh against real units and a real +# systemd, which carries the Requires= stop propagation; nothing here depends +# on what the binaries do. +# +# Scenarios, each on a fresh container: +# all-active fips, fips-dns and fips-gateway running before the upgrade. +# All three must be running after it, and fips must have been +# restarted (its InvocationID changes). +# fips-only only fips running. fips must be running after, and the other +# two must still be stopped. +# none-active nothing running. Nothing may be running after. +# +# Usage: ./test.sh [--install-sh PATH] [scenario ...] +# No scenarios = run all of them. +# --install-sh PATH runs another install.sh in place of the tree's, so the +# suite can be pointed at an older script without editing the tree. +# +# Requirements: Docker able to grant SYS_ADMIN and NET_ADMIN and an unconfined +# AppArmor profile (the container is not privileged; see +# testing/lib/systemd-container.sh), and /dev/net/tun on the host. +# +# Exit 0 only when every check passed. A container that cannot boot or be +# inspected is a FAIL, never a skip. + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +# shellcheck source=SCRIPTDIR/../lib/systemd-container.sh +source "$SCRIPT_DIR/../lib/systemd-container.sh" +# shellcheck source=SCRIPTDIR/../lib/image-build.sh +source "$SCRIPT_DIR/../lib/image-build.sh" +REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" + +IMAGE="fips-tarball-test:debian12" +BOOT_TIMEOUT=60 +# Bounds one install.sh run. It starts at most three units, and the gateway's +# ExecStartPre returns at once because fips0 exists before the first start. +INSTALL_TIMEOUT=120 +UNIT_START_TIMEOUT=60 +TARBALL_DIR=/opt/fips-tarball + +PASS=0 +FAIL=0 + +INSTALL_SH="$REPO_ROOT/packaging/systemd/install.sh" +STAGE="" + +log() { echo "=== $*"; } +pass() { echo " PASS: $*"; PASS=$((PASS + 1)); } +fail() { echo " FAIL: $*"; FAIL=$((FAIL + 1)); } + +# Remove a container, ignoring one that does not exist. +cleanup_container() { + local name="$1" + docker rm -f "$name" >/dev/null 2>&1 || true +} + +# Remove every scenario's container and the staged tarball. +cleanup_all() { + local s + for s in $ALL_SCENARIOS; do + cleanup_container "$(container_name "$s")" + done + [ -n "$STAGE" ] && rm -rf "$STAGE" +} + +# The container name for a scenario, scoped to this CI run. +container_name() { + echo "fips-tarball-test-$1${FIPS_CI_NAME_SUFFIX:-}" + return 0 +} + +# Build the systemd image. The build context is an empty directory: the +# tarball is copied into the container after it starts, not baked in. +build_image() { + local ctx rc=0 + ctx=$(mktemp -d) + retry_build "docker build -t $IMAGE" build_inline "$IMAGE" "$(cat <<'DOCKERFILE' +FROM debian:12 +ENV DEBIAN_FRONTEND=noninteractive +RUN apt-get update && apt-get install -y --no-install-recommends \ + systemd iproute2 procps && \ + apt-get clean && rm -rf /var/lib/apt/lists/* +CMD ["/lib/systemd/systemd"] +DOCKERFILE + )" "$ctx" || rc=$? + rm -rf "$ctx" + return "$rc" +} + +# Stage a tarball directory: the real install script, units and configuration, +# with stub binaries and DNS helpers. +stage_tarball() { + local f + STAGE=$(mktemp -d) + cp "$INSTALL_SH" "$STAGE/install.sh" || return 1 + for f in fips.service fips-dns.service fips-gateway.service fips-firewall.service; do + cp "$REPO_ROOT/packaging/systemd/$f" "$STAGE/" || return 1 + done + for f in fips.yaml hosts fips.nft; do + cp "$REPO_ROOT/packaging/common/$f" "$STAGE/" || return 1 + done + printf '#!/bin/sh\nexec sleep infinity\n' > "$STAGE/fips" + printf '#!/bin/sh\nexec sleep infinity\n' > "$STAGE/fips-gateway" + printf '#!/bin/sh\nexit 0\n' > "$STAGE/fipsctl" + printf '#!/bin/sh\nexit 0\n' > "$STAGE/fips-dns-setup" + printf '#!/bin/sh\nexit 0\n' > "$STAGE/fips-dns-teardown" + chmod 0755 "$STAGE"/install.sh "$STAGE"/fips "$STAGE"/fips-gateway \ + "$STAGE"/fipsctl "$STAGE"/fips-dns-setup "$STAGE"/fips-dns-teardown + return 0 +} + +# Start the scenario's systemd container. Not privileged: see +# testing/lib/systemd-container.sh for the flags and why. The TUN device is +# passed so the scenario can create the fips0 that fips-gateway.service's +# ExecStartPre waits for. +start_container() { + local name="$1" + cleanup_container "$name" + run_quiet "docker run $name" \ + docker run -d --name "$name" \ + --label com.corganlabs.fips-ci=1 \ + "${SYSTEMD_CAPS[@]}" \ + --cgroupns=host \ + --device /dev/net/tun \ + -v /sys/fs/cgroup:/sys/fs/cgroup:rw \ + --tmpfs /run --tmpfs /run/lock \ + "$IMAGE" || return + check_isolation "$name" + return +} + +# Wait for systemd to finish booting. `degraded` counts as booted: units such +# as systemd-modules-load cannot succeed in a container, and the system still +# finished starting. The state string is tested directly rather than piped, so +# pipefail cannot turn a degraded boot into a failure. +wait_for_systemd() { + local name="$1" state + for _i in $(seq 1 "$BOOT_TIMEOUT"); do + state=$(docker exec "$name" systemctl is-system-running --wait 2>/dev/null || true) + case "$state" in + running | degraded) return 0 ;; + esac + sleep 1 + done + echo " ERROR: systemd did not reach running state in ${BOOT_TIMEOUT}s" >&2 + return 1 +} + +# Run the staged install.sh in the container, under a bound. +run_install() { + local name="$1" label="$2" out rc=0 + out=$(timeout "$INSTALL_TIMEOUT" docker exec "$name" "$TARBALL_DIR/install.sh" 2>&1) || rc=$? + if [ "$rc" -ne 0 ]; then + fail "$label: install.sh exited $rc" + echo "$out" | sed 's/^/ /' + return 1 + fi + pass "$label: install.sh exits 0" + return 0 +} + +# Print a unit's active state ("unknown" when the container cannot be asked). +unit_state() { + local name="$1" unit="$2" state + state=$(docker exec "$name" systemctl is-active "$unit" 2>/dev/null) + echo "${state:-unknown}" + return 0 +} + +# Print a unit's InvocationID, which changes on every start. +invocation_id() { + docker exec "$1" systemctl show -p InvocationID --value "$2" 2>/dev/null + return +} + +# Record a PASS when a unit is in the wanted state, a FAIL otherwise. +expect_state() { + local name="$1" label="$2" unit="$3" want="$4" got + got=$(unit_state "$name" "$unit") + if [ "$got" = "$want" ]; then + pass "$label: $unit is $got" + else + fail "$label: $unit is $got (want $want)" + fi + return 0 +} + +# Start a unit under a bound, recording a FAIL when it does not start. +start_unit() { + local name="$1" unit="$2" out + if ! out=$(timeout "$UNIT_START_TIMEOUT" docker exec "$name" systemctl start "$unit" 2>&1); then + fail "setup: could not start $unit: ${out:-no output}" + return 1 + fi + return 0 +} + +# Boot a fresh container, copy the tarball in and run the first install. +# Returns 1, having recorded a FAIL, when any of that does not happen. +prepare() { + local name="$1" label="$2" + start_container "$name" || { fail "$label: container did not start"; return 1; } + wait_for_systemd "$name" || { + fail "$label: systemd did not boot; the checks would read an unstarted system" + return 1 + } + if ! docker cp "$STAGE/." "$name:$TARBALL_DIR" >/dev/null; then + fail "$label: could not copy the tarball into $name" + return 1 + fi + if ! docker exec "$name" ip tuntap add fips0 mode tun >/dev/null 2>&1; then + fail "$label: could not create fips0 in $name" + return 1 + fi + run_install "$name" "$label: first install" || return 1 + return 0 +} + +scenario_all_active() { + local label="all-active" name before after + name=$(container_name "$label") + log "$label: fips, fips-dns and fips-gateway running before the upgrade" + if prepare "$name" "$label" \ + && start_unit "$name" fips.service \ + && start_unit "$name" fips-dns.service \ + && start_unit "$name" fips-gateway.service; then + expect_state "$name" "$label: before" fips.service active + expect_state "$name" "$label: before" fips-dns.service active + expect_state "$name" "$label: before" fips-gateway.service active + before=$(invocation_id "$name" fips.service) + if run_install "$name" "$label: upgrade"; then + expect_state "$name" "$label: after" fips.service active + expect_state "$name" "$label: after" fips-dns.service active + expect_state "$name" "$label: after" fips-gateway.service active + after=$(invocation_id "$name" fips.service) + if [ -z "$before" ] || [ -z "$after" ]; then + fail "$label: could not read fips.service's InvocationID (before '${before}', after '${after}')" + elif [ "$before" != "$after" ]; then + pass "$label: the upgrade restarted fips.service" + else + fail "$label: fips.service kept InvocationID $before, so the upgrade did not stop and restart it" + fi + fi + fi + cleanup_container "$name" + return 0 +} + +scenario_fips_only() { + local label="fips-only" name + name=$(container_name "$label") + log "$label: only fips running before the upgrade" + if prepare "$name" "$label" && start_unit "$name" fips.service; then + expect_state "$name" "$label: before" fips.service active + expect_state "$name" "$label: before" fips-dns.service inactive + expect_state "$name" "$label: before" fips-gateway.service inactive + if run_install "$name" "$label: upgrade"; then + expect_state "$name" "$label: after" fips.service active + expect_state "$name" "$label: after" fips-dns.service inactive + expect_state "$name" "$label: after" fips-gateway.service inactive + fi + fi + cleanup_container "$name" + return 0 +} + +scenario_none_active() { + local label="none-active" name + name=$(container_name "$label") + log "$label: nothing running before the upgrade" + if prepare "$name" "$label"; then + expect_state "$name" "$label: before" fips.service inactive + if run_install "$name" "$label: upgrade"; then + expect_state "$name" "$label: after" fips.service inactive + expect_state "$name" "$label: after" fips-dns.service inactive + expect_state "$name" "$label: after" fips-gateway.service inactive + fi + fi + cleanup_container "$name" + return 0 +} + +ALL_SCENARIOS="all-active fips-only none-active" + +_args=() +while [ $# -gt 0 ]; do + case "$1" in + --install-sh) + INSTALL_SH="${2:?--install-sh requires a path}" + shift 2 + ;; + -h|--help) + echo "usage: test.sh [--install-sh PATH] [scenario ...]" + echo "scenarios: $ALL_SCENARIOS" + exit 0 + ;; + -*) + echo "Unknown option: $1" >&2 + exit 1 + ;; + *) + _args+=("$1") + shift + ;; + esac +done +set -- ${_args[@]+"${_args[@]}"} + +if [ $# -eq 0 ]; then + scenarios="$ALL_SCENARIOS" +else + scenarios="$*" +fi + +for scenario in $scenarios; do + case " $ALL_SCENARIOS " in + *" $scenario "*) ;; + *) + echo "Unknown scenario: $scenario (available: $ALL_SCENARIOS)" >&2 + exit 1 + ;; + esac +done + +if [ ! -f "$INSTALL_SH" ]; then + echo "install script not found: $INSTALL_SH" >&2 + exit 1 +fi + +trap cleanup_all EXIT + +log "Building $IMAGE" +if ! build_image; then + fail "image build failed" +elif ! stage_tarball; then + fail "could not stage the tarball" +else + for scenario in $scenarios; do + case "$scenario" in + all-active) scenario_all_active ;; + fips-only) scenario_fips_only ;; + none-active) scenario_none_active ;; + esac + echo + done +fi + +echo "═══════════════════════════════════════" +echo "Results: $PASS passed, $FAIL failed" +echo "═══════════════════════════════════════" + +[ "$FAIL" -eq 0 ] && [ "$PASS" -gt 0 ] From efe77410a1636dba576231cd912ff685ca33990b Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 30 Sep 2026 13:51:00 +0000 Subject: [PATCH 05/13] Version release-candidate Debian packages as X.Y.Z~rcN A tag's -rcN suffix became the .deb version X.Y.Z-rcN, which dpkg reads as a revision of X.Y.Z and sorts above the release, so the release never upgraded a host that had installed the candidate. git does not allow '~' in a tag, so the Linux package workflow now maps a pre-release suffix to ~ for the .deb Version only. The tarball, the workflow artifact and the .deb file name keep the tag's form: GitHub renames a release asset whose name has special characters, which would leave checksums-linux.txt naming a file the release does not carry. Release and branch builds are unchanged. A new check runs the workflow's own derivation on a release tag, a candidate tag and a branch, confirms with dpkg that the candidate sorts below the release, and checks that the output is declared, passed to the .deb build and that the file is renamed. Both local and GitHub CI run it. --- .github/workflows/ci.yml | 5 + .github/workflows/package-linux.yml | 27 ++++- testing/check-deb-version.sh | 175 ++++++++++++++++++++++++++++ testing/ci-local.sh | 12 ++ 4 files changed, 218 insertions(+), 1 deletion(-) create mode 100755 testing/check-deb-version.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f936aead..e7021746 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -84,6 +84,11 @@ jobs: # a matrix suite. - name: Run convergence-gate unit tests run: bash testing/lib/wait-converge-test.sh + # Runs package-linux.yml's own version derivation on a release tag, a + # candidate tag and a branch. Not a matrix suite either, so it is kept in + # step with ci-local.sh's run_deb_version by hand. + - name: Check the Debian version derived for tags and branches + run: bash testing/check-deb-version.sh fmt: name: Format check diff --git a/.github/workflows/package-linux.yml b/.github/workflows/package-linux.yml index 0c4db44e..42a37d87 100644 --- a/.github/workflows/package-linux.yml +++ b/.github/workflows/package-linux.yml @@ -18,6 +18,7 @@ jobs: runs-on: ubuntu-latest outputs: linux_package_version: ${{ steps.linux_version.outputs.linux_package_version }} + deb_package_version: ${{ steps.linux_version.outputs.deb_package_version }} steps: - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 with: @@ -42,7 +43,18 @@ jobs: VERSION="${BASE_VERSION}+${BRANCH}.${HEIGHT}.${HASH}" fi + # dpkg reads X.Y.Z-rcN as revision rcN of X.Y.Z and sorts it above + # the release; X.Y.Z~rcN sorts below it. git refuses '~' in a ref + # name, so the tag carries '-' and this maps it, for the .deb only. + # testing/check-deb-version.sh runs this step's text. + DEB_VERSION="$VERSION" + if [[ "$GITHUB_REF" == refs/tags/* ]] \ + && [[ "$VERSION" =~ ^([0-9]+\.[0-9]+\.[0-9]+)-((alpha|beta|pre|rc)[0-9]*)$ ]]; then + DEB_VERSION="${BASH_REMATCH[1]}~${BASH_REMATCH[2]}" + fi + echo "linux_package_version=${VERSION}" >> "$GITHUB_OUTPUT" + echo "deb_package_version=${DEB_VERSION}" >> "$GITHUB_OUTPUT" build: name: Build Linux artifacts (${{ matrix.artifact_arch }}) @@ -128,7 +140,7 @@ jobs: : ${GITHUB_OUTPUT:=/tmp/github_output} packaging/debian/build-deb-container.sh \ - --version "${{ needs.determine-versioning.outputs.linux_package_version }}" \ + --version "${{ needs.determine-versioning.outputs.deb_package_version }}" \ --output-dir deploy \ --image-archive "$RUNNER_TEMP/deb-builder-image.tar" \ | tee /tmp/build-deb-container.log @@ -149,6 +161,19 @@ jobs: ;; esac + # A candidate's package Version carries '~' (see determine-versioning), + # and cargo-deb puts it in the file name. GitHub renames a release + # asset whose name has special characters, which would leave + # checksums-linux.txt naming a file the release does not have. The + # file takes the tag's '-' instead; the Version inside is unchanged. + case "$DEB_FILE" in + *~*) + RENAMED="$(dirname "$DEB_FILE")/$(basename "$DEB_FILE" | tr '~' '-')" + mv "$DEB_FILE" "$RENAMED" + DEB_FILE="$RENAMED" + ;; + esac + # Record it relative to the checkout: upload-artifact derives the # archive layout from the common ancestor of its paths, and an # absolute path here would nest the package under directories the diff --git a/testing/check-deb-version.sh b/testing/check-deb-version.sh new file mode 100755 index 00000000..a18b747b --- /dev/null +++ b/testing/check-deb-version.sh @@ -0,0 +1,175 @@ +#!/bin/bash +# ── Debian package version derivation check ───────────────────────────────── +# A release candidate is tagged vX.Y.Z-rcN, because git refuses '~' in a ref +# name. dpkg reads X.Y.Z-rcN as revision rcN of X.Y.Z and sorts it ABOVE the +# release, so a host that installed the candidate is never upgraded by the +# release. package-linux.yml's "Derive Linux package version" step therefore +# maps the tag's pre-release suffix to '~' for the .deb, which dpkg sorts below. +# +# This runs that step's own text, taken from the workflow, rather than a copy, +# so the check and the workflow cannot drift apart. Cases: +# refs/tags/v0.5.3 both versions 0.5.3 +# refs/tags/v0.5.3-rc1 deb 0.5.3~rc1, tarball and artifact 0.5.3-rc1, and +# dpkg orders 0.5.2 < 0.5.3~rc1 < 0.5.3 +# refs/heads/maint deb equals the tarball version, which is +# +maint.. +# +# It also checks the wiring the derivation needs to have any effect: the job +# declares the deb_package_version output, the .deb build passes it as +# --version, and that step renames a '~' in the package file name to '-' (a +# GitHub release renames an asset with special characters in its name, which +# would leave checksums-linux.txt naming a file the release does not have). +# Those three are read from the text, not executed. +# +# Exit 0 = clean. Exit 1 = a case or a wiring check failed. Exit 2 = the check +# could not run (no dpkg, no PyYAML, the step not found, or an output missing); +# never treated as a pass. +# ───────────────────────────────────────────────────────────────────────────── +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +WORKFLOW="$PROJECT_ROOT/.github/workflows/package-linux.yml" + +cant() { + echo "check-deb-version: $*; cannot verify the Debian version derivation" >&2 + exit 2 +} + +[ -f "$WORKFLOW" ] || cant "missing $WORKFLOW" +command -v dpkg >/dev/null 2>&1 || cant "dpkg not found" +command -v python3 >/dev/null 2>&1 || cant "python3 not found" +python3 -c "import yaml" >/dev/null 2>&1 || cant "python3 module 'yaml' not found" + +WORK=$(mktemp -d) || cant "mktemp failed" +trap 'rm -rf "$WORK"' EXIT + +# Pull the derivation step's run text, the job's declared outputs and the +# .deb build step's run text out of the workflow. +if ! python3 - "$WORKFLOW" "$WORK" <<'PY' +import sys +from pathlib import Path + +import yaml + +workflow, work = sys.argv[1], Path(sys.argv[2]) +doc = yaml.safe_load(Path(workflow).read_text(encoding="utf-8")) +jobs = doc.get("jobs") or {} + + +def step_run(job, name): + for step in (jobs.get(job) or {}).get("steps") or []: + if step.get("name") == name: + return step.get("run") + return None + + +derive = step_run("determine-versioning", "Derive Linux package version") +build = step_run("build", "Build Debian package in the pinned container") +if not derive or not build: + missing = "derivation" if not derive else "Debian build" + print(f"check-deb-version: the {missing} step was not found in {workflow}", + file=sys.stderr) + sys.exit(2) +(work / "derive.sh").write_text(derive, encoding="utf-8") +(work / "build.sh").write_text(build, encoding="utf-8") +outputs = (jobs.get("determine-versioning") or {}).get("outputs") or {} +(work / "outputs").write_text( + "".join(f"{k}={v}\n" for k, v in outputs.items()), encoding="utf-8") +PY +then + exit 2 +fi + +FAILED=0 +ok() { echo " PASS $*"; } +bad() { echo " FAIL $*"; FAILED=$((FAILED + 1)); } + +# Run the derivation for one ref and load its outputs into LINUX and DEB. +# Exits 2 when the step fails or does not write both outputs: no version was +# established, which is not a failed case. +derive() { + local ref="$1" out="$WORK/github_output" + : > "$out" + if ! (cd "$PROJECT_ROOT" && GITHUB_OUTPUT="$out" GITHUB_REF="$ref" \ + GITHUB_REF_NAME="${ref#refs/*/}" bash -eo pipefail "$WORK/derive.sh") >"$WORK/derive.log" 2>&1; then + echo "check-deb-version: the derivation failed for $ref:" >&2 + cat "$WORK/derive.log" >&2 + exit 2 + fi + LINUX=$(sed -n 's/^linux_package_version=//p' "$out") + DEB=$(sed -n 's/^deb_package_version=//p' "$out") + if [ -z "$LINUX" ] || [ -z "$DEB" ]; then + cant "the derivation for $ref did not write both outputs (linux '$LINUX', deb '$DEB')" + fi + return 0 +} + +# Record whether dpkg orders $1 $2 $3 (lt, gt, ...). +expect_order() { + if dpkg --compare-versions "$1" "$2" "$3"; then + ok "dpkg: $1 $2 $3" + else + bad "dpkg: $1 $2 $3 does not hold" + fi + return 0 +} + +# Record whether a derived version equals the expected one. +expect_eq() { + local label="$1" got="$2" want="$3" + if [ "$got" = "$want" ]; then + ok "$label: $got" + else + bad "$label: $got (want $want)" + fi + return 0 +} + +echo "Release tag refs/tags/v0.5.3" +derive refs/tags/v0.5.3 +expect_eq "linux_package_version" "$LINUX" "0.5.3" +expect_eq "deb_package_version" "$DEB" "0.5.3" + +echo "Candidate tag refs/tags/v0.5.3-rc1" +derive refs/tags/v0.5.3-rc1 +expect_eq "linux_package_version" "$LINUX" "0.5.3-rc1" +expect_eq "deb_package_version" "$DEB" "0.5.3~rc1" +expect_order "$DEB" lt 0.5.3 +expect_order "$DEB" gt 0.5.2 + +echo "Branch refs/heads/maint" +derive refs/heads/maint +CRATE=$(awk -F'"' '/^version = /{print $2; exit}' "$PROJECT_ROOT/Cargo.toml") +HEIGHT=$(git -C "$PROJECT_ROOT" rev-list --count HEAD) || cant "git rev-list failed" +HASH=$(git -C "$PROJECT_ROOT" rev-parse --short HEAD) || cant "git rev-parse failed" +[ -n "$CRATE" ] || cant "no version in Cargo.toml" +expect_eq "linux_package_version" "$LINUX" "${CRATE}+maint.${HEIGHT}.${HASH}" +expect_eq "deb_package_version" "$DEB" "$LINUX" + +echo "Wiring" +# shellcheck disable=SC2016 # the ${{ }} expressions are workflow text, not shell +if grep -qxF 'deb_package_version=${{ steps.linux_version.outputs.deb_package_version }}' "$WORK/outputs"; then + ok "determine-versioning declares the deb_package_version output" +else + bad "determine-versioning does not declare deb_package_version from the derivation step" +fi +# shellcheck disable=SC2016 +if grep -qF -- '--version "${{ needs.determine-versioning.outputs.deb_package_version }}"' "$WORK/build.sh"; then + ok "the .deb build passes deb_package_version as --version" +else + bad "the .deb build does not pass deb_package_version as --version" +fi +if grep -qF "tr '~' '-'" "$WORK/build.sh"; then + ok "the .deb build renames a '~' in the package file name" +else + bad "the .deb build does not rename a '~' in the package file name" +fi + +echo +if [ "$FAILED" -eq 0 ]; then + echo "check-deb-version: all checks passed" + exit 0 +fi +echo "check-deb-version: $FAILED check(s) failed" +exit 1 diff --git a/testing/ci-local.sh b/testing/ci-local.sh index 7ff15dc8..d48acb9b 100755 --- a/testing/ci-local.sh +++ b/testing/ci-local.sh @@ -1572,6 +1572,17 @@ run_wait_converge() { record "wait-converge" $rc } +# The .deb version package-linux.yml derives for a release tag, a candidate +# tag and a branch. A candidate must sort below its release under dpkg, which +# the tag's -rcN does not, so the workflow maps it to ~rcN; this runs the +# workflow's own step text. Static, about a second, and needs nothing built. +run_deb_version() { + local rc=0 + info "[deb-version] Checking the Debian version derived for tags and branches" + bash "$SCRIPT_DIR/check-deb-version.sh" || rc=$? + record "deb-version" $rc +} + # ── Main ─────────────────────────────────────────────────────────────────── main() { @@ -1593,6 +1604,7 @@ main() { run_action_pins run_comment_refs run_wait_converge + run_deb_version if [[ "$TEST_ONLY" == true ]]; then run_tests From dc328cecc3d0a3141495c8f34f27596d354231bf Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 30 Sep 2026 13:51:11 +0000 Subject: [PATCH 06/13] Add the changelog entries for the upgrade restart fix, candidate package versions and the rekey caveat --- CHANGELOG.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 042f2cca..5a727713 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +#### Linux packages + +- A release-candidate `.deb` is versioned `X.Y.Z~rcN` instead of + `X.Y.Z-rcN`. dpkg read the old form as a revision of the release and sorted + it above `X.Y.Z`, so a host that installed a candidate was not upgraded by + the release. The tarball, artifact and `.deb` file names keep the tag's + `-rcN`. + +### Deprecated + +#### Sessions and rekey + +- Setting `node.rekey.enabled: false`. The disabled path is less tested and is + not supported, and the option is removed in v2. It still works as before; + the configuration reference and the mesh-layer design now say so. + +### Fixed + +#### Linux packages + +- The systemd tarball's `install.sh` restarts fips-dns and fips-gateway after + an upgrade when they were running. It stopped fips first, which stopped both + through `Requires=fips.service`, then restarted only fips, so `.fips` + resolution stayed down and the gateway stayed stopped until started by hand. + ## [0.5.2] - 2026-09-28 ### Added From 13c53785ad6bbb73fc550b354a65d07f1d26b8ce Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 30 Sep 2026 13:45:22 +0000 Subject: [PATCH 07/13] Correct the pfSense firmware-upgrade statement in the packaging docs packaging/README.md and the pfSense builder's header said a firmware upgrade removes the package, and the fips-dns-setup comment said the same of everything under /usr/local. pfSense-upgrade reinstalls only pfSense-pkg-* packages, and a live Plus 26.03.1 to 26.07 upgrade kept this one, as the pfSense README, the post-install banner and pkg-descr already say. A major base change still calls for the package built for the new base. --- packaging/README.md | 8 +++++--- packaging/pfsense/build-pkg.sh | 6 ++++-- packaging/pfsense/fips-dns-setup | 9 +++++---- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/packaging/README.md b/packaging/README.md index ae948cae..a989409c 100644 --- a/packaging/README.md +++ b/packaging/README.md @@ -393,9 +393,11 @@ pkg add ./fips--pfsense-ce2.8-amd64.pkg /usr/local/libexec/fips/fips-dns-setup # edits config.xml; run deliberately ``` -Not a Netgate-supported package, and a pfSense firmware upgrade removes -it. See [pfsense/README.md](pfsense/README.md) for the "Allow IPv6" -prerequisite the mesh depends on, firewall-rule notes, and removal +Not a Netgate-supported package. A pfSense firmware upgrade keeps it (it +is a plain pkg, not a `pfSense-pkg-*`); after a major base change, +reinstall the package built for the new base. See +[pfsense/README.md](pfsense/README.md) for the "Allow IPv6" prerequisite +the mesh depends on, firewall-rule notes, and upgrade and removal behaviour. ### Windows (`.zip`) diff --git a/packaging/pfsense/build-pkg.sh b/packaging/pfsense/build-pkg.sh index 37efbbda..4840767e 100755 --- a/packaging/pfsense/build-pkg.sh +++ b/packaging/pfsense/build-pkg.sh @@ -24,8 +24,10 @@ # This package integrates through the DNS Resolver custom options. # - The responder's bind address, for the reason recorded in # fips.yaml.dns. -# - Lifetime. A pfSense firmware upgrade reinstalls the base image and -# takes third-party packages with it, so post-install says so. +# - Lifetime. A firmware upgrade keeps the package (pfSense-upgrade +# reinstalls only pfSense-pkg-* packages), but a major upgrade changes +# the FreeBSD base, so post-install says to reinstall the package +# built for the new base. # # Ships fips, fipsctl and fipstop. fips-gateway is excluded: its NAT # backend is nftables (Linux-only), and pfSense has pf for that anyway. diff --git a/packaging/pfsense/fips-dns-setup b/packaging/pfsense/fips-dns-setup index 41b33640..04ed4a42 100755 --- a/packaging/pfsense/fips-dns-setup +++ b/packaging/pfsense/fips-dns-setup @@ -13,10 +13,11 @@ # options" box, which unbound.inc splices into the generated config # verbatim. It is stored base64-encoded in config.xml, which is the part # that makes it the right home: config.xml is what survives a reboot, a -# firmware upgrade and a config restore, whereas everything this package -# installs under /usr/local does not. So the .fips zone keeps resolving -# across an upgrade that removes the daemon, which is a loud failure -# (SERVFAIL on .fips) rather than a quiet one. +# firmware upgrade and a config restore. What this package installs +# under /usr/local survives a firmware upgrade too (pfSense-upgrade +# reinstalls only pfSense-pkg-* packages), but not a removal of the +# package. So the .fips zone can outlive the daemon, which is a loud +# failure (SERVFAIL on .fips) rather than a quiet one. # # This edits the firewall's live configuration, so it is deliberately # NOT run from the package's post-install: installing a package should From 968d75dae389bf3b47b4297182702a8e952ccc37 Mon Sep 17 00:00:00 2001 From: Martti Malmi Date: Tue, 29 Sep 2026 11:19:57 +0300 Subject: [PATCH 08/13] Close Ethernet socket when interface lookup fails An invalid or missing interface returns before PacketSocket owns its raw descriptor, leaking a socket on each failed startup. Keep an OwnedFd guard throughout setup and transfer it only after success. Add a CAP_NET_RAW regression that checks missing and invalid interface names in an isolated process. --- CHANGELOG.md | 5 +++ src/transport/ethernet/io_linux.rs | 57 +++++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a727713..d56b3851 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 through `Requires=fips.service`, then restarted only fips, so `.fips` resolution stayed down and the gateway stayed stopped until started by hand. +#### Links and transports + +- Ethernet startup no longer leaks a socket when the configured interface is + missing or invalid on Linux. + ## [0.5.2] - 2026-09-28 ### Added diff --git a/src/transport/ethernet/io_linux.rs b/src/transport/ethernet/io_linux.rs index 806e3269..73e1588e 100644 --- a/src/transport/ethernet/io_linux.rs +++ b/src/transport/ethernet/io_linux.rs @@ -1,7 +1,7 @@ //! AF_PACKET socket creation, binding, and ioctl helpers (Linux). use crate::transport::TransportError; -use std::os::unix::io::{AsRawFd, RawFd}; +use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, OwnedFd, RawFd}; /// Wrapper around an AF_PACKET SOCK_DGRAM file descriptor. /// @@ -39,6 +39,9 @@ impl PacketSocket { err ))); } + // SAFETY: socket returned a new descriptor, owned only by this guard + // until construction succeeds. Every setup error closes it on drop. + let owned_fd = unsafe { OwnedFd::from_raw_fd(fd) }; // Look up interface index let if_index = get_if_index(fd, interface)?; @@ -58,7 +61,6 @@ impl PacketSocket { }; if ret < 0 { let err = std::io::Error::last_os_error(); - unsafe { libc::close(fd) }; return Err(TransportError::StartFailed(format!( "bind(AF_PACKET, {}) failed: {}", interface, err @@ -69,7 +71,6 @@ impl PacketSocket { let flags = unsafe { libc::fcntl(fd, libc::F_GETFL) }; if flags < 0 { let err = std::io::Error::last_os_error(); - unsafe { libc::close(fd) }; return Err(TransportError::StartFailed(format!( "fcntl(F_GETFL) failed: {}", err @@ -78,7 +79,6 @@ impl PacketSocket { let ret = unsafe { libc::fcntl(fd, libc::F_SETFL, flags | libc::O_NONBLOCK) }; if ret < 0 { let err = std::io::Error::last_os_error(); - unsafe { libc::close(fd) }; return Err(TransportError::StartFailed(format!( "fcntl(F_SETFL, O_NONBLOCK) failed: {}", err @@ -86,7 +86,7 @@ impl PacketSocket { } Ok(Self { - fd, + fd: owned_fd.into_raw_fd(), if_index, ethertype, }) @@ -346,3 +346,50 @@ fn get_if_mtu(fd: RawFd, if_index: i32) -> Result { let mtu = unsafe { ifr.ifr_ifru.ifru_mtu } as u16; Ok(mtu) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + #[ignore = "requires CAP_NET_RAW"] + fn invalid_interface_does_not_leak_socket() { + const CHILD: &str = "FIPS_TEST_PACKET_SOCKET_CHILD"; + if std::env::var_os(CHILD).is_none() { + // Count descriptors in a separate process so concurrently running + // tests cannot affect the result. + let output = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "transport::ethernet::io::platform::tests::invalid_interface_does_not_leak_socket", + "--ignored", + "--nocapture", + ]) + .env(CHILD, "1") + .output() + .unwrap(); + assert!( + output.status.success(), + "{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + return; + } + + let open_fds = || std::fs::read_dir("/proc/self/fd").unwrap().count(); + let before = open_fds(); + for interface in ["fips-missing-interface", "invalid\0interface"] { + for _ in 0..8 { + let result = PacketSocket::open(interface, 0x88b5); + assert!(matches!( + result, + Err(TransportError::StartFailed(ref message)) + if message.starts_with("interface not found:") + || message.starts_with("invalid interface name:") + )); + } + } + assert_eq!(open_fds(), before, "failed setup leaked a socket"); + } +} From 1b55a0def3347e79af784974a609217062c01b70 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 30 Sep 2026 15:52:58 +0000 Subject: [PATCH 09/13] Make the Ethernet socket leak test prove it ran, and widen its changelog entry The regression test re-runs itself in a child process, and the child selected the test with --ignored. libtest exits 0 when its filter selects nothing, so the parent passed without the test running if the module path changed or the #[ignore] attribute was removed, since --ignored then selects nothing. The child now uses --include-ignored, the parent requires "1 passed" in the child's output, and a failed match on the open result prints the result, which shows the CAP_NET_RAW error from an unprivileged run. The same leak happened whenever the beacon sender reopened its socket after the interface went away: a failed reopen is retried on each later beacon, so a node whose interface was removed and not recreated leaked one descriptor per beacon interval. The changelog entry now says so. --- CHANGELOG.md | 7 +++++-- src/transport/ethernet/io_linux.rs | 29 ++++++++++++++++++----------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d56b3851..c46d2eb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,8 +36,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 #### Links and transports -- Ethernet startup no longer leaks a socket when the configured interface is - missing or invalid on Linux. +- The Ethernet transport on Linux no longer leaks a socket when the configured + interface is missing or invalid, at startup or when the beacon socket is + reopened after the interface goes away. A running node whose interface was + removed and not recreated retried the reopen on every beacon interval and + leaked one descriptor each time. ## [0.5.2] - 2026-09-28 diff --git a/src/transport/ethernet/io_linux.rs b/src/transport/ethernet/io_linux.rs index 73e1588e..ede23711 100644 --- a/src/transport/ethernet/io_linux.rs +++ b/src/transport/ethernet/io_linux.rs @@ -362,17 +362,20 @@ mod tests { .args([ "--exact", "transport::ethernet::io::platform::tests::invalid_interface_does_not_leak_socket", - "--ignored", + "--include-ignored", "--nocapture", ]) .env(CHILD, "1") .output() .unwrap(); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(output.status.success(), "{stdout}\n{stderr}"); + // libtest exits 0 when the filter selects nothing, so require + // that the child actually ran this test. assert!( - output.status.success(), - "{}\n{}", - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr), + stdout.contains("1 passed"), + "child did not run the test:\n{stdout}\n{stderr}" ); return; } @@ -382,12 +385,16 @@ mod tests { for interface in ["fips-missing-interface", "invalid\0interface"] { for _ in 0..8 { let result = PacketSocket::open(interface, 0x88b5); - assert!(matches!( - result, - Err(TransportError::StartFailed(ref message)) - if message.starts_with("interface not found:") - || message.starts_with("invalid interface name:") - )); + assert!( + matches!( + result, + Err(TransportError::StartFailed(ref message)) + if message.starts_with("interface not found:") + || message.starts_with("invalid interface name:") + ), + "unexpected result for {interface:?}: {:?}", + result.as_ref().map(|_| "PacketSocket") + ); } } assert_eq!(open_fds(), before, "failed setup leaked a socket"); From 712faebddd68a242a757148f0d6d17370f53cdcc Mon Sep 17 00:00:00 2001 From: Martti Malmi Date: Tue, 29 Sep 2026 11:20:12 +0300 Subject: [PATCH 10/13] fix(tcp): try remaining DNS addresses after connection failure TCP connections failed when the first resolved address was unreachable even if another address worked. Retain all resolver candidates for both connection paths and let Tokio try them within one connection timeout. Cover fallback with a real listener and exercise hostname connections through connect-on-send and background connect. --- CHANGELOG.md | 2 + src/transport/mod.rs | 31 +++---- src/transport/tcp/mod.rs | 188 ++++++++++++++++++++++++++++----------- 3 files changed, 153 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c46d2eb3..5866b61e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 reopened after the interface goes away. A running node whose interface was removed and not recreated retried the reopen on every beacon interval and leaked one descriptor each time. +- TCP connections try the remaining addresses for a hostname after a + connection fails, within the existing overall connection timeout. ## [0.5.2] - 2026-09-28 diff --git a/src/transport/mod.rs b/src/transport/mod.rs index bb39a6d3..bc839500 100644 --- a/src/transport/mod.rs +++ b/src/transport/mod.rs @@ -1082,28 +1082,23 @@ impl TransportHandle { pub(crate) async fn resolve_socket_addr( addr: &TransportAddr, ) -> Result { + resolve_socket_addrs(addr).await?.next().ok_or_else(|| { + TransportError::InvalidAddress(format!("DNS resolution returned no addresses for {}", addr)) + }) +} + +/// Resolve every socket address in resolver order, bypassing DNS for numeric IPs. +pub(crate) async fn resolve_socket_addrs( + addr: &TransportAddr, +) -> Result, TransportError> { let s = addr .as_str() .ok_or_else(|| TransportError::InvalidAddress("not valid UTF-8".into()))?; - // Fast path: numeric IP address — no DNS lookup - if let Ok(sock_addr) = s.parse::() { - return Ok(sock_addr); - } - - // Slow path: DNS resolution - tokio::net::lookup_host(s) - .await - .map_err(|e| { - TransportError::InvalidAddress(format!("DNS resolution failed for {}: {}", s, e)) - })? - .next() - .ok_or_else(|| { - TransportError::InvalidAddress(format!( - "DNS resolution returned no addresses for {}", - s - )) - }) + // lookup_host handles numeric addresses without allocating or querying DNS. + tokio::net::lookup_host(s).await.map_err(|e| { + TransportError::InvalidAddress(format!("DNS resolution failed for {}: {}", s, e)) + }) } // ============================================================================ diff --git a/src/transport/tcp/mod.rs b/src/transport/tcp/mod.rs index e74f640a..9b9d87bc 100644 --- a/src/transport/tcp/mod.rs +++ b/src/transport/tcp/mod.rs @@ -25,7 +25,7 @@ mod pool; pub mod stats; -use super::resolve_socket_addr; +use super::resolve_socket_addrs; use super::{ ConnectionState, DiscoveredPeer, PacketTx, ReceivedPacket, Transport, TransportAddr, TransportError, TransportId, TransportState, TransportType, @@ -379,25 +379,20 @@ impl TcpTransport { &self, addr: &TransportAddr, ) -> Result>, TransportError> { - let socket_addr = resolve_socket_addr(addr).await?; + let socket_addrs: Vec<_> = resolve_socket_addrs(addr).await?.collect(); let timeout_ms = self.config.connect_timeout_ms(); - // Connect with timeout - let stream = match tokio::time::timeout( - Duration::from_millis(timeout_ms), - TcpStream::connect(socket_addr), - ) - .await - { - Ok(Ok(stream)) => stream, - Ok(Err(_)) => { + let stream = match connect_to_any_addr(&socket_addrs, timeout_ms).await { + Ok(stream) => stream, + Err(error @ TransportError::ConnectionRefused) => { self.stats.record_connect_refused(); - return Err(TransportError::ConnectionRefused); + return Err(error); } - Err(_) => { + Err(error @ TransportError::Timeout) => { self.stats.record_connect_timeout(); - return Err(TransportError::Timeout); + return Err(error); } + Err(error) => return Err(error), }; // Configure socket options via socket2 @@ -518,10 +513,8 @@ impl TcpTransport { } // Validate address is UTF-8 before spawning (fail fast on bad input) - let addr_string = addr - .as_str() - .ok_or_else(|| TransportError::InvalidAddress("not valid UTF-8".into()))? - .to_string(); + addr.as_str() + .ok_or_else(|| TransportError::InvalidAddress("not valid UTF-8".into()))?; let timeout_ms = self.config.connect_timeout_ms(); let config = self.config.clone(); let transport_id = self.transport_id; @@ -536,51 +529,28 @@ impl TcpTransport { let task = tokio::spawn(async move { // Resolve address (may involve DNS for hostnames) - let socket_addr: SocketAddr = if let Ok(sa) = addr_string.parse() { - sa - } else { - tokio::net::lookup_host(&addr_string) - .await - .map_err(|e| { - TransportError::InvalidAddress(format!( - "DNS resolution failed for {}: {}", - addr_string, e - )) - })? - .next() - .ok_or_else(|| { - TransportError::InvalidAddress(format!( - "DNS resolution returned no addresses for {}", - addr_string - )) - })? - }; + let socket_addrs: Vec<_> = resolve_socket_addrs(&remote_addr).await?.collect(); - // Connect with timeout - let stream = match tokio::time::timeout( - Duration::from_millis(timeout_ms), - TcpStream::connect(socket_addr), - ) - .await - { - Ok(Ok(stream)) => stream, - Ok(Err(e)) => { + let stream = match connect_to_any_addr(&socket_addrs, timeout_ms).await { + Ok(stream) => stream, + Err(error @ TransportError::ConnectionRefused) => { debug!( transport_id = %transport_id, remote_addr = %remote_addr, - error = %e, + error = %error, "Background TCP connect refused" ); - return Err(TransportError::ConnectionRefused); + return Err(error); } - Err(_) => { + Err(error @ TransportError::Timeout) => { debug!( transport_id = %transport_id, remote_addr = %remote_addr, "Background TCP connect timed out" ); - return Err(TransportError::Timeout); + return Err(error); } + Err(error) => return Err(error), }; // Configure socket options via socket2 @@ -1123,6 +1093,32 @@ async fn tcp_receive_loop( // Socket Configuration Helpers // ============================================================================ +async fn connect_to_any_addr( + socket_addrs: &[SocketAddr], + timeout_ms: u64, +) -> Result { + if socket_addrs.is_empty() { + return Err(TransportError::InvalidAddress( + "DNS resolution returned no addresses".into(), + )); + } + + // Try candidates in resolver order within one overall connection timeout. + match tokio::time::timeout( + Duration::from_millis(timeout_ms), + TcpStream::connect(socket_addrs), + ) + .await + { + Ok(Ok(stream)) => Ok(stream), + Ok(Err(error)) => { + trace!(error = %error, "TCP connection candidates failed"); + Err(TransportError::ConnectionRefused) + } + Err(_) => Err(TransportError::Timeout), + } +} + /// Configure a TCP socket with the transport's settings. fn configure_socket( stream: &std::net::TcpStream, @@ -1264,6 +1260,98 @@ mod tests { } } + #[tokio::test] + async fn test_connect_tries_later_candidates() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let good_addr = listener.local_addr().unwrap(); + + // No listener can bind port zero: binding it allocates an ephemeral port. + let bad_addr = "127.0.0.1:0".parse().unwrap(); + + let stream = connect_to_any_addr(&[bad_addr, good_addr], 1_000) + .await + .expect("second TCP candidate should connect"); + let (accepted, _) = timeout(Duration::from_secs(1), listener.accept()) + .await + .unwrap() + .unwrap(); + assert_eq!(stream.peer_addr().unwrap(), good_addr); + assert_eq!(accepted.peer_addr().unwrap(), stream.local_addr().unwrap()); + } + + #[tokio::test] + async fn test_connect_candidates_fail() { + assert!(matches!( + connect_to_any_addr(&[], 1_000).await, + Err(TransportError::InvalidAddress(_)) + )); + assert!(matches!( + connect_to_any_addr(&["127.0.0.1:0".parse().unwrap()], 1_000).await, + Err(TransportError::ConnectionRefused) + )); + } + + async fn send_to_hostname(background: bool) { + // Prefer the last localhost address so dual-stack hosts also + // exercise fallback through the full transport connection path. + let mut addresses: Vec<_> = tokio::net::lookup_host("localhost:0") + .await + .unwrap() + .collect(); + addresses.reverse(); + let listener = TcpListener::bind(addresses.as_slice()).await.unwrap(); + let (tx, _rx) = packet_channel(10); + let config = make_outbound_config(); + // A refused localhost connection can take over two seconds on Windows. + // Allow the configured connection timeout, plus DNS/scheduling margin. + let connect_deadline = + Duration::from_millis(config.connect_timeout_ms()) + Duration::from_secs(2); + let mut sender = TcpTransport::new(TransportId::new(1), None, config, tx); + sender.start_async().await.unwrap(); + let remote = TransportAddr::from_string(&format!( + "localhost:{}", + listener.local_addr().unwrap().port() + )); + + if background { + sender.connect_async(&remote).await.unwrap(); + assert!( + wait_until( + || sender.connection_state_sync(&remote) == ConnectionState::Connected, + connect_deadline, + ) + .await + ); + } + timeout( + connect_deadline, + sender.send_async(&remote, &build_msg1_frame()), + ) + .await + .expect("hostname send exceeded connection deadline") + .unwrap(); + let (mut stream, _) = timeout(Duration::from_secs(2), listener.accept()) + .await + .unwrap() + .unwrap(); + let packet = timeout(Duration::from_secs(2), read_fmp_packet(&mut stream, 1400)) + .await + .unwrap() + .unwrap(); + assert_eq!(packet, build_msg1_frame()); + sender.stop_async().await.unwrap(); + } + + #[tokio::test] + async fn test_connect_on_send_hostname() { + send_to_hostname(false).await; + } + + #[tokio::test] + async fn test_connect_async_hostname() { + send_to_hostname(true).await; + } + /// Listener on every local address, so that two connections can reach /// it from one source port on two different local addresses. fn wildcard_config() -> TcpConfig { From af40a158fe8539b91d2a56f2cd33dd77ef41a3c9 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 30 Sep 2026 17:24:41 +0000 Subject: [PATCH 11/13] Log the OS error and peer when a TCP connect fails Trying every resolved address moved the connect into connect_to_any_addr, which maps any I/O failure to ConnectionRefused and kept the real error only at trace level with no transport or peer fields. The background connect's debug line then printed the variant, whose text is always "connection refused", so an operator at debug level saw that for "No route to host" and "Network is unreachable" too, including for a single numeric address. connect_to_any_addr now takes the transport id and the peer address, logs the last candidate's OS error at debug level with both, and names the peer in its no-addresses error as the resolver did before. The background path's own refusal line is dropped, since it would repeat the same failure without the error. Connect-on-send, which logged nothing on a refusal, now gets the same debug line. A test captures the log and requires the debug line to carry the OS error, transport id and peer. It registers the log callsite before installing the capture, because a parallel test that registers it first while the capture is the only subscriber caches its own thread's "never" interest and the event is lost. --- src/testutil.rs | 5 +++ src/transport/tcp/mod.rs | 81 +++++++++++++++++++++++++++++----------- 2 files changed, 65 insertions(+), 21 deletions(-) diff --git a/src/testutil.rs b/src/testutil.rs index 65874819..f55e42e8 100644 --- a/src/testutil.rs +++ b/src/testutil.rs @@ -19,6 +19,11 @@ pub(crate) fn make_node_addr(val: u8) -> NodeAddr { pub(crate) struct LogCapture(std::sync::Arc>>); impl LogCapture { + /// Every captured line, each prefixed with its level. + pub(crate) fn lines(&self) -> Vec { + self.0.lock().unwrap().clone() + } + /// Only the captured lines emitted at WARN. pub(crate) fn warnings(&self) -> Vec { self.0 diff --git a/src/transport/tcp/mod.rs b/src/transport/tcp/mod.rs index 9b9d87bc..e85940ff 100644 --- a/src/transport/tcp/mod.rs +++ b/src/transport/tcp/mod.rs @@ -382,7 +382,9 @@ impl TcpTransport { let socket_addrs: Vec<_> = resolve_socket_addrs(addr).await?.collect(); let timeout_ms = self.config.connect_timeout_ms(); - let stream = match connect_to_any_addr(&socket_addrs, timeout_ms).await { + let connected = + connect_to_any_addr(self.transport_id, addr, &socket_addrs, timeout_ms).await; + let stream = match connected { Ok(stream) => stream, Err(error @ TransportError::ConnectionRefused) => { self.stats.record_connect_refused(); @@ -531,17 +533,11 @@ impl TcpTransport { // Resolve address (may involve DNS for hostnames) let socket_addrs: Vec<_> = resolve_socket_addrs(&remote_addr).await?.collect(); - let stream = match connect_to_any_addr(&socket_addrs, timeout_ms).await { + // A refusal is logged with its OS error by connect_to_any_addr. + let connected = + connect_to_any_addr(transport_id, &remote_addr, &socket_addrs, timeout_ms).await; + let stream = match connected { Ok(stream) => stream, - Err(error @ TransportError::ConnectionRefused) => { - debug!( - transport_id = %transport_id, - remote_addr = %remote_addr, - error = %error, - "Background TCP connect refused" - ); - return Err(error); - } Err(error @ TransportError::Timeout) => { debug!( transport_id = %transport_id, @@ -1093,14 +1089,18 @@ async fn tcp_receive_loop( // Socket Configuration Helpers // ============================================================================ +/// Connect to the first reachable of a peer's resolved addresses within one timeout. async fn connect_to_any_addr( + transport_id: TransportId, + remote_addr: &TransportAddr, socket_addrs: &[SocketAddr], timeout_ms: u64, ) -> Result { if socket_addrs.is_empty() { - return Err(TransportError::InvalidAddress( - "DNS resolution returned no addresses".into(), - )); + return Err(TransportError::InvalidAddress(format!( + "DNS resolution returned no addresses for {}", + remote_addr + ))); } // Try candidates in resolver order within one overall connection timeout. @@ -1112,7 +1112,14 @@ async fn connect_to_any_addr( { Ok(Ok(stream)) => Ok(stream), Ok(Err(error)) => { - trace!(error = %error, "TCP connection candidates failed"); + // Tokio returns the last candidate's error; keep it in the log, + // since the returned variant does not carry it. + debug!( + transport_id = %transport_id, + remote_addr = %remote_addr, + error = %error, + "TCP connect failed" + ); Err(TransportError::ConnectionRefused) } Err(_) => Err(TransportError::Timeout), @@ -1268,9 +1275,11 @@ mod tests { // No listener can bind port zero: binding it allocates an ephemeral port. let bad_addr = "127.0.0.1:0".parse().unwrap(); - let stream = connect_to_any_addr(&[bad_addr, good_addr], 1_000) - .await - .expect("second TCP candidate should connect"); + let remote = TransportAddr::from_string(&format!("localhost:{}", good_addr.port())); + let stream = + connect_to_any_addr(TransportId::new(1), &remote, &[bad_addr, good_addr], 1_000) + .await + .expect("second TCP candidate should connect"); let (accepted, _) = timeout(Duration::from_secs(1), listener.accept()) .await .unwrap() @@ -1281,16 +1290,46 @@ mod tests { #[tokio::test] async fn test_connect_candidates_fail() { + let id = TransportId::new(1); + let remote = TransportAddr::from_string("localhost:0"); assert!(matches!( - connect_to_any_addr(&[], 1_000).await, - Err(TransportError::InvalidAddress(_)) + connect_to_any_addr(id, &remote, &[], 1_000).await, + Err(TransportError::InvalidAddress(ref message)) if message.ends_with("localhost:0") )); assert!(matches!( - connect_to_any_addr(&["127.0.0.1:0".parse().unwrap()], 1_000).await, + connect_to_any_addr(id, &remote, &["127.0.0.1:0".parse().unwrap()], 1_000).await, Err(TransportError::ConnectionRefused) )); } + #[tokio::test] + async fn test_connect_failure_logs_os_error_with_peer() { + let remote = TransportAddr::from_string("localhost:0"); + let refused = ["127.0.0.1:0".parse().unwrap()]; + // Register the log callsite before installing the capture. Other + // tests reach it in parallel, and one that registers it first while + // this capture is the only subscriber caches its own thread's "never" + // interest; installing the capture rebuilds already-registered + // callsites, so registering first makes the capture see the event. + let _ = connect_to_any_addr(TransportId::new(7), &remote, &refused, 1_000).await; + let (logs, guard) = crate::testutil::capture_logs_scoped(); + let result = connect_to_any_addr(TransportId::new(7), &remote, &refused, 1_000).await; + drop(guard); + + assert!(matches!(result, Err(TransportError::ConnectionRefused))); + // The returned variant always reads "connection refused"; the log + // line must carry the OS error and the peer instead. + let lines = logs.lines(); + assert!( + lines.iter().any(|line| line.starts_with("DEBUG") + && line.contains("TCP connect failed") + && line.contains("transport_id=transport:7") + && line.contains("remote_addr=localhost:0") + && line.contains("os error")), + "expected a debug line with the OS error and peer, got {lines:?}", + ); + } + async fn send_to_hostname(background: bool) { // Prefer the last localhost address so dual-stack hosts also // exercise fallback through the full transport connection path. From f1997d7f78008e083d2e4d6e3f98d8d3f6fffcf8 Mon Sep 17 00:00:00 2001 From: Martti Malmi Date: Tue, 29 Sep 2026 11:31:13 +0300 Subject: [PATCH 12/13] 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(); From 390acf6d4e1d3f2976964b1ec098faa2fb0b4aa0 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 30 Sep 2026 17:46:32 +0000 Subject: [PATCH 13/13] Test that a gateway NAT rebuild clears its pending mark, and log retry failures once Nothing CI runs observed the pending mark being cleared on success: only the ignored kernel test reached that line, so a gateway that never cleared it would rebuild its whole table and log every ten seconds without a test going red. The mark is now set and cleared in track_rebuild, which takes the rebuild as a closure, and unprivileged tests drive it with a failing and a succeeding stand-in to check that a failure leaves the retry pending and a success leaves retry_pending nothing to do. The coverage note above the tests says what remains only in the ignored test. A failure that persists, such as the kernel refusing the batch while a module or capability is missing, warned on every retry, six lines a minute for the life of the process. The retry now warns when its failure changes, logs repeats at debug level, and reports recovery once, whether the retry applied the rules or a later mapping change did, following the conntrack read log's pattern. rebuild_desired gets a doc comment. --- src/bin/fips-gateway.rs | 31 +++++++-- src/gateway/nat.rs | 143 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 168 insertions(+), 6 deletions(-) diff --git a/src/bin/fips-gateway.rs b/src/bin/fips-gateway.rs index 60173841..4fa248c7 100644 --- a/src/bin/fips-gateway.rs +++ b/src/bin/fips-gateway.rs @@ -107,6 +107,30 @@ fn report_unreadable_conntrack( } } +/// Log a pending NAT rebuild retry once per change of outcome. +/// +/// A recovery is reported whether the retry applied the rules itself or a +/// mapping change applied them in between and left nothing pending. +#[cfg(target_os = "linux")] +fn report_nat_retry(log: &mut nat::RetryLog, result: &Result) { + match (log.observe(result.as_ref().err()), result) { + (nat::RetryReport::Failed, Err(e)) => { + warn!(error = %e, "Failed to retry pending NAT rules") + } + (nat::RetryReport::Repeated, Err(e)) => { + debug!(error = %e, "Pending NAT rules still failing to apply") + } + (nat::RetryReport::Recovered, Ok(true)) => { + info!("Applied pending NAT rules; retries recovered") + } + (nat::RetryReport::Recovered, _) => { + info!("Pending NAT rules were applied by a later update; retries recovered") + } + (nat::RetryReport::Clean, Ok(true)) => info!("Applied pending NAT rules"), + _ => {} + } +} + /// Check once at startup which conntrack source the tick will read, and say so. /// /// Without this, an operator on a kernel with no readable source learns that @@ -547,14 +571,11 @@ async fn main() { 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); + let mut nat_retry_log = nat::RetryLog::default(); 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"), - } + report_nat_retry(&mut nat_retry_log, &nat_mgr.retry_pending()); } Some(event) = event_rx.recv() => { match event { diff --git a/src/gateway/nat.rs b/src/gateway/nat.rs index e7073c27..72de3a89 100644 --- a/src/gateway/nat.rs +++ b/src/gateway/nat.rs @@ -519,14 +519,80 @@ impl NatManager { (result, elapsed_us) } + /// Rebuild the desired state, leaving it marked pending unless the kernel + /// accepts it. fn rebuild_desired(&mut self) -> Result<(), NatError> { + self.track_rebuild(Self::rebuild) + } + + /// Run `apply` with the desired state marked pending, and clear the mark + /// only if it succeeds. + /// + /// Split from `rebuild_desired` so a test can drive both outcomes without + /// a netlink socket. + fn track_rebuild( + &mut self, + apply: impl FnOnce(&Self) -> Result<(), NatError>, + ) -> Result<(), NatError> { self.rebuild_pending = true; - self.rebuild()?; + apply(self)?; self.rebuild_pending = false; Ok(()) } } +/// How a pending-rebuild retry compares with the retry before it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RetryReport { + /// A failure that differs from the previous retry's outcome, or the first. + Failed, + /// The same failure as the previous retry. + Repeated, + /// The first success after a failed retry. + Recovered, + /// A success with no failed retry before it. + Clean, +} + +/// Remembers the last failed retry of a pending rebuild. +/// +/// A failure that does not clear on its own, for example the kernel refusing +/// the batch while a module or capability is missing, fails every retry the +/// same way, and a per-retry warning would repeat for the life of the +/// process. Warning on a change of outcome, and reporting the recovery once, +/// keeps the log readable. A retry that finds nothing pending after a failure +/// counts as recovery, since a mapping change rebuilt the table in between. +#[derive(Debug, Default)] +pub struct RetryLog { + last_failure: Option, +} + +impl RetryLog { + /// Record a retry outcome and say how it compares with the last one. + /// + /// `None` is a success, whether or not anything was pending; `Some` is + /// the retry's error. Two failures are the same when their messages are. + pub fn observe(&mut self, failure: Option<&NatError>) -> RetryReport { + match (failure.map(ToString::to_string), self.last_failure.take()) { + (Some(now), Some(before)) => { + let report = if now == before { + RetryReport::Repeated + } else { + RetryReport::Failed + }; + self.last_failure = Some(now); + report + } + (Some(now), None) => { + self.last_failure = Some(now); + RetryReport::Failed + } + (None, Some(_)) => RetryReport::Recovered, + (None, None) => RetryReport::Clean, + } + } +} + /// Largest batch, in bytes, that the kernel can admit in one send. fn admissible_limit() -> u64 { 2 * MAX_SNDBUF as u64 - SNDBUF_OVERHEAD @@ -877,6 +943,13 @@ fn send_batch(bytes: &[u8]) -> Result<(), NatError> { // `SO_SNDBUF` fallback after `SO_SNDBUFFORCE` returns `EPERM` needs a process // without CAP_NET_ADMIN, and the gateway suite's container is privileged, so // nothing runs that either. The gateway suite covers only the success path. +// +// The pending-rebuild flag is covered here on both sides through +// `track_rebuild`, with a stand-in for the rebuild: a failure leaves it set and +// `retry_pending` still retries, and a success clears it so `retry_pending` +// finds nothing to do. What stays only in the ignored kernel test, which no +// suite runs, is `retry_pending` itself applying a pending rebuild and +// returning `Ok(true)`, and a kernel rejection leaving the old table in place. #[cfg(test)] mod tests { use super::*; @@ -927,6 +1000,74 @@ mod tests { ); } + #[test] + fn failed_tracked_rebuild_stays_pending_and_retry_still_rebuilds() { + let mut mgr = manager_with_mappings(1); + let refused = || NatError::Nftables("refused".into()); + + assert!(mgr.track_rebuild(|_| Err(refused())).is_err()); + assert!(mgr.rebuild_pending, "a failed rebuild must stay pending"); + + // With encoding made to fail, a retry that is still pending attempts + // the rebuild, and fails, instead of reporting nothing to do. + mgr.pre_chain = Chain::new(&mgr.table); + assert!(mgr.retry_pending().is_err()); + assert!(mgr.rebuild_pending); + } + + #[test] + fn successful_tracked_rebuild_clears_pending_and_retry_does_nothing() { + let mut mgr = manager_with_mappings(1); + assert!( + mgr.track_rebuild(|_| Err(NatError::Nftables("refused".into()))) + .is_err() + ); + assert!(mgr.rebuild_pending); + + let mut applied = 0; + mgr.track_rebuild(|m| { + assert!(m.rebuild_pending, "the rebuild runs with the mark set"); + applied += 1; + Ok(()) + }) + .unwrap(); + assert_eq!(applied, 1); + assert!( + !mgr.rebuild_pending, + "a successful rebuild must clear pending" + ); + + // Encoding would fail, so `Ok(false)` shows the retry did not rebuild. + mgr.pre_chain = Chain::new(&mgr.table); + assert!(!mgr.retry_pending().unwrap()); + } + + #[test] + fn retry_log_warns_on_a_new_failure_and_reports_recovery_once() { + let kernel = |errno| NatError::Kernel { + errno: Errno(errno), + seq: 3, + }; + let mut log = RetryLog::default(); + + assert_eq!(log.observe(None), RetryReport::Clean); + assert_eq!(log.observe(Some(&kernel(22))), RetryReport::Failed); + assert_eq!(log.observe(Some(&kernel(22))), RetryReport::Repeated); + assert_eq!(log.observe(Some(&kernel(22))), RetryReport::Repeated); + assert_eq!( + log.observe(Some(&kernel(1))), + RetryReport::Failed, + "a different failure is a different outcome and is worth a line" + ); + assert_eq!(log.observe(None), RetryReport::Recovered); + assert_eq!(log.observe(None), RetryReport::Clean); + assert_eq!( + log.observe(Some(&kernel(1))), + RetryReport::Failed, + "a failure after recovery warns again" + ); + } + #[test] fn failed_port_forward_rebuild_remains_pending() { let mut mgr = manager_with_mappings(1);