From 13c42e3513d5c44095c12688d5df6af5fa1547b2 Mon Sep 17 00:00:00 2001 From: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> Date: Fri, 31 Oct 2025 13:53:53 +0000 Subject: [PATCH 1/4] Add Configuration Management with Hot Reload Support - pkg/config/config.go - ConfigLoader interface and YAML parsing for the specified config schema - pkg/config/manager.go - Thread-safe ConfigManager with file watching capabilities - pkg/config/config_test.go - Tests for config loading and validation - pkg/config/manager_test.go - Tests for ConfigManager including concurrent access and file watching - pkg/config/example.yaml - Example configuration file Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> --- go.mod | 9 +- go.sum | 12 - pkg/config/config.go | 35 ++ pkg/config/config_test.go | 118 ++++++ pkg/config/manager.go | 226 +++++++++++ pkg/config/manager_test.go | 772 +++++++++++++++++++++++++++++++++++++ 6 files changed, 1153 insertions(+), 19 deletions(-) create mode 100644 pkg/config/manager.go create mode 100644 pkg/config/manager_test.go diff --git a/go.mod b/go.mod index 7c73f05..af5fa7b 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module github.com/stacklok/toolhive-registry-server go 1.24.6 require ( + github.com/fsnotify/fsnotify v1.9.0 github.com/go-chi/chi/v5 v5.2.3 - github.com/go-git/go-git/v5 v5.16.3 github.com/spf13/viper v1.21.0 github.com/stacklok/toolhive v0.4.0 github.com/stretchr/testify v1.11.1 @@ -21,16 +21,12 @@ require ( github.com/KyleBanks/depth v1.2.1 // indirect github.com/cenkalti/backoff/v5 v5.0.3 // indirect github.com/cpuguy83/go-md2man/v2 v2.0.7 // indirect - github.com/cyphar/filepath-securejoin v0.4.1 // indirect github.com/danieljoos/wincred v1.2.2 // indirect github.com/dylibso/observe-sdk/go v0.0.0-20240819160327-2d926c5d788a // indirect github.com/emicklei/go-restful/v3 v3.12.2 // indirect github.com/evanphx/json-patch/v5 v5.9.11 // indirect github.com/extism/go-sdk v1.7.0 // indirect - github.com/fsnotify/fsnotify v1.9.0 // indirect github.com/fxamacker/cbor/v2 v2.9.0 // indirect - github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect - github.com/go-git/go-billy/v5 v5.6.2 // indirect github.com/go-logr/zapr v1.3.0 // indirect github.com/go-openapi/jsonpointer v0.21.1 // indirect github.com/go-openapi/jsonreference v0.21.0 // indirect @@ -66,7 +62,7 @@ require ( github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f // indirect github.com/pelletier/go-toml/v2 v2.2.4 // indirect - github.com/pjbgf/sha1cd v0.3.2 // indirect + github.com/rogpeppe/go-internal v1.14.1 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/sagikazarmark/locafero v0.11.0 // indirect github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 // indirect @@ -99,7 +95,6 @@ require ( google.golang.org/protobuf v1.36.9 // indirect gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect - gopkg.in/warnings.v0 v0.1.2 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect k8s.io/api v0.34.1 // indirect k8s.io/klog/v2 v2.130.1 // indirect diff --git a/go.sum b/go.sum index e4c703f..4c60d54 100644 --- a/go.sum +++ b/go.sum @@ -19,8 +19,6 @@ github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XL github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/cpuguy83/go-md2man/v2 v2.0.7 h1:zbFlGlXEAKlwXpmvle3d8Oe3YnkKIK4xSRTd3sHPnBo= github.com/cpuguy83/go-md2man/v2 v2.0.7/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= -github.com/cyphar/filepath-securejoin v0.4.1 h1:JyxxyPEaktOD+GAnqIqTf9A8tHyAG22rowi7HkoSU1s= -github.com/cyphar/filepath-securejoin v0.4.1/go.mod h1:Sdj7gXlvMcPZsbhwhQ33GguGLDGQL7h7bg04C/+u9jI= github.com/danieljoos/wincred v1.2.2 h1:774zMFJrqaeYCK2W57BgAem/MLi6mtSE47MB6BOJ0i0= github.com/danieljoos/wincred v1.2.2/go.mod h1:w7w4Utbrz8lqeMbDAK0lkNJUv5sAOkFi7nd/ogr0Uh8= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -45,12 +43,6 @@ github.com/fxamacker/cbor/v2 v2.9.0 h1:NpKPmjDBgUfBms6tr6JZkTHtfFGcMKsw3eGcmD/sa github.com/fxamacker/cbor/v2 v2.9.0/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ= github.com/go-chi/chi/v5 v5.2.3 h1:WQIt9uxdsAbgIYgid+BpYc+liqQZGMHRaUwp0JUcvdE= github.com/go-chi/chi/v5 v5.2.3/go.mod h1:L2yAIGWB3H+phAw1NxKwWM+7eUH/lU8pOMm5hHcoops= -github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 h1:+zs/tPmkDkHx3U66DAb0lQFJrpS6731Oaa12ikc+DiI= -github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376/go.mod h1:an3vInlBmSxCcxctByoQdvwPiA7DTK7jaaFDBTtu0ic= -github.com/go-git/go-billy/v5 v5.6.2 h1:6Q86EsPXMa7c3YZ3aLAQsMA0VlWmy43r6FHqa/UNbRM= -github.com/go-git/go-billy/v5 v5.6.2/go.mod h1:rcFC2rAsp/erv7CMz9GczHcuD0D32fWzH+MJAU+jaUU= -github.com/go-git/go-git/v5 v5.16.3 h1:Z8BtvxZ09bYm/yYNgPKCzgWtaRqDTgIKRgIRHBfU6Z8= -github.com/go-git/go-git/v5 v5.16.3/go.mod h1:4Ge4alE/5gPs30F2H1esi2gPd69R0C39lolkucHBOp8= github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= github.com/go-logr/zapr v1.3.0 h1:XGdV8XW8zdwFiwOA2Dryh1gj2KRQyOOoNmBy4EplIcQ= @@ -148,8 +140,6 @@ github.com/onsi/gomega v1.38.2 h1:eZCjf2xjZAqe+LeWvKb5weQ+NcPwX84kqJ0cZNxok2A= github.com/onsi/gomega v1.38.2/go.mod h1:W2MJcYxRGV63b418Ai34Ud0hEdTVXq9NW9+Sx6uXf3k= github.com/pelletier/go-toml/v2 v2.2.4 h1:mye9XuhQ6gvn5h28+VilKrrPoQVanw5PMw/TB0t5Ec4= github.com/pelletier/go-toml/v2 v2.2.4/go.mod h1:2gIqNv+qfxSVS7cM2xJQKtLSTLUE9V8t9Stt+h56mCY= -github.com/pjbgf/sha1cd v0.3.2 h1:a9wb0bp1oC2TGwStyn0Umc/IGKQnEgF0vVaZ8QF8eo4= -github.com/pjbgf/sha1cd v0.3.2/go.mod h1:zQWigSxVmsHEZow5qaLtPYxpcKMMQpa09ixqBxuCS6A= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= @@ -288,8 +278,6 @@ gopkg.in/evanphx/json-patch.v4 v4.12.0 h1:n6jtcsulIzXPJaxegRbvFNNrZDjbij7ny3gmSP gopkg.in/evanphx/json-patch.v4 v4.12.0/go.mod h1:p8EYWUEYMpynmqDbY58zCKCFZw8pRWMG4EsWvDvM72M= gopkg.in/inf.v0 v0.9.1 h1:73M5CoZyi3ZLMOyDlQh031Cx6N9NDJ2Vvfl76EDAgDc= gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= -gopkg.in/warnings.v0 v0.1.2 h1:wFXVbFY8DY5/xOe1ECiWdKCzZlxgshcYVNkBHstARME= -gopkg.in/warnings.v0 v0.1.2/go.mod h1:jksf8JmL6Qr/oQM2OXTHunEvvTAsrWBLb6OOjuVWRNI= gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/pkg/config/config.go b/pkg/config/config.go index ba1b297..77be2ed 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -3,6 +3,7 @@ package config import ( "fmt" "os" + "time" "gopkg.in/yaml.v3" ) @@ -70,3 +71,37 @@ func (c *configLoader) LoadConfig(path string) (*Config, error) { return &config, nil } + +// Validate performs validation on the configuration +func (c *Config) Validate() error { + if c == nil { + return fmt.Errorf("config cannot be nil") + } + + // Validate source configuration + if c.Source.Type == "" { + return fmt.Errorf("source.type is required") + } + + // Validate ConfigMap settings if type is configmap + if c.Source.Type == "configmap" { + if c.Source.ConfigMap == nil { + return fmt.Errorf("source.configmap is required when type is configmap") + } + if c.Source.ConfigMap.Name == "" { + return fmt.Errorf("source.configmap.name is required") + } + } + + // Validate sync policy + if c.SyncPolicy.Interval == "" { + return fmt.Errorf("syncPolicy.interval is required") + } + + // Try to parse the interval to ensure it's valid + if _, err := time.ParseDuration(c.SyncPolicy.Interval); err != nil { + return fmt.Errorf("syncPolicy.interval must be a valid duration (e.g., '30m', '1h'): %w", err) + } + + return nil +} diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 4725c5a..0fc1a08 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -193,6 +193,124 @@ filter: } } +func TestConfigValidate(t *testing.T) { + tests := []struct { + name string + config *Config + wantErr bool + errMsg string + }{ + { + name: "valid_config", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{ + Name: "test-configmap", + }, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: false, + }, + { + name: "missing_source_type", + config: &Config{ + Source: SourceConfig{ + ConfigMap: &ConfigMapConfig{ + Name: "test", + }, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: true, + errMsg: "source.type is required", + }, + { + name: "missing_configmap_when_type_is_configmap", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: true, + errMsg: "source.configmap is required", + }, + { + name: "missing_configmap_name", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{}, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: true, + errMsg: "source.configmap.name is required", + }, + { + name: "missing_sync_interval", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{ + Name: "test", + }, + }, + SyncPolicy: SyncPolicyConfig{}, + }, + wantErr: true, + errMsg: "syncPolicy.interval is required", + }, + { + name: "invalid_sync_interval", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{ + Name: "test", + }, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "invalid", + }, + }, + wantErr: true, + errMsg: "syncPolicy.interval must be a valid duration", + }, + { + name: "nil_config", + config: nil, + wantErr: true, + errMsg: "config cannot be nil", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.config.Validate() + + if tt.wantErr { + require.Error(t, err) + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg) + } + } else { + require.NoError(t, err) + } + }) + } +} + func TestConfigStructure(t *testing.T) { // Test that the Config struct can be properly marshaled and unmarshaled originalConfig := &Config{ diff --git a/pkg/config/manager.go b/pkg/config/manager.go new file mode 100644 index 0000000..8d72e65 --- /dev/null +++ b/pkg/config/manager.go @@ -0,0 +1,226 @@ +package config + +import ( + "context" + "fmt" + "sync" + + "github.com/fsnotify/fsnotify" + "github.com/stacklok/toolhive/pkg/logger" +) + +// ConfigManager provides thread-safe configuration management with reload capabilities. +// It supports atomic configuration updates and file watching for automatic reloads. +// +// Design decisions: +// - Uses sync.RWMutex for efficient concurrent reads with occasional writes +// - Validates configuration before applying to prevent invalid states +// - Preserves old configuration on validation or load failures +// - Optimized for container environments (Kubernetes ConfigMap updates, volume mounts) +// - Provides context-aware watching for graceful shutdown +type ConfigManager interface { + // GetConfig safely retrieves the current configuration + GetConfig() *Config + + // ReloadConfig atomically loads and applies a new configuration from the file path + // Returns error if loading or validation fails, preserving the old config + ReloadConfig() error + + // WatchConfig starts watching the configuration file for changes + // Automatically reloads when changes are detected (with debouncing) + // Blocks until context is cancelled or an unrecoverable error occurs + WatchConfig(ctx context.Context) error + + // Close releases any resources held by the manager + Close() error +} + +// ConfigValidator defines the interface for validating configurations +// This allows custom validation logic to be injected beyond the basic validation +type ConfigValidator interface { + Validate(config *Config) error +} + +// defaultValidator uses the Config's built-in validation +type defaultValidator struct{} + +// Validate delegates to the Config's own Validate method +func (v *defaultValidator) Validate(config *Config) error { + return config.Validate() +} + +// configManager is the concrete implementation of ConfigManager +type configManager struct { + mu sync.RWMutex // Protects concurrent access to config + config *Config // Current active configuration + configPath string // Path to configuration file + loader ConfigLoader // Loader for reading config files + validator ConfigValidator // Validator for checking config validity + watcher *fsnotify.Watcher // File system watcher (nil if not watching) + watcherMu sync.Mutex // Protects watcher field +} + +// ConfigManagerOption allows customizing ConfigManager behavior +type ConfigManagerOption func(*configManager) + +// WithValidator sets a custom validator for the config manager +func WithValidator(validator ConfigValidator) ConfigManagerOption { + return func(cm *configManager) { + cm.validator = validator + } +} + +// WithLoader sets a custom config loader for the config manager +func WithLoader(loader ConfigLoader) ConfigManagerOption { + return func(cm *configManager) { + cm.loader = loader + } +} + +// NewConfigManager creates a new ConfigManager with the given configuration file path. +// It loads and validates the initial configuration. +// Returns error if initial load or validation fails. +func NewConfigManager(configPath string, opts ...ConfigManagerOption) (ConfigManager, error) { + cm := &configManager{ + configPath: configPath, + loader: NewConfigLoader(), + validator: &defaultValidator{}, // Uses Config.Validate() by default + } + + // Apply options + for _, opt := range opts { + opt(cm) + } + + // Load initial configuration + if err := cm.ReloadConfig(); err != nil { + return nil, fmt.Errorf("failed to load initial configuration: %w", err) + } + + return cm, nil +} + +// GetConfig safely retrieves the current configuration +// Multiple goroutines can safely call this method concurrently +func (cm *configManager) GetConfig() *Config { + cm.mu.RLock() + defer cm.mu.RUnlock() + + // Return a copy to prevent external modifications + // This is a shallow copy, which is sufficient for our use case + // as the Config struct fields are not modified in place + configCopy := *cm.config + return &configCopy +} + +// ReloadConfig atomically loads and applies a new configuration +// If loading or validation fails, the old configuration is preserved +func (cm *configManager) ReloadConfig() error { + // Load new configuration (outside the lock to allow concurrent reads) + newConfig, err := cm.loader.LoadConfig(cm.configPath) + if err != nil { + return fmt.Errorf("failed to load config: %w", err) + } + + // Validate new configuration + if err := cm.validator.Validate(newConfig); err != nil { + return fmt.Errorf("config validation failed: %w", err) + } + + // Atomically swap the configuration + cm.mu.Lock() + cm.config = newConfig + cm.mu.Unlock() + + logger.Infof("Configuration reloaded successfully from %s", cm.configPath) + return nil +} + +// WatchConfig starts watching the configuration file for changes. +// It uses fsnotify to detect file system events. +// +// In container environments, config updates typically happen atomically through: +// - Kubernetes ConfigMap volume mounts (using symlink swaps) +// - Docker volume updates +// - Orchestration tool updates +// +// Blocks until the context is cancelled or an unrecoverable error occurs. +func (cm *configManager) WatchConfig(ctx context.Context) error { + cm.watcherMu.Lock() + if cm.watcher != nil { + cm.watcherMu.Unlock() + return fmt.Errorf("config watcher is already running") + } + + watcher, err := fsnotify.NewWatcher() + if err != nil { + cm.watcherMu.Unlock() + return fmt.Errorf("failed to create file watcher: %w", err) + } + cm.watcher = watcher + cm.watcherMu.Unlock() + + // Add config file to watcher + if err := watcher.Add(cm.configPath); err != nil { + return fmt.Errorf("failed to watch config file %s: %w", cm.configPath, err) + } + + logger.Infof("Started watching configuration file: %s", cm.configPath) + + // Watch loop + for { + select { + case <-ctx.Done(): + logger.Info("Stopping config file watcher due to context cancellation") + return ctx.Err() + + case event, ok := <-watcher.Events: + if !ok { + return fmt.Errorf("watcher event channel closed") + } + + // In containers, we primarily care about Write events + // Kubernetes ConfigMaps update files atomically via symlink swaps + if event.Has(fsnotify.Write) || event.Has(fsnotify.Create) { + logger.Infof("Config file change detected (%s), reloading configuration", event.Op) + + // Reload immediately - container environments have atomic updates + if err := cm.ReloadConfig(); err != nil { + logger.Errorf("Failed to reload config after file change: %v", err) + // Continue watching - old configuration remains active + } + } + + // Handle Remove events for Kubernetes ConfigMap updates + // K8s may remove and recreate the symlink during updates + if event.Has(fsnotify.Remove) { + logger.Debugf("Config file removed (likely K8s ConfigMap update), re-watching") + _ = watcher.Add(cm.configPath) + } + + case err, ok := <-watcher.Errors: + if !ok { + return fmt.Errorf("watcher error channel closed") + } + // Log error but continue watching + logger.Errorf("File watcher error: %v", err) + } + } +} + +// Close releases resources held by the config manager +// Specifically, it closes the file watcher if active +func (cm *configManager) Close() error { + cm.watcherMu.Lock() + defer cm.watcherMu.Unlock() + + if cm.watcher != nil { + if err := cm.watcher.Close(); err != nil { + return fmt.Errorf("failed to close file watcher: %w", err) + } + cm.watcher = nil + logger.Info("Config watcher closed") + } + + return nil +} diff --git a/pkg/config/manager_test.go b/pkg/config/manager_test.go new file mode 100644 index 0000000..9155820 --- /dev/null +++ b/pkg/config/manager_test.go @@ -0,0 +1,772 @@ +package config + +import ( + "context" + "fmt" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// validConfigYAML returns a valid configuration YAML string +func validConfigYAML() string { + return `source: + type: configmap + configmap: + name: test-registry +syncPolicy: + interval: "30m" +filter: + tags: + include: ["production"] + exclude: ["beta"]` +} + +// invalidConfigYAML returns an invalid configuration YAML string (missing required fields) +func invalidConfigYAML() string { + return `source: + type: "" +syncPolicy: + interval: "30m" +filter: + tags: + include: []` +} + +// TestNewConfigManager tests the creation of a new ConfigManager +func TestNewConfigManager(t *testing.T) { + tests := []struct { + name string + setupConfig func(t *testing.T) string + wantErr bool + errMsg string + }{ + { + name: "valid_config", + setupConfig: func(t *testing.T) string { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + return configPath + }, + wantErr: false, + }, + { + name: "invalid_config", + setupConfig: func(t *testing.T) string { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + err := os.WriteFile(configPath, []byte(invalidConfigYAML()), 0644) + require.NoError(t, err) + return configPath + }, + wantErr: true, + errMsg: "validation failed", + }, + { + name: "nonexistent_config", + setupConfig: func(t *testing.T) string { + tmpDir := t.TempDir() + return filepath.Join(tmpDir, "nonexistent.yaml") + }, + wantErr: true, + errMsg: "failed to load initial configuration", + }, + { + name: "invalid_yaml_syntax", + setupConfig: func(t *testing.T) string { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + err := os.WriteFile(configPath, []byte("invalid: [yaml"), 0644) + require.NoError(t, err) + return configPath + }, + wantErr: true, + errMsg: "failed to load initial configuration", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + configPath := tt.setupConfig(t) + + manager, err := NewConfigManager(configPath) + + if tt.wantErr { + require.Error(t, err) + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg) + } + return + } + + require.NoError(t, err) + require.NotNil(t, manager) + + // Verify we can get the config + config := manager.GetConfig() + assert.NotNil(t, config) + + // Clean up + err = manager.Close() + assert.NoError(t, err) + }) + } +} + +// TestGetConfig tests thread-safe config retrieval +func TestGetConfig(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + // Test basic retrieval + config := manager.GetConfig() + require.NotNil(t, config) + assert.Equal(t, "configmap", config.Source.Type) + assert.Equal(t, "test-registry", config.Source.ConfigMap.Name) + assert.Equal(t, "30m", config.SyncPolicy.Interval) + + // Test that returned config is a copy (modifications don't affect stored config) + config.Source.Type = "modified" + config2 := manager.GetConfig() + assert.Equal(t, "configmap", config2.Source.Type, "Config should be a copy") +} + +// TestGetConfigConcurrent tests concurrent access to GetConfig +func TestGetConfigConcurrent(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + // Run many concurrent reads + const numReaders = 100 + var wg sync.WaitGroup + wg.Add(numReaders) + + for i := 0; i < numReaders; i++ { + go func() { + defer wg.Done() + config := manager.GetConfig() + assert.NotNil(t, config) + assert.Equal(t, "configmap", config.Source.Type) + }() + } + + wg.Wait() +} + +// TestReloadConfig tests configuration reloading +func TestReloadConfig(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + // Write initial config + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + // Verify initial config + config := manager.GetConfig() + assert.Equal(t, "test-registry", config.Source.ConfigMap.Name) + + // Update config file + newConfig := `source: + type: configmap + configmap: + name: updated-registry +syncPolicy: + interval: "1h" +filter: + tags: + include: ["development"] + exclude: []` + err = os.WriteFile(configPath, []byte(newConfig), 0644) + require.NoError(t, err) + + // Reload config + err = manager.ReloadConfig() + require.NoError(t, err) + + // Verify updated config + config = manager.GetConfig() + assert.Equal(t, "updated-registry", config.Source.ConfigMap.Name) + assert.Equal(t, "1h", config.SyncPolicy.Interval) + assert.Equal(t, []string{"development"}, config.Filter.Tags.Include) +} + +// TestReloadConfigFailure tests that old config is preserved on reload failure +func TestReloadConfigFailure(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + // Write initial valid config + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + // Verify initial config + config := manager.GetConfig() + assert.Equal(t, "test-registry", config.Source.ConfigMap.Name) + + // Write invalid config + err = os.WriteFile(configPath, []byte(invalidConfigYAML()), 0644) + require.NoError(t, err) + + // Attempt to reload (should fail) + err = manager.ReloadConfig() + require.Error(t, err) + assert.Contains(t, err.Error(), "validation failed") + + // Verify old config is still active + config = manager.GetConfig() + assert.Equal(t, "test-registry", config.Source.ConfigMap.Name, "Old config should be preserved") +} + +// TestReloadConfigConcurrentReads tests reloading while reading +func TestReloadConfigConcurrentReads(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + // Write initial config + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + // Start many concurrent readers + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + var wg sync.WaitGroup + const numReaders = 50 + + // Continuous readers + wg.Add(numReaders) + for i := 0; i < numReaders; i++ { + go func() { + defer wg.Done() + for { + select { + case <-ctx.Done(): + return + default: + config := manager.GetConfig() + assert.NotNil(t, config) + // Config should always be valid + assert.NotEmpty(t, config.Source.Type) + } + } + }() + } + + // Perform multiple reloads while readers are active + for i := 0; i < 10; i++ { + newConfig := fmt.Sprintf(`source: + type: configmap + configmap: + name: registry-%d +syncPolicy: + interval: "30m" +filter: + tags: + include: ["test"] + exclude: []`, i) + err = os.WriteFile(configPath, []byte(newConfig), 0644) + require.NoError(t, err) + + err = manager.ReloadConfig() + require.NoError(t, err) + + time.Sleep(50 * time.Millisecond) + } + + cancel() + wg.Wait() +} + +// TestConfigValidator tests the default validator +func TestConfigValidator(t *testing.T) { + validator := &defaultValidator{} + + tests := []struct { + name string + config *Config + wantErr bool + errMsg string + }{ + { + name: "valid_configmap_config", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{ + Name: "test-registry", + }, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "30m", + }, + Filter: FilterConfig{}, + }, + wantErr: false, + }, + { + name: "nil_config", + config: nil, + wantErr: true, + errMsg: "config cannot be nil", + }, + { + name: "missing_source_type", + config: &Config{ + Source: SourceConfig{ + Type: "", + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: true, + errMsg: "source.type is required", + }, + { + name: "configmap_missing_config", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: nil, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: true, + errMsg: "source.configmap is required when type is configmap", + }, + { + name: "configmap_missing_name", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{ + Name: "", + }, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "30m", + }, + }, + wantErr: true, + errMsg: "source.configmap.name is required", + }, + { + name: "missing_sync_interval", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{ + Name: "test", + }, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "", + }, + }, + wantErr: true, + errMsg: "syncPolicy.interval is required", + }, + { + name: "invalid_sync_interval_format", + config: &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{ + Name: "test", + }, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "invalid", + }, + }, + wantErr: true, + errMsg: "must be a valid duration", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validator.Validate(tt.config) + + if tt.wantErr { + require.Error(t, err) + if tt.errMsg != "" { + assert.Contains(t, err.Error(), tt.errMsg) + } + return + } + + require.NoError(t, err) + }) + } +} + +// TestWatchConfig tests file watching functionality +func TestWatchConfig(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + // Write initial config + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + // Start watching in background + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + watchErr := make(chan error, 1) + go func() { + watchErr <- manager.WatchConfig(ctx) + }() + + // Wait for watcher to initialize + time.Sleep(100 * time.Millisecond) + + // Verify initial config + config := manager.GetConfig() + assert.Equal(t, "test-registry", config.Source.ConfigMap.Name) + + // Modify the config file + newConfig := `source: + type: configmap + configmap: + name: watched-registry +syncPolicy: + interval: "45m" +filter: + tags: + include: ["watched"] + exclude: []` + err = os.WriteFile(configPath, []byte(newConfig), 0644) + require.NoError(t, err) + + // Wait for debounce and reload (debounce is 500ms + some processing time) + time.Sleep(800 * time.Millisecond) + + // Verify config was reloaded + config = manager.GetConfig() + assert.Equal(t, "watched-registry", config.Source.ConfigMap.Name) + assert.Equal(t, "45m", config.SyncPolicy.Interval) + + // Stop watching + cancel() + + // Wait for watch to stop + select { + case err := <-watchErr: + // Context cancellation is expected + assert.ErrorIs(t, err, context.Canceled) + case <-time.After(2 * time.Second): + t.Fatal("WatchConfig did not stop after context cancellation") + } +} + +// TestWatchConfigImmediate tests that config changes are applied immediately (no debouncing) +// This simulates container environments where updates are atomic (e.g., K8s ConfigMaps) +func TestWatchConfigImmediate(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + // Write initial config + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + // Start watching in background + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + go func() { + _ = manager.WatchConfig(ctx) + }() + + // Wait for watcher to initialize + time.Sleep(100 * time.Millisecond) + + // Test immediate application of changes + testCases := []string{"update-1", "update-2", "update-3"} + + for _, name := range testCases { + newConfig := fmt.Sprintf(`source: + type: configmap + configmap: + name: %s +syncPolicy: + interval: "30m" +filter: + tags: + include: [] + exclude: []`, name) + + err = os.WriteFile(configPath, []byte(newConfig), 0644) + require.NoError(t, err) + + // In container environments, changes should be applied immediately + // Small wait for file system events to propagate + time.Sleep(100 * time.Millisecond) + + config := manager.GetConfig() + assert.Equal(t, name, config.Source.ConfigMap.Name, + "Config should be updated immediately to %s", name) + } + + cancel() +} + +// TestWatchConfigInvalidUpdate tests that invalid config updates don't break watching +func TestWatchConfigInvalidUpdate(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + // Write initial config + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + // Start watching in background + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + go func() { + _ = manager.WatchConfig(ctx) + }() + + // Wait for watcher to initialize + time.Sleep(100 * time.Millisecond) + + // Get initial config + initialConfig := manager.GetConfig() + assert.Equal(t, "test-registry", initialConfig.Source.ConfigMap.Name) + + // Write invalid config + err = os.WriteFile(configPath, []byte(invalidConfigYAML()), 0644) + require.NoError(t, err) + + // Wait for debounce and reload attempt + time.Sleep(800 * time.Millisecond) + + // Config should still be the old valid one + config := manager.GetConfig() + assert.Equal(t, "test-registry", config.Source.ConfigMap.Name) + + // Write valid config again + validConfig := `source: + type: configmap + configmap: + name: recovered-registry +syncPolicy: + interval: "15m" +filter: + tags: + include: [] + exclude: []` + err = os.WriteFile(configPath, []byte(validConfig), 0644) + require.NoError(t, err) + + // Wait for reload + time.Sleep(800 * time.Millisecond) + + // Should have new valid config + config = manager.GetConfig() + assert.Equal(t, "recovered-registry", config.Source.ConfigMap.Name) + + cancel() +} + +// TestWatchConfigAlreadyWatching tests that we can't start watching twice +func TestWatchConfigAlreadyWatching(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Start first watcher + go func() { + _ = manager.WatchConfig(ctx) + }() + + // Wait for watcher to initialize + time.Sleep(100 * time.Millisecond) + + // Try to start second watcher + err = manager.WatchConfig(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), "already running") + + cancel() +} + +// TestConfigManagerWithCustomValidator tests using a custom validator +func TestConfigManagerWithCustomValidator(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + // Write config + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + // Custom validator that always fails + customValidator := &mockValidator{ + validateFunc: func(config *Config) error { + return fmt.Errorf("custom validation error") + }, + } + + // Should fail with custom validator + _, err = NewConfigManager(configPath, WithValidator(customValidator)) + require.Error(t, err) + assert.Contains(t, err.Error(), "custom validation error") +} + +// TestConfigManagerWithCustomLoader tests using a custom loader +func TestConfigManagerWithCustomLoader(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + // Custom loader that returns a fixed config + customLoader := &mockLoader{ + loadFunc: func(path string) (*Config, error) { + return &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{ + Name: "custom-loader-registry", + }, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "1h", + }, + Filter: FilterConfig{}, + }, nil + }, + } + + manager, err := NewConfigManager(configPath, WithLoader(customLoader)) + require.NoError(t, err) + defer func() { + assert.NoError(t, manager.Close()) + }() + + config := manager.GetConfig() + assert.Equal(t, "custom-loader-registry", config.Source.ConfigMap.Name) +} + +// TestClose tests proper cleanup of resources +func TestClose(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.yaml") + + err := os.WriteFile(configPath, []byte(validConfigYAML()), 0644) + require.NoError(t, err) + + manager, err := NewConfigManager(configPath) + require.NoError(t, err) + + // Start watching + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + go func() { + _ = manager.WatchConfig(ctx) + }() + + time.Sleep(100 * time.Millisecond) + + // Close should succeed + err = manager.Close() + assert.NoError(t, err) + + // Second close should also succeed (idempotent) + err = manager.Close() + assert.NoError(t, err) + + cancel() +} + +// Mock implementations for testing + +type mockValidator struct { + validateFunc func(*Config) error +} + +func (m *mockValidator) Validate(config *Config) error { + return m.validateFunc(config) +} + +type mockLoader struct { + loadFunc func(string) (*Config, error) +} + +func (m *mockLoader) LoadConfig(path string) (*Config, error) { + return m.loadFunc(path) +} From edfbf092662dbcbec43ba0a53ce2f52591f9ec53 Mon Sep 17 00:00:00 2001 From: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> Date: Fri, 31 Oct 2025 16:26:10 +0000 Subject: [PATCH 2/4] removes write Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> --- pkg/config/config.go | 15 +++++--- pkg/config/config_test.go | 2 +- pkg/config/manager.go | 76 +++++++++++++++++++------------------- pkg/config/manager_test.go | 4 +- 4 files changed, 50 insertions(+), 47 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 77be2ed..c43dc0f 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -8,8 +8,12 @@ import ( "gopkg.in/yaml.v3" ) -// ConfigLoader defines the interface for loading configuration +// ConfigLoader defines the interface for loading configuration from files. +// This is a read-only interface - configuration files are never modified by the application. +// All config changes come from external sources (Kubernetes ConfigMaps, volume mounts, etc.) type ConfigLoader interface { + // LoadConfig reads and parses a configuration file from the given path. + // The file is only read, never modified. LoadConfig(path string) (*Config, error) } @@ -55,18 +59,19 @@ func NewConfigLoader() ConfigLoader { return &configLoader{} } -// LoadConfig loads and parses configuration from a YAML file +// LoadConfig reads and parses a YAML configuration file. +// This is a read-only operation - the file is never modified. func (c *configLoader) LoadConfig(path string) (*Config, error) { - // Read the entire file into memory + // Read-only file access data, err := os.ReadFile(path) if err != nil { return nil, fmt.Errorf("failed to read config file: %w", err) } - // Parse YAML content + // Parse the YAML content var config Config if err := yaml.Unmarshal(data, &config); err != nil { - return nil, fmt.Errorf("failed to parse YAML config: %w", err) + return nil, fmt.Errorf("failed to parse YAML: %w", err) } return &config, nil diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 0fc1a08..90569d0 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -144,7 +144,7 @@ filter: yamlContent: `source: [invalid yaml`, wantConfig: nil, wantErr: true, - errMsg: "failed to parse YAML config", + errMsg: "failed to parse YAML", }, { name: "file_not_found", diff --git a/pkg/config/manager.go b/pkg/config/manager.go index 8d72e65..6efc383 100644 --- a/pkg/config/manager.go +++ b/pkg/config/manager.go @@ -9,29 +9,31 @@ import ( "github.com/stacklok/toolhive/pkg/logger" ) -// ConfigManager provides thread-safe configuration management with reload capabilities. -// It supports atomic configuration updates and file watching for automatic reloads. +// ConfigManager provides thread-safe, read-only configuration management. +// Configuration files are never modified by the application - all updates come from +// external sources (Kubernetes ConfigMaps, Docker volume mounts, orchestration tools). // -// Design decisions: -// - Uses sync.RWMutex for efficient concurrent reads with occasional writes -// - Validates configuration before applying to prevent invalid states -// - Preserves old configuration on validation or load failures -// - Optimized for container environments (Kubernetes ConfigMap updates, volume mounts) -// - Provides context-aware watching for graceful shutdown +// Design for read-only operation: +// - No file locking needed - we only observe external changes +// - No write coordination required - we never modify files +// - Uses sync.RWMutex optimized for concurrent reads +// - Validates externally-updated configs before applying +// - Preserves last known good configuration on invalid updates +// - Optimized for container environments with atomic file updates type ConfigManager interface { // GetConfig safely retrieves the current configuration GetConfig() *Config - // ReloadConfig atomically loads and applies a new configuration from the file path - // Returns error if loading or validation fails, preserving the old config + // ReloadConfig reads the latest configuration from disk and applies it if valid. + // The file is only read, never written. Returns error if the new config is invalid. ReloadConfig() error - // WatchConfig starts watching the configuration file for changes - // Automatically reloads when changes are detected (with debouncing) - // Blocks until context is cancelled or an unrecoverable error occurs + // WatchConfig observes the configuration file for external changes. + // Automatically reloads when the file is updated by external systems. + // Blocks until context is cancelled. WatchConfig(ctx context.Context) error - // Close releases any resources held by the manager + // Close releases the file watcher resources Close() error } @@ -113,38 +115,37 @@ func (cm *configManager) GetConfig() *Config { return &configCopy } -// ReloadConfig atomically loads and applies a new configuration -// If loading or validation fails, the old configuration is preserved +// ReloadConfig reads the configuration file and applies it if valid. +// The file is treated as read-only - we never modify it. +// If the new configuration is invalid, the previous configuration remains active. func (cm *configManager) ReloadConfig() error { - // Load new configuration (outside the lock to allow concurrent reads) + // Read the configuration file (read-only operation) newConfig, err := cm.loader.LoadConfig(cm.configPath) if err != nil { - return fmt.Errorf("failed to load config: %w", err) + return fmt.Errorf("failed to read config file: %w", err) } - // Validate new configuration + // Validate before applying if err := cm.validator.Validate(newConfig); err != nil { - return fmt.Errorf("config validation failed: %w", err) + return fmt.Errorf("invalid configuration: %w", err) } - // Atomically swap the configuration + // Atomically update the in-memory configuration cm.mu.Lock() cm.config = newConfig cm.mu.Unlock() - logger.Infof("Configuration reloaded successfully from %s", cm.configPath) + logger.Infof("Configuration reloaded from %s", cm.configPath) return nil } -// WatchConfig starts watching the configuration file for changes. -// It uses fsnotify to detect file system events. +// WatchConfig observes the configuration file for external changes. +// Since we never write to the file, all changes come from external sources: +// - Kubernetes ConfigMap updates (atomic via symlink swaps) +// - Docker volume mount updates +// - Configuration management tools // -// In container environments, config updates typically happen atomically through: -// - Kubernetes ConfigMap volume mounts (using symlink swaps) -// - Docker volume updates -// - Orchestration tool updates -// -// Blocks until the context is cancelled or an unrecoverable error occurs. +// This method blocks until the context is cancelled. func (cm *configManager) WatchConfig(ctx context.Context) error { cm.watcherMu.Lock() if cm.watcher != nil { @@ -179,22 +180,19 @@ func (cm *configManager) WatchConfig(ctx context.Context) error { return fmt.Errorf("watcher event channel closed") } - // In containers, we primarily care about Write events - // Kubernetes ConfigMaps update files atomically via symlink swaps + // Detect external file modifications if event.Has(fsnotify.Write) || event.Has(fsnotify.Create) { - logger.Infof("Config file change detected (%s), reloading configuration", event.Op) + logger.Infof("External config update detected, reloading") - // Reload immediately - container environments have atomic updates if err := cm.ReloadConfig(); err != nil { - logger.Errorf("Failed to reload config after file change: %v", err) - // Continue watching - old configuration remains active + logger.Errorf("Failed to reload config: %v", err) + // Continue observing - previous config remains active } } - // Handle Remove events for Kubernetes ConfigMap updates - // K8s may remove and recreate the symlink during updates + // Handle K8s ConfigMap updates (may remove/recreate symlinks) if event.Has(fsnotify.Remove) { - logger.Debugf("Config file removed (likely K8s ConfigMap update), re-watching") + logger.Debugf("Config file removed (K8s update), re-watching") _ = watcher.Add(cm.configPath) } diff --git a/pkg/config/manager_test.go b/pkg/config/manager_test.go index 9155820..459faed 100644 --- a/pkg/config/manager_test.go +++ b/pkg/config/manager_test.go @@ -67,7 +67,7 @@ func TestNewConfigManager(t *testing.T) { return configPath }, wantErr: true, - errMsg: "validation failed", + errMsg: "invalid configuration", }, { name: "nonexistent_config", @@ -246,7 +246,7 @@ func TestReloadConfigFailure(t *testing.T) { // Attempt to reload (should fail) err = manager.ReloadConfig() require.Error(t, err) - assert.Contains(t, err.Error(), "validation failed") + assert.Contains(t, err.Error(), "invalid configuration") // Verify old config is still active config = manager.GetConfig() From aa61309599b2dcdc3b14e983d58095988746ab6b Mon Sep 17 00:00:00 2001 From: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> Date: Mon, 3 Nov 2025 17:05:30 +0000 Subject: [PATCH 3/4] removes duplicate test Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> --- pkg/config/config_test.go | 166 -------------------------------------- 1 file changed, 166 deletions(-) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index e8a1aeb..cef6e2f 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -193,172 +193,6 @@ filter: } } -func TestConfigValidate(t *testing.T) { - tests := []struct { - name string - config *Config - wantErr bool - errMsg string - }{ - { - name: "valid_config", - config: &Config{ - Source: SourceConfig{ - Type: "configmap", - ConfigMap: &ConfigMapConfig{ - Name: "test-configmap", - }, - }, - SyncPolicy: SyncPolicyConfig{ - Interval: "30m", - }, - }, - wantErr: false, - }, - { - name: "missing_source_type", - config: &Config{ - Source: SourceConfig{ - ConfigMap: &ConfigMapConfig{ - Name: "test", - }, - }, - SyncPolicy: SyncPolicyConfig{ - Interval: "30m", - }, - }, - wantErr: true, - errMsg: "source.type is required", - }, - { - name: "missing_configmap_when_type_is_configmap", - config: &Config{ - Source: SourceConfig{ - Type: "configmap", - }, - SyncPolicy: SyncPolicyConfig{ - Interval: "30m", - }, - }, - wantErr: true, - errMsg: "source.configmap is required", - }, - { - name: "missing_configmap_name", - config: &Config{ - Source: SourceConfig{ - Type: "configmap", - ConfigMap: &ConfigMapConfig{}, - }, - SyncPolicy: SyncPolicyConfig{ - Interval: "30m", - }, - }, - wantErr: true, - errMsg: "source.configmap.name is required", - }, - { - name: "missing_sync_interval", - config: &Config{ - Source: SourceConfig{ - Type: "configmap", - ConfigMap: &ConfigMapConfig{ - Name: "test", - }, - }, - SyncPolicy: SyncPolicyConfig{}, - }, - wantErr: true, - errMsg: "syncPolicy.interval is required", - }, - { - name: "invalid_sync_interval", - config: &Config{ - Source: SourceConfig{ - Type: "configmap", - ConfigMap: &ConfigMapConfig{ - Name: "test", - }, - }, - SyncPolicy: SyncPolicyConfig{ - Interval: "invalid", - }, - }, - wantErr: true, - errMsg: "syncPolicy.interval must be a valid duration", - }, - { - name: "nil_config", - config: nil, - wantErr: true, - errMsg: "config cannot be nil", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := tt.config.Validate() - - if tt.wantErr { - require.Error(t, err) - if tt.errMsg != "" { - assert.Contains(t, err.Error(), tt.errMsg) - } - } else { - require.NoError(t, err) - } - }) - } -} - -func TestConfigStructure(t *testing.T) { - // Test that the Config struct can be properly marshaled and unmarshaled - originalConfig := &Config{ - Source: SourceConfig{ - Type: "configmap", - ConfigMap: &ConfigMapConfig{ - Name: "test-configmap", - }, - }, - SyncPolicy: SyncPolicyConfig{ - Interval: "45m", - }, - Filter: FilterConfig{ - Tags: TagFilterConfig{ - Include: []string{"prod", "stable"}, - Exclude: []string{"beta", "alpha"}, - }, - }, - } - - // Create a temporary directory and file - tmpDir := t.TempDir() - configPath := filepath.Join(tmpDir, "test-config.yaml") - - // Write the config using YAML - yamlContent := `source: - type: configmap - configmap: - name: test-configmap -syncPolicy: - interval: "45m" -filter: - tags: - include: ["prod", "stable"] - exclude: ["beta", "alpha"]` - - err := os.WriteFile(configPath, []byte(yamlContent), 0644) - require.NoError(t, err) - - // Load it back - loader := NewConfigLoader() - loadedConfig, err := loader.LoadConfig(configPath) - require.NoError(t, err) - - // Compare the structures - assert.Equal(t, originalConfig, loadedConfig) -} - func TestConfigValidate(t *testing.T) { tests := []struct { name string From 328d632c20169142981641921d6aabaa01ad475b Mon Sep 17 00:00:00 2001 From: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> Date: Mon, 3 Nov 2025 17:06:12 +0000 Subject: [PATCH 4/4] oops Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> --- pkg/config/config_test.go | 48 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index cef6e2f..cb3620e 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -193,6 +193,54 @@ filter: } } +func TestConfigStructure(t *testing.T) { + // Test that the Config struct can be properly marshaled and unmarshaled + originalConfig := &Config{ + Source: SourceConfig{ + Type: "configmap", + ConfigMap: &ConfigMapConfig{ + Name: "test-configmap", + }, + }, + SyncPolicy: SyncPolicyConfig{ + Interval: "45m", + }, + Filter: FilterConfig{ + Tags: TagFilterConfig{ + Include: []string{"prod", "stable"}, + Exclude: []string{"beta", "alpha"}, + }, + }, + } + + // Create a temporary directory and file + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "test-config.yaml") + + // Write the config using YAML + yamlContent := `source: + type: configmap + configmap: + name: test-configmap +syncPolicy: + interval: "45m" +filter: + tags: + include: ["prod", "stable"] + exclude: ["beta", "alpha"]` + + err := os.WriteFile(configPath, []byte(yamlContent), 0644) + require.NoError(t, err) + + // Load it back + loader := NewConfigLoader() + loadedConfig, err := loader.LoadConfig(configPath) + require.NoError(t, err) + + // Compare the structures + assert.Equal(t, originalConfig, loadedConfig) +} + func TestConfigValidate(t *testing.T) { tests := []struct { name string