feat: add systemctl reload support for host configuration #114
Conversation
- Add SIGHUP signal handler to reload host configuration without restart - Implement Reload() method in HostManager to update hosts from config - Preserve existing host objects during reload to maintain session state - Add SetContext() to tie host goroutines to service lifecycle - Update systemd service to support reload via systemctl reload This allows updating host configurations (captcha, ban, appsec) without losing active sessions or requiring a full service restart.
The context was previously used for captcha/appsec goroutines, but since sessions are now managed globally and Init() methods no longer take context, the host manager no longer needs to store or manage context.
DRY up the code by consolidating captcha, ban, and appsec initialization into a single method on the Host struct. This makes the code more maintainable and ensures consistent initialization behavior.
Remove the complex logic to preserve existing host objects during reload. Since sessions are now managed globally and persist independently of host objects, we can simply replace all hosts with new ones. This simplifies the code significantly and makes reload behavior more predictable.
…nager - Fix race condition in MatchFirstHost: properly release RLock before acquiring Lock for cache writes - Extract duplicate host loading logic from SetHosts and Reload into loadHostsFromSources() helper function - Improve error handling with continueOnError flag for reload resilience This makes the code thread-safe, more maintainable, and eliminates code duplication.
Replace boolean control flag with string parameter to avoid control coupling. Extract directory loading logic into separate function for better separation of concerns.
Replace control flag pattern with error collection. Functions now return a slice of errors, allowing callers to decide how to handle them: - SetHosts (startup): returns first error (fail-fast) - Reload: logs all errors but continues This follows Go best practices and eliminates control coupling.
- Use concise error format: just file path and wrapped error - Add summary log line with error count before individual errors - Use shorter log message to avoid duplication with error content This makes logs cleaner and easier to read, especially when multiple files fail to load during reload.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for hot-reloading host configurations via systemctl reload without requiring a full service restart. The implementation removes the channel-based host operation system in favor of a simpler synchronous reload mechanism triggered by SIGHUP signals.
Key Changes:
- Replaced async channel-based host management (
Chan,OpAdd,OpRemove,OpPatch) with direct synchronous methods (SetHosts,Reload) - Added SIGHUP signal handler to trigger configuration reload for both config file hosts and
hosts_dirYAML files - Implemented
ExecReloaddirective in systemd service file to send SIGHUP to the main process
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/host/root.go | Replaced channel-based host operations with synchronous SetHosts/Reload methods; improved MatchFirstHost locking granularity; added InitComponents helper method; removed context import |
| pkg/host/README.md | Added comprehensive documentation for reload functionality, including usage examples, behavior description, and limitations |
| config/crowdsec-spoa-bouncer.service | Added ExecReload directive to send SIGHUP signal for graceful reload |
| cmd/root.go | Added SIGHUP signal handler goroutine; replaced host channel operations with SetHosts call; reload re-reads config file and calls HostManager.Reload() |
| README.md | Added brief reload feature summary with reference to detailed documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Then, add config hosts (they override directory hosts) | ||
| for _, host := range configHosts { | ||
| allHosts[host.Host] = host | ||
| } |
There was a problem hiding this comment.
[nitpick] Duplicate host handling could lead to unexpected behavior. When merging hosts from directory and config, if the same host.Host value exists in multiple files or in both sources, only the last one processed will be kept (line 159 overwrites). This could silently ignore duplicate configurations without warning the user.
Consider:
- Logging a warning when a duplicate host is detected
- Documenting this precedence behavior more clearly
- Validating for duplicates within the directory files themselves
| h.Lock() | ||
| h.cache[toMatch] = matchedHost | ||
| h.Unlock() |
There was a problem hiding this comment.
Potential race condition: The cache write after releasing the read lock could result in stale data. Between releasing RUnlock() at line 101 and acquiring Lock() at line 109, another goroutine could reload hosts (via Reload() or SetHosts()), which clears the cache and replaces the host slice. This could lead to caching a stale host reference that no longer exists in the active host list.
Consider moving the cache update to occur while still holding the read lock (upgrade to write lock), or accept that the cache might contain stale entries and add validation when retrieving from cache.
| // Store hostsDir for future reloads | ||
| h.hostsDir = hostsDir |
There was a problem hiding this comment.
The hostsDir field is being written without proper synchronization. This field is written in loadHostsFromDirectory() which is called without holding the Manager's lock. Multiple concurrent calls to Reload() could race on this field update.
Consider moving the h.hostsDir = hostsDir assignment to occur under the Manager's lock (in SetHosts() or Reload()), or use atomic operations if this field needs to be accessed concurrently.
| // Store hostsDir for future reloads | |
| h.hostsDir = hostsDir | |
| // Store hostsDir for future reloads | |
| h.Lock() | |
| h.hostsDir = hostsDir | |
| h.Unlock() |
| g.Go(func() error { | ||
| HostManager.Run(ctx) | ||
| return nil | ||
| }) | ||
| for { | ||
| select { | ||
| case <-reloadChan: | ||
| log.Info("Received SIGHUP, reloading host configuration") | ||
|
|
||
| // Re-read config file to get updated hosts | ||
| var reloadConfigHosts []*host.Host | ||
| if *configPath != "" { | ||
| configMerged, err := cfg.MergedConfig(*configPath) | ||
| if err != nil { | ||
| log.WithError(err).Error("Failed to read config file during reload") | ||
| } else { | ||
| configExpanded := csstring.StrictExpand(string(configMerged), os.LookupEnv) | ||
| reloadConfig, err := cfg.NewConfig(strings.NewReader(configExpanded)) | ||
| if err != nil { | ||
| log.WithError(err).Error("Failed to parse config file during reload") | ||
| } else { | ||
| reloadConfigHosts = reloadConfig.Hosts | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for _, h := range config.Hosts { | ||
| HostManager.Chan <- host.HostOp{ | ||
| Host: h, | ||
| Op: host.OpAdd, | ||
| if err := HostManager.Reload(reloadConfigHosts); err != nil { | ||
| log.WithError(err).Error("Failed to reload host configuration") | ||
| // Don't return error, just log it so the service continues running | ||
| } | ||
| case <-ctx.Done(): | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Missing validation that HostManager.Reload() is not called before SetHosts(). The Reload() method relies on h.hostsDir being set, but if SIGHUP is received before SetHosts() is called (lines 213-216), h.hostsDir will be empty and the reload will silently skip loading from the directory.
Consider either:
- Adding a check in
Reload()to ensure initialization has completed - Deferring SIGHUP handler registration until after
SetHosts()completes - Documenting this initialization order requirement
|
due to #115 , this makes reloading hosts less of a risk for sessions and since reload only handles host configuration it doesnt seem wise to implement this. closing for now till a better reason comes to implement such a system. |
No description provided.