[MM-32390] Config logic refactor (#17578)

* Replace config generator

* Cleanup

* Some renaming and docs additions to add clarity

* Cleanup logging related methods

* Cleanup emitter

* Fix TestDefaultsGenerator

* Move feature flags synchronization logic out of config package

* Remove unnecessary util functions

* Simplify load/set logic

* Refine semantics and add some test to cover them

* Remove unnecessary deep copies

* Improve logic further

* Fix license header

* Review file store tests

* Fix test

* Fix test

* Avoid additional write during initialization

* More consistent naming

* Update app/feature_flags.go

Co-authored-by: Christopher Speller <crspeller@gmail.com>

* Update config/store.go

Co-authored-by: Christopher Speller <crspeller@gmail.com>

* Update config/store.go

Co-authored-by: Christopher Speller <crspeller@gmail.com>

* Update config/store.go

Co-authored-by: Ibrahim Serdar Acikgoz <serdaracikgoz86@gmail.com>

* Move FF synchronizer to its own package

* Remove unidiomatic use of sync.Once

* Add some comments

* Rename function

* More comment

Co-authored-by: Christopher Speller <crspeller@gmail.com>
Co-authored-by: Ibrahim Serdar Acikgoz <serdaracikgoz86@gmail.com>

config/store.go

@@ -4,7 +4,6 @@
 package config
 
 import (
-	"bytes"
 	"encoding/json"
 	"reflect"
 	"sync"
@@ -21,9 +20,23 @@ var (
 	ErrReadOnlyStore = errors.New("configuration store is read-only")
 )
 
-// Listener is a callback function invoked when the configuration changes.
-type Listener func(oldConfig *model.Config, newConfig *model.Config)
+// Store is the higher level object that handles storing and retrieval of config data.
+// To do so it relies on a variety of backing stores (e.g. file, database, memory).
+type Store struct {
+	emitter
+	backingStore BackingStore
 
+	configLock           sync.RWMutex
+	config               *model.Config
+	configNoEnv          *model.Config
+	configCustomDefaults *model.Config
+
+	readOnly   bool
+	readOnlyFF bool
+}
+
+// BackingStore defines the behaviour exposed by the underlying store
+// implementation (e.g. file, database).
 type BackingStore interface {
 	// Set replaces the current configuration in its entirety and updates the backing store.
 	Set(*model.Config) error
@@ -54,27 +67,13 @@ type BackingStore interface {
 	Close() error
 }
 
-// NewStore creates a database or file store given a data source name by which to connect.
-func NewStore(dsn string, watch, readOnly bool, customDefaults *model.Config) (*Store, error) {
-	backingStore, err := getBackingStore(dsn, watch)
-	if err != nil {
-		return nil, err
-	}
-
-	store, err := NewStoreFromBacking(backingStore, customDefaults, readOnly)
-	if err != nil {
-		backingStore.Close()
-		return nil, errors.Wrap(err, "failed to create store")
-	}
-
-	return store, nil
-}
-
+// NewStoreFromBacking creates and returns a new config store given a backing store.
 func NewStoreFromBacking(backingStore BackingStore, customDefaults *model.Config, readOnly bool) (*Store, error) {
 	store := &Store{
 		backingStore:         backingStore,
 		configCustomDefaults: customDefaults,
 		readOnly:             readOnly,
+		readOnlyFF:           true,
 	}
 
 	if err := store.Load(); err != nil {
@@ -90,14 +89,31 @@ func NewStoreFromBacking(backingStore BackingStore, customDefaults *model.Config
 	return store, nil
 }
 
-func getBackingStore(dsn string, watch bool) (BackingStore, error) {
+// NewStoreFromDSN creates and returns a new config store backed by either a database or file store
+// depending on the value of the given data source name string.
+func NewStoreFromDSN(dsn string, watch, readOnly bool, customDefaults *model.Config) (*Store, error) {
+	var err error
+	var backingStore BackingStore
 	if IsDatabaseDSN(dsn) {
-		return NewDatabaseStore(dsn)
+		backingStore, err = NewDatabaseStore(dsn)
+	} else {
+		backingStore, err = NewFileStore(dsn, watch)
+	}
+	if err != nil {
+		return nil, err
 	}
 
-	return NewFileStore(dsn, watch)
+	store, err := NewStoreFromBacking(backingStore, customDefaults, readOnly)
+	if err != nil {
+		backingStore.Close()
+		return nil, errors.Wrap(err, "failed to create store")
+	}
+
+	return store, nil
 }
 
+// NewTestMemoryStore returns a new config store backed by a memory store
+// to be used for testing purposes.
 func NewTestMemoryStore() *Store {
 	memoryStore, err := NewMemoryStore()
 	if err != nil {
@@ -112,19 +128,6 @@ func NewTestMemoryStore() *Store {
 	return configStore
 }
 
-type Store struct {
-	emitter
-	backingStore BackingStore
-
-	configLock           sync.RWMutex
-	config               *model.Config
-	configNoEnv          *model.Config
-	configCustomDefaults *model.Config
-
-	persistFeatureFlags bool
-	readOnly            bool
-}
-
 // Get fetches the current, cached configuration.
 func (s *Store) Get() *model.Config {
 	s.configLock.RLock()
@@ -132,7 +135,7 @@ func (s *Store) Get() *model.Config {
 	return s.config
 }
 
-// Get fetches the current, cached configuration without environment variable overrides.
+// GetNoEnv fetches the current cached configuration without environment variable overrides.
 func (s *Store) GetNoEnv() *model.Config {
 	s.configLock.RLock()
 	defer s.configLock.RUnlock()
@@ -151,33 +154,37 @@ func (s *Store) GetEnvironmentOverridesWithFilter(filter func(reflect.StructFiel
 }
 
 // RemoveEnvironmentOverrides returns a new config without the environment
-// overrides
+// overrides.
 func (s *Store) RemoveEnvironmentOverrides(cfg *model.Config) *model.Config {
 	s.configLock.RLock()
 	defer s.configLock.RUnlock()
 	return removeEnvOverrides(cfg, s.configNoEnv, s.GetEnvironmentOverrides())
 }
 
-// PersistFeatures sets if the store should persist feature flags.
-func (s *Store) PersistFeatures(persist bool) {
+// SetReadOnlyFF sets whether feature flags should be written out to
+// config or treated as read-only.
+func (s *Store) SetReadOnlyFF(readOnly bool) {
 	s.configLock.Lock()
 	defer s.configLock.Unlock()
-	s.persistFeatureFlags = persist
+	s.readOnlyFF = readOnly
 }
 
 // Set replaces the current configuration in its entirety and updates the backing store.
 func (s *Store) Set(newCfg *model.Config) (*model.Config, error) {
 	s.configLock.Lock()
-	var unlockOnce sync.Once
-	defer unlockOnce.Do(s.configLock.Unlock)
+	defer s.configLock.Unlock()
 
 	if s.readOnly {
 		return nil, ErrReadOnlyStore
 	}
 
-	oldCfg := s.config.Clone()
+	newCfg = newCfg.Clone()
+	// no need to clone these as cached configs are getting replaced
+	// with brand new objects.
+	oldCfg := s.config
+	oldCfgNoEnv := s.configNoEnv
 
-	// Really just for some tests we need to set defaults here
+	// Setting defaults allows us to accept partial config objects.
 	newCfg.SetDefaults()
 
 	// Sometimes the config is received with "fake" data in sensitive fields. Apply the real
@@ -188,124 +195,159 @@ func (s *Store) Set(newCfg *model.Config) (*model.Config, error) {
 		return nil, errors.Wrap(err, "new configuration is invalid")
 	}
 
-	newCfg = removeEnvOverrides(newCfg, s.configNoEnv, s.GetEnvironmentOverrides())
+	// We attempt to remove any environment override that may be present in the input config.
+	newCfgNoEnv := removeEnvOverrides(newCfg, oldCfgNoEnv, s.GetEnvironmentOverrides())
 
-	// Don't persist feature flags unless we are on MM cloud
+	// Don't store feature flags unless we are on MM cloud
 	// MM cloud uses config in the DB as a cache of the feature flag
 	// settings in case the management system is down when a pod starts.
-	if !s.persistFeatureFlags {
+
+	// Backing up feature flags section in case we need to restore them later on.
+	oldCfgFF := oldCfg.FeatureFlags
+	oldCfgNoEnvFF := oldCfgNoEnv.FeatureFlags
+	// Clearing FF sections to avoid both comparing and persisting them.
+	if s.readOnlyFF {
+		oldCfg.FeatureFlags = nil
 		newCfg.FeatureFlags = nil
+		newCfgNoEnv.FeatureFlags = nil
 	}
 
-	if err := s.backingStore.Set(newCfg); err != nil {
+	if err := s.backingStore.Set(newCfgNoEnv); err != nil {
 		return nil, errors.Wrap(err, "failed to persist")
 	}
 
-	if err := s.loadLockedWithOld(oldCfg, &unlockOnce); err != nil {
-		return nil, errors.Wrap(err, "failed to load on save")
+	// We apply back environment overrides since the input config may or
+	// may not have them applied.
+	newCfg = applyEnvironmentMap(newCfgNoEnv, GetEnvironment())
+	fixConfig(newCfg)
+	if err := newCfg.IsValid(); err != nil {
+		return nil, errors.Wrap(err, "new configuration is invalid")
+	}
+
+	hasChanged, err := equal(oldCfg, newCfg)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to compare configs")
+	}
+
+	// We restore the previously cleared feature flags sections back.
+	if s.readOnlyFF {
+		oldCfg.FeatureFlags = oldCfgFF
+		newCfg.FeatureFlags = oldCfgFF
+		newCfgNoEnv.FeatureFlags = oldCfgNoEnvFF
+	}
+
+	s.configNoEnv = newCfgNoEnv
+	s.config = newCfg
+
+	if hasChanged {
+		s.configLock.Unlock()
+		s.invokeConfigListeners(oldCfg, newCfg.Clone())
+		s.configLock.Lock()
 	}
 
 	return oldCfg, nil
 }
 
-func (s *Store) loadLockedWithOld(oldCfg *model.Config, unlockOnce *sync.Once) error {
+// Load updates the current configuration from the backing store, possibly initializing.
+func (s *Store) Load() error {
+	s.configLock.Lock()
+	defer s.configLock.Unlock()
+
+	oldCfg := &model.Config{}
+	if s.config != nil {
+		oldCfg = s.config
+	}
+
 	configBytes, err := s.backingStore.Load()
 	if err != nil {
 		return err
 	}
 
-	loadedConfig := &model.Config{}
+	loadedCfg := &model.Config{}
 	if len(configBytes) != 0 {
-		if err = json.Unmarshal(configBytes, &loadedConfig); err != nil {
+		if err = json.Unmarshal(configBytes, &loadedCfg); err != nil {
 			return jsonutils.HumanizeJSONError(err, configBytes)
 		}
 	}
 
-	loadedFeatureFlags := loadedConfig.FeatureFlags
-
 	// If we have custom defaults set, the initial config is merged on
 	// top of them and we delete them not to be used again in the
 	// configuration reloads
 	if s.configCustomDefaults != nil {
 		var mErr error
-		loadedConfig, mErr = Merge(s.configCustomDefaults, loadedConfig, nil)
+		loadedCfg, mErr = Merge(s.configCustomDefaults, loadedCfg, nil)
 		if mErr != nil {
 			return errors.Wrap(mErr, "failed to merge custom config defaults")
 		}
 		s.configCustomDefaults = nil
 	}
 
-	loadedConfig.SetDefaults()
+	// We set the SiteURL to empty (if nil) so that the following call to
+	// SetDefaults() will generate missing data. This avoids an additional write
+	// to the backing store.
+	if loadedCfg.ServiceSettings.SiteURL == nil {
+		loadedCfg.ServiceSettings.SiteURL = model.NewString("")
+	}
 
-	s.configNoEnv = loadedConfig.Clone()
-	fixConfig(s.configNoEnv)
+	// Setting defaults allows us to accept partial config objects.
+	loadedCfg.SetDefaults()
 
-	loadedConfig = applyEnvironmentMap(loadedConfig, GetEnvironment())
+	// No need to clone here since the below call to applyEnvironmentMap
+	// already does that internally.
+	loadedCfgNoEnv := loadedCfg
+	fixConfig(loadedCfgNoEnv)
 
-	fixConfig(loadedConfig)
-
-	if err := loadedConfig.IsValid(); err != nil {
+	loadedCfg = applyEnvironmentMap(loadedCfg, GetEnvironment())
+	fixConfig(loadedCfg)
+	if err := loadedCfg.IsValid(); err != nil {
 		return errors.Wrap(err, "invalid config")
 	}
 
-	// Apply changes that may have happened on load to the backing store.
-	oldCfgBytes, err := json.Marshal(oldCfg)
+	// Backing up feature flags section in case we need to restore them later on.
+	oldCfgFF := oldCfg.FeatureFlags
+	loadedCfgFF := loadedCfg.FeatureFlags
+	loadedCfgNoEnvFF := loadedCfgNoEnv.FeatureFlags
+	// Clearing FF sections to avoid both comparing and persisting them.
+	if s.readOnlyFF {
+		oldCfg.FeatureFlags = nil
+		loadedCfg.FeatureFlags = nil
+		loadedCfgNoEnv.FeatureFlags = nil
+	}
+
+	// Check for changes that may have happened on load to the backing store.
+	hasChanged, err := equal(oldCfg, loadedCfg)
 	if err != nil {
-		return errors.Wrap(err, "failed to marshal old config")
-	}
-	newCfgBytes, err := json.Marshal(loadedConfig)
-	if err != nil {
-		return errors.Wrap(err, "failed to marshal loaded config")
+		return errors.Wrap(err, "failed to compare configs")
 	}
 
-	var shouldStore bool
-	hasChanged := len(configBytes) == 0 || !bytes.Equal(oldCfgBytes, newCfgBytes)
-	if hasChanged {
-		featureFlags := s.configNoEnv.FeatureFlags
-		// Don't persist feature flags unless we are on MM cloud
-		// MM cloud uses config in the DB as a cache of the feature flag
-		// settings in case the management system is down when a pod starts.
-		if !s.persistFeatureFlags {
-			s.configNoEnv.FeatureFlags = loadedFeatureFlags
-		}
-		toStoreBytes, err := json.Marshal(s.configNoEnv)
-		if err != nil {
-			return errors.Wrap(err, "failed to marshal old config")
-		}
-		shouldStore = !bytes.Equal(toStoreBytes, configBytes)
-		// We write back to the backing store only if
-		// the config has changed and the store is not read-only.
-		if !s.readOnly && shouldStore {
-			err := s.backingStore.Set(s.configNoEnv)
-			s.configNoEnv.FeatureFlags = featureFlags
-			if err != nil && !errors.Is(err, ErrReadOnlyConfiguration) {
-				return errors.Wrap(err, "failed to persist")
-			}
+	// We write back to the backing store only if the store is not read-only
+	// and the config has either changed or is missing.
+	if !s.readOnly && (hasChanged || len(configBytes) == 0) {
+		err := s.backingStore.Set(loadedCfgNoEnv)
+		if err != nil && !errors.Is(err, ErrReadOnlyConfiguration) {
+			return errors.Wrap(err, "failed to persist")
 		}
 	}
 
-	s.config = loadedConfig
+	// We restore the previously cleared feature flags sections back.
+	if s.readOnlyFF {
+		oldCfg.FeatureFlags = oldCfgFF
+		loadedCfg.FeatureFlags = loadedCfgFF
+		loadedCfgNoEnv.FeatureFlags = loadedCfgNoEnvFF
+	}
 
-	unlockOnce.Do(s.configLock.Unlock)
+	s.config = loadedCfg
+	s.configNoEnv = loadedCfgNoEnv
 
 	if hasChanged {
-		s.invokeConfigListeners(oldCfg, loadedConfig)
+		s.configLock.Unlock()
+		s.invokeConfigListeners(oldCfg, loadedCfg.Clone())
+		s.configLock.Lock()
 	}
 
 	return nil
 }
 
-// Load updates the current configuration from the backing store, possibly initializing.
-func (s *Store) Load() error {
-	s.configLock.Lock()
-	var unlockOnce sync.Once
-	defer unlockOnce.Do(s.configLock.Unlock)
-
-	oldCfg := s.config.Clone()
-
-	return s.loadLockedWithOld(oldCfg, &unlockOnce)
-}
-
 // GetFile fetches the contents of a previously persisted configuration file.
 // If no such file exists, an empty byte array will be returned without error.
 func (s *Store) GetFile(name string) ([]byte, error) {