Skip to content

fix(spur-net): persist WireGuard peer mutations to the interface config - #792

Open
yansun1996 wants to merge 5 commits into
ROCm:mainfrom
yansun1996:fix/wg-peer-reboot
Open

fix(spur-net): persist WireGuard peer mutations to the interface config#792
yansun1996 wants to merge 5 commits into
ROCm:mainfrom
yansun1996:fix/wg-peer-reboot

Conversation

@yansun1996

@yansun1996 yansun1996 commented Sep 2, 2026

Copy link
Copy Markdown
Member

What

spur net add-peer, remove-peer, and mesh mutated a running WireGuard interface live via wg set only — they never persisted the change to the interface's /etc/wireguard/<iface>.conf. Any subsequent interface reload (host reboot, wg-quick restart, crash) silently dropped every dynamically-added peer, since wg-quick up only replays the static file. This matched a report of a controller losing a worker's WireGuard peer from its active spur0 interface, breaking scheduler/agent traffic over the mesh, with no indication anything had changed.

Separately, under a SPUR-managed k0s cluster, the k0s reconcile loop's peer-pruning pass (reconcile_mesh) could remove a peer that was durably added by hand for something outside the k0s cluster's own membership (e.g. a non-k0s node) — it only knew about its own membership set, not what a human had explicitly configured. This produced the same symptom (peer live-pruned, still in the file) via a different mechanism.

Approach

  • Added WgConfig::parse (the inverse of the existing to_ini serializer) plus read_from/upsert_peer/remove_peer_by_key in spur-net.
  • Added add_peer_durable/remove_peer_durable (wireguard.rs) and apply_mesh_durable (mesh.rs): each persists the change to the config file before applying it live and while holding an advisory file lock across both steps, so a failed live apply still leaves the file correct, and two concurrent invocations against the same interface can't have their live and persisted state land in different orders.
  • WgConfig::write_to writes through a temp file, fsyncs it, renames over the target, then fsyncs the containing directory — durable, not just atomic, since this write path went from "once, at provisioning" to "on every peer mutation."
  • Wired a new --config-dir flag (default /etc/wireguard) through add-peer, remove-peer, and mesh; trimmed --key before use so shell-substitution whitespace can't cause a duplicate peer entry.
  • reconcile_mesh/peers_to_prune now take a protected key set, populated by spurd's ApplyMesh handler from the node's own persisted WireGuard config before reconciling: a peer present in that file was explicitly, durably configured by a human, so the k0s reconcile never removes it regardless of the pushed membership. Threaded a wg_config_dir (env SPUR_WG_CONFIG_DIR, default /etc/wireguard) through spurd alongside the existing wg_iface resolution.
  • Updated docs/deployment/wireguard.rst to document the new persistence behavior, --config-dir, and the reconcile-protection guarantee.

Known limitations

  • The k0s full-mesh reconcile's own peer pushes (as opposed to the protection check) remain live-only — it already self-heals every ~30s from live inventory, so persisting there would be redundant.

Testing

Unit tests cover the INI parser, upsert_peer/remove_peer_by_key, the durable write (atomicity, permissions, no leftover temp file), the persist-before-live-apply ordering (including missing-file and missing-directory idempotency), a concurrency test proving the advisory lock serializes competing critical sections, and the reconcile's protected-peer exemption. Each behavioral fix was verified red (reverting it) then green.

Live-tested end-to-end on an isolated 2-node deployment across the full transport × CNI matrix:

  • Direct transport + kuberouter: k0s cluster healthy, pod scheduling/exec verified (no mesh involved — regression check only).
  • Direct transport + calico: confirmed not viable independent of this PR — calico's control-plane address selection unconditionally advertises the mesh CIDR's .1 address regardless of wg_enabled, so without a mesh that address is unbound and the cluster never reaches a working state. Matches the existing docs, which already state calico requires the mesh; unrelated to this change, called out here for completeness.
  • WireGuard transport + calico: cluster healthy over the mesh; added a peer for a fake, out-of-cluster node, then confirmed it survived 4 consecutive ~30s k0s reconcile ticks in both wg show and the persisted config, with the real k0s peer's endpoint/pod-CIDR continuing to update normally alongside it.
  • WireGuard transport + kuberouter: same protected-peer verification, confirming the protection is independent of CNI choice (kuberouter doesn't route over the mesh, but the reconcile still manages spur0 peers whenever the mesh is enabled).

All isolated instances ran on non-default ports/interfaces/state dirs; production spurctld/spurd and all lab state were verified unaffected and fully cleaned up afterward.

cargo clippy --workspace --exclude spur-ffi --all-targets --locked and cargo fmt --all --check are clean; cargo build --workspace --exclude spur-ffi succeeds; targeted cargo test is clean across spur-net, spur-cli, and spurd.

`spur net add-peer`, `remove-peer`, and `mesh` mutated the running
WireGuard interface live via `wg set` only, never writing the change
to the interface's config file. Any subsequent interface reload
(wg-quick restart, host reboot, crash) silently dropped every
dynamically-added peer, since wg-quick only replays the static file.
The k0s full-mesh reconcile loop is unaffected — it already re-pushes
membership every ~30s and self-heals.

Adds a `WgConfig` INI parser (inverse of the existing serializer) plus
upsert/remove helpers, and durable add/remove/apply-mesh functions
that persist the change before applying it live, so a failed live
apply still leaves the file correct for the next reload or retry. The
persist step is guarded by an advisory file lock (and creates the
config directory if missing, so a never-initialized `--config-dir`
doesn't turn `remove-peer`'s idempotency into a hard error) to avoid
two concurrent CLI invocations clobbering each other, and config
writes go through a temp-file-plus-rename so a crash mid-write can't
corrupt the file wg-quick depends on for every future boot.

Tested: unit tests for the parser, upsert/remove, atomic write, and
the persist-before-live-apply ordering (including both missing-file
and missing-directory idempotency cases). Also validated end-to-end on
an isolated interface on real hardware: added a peer, confirmed it
landed in both the live interface and the config file, then simulated
a reboot (wg-quick down/up) and confirmed the peer survived — which
reproduces and fixes the exact reported failure mode.
@codecov-commenter

codecov-commenter commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.64730% with 74 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #792      +/-   ##
==========================================
+ Coverage   80.10%   80.21%   +0.11%     
==========================================
  Files         184      184              
  Lines       87409    88244     +835     
==========================================
+ Hits        70012    70781     +769     
- Misses      17397    17463      +66     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yansun1996
yansun1996 marked this pull request as ready for review September 2, 2026 01:18
Copilot AI lite review requested due to automatic review settings September 2, 2026 01:18
Atomic rename prevents a torn file but not data loss: the temp file's
content could still be sitting in the page cache when a crash hits,
and a bare rename is itself just a directory-entry update that can be
lost on some filesystems without fsyncing the containing directory.
fsync the temp file before the rename and the directory after, so the
write actually survives a power loss, not just looks atomic. Directory
fsync failures are logged, not fatal, since not every filesystem
supports it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new config file locking helper appears to use a non-existent File::lock() API and may drop the lock handle before executing the critical section, undermining both correctness and buildability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a durability gap in spur net WireGuard peer management: CLI-driven peer mutations (add-peer, remove-peer, mesh) are now intended to be persisted to the interface’s wg-quick config on disk so they survive reboot/interface reload, aligning spur-net behavior with operational expectations for non-daemon-driven mesh changes.

Changes:

  • Added parsing + read/modify/write helpers to spur-net WireGuard config handling, plus “durable” peer mutation functions that persist before applying live.
  • Added apply_mesh_durable to persist full-mesh peers in one RMW cycle, then apply live.
  • Threaded a new --config-dir flag through the CLI commands and updated WireGuard deployment docs to describe persistence.
File summaries
File Description
docs/deployment/wireguard.rst Documents that add-peer/remove-peer/mesh now persist results and introduces --config-dir.
crates/spur-net/src/wireguard.rs Adds config parser/RMW helpers, atomic write path, config file locking helper, and durable add/remove peer functions.
crates/spur-net/src/mesh.rs Adds apply_mesh_durable to persist peers before live application.
crates/spur-cli/src/net.rs Adds --config-dir to relevant subcommands and switches to durable persistence paths.
Review details

Suppressed comments (1)

crates/spur-net/src/wireguard.rs:238

  • On Unix, the temp file is created with default permissions and only later chmod’d to 0600. Because the file contains the private key, a crash between the write and set_permissions can leave a world-readable temp file behind. Create the temp file with 0600 at creation time (via OpenOptionsExt::mode) and write through the opened handle.
        let tmp_path = dir.join(format!(".{file_name}.tmp.{}", std::process::id()));

        let mut tmp_file = std::fs::File::create(&tmp_path).with_context(|| {
            format!(
                "failed to create temp WireGuard config at {}",
  • Files reviewed: 4/4 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/spur-net/src/wireguard.rs
Comment thread crates/spur-net/src/wireguard.rs
Comment thread crates/spur-net/src/wireguard.rs
add_peer_durable/remove_peer_durable/apply_mesh_durable released the
advisory file lock as soon as the config-file write finished, then
applied the same change live afterward. Two concurrent CLI invocations
against the same interface could have their live wg calls land in a
different order than their config writes, leaving the live interface
and the persisted file transiently out of sync with each other --
undermining the persist-matches-live invariant this is built on. Move
the live apply inside the locked closure so both halves are atomic
with respect to a concurrent invocation.

Also corrected a doc comment on WgConfig::parse that overstated a
manually-annotated file "still round-trips" -- parsing tolerates
comments/blank lines, but to_ini always rewrites a normalized file, so
they don't survive a subsequent write.
The k0s reconcile loop's ApplyMesh path (spurctld pushing membership to
spurd every ~30s) prunes any live peer not in its current k0s
membership -- including a peer an admin manually added via `spur net
add-peer` for something outside the k0s cluster (a non-k0s node, or a
peer staged ahead of that node formally joining). The prune only
touches live wg state, never the config file, so the pruned peer would
still show in the persisted config while missing from `wg show` -- the
same symptom as the bug this durability work already fixed, just via
a different mechanism.

Give reconcile_mesh/peers_to_prune a protected-keys set and have
spurd's apply_mesh handler populate it from the node's own persisted
WireGuard config before reconciling: a peer present in that file was
explicitly, durably configured by a human, so it's never this
reconcile's to remove regardless of the pushed membership.
Adds a native-host e2e test proving a peer wired up by net join/add-peer
survives wg-quick down/up (simulating a reboot or service restart), not
just the current live session -- both in the persisted config file and
in the live peer table afterward, with connectivity still working.
Also cleans up the advisory lock file the durable persistence path
creates, alongside the existing conf/interface teardown.

Verified against both the fixed and pre-fix binary: fails on the old
live-only add-peer/join, passes with the durable persistence fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants