keyop-go/nfq_forwarder
1
0
Fork 0
Code Issues 3 Pull requests 1 Projects Releases 21 Packages Wiki Activity Actions

Replace service restart with in-process config reload #84

Merged
danny merged 2 commits from issue29_config_reload into main 2026-07-09 19:53:17 +01:00
Conversation 3 Commits 2 Files changed 17 +714 -144
claude commented 2026-07-09 19:23:52 +01:00
Collaborator
Copy link

Closes #29.

  • New connectionManager (internal/nfq_forwarder/connection_manager.go) tracks each running connection by queue number - the same stable identity issue #20's auto-assignment establishes. reload(newConns) diffs the currently-running set against a freshly loaded config: removed connections are torn down (SSH tunnel closed, iptables rules removed), new ones are started, changed ones are stopped and restarted with the new details, and unchanged ones are left completely untouched - so an open tunnel on an unrelated connection survives a reload.
  • SIGHUP is wired into initSignalHandler alongside SIGINT/SIGTERM/SIGUSR1.
  • The web UI's old POST /api/config/restart (systemctl --user restart) is removed entirely and replaced with POST /api/config/reload, going through the exact same code path SIGHUP uses. The config-edit page's "Restart service" button is now "Apply configuration", distinct from the existing "Reload latest version" button (which discards an in-progress edit and re-fetches the on-disk file - unrelated to this feature).
  • docs/nfq_forwarder.service's ExecReload now sends SIGHUP instead of doing a full systemctl restart.
  • A shutdown-race guard: reload refuses to start anything once the manager's parent context is already cancelled, since a sync.WaitGroup's counter can legitimately hit zero mid-reload (stop phase before start phase) and a concurrent Wait() could return right at that crossing otherwise. Covered by a direct regression test.
  • Scope is deliberately limited to connections: - a change to web_ui: or any other top-level setting still needs a real restart.

Note for danny/scripts' nfqctl: no change needed there - nfqctl reload already just calls systemctl --user reload, which will now deliver SIGHUP instead of a full restart automatically.

make lint/make test clean. Manually verified via a real httptest.Server round-trip that POST /api/config/reload's response shape matches what config_edit.js expects and that the rendered page serves the new button. Real-host verification (a genuine SIGHUP/systemd reload against live iptables/SSH state) isn't exercisable in this sandbox - flagged as a new checklist item in CLAUDE.md's Known issues section.

Closes #29. - New `connectionManager` (`internal/nfq_forwarder/connection_manager.go`) tracks each running connection by queue number - the same stable identity issue #20's auto-assignment establishes. `reload(newConns)` diffs the currently-running set against a freshly loaded config: removed connections are torn down (SSH tunnel closed, iptables rules removed), new ones are started, changed ones are stopped and restarted with the new details, and unchanged ones are left completely untouched - so an open tunnel on an unrelated connection survives a reload. - `SIGHUP` is wired into `initSignalHandler` alongside SIGINT/SIGTERM/SIGUSR1. - The web UI's old `POST /api/config/restart` (`systemctl --user restart`) is removed entirely and replaced with `POST /api/config/reload`, going through the exact same code path SIGHUP uses. The config-edit page's "Restart service" button is now "Apply configuration", distinct from the existing "Reload latest version" button (which discards an in-progress edit and re-fetches the on-disk file - unrelated to this feature). - `docs/nfq_forwarder.service`'s `ExecReload` now sends `SIGHUP` instead of doing a full `systemctl restart`. - A shutdown-race guard: `reload` refuses to start anything once the manager's parent context is already cancelled, since a `sync.WaitGroup`'s counter can legitimately hit zero mid-reload (stop phase before start phase) and a concurrent `Wait()` could return right at that crossing otherwise. Covered by a direct regression test. - Scope is deliberately limited to `connections:` - a change to `web_ui:` or any other top-level setting still needs a real restart. Note for `danny/scripts`' `nfqctl`: no change needed there - `nfqctl reload` already just calls `systemctl --user reload`, which will now deliver SIGHUP instead of a full restart automatically. `make lint`/`make test` clean. Manually verified via a real httptest.Server round-trip that `POST /api/config/reload`'s response shape matches what `config_edit.js` expects and that the rendered page serves the new button. Real-host verification (a genuine SIGHUP/systemd reload against live iptables/SSH state) isn't exercisable in this sandbox - flagged as a new checklist item in CLAUDE.md's Known issues section.
Replace service restart with in-process config reload
All checks were successful
CI / lint (push) Successful in 24s
Details
CI / build (push) Successful in 28s
Details
CI / release (push) Has been skipped
Details
CI / lint (pull_request) Successful in 25s
Details
CI / build (pull_request) Successful in 27s
Details
CI / release (pull_request) Has been skipped
Details
06c878f84f 
Adds a connectionManager that tracks each running connection by queue
number, letting a config reload add, remove, or restart individual
connections without disturbing any that are unchanged (and therefore
without dropping their open SSH tunnels). Wired into SIGHUP and a new
web UI "Apply configuration" action, replacing the old
systemctl-restart-based flow entirely. docs/nfq_forwarder.service's
ExecReload now sends SIGHUP instead of restarting the whole service.

Closes #29

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
claude requested review from danny 2026-07-09 19:24:02 +01:00
danny scheduled this pull request to auto merge when all checks succeed 2026-07-09 19:34:23 +01:00
danny requested changes 2026-07-09 19:46:17 +01:00
Dismissed
danny left a comment
Copy link

A couple of comments to resolve.

A couple of comments to resolve.
docs/nfq_forwarder.service Outdated
@ -29,2 +23,2 @@
# a full restart converges exactly to whatever --config's connections list says - nothing from
# the old configuration is left behind, even if a connection was removed entirely.
# Reload (GitLab issue #29): ExecReload below sends SIGHUP, the traditional Unix signal for this
# - `systemctl --user reload` (and `nfqctl reload`, which just calls that) both trigger it with
danny commented 2026-07-09 19:37:30 +01:00
Owner
Copy link

nfqctl is a local script wrapper and not published to the wider audience for this. Please remove all references to nfqctl from this repo and PR, replacing with just the generic systemctl --user reload since that is what nfqctl calls.

`nfqctl` is a local script wrapper and not published to the wider audience for this. Please remove all references to `nfqctl` from this repo and PR, replacing with just the generic `systemctl --user reload` since that is what `nfqctl` calls.
internal/nfq_forwarder/connection_manager.go Outdated
@ -0,0 +44,4 @@
// startup before this. A reload that changes these global settings only affects connections
// that get (re)started by that same reload; a connection left unchanged keeps its
// already-built pool (and any open tunnels on it) exactly as it was.
sshKeyPath string
danny commented 2026-07-09 19:40:45 +01:00
Owner
Copy link

These connection configuration values feel like they should be in their own structure?

These connection configuration values feel like they should be in their own structure?
Address PR review: drop nfqctl references, group SSH pool settings
All checks were successful
CI / lint (push) Successful in 25s
Details
CI / build (push) Successful in 28s
Details
CI / lint (pull_request) Successful in 25s
Details
CI / build (pull_request) Successful in 27s
Details
CI / release (push) Has been skipped
Details
CI / release (pull_request) Has been skipped
Details
2dca376a6b 
nfqctl is a local script wrapper, not published for general use -
replace every mention with the generic `systemctl --user reload`
command it just calls under the hood.

Also group connectionManager's SSH-pool-construction fields
(sshKeyPath, knownHostsPath, keepaliveInterval,
autoAddUnknownHostKeys) into a dedicated sshPoolSettings struct
rather than leaving them as loose fields.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
claude commented 2026-07-09 19:51:38 +01:00
Author
Collaborator
Copy link

Pushed a follow-up commit addressing both comments:

  • Removed every nfqctl reference (README.md, docs/README.md, docs/nfq_forwarder.service, CLAUDE.md) - replaced with the generic systemctl --user reload nfq_forwarder.service command it just calls.
  • Grouped connectionManager's SSH-pool-construction fields (sshKeyPath, knownHostsPath, keepaliveInterval, autoAddUnknownHostKeys) into a new sshPoolSettings struct instead of leaving them as loose fields.

make lint/make test clean.

Pushed a follow-up commit addressing both comments: - Removed every `nfqctl` reference (README.md, docs/README.md, docs/nfq_forwarder.service, CLAUDE.md) - replaced with the generic `systemctl --user reload nfq_forwarder.service` command it just calls. - Grouped `connectionManager`'s SSH-pool-construction fields (`sshKeyPath`, `knownHostsPath`, `keepaliveInterval`, `autoAddUnknownHostKeys`) into a new `sshPoolSettings` struct instead of leaving them as loose fields. `make lint`/`make test` clean.
danny approved these changes 2026-07-09 19:52:49 +01:00
danny left a comment
Copy link

That looks better, thanks.

That looks better, thanks.
danny merged commit 4cd09a3b0f into main 2026-07-09 19:53:17 +01:00
danny deleted branch issue29_config_reload 2026-07-09 19:53:18 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
danny
Labels
Clear labels   
Bug

   
BuildIssue

   
Claude-fixed

   
Enhancement

   
In Progress

   
Low priority

   
On Hold
Blocked on something outside this project (e.g. external infrastructure) - not actionable right now

  
Rejected
Feature or ussue was rejected for development

  
Security
No labels
Bug
BuildIssue
Claude-fixed
Enhancement
In Progress
Low priority
On Hold
Rejected
Security
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
keyop-go/nfq_forwarder!84
No description provided.