From 399815f92b28a03a066c344aa0198f31051250a9 Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 20 Nov 2025 09:37:15 +0000 Subject: [PATCH 1/8] feat: add systemctl reload support for host configuration - 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. --- README.md | 6 + cmd/root.go | 50 ++++-- config/crowdsec-spoa-bouncer.service | 1 + pkg/host/README.md | 44 ++++++ pkg/host/root.go | 224 +++++++++++++++++---------- 5 files changed, 230 insertions(+), 95 deletions(-) diff --git a/README.md b/README.md index 4c67244..fd1fb4f 100644 --- a/README.md +++ b/README.md @@ -18,3 +18,9 @@ See [public documentation](https://doc.crowdsec.net/u/bouncers/haproxy_spoa) This outlines the goals of the project, and the current status of each. We are currently working on AppSec integration to this bouncer. + +## Reload Support + +The bouncer supports reloading host configurations without restarting the service. Use `systemctl reload crowdsec-spoa-bouncer` to reload hosts from both the main configuration file (`hosts:` section) and the `hosts_dir` directory. See [pkg/host/README.md](pkg/host/README.md) for detailed documentation. + +**Note:** Only host configurations are reloadable. Changes to listener addresses, LAPI settings, or other configuration require a full service restart. diff --git a/cmd/root.go b/cmd/root.go index ecc5857..2452813 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -91,6 +91,10 @@ func Execute() error { ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() + // Set up SIGHUP handler for reload + reloadChan := make(chan os.Signal, 1) + signal.Notify(reloadChan, syscall.SIGHUP) + g, ctx := errgroup.WithContext(ctx) config.Geo.Init(ctx) @@ -162,6 +166,7 @@ func Execute() error { // Create a base logger for the host manager hostManagerLogger := log.WithField("component", "host_manager") HostManager := host.NewManager(hostManagerLogger) + HostManager.SetContext(ctx) // Set base service context for goroutines // Create and initialize global session manager (single GC goroutine for all hosts) globalSessions := &session.Sessions{ @@ -172,22 +177,43 @@ func Execute() error { sessionLogger := log.WithField("component", "global_sessions") globalSessions.Init(sessionLogger, ctx) + // Handle SIGHUP for reload 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 + } } - } + }) - if config.HostsDir != "" { - if err := HostManager.LoadFromDirectory(config.HostsDir); err != nil { - return fmt.Errorf("failed to load hosts from directory: %w", err) - } + // Set hosts from both config and directory (merged, config takes precedence) + if err := HostManager.SetHosts(config.Hosts, config.HostsDir); err != nil { + return fmt.Errorf("failed to load hosts: %w", err) } // Create single SPOA listener - ultra-simplified architecture diff --git a/config/crowdsec-spoa-bouncer.service b/config/crowdsec-spoa-bouncer.service index 7a992a4..03c4990 100644 --- a/config/crowdsec-spoa-bouncer.service +++ b/config/crowdsec-spoa-bouncer.service @@ -12,6 +12,7 @@ Group=crowdsec-spoa ExecStart=${BIN} -c ${CFG}/crowdsec-spoa-bouncer.yaml ExecStartPre=${BIN} -c ${CFG}/crowdsec-spoa-bouncer.yaml -t ExecStartPost=/bin/sleep 0.1 +ExecReload=/bin/kill -HUP $MAINPID Restart=always RestartSec=10 # Security hardening diff --git a/pkg/host/README.md b/pkg/host/README.md index 0562202..1482197 100644 --- a/pkg/host/README.md +++ b/pkg/host/README.md @@ -66,3 +66,47 @@ If no host is found for the incoming request, then the remediation will be sent If the remediation is `captcha` and no host is found, then the remediation will be automatically changed to a `ban` since we have no way to display the captcha. This is why we recommend having a catch-all configuration for the `ban` remediation it will allow you to change the `fallback_remediation` to `allow` or provide a `contact_us_url`. As this will impact user experience if they are not able to contact anyone for help. + +#### Reloading Host Configuration + +The bouncer supports reloading host configurations without restarting the service. This is useful when using the `hosts_dir` configuration option to manage hosts via individual YAML files. + +**How to reload:** + +```bash +# Using systemctl (recommended) +sudo systemctl reload crowdsec-spoa-bouncer + +# Or manually send SIGHUP +sudo kill -HUP $(pidof crowdsec-spoa-bouncer) +``` + +**What gets reloaded:** + +- ✅ Hosts from the main configuration file (`hosts:` section) +- ✅ Hosts loaded from `hosts_dir` directory (all `*.yaml` files) +- ✅ New hosts are added +- ✅ Removed hosts are deleted +- ✅ Modified hosts are updated + +**Precedence:** If a host exists in both the main config and `hosts_dir`, the main config version takes precedence and will replace the directory version on reload. + +**What does NOT get reloaded:** + +- ❌ SPOA listener addresses (`listen_tcp`, `listen_unix`) +- ❌ LAPI connection settings (`api_url`, `api_key`, etc.) +- ❌ Logging configuration +- ❌ Prometheus configuration + +For changes to non-host configuration, you must restart the service: + +```bash +sudo systemctl restart crowdsec-spoa-bouncer +``` + +**Important notes:** + +- Both hosts from the main config file and `hosts_dir` are reloaded on `systemctl reload`. +- If a host exists in both sources, the main config version takes precedence. +- If a host file has errors during reload, it will be logged but the reload will continue processing other files. +- The reload operation is thread-safe and does not interrupt active requests. diff --git a/pkg/host/root.go b/pkg/host/root.go index 0d97d3b..c351b37 100644 --- a/pkg/host/root.go +++ b/pkg/host/root.go @@ -2,6 +2,7 @@ package host import ( "context" + "fmt" "os" "path/filepath" "sort" @@ -16,19 +17,6 @@ import ( "gopkg.in/yaml.v2" ) -const ( - OpAdd Op = "add" - OpRemove Op = "remove" - OpPatch Op = "patch" -) - -type Op string - -type HostOp struct { - Host *Host - Op Op -} - type Host struct { Host string `yaml:"host"` Captcha captcha.Captcha `yaml:"captcha"` @@ -39,10 +27,11 @@ type Host struct { } type Manager struct { - Hosts []*Host - Chan chan HostOp - Logger *log.Entry - cache map[string]*Host + Hosts []*Host + Logger *log.Entry + cache map[string]*Host + hostsDir string + ctx context.Context // Base service context for goroutines (not reload context) sync.RWMutex } @@ -67,7 +56,6 @@ func (h *Manager) String() string { func NewManager(l *log.Entry) *Manager { return &Manager{ Hosts: make([]*Host, 0), - Chan: make(chan HostOp), Logger: l, cache: make(map[string]*Host), } @@ -98,45 +86,97 @@ func (h *Manager) MatchFirstHost(toMatch string) *Host { return nil } -func (h *Manager) Run(ctx context.Context) { - - for { - select { - case instruction := <-h.Chan: - h.Lock() - switch instruction.Op { - case OpRemove: - h.cache = make(map[string]*Host) - h.removeHost(instruction.Host) - case OpAdd: - h.cache = make(map[string]*Host) - h.addHost(instruction.Host) - h.sort() - case OpPatch: - h.patchHost(instruction.Host) +// SetContext sets the base service context for host goroutines (captcha, appsec). +// This should be called once during initialization with the main service context. +func (h *Manager) SetContext(ctx context.Context) { + h.ctx = ctx +} + +// SetHosts sets hosts from both config and directory, merging them (config takes precedence). +func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { + allHosts := make(map[string]*Host) + + // First, load from directory if provided + if hostsDir != "" { + h.hostsDir = hostsDir + files, err := filepath.Glob(filepath.Join(hostsDir, "*.yaml")) + if err != nil { + return err + } + for _, file := range files { + host, err := LoadHostFromFile(file) + if err != nil { + return err } - h.Unlock() - case <-ctx.Done(): - return + allHosts[host.Host] = host } } -} -func (h *Manager) LoadFromDirectory(path string) error { - files, err := filepath.Glob(filepath.Join(path, "*.yaml")) - if err != nil { - return err + // Then, add config hosts (they override directory hosts) + for _, host := range configHosts { + allHosts[host.Host] = host } - for _, file := range files { - host, err := LoadHostFromFile(file) + + // Convert to slice + hostsSlice := make([]*Host, 0, len(allHosts)) + for _, host := range allHosts { + hostsSlice = append(hostsSlice, host) + } + + // Replace hosts directly with locking + h.Lock() + h.cache = make(map[string]*Host) + h.replaceHosts(hostsSlice) + h.Unlock() + + return nil +} + +// Reload reloads hosts from the configured hosts directory and/or main config file. +// It builds the desired state from both sources (config takes precedence) and syncs to it. +// Uses bulk replace for efficiency when there are many hosts. +// Note: Uses the base service context (set via SetContext), not a reload-specific context. +func (h *Manager) Reload(configHosts []*Host) error { + h.Logger.Info("Reloading host configuration") + + // Build desired state: config hosts + directory hosts (config takes precedence) + desiredHosts := make(map[string]*Host) + + // First, add directory hosts + if h.hostsDir != "" { + files, err := filepath.Glob(filepath.Join(h.hostsDir, "*.yaml")) if err != nil { - return err + return fmt.Errorf("failed to glob host files: %w", err) } - h.Chan <- HostOp{ - Host: host, - Op: OpAdd, + for _, file := range files { + host, err := LoadHostFromFile(file) + if err != nil { + h.Logger.WithError(err).WithField("file", file).Error("Failed to load host file during reload") + continue + } + desiredHosts[host.Host] = host } } + + // Then, add config hosts (they override directory hosts) + for _, host := range configHosts { + desiredHosts[host.Host] = host + } + + // Convert map to slice + desiredHostsSlice := make([]*Host, 0, len(desiredHosts)) + for _, host := range desiredHosts { + desiredHostsSlice = append(desiredHostsSlice, host) + } + + // Replace hosts directly with locking + h.Logger.WithField("host_count", len(desiredHostsSlice)).Info("Replacing hosts") + h.Lock() + h.cache = make(map[string]*Host) + h.replaceHosts(desiredHostsSlice) + h.Unlock() + + h.Logger.Info("Host reload completed") return nil } @@ -177,24 +217,6 @@ func (h *Manager) sort() { }) } -func (h *Manager) removeHost(host *Host) { - for i, th := range h.Hosts { - if th == host { - // Sessions persist in global manager, no cleanup needed - if i == len(h.Hosts)-1 { - h.Hosts = h.Hosts[:i] - } else { - h.Hosts = append(h.Hosts[:i], h.Hosts[i+1:]...) - } - return - } - } -} - -func (h *Manager) patchHost(host *Host) { - //TODO -} - // createHostLogger creates a logger for a host that inherits from the base logger // while allowing host-specific overrides like log level func (h *Manager) createHostLogger(host *Host) *log.Entry { @@ -229,25 +251,61 @@ func (h *Manager) createHostLogger(host *Host) *log.Entry { return hostLogger.WithField("host", host.Host) } -func (h *Manager) addHost(host *Host) { - // Create a logger for this host that inherits base logger values - host.logger = h.createHostLogger(host) - - // Add additional useful fields for host context - host.logger = host.logger.WithFields(log.Fields{ - "has_captcha": host.Captcha.Provider != "", - "has_ban": true, // Ban is always available - }) +// replaceHosts replaces the entire host list with a new set of hosts. +// This is used for bulk updates to avoid many individual add/remove operations. +// Uses the base service context (h.ctx) so goroutines are tied to service lifecycle, not reload. +// Preserves existing Host objects when host string matches to maintain sessions/state. +func (h *Manager) replaceHosts(newHosts []*Host) { + // Build map of existing hosts by host string + existingHosts := make(map[string]*Host) + for _, host := range h.Hosts { + existingHosts[host.Host] = host + } - // Initialize captcha (no longer needs sessions - SPOA handles that) - if err := host.Captcha.Init(host.logger); err != nil { - host.logger.Error(err) + // Build map of new hosts by host string + newHostsMap := make(map[string]*Host) + for _, host := range newHosts { + newHostsMap[host.Host] = host } - if err := host.Ban.Init(host.logger); err != nil { - host.logger.Error(err) + + // Remove hosts that are no longer needed + // Note: Sessions persist in global manager, so no cleanup needed + for hostStr := range existingHosts { + if _, exists := newHostsMap[hostStr]; !exists { + // Host removed - sessions will be garbage collected by global session manager + } } - if err := host.AppSec.Init(host.logger); err != nil { - host.logger.Error(err) + + // Process new hosts - update existing or create new + finalHosts := make([]*Host, 0, len(newHosts)) + for _, newHost := range newHosts { + if existingHost, exists := existingHosts[newHost.Host]; exists { + // Host already exists - preserve it completely to maintain sessions/state + // Configuration changes require a service restart to take effect + // This ensures active captcha sessions are not lost during reload + finalHosts = append(finalHosts, existingHost) + } else { + // New host - initialize it + newHost.logger = h.createHostLogger(newHost) + newHost.logger = newHost.logger.WithFields(log.Fields{ + "has_captcha": newHost.Captcha.Provider != "", + "has_ban": true, + }) + + if err := newHost.Captcha.Init(newHost.logger); err != nil { + newHost.logger.Error(err) + } + if err := newHost.Ban.Init(newHost.logger); err != nil { + newHost.logger.Error(err) + } + if err := newHost.AppSec.Init(newHost.logger); err != nil { + newHost.logger.Error(err) + } + finalHosts = append(finalHosts, newHost) + } } - h.Hosts = append(h.Hosts, host) + + // Replace the entire slice + h.Hosts = finalHosts + h.sort() } From ea28c41ee828226b4113f4b65298b5319ab07b04 Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 20 Nov 2025 09:40:17 +0000 Subject: [PATCH 2/8] refactor: remove unused context from host manager 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. --- cmd/root.go | 1 - pkg/host/root.go | 10 ---------- 2 files changed, 11 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 2452813..1b98393 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -166,7 +166,6 @@ func Execute() error { // Create a base logger for the host manager hostManagerLogger := log.WithField("component", "host_manager") HostManager := host.NewManager(hostManagerLogger) - HostManager.SetContext(ctx) // Set base service context for goroutines // Create and initialize global session manager (single GC goroutine for all hosts) globalSessions := &session.Sessions{ diff --git a/pkg/host/root.go b/pkg/host/root.go index c351b37..befca31 100644 --- a/pkg/host/root.go +++ b/pkg/host/root.go @@ -1,7 +1,6 @@ package host import ( - "context" "fmt" "os" "path/filepath" @@ -31,7 +30,6 @@ type Manager struct { Logger *log.Entry cache map[string]*Host hostsDir string - ctx context.Context // Base service context for goroutines (not reload context) sync.RWMutex } @@ -86,12 +84,6 @@ func (h *Manager) MatchFirstHost(toMatch string) *Host { return nil } -// SetContext sets the base service context for host goroutines (captcha, appsec). -// This should be called once during initialization with the main service context. -func (h *Manager) SetContext(ctx context.Context) { - h.ctx = ctx -} - // SetHosts sets hosts from both config and directory, merging them (config takes precedence). func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { allHosts := make(map[string]*Host) @@ -135,7 +127,6 @@ func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { // Reload reloads hosts from the configured hosts directory and/or main config file. // It builds the desired state from both sources (config takes precedence) and syncs to it. // Uses bulk replace for efficiency when there are many hosts. -// Note: Uses the base service context (set via SetContext), not a reload-specific context. func (h *Manager) Reload(configHosts []*Host) error { h.Logger.Info("Reloading host configuration") @@ -253,7 +244,6 @@ func (h *Manager) createHostLogger(host *Host) *log.Entry { // replaceHosts replaces the entire host list with a new set of hosts. // This is used for bulk updates to avoid many individual add/remove operations. -// Uses the base service context (h.ctx) so goroutines are tied to service lifecycle, not reload. // Preserves existing Host objects when host string matches to maintain sessions/state. func (h *Manager) replaceHosts(newHosts []*Host) { // Build map of existing hosts by host string From 1479273c15b52bdd7602dba1925b338366290d3a Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 20 Nov 2025 09:42:27 +0000 Subject: [PATCH 3/8] refactor: extract component initialization to Host.InitComponents() 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. --- pkg/host/root.go | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/pkg/host/root.go b/pkg/host/root.go index befca31..96fb443 100644 --- a/pkg/host/root.go +++ b/pkg/host/root.go @@ -25,6 +25,20 @@ type Host struct { logger *log.Entry `yaml:"-"` } +// InitComponents initializes all components (captcha, ban, appsec) for the host. +// This should be called after the host logger has been set. +func (h *Host) InitComponents() { + if err := h.Captcha.Init(h.logger); err != nil { + h.logger.Error(err) + } + if err := h.Ban.Init(h.logger); err != nil { + h.logger.Error(err) + } + if err := h.AppSec.Init(h.logger); err != nil { + h.logger.Error(err) + } +} + type Manager struct { Hosts []*Host Logger *log.Entry @@ -282,15 +296,8 @@ func (h *Manager) replaceHosts(newHosts []*Host) { "has_ban": true, }) - if err := newHost.Captcha.Init(newHost.logger); err != nil { - newHost.logger.Error(err) - } - if err := newHost.Ban.Init(newHost.logger); err != nil { - newHost.logger.Error(err) - } - if err := newHost.AppSec.Init(newHost.logger); err != nil { - newHost.logger.Error(err) - } + // Initialize all components + newHost.InitComponents() finalHosts = append(finalHosts, newHost) } } From a334eef03e3841cb39e32598cad65d223cea25de Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 20 Nov 2025 09:45:08 +0000 Subject: [PATCH 4/8] refactor: simplify replaceHosts since sessions are managed globally 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. --- pkg/host/root.go | 52 ++++++++++-------------------------------------- 1 file changed, 11 insertions(+), 41 deletions(-) diff --git a/pkg/host/root.go b/pkg/host/root.go index 96fb443..038d9a7 100644 --- a/pkg/host/root.go +++ b/pkg/host/root.go @@ -258,51 +258,21 @@ func (h *Manager) createHostLogger(host *Host) *log.Entry { // replaceHosts replaces the entire host list with a new set of hosts. // This is used for bulk updates to avoid many individual add/remove operations. -// Preserves existing Host objects when host string matches to maintain sessions/state. +// Sessions are managed globally and persist independently of host objects. func (h *Manager) replaceHosts(newHosts []*Host) { - // Build map of existing hosts by host string - existingHosts := make(map[string]*Host) - for _, host := range h.Hosts { - existingHosts[host.Host] = host - } - - // Build map of new hosts by host string - newHostsMap := make(map[string]*Host) + // Initialize all new hosts for _, host := range newHosts { - newHostsMap[host.Host] = host - } - - // Remove hosts that are no longer needed - // Note: Sessions persist in global manager, so no cleanup needed - for hostStr := range existingHosts { - if _, exists := newHostsMap[hostStr]; !exists { - // Host removed - sessions will be garbage collected by global session manager - } - } - - // Process new hosts - update existing or create new - finalHosts := make([]*Host, 0, len(newHosts)) - for _, newHost := range newHosts { - if existingHost, exists := existingHosts[newHost.Host]; exists { - // Host already exists - preserve it completely to maintain sessions/state - // Configuration changes require a service restart to take effect - // This ensures active captcha sessions are not lost during reload - finalHosts = append(finalHosts, existingHost) - } else { - // New host - initialize it - newHost.logger = h.createHostLogger(newHost) - newHost.logger = newHost.logger.WithFields(log.Fields{ - "has_captcha": newHost.Captcha.Provider != "", - "has_ban": true, - }) - - // Initialize all components - newHost.InitComponents() - finalHosts = append(finalHosts, newHost) - } + host.logger = h.createHostLogger(host) + host.logger = host.logger.WithFields(log.Fields{ + "has_captcha": host.Captcha.Provider != "", + "has_ban": true, + }) + + // Initialize all components + host.InitComponents() } // Replace the entire slice - h.Hosts = finalHosts + h.Hosts = newHosts h.sort() } From 0077ec285ba12827e5a18e5fa07245808c8f5f9b Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 20 Nov 2025 09:50:16 +0000 Subject: [PATCH 5/8] fix: resolve race condition and eliminate code duplication in host manager - 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. --- pkg/host/root.go | 102 ++++++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 46 deletions(-) diff --git a/pkg/host/root.go b/pkg/host/root.go index 038d9a7..cce92aa 100644 --- a/pkg/host/root.go +++ b/pkg/host/root.go @@ -75,44 +75,67 @@ func NewManager(l *log.Entry) *Manager { func (h *Manager) MatchFirstHost(toMatch string) *Host { h.RLock() - defer h.RUnlock() - - if len(h.Hosts) == 0 { - return nil - } - + // Fast path: check cache first if host, ok := h.cache[toMatch]; ok { + h.RUnlock() host.logger.WithField("requested_host", toMatch).Debug("matched host from cache") return host } + // Check if we have any hosts + if len(h.Hosts) == 0 { + h.RUnlock() + h.Logger.WithField("requested_host", toMatch).Debug("no matching host found") + return nil + } + + // Search for matching host + var matchedHost *Host for _, host := range h.Hosts { matched, err := filepath.Match(host.Host, toMatch) if matched && err == nil { - host.logger.WithField("requested_host", toMatch).Debug("matched host pattern") - h.cache[toMatch] = host - return host + matchedHost = host + break } } - h.Logger.WithField("requested_host", toMatch).Debug("no matching host found") - return nil + h.RUnlock() + + if matchedHost == nil { + h.Logger.WithField("requested_host", toMatch).Debug("no matching host found") + return nil + } + + // Cache the result (need write lock for this) + h.Lock() + h.cache[toMatch] = matchedHost + h.Unlock() + + matchedHost.logger.WithField("requested_host", toMatch).Debug("matched host pattern") + return matchedHost } -// SetHosts sets hosts from both config and directory, merging them (config takes precedence). -func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { +// loadHostsFromSources loads hosts from both directory and config, merging them. +// Config hosts take precedence over directory hosts. +// Returns a slice of hosts and an error if directory loading fails (config errors are ignored during reload). +func (h *Manager) loadHostsFromSources(configHosts []*Host, hostsDir string, continueOnError bool) ([]*Host, error) { allHosts := make(map[string]*Host) // First, load from directory if provided if hostsDir != "" { + // Store hostsDir for future reloads h.hostsDir = hostsDir files, err := filepath.Glob(filepath.Join(hostsDir, "*.yaml")) if err != nil { - return err + return nil, fmt.Errorf("failed to glob host files: %w", err) } for _, file := range files { host, err := LoadHostFromFile(file) if err != nil { - return err + if continueOnError { + h.Logger.WithError(err).WithField("file", file).Error("Failed to load host file") + continue + } + return nil, err } allHosts[host.Host] = host } @@ -123,12 +146,22 @@ func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { allHosts[host.Host] = host } - // Convert to slice + // Convert map to slice hostsSlice := make([]*Host, 0, len(allHosts)) for _, host := range allHosts { hostsSlice = append(hostsSlice, host) } + return hostsSlice, nil +} + +// SetHosts sets hosts from both config and directory, merging them (config takes precedence). +func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { + hostsSlice, err := h.loadHostsFromSources(configHosts, hostsDir, false) + if err != nil { + return err + } + // Replace hosts directly with locking h.Lock() h.cache = make(map[string]*Host) @@ -141,44 +174,21 @@ func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { // Reload reloads hosts from the configured hosts directory and/or main config file. // It builds the desired state from both sources (config takes precedence) and syncs to it. // Uses bulk replace for efficiency when there are many hosts. +// Errors loading individual files are logged but don't stop the reload. func (h *Manager) Reload(configHosts []*Host) error { h.Logger.Info("Reloading host configuration") - // Build desired state: config hosts + directory hosts (config takes precedence) - desiredHosts := make(map[string]*Host) - - // First, add directory hosts - if h.hostsDir != "" { - files, err := filepath.Glob(filepath.Join(h.hostsDir, "*.yaml")) - if err != nil { - return fmt.Errorf("failed to glob host files: %w", err) - } - for _, file := range files { - host, err := LoadHostFromFile(file) - if err != nil { - h.Logger.WithError(err).WithField("file", file).Error("Failed to load host file during reload") - continue - } - desiredHosts[host.Host] = host - } - } - - // Then, add config hosts (they override directory hosts) - for _, host := range configHosts { - desiredHosts[host.Host] = host - } - - // Convert map to slice - desiredHostsSlice := make([]*Host, 0, len(desiredHosts)) - for _, host := range desiredHosts { - desiredHostsSlice = append(desiredHostsSlice, host) + // Load hosts from both sources (continue on error for individual files during reload) + hostsSlice, err := h.loadHostsFromSources(configHosts, h.hostsDir, true) + if err != nil { + return err } // Replace hosts directly with locking - h.Logger.WithField("host_count", len(desiredHostsSlice)).Info("Replacing hosts") + h.Logger.WithField("host_count", len(hostsSlice)).Info("Replacing hosts") h.Lock() h.cache = make(map[string]*Host) - h.replaceHosts(desiredHostsSlice) + h.replaceHosts(hostsSlice) h.Unlock() h.Logger.Info("Host reload completed") From 3c3c3d56ca060bbbadb2204a4d778c91029d9182 Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 20 Nov 2025 09:54:28 +0000 Subject: [PATCH 6/8] fix: resolve golangci-lint control flag warning Replace boolean control flag with string parameter to avoid control coupling. Extract directory loading logic into separate function for better separation of concerns. --- pkg/host/root.go | 51 ++++++++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/pkg/host/root.go b/pkg/host/root.go index cce92aa..efd065e 100644 --- a/pkg/host/root.go +++ b/pkg/host/root.go @@ -114,31 +114,44 @@ func (h *Manager) MatchFirstHost(toMatch string) *Host { return matchedHost } +// loadHostsFromDirectory loads hosts from a directory, handling errors based on the error handling mode. +func (h *Manager) loadHostsFromDirectory(hostsDir string, errorMode string) (map[string]*Host, error) { + allHosts := make(map[string]*Host) + + // Store hostsDir for future reloads + h.hostsDir = hostsDir + files, err := filepath.Glob(filepath.Join(hostsDir, "*.yaml")) + if err != nil { + return nil, fmt.Errorf("failed to glob host files: %w", err) + } + + for _, file := range files { + host, err := LoadHostFromFile(file) + if err != nil { + if errorMode == "continue" { + h.Logger.WithError(err).WithField("file", file).Error("Failed to load host file") + continue + } + return nil, err + } + allHosts[host.Host] = host + } + + return allHosts, nil +} + // loadHostsFromSources loads hosts from both directory and config, merging them. // Config hosts take precedence over directory hosts. -// Returns a slice of hosts and an error if directory loading fails (config errors are ignored during reload). -func (h *Manager) loadHostsFromSources(configHosts []*Host, hostsDir string, continueOnError bool) ([]*Host, error) { +func (h *Manager) loadHostsFromSources(configHosts []*Host, hostsDir string, errorMode string) ([]*Host, error) { allHosts := make(map[string]*Host) // First, load from directory if provided if hostsDir != "" { - // Store hostsDir for future reloads - h.hostsDir = hostsDir - files, err := filepath.Glob(filepath.Join(hostsDir, "*.yaml")) + dirHosts, err := h.loadHostsFromDirectory(hostsDir, errorMode) if err != nil { - return nil, fmt.Errorf("failed to glob host files: %w", err) - } - for _, file := range files { - host, err := LoadHostFromFile(file) - if err != nil { - if continueOnError { - h.Logger.WithError(err).WithField("file", file).Error("Failed to load host file") - continue - } - return nil, err - } - allHosts[host.Host] = host + return nil, err } + allHosts = dirHosts } // Then, add config hosts (they override directory hosts) @@ -157,7 +170,7 @@ func (h *Manager) loadHostsFromSources(configHosts []*Host, hostsDir string, con // SetHosts sets hosts from both config and directory, merging them (config takes precedence). func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { - hostsSlice, err := h.loadHostsFromSources(configHosts, hostsDir, false) + hostsSlice, err := h.loadHostsFromSources(configHosts, hostsDir, "fail") if err != nil { return err } @@ -179,7 +192,7 @@ func (h *Manager) Reload(configHosts []*Host) error { h.Logger.Info("Reloading host configuration") // Load hosts from both sources (continue on error for individual files during reload) - hostsSlice, err := h.loadHostsFromSources(configHosts, h.hostsDir, true) + hostsSlice, err := h.loadHostsFromSources(configHosts, h.hostsDir, "continue") if err != nil { return err } From d073b0d0546b192a57870ec3b07431a274bb8db7 Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 20 Nov 2025 09:56:32 +0000 Subject: [PATCH 7/8] refactor: collect errors instead of using control flags 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. --- pkg/host/root.go | 54 ++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/pkg/host/root.go b/pkg/host/root.go index efd065e..0bfbab6 100644 --- a/pkg/host/root.go +++ b/pkg/host/root.go @@ -114,43 +114,43 @@ func (h *Manager) MatchFirstHost(toMatch string) *Host { return matchedHost } -// loadHostsFromDirectory loads hosts from a directory, handling errors based on the error handling mode. -func (h *Manager) loadHostsFromDirectory(hostsDir string, errorMode string) (map[string]*Host, error) { +// loadHostsFromDirectory loads hosts from a directory. +// Returns the loaded hosts and any errors encountered (one per file that failed to load). +// Errors loading individual files are collected and returned rather than stopping the process. +func (h *Manager) loadHostsFromDirectory(hostsDir string) (map[string]*Host, []error) { allHosts := make(map[string]*Host) - + var errors []error + // Store hostsDir for future reloads h.hostsDir = hostsDir files, err := filepath.Glob(filepath.Join(hostsDir, "*.yaml")) if err != nil { - return nil, fmt.Errorf("failed to glob host files: %w", err) + return nil, []error{fmt.Errorf("failed to glob host files: %w", err)} } - + for _, file := range files { host, err := LoadHostFromFile(file) if err != nil { - if errorMode == "continue" { - h.Logger.WithError(err).WithField("file", file).Error("Failed to load host file") - continue - } - return nil, err + errors = append(errors, fmt.Errorf("failed to load host file %s: %w", file, err)) + continue } allHosts[host.Host] = host } - - return allHosts, nil + + return allHosts, errors } // loadHostsFromSources loads hosts from both directory and config, merging them. // Config hosts take precedence over directory hosts. -func (h *Manager) loadHostsFromSources(configHosts []*Host, hostsDir string, errorMode string) ([]*Host, error) { +// Returns the loaded hosts and any errors encountered while loading directory files. +func (h *Manager) loadHostsFromSources(configHosts []*Host, hostsDir string) ([]*Host, []error) { allHosts := make(map[string]*Host) + var errors []error // First, load from directory if provided if hostsDir != "" { - dirHosts, err := h.loadHostsFromDirectory(hostsDir, errorMode) - if err != nil { - return nil, err - } + dirHosts, dirErrors := h.loadHostsFromDirectory(hostsDir) + errors = append(errors, dirErrors...) allHosts = dirHosts } @@ -165,14 +165,16 @@ func (h *Manager) loadHostsFromSources(configHosts []*Host, hostsDir string, err hostsSlice = append(hostsSlice, host) } - return hostsSlice, nil + return hostsSlice, errors } // SetHosts sets hosts from both config and directory, merging them (config takes precedence). +// During initial startup, any errors loading directory files will cause this to fail. func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { - hostsSlice, err := h.loadHostsFromSources(configHosts, hostsDir, "fail") - if err != nil { - return err + hostsSlice, errors := h.loadHostsFromSources(configHosts, hostsDir) + if len(errors) > 0 { + // During initial startup, return the first error + return errors[0] } // Replace hosts directly with locking @@ -191,10 +193,12 @@ func (h *Manager) SetHosts(configHosts []*Host, hostsDir string) error { func (h *Manager) Reload(configHosts []*Host) error { h.Logger.Info("Reloading host configuration") - // Load hosts from both sources (continue on error for individual files during reload) - hostsSlice, err := h.loadHostsFromSources(configHosts, h.hostsDir, "continue") - if err != nil { - return err + // Load hosts from both sources + hostsSlice, errors := h.loadHostsFromSources(configHosts, h.hostsDir) + + // Log all errors but continue with reload + for _, err := range errors { + h.Logger.WithError(err).Error("Failed to load host file during reload") } // Replace hosts directly with locking From 3cb83283fd89e0eecbe89584ff49b70a28dcb807 Mon Sep 17 00:00:00 2001 From: Laurence Date: Thu, 20 Nov 2025 09:57:38 +0000 Subject: [PATCH 8/8] refactor: simplify error messages to avoid redundancy in logs - 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. --- pkg/host/root.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/host/root.go b/pkg/host/root.go index 0bfbab6..2fd2e32 100644 --- a/pkg/host/root.go +++ b/pkg/host/root.go @@ -131,7 +131,7 @@ func (h *Manager) loadHostsFromDirectory(hostsDir string) (map[string]*Host, []e for _, file := range files { host, err := LoadHostFromFile(file) if err != nil { - errors = append(errors, fmt.Errorf("failed to load host file %s: %w", file, err)) + errors = append(errors, fmt.Errorf("%s: %w", file, err)) continue } allHosts[host.Host] = host @@ -195,10 +195,13 @@ func (h *Manager) Reload(configHosts []*Host) error { // Load hosts from both sources hostsSlice, errors := h.loadHostsFromSources(configHosts, h.hostsDir) - + // Log all errors but continue with reload - for _, err := range errors { - h.Logger.WithError(err).Error("Failed to load host file during reload") + if len(errors) > 0 { + h.Logger.WithField("error_count", len(errors)).Warn("Some host files failed to load during reload") + for _, err := range errors { + h.Logger.WithError(err).Error("Host file load error") + } } // Replace hosts directly with locking