From e77a3923c9756018f49261c13cd135baacbc9a96 Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Wed, 3 Feb 2021 21:03:09 +0100 Subject: [PATCH] [MM-32389] Fix FeatureFlags section erroneously getting written to config (#16836) * Fix FeatureFlags section erroneously getting written to config * Avoid invoking config listeners if config has not changed * Avoid resetting feature flags on store creation --- config/common_test.go | 7 ++++++- config/database_test.go | 42 +++++++++++++++++++++++++++++++++++++---- config/file_test.go | 34 ++++++++++++++++++++++++++++----- config/store.go | 31 ++++++++++++++++++++++++++---- 4 files changed, 100 insertions(+), 14 deletions(-) diff --git a/config/common_test.go b/config/common_test.go index 603ce6213c..d5be55252d 100644 --- a/config/common_test.go +++ b/config/common_test.go @@ -14,7 +14,7 @@ import ( "github.com/mattermost/mattermost-server/v5/model" ) -var emptyConfig, readOnlyConfig, minimalConfig, invalidConfig, fixesRequiredConfig, ldapConfig, testConfig, customConfigDefaults *model.Config +var emptyConfig, readOnlyConfig, minimalConfig, minimalConfigNoFF, invalidConfig, fixesRequiredConfig, ldapConfig, testConfig, customConfigDefaults *model.Config func init() { emptyConfig = &model.Config{} @@ -39,7 +39,12 @@ func init() { DefaultClientLocale: sToP("en"), }, } + minimalConfig.SetDefaults() + + minimalConfigNoFF = minimalConfig.Clone() + minimalConfigNoFF.FeatureFlags = nil + invalidConfig = &model.Config{ ServiceSettings: model.ServiceSettings{ SiteURL: sToP("invalid"), diff --git a/config/database_test.go b/config/database_test.go index de5e459ca4..4d6e5f4298 100644 --- a/config/database_test.go +++ b/config/database_test.go @@ -180,7 +180,7 @@ func TestDatabaseStoreNew(t *testing.T) { }) t.Run("already minimally configured", func(t *testing.T) { - _, tearDown := setupConfigDatabase(t, minimalConfig, nil) + _, tearDown := setupConfigDatabase(t, minimalConfigNoFF, nil) defer tearDown() ds, err := newTestDatabaseStore(t, nil) @@ -188,11 +188,11 @@ func TestDatabaseStoreNew(t *testing.T) { defer ds.Close() assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL) - assertDatabaseEqualsConfig(t, minimalConfig) + assertDatabaseEqualsConfig(t, minimalConfigNoFF) }) t.Run("already minimally configured with custom defaults", func(t *testing.T) { - _, tearDown := setupConfigDatabase(t, minimalConfig, nil) + _, tearDown := setupConfigDatabase(t, minimalConfigNoFF, nil) defer tearDown() ds, err := newTestDatabaseStore(t, customConfigDefaults) @@ -203,7 +203,7 @@ func TestDatabaseStoreNew(t *testing.T) { // defaults should have no effect assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL) assert.NotEqual(t, *customConfigDefaults.DisplaySettings.ExperimentalTimezone, *ds.Get().DisplaySettings.ExperimentalTimezone) - assertDatabaseEqualsConfig(t, minimalConfig) + assertDatabaseEqualsConfig(t, minimalConfigNoFF) }) t.Run("invalid url", func(t *testing.T) { @@ -603,6 +603,40 @@ func TestDatabaseStoreSet(t *testing.T) { require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written") }) + + t.Run("setting config without persistent feature flag", func(t *testing.T) { + _, tearDown := setupConfigDatabase(t, minimalConfig, nil) + defer tearDown() + + ds, err := newTestDatabaseStore(t, nil) + require.NoError(t, err) + defer ds.Close() + + ds.PersistFeatures(false) + _, err = ds.Set(minimalConfig) + require.NoError(t, err) + + assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL) + assertDatabaseEqualsConfig(t, minimalConfigNoFF) + }) + + t.Run("setting config with persistent feature flags", func(t *testing.T) { + _, tearDown := setupConfigDatabase(t, minimalConfig, nil) + defer tearDown() + + ds, err := newTestDatabaseStore(t, nil) + require.NoError(t, err) + defer ds.Close() + + ds.PersistFeatures(true) + + _, err = ds.Set(minimalConfig) + require.NoError(t, err) + + assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL) + assertDatabaseEqualsConfig(t, minimalConfig) + }) + } func TestDatabaseStoreLoad(t *testing.T) { diff --git a/config/file_test.go b/config/file_test.go index 34f2467940..e75ba47b92 100644 --- a/config/file_test.go +++ b/config/file_test.go @@ -119,7 +119,7 @@ func TestFileStoreNew(t *testing.T) { }) t.Run("absolute path, already minimally configured", func(t *testing.T) { - path, tearDown := setupConfigFile(t, minimalConfig) + path, tearDown := setupConfigFile(t, minimalConfigNoFF) defer tearDown() fs, err := config.NewFileStore(path, false) @@ -129,11 +129,11 @@ func TestFileStoreNew(t *testing.T) { defer configStore.Close() assert.Equal(t, "http://minimal", *configStore.Get().ServiceSettings.SiteURL) - assertFileEqualsConfig(t, minimalConfig, path) + assertFileEqualsConfig(t, minimalConfigNoFF, path) }) t.Run("absolute path, already minimally configured, with custom defaults", func(t *testing.T) { - path, tearDown := setupConfigFile(t, minimalConfig) + path, tearDown := setupConfigFile(t, minimalConfigNoFF) defer tearDown() fs, err := config.NewFileStore(path, false) @@ -146,7 +146,7 @@ func TestFileStoreNew(t *testing.T) { // defaults should have no effect assert.Equal(t, "http://minimal", *configStore.Get().ServiceSettings.SiteURL) assert.NotEqual(t, *customConfigDefaults.DisplaySettings.ExperimentalTimezone, *configStore.Get().DisplaySettings.ExperimentalTimezone) - assertFileEqualsConfig(t, minimalConfig, path) + assertFileEqualsConfig(t, minimalConfigNoFF, path) }) t.Run("absolute path, file does not exist", func(t *testing.T) { @@ -924,12 +924,36 @@ func TestFileStoreWatcherEmitter(t *testing.T) { fs.AddListener(callback) // Rewrite the config to the file on disk - cfgData, err := config.MarshalConfig(emptyConfig) + cfgData, err := config.MarshalConfig(minimalConfig) require.NoError(t, err) ioutil.WriteFile(path, cfgData, 0644) require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written") }) + + t.Run("no change", func(t *testing.T) { + path, tearDown := setupConfigFile(t, minimalConfig) + defer tearDown() + fsInner, err := config.NewFileStore(path, false) + require.NoError(t, err) + fs, err := config.NewStoreFromBacking(fsInner, nil, false) + require.NoError(t, err) + defer fs.Close() + + // Let the initial call to invokeConfigListeners finish. + time.Sleep(1 * time.Second) + + called := make(chan bool, 1) + callback := func(oldfg, newCfg *model.Config) { + called <- true + } + fs.AddListener(callback) + + _, err = fs.Set(minimalConfig) + require.NoError(t, err) + + require.False(t, wasCalled(called, 1*time.Second), "callback should not have been called since no change has happened") + }) } func TestFileStoreSave(t *testing.T) { diff --git a/config/store.go b/config/store.go index c685be5fca..91b0278054 100644 --- a/config/store.go +++ b/config/store.go @@ -214,6 +214,8 @@ func (s *Store) loadLockedWithOld(oldCfg *model.Config, unlockOnce *sync.Once) e } } + 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 @@ -248,9 +250,28 @@ func (s *Store) loadLockedWithOld(oldCfg *model.Config, unlockOnce *sync.Once) e if err != nil { return errors.Wrap(err, "failed to marshal loaded config") } - if !s.readOnly && (len(configBytes) == 0 || !bytes.Equal(oldCfgBytes, newCfgBytes)) { - if err := s.backingStore.Set(s.configNoEnv); err != nil { - if !errors.Is(err, ErrReadOnlyConfiguration) { + + 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") } } @@ -260,7 +281,9 @@ func (s *Store) loadLockedWithOld(oldCfg *model.Config, unlockOnce *sync.Once) e unlockOnce.Do(s.configLock.Unlock) - s.invokeConfigListeners(oldCfg, loadedConfig) + if hasChanged { + s.invokeConfigListeners(oldCfg, loadedConfig) + } return nil }