[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

@@ -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"),

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) {

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) {

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
 }