Replace restart with in-process config reload #29

New issue

Closed

opened 2026-07-07 13:16:34 +01:00 by danny · 2 comments

danny commented

2026-07-07 13:16:34 +01:00

(Migrated from gitlab.keyop.co.uk)

Currently we rather awkwardly rely on using systemd to restart the process when we reload configuration after changes in the UI.

We should support a config reload option, which brings running configuration in line with the stored configuration, taking down any tunnels which are no longer defined and making sure that all firewall rules are cleaned up or created as needed.

This should be used in the UI reload options as well as being available via a signal (SIGHUP is traditional for this). The systemd unit can then also be updated to support the reload option.

Currently we rather awkwardly rely on using systemd to restart the process when we reload configuration after changes in the UI. We should support a config reload option, which brings running configuration in line with the stored configuration, taking down any tunnels which are no longer defined and making sure that all firewall rules are cleaned up or created as needed. This should be used in the UI reload options as well as being available via a signal (SIGHUP is traditional for this). The systemd unit can then also be updated to support the reload option.

danny commented

2026-07-07 13:18:39 +01:00

(Migrated from gitlab.keyop.co.uk)

changed the description

claude commented

2026-07-09 18:51:55 +01:00

Collaborator

Plan

Today nfq_forwarder reads --config exactly once at startup. The web UI's config-edit feature
(#25) validates and writes a new file to disk but never applies it - the only way to pick up a
change today is POST /api/config/restart, which shells out to systemctl --user restart --no-block nfq_forwarder.service (a full process restart). This ticket replaces that with a real
in-process reload, plus SIGHUP support and a systemd ExecReload update.

Design

New connectionManager type (internal/nfq_forwarder/connection_manager.go) tracks each
running connection by queue number - the stable identity issue #20's name-hash-derived
auto-assignment already establishes, and the same identity deleteNfqIptablesRulesFunc's
crash-recovery cleanup keys on.

reload(newConns):

Diff currently-running queue numbers against the newly loaded config's queue numbers.
- In old, not in new → stop (cancel that connection's own child context, wait for its
  existing teardown path - deleteNfqIptablesRulesFunc + sshConns.closeAll() - to finish).
- In both, but the Connection struct differs (jump host, network, ports, etc.) → stop and
  restart with the new details.
- In both, unchanged → left alone entirely - its sshConns pool and any open tunnels on
  it are untouched by an unrelated add/remove elsewhere in the same reload.
- In new, not in old → start.
Returns a summary (added/removed/restarted/unchanged connection labels) for logging and for
the web UI response.

A subtle correctness point worth calling out up front: NFQForwarderMain needs one blocking call
that waits for every connection goroutine to exit at shutdown, including ones started by a later
reload. A bare sync.WaitGroup.Wait() isn't safe for that on its own, since a reload's
stop-then-start sequence can transiently drop the counter to zero before repopulating it, and
Go's WaitGroup permits Wait() to return right at that zero-crossing. Fix: reload() refuses to
start anything once the manager's parent context is already cancelled (shutdown in progress), and
the shutdown wait blocks on that context being done before calling wg.Wait() - so by the time
wg.Wait() runs, the counter can only ever decrease. This gets a direct regression test.

SIGHUP: added to initSignalHandler alongside the existing SIGINT/SIGTERM/SIGUSR1, calling
the same reload closure the web UI uses - one reload code path, not two.

Web UI: POST /api/config/restart (handleAPIConfigRestart, restartServiceFunc,
os/exec) is removed entirely and replaced with POST /api/config/reload, going through the
same closure. The config-edit page's "Restart service" button becomes "Reload configuration" and
shows a summary of what changed.

systemd unit: ExecReload= changes from systemctl --user restart ... to the standard
kill -HUP $MAINPID idiom. danny/scripts' nfqctl reload already just delegates to
systemctl --user reload, so it needs no change - it'll pick up the new SIGHUP-based behavior
automatically.

Scope boundary (documented explicitly, not just implied): reload only reconciles
connections: - added/removed/changed tunnels and their firewall rules. It does not restart
the HTTP listener even if web_ui: settings changed in the file; that still needs a real
restart (unchanged from today).

Testing

New connection_manager_test.go covering add/remove/restart-in-place/no-op-when-unchanged, plus
the shutdown-guard regression test above, using this codebase's existing
initNfqueueFunc/sshDialFunc-style stubbing seams (no real iptables/netlink/SSH). SIGHUP gets a
real-signal-to-this-process test matching the existing SIGUSR1/SIGTERM ones. The three
TestAPIConfigRestart* web UI tests become TestAPIConfigReload* equivalents. Real-host
verification (SIGHUP against an installed systemd unit; a UI reload actually removing/adding
iptables rules against live traffic) isn't exercisable in this sandbox and will be flagged as an
unverified item, matching every other privileged-network feature in this project so far.

Branch: issue29_config_reload.

## Plan Today `nfq_forwarder` reads `--config` exactly once at startup. The web UI's config-edit feature (#25) validates and writes a new file to disk but never applies it - the only way to pick up a change today is `POST /api/config/restart`, which shells out to `systemctl --user restart --no-block nfq_forwarder.service` (a full process restart). This ticket replaces that with a real in-process reload, plus SIGHUP support and a systemd `ExecReload` update. ### Design **New `connectionManager` type** (`internal/nfq_forwarder/connection_manager.go`) tracks each running connection by **queue number** - the stable identity issue #20's name-hash-derived auto-assignment already establishes, and the same identity `deleteNfqIptablesRulesFunc`'s crash-recovery cleanup keys on. `reload(newConns)`: 1. Diff currently-running queue numbers against the newly loaded config's queue numbers. - In old, not in new → **stop** (cancel that connection's own child context, wait for its existing teardown path - `deleteNfqIptablesRulesFunc` + `sshConns.closeAll()` - to finish). - In both, but the `Connection` struct differs (jump host, network, ports, etc.) → **stop and restart** with the new details. - In both, unchanged → **left alone entirely** - its `sshConns` pool and any open tunnels on it are untouched by an unrelated add/remove elsewhere in the same reload. - In new, not in old → **start**. 2. Returns a summary (added/removed/restarted/unchanged connection labels) for logging and for the web UI response. A subtle correctness point worth calling out up front: `NFQForwarderMain` needs one blocking call that waits for every connection goroutine to exit at shutdown, including ones started by a later reload. A bare `sync.WaitGroup.Wait()` isn't safe for that on its own, since a reload's stop-then-start sequence can transiently drop the counter to zero before repopulating it, and Go's WaitGroup permits `Wait()` to return right at that zero-crossing. Fix: `reload()` refuses to start anything once the manager's parent context is already cancelled (shutdown in progress), and the shutdown wait blocks on that context being done *before* calling `wg.Wait()` - so by the time `wg.Wait()` runs, the counter can only ever decrease. This gets a direct regression test. **SIGHUP**: added to `initSignalHandler` alongside the existing SIGINT/SIGTERM/SIGUSR1, calling the same reload closure the web UI uses - one reload code path, not two. **Web UI**: `POST /api/config/restart` (`handleAPIConfigRestart`, `restartServiceFunc`, `os/exec`) is removed entirely and replaced with `POST /api/config/reload`, going through the same closure. The config-edit page's "Restart service" button becomes "Reload configuration" and shows a summary of what changed. **systemd unit**: `ExecReload=` changes from `systemctl --user restart ...` to the standard `kill -HUP $MAINPID` idiom. `danny/scripts`' `nfqctl reload` already just delegates to `systemctl --user reload`, so it needs no change - it'll pick up the new SIGHUP-based behavior automatically. **Scope boundary** (documented explicitly, not just implied): reload only reconciles `connections:` - added/removed/changed tunnels and their firewall rules. It does **not** restart the HTTP listener even if `web_ui:` settings changed in the file; that still needs a real restart (unchanged from today). ### Testing New `connection_manager_test.go` covering add/remove/restart-in-place/no-op-when-unchanged, plus the shutdown-guard regression test above, using this codebase's existing `initNfqueueFunc`/`sshDialFunc`-style stubbing seams (no real iptables/netlink/SSH). SIGHUP gets a real-signal-to-this-process test matching the existing SIGUSR1/SIGTERM ones. The three `TestAPIConfigRestart*` web UI tests become `TestAPIConfigReload*` equivalents. Real-host verification (SIGHUP against an installed systemd unit; a UI reload actually removing/adding iptables rules against live traffic) isn't exercisable in this sandbox and will be flagged as an unverified item, matching every other privileged-network feature in this project so far. Branch: `issue29_config_reload`.

claude added the

In Progress

label

2026-07-09 18:52:07 +01:00

claude referenced this issue from a commit

2026-07-09 19:23:37 +01:00

Replace service restart with in-process config reload

claude referenced this issue from a pull request that will close it,

2026-07-09 19:23:52 +01:00

Replace service restart with in-process config reload #84