diff --git a/.gitignore b/.gitignore index 767b157b89a..16f04cf514d 100644 --- a/.gitignore +++ b/.gitignore @@ -52,3 +52,4 @@ volume go.work go.work.sum +main diff --git a/cmd/relayproxy/config/config.go b/cmd/relayproxy/config/config.go index 5b0dede50aa..dc3c40bdbfa 100644 --- a/cmd/relayproxy/config/config.go +++ b/cmd/relayproxy/config/config.go @@ -218,6 +218,32 @@ func New(flagSet *pflag.FlagSet, log *zap.Logger, version string) (*Config, erro return proxyConf, nil } +// ReloadFromFile reloads the configuration from the config file. +// It preserves command line flags and environment variables. +func ReloadFromFile(flagSet *pflag.FlagSet, log *zap.Logger, version string) (*Config, error) { + // Reload config file (this will overwrite the file-based config) + loadConfigFile(log) + + // Map environment variables (they take precedence over file config) + _ = k.Load(mapEnvVariablesProvider(k.String("envVariablePrefix"), log), nil) + _ = k.Set("version", version) + + proxyConf := &Config{} + errUnmarshal := k.Unmarshal("", &proxyConf) + if errUnmarshal != nil { + return nil, errUnmarshal + } + + processExporters(proxyConf) + + return proxyConf, nil +} + +// GetConfigFilePath returns the path to the configuration file being used. +func GetConfigFilePath() (string, error) { + return locateConfigFile(k.String("config")) +} + // loadConfigFile handles the loading of configuration files func loadConfigFile(log *zap.Logger) { configFileLocation, errFileLocation := locateConfigFile(k.String("config")) @@ -232,6 +258,45 @@ func loadConfigFile(log *zap.Logger) { } } +// WatchConfigFile sets up a file watcher using koanf's built-in file watching +// and calls the reloadCallback when the configuration file changes. +// This function spawns a goroutine to watch for changes (Watch() is blocking). +func WatchConfigFile( + configFilePath string, + reloadCallback func() error, + log *zap.Logger, +) error { + parser := getParserForFile(configFilePath) + fileProvider := file.Provider(configFilePath) + + // Watch for changes using koanf's built-in Watch() method + // Watch() is blocking and spawns a goroutine internally, so we call it in a goroutine + go func() { + if err := fileProvider.Watch(func(event interface{}, err error) { + if err != nil { + log.Error("error watching config file", zap.Error(err)) + return + } + + // Reload the configuration file + if err := k.Load(fileProvider, parser); err != nil { + log.Error("error reloading config file", zap.Error(err)) + return + } + + // Call the reload callback + if err := reloadCallback(); err != nil { + log.Error("error in reload callback", zap.Error(err)) + } + }); err != nil { + log.Error("failed to start file watcher", zap.Error(err)) + } + }() + + log.Info("watching configuration file for changes", zap.String("file", configFilePath)) + return nil +} + // getParserForFile returns the appropriate parser based on file extension func getParserForFile(configFileLocation string) koanf.Parser { ext := filepath.Ext(configFileLocation) diff --git a/cmd/relayproxy/helper/echo_helper_test.go b/cmd/relayproxy/helper/echo_helper_test.go index 7d551a89bfc..960d3bec60f 100644 --- a/cmd/relayproxy/helper/echo_helper_test.go +++ b/cmd/relayproxy/helper/echo_helper_test.go @@ -9,8 +9,11 @@ import ( "github.com/labstack/echo/v4" "github.com/stretchr/testify/assert" ffclient "github.com/thomaspoignant/go-feature-flag" + "github.com/thomaspoignant/go-feature-flag/cmd/relayproxy/config" "github.com/thomaspoignant/go-feature-flag/cmd/relayproxy/helper" "github.com/thomaspoignant/go-feature-flag/cmd/relayproxy/service" + "github.com/thomaspoignant/go-feature-flag/notifier" + "go.uber.org/zap" ) func TestGetAPIKey(t *testing.T) { @@ -256,11 +259,11 @@ type MockFlagsetManager struct { err error } -func (m *MockFlagsetManager) GetFlagSet(apiKey string) (*ffclient.GoFeatureFlag, error) { +func (m *MockFlagsetManager) GetFlagSet(_ string) (*ffclient.GoFeatureFlag, error) { return m.flagset, m.err } -func (m *MockFlagsetManager) GetFlagSetName(apiKey string) (string, error) { +func (m *MockFlagsetManager) GetFlagSetName(_ string) (string, error) { return "", nil } @@ -276,6 +279,11 @@ func (m *MockFlagsetManager) IsDefaultFlagSet() bool { return false } +func (m *MockFlagsetManager) ReloadFlagsets(newConfig *config.Config, logger *zap.Logger, notifiers []notifier.Notifier) error { + // nothing to do for mock + return nil +} + func (m *MockFlagsetManager) Close() { // nothing to do } diff --git a/cmd/relayproxy/main.go b/cmd/relayproxy/main.go index cfd751efedd..391018e0f45 100644 --- a/cmd/relayproxy/main.go +++ b/cmd/relayproxy/main.go @@ -120,5 +120,54 @@ func main() { defer cancel() apiServer.Stop(ctx) }() + + // Start config file watcher if flagsets are configured + if len(proxyConf.FlagSets) > 0 { + configFilePath, err := config.GetConfigFilePath() + if err == nil { + err = config.WatchConfigFile(configFilePath, func() error { + return reloadFlagsets(f, flagsetManager, logger.ZapLogger, version, []notifier.Notifier{ + prometheusNotifier, + proxyNotifier, + }) + }, logger.ZapLogger) + if err != nil { + logger.ZapLogger.Warn("could not start config file watcher", zap.Error(err)) + } + } else { + logger.ZapLogger.Warn("could not start config file watcher", zap.Error(err)) + } + } + apiServer.StartWithContext(context.Background()) } + +// reloadFlagsets reloads the configuration file and updates flagsets +func reloadFlagsets( + flagSet *pflag.FlagSet, + flagsetManager service.FlagsetManager, + logger *zap.Logger, + version string, + notifiers []notifier.Notifier, +) error { + logger.Info("configuration file changed, reloading flagsets") + + // Reload configuration from file + newConfig, err := config.ReloadFromFile(flagSet, logger, version) + if err != nil { + return fmt.Errorf("failed to reload configuration file: %w", err) + } + + // Validate configuration + if err := newConfig.IsValid(); err != nil { + return fmt.Errorf("reloaded configuration is invalid: %w", err) + } + + // Reload flagsets + if err := flagsetManager.ReloadFlagsets(newConfig, logger, notifiers); err != nil { + return fmt.Errorf("failed to reload flagsets: %w", err) + } + + logger.Info("flagsets reloaded successfully") + return nil +} diff --git a/cmd/relayproxy/service/flagset_manager.go b/cmd/relayproxy/service/flagset_manager.go index 98c6f9144e9..596e23beb0e 100644 --- a/cmd/relayproxy/service/flagset_manager.go +++ b/cmd/relayproxy/service/flagset_manager.go @@ -3,6 +3,8 @@ package service import ( "errors" "fmt" + "reflect" + "sync" "github.com/google/uuid" ffclient "github.com/thomaspoignant/go-feature-flag" @@ -32,6 +34,10 @@ type FlagsetManager interface { GetDefaultFlagSet() *ffclient.GoFeatureFlag // IsDefaultFlagSet returns true if the manager is in default mode (no flagsets configured) IsDefaultFlagSet() bool + // ReloadFlagsets reloads flagsets from the new configuration. + // It validates that existing flagsets haven't been modified, and adds/removes flagsets as needed. + // Returns an error if any existing flagset has been modified. + ReloadFlagsets(newConfig *config.Config, logger *zap.Logger, notifiers []notifier.Notifier) error // Close closes the flagset manager Close() } @@ -57,6 +63,9 @@ type flagsetManagerImpl struct { // Mode is the mode of the flagset manager. mode flagsetManagerMode + + // mu protects concurrent access to flagsets + mu sync.RWMutex } // NewFlagsetManager is creating a new FlagsetManager. @@ -157,6 +166,9 @@ func newFlagsetManagerWithFlagsets( // GetFlagSet is returning the flag set linked to the API Key func (m *flagsetManagerImpl) GetFlagSet(apiKey string) (*ffclient.GoFeatureFlag, error) { + m.mu.RLock() + defer m.mu.RUnlock() + switch m.mode { case flagsetManagerModeFlagsets: if apiKey == "" { @@ -182,6 +194,9 @@ func (m *flagsetManagerImpl) GetFlagSet(apiKey string) (*ffclient.GoFeatureFlag, // GetFlagSetName returns the name of the flagset linked to the API Key func (m *flagsetManagerImpl) GetFlagSetName(apiKey string) (string, error) { + m.mu.RLock() + defer m.mu.RUnlock() + switch m.mode { case flagsetManagerModeFlagsets: if name, ok := m.APIKeysToFlagSetName[apiKey]; ok { @@ -195,12 +210,20 @@ func (m *flagsetManagerImpl) GetFlagSetName(apiKey string) (string, error) { // GetFlagSets returns the flag sets of the flagset manager. func (m *flagsetManagerImpl) GetFlagSets() (map[string]*ffclient.GoFeatureFlag, error) { + m.mu.RLock() + defer m.mu.RUnlock() + switch m.mode { case flagsetManagerModeFlagsets: if len(m.FlagSets) == 0 { return nil, fmt.Errorf("no flagsets configured") } - return m.FlagSets, nil + // Return a copy to prevent external modifications + result := make(map[string]*ffclient.GoFeatureFlag, len(m.FlagSets)) + for k, v := range m.FlagSets { + result[k] = v + } + return result, nil default: if m.DefaultFlagSet == nil { return nil, fmt.Errorf("no default flagset configured") @@ -221,7 +244,238 @@ func (m *flagsetManagerImpl) IsDefaultFlagSet() bool { return m.mode == flagsetManagerModeDefault } +// ReloadFlagsets reloads flagsets from the new configuration. +// It validates that existing flagsets haven't been modified, and adds/removes flagsets as needed. +// Returns an error if any existing flagset has been modified. +func (m *flagsetManagerImpl) ReloadFlagsets( + newConfig *config.Config, + logger *zap.Logger, + notifiers []notifier.Notifier, +) error { + m.mu.Lock() + defer m.mu.Unlock() + + if err := m.validateReloadPreconditions(newConfig); err != nil { + return err + } + + currentMappings := m.buildCurrentFlagsetMappings() + + if err := m.validateFlagsetChanges(newConfig, currentMappings); err != nil { + return err + } + + newFlagsets, newAPIKeysToFlagSetName, err := m.createNewFlagsets(newConfig, logger, notifiers) + if err != nil { + return err + } + + m.closeRemovedFlagsets(newFlagsets, logger) + m.updateFlagsets(newFlagsets, newAPIKeysToFlagSetName, newConfig, logger) + + return nil +} + +// validateReloadPreconditions checks if reload is allowed +func (m *flagsetManagerImpl) validateReloadPreconditions(newConfig *config.Config) error { + if m.mode == flagsetManagerModeDefault { + return fmt.Errorf("cannot reload flagsets in default mode") + } + if len(newConfig.FlagSets) == 0 { + return fmt.Errorf("cannot reload: new configuration has no flagsets") + } + return nil +} + +type flagsetMappings struct { + apiKeyToConfig map[string]*config.FlagSet + apiKeyToName map[string]string +} + +// buildCurrentFlagsetMappings builds maps of API keys to flagset configurations and names +func (m *flagsetManagerImpl) buildCurrentFlagsetMappings() flagsetMappings { + mappings := flagsetMappings{ + apiKeyToConfig: make(map[string]*config.FlagSet), + apiKeyToName: make(map[string]string), + } + + for i := range m.config.FlagSets { + flagset := &m.config.FlagSets[i] + flagSetName := normalizeFlagsetName(flagset.Name) + for _, apiKey := range flagset.APIKeys { + mappings.apiKeyToConfig[apiKey] = flagset + mappings.apiKeyToName[apiKey] = flagSetName + } + } + return mappings +} + +// normalizeFlagsetName returns the flagset name or generates a UUID if empty/default +func normalizeFlagsetName(name string) string { + if name == "" || name == utils.DefaultFlagSetName { + return uuid.New().String() + } + return name +} + +// validateFlagsetChanges validates that existing flagsets haven't been modified +func (m *flagsetManagerImpl) validateFlagsetChanges(newConfig *config.Config, currentMappings flagsetMappings) error { + for _, newFlagset := range newConfig.FlagSets { + if err := m.validateSingleFlagset(&newFlagset, currentMappings); err != nil { + return err + } + } + return nil +} + +// validateSingleFlagset validates a single flagset hasn't been modified +func (m *flagsetManagerImpl) validateSingleFlagset(newFlagset *config.FlagSet, currentMappings flagsetMappings) error { + existingConfig, existingFlagsetName := m.findMatchingFlagset(newFlagset, currentMappings) + if existingConfig == nil { + return nil // New flagset, no validation needed + } + + if !flagsetConfigsEqual(existingConfig, newFlagset) { + return fmt.Errorf("flagset '%s' has been modified, reload rejected", existingFlagsetName) + } + + if err := m.validateFlagsetNameChange(existingConfig, newFlagset, existingFlagsetName); err != nil { + return err + } + + return m.validateAPIKeyMovements(newFlagset, currentMappings) +} + +// findMatchingFlagset finds an existing flagset that matches the new one by API key +func (m *flagsetManagerImpl) findMatchingFlagset( + newFlagset *config.FlagSet, + currentMappings flagsetMappings, +) (*config.FlagSet, string) { + for _, apiKey := range newFlagset.APIKeys { + if existing, exists := currentMappings.apiKeyToConfig[apiKey]; exists { + return existing, currentMappings.apiKeyToName[apiKey] + } + } + return nil, "" +} + +// validateFlagsetNameChange validates that flagset names haven't changed +func (m *flagsetManagerImpl) validateFlagsetNameChange( + existingConfig, newFlagset *config.FlagSet, + existingFlagsetName string, +) error { + existingHasRealName := hasRealFlagsetName(existingConfig.Name) + newHasRealName := hasRealFlagsetName(newFlagset.Name) + + if existingHasRealName && newHasRealName && existingFlagsetName != normalizeFlagsetName(newFlagset.Name) { + return fmt.Errorf( + "flagset configuration changed (name changed from '%s' to '%s'), reload rejected", + existingFlagsetName, + normalizeFlagsetName(newFlagset.Name), + ) + } + return nil +} + +// hasRealFlagsetName checks if a flagset has a real (non-empty, non-default) name +func hasRealFlagsetName(name string) bool { + return name != "" && name != utils.DefaultFlagSetName +} + +// validateAPIKeyMovements validates that API keys haven't moved between flagsets +func (m *flagsetManagerImpl) validateAPIKeyMovements( + newFlagset *config.FlagSet, + currentMappings flagsetMappings, +) error { + newFlagSetName := normalizeFlagsetName(newFlagset.Name) + newHasRealName := hasRealFlagsetName(newFlagset.Name) + + for _, apiKey := range newFlagset.APIKeys { + if oldFlagsetConfig, exists := currentMappings.apiKeyToConfig[apiKey]; exists { + oldHasRealName := hasRealFlagsetName(oldFlagsetConfig.Name) + if oldHasRealName && newHasRealName { + oldRealName := oldFlagsetConfig.Name + if oldRealName != newFlagSetName { + return fmt.Errorf("API key moved from flagset '%s' to '%s', reload rejected", oldRealName, newFlagSetName) + } + } + } + } + return nil +} + +// createNewFlagsets creates new flagset clients from the configuration +func (m *flagsetManagerImpl) createNewFlagsets( + newConfig *config.Config, + logger *zap.Logger, + notifiers []notifier.Notifier, +) (map[string]*ffclient.GoFeatureFlag, map[string]string, error) { + newFlagsets := make(map[string]*ffclient.GoFeatureFlag) + newAPIKeysToFlagSetName := make(map[string]string) + + for index, flagset := range newConfig.FlagSets { + client, err := NewGoFeatureFlagClient(&flagset, logger, notifiers) + if err != nil { + logger.Error( + "failed to create goff client for flagset during reload", + zap.Int("flagset_index", index), + zap.String("flagset", flagset.Name), + zap.Error(err), + ) + return nil, nil, fmt.Errorf("failed to create flagset '%s': %w", flagset.Name, err) + } + + flagSetName := normalizeFlagsetName(flagset.Name) + newFlagsets[flagSetName] = client + for _, apiKey := range flagset.APIKeys { + newAPIKeysToFlagSetName[apiKey] = flagSetName + } + } + + return newFlagsets, newAPIKeysToFlagSetName, nil +} + +// closeRemovedFlagsets closes flagsets that are no longer in the configuration +func (m *flagsetManagerImpl) closeRemovedFlagsets(newFlagsets map[string]*ffclient.GoFeatureFlag, logger *zap.Logger) { + for name, flagset := range m.FlagSets { + if _, exists := newFlagsets[name]; !exists { + logger.Info("closing removed flagset", zap.String("flagset", name)) + flagset.Close() + } + } +} + +// updateFlagsets updates the manager with the new flagsets +func (m *flagsetManagerImpl) updateFlagsets( + newFlagsets map[string]*ffclient.GoFeatureFlag, + newAPIKeysToFlagSetName map[string]string, + newConfig *config.Config, + logger *zap.Logger, +) { + m.FlagSets = newFlagsets + m.APIKeysToFlagSetName = newAPIKeysToFlagSetName + m.config = newConfig + + logger.Info("flagsets reloaded successfully", + zap.Int("flagsets_count", len(newFlagsets)), + ) +} + +// flagsetConfigsEqual compares two flagset configurations, excluding APIKeys +func flagsetConfigsEqual(a, b *config.FlagSet) bool { + // Create copies without APIKeys for comparison + aCopy := *a + bCopy := *b + aCopy.APIKeys = nil + bCopy.APIKeys = nil + + return reflect.DeepEqual(aCopy, bCopy) +} + func (m *flagsetManagerImpl) Close() { + m.mu.Lock() + defer m.mu.Unlock() + if m.DefaultFlagSet != nil { m.DefaultFlagSet.Close() } diff --git a/cmd/relayproxy/service/flagset_manager_test.go b/cmd/relayproxy/service/flagset_manager_test.go index bbf3aa3381b..1e12a80f2b8 100644 --- a/cmd/relayproxy/service/flagset_manager_test.go +++ b/cmd/relayproxy/service/flagset_manager_test.go @@ -11,8 +11,22 @@ import ( "go.uber.org/zap" ) +const ( + testFlagConfigPath = "../testdata/controller/configuration_flags.yaml" + testFlagset1 = "flagset-1" + testFlagset2 = "flagset-2" + testFlagsetName = "test-flagset" + testFlagset2Name = "test-flagset-2" + testAPIKey = "test-api-key" + testKey1 = "key-1" + testKey2 = "key-2" + testModeFlagset = "flagset mode" + testModeDefault = "default mode" + errMsgFailedToCreate = "failed to create FlagsetManager: %v" +) + func TestNewFlagsetManager(t *testing.T) { - flagConfig := "../testdata/controller/configuration_flags.yaml" + flagConfig := testFlagConfigPath tests := []struct { name string config *config.Config @@ -49,14 +63,14 @@ func TestNewFlagsetManager(t *testing.T) { config: &config.Config{ FlagSets: []config.FlagSet{ { - Name: "test-flagset", + Name: testFlagsetName, CommonFlagSet: config.CommonFlagSet{ Retriever: &retrieverconf.RetrieverConf{ Kind: "file", Path: flagConfig, }, }, - APIKeys: []string{"test-api-key"}, + APIKeys: []string{testAPIKey}, }, }, }, @@ -76,7 +90,7 @@ func TestNewFlagsetManager(t *testing.T) { Path: flagConfig, }, }, - APIKeys: []string{"test-api-key"}, + APIKeys: []string{testAPIKey}, }, }, CommonFlagSet: config.CommonFlagSet{ @@ -112,21 +126,21 @@ func TestNewFlagsetManager(t *testing.T) { } func TestFlagsetManager_GetFlagSet(t *testing.T) { - flagConfig := "../testdata/controller/configuration_flags.yaml" + flagConfig := testFlagConfigPath // Test flagset mode - t.Run("flagset mode", func(t *testing.T) { + t.Run(testModeFlagset, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{ { - Name: "test-flagset", + Name: testFlagsetName, CommonFlagSet: config.CommonFlagSet{ Retriever: &retrieverconf.RetrieverConf{ Kind: "file", Path: flagConfig, }, }, - APIKeys: []string{"test-api-key"}, + APIKeys: []string{testAPIKey}, }, }, } @@ -134,13 +148,13 @@ func TestFlagsetManager_GetFlagSet(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() t.Run("valid api key", func(t *testing.T) { - flagset, err := manager.GetFlagSet("test-api-key") + flagset, err := manager.GetFlagSet(testAPIKey) assert.NoError(t, err) assert.NotNil(t, flagset) }) @@ -163,7 +177,7 @@ func TestFlagsetManager_GetFlagSet(t *testing.T) { }) // Test default mode - t.Run("default mode", func(t *testing.T) { + t.Run(testModeDefault, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{}, CommonFlagSet: config.CommonFlagSet{ @@ -177,7 +191,7 @@ func TestFlagsetManager_GetFlagSet(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() @@ -191,21 +205,21 @@ func TestFlagsetManager_GetFlagSet(t *testing.T) { } func TestFlagsetManager_GetFlagSetName(t *testing.T) { - flagConfig := "../testdata/controller/configuration_flags.yaml" + flagConfig := testFlagConfigPath // Test flagset mode - t.Run("flagset mode", func(t *testing.T) { + t.Run(testModeFlagset, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{ { - Name: "test-flagset", + Name: testFlagsetName, CommonFlagSet: config.CommonFlagSet{ Retriever: &retrieverconf.RetrieverConf{ Kind: "file", Path: flagConfig, }, }, - APIKeys: []string{"test-api-key"}, + APIKeys: []string{testAPIKey}, }, }, } @@ -213,15 +227,15 @@ func TestFlagsetManager_GetFlagSetName(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() t.Run("existing api key", func(t *testing.T) { - name, err := manager.GetFlagSetName("test-api-key") + name, err := manager.GetFlagSetName(testAPIKey) assert.NoError(t, err) - assert.Equal(t, "test-flagset", name) + assert.Equal(t, testFlagsetName, name) }) t.Run("non-existing api key", func(t *testing.T) { name, err := manager.GetFlagSetName("invalid-key") @@ -232,7 +246,7 @@ func TestFlagsetManager_GetFlagSetName(t *testing.T) { }) // Test default mode - t.Run("default mode", func(t *testing.T) { + t.Run(testModeDefault, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{}, CommonFlagSet: config.CommonFlagSet{ @@ -246,7 +260,7 @@ func TestFlagsetManager_GetFlagSetName(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() @@ -260,10 +274,10 @@ func TestFlagsetManager_GetFlagSetName(t *testing.T) { } func TestFlagsetManager_GetFlagSets(t *testing.T) { - flagConfig := "../testdata/controller/configuration_flags.yaml" + flagConfig := testFlagConfigPath // Test flagset mode - t.Run("flagset mode", func(t *testing.T) { + t.Run(testModeFlagset, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{ { @@ -277,7 +291,7 @@ func TestFlagsetManager_GetFlagSets(t *testing.T) { APIKeys: []string{"api-key-1"}, }, { - Name: "test-flagset-2", + Name: testFlagset2Name, CommonFlagSet: config.CommonFlagSet{ Retriever: &retrieverconf.RetrieverConf{ Kind: "file", @@ -292,7 +306,7 @@ func TestFlagsetManager_GetFlagSets(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() @@ -301,7 +315,7 @@ func TestFlagsetManager_GetFlagSets(t *testing.T) { assert.NoError(t, err) assert.Len(t, flagsets, 2) assert.Contains(t, flagsets, "test-flagset-1") - assert.Contains(t, flagsets, "test-flagset-2") + assert.Contains(t, flagsets, testFlagset2Name) }) t.Run("flagset mode using default flagset name", func(t *testing.T) { @@ -318,7 +332,7 @@ func TestFlagsetManager_GetFlagSets(t *testing.T) { APIKeys: []string{"api-key-1"}, }, { - Name: "test-flagset-2", + Name: testFlagset2Name, CommonFlagSet: config.CommonFlagSet{ Retriever: &retrieverconf.RetrieverConf{ Kind: "file", @@ -333,7 +347,7 @@ func TestFlagsetManager_GetFlagSets(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() @@ -342,11 +356,11 @@ func TestFlagsetManager_GetFlagSets(t *testing.T) { assert.NoError(t, err) assert.Len(t, flagsets, 2) assert.NotContains(t, flagsets, "default") - assert.Contains(t, flagsets, "test-flagset-2") + assert.Contains(t, flagsets, testFlagset2Name) }) // Test default mode - t.Run("default mode", func(t *testing.T) { + t.Run(testModeDefault, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{}, CommonFlagSet: config.CommonFlagSet{ @@ -360,7 +374,7 @@ func TestFlagsetManager_GetFlagSets(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() @@ -373,10 +387,10 @@ func TestFlagsetManager_GetFlagSets(t *testing.T) { } func TestFlagsetManager_GetDefaultFlagSet(t *testing.T) { - flagConfig := "../testdata/controller/configuration_flags.yaml" + flagConfig := testFlagConfigPath // Test default mode - t.Run("default mode", func(t *testing.T) { + t.Run(testModeDefault, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{}, CommonFlagSet: config.CommonFlagSet{ @@ -390,7 +404,7 @@ func TestFlagsetManager_GetDefaultFlagSet(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() @@ -400,18 +414,18 @@ func TestFlagsetManager_GetDefaultFlagSet(t *testing.T) { }) // Test flagset mode - t.Run("flagset mode", func(t *testing.T) { + t.Run(testModeFlagset, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{ { - Name: "test-flagset", + Name: testFlagsetName, CommonFlagSet: config.CommonFlagSet{ Retriever: &retrieverconf.RetrieverConf{ Kind: "file", Path: flagConfig, }, }, - APIKeys: []string{"test-api-key"}, + APIKeys: []string{testAPIKey}, }, }, } @@ -419,7 +433,7 @@ func TestFlagsetManager_GetDefaultFlagSet(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() @@ -430,10 +444,10 @@ func TestFlagsetManager_GetDefaultFlagSet(t *testing.T) { } func TestFlagsetManager_IsDefaultFlagSet(t *testing.T) { - flagConfig := "../testdata/controller/configuration_flags.yaml" + flagConfig := testFlagConfigPath // Test default mode - t.Run("default mode", func(t *testing.T) { + t.Run(testModeDefault, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{}, CommonFlagSet: config.CommonFlagSet{ @@ -447,7 +461,7 @@ func TestFlagsetManager_IsDefaultFlagSet(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() @@ -456,18 +470,18 @@ func TestFlagsetManager_IsDefaultFlagSet(t *testing.T) { }) // Test flagset mode - t.Run("flagset mode", func(t *testing.T) { + t.Run(testModeFlagset, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{ { - Name: "test-flagset", + Name: testFlagsetName, CommonFlagSet: config.CommonFlagSet{ Retriever: &retrieverconf.RetrieverConf{ Kind: "file", Path: flagConfig, }, }, - APIKeys: []string{"test-api-key"}, + APIKeys: []string{testAPIKey}, }, }, } @@ -475,7 +489,7 @@ func TestFlagsetManager_IsDefaultFlagSet(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) defer manager.Close() @@ -485,10 +499,10 @@ func TestFlagsetManager_IsDefaultFlagSet(t *testing.T) { } func TestFlagsetManager_Close(t *testing.T) { - flagConfig := "../testdata/controller/configuration_flags.yaml" + flagConfig := testFlagConfigPath // Test default mode - t.Run("default mode", func(t *testing.T) { + t.Run(testModeDefault, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{}, CommonFlagSet: config.CommonFlagSet{ @@ -502,7 +516,7 @@ func TestFlagsetManager_Close(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) @@ -512,18 +526,18 @@ func TestFlagsetManager_Close(t *testing.T) { }) // Test flagset mode - t.Run("flagset mode", func(t *testing.T) { + t.Run(testModeFlagset, func(t *testing.T) { config := &config.Config{ FlagSets: []config.FlagSet{ { - Name: "test-flagset", + Name: testFlagsetName, CommonFlagSet: config.CommonFlagSet{ Retriever: &retrieverconf.RetrieverConf{ Kind: "file", Path: flagConfig, }, }, - APIKeys: []string{"test-api-key"}, + APIKeys: []string{testAPIKey}, }, }, } @@ -531,7 +545,7 @@ func TestFlagsetManager_Close(t *testing.T) { notifiers := []notifier.Notifier{} manager, err := service.NewFlagsetManager(config, logger, notifiers) if err != nil { - t.Fatalf("failed to create FlagsetManager: %v", err) + t.Fatalf(errMsgFailedToCreate, err) } assert.NotNil(t, manager) @@ -540,3 +554,459 @@ func TestFlagsetManager_Close(t *testing.T) { }) }) } + +func TestFlagsetManager_ReloadFlagsets(t *testing.T) { + flagConfig := testFlagConfigPath + logger := zap.NewNop() + notifiers := []notifier.Notifier{} + + t.Run("successfully add new flagset", func(t *testing.T) { + initialConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + }, + } + + manager, err := service.NewFlagsetManager(initialConfig, logger, notifiers) + assert.NoError(t, err) + defer manager.Close() + + // Verify initial state + flagsets, err := manager.GetFlagSets() + assert.NoError(t, err) + assert.Len(t, flagsets, 1) + + // Reload with new flagset added + newConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + { + Name: testFlagset2, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey2}, + }, + }, + } + + err = manager.ReloadFlagsets(newConfig, logger, notifiers) + assert.NoError(t, err) + + // Verify new flagset was added + flagsets, err = manager.GetFlagSets() + assert.NoError(t, err) + assert.Len(t, flagsets, 2) + assert.Contains(t, flagsets, testFlagset1) + assert.Contains(t, flagsets, testFlagset2) + + // Verify both flagsets are accessible + flagset1, err := manager.GetFlagSet(testKey1) + assert.NoError(t, err) + assert.NotNil(t, flagset1) + + flagset2, err := manager.GetFlagSet(testKey2) + assert.NoError(t, err) + assert.NotNil(t, flagset2) + }) + + t.Run("successfully remove flagset", func(t *testing.T) { + initialConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + { + Name: testFlagset2, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey2}, + }, + }, + } + + manager, err := service.NewFlagsetManager(initialConfig, logger, notifiers) + assert.NoError(t, err) + defer manager.Close() + + // Verify initial state + flagsets, err := manager.GetFlagSets() + assert.NoError(t, err) + assert.Len(t, flagsets, 2) + + // Reload with one flagset removed + newConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + }, + } + + err = manager.ReloadFlagsets(newConfig, logger, notifiers) + assert.NoError(t, err) + + // Verify flagset was removed + flagsets, err = manager.GetFlagSets() + assert.NoError(t, err) + assert.Len(t, flagsets, 1) + assert.Contains(t, flagsets, testFlagset1) + + // Verify removed flagset is no longer accessible + _, err = manager.GetFlagSet(testKey2) + assert.Error(t, err) + }) + + t.Run("reject when existing flagset is modified", func(t *testing.T) { + initialConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + PollingInterval: 60000, + }, + APIKeys: []string{testKey1}, + }, + }, + } + + manager, err := service.NewFlagsetManager(initialConfig, logger, notifiers) + assert.NoError(t, err) + defer manager.Close() + + // Try to reload with modified polling interval + newConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + PollingInterval: 30000, // Changed + }, + APIKeys: []string{testKey1}, + }, + }, + } + + err = manager.ReloadFlagsets(newConfig, logger, notifiers) + assert.Error(t, err) + assert.Contains(t, err.Error(), "has been modified, reload rejected") + }) + + t.Run("reject when API key moved to different flagset", func(t *testing.T) { + initialConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + { + Name: testFlagset2, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey2}, + }, + }, + } + + manager, err := service.NewFlagsetManager(initialConfig, logger, notifiers) + assert.NoError(t, err) + defer manager.Close() + + // Try to reload with API key moved to different flagset + newConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1, testKey2}, // key-2 moved here + }, + { + Name: testFlagset2, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{}, // key-2 removed + }, + }, + } + + err = manager.ReloadFlagsets(newConfig, logger, notifiers) + assert.Error(t, err) + assert.Contains(t, err.Error(), "API key moved from flagset") + }) + + t.Run("reject when in default mode", func(t *testing.T) { + initialConfig := &config.Config{ + FlagSets: []config.FlagSet{}, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + } + + manager, err := service.NewFlagsetManager(initialConfig, logger, notifiers) + assert.NoError(t, err) + defer manager.Close() + + assert.True(t, manager.IsDefaultFlagSet()) + + newConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + }, + } + + err = manager.ReloadFlagsets(newConfig, logger, notifiers) + assert.Error(t, err) + assert.Contains(t, err.Error(), "cannot reload flagsets in default mode") + }) + + t.Run("reject when new config has no flagsets", func(t *testing.T) { + initialConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + }, + } + + manager, err := service.NewFlagsetManager(initialConfig, logger, notifiers) + assert.NoError(t, err) + defer manager.Close() + + newConfig := &config.Config{ + FlagSets: []config.FlagSet{}, + } + + err = manager.ReloadFlagsets(newConfig, logger, notifiers) + assert.Error(t, err) + assert.Contains(t, err.Error(), "cannot reload: new configuration has no flagsets") + }) + + t.Run("allow API keys to be added to existing flagset", func(t *testing.T) { + initialConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + }, + } + + manager, err := service.NewFlagsetManager(initialConfig, logger, notifiers) + assert.NoError(t, err) + defer manager.Close() + + // Reload with additional API key + newConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1, testKey2}, // Added key-2 + }, + }, + } + + err = manager.ReloadFlagsets(newConfig, logger, notifiers) + assert.NoError(t, err) + + // Verify both API keys work + flagset1, err := manager.GetFlagSet(testKey1) + assert.NoError(t, err) + assert.NotNil(t, flagset1) + + flagset2, err := manager.GetFlagSet(testKey2) + assert.NoError(t, err) + assert.NotNil(t, flagset2) + + // Both should return the same flagset + assert.Equal(t, flagset1, flagset2) + }) + + t.Run("allow API keys to be removed from existing flagset", func(t *testing.T) { + initialConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1, testKey2}, + }, + }, + } + + manager, err := service.NewFlagsetManager(initialConfig, logger, notifiers) + assert.NoError(t, err) + defer manager.Close() + + // Reload with one API key removed + newConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: testFlagset1, + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, // Removed key-2 + }, + }, + } + + err = manager.ReloadFlagsets(newConfig, logger, notifiers) + assert.NoError(t, err) + + // Verify key-1 still works + _, err = manager.GetFlagSet(testKey1) + assert.NoError(t, err) + + // Verify key-2 no longer works + _, err = manager.GetFlagSet(testKey2) + assert.Error(t, err) + }) + + t.Run("handle flagsets without names", func(t *testing.T) { + initialConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: "", // No name + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + }, + } + + manager, err := service.NewFlagsetManager(initialConfig, logger, notifiers) + assert.NoError(t, err) + defer manager.Close() + + // Reload with same configuration (no name still) + newConfig := &config.Config{ + FlagSets: []config.FlagSet{ + { + Name: "", // Still no name + CommonFlagSet: config.CommonFlagSet{ + Retriever: &retrieverconf.RetrieverConf{ + Kind: "file", + Path: flagConfig, + }, + }, + APIKeys: []string{testKey1}, + }, + }, + } + + err = manager.ReloadFlagsets(newConfig, logger, notifiers) + assert.NoError(t, err) + + // Verify it still works + _, err = manager.GetFlagSet(testKey1) + assert.NoError(t, err) + }) +} diff --git a/cmd/relayproxy/testdata/mock/flagset_manager.go b/cmd/relayproxy/testdata/mock/flagset_manager.go index f208a2dcc03..18acf6ed0e2 100644 --- a/cmd/relayproxy/testdata/mock/flagset_manager.go +++ b/cmd/relayproxy/testdata/mock/flagset_manager.go @@ -1,6 +1,11 @@ package mock -import ffclient "github.com/thomaspoignant/go-feature-flag" +import ( + "github.com/thomaspoignant/go-feature-flag/cmd/relayproxy/config" + ffclient "github.com/thomaspoignant/go-feature-flag" + "github.com/thomaspoignant/go-feature-flag/notifier" + "go.uber.org/zap" +) // MockFlagsetManager is a mock implementation for testing error scenarios type MockFlagsetManager struct { @@ -30,6 +35,11 @@ func (m *MockFlagsetManager) IsDefaultFlagSet() bool { return m.IsDefaultFlagSeItem } +func (m *MockFlagsetManager) ReloadFlagsets(newConfig *config.Config, logger *zap.Logger, notifiers []notifier.Notifier) error { + // nothing to do for mock + return nil +} + func (m *MockFlagsetManager) Close() { // nothing to do }