MM-28859 Add feature flag managment system using split.io and remove viper. (#15954)

* Add feature flag managment system using split.io and remove viper.

* Fixing tests.

* Attempt to fix postgres tests.

* Fix watch filepath for advanced logging.

* Review fixes.

* Some error wrapping.

* Remove unessisary store interface.

* Desanitize SplitKey

* Simplify.

* Review feedback.

* Rename split mlog adatper to split logger.

* fsInner

* Style.

* Restore oldcfg test.

* Downgrading non-actionable feature flag errors to warnings.

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

config/common.go

@@ -1,177 +0,0 @@
-// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
-// See LICENSE.txt for license information.
-
-package config
-
-import (
-	"bytes"
-	"io"
-	"strings"
-	"sync"
-
-	"github.com/mattermost/mattermost-server/v5/model"
-	"github.com/pkg/errors"
-)
-
-// commonStore enables code sharing between different backing implementations
-type commonStore struct {
-	emitter
-
-	configLock             sync.RWMutex
-	config                 *model.Config
-	configWithoutOverrides *model.Config
-	environmentOverrides   map[string]interface{}
-}
-
-// Get fetches the current, cached configuration.
-func (cs *commonStore) Get() *model.Config {
-	cs.configLock.RLock()
-	defer cs.configLock.RUnlock()
-
-	return cs.config
-}
-
-// GetEnvironmentOverrides fetches the configuration fields overridden by environment variables.
-func (cs *commonStore) GetEnvironmentOverrides() map[string]interface{} {
-	cs.configLock.RLock()
-	defer cs.configLock.RUnlock()
-
-	return cs.environmentOverrides
-}
-
-// set replaces the current configuration in its entirety, and updates the backing store
-// using the persist function argument.
-//
-// This function assumes no lock has been acquired, as it acquires a write lock itself.
-func (cs *commonStore) set(newCfg *model.Config, allowEnvironmentOverrides bool, validate func(*model.Config) error, persist func(*model.Config) error) (*model.Config, error) {
-	cs.configLock.Lock()
-	var unlockOnce sync.Once
-	defer unlockOnce.Do(cs.configLock.Unlock)
-
-	oldCfg := cs.config
-
-	// TODO: disallow attempting to save a directly modified config (comparing pointers). This
-	// wouldn't be an exhaustive check, given the use of pointers throughout the data
-	// structure, but might prevent common mistakes. Requires upstream changes first.
-	// if newCfg == oldCfg {
-	// 	return nil, errors.New("old configuration modified instead of cloning")
-	// }
-
-	// To both clone and re-apply the environment variable overrides we marshal and then
-	// unmarshal the config again.
-	var err error
-	newCfg, _, err = unmarshalConfig(strings.NewReader(newCfg.ToJson()), allowEnvironmentOverrides)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to unmarshal config with env overrides")
-	}
-
-	newCfg.SetDefaults()
-
-	// Sometimes the config is received with "fake" data in sensitive fields. Apply the real
-	// data from the existing config as necessary.
-	desanitize(oldCfg, newCfg)
-
-	if validate != nil {
-		if err := validate(newCfg); err != nil {
-			return nil, errors.Wrap(err, "new configuration is invalid")
-		}
-	}
-
-	if err := persist(cs.RemoveNonPersistable(newCfg)); err != nil {
-		return nil, errors.Wrap(err, "failed to persist")
-	}
-
-	cs.config = newCfg
-
-	unlockOnce.Do(cs.configLock.Unlock)
-
-	// Notify listeners synchronously. Ideally, this would be asynchronous, but existing code
-	// assumes this and there would be increased complexity to avoid racing updates.
-	cs.invokeConfigListeners(oldCfg, newCfg)
-
-	return oldCfg, nil
-}
-
-// load updates the current configuration from the given io.ReadCloser.
-//
-// This function assumes no lock has been acquired, as it acquires a write lock itself.
-func (cs *commonStore) load(f io.ReadCloser, needsSave bool, validate func(*model.Config) error, persist func(*model.Config) error) error {
-	// Duplicate f so that we can read a configuration without applying environment overrides
-	f2 := new(bytes.Buffer)
-	tee := io.TeeReader(f, f2)
-
-	allowEnvironmentOverrides := true
-	loadedCfg, environmentOverrides, err := unmarshalConfig(tee, allowEnvironmentOverrides)
-	if err != nil {
-		return errors.Wrapf(err, "failed to unmarshal config with env overrides")
-	}
-
-	// Keep track of the original values that the Environment settings overrode
-	loadedCfgWithoutEnvOverrides, _, err := unmarshalConfig(f2, false)
-	if err != nil {
-		return errors.Wrapf(err, "failed to unmarshal config without env overrides")
-	}
-
-	// SetDefaults generates various keys and salts if not previously configured. Determine if
-	// such a change will be made before invoking.
-	needsSave = needsSave || loadedCfg.SqlSettings.AtRestEncryptKey == nil || len(*loadedCfg.SqlSettings.AtRestEncryptKey) == 0
-	needsSave = needsSave || loadedCfg.FileSettings.PublicLinkSalt == nil || len(*loadedCfg.FileSettings.PublicLinkSalt) == 0
-
-	loadedCfg.SetDefaults()
-	loadedCfgWithoutEnvOverrides.SetDefaults()
-
-	if validate != nil {
-		if err = validate(loadedCfg); err != nil {
-			return errors.Wrap(err, "invalid config")
-		}
-	}
-
-	if changed := fixConfig(loadedCfg); changed {
-		needsSave = true
-	}
-
-	cs.configLock.Lock()
-	var unlockOnce sync.Once
-	defer unlockOnce.Do(cs.configLock.Unlock)
-
-	if needsSave && persist != nil {
-		cfgWithoutEnvOverrides := removeEnvOverrides(loadedCfg, loadedCfgWithoutEnvOverrides, environmentOverrides)
-		if err = persist(cfgWithoutEnvOverrides); err != nil {
-			return errors.Wrap(err, "failed to persist required changes after load")
-		}
-	}
-
-	oldCfg := cs.config
-	cs.config = loadedCfg
-	cs.configWithoutOverrides = loadedCfgWithoutEnvOverrides
-	cs.environmentOverrides = environmentOverrides
-
-	unlockOnce.Do(cs.configLock.Unlock)
-
-	// Notify listeners synchronously. Ideally, this would be asynchronous, but existing code
-	// assumes this and there would be increased complexity to avoid racing updates.
-	cs.invokeConfigListeners(oldCfg, loadedCfg)
-
-	return nil
-}
-
-// validate checks if the given configuration is valid
-func (cs *commonStore) validate(cfg *model.Config) error {
-	if err := cfg.IsValid(); err != nil {
-		return err
-	}
-
-	return nil
-}
-
-// RemoveEnvironmentOverrides returns a new config without the given environment overrides.
-func (cs *commonStore) RemoveEnvironmentOverrides(cfg *model.Config) *model.Config {
-	return removeEnvOverrides(cfg, cs.configWithoutOverrides, cs.environmentOverrides)
-}
-
-// RemoveNonPersistable removes any aspect of the configuration we do not want to persist
-func (cs *commonStore) RemoveNonPersistable(cfg *model.Config) *model.Config {
-	newCfg := cs.RemoveEnvironmentOverrides(cfg)
-	newCfg.FeatureFlags = nil
-	return newCfg
-}

config/common_test.go

@@ -76,61 +76,63 @@ func init() {
 
 func TestMergeConfigs(t *testing.T) {
 	t.Run("merge two default configs with different salts/keys", func(t *testing.T) {
-		base, err := config.NewMemoryStore()
-		require.NoError(t, err)
-		patch, err := config.NewMemoryStore()
+		base := &model.Config{}
+		base.SetDefaults()
+		patch := &model.Config{}
+		patch.SetDefaults()
+
+		merged, err := config.Merge(base, patch, nil)
 		require.NoError(t, err)
 
-		merged, err := config.Merge(base.Get(), patch.Get(), nil)
-		require.NoError(t, err)
-
-		assert.Equal(t, patch.Get(), merged)
+		assert.Equal(t, patch, merged)
 	})
 	t.Run("merge identical configs", func(t *testing.T) {
-		base, err := config.NewMemoryStore()
-		require.NoError(t, err)
-		patch := base.Get().Clone()
+		base := &model.Config{}
+		base.SetDefaults()
+		patch := base.Clone()
 
-		merged, err := config.Merge(base.Get(), patch, nil)
+		merged, err := config.Merge(base, patch, nil)
 		require.NoError(t, err)
 
-		assert.Equal(t, base.Get(), merged)
+		assert.Equal(t, base, merged)
 		assert.Equal(t, patch, merged)
 	})
 	t.Run("merge configs with a different setting", func(t *testing.T) {
-		base, err := config.NewMemoryStore()
-		require.NoError(t, err)
-		patch := base.Get().Clone()
+		base := &model.Config{}
+		base.SetDefaults()
+		patch := base.Clone()
 		patch.ServiceSettings.SiteURL = newString("http://newhost.ca")
 
-		merged, err := config.Merge(base.Get(), patch, nil)
+		merged, err := config.Merge(base, patch, nil)
 		require.NoError(t, err)
 
-		assert.NotEqual(t, base.Get(), merged)
+		assert.NotEqual(t, base, merged)
 		assert.Equal(t, patch, merged)
 	})
 	t.Run("merge default config with changes from a mostly nil patch", func(t *testing.T) {
-		base, err := config.NewMemoryStore()
-		require.NoError(t, err)
+		base := &model.Config{}
+		base.SetDefaults()
 		patch := &model.Config{}
 		patch.ServiceSettings.SiteURL = newString("http://newhost.ca")
 		patch.GoogleSettings.Enable = newBool(true)
 
-		expected := base.Get().Clone()
+		expected := base.Clone()
 		expected.ServiceSettings.SiteURL = newString("http://newhost.ca")
 		expected.GoogleSettings.Enable = newBool(true)
 
-		merged, err := config.Merge(base.Get(), patch, nil)
+		merged, err := config.Merge(base, patch, nil)
 		require.NoError(t, err)
 
-		assert.NotEqual(t, base.Get(), merged)
+		assert.NotEqual(t, base, merged)
 		assert.NotEqual(t, patch, merged)
 		assert.Equal(t, expected, merged)
 	})
 }
 
 func TestConfigEnvironmentOverrides(t *testing.T) {
-	base, err := config.NewMemoryStore()
+	memstore, err := config.NewMemoryStore()
+	require.NoError(t, err)
+	base, err := config.NewStoreFromBacking(memstore)
 	require.NoError(t, err)
 	originalConfig := &model.Config{}
 	originalConfig.ServiceSettings.SiteURL = newString("http://notoverriden.ca")
@@ -157,7 +159,9 @@ func TestRemoveEnvironmentOverrides(t *testing.T) {
 	os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://overridden.ca")
 	defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
 
-	base, err := config.NewMemoryStore()
+	memstore, err := config.NewMemoryStore()
+	require.NoError(t, err)
+	base, err := config.NewStoreFromBacking(memstore)
 	require.NoError(t, err)
 	oldCfg := base.Get()
 	assert.Equal(t, "http://overridden.ca", *oldCfg.ServiceSettings.SiteURL)
@@ -165,22 +169,5 @@ func TestRemoveEnvironmentOverrides(t *testing.T) {
 	assert.Equal(t, "", *newCfg.ServiceSettings.SiteURL)
 }
 
-func TestRemoveNonPersistable(t *testing.T) {
-	os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://overridden.ca")
-	defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
-
-	base, err := config.NewMemoryStore()
-	require.NoError(t, err)
-	oldCfg := base.Get()
-	assert.Equal(t, "http://overridden.ca", *oldCfg.ServiceSettings.SiteURL)
-	oldCfg.FeatureFlags = &model.FeatureFlags{
-		TestFeature: "teststring",
-	}
-
-	newCfg := base.RemoveNonPersistable(oldCfg)
-	assert.Equal(t, "", *newCfg.ServiceSettings.SiteURL)
-	assert.Nil(t, newCfg.FeatureFlags)
-}
-
 func newBool(b bool) *bool       { return &b }
 func newString(s string) *string { return &s }

config/database.go

@@ -6,7 +6,7 @@ package config
 import (
 	"bytes"
 	"database/sql"
-	"io/ioutil"
+	"encoding/json"
 	"strings"
 
 	"github.com/jmoiron/sqlx"
@@ -28,9 +28,8 @@ import (
 const MaxWriteLength = 4 * 1024 * 1024
 
 // DatabaseStore is a config store backed by a database.
+// Not to be used directly. Only to be used as a backing store for config.Store
 type DatabaseStore struct {
-	commonStore
-
 	originalDsn    string
 	driverName     string
 	dataSourceName string
@@ -59,10 +58,6 @@ func NewDatabaseStore(dsn string) (ds *DatabaseStore, err error) {
 		return nil, errors.Wrap(err, "failed to initialize")
 	}
 
-	if err = ds.Load(); err != nil {
-		return nil, errors.Wrap(err, "failed to load")
-	}
-
 	return ds, nil
 }
 
@@ -159,8 +154,8 @@ func parseDSN(dsn string) (string, string, error) {
 }
 
 // Set replaces the current configuration in its entirety and updates the backing store.
-func (ds *DatabaseStore) Set(newCfg *model.Config) (*model.Config, error) {
-	return ds.commonStore.set(newCfg, true, ds.commonStore.validate, ds.persist)
+func (ds *DatabaseStore) Set(newCfg *model.Config) error {
+	return ds.persist(newCfg)
 }
 
 // maxLength identifies the maximum length of a configuration or configuration file
@@ -232,35 +227,23 @@ func (ds *DatabaseStore) persist(cfg *model.Config) error {
 }
 
 // Load updates the current configuration from the backing store.
-func (ds *DatabaseStore) Load() (err error) {
-	var needsSave bool
+func (ds *DatabaseStore) Load() ([]byte, error) {
 	var configurationData []byte
 
 	row := ds.db.QueryRow("SELECT Value FROM Configurations WHERE Active")
-	if err = row.Scan(&configurationData); err != nil && err != sql.ErrNoRows {
-		return errors.Wrap(err, "failed to query active configuration")
+	if err := row.Scan(&configurationData); err != nil && err != sql.ErrNoRows {
+		return nil, errors.Wrap(err, "failed to query active configuration")
 	}
 
 	// Initialize from the default config if no active configuration could be found.
 	if len(configurationData) == 0 {
-		needsSave = true
-
-		defaultCfg := &model.Config{}
-		defaultCfg.SetDefaults()
-
-		// Assume the database storing the config is also to be used for the application.
-		// This can be overridden using environment variables on first start if necessary,
-		// or changed from the system console afterwards.
-		*defaultCfg.SqlSettings.DriverName = ds.driverName
-		*defaultCfg.SqlSettings.DataSource = ds.dataSourceName
-
-		configurationData, err = marshalConfig(defaultCfg)
-		if err != nil {
-			return errors.Wrap(err, "failed to serialize default config")
-		}
+		configWithDB := model.Config{}
+		configWithDB.SqlSettings.DriverName = model.NewString(ds.driverName)
+		configWithDB.SqlSettings.DataSource = model.NewString(ds.dataSourceName)
+		return json.Marshal(configWithDB)
 	}
 
-	return ds.commonStore.load(ioutil.NopCloser(bytes.NewReader(configurationData)), needsSave, ds.commonStore.validate, ds.persist)
+	return configurationData, nil
 }
 
 // GetFile fetches the contents of a previously persisted configuration file.
@@ -287,7 +270,6 @@ func (ds *DatabaseStore) SetFile(name string, data []byte) error {
 	if err != nil {
 		return errors.Wrap(err, "file data failed length check")
 	}
-
 	params := map[string]interface{}{
 		"name":      name,
 		"data":      data,
@@ -352,8 +334,10 @@ func (ds *DatabaseStore) String() string {
 
 // Close cleans up resources associated with the store.
 func (ds *DatabaseStore) Close() error {
-	ds.configLock.Lock()
-	defer ds.configLock.Unlock()
-
 	return ds.db.Close()
 }
+
+// Watch nothing on memory store
+func (ds *DatabaseStore) Watch(_ func()) error {
+	return nil
+}

config/database_test.go

@@ -5,6 +5,7 @@ package config_test
 
 import (
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"os"
 	"strings"
@@ -79,7 +80,8 @@ func getActualDatabaseConfig(t *testing.T) (string, *model.Config) {
 		err := db.Get(&actual, "SELECT Id, Value FROM Configurations WHERE Active")
 		require.NoError(t, err)
 
-		actualCfg, _, err := config.UnmarshalConfig(bytes.NewReader(actual.Value), false)
+		var actualCfg *model.Config
+		err = json.Unmarshal(actual.Value, &actualCfg)
 		require.Nil(t, err)
 		return actual.ID, actualCfg
 	}
@@ -91,7 +93,8 @@ func getActualDatabaseConfig(t *testing.T) (string, *model.Config) {
 	err := db.Get(&actual, "SELECT Id, Value FROM Configurations WHERE Active")
 	require.NoError(t, err)
 
-	actualCfg, _, err := config.UnmarshalConfig(bytes.NewReader(actual.Value), false)
+	var actualCfg *model.Config
+	err = json.Unmarshal(actual.Value, &actualCfg)
 	require.Nil(t, err)
 	return actual.ID, actualCfg
 }
@@ -112,6 +115,17 @@ func assertDatabaseNotEqualsConfig(t *testing.T, expectedCfg *model.Config) {
 	assert.NotEqual(t, expectedCfg, actualCfg)
 }
 
+func newTestDatabaseStore(t *testing.T) (*config.Store, error) {
+	sqlSettings := mainHelper.GetSQLSettings()
+	dss, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+	require.NoError(t, err)
+
+	cStore, err := config.NewStoreFromBacking(dss)
+	require.NoError(t, err)
+
+	return cStore, nil
+}
+
 func TestDatabaseStoreNew(t *testing.T) {
 	if testing.Short() {
 		t.SkipNow()
@@ -119,7 +133,7 @@ func TestDatabaseStoreNew(t *testing.T) {
 	sqlSettings := mainHelper.GetSQLSettings()
 
 	t.Run("no existing configuration - initialization required", func(t *testing.T) {
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -130,7 +144,7 @@ func TestDatabaseStoreNew(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, testConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -142,7 +156,7 @@ func TestDatabaseStoreNew(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -173,8 +187,7 @@ func TestDatabaseStoreGet(t *testing.T) {
 	_, tearDown := setupConfigDatabase(t, testConfig, nil)
 	defer tearDown()
 
-	sqlSettings := mainHelper.GetSQLSettings()
-	ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+	ds, err := newTestDatabaseStore(t)
 	require.NoError(t, err)
 	defer ds.Close()
 
@@ -185,13 +198,6 @@ func TestDatabaseStoreGet(t *testing.T) {
 	assert.Equal(t, "http://TestStoreNew", *cfg.ServiceSettings.SiteURL)
 
 	assert.True(t, cfg == cfg2, "Get() returned different configuration instances")
-
-	newCfg := &model.Config{}
-	oldCfg, err := ds.Set(newCfg)
-	require.NoError(t, err)
-
-	assert.True(t, oldCfg == cfg, "returned config after set() changed original")
-	assert.False(t, newCfg == cfg, "returned config should have been different from original")
 }
 
 func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
@@ -199,8 +205,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, testConfig, nil)
 		defer tearDown()
 
-		sqlSettings := mainHelper.GetSQLSettings()
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -210,7 +215,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override")
 		defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
 
-		ds, err = config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err = newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -222,8 +227,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, testConfig, nil)
 		defer tearDown()
 
-		sqlSettings := mainHelper.GetSQLSettings()
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -233,7 +237,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_PLUGINSETTINGS_ENABLEUPLOADS", "true")
 		defer os.Unsetenv("MM_PLUGINSETTINGS_ENABLEUPLOADS")
 
-		ds, err = config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err = newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -245,8 +249,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, testConfig, nil)
 		defer tearDown()
 
-		sqlSettings := mainHelper.GetSQLSettings()
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -256,7 +259,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM", "3000")
 		defer os.Unsetenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM")
 
-		ds, err = config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err = newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -268,8 +271,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, testConfig, nil)
 		defer tearDown()
 
-		sqlSettings := mainHelper.GetSQLSettings()
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -279,7 +281,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE", "123456")
 		defer os.Unsetenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE")
 
-		ds, err = config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err = newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -291,8 +293,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, testConfig, nil)
 		defer tearDown()
 
-		sqlSettings := mainHelper.GetSQLSettings()
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -302,7 +303,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db")
 		defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS")
 
-		ds, err = config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err = newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -317,8 +318,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, testConfig, nil)
 		defer tearDown()
 
-		sqlSettings := mainHelper.GetSQLSettings()
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -328,7 +328,7 @@ func TestDatabaseStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db user:pwd@db2:5433/test-db2 user:pwd@db3:5434/test-db3")
 		defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS")
 
-		ds, err = config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err = newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -341,7 +341,6 @@ func TestDatabaseStoreSet(t *testing.T) {
 	if testing.Short() {
 		t.SkipNow()
 	}
-	sqlSettings := mainHelper.GetSQLSettings()
 
 	t.Run("set same pointer value", func(t *testing.T) {
 		t.Skip("not yet implemented")
@@ -349,7 +348,7 @@ func TestDatabaseStoreSet(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -363,17 +362,14 @@ func TestDatabaseStoreSet(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
-		oldCfg := ds.Get()
-
 		newCfg := &model.Config{}
 
-		retCfg, err := ds.Set(newCfg)
+		_, err = ds.Set(newCfg)
 		require.NoError(t, err)
-		assert.Equal(t, oldCfg, retCfg)
 
 		assert.Equal(t, "", *ds.Get().ServiceSettings.SiteURL)
 	})
@@ -382,18 +378,15 @@ func TestDatabaseStoreSet(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, ldapConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
-		oldCfg := ds.Get()
-
 		newCfg := &model.Config{}
 		newCfg.LdapSettings.BindPassword = sToP(model.FAKE_SETTING)
 
-		retCfg, err := ds.Set(newCfg)
+		_, err = ds.Set(newCfg)
 		require.NoError(t, err)
-		assert.Equal(t, oldCfg, retCfg)
 
 		assert.Equal(t, "password", *ds.Get().LdapSettings.BindPassword)
 	})
@@ -402,7 +395,7 @@ func TestDatabaseStoreSet(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -421,7 +414,7 @@ func TestDatabaseStoreSet(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -440,7 +433,7 @@ func TestDatabaseStoreSet(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, readOnlyConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -460,7 +453,7 @@ func TestDatabaseStoreSet(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -483,10 +476,11 @@ func TestDatabaseStoreSet(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
+		sqlSettings := mainHelper.GetSQLSettings()
 		db := sqlx.NewDb(mainHelper.GetSQLSupplier().GetMaster().Db, *sqlSettings.DriverName)
 		_, err = db.Exec("DROP TABLE Configurations")
 		require.NoError(t, err)
@@ -507,7 +501,7 @@ func TestDatabaseStoreSet(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -524,12 +518,10 @@ func TestDatabaseStoreSet(t *testing.T) {
 		activeID, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
-		oldCfg := ds.Get()
-
 		called := make(chan bool, 1)
 		callback := func(oldfg, newCfg *model.Config) {
 			called <- true
@@ -538,9 +530,8 @@ func TestDatabaseStoreSet(t *testing.T) {
 
 		newCfg := &model.Config{}
 
-		retCfg, err := ds.Set(newCfg)
+		_, err = ds.Set(newCfg)
 		require.NoError(t, err)
-		assert.Equal(t, oldCfg, retCfg)
 
 		id, _ := getActualDatabaseConfig(t)
 		assert.NotEqual(t, activeID, id, "new record should have been written")
@@ -553,13 +544,12 @@ func TestDatabaseStoreLoad(t *testing.T) {
 	if testing.Short() {
 		t.SkipNow()
 	}
-	sqlSettings := mainHelper.GetSQLSettings()
 
 	t.Run("active configuration no longer exists", func(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -574,7 +564,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -596,7 +586,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://overridePersistEnvVariables")
 		defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -617,7 +607,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		os.Setenv("MM_PLUGINSETTINGS_ENABLEUPLOADS", "true")
 		defer os.Unsetenv("MM_PLUGINSETTINGS_ENABLEUPLOADS")
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -640,7 +630,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		os.Setenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM", "3000")
 		defer os.Unsetenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM")
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -663,7 +653,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		os.Setenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE", "123456")
 		defer os.Unsetenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE")
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -686,7 +676,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db")
 		defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS")
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -711,7 +701,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db")
 		defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS")
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -731,13 +721,14 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
 		cfgData, err := config.MarshalConfig(invalidConfig)
 		require.NoError(t, err)
 
+		sqlSettings := mainHelper.GetSQLSettings()
 		db := sqlx.NewDb(mainHelper.GetSQLSupplier().GetMaster().Db, *sqlSettings.DriverName)
 		truncateTables(t)
 		id := model.NewId()
@@ -758,7 +749,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, fixesRequiredConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -772,7 +763,7 @@ func TestDatabaseStoreLoad(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -796,7 +787,7 @@ func TestDatabaseGetFile(t *testing.T) {
 	})
 	defer tearDown()
 
-	ds, err := config.NewDatabaseStore(getDsn(*mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+	ds, err := newTestDatabaseStore(t)
 	require.NoError(t, err)
 	defer ds.Close()
 
@@ -827,7 +818,7 @@ func TestDatabaseSetFile(t *testing.T) {
 	_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 	defer tearDown()
 
-	ds, err := config.NewDatabaseStore(getDsn(*mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+	ds, err := newTestDatabaseStore(t)
 	require.NoError(t, err)
 	defer ds.Close()
 
@@ -880,7 +871,7 @@ func TestDatabaseHasFile(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -893,7 +884,7 @@ func TestDatabaseHasFile(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -911,7 +902,7 @@ func TestDatabaseHasFile(t *testing.T) {
 		})
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -924,7 +915,7 @@ func TestDatabaseHasFile(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -939,7 +930,7 @@ func TestDatabaseRemoveFile(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -951,7 +942,7 @@ func TestDatabaseRemoveFile(t *testing.T) {
 		_, tearDown := setupConfigDatabase(t, minimalConfig, nil)
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -975,7 +966,7 @@ func TestDatabaseRemoveFile(t *testing.T) {
 		})
 		defer tearDown()
 
-		ds, err := config.NewDatabaseStore(getDsn(*mainHelper.Settings.DriverName, *mainHelper.Settings.DataSource))
+		ds, err := newTestDatabaseStore(t)
 		require.NoError(t, err)
 		defer ds.Close()
 
@@ -998,8 +989,7 @@ func TestDatabaseStoreString(t *testing.T) {
 	_, tearDown := setupConfigDatabase(t, emptyConfig, nil)
 	defer tearDown()
 
-	sqlSettings := mainHelper.GetSQLSettings()
-	ds, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource))
+	ds, err := newTestDatabaseStore(t)
 	require.NoError(t, err)
 	require.NotNil(t, ds)
 	defer ds.Close()

config/environment.go

@@ -4,11 +4,122 @@
 package config
 
 import (
+	"os"
 	"reflect"
+	"strconv"
+	"strings"
 
 	"github.com/mattermost/mattermost-server/v5/model"
 )
 
+func GetEnvironment() map[string]string {
+	mmenv := make(map[string]string)
+	for _, env := range os.Environ() {
+		kv := strings.SplitN(env, "=", 2)
+		key := strings.ToUpper(kv[0])
+		if strings.HasPrefix(key, "MM") {
+			mmenv[key] = kv[1]
+		}
+	}
+
+	return mmenv
+}
+
+func applyEnvKey(key, value string, rValueSubject reflect.Value) {
+	keyParts := strings.SplitN(key, "_", 2)
+	if len(keyParts) < 1 {
+		return
+	}
+	rFieldValue := rValueSubject.FieldByNameFunc(func(candidate string) bool {
+		candidateUpper := strings.ToUpper(candidate)
+		return candidateUpper == keyParts[0]
+	})
+
+	if !rFieldValue.IsValid() {
+		return
+	}
+
+	if rFieldValue.Kind() == reflect.Ptr {
+		rFieldValue = rFieldValue.Elem()
+		if !rFieldValue.IsValid() {
+			return
+		}
+	}
+
+	switch rFieldValue.Kind() {
+	case reflect.Struct:
+		// If we have only one part left, we can't deal with a struct
+		// the env var is incomplete so give up.
+		if len(keyParts) < 2 {
+			return
+		}
+		applyEnvKey(keyParts[1], value, rFieldValue)
+	case reflect.String:
+		rFieldValue.Set(reflect.ValueOf(value))
+	case reflect.Bool:
+		boolVal, err := strconv.ParseBool(value)
+		if err == nil {
+			rFieldValue.Set(reflect.ValueOf(boolVal))
+		}
+	case reflect.Int:
+		intVal, err := strconv.ParseInt(value, 10, 0)
+		if err == nil {
+			rFieldValue.Set(reflect.ValueOf(int(intVal)))
+		}
+	case reflect.Int64:
+		intVal, err := strconv.ParseInt(value, 10, 0)
+		if err == nil {
+			rFieldValue.Set(reflect.ValueOf(intVal))
+		}
+	case reflect.SliceOf(reflect.TypeOf("")).Kind():
+		rFieldValue.Set(reflect.ValueOf(strings.Split(value, " ")))
+	}
+}
+
+func applyEnvironmentMap(inputConfig *model.Config, env map[string]string) *model.Config {
+	appliedConfig := inputConfig.Clone()
+
+	rvalConfig := reflect.ValueOf(appliedConfig).Elem()
+	for envKey, envValue := range env {
+		applyEnvKey(strings.TrimPrefix(envKey, "MM_"), envValue, rvalConfig)
+	}
+
+	return appliedConfig
+}
+
+// generateEnvironmentMap creates a map[string]interface{} containing true at the leaves mirroring the
+// configuration structure so the client can know which env variables are overridden
+func generateEnvironmentMap(env map[string]string) map[string]interface{} {
+	rType := reflect.TypeOf(model.Config{})
+	return generateEnvironmentMapWithBaseKey(env, rType, "MM")
+}
+
+func generateEnvironmentMapWithBaseKey(env map[string]string, rType reflect.Type, base string) map[string]interface{} {
+	if rType.Kind() != reflect.Struct {
+		return nil
+	}
+
+	mapRepresentation := make(map[string]interface{})
+	for i := 0; i < rType.NumField(); i++ {
+		rField := rType.Field(i)
+		if rField.Type.Kind() == reflect.Struct {
+			if val := generateEnvironmentMapWithBaseKey(env, rField.Type, base+"_"+rField.Name); val != nil {
+				mapRepresentation[rField.Name] = val
+			}
+		} else {
+			if _, ok := env[strings.ToUpper(base+"_"+rField.Name)]; ok {
+				mapRepresentation[rField.Name] = true
+			}
+		}
+	}
+
+	if len(mapRepresentation) == 0 {
+		return nil
+	}
+
+	return mapRepresentation
+}
+
 // removeEnvOverrides returns a new config without the given environment overrides.
 // If a config variable has an environment override, that variable is set to the value that was
 // read from the store.

config/environment_test.go

@@ -10,28 +10,118 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func modifiedDefault(modify func(*model.Config)) *model.Config {
+	def := defaultConfig()
+	modify(def)
+	return def
+}
+
+func defaultConfig() *model.Config {
+	def := &model.Config{}
+	def.SetDefaults()
+	return def
+}
+
 func TestRemoveEnvOverrides(t *testing.T) {
-	defaultCfg := &model.Config{}
-	defaultCfg.SetDefaults()
-
-	newCfg := defaultCfg.Clone()
-	newCfg.EmailSettings.EnableSignUpWithEmail = model.NewBool(false)
-
-	envOverrides := map[string]interface{}{
-		"EmailSettings": map[string]interface{}{
-			"EnableSignUpWithEmail": false,
+	var tests = []struct {
+		name           string
+		inputConfig    *model.Config
+		env            map[string]string
+		expectedConfig *model.Config
+	}{
+		{
+			name: "basic override",
+			inputConfig: modifiedDefault(func(in *model.Config) {
+				*in.ServiceSettings.TLSMinVer = "1.4"
+			}),
+			env: map[string]string{
+				"MM_SERVICESETTINGS_TLSMINVER": "1.5",
+			},
+			expectedConfig: modifiedDefault(func(in *model.Config) {
+				*in.ServiceSettings.TLSMinVer = "1.5"
+			}),
+		},
+		{
+			name: "feature flags",
+			inputConfig: modifiedDefault(func(in *model.Config) {
+				in.FeatureFlags.TestFeature = "somevalue"
+			}),
+			env: map[string]string{
+				"MM_FEATUREFLAGS_TESTFEATURE": "correctvalue",
+			},
+			expectedConfig: modifiedDefault(func(in *model.Config) {
+				in.FeatureFlags.TestFeature = "correctvalue"
+			}),
+		},
+		{
+			name: "int setting",
+			inputConfig: modifiedDefault(func(in *model.Config) {
+				*in.ClusterSettings.GossipPort = 500
+			}),
+			env: map[string]string{
+				"MM_CLUSTERSETTINGS_GOSSIPPORT": "600",
+			},
+			expectedConfig: modifiedDefault(func(in *model.Config) {
+				*in.ClusterSettings.GossipPort = 600
+			}),
+		},
+		{
+			name: "int64 setting",
+			inputConfig: modifiedDefault(func(in *model.Config) {
+				*in.ServiceSettings.TLSStrictTransportMaxAge = 500
+			}),
+			env: map[string]string{
+				"MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE": "4294967294",
+			},
+			expectedConfig: modifiedDefault(func(in *model.Config) {
+				*in.ServiceSettings.TLSStrictTransportMaxAge = 4294967294
+			}),
+		},
+		{
+			name: "bool setting",
+			inputConfig: modifiedDefault(func(in *model.Config) {
+				*in.ClusterSettings.UseIpAddress = false
+			}),
+			env: map[string]string{
+				"MM_CLUSTERSETTINGS_USEIPADDRESS": "true",
+			},
+			expectedConfig: modifiedDefault(func(in *model.Config) {
+				*in.ClusterSettings.UseIpAddress = true
+			}),
+		},
+		{
+			name: "[]string setting",
+			inputConfig: modifiedDefault(func(in *model.Config) {
+				in.SqlSettings.DataSourceReplicas = []string{"somthing"}
+			}),
+			env: map[string]string{
+				"MM_SQLSETTINGS_DATASOURCEREPLICAS": "otherthing alsothis",
+			},
+			expectedConfig: modifiedDefault(func(in *model.Config) {
+				in.SqlSettings.DataSourceReplicas = []string{"otherthing", "alsothis"}
+			}),
+		},
+		{
+			name: "bad env",
+			inputConfig: modifiedDefault(func(in *model.Config) {
+			}),
+			env: map[string]string{
+				"MM_SERVICESETTINGS":        "huh?",
+				"NOTMM":                     "huh?",
+				"MM_NOTEXIST":               "huh?",
+				"MM_NOTEXIST_MORE_AND_MORE": "huh?",
+				"MM_":                       "huh?",
+				"MM":                        "huh?",
+				"MM__":                      "huh?",
+				"_":                         "huh?",
+			},
+			expectedConfig: modifiedDefault(func(in *model.Config) {
+			}),
 		},
 	}
-
-	updatedCfg := removeEnvOverrides(newCfg, defaultCfg, envOverrides)
-	require.NotNil(t, updatedCfg)
-	require.True(t, *updatedCfg.EmailSettings.EnableSignUpWithEmail)
-
-	envOverrides["ServiceSettings"] = map[string]interface{}{
-		"NonExistentConfig": true,
+	for _, testCase := range tests {
+		t.Run(testCase.name, func(t *testing.T) {
+			require.Equal(t, testCase.expectedConfig, applyEnvironmentMap(testCase.inputConfig, testCase.env))
+		})
 	}
-
-	require.NotPanics(t, func() {
-		_ = removeEnvOverrides(defaultCfg, defaultCfg, envOverrides)
-	}, "invalid setting should not panic")
 }

config/export_test.go

@@ -4,8 +4,6 @@
 package config
 
 import (
-	"io"
-
 	"github.com/jmoiron/sqlx"
 	"github.com/mattermost/mattermost-server/v5/model"
 )
@@ -15,11 +13,6 @@ func MarshalConfig(cfg *model.Config) ([]byte, error) {
 	return marshalConfig(cfg)
 }
 
-// UnmarshalConfig exposes the internal unmarshalConfig to tests only.
-func UnmarshalConfig(r io.Reader, allowEnvironmentOverrides bool) (*model.Config, map[string]interface{}, error) {
-	return unmarshalConfig(r, allowEnvironmentOverrides)
-}
-
 // InitializeConfigurationsTable exposes the internal initializeConfigurationsTable to test only.
 func InitializeConfigurationsTable(db *sqlx.DB) error {
 	return initializeConfigurationsTable(db)

config/feature_flags.go

@@ -0,0 +1,99 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package config
+
+import (
+	"math"
+	"reflect"
+
+	"github.com/mattermost/mattermost-server/v5/mlog"
+	"github.com/mattermost/mattermost-server/v5/model"
+	"github.com/pkg/errors"
+	"github.com/splitio/go-client/v6/splitio/client"
+	"github.com/splitio/go-client/v6/splitio/conf"
+)
+
+type FeatureFlagSyncParams struct {
+	ServerID            string
+	SplitKey            string
+	SyncIntervalSeconds int
+	Log                 *mlog.Logger
+}
+
+type FeatureFlagSynchronizer struct {
+	FeatureFlagSyncParams
+
+	client  *client.SplitClient
+	stop    chan struct{}
+	stopped chan struct{}
+}
+
+var featureNames = getStructFields(model.FeatureFlags{})
+
+func NewFeatureFlagSynchronizer(params FeatureFlagSyncParams) (*FeatureFlagSynchronizer, error) {
+	cfg := conf.Default()
+	if params.Log != nil {
+		cfg.Logger = &splitLogger{wrappedLog: params.Log.With(mlog.String("service", "split"))}
+	} else {
+		cfg.LoggerConfig.LogLevel = math.MinInt32
+	}
+	factory, err := client.NewSplitFactory(params.SplitKey, cfg)
+	if err != nil {
+		return nil, errors.Wrap(err, "unable to create split factory")
+	}
+
+	return &FeatureFlagSynchronizer{
+		FeatureFlagSyncParams: params,
+		client:                factory.Client(),
+		stop:                  make(chan struct{}),
+		stopped:               make(chan struct{}),
+	}, nil
+}
+
+// ensureReady blocks until the syncronizer is ready to update feature flag values
+func (f *FeatureFlagSynchronizer) EnsureReady() error {
+	if err := f.client.BlockUntilReady(10); err != nil {
+		return errors.Wrap(err, "split.io client could not initalize")
+	}
+
+	return nil
+}
+
+func (f *FeatureFlagSynchronizer) UpdateFeatureFlagValues(base model.FeatureFlags) model.FeatureFlags {
+	featuresMap := f.client.Treatments(f.ServerID, featureNames, nil)
+	ffm := featureFlagsFromMap(featuresMap, base)
+	return ffm
+}
+
+func (f *FeatureFlagSynchronizer) Close() {
+	f.client.Destroy()
+}
+
+// featureFlagsFromMap sets the feature flags from a map[string]string.
+// It starts with baseFeatureFlags and only sets values that are
+// given by the upstream management system.
+// Makes the assumption that all feature flags are strings for now.
+func featureFlagsFromMap(featuresMap map[string]string, baseFeatureFlags model.FeatureFlags) model.FeatureFlags {
+	refStruct := reflect.ValueOf(&baseFeatureFlags).Elem()
+	for fieldName, fieldValue := range featuresMap {
+		refField := refStruct.FieldByName(fieldName)
+		// "control" is returned by split.io if the treatment is not found, in this case we should use the default value.
+		if !refField.IsValid() || !refField.CanSet() || fieldValue == "control" {
+			continue
+		}
+
+		refField.Set(reflect.ValueOf(fieldValue))
+	}
+	return baseFeatureFlags
+}
+
+func getStructFields(s interface{}) []string {
+	structType := reflect.TypeOf(s)
+	fieldNames := make([]string, 0, structType.NumField())
+	for i := 0; i < structType.NumField(); i++ {
+		fieldNames = append(fieldNames, structType.Field(i).Name)
+	}
+
+	return fieldNames
+}

config/feature_flags_test.go

@@ -0,0 +1,75 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package config
+
+import (
+	"testing"
+
+	"github.com/mattermost/mattermost-server/v5/model"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGetStructFields(t *testing.T) {
+	type testStruct struct {
+		FieldOne       string
+		SecondField    bool
+		SomeOtherField int
+	}
+
+	fields := getStructFields(testStruct{})
+	require.Equal(t,
+		[]string{
+			"FieldOne",
+			"SecondField",
+			"SomeOtherField",
+		},
+		fields,
+	)
+
+	featureFlagsFields := getStructFields(model.FeatureFlags{})
+	require.Contains(t, featureFlagsFields, "TestFeature")
+}
+
+func TestFeatureFlagsFromMap(t *testing.T) {
+	for name, tc := range map[string]struct {
+		FeatureMap        map[string]string
+		Base              model.FeatureFlags
+		ExpectedTestValue string
+	}{
+		"empty": {
+			FeatureMap:        map[string]string{},
+			Base:              model.FeatureFlags{},
+			ExpectedTestValue: "",
+		},
+		"no base value": {
+			FeatureMap:        map[string]string{"TestFeature": "expectedvalue"},
+			Base:              model.FeatureFlags{},
+			ExpectedTestValue: "expectedvalue",
+		},
+		"only base value": {
+			FeatureMap:        map[string]string{},
+			Base:              model.FeatureFlags{TestFeature: "somebasevalue"},
+			ExpectedTestValue: "somebasevalue",
+		},
+		"override base value": {
+			FeatureMap:        map[string]string{"TestFeature": "overridevalue"},
+			Base:              model.FeatureFlags{TestFeature: "somebasevalue"},
+			ExpectedTestValue: "overridevalue",
+		},
+		"override base value with extras": {
+			FeatureMap:        map[string]string{"TestFeature": "overridevalue", "SomeOldFlag": "oldvalue"},
+			Base:              model.FeatureFlags{TestFeature: "somebasevalue"},
+			ExpectedTestValue: "overridevalue",
+		},
+		"all values do not exist": {
+			FeatureMap:        map[string]string{"SomeOldFlag": "oldvalue"},
+			Base:              model.FeatureFlags{},
+			ExpectedTestValue: "",
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			require.Equal(t, tc.ExpectedTestValue, featureFlagsFromMap(tc.FeatureMap, tc.Base).TestFeature)
+		})
+	}
+}

config/file.go

@@ -4,9 +4,7 @@
 package config
 
 import (
-	"bytes"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -26,12 +24,12 @@ var (
 // FileStore is a config store backed by a file such as config/config.json.
 //
 // It also uses the folder containing the configuration file for storing other configuration files.
+// Not to be used directly. Only to be used as a backing store for config.Store
 type FileStore struct {
-	commonStore
-
-	path    string
-	watch   bool
-	watcher *watcher
+	path     string
+	watch    bool
+	watcher  *watcher
+	callback func()
 }
 
 // NewFileStore creates a new instance of a config store backed by the given file path.
@@ -47,15 +45,6 @@ func NewFileStore(path string, watch bool) (fs *FileStore, err error) {
 		path:  resolvedPath,
 		watch: watch,
 	}
-	if err = fs.Load(); err != nil {
-		return nil, errors.Wrap(err, "failed to load")
-	}
-
-	if fs.watch {
-		if err = fs.startWatcher(); err != nil {
-			mlog.Error("failed to start config watcher", mlog.String("path", path), mlog.Err(err))
-		}
-	}
 
 	return fs, nil
 }
@@ -104,19 +93,21 @@ func (fs *FileStore) resolveFilePath(name string) string {
 }
 
 // Set replaces the current configuration in its entirety and updates the backing store.
-func (fs *FileStore) Set(newCfg *model.Config) (*model.Config, error) {
-	return fs.commonStore.set(newCfg, true, func(cfg *model.Config) error {
-		if *fs.config.ClusterSettings.Enable && *fs.config.ClusterSettings.ReadOnlyConfig {
-			return ErrReadOnlyConfiguration
-		}
+func (fs *FileStore) Set(newCfg *model.Config) error {
+	if *newCfg.ClusterSettings.Enable && *newCfg.ClusterSettings.ReadOnlyConfig {
+		return ErrReadOnlyConfiguration
+	}
 
-		return fs.commonStore.validate(cfg)
-	}, fs.persist)
+	return fs.persist(newCfg)
 }
 
 // persist writes the configuration to the configured file.
 func (fs *FileStore) persist(cfg *model.Config) error {
-	fs.stopWatcher()
+	needsRestart := false
+	if fs.watcher != nil {
+		fs.stopWatcher()
+		needsRestart = true
+	}
 
 	b, err := marshalConfig(cfg)
 	if err != nil {
@@ -128,8 +119,8 @@ func (fs *FileStore) persist(cfg *model.Config) error {
 		return errors.Wrap(err, "failed to write file")
 	}
 
-	if fs.watch {
-		if err = fs.startWatcher(); err != nil {
+	if fs.watch && needsRestart {
+		if err = fs.Watch(fs.callback); err != nil {
 			mlog.Error("failed to start config watcher", mlog.String("path", fs.path), mlog.Err(err))
 		}
 	}
@@ -138,35 +129,22 @@ func (fs *FileStore) persist(cfg *model.Config) error {
 }
 
 // Load updates the current configuration from the backing store.
-func (fs *FileStore) Load() (err error) {
-	var needsSave bool
-	var f io.ReadCloser
-
-	f, err = os.Open(fs.path)
+func (fs *FileStore) Load() ([]byte, error) {
+	f, err := os.Open(fs.path)
 	if os.IsNotExist(err) {
-		needsSave = true
-		defaultCfg := &model.Config{}
-		defaultCfg.SetDefaults()
-
-		var defaultCfgBytes []byte
-		defaultCfgBytes, err = marshalConfig(defaultCfg)
-		if err != nil {
-			return errors.Wrap(err, "failed to serialize default config")
-		}
-
-		f = ioutil.NopCloser(bytes.NewReader(defaultCfgBytes))
+		return nil, nil
 
 	} else if err != nil {
-		return errors.Wrapf(err, "failed to open %s for reading", fs.path)
+		return nil, errors.Wrapf(err, "failed to open %s for reading", fs.path)
 	}
-	defer func() {
-		closeErr := f.Close()
-		if err == nil && closeErr != nil {
-			err = errors.Wrap(closeErr, "failed to close")
-		}
-	}()
+	defer f.Close()
 
-	return fs.commonStore.load(f, needsSave, fs.commonStore.validate, fs.persist)
+	fileBytes, err := ioutil.ReadAll(f)
+	if err != nil {
+		return nil, err
+	}
+
+	return fileBytes, nil
 }
 
 // GetFile fetches the contents of a previously persisted configuration file.
@@ -237,17 +215,13 @@ func (fs *FileStore) RemoveFile(name string) error {
 	return nil
 }
 
-// startWatcher starts a watcher to monitor for external config file changes.
-func (fs *FileStore) startWatcher() error {
-	if fs.watcher != nil {
+func (fs *FileStore) Watch(callback func()) error {
+	if fs.watcher != nil || !fs.watch {
 		return nil
 	}
 
-	watcher, err := newWatcher(fs.path, func() {
-		if err := fs.Load(); err != nil {
-			mlog.Error("failed to reload file on change", mlog.String("path", fs.path), mlog.Err(err))
-		}
-	})
+	fs.callback = callback
+	watcher, err := newWatcher(fs.path, callback)
 	if err != nil {
 		return err
 	}
@@ -276,9 +250,6 @@ func (fs *FileStore) String() string {
 
 // Close cleans up resources associated with the store.
 func (fs *FileStore) Close() error {
-	fs.configLock.Lock()
-	defer fs.configLock.Unlock()
-
 	fs.stopWatcher()
 
 	return nil

config/file_test.go

@@ -4,6 +4,7 @@
 package config_test
 
 import (
+	"encoding/json"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -56,7 +57,8 @@ func getActualFileConfig(t *testing.T, path string) *model.Config {
 	require.Nil(t, err)
 	defer f.Close()
 
-	actualCfg, _, err := config.UnmarshalConfig(f, false)
+	var actualCfg *model.Config
+	err = json.NewDecoder(f).Decode(&actualCfg)
 	require.Nil(t, err)
 
 	return actualCfg
@@ -89,9 +91,11 @@ func TestFileStoreNew(t *testing.T) {
 
 		fs, err := config.NewFileStore(path, false)
 		require.NoError(t, err)
-		defer fs.Close()
+		configStore, err := config.NewStoreFromBacking(fs)
+		require.NoError(t, err)
+		defer configStore.Close()
 
-		assert.Equal(t, "http://TestStoreNew", *fs.Get().ServiceSettings.SiteURL)
+		assert.Equal(t, "http://TestStoreNew", *configStore.Get().ServiceSettings.SiteURL)
 		assertFileNotEqualsConfig(t, testConfig, path)
 	})
 
@@ -101,9 +105,11 @@ func TestFileStoreNew(t *testing.T) {
 
 		fs, err := config.NewFileStore(path, false)
 		require.NoError(t, err)
-		defer fs.Close()
+		configStore, err := config.NewStoreFromBacking(fs)
+		require.NoError(t, err)
+		defer configStore.Close()
 
-		assert.Equal(t, "http://minimal", *fs.Get().ServiceSettings.SiteURL)
+		assert.Equal(t, "http://minimal", *configStore.Get().ServiceSettings.SiteURL)
 		assertFileEqualsConfig(t, minimalConfig, path)
 	})
 
@@ -118,9 +124,11 @@ func TestFileStoreNew(t *testing.T) {
 		path := filepath.Join(tempDir, "does_not_exist")
 		fs, err := config.NewFileStore(path, false)
 		require.NoError(t, err)
-		defer fs.Close()
+		configStore, err := config.NewStoreFromBacking(fs)
+		require.NoError(t, err)
+		defer configStore.Close()
 
-		assert.Equal(t, "", *fs.Get().ServiceSettings.SiteURL)
+		assert.Equal(t, "", *configStore.Get().ServiceSettings.SiteURL)
 		assertFileNotEqualsConfig(t, testConfig, path)
 	})
 
@@ -133,7 +141,10 @@ func TestFileStoreNew(t *testing.T) {
 		defer os.RemoveAll(tempDir)
 
 		path := filepath.Join(tempDir, "does/not/exist")
-		_, err = config.NewFileStore(path, false)
+		fs, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		configStore, err := config.NewStoreFromBacking(fs)
+		require.Nil(t, configStore)
 		require.Error(t, err)
 	})
 
@@ -154,9 +165,11 @@ func TestFileStoreNew(t *testing.T) {
 
 		fs, err := config.NewFileStore(path, false)
 		require.NoError(t, err)
-		defer fs.Close()
+		configStore, err := config.NewStoreFromBacking(fs)
+		require.NoError(t, err)
+		defer configStore.Close()
 
-		assert.Equal(t, "http://TestStoreNew", *fs.Get().ServiceSettings.SiteURL)
+		assert.Equal(t, "http://TestStoreNew", *configStore.Get().ServiceSettings.SiteURL)
 		assertFileNotEqualsConfig(t, testConfig, path)
 	})
 
@@ -171,9 +184,11 @@ func TestFileStoreNew(t *testing.T) {
 		path := "TestFileStoreNew/a/b/c/config.json"
 		fs, err := config.NewFileStore(path, false)
 		require.NoError(t, err)
-		defer fs.Close()
+		configStore, err := config.NewStoreFromBacking(fs)
+		require.NoError(t, err)
+		defer configStore.Close()
 
-		assert.Equal(t, "", *fs.Get().ServiceSettings.SiteURL)
+		assert.Equal(t, "", *configStore.Get().ServiceSettings.SiteURL)
 		assertFileNotEqualsConfig(t, testConfig, filepath.Join("config", path))
 	})
 }
@@ -184,21 +199,22 @@ func TestFileStoreGet(t *testing.T) {
 
 	fs, err := config.NewFileStore(path, false)
 	require.NoError(t, err)
-	defer fs.Close()
+	configStore, err := config.NewStoreFromBacking(fs)
+	require.NoError(t, err)
+	defer configStore.Close()
 
-	cfg := fs.Get()
+	cfg := configStore.Get()
 	assert.Equal(t, "http://TestStoreNew", *cfg.ServiceSettings.SiteURL)
 
-	cfg2 := fs.Get()
+	cfg2 := configStore.Get()
 	assert.Equal(t, "http://TestStoreNew", *cfg.ServiceSettings.SiteURL)
 
 	assert.True(t, cfg == cfg2, "Get() returned different configuration instances")
 
 	newCfg := &model.Config{}
-	oldCfg, err := fs.Set(newCfg)
+	_, err = configStore.Set(newCfg)
 	require.NoError(t, err)
 
-	assert.True(t, oldCfg == cfg, "returned config after set() changed original")
 	assert.False(t, newCfg == cfg, "returned config should have been different from original")
 }
 
@@ -207,7 +223,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		path, tearDown := setupConfigFile(t, testConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -217,7 +235,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override")
 		defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
 
-		fs, err = config.NewFileStore(path, false)
+		fsInner, err = config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err = config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -229,7 +249,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		path, tearDown := setupConfigFile(t, testConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -239,7 +261,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_PLUGINSETTINGS_ENABLEUPLOADS", "true")
 		defer os.Unsetenv("MM_PLUGINSETTINGS_ENABLEUPLOADS")
 
-		fs, err = config.NewFileStore(path, false)
+		fsInner, err = config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err = config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -251,7 +275,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		path, tearDown := setupConfigFile(t, testConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -261,7 +287,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM", "3000")
 		defer os.Unsetenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM")
 
-		fs, err = config.NewFileStore(path, false)
+		fsInner, err = config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err = config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -273,7 +301,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		path, tearDown := setupConfigFile(t, testConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -283,7 +313,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE", "123456")
 		defer os.Unsetenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE")
 
-		fs, err = config.NewFileStore(path, false)
+		fsInner, err = config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err = config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -295,7 +327,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		path, tearDown := setupConfigFile(t, testConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -305,7 +339,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db")
 		defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS")
 
-		fs, err = config.NewFileStore(path, false)
+		fsInner, err = config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err = config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -320,7 +356,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		path, tearDown := setupConfigFile(t, testConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -330,7 +368,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 		os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db user:pwd@db2:5433/test-db2 user:pwd@db3:5434/test-db3")
 		defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS")
 
-		fs, err = config.NewFileStore(path, false)
+		fsInner, err = config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err = config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -340,37 +380,23 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) {
 }
 
 func TestFileStoreSet(t *testing.T) {
-	t.Run("set same pointer value", func(t *testing.T) {
-		t.Skip("not yet implemented")
-
-		path, tearDown := setupConfigFile(t, emptyConfig)
-		defer tearDown()
-
-		fs, err := config.NewFileStore(path, false)
-		require.NoError(t, err)
-		defer fs.Close()
-
-		_, err = fs.Set(fs.Get())
-		if assert.Error(t, err) {
-			assert.EqualError(t, err, "old configuration modified instead of cloning")
-		}
-	})
-
 	t.Run("defaults required", func(t *testing.T) {
 		path, tearDown := setupConfigFile(t, minimalConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
-		oldCfg := fs.Get()
+		oldCfg := fs.Get().Clone()
 
 		newCfg := &model.Config{}
 
 		retCfg, err := fs.Set(newCfg)
 		require.NoError(t, err)
-		assert.Equal(t, oldCfg, retCfg)
+		require.Equal(t, oldCfg, retCfg)
 
 		assert.Equal(t, "", *fs.Get().ServiceSettings.SiteURL)
 	})
@@ -379,18 +405,17 @@ func TestFileStoreSet(t *testing.T) {
 		path, tearDown := setupConfigFile(t, ldapConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
-		oldCfg := fs.Get()
-
 		newCfg := &model.Config{}
 		newCfg.LdapSettings.BindPassword = sToP(model.FAKE_SETTING)
 
-		retCfg, err := fs.Set(newCfg)
+		_, err = fs.Set(newCfg)
 		require.NoError(t, err)
-		assert.Equal(t, oldCfg, retCfg)
 
 		assert.Equal(t, "password", *fs.Get().LdapSettings.BindPassword)
 	})
@@ -399,7 +424,9 @@ func TestFileStoreSet(t *testing.T) {
 		path, tearDown := setupConfigFile(t, emptyConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -418,13 +445,13 @@ func TestFileStoreSet(t *testing.T) {
 		path, tearDown := setupConfigFile(t, readOnlyConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
-		newCfg := &model.Config{}
-
-		_, err = fs.Set(newCfg)
+		_, err = fs.Set(readOnlyConfig)
 		if assert.Error(t, err) {
 			assert.Equal(t, config.ErrReadOnlyConfiguration, errors.Cause(err))
 		}
@@ -437,7 +464,9 @@ func TestFileStoreSet(t *testing.T) {
 		path, tearDown := setupConfigFile(t, emptyConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -457,12 +486,12 @@ func TestFileStoreSet(t *testing.T) {
 		path, tearDown := setupConfigFile(t, emptyConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
-		oldCfg := fs.Get()
-
 		called := make(chan bool, 1)
 		callback := func(oldfg, newCfg *model.Config) {
 			called <- true
@@ -471,9 +500,8 @@ func TestFileStoreSet(t *testing.T) {
 
 		newCfg := &model.Config{}
 
-		retCfg, err := fs.Set(newCfg)
+		_, err = fs.Set(newCfg)
 		require.NoError(t, err)
-		assert.Equal(t, oldCfg, retCfg)
 
 		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written")
 	})
@@ -486,7 +514,9 @@ func TestFileStoreSet(t *testing.T) {
 		path, tearDown := setupConfigFile(t, emptyConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, true)
+		fsInner, err := config.NewFileStore(path, true)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -516,7 +546,9 @@ func TestFileStoreLoad(t *testing.T) {
 		path, tearDown := setupConfigFile(t, emptyConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -531,7 +563,9 @@ func TestFileStoreLoad(t *testing.T) {
 		path, tearDown := setupConfigFile(t, minimalConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -553,7 +587,9 @@ func TestFileStoreLoad(t *testing.T) {
 		os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://overridePersistEnvVariables")
 		defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -576,7 +612,9 @@ func TestFileStoreLoad(t *testing.T) {
 		os.Setenv("MM_PLUGINSETTINGS_ENABLEUPLOADS", "true")
 		defer os.Unsetenv("MM_PLUGINSETTINGS_ENABLEUPLOADS")
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -599,7 +637,9 @@ func TestFileStoreLoad(t *testing.T) {
 		os.Setenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM", "3000")
 		defer os.Unsetenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM")
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -622,7 +662,9 @@ func TestFileStoreLoad(t *testing.T) {
 		os.Setenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE", "123456")
 		defer os.Unsetenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE")
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -645,7 +687,9 @@ func TestFileStoreLoad(t *testing.T) {
 		os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db")
 		defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS")
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -670,7 +714,9 @@ func TestFileStoreLoad(t *testing.T) {
 		os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db")
 		defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS")
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -690,7 +736,9 @@ func TestFileStoreLoad(t *testing.T) {
 		path, tearDown := setupConfigFile(t, emptyConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -709,7 +757,9 @@ func TestFileStoreLoad(t *testing.T) {
 		path, tearDown := setupConfigFile(t, fixesRequiredConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -726,7 +776,9 @@ func TestFileStoreLoad(t *testing.T) {
 		path, tearDown := setupConfigFile(t, emptyConfig)
 		defer tearDown()
 
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -754,7 +806,9 @@ func TestFileStoreWatcherEmitter(t *testing.T) {
 	defer tearDown()
 
 	t.Run("disabled", func(t *testing.T) {
-		fs, err := config.NewFileStore(path, false)
+		fsInner, err := config.NewFileStore(path, false)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -776,7 +830,9 @@ func TestFileStoreWatcherEmitter(t *testing.T) {
 	})
 
 	t.Run("enabled", func(t *testing.T) {
-		fs, err := config.NewFileStore(path, true)
+		fsInner, err := config.NewFileStore(path, true)
+		require.NoError(t, err)
+		fs, err := config.NewStoreFromBacking(fsInner)
 		require.NoError(t, err)
 		defer fs.Close()
 
@@ -799,7 +855,9 @@ func TestFileStoreSave(t *testing.T) {
 	path, tearDown := setupConfigFile(t, minimalConfig)
 	defer tearDown()
 
-	fs, err := config.NewFileStore(path, true)
+	fsInner, err := config.NewFileStore(path, false)
+	require.NoError(t, err)
+	fs, err := config.NewStoreFromBacking(fsInner)
 	require.NoError(t, err)
 	defer fs.Close()
 

config/memory.go

@@ -4,9 +4,7 @@
 package config
 
 import (
-	"bytes"
 	"fmt"
-	"io/ioutil"
 
 	"github.com/pkg/errors"
 
@@ -14,9 +12,8 @@ import (
 )
 
 // MemoryStore implements the Store interface. It is meant primarily for testing.
+// Not to be used directly. Only to be used as a backing store for config.Store
 type MemoryStore struct {
-	commonStore
-
 	allowEnvironmentOverrides bool
 	validate                  bool
 	files                     map[string][]byte
@@ -56,24 +53,12 @@ func NewMemoryStoreWithOptions(options *MemoryStoreOptions) (*MemoryStore, error
 		savedConfig:               savedConfig,
 	}
 
-	ms.commonStore.config = &model.Config{}
-	ms.commonStore.config.SetDefaults()
-
-	if err := ms.Load(); err != nil {
-		return nil, err
-	}
-
 	return ms, nil
 }
 
 // Set replaces the current configuration in its entirety.
-func (ms *MemoryStore) Set(newCfg *model.Config) (*model.Config, error) {
-	validate := ms.commonStore.validate
-	if !ms.validate {
-		validate = nil
-	}
-
-	return ms.commonStore.set(newCfg, ms.allowEnvironmentOverrides, validate, ms.persist)
+func (ms *MemoryStore) Set(newCfg *model.Config) error {
+	return ms.persist(newCfg)
 }
 
 // persist copies the active config to the saved config.
@@ -84,28 +69,18 @@ func (ms *MemoryStore) persist(cfg *model.Config) error {
 }
 
 // Load applies environment overrides to the default config as if a re-load had occurred.
-func (ms *MemoryStore) Load() (err error) {
-	var cfgBytes []byte
-	cfgBytes, err = marshalConfig(ms.savedConfig)
+func (ms *MemoryStore) Load() ([]byte, error) {
+	cfgBytes, err := marshalConfig(ms.savedConfig)
 	if err != nil {
-		return errors.Wrap(err, "failed to serialize config")
+		return nil, errors.Wrap(err, "failed to serialize config")
 	}
 
-	f := ioutil.NopCloser(bytes.NewReader(cfgBytes))
+	return cfgBytes, nil
 
-	validate := ms.commonStore.validate
-	if !ms.validate {
-		validate = nil
-	}
-
-	return ms.commonStore.load(f, false, validate, ms.persist)
 }
 
 // GetFile fetches the contents of a previously persisted configuration file.
 func (ms *MemoryStore) GetFile(name string) ([]byte, error) {
-	ms.configLock.RLock()
-	defer ms.configLock.RUnlock()
-
 	data, ok := ms.files[name]
 	if !ok {
 		return nil, fmt.Errorf("file %s not stored", name)
@@ -116,9 +91,6 @@ func (ms *MemoryStore) GetFile(name string) ([]byte, error) {
 
 // SetFile sets or replaces the contents of a configuration file.
 func (ms *MemoryStore) SetFile(name string, data []byte) error {
-	ms.configLock.Lock()
-	defer ms.configLock.Unlock()
-
 	ms.files[name] = data
 
 	return nil
@@ -126,18 +98,12 @@ func (ms *MemoryStore) SetFile(name string, data []byte) error {
 
 // HasFile returns true if the given file was previously persisted.
 func (ms *MemoryStore) HasFile(name string) (bool, error) {
-	ms.configLock.RLock()
-	defer ms.configLock.RUnlock()
-
 	_, ok := ms.files[name]
 	return ok, nil
 }
 
 // RemoveFile removes a previously persisted configuration file.
 func (ms *MemoryStore) RemoveFile(name string) error {
-	ms.configLock.Lock()
-	defer ms.configLock.Unlock()
-
 	delete(ms.files, name)
 
 	return nil
@@ -152,3 +118,8 @@ func (ms *MemoryStore) String() string {
 func (ms *MemoryStore) Close() error {
 	return nil
 }
+
+// Watch nothing on memory store
+func (ms *MemoryStore) Watch(_ func()) error {
+	return nil
+}

config/memory_test.go

@@ -6,13 +6,11 @@ package config_test
 import (
 	"os"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost-server/v5/config"
-	"github.com/mattermost/mattermost-server/v5/model"
 )
 
 func setupConfigMemory(t *testing.T) {
@@ -20,254 +18,6 @@ func setupConfigMemory(t *testing.T) {
 	os.Clearenv()
 }
 
-func TestMemoryStoreNew(t *testing.T) {
-	t.Run("no existing configuration - initialization required", func(t *testing.T) {
-		ms, err := config.NewMemoryStore()
-		require.NoError(t, err)
-		defer ms.Close()
-
-		assert.Equal(t, "", *ms.Get().ServiceSettings.SiteURL)
-	})
-
-	t.Run("existing config, initialization required", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: testConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		assert.Equal(t, "http://TestStoreNew", *ms.Get().ServiceSettings.SiteURL)
-	})
-
-	t.Run("already minimally configured", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: minimalConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		assert.Equal(t, "http://minimal", *ms.Get().ServiceSettings.SiteURL)
-	})
-
-	t.Run("invalid config, validation enabled", func(t *testing.T) {
-		_, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: invalidConfig})
-		require.Error(t, err)
-	})
-
-	t.Run("invalid config, validation disabled", func(t *testing.T) {
-		_, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: invalidConfig, SkipValidation: true})
-		require.NoError(t, err)
-	})
-}
-
-func TestMemoryStoreGet(t *testing.T) {
-	setupConfigMemory(t)
-
-	ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: testConfig})
-	require.NoError(t, err)
-	defer ms.Close()
-
-	cfg := ms.Get()
-	assert.Equal(t, "http://TestStoreNew", *cfg.ServiceSettings.SiteURL)
-
-	cfg2 := ms.Get()
-	assert.Equal(t, "http://TestStoreNew", *cfg.ServiceSettings.SiteURL)
-
-	assert.True(t, cfg == cfg2, "Get() returned different configuration instances")
-
-	newCfg := &model.Config{}
-	oldCfg, err := ms.Set(newCfg)
-	require.NoError(t, err)
-
-	assert.True(t, oldCfg == cfg, "returned config after set() changed original")
-	assert.False(t, newCfg == cfg, "returned config should have been different from original")
-}
-
-func TestMemoryStoreGetEnivironmentOverrides(t *testing.T) {
-	setupConfigMemory(t)
-
-	ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: testConfig})
-	require.NoError(t, err)
-	defer ms.Close()
-
-	assert.Equal(t, "http://TestStoreNew", *ms.Get().ServiceSettings.SiteURL)
-	assert.Empty(t, ms.GetEnvironmentOverrides())
-
-	os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override")
-	defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
-
-	ms, err = config.NewMemoryStore()
-	require.NoError(t, err)
-	defer ms.Close()
-
-	assert.Equal(t, "http://override", *ms.Get().ServiceSettings.SiteURL)
-	assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, ms.GetEnvironmentOverrides())
-}
-
-func TestMemoryStoreSet(t *testing.T) {
-	t.Run("set same pointer value", func(t *testing.T) {
-		t.Skip("not yet implemented")
-
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: emptyConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		_, err = ms.Set(ms.Get())
-		if assert.Error(t, err) {
-			assert.EqualError(t, err, "old configuration modified instead of cloning")
-		}
-	})
-
-	t.Run("defaults required", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: minimalConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		oldCfg := ms.Get()
-
-		newCfg := &model.Config{}
-
-		retCfg, err := ms.Set(newCfg)
-		require.NoError(t, err)
-		assert.Equal(t, oldCfg, retCfg)
-
-		assert.Equal(t, "", *ms.Get().ServiceSettings.SiteURL)
-	})
-
-	t.Run("desanitization required", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: ldapConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		oldCfg := ms.Get()
-
-		newCfg := &model.Config{}
-		newCfg.LdapSettings.BindPassword = sToP(model.FAKE_SETTING)
-
-		retCfg, err := ms.Set(newCfg)
-		require.NoError(t, err)
-		assert.Equal(t, oldCfg, retCfg)
-
-		assert.Equal(t, "password", *ms.Get().LdapSettings.BindPassword)
-	})
-
-	t.Run("invalid", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: emptyConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		newCfg := &model.Config{}
-		newCfg.ServiceSettings.SiteURL = sToP("invalid")
-
-		_, err = ms.Set(newCfg)
-		if assert.Error(t, err) {
-			assert.EqualError(t, err, "new configuration is invalid: Config.IsValid: model.config.is_valid.site_url.app_error, ")
-		}
-
-		assert.Equal(t, "", *ms.Get().ServiceSettings.SiteURL)
-	})
-
-	t.Run("read-only ignored", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: readOnlyConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		newCfg := &model.Config{
-			ServiceSettings: model.ServiceSettings{
-				SiteURL: sToP("http://new"),
-			},
-		}
-
-		_, err = ms.Set(newCfg)
-		require.NoError(t, err)
-
-		assert.Equal(t, "http://new", *ms.Get().ServiceSettings.SiteURL)
-	})
-
-	t.Run("listeners notified", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: emptyConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		oldCfg := ms.Get()
-
-		called := make(chan bool, 1)
-		callback := func(oldfg, newCfg *model.Config) {
-			called <- true
-		}
-		ms.AddListener(callback)
-
-		newCfg := &model.Config{}
-
-		retCfg, err := ms.Set(newCfg)
-		require.NoError(t, err)
-		assert.Equal(t, oldCfg, retCfg)
-
-		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written")
-	})
-}
-
-func TestMemoryStoreLoad(t *testing.T) {
-	t.Run("honour environment", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: minimalConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override")
-		defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
-
-		err = ms.Load()
-		require.NoError(t, err)
-		assert.Equal(t, "http://override", *ms.Get().ServiceSettings.SiteURL)
-		assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, ms.GetEnvironmentOverrides())
-	})
-
-	t.Run("fixes required", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: fixesRequiredConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		err = ms.Load()
-		require.NoError(t, err)
-		assert.Equal(t, "http://trailingslash", *ms.Get().ServiceSettings.SiteURL)
-	})
-
-	t.Run("listeners notifed", func(t *testing.T) {
-		setupConfigMemory(t)
-
-		ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: emptyConfig})
-		require.NoError(t, err)
-		defer ms.Close()
-
-		called := make(chan bool, 1)
-		callback := func(oldfg, newCfg *model.Config) {
-			called <- true
-		}
-		ms.AddListener(callback)
-
-		err = ms.Load()
-		require.NoError(t, err)
-
-		require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config loaded")
-	})
-}
-
 func TestMemoryGetFile(t *testing.T) {
 	setupConfigMemory(t)
 

config/migrate.go

@@ -46,7 +46,7 @@ func Migrate(from, to string) error {
 	return nil
 }
 
-func migrateFile(name string, source Store, destination Store) error {
+func migrateFile(name string, source *Store, destination *Store) error {
 	fileExists, err := source.HasFile(name)
 	if err != nil {
 		return errors.Wrapf(err, "failed to check existence of %s", name)

config/migrate_test.go

@@ -46,7 +46,7 @@ func TestMigrate(t *testing.T) {
 		truncateTables(t)
 	}
 
-	setupSource := func(t *testing.T, source config.Store) {
+	setupSource := func(t *testing.T, source *config.Store) {
 		t.Helper()
 
 		cfg := source.Get()
@@ -74,7 +74,7 @@ func TestMigrate(t *testing.T) {
 		}
 	}
 
-	assertDestination := func(t *testing.T, destination config.Store, source config.Store) {
+	assertDestination := func(t *testing.T, destination *config.Store, source *config.Store) {
 		t.Helper()
 
 		for i, file := range files {
@@ -100,7 +100,9 @@ func TestMigrate(t *testing.T) {
 		destinationDSN := path.Join(pwd, "config-custom.json")
 		sourceDSN := getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource)
 
-		source, err := config.NewDatabaseStore(sourceDSN)
+		sourcedb, err := config.NewDatabaseStore(sourceDSN)
+		require.NoError(t, err)
+		source, err := config.NewStoreFromBacking(sourcedb)
 		require.NoError(t, err)
 		defer source.Close()
 
@@ -108,7 +110,9 @@ func TestMigrate(t *testing.T) {
 		err = config.Migrate(sourceDSN, destinationDSN)
 		require.NoError(t, err)
 
-		destination, err := config.NewFileStore(destinationDSN, false)
+		destinationfile, err := config.NewFileStore(destinationDSN, false)
+		require.NoError(t, err)
+		destination, err := config.NewStoreFromBacking(destinationfile)
 		require.NoError(t, err)
 		defer destination.Close()
 
@@ -125,7 +129,9 @@ func TestMigrate(t *testing.T) {
 		sourceDSN := path.Join(pwd, "config-custom.json")
 		destinationDSN := getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource)
 
-		source, err := config.NewFileStore(sourceDSN, false)
+		sourcefile, err := config.NewFileStore(sourceDSN, false)
+		require.NoError(t, err)
+		source, err := config.NewStoreFromBacking(sourcefile)
 		require.NoError(t, err)
 		defer source.Close()
 
@@ -133,7 +139,9 @@ func TestMigrate(t *testing.T) {
 		err = config.Migrate(sourceDSN, destinationDSN)
 		require.NoError(t, err)
 
-		destination, err := config.NewDatabaseStore(destinationDSN)
+		destinationdb, err := config.NewDatabaseStore(destinationDSN)
+		require.NoError(t, err)
+		destination, err := config.NewStoreFromBacking(destinationdb)
 		require.NoError(t, err)
 		defer destination.Close()
 

config/split_logger.go

@@ -0,0 +1,35 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package config
+
+import (
+	"fmt"
+
+	"github.com/mattermost/mattermost-server/v5/mlog"
+)
+
+type splitLogger struct {
+	wrappedLog *mlog.Logger
+}
+
+func (s *splitLogger) Error(msg ...interface{}) {
+	s.wrappedLog.Error(fmt.Sprint(msg...))
+}
+
+func (s *splitLogger) Warning(msg ...interface{}) {
+	s.wrappedLog.Warn(fmt.Sprint(msg...))
+}
+
+// Ignoring more verbose messages from split
+func (s *splitLogger) Info(msg ...interface{}) {
+	//s.wrappedLog.Info(fmt.Sprint(msg...))
+}
+
+func (s *splitLogger) Debug(msg ...interface{}) {
+	//s.wrappedLog.Debug(fmt.Sprint(msg...))
+}
+
+func (s *splitLogger) Verbose(msg ...interface{}) {
+	//s.wrappedLog.Info(fmt.Sprint(msg...))
+}

config/store.go

@@ -4,37 +4,26 @@
 package config
 
 import (
+	"bytes"
+	"encoding/json"
 	"strings"
+	"sync"
 
 	"github.com/mattermost/mattermost-server/v5/model"
+	"github.com/mattermost/mattermost-server/v5/utils/jsonutils"
+	"github.com/pkg/errors"
 )
 
 // Listener is a callback function invoked when the configuration changes.
 type Listener func(oldConfig *model.Config, newConfig *model.Config)
 
-// Store abstracts the act of getting and setting the configuration.
-type Store interface {
-	// Get fetches the current, cached configuration.
-	Get() *model.Config
-
-	// GetEnvironmentOverrides fetches the configuration fields overridden by environment variables.
-	GetEnvironmentOverrides() map[string]interface{}
-
-	// RemoveEnvironmentOverrides returns a new config without the environment
-	// overrides
-	RemoveEnvironmentOverrides(cfg *model.Config) *model.Config
-
+type BackingStore interface {
 	// Set replaces the current configuration in its entirety and updates the backing store.
-	Set(*model.Config) (*model.Config, error)
+	Set(*model.Config) error
 
-	// Load updates the current configuration from the backing store, possibly initializing.
-	Load() (err error)
-
-	// AddListener adds a callback function to invoke when the configuration is modified.
-	AddListener(listener Listener) string
-
-	// RemoveListener removes a callback function using an id returned from AddListener.
-	RemoveListener(id string)
+	// Load retrieves the configuration stored. If there is no configuration stored
+	// the io.ReadCloser will be nil
+	Load() ([]byte, error)
 
 	// GetFile fetches the contents of a previously persisted configuration file.
 	// If no such file exists, an empty byte array will be returned without error.
@@ -52,15 +41,247 @@ type Store interface {
 	// String describes the backing store for the config.
 	String() string
 
+	Watch(callback func()) error
+
 	// Close cleans up resources associated with the store.
 	Close() error
 }
 
 // NewStore creates a database or file store given a data source name by which to connect.
-func NewStore(dsn string, watch bool) (Store, error) {
+func NewStore(dsn string, watch bool) (*Store, error) {
+	backingStore, err := getBackingStore(dsn, watch)
+	if err != nil {
+		return nil, err
+	}
+
+	return NewStoreFromBacking(backingStore)
+
+}
+
+func NewStoreFromBacking(backingStore BackingStore) (*Store, error) {
+	store := &Store{
+		backingStore: backingStore,
+	}
+
+	if err := store.Load(); err != nil {
+		return nil, errors.Wrap(err, "unable to load on store creation")
+	}
+
+	if err := backingStore.Watch(func() {
+		store.Load()
+	}); err != nil {
+		return nil, errors.Wrap(err, "failed to watch backing store")
+	}
+
+	return store, nil
+}
+
+func getBackingStore(dsn string, watch bool) (BackingStore, error) {
 	if strings.HasPrefix(dsn, "mysql://") || strings.HasPrefix(dsn, "postgres://") {
 		return NewDatabaseStore(dsn)
 	}
 
 	return NewFileStore(dsn, watch)
 }
+
+func NewTestMemoryStore() *Store {
+	memoryStore, err := NewMemoryStore()
+	if err != nil {
+		panic("failed to initialize memory store: " + err.Error())
+	}
+
+	configStore, err := NewStoreFromBacking(memoryStore)
+	if err != nil {
+		panic("failed to initialize config store: " + err.Error())
+	}
+
+	return configStore
+}
+
+type Store struct {
+	emitter
+	backingStore BackingStore
+
+	configLock  sync.RWMutex
+	config      *model.Config
+	configNoEnv *model.Config
+
+	persistFeatureFlags bool
+}
+
+// Get fetches the current, cached configuration.
+func (s *Store) Get() *model.Config {
+	s.configLock.RLock()
+	defer s.configLock.RUnlock()
+	return s.config
+}
+
+// Get fetches the current, cached configuration without environment variable overrides.
+func (s *Store) GetNoEnv() *model.Config {
+	s.configLock.RLock()
+	defer s.configLock.RUnlock()
+	return s.configNoEnv
+}
+
+// GetEnvironmentOverrides fetches the configuration fields overridden by environment variables.
+func (s *Store) GetEnvironmentOverrides() map[string]interface{} {
+	return generateEnvironmentMap(GetEnvironment())
+}
+
+// RemoveEnvironmentOverrides returns a new config without the environment
+// 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) {
+	s.configLock.Lock()
+	defer s.configLock.Unlock()
+	s.persistFeatureFlags = persist
+}
+
+// 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)
+
+	oldCfg := s.config.Clone()
+
+	// Really just for some tests we need to set defaults here
+	newCfg.SetDefaults()
+
+	// Sometimes the config is received with "fake" data in sensitive fields. Apply the real
+	// data from the existing config as necessary.
+	desanitize(oldCfg, newCfg)
+
+	if err := newCfg.IsValid(); err != nil {
+		return nil, errors.Wrap(err, "new configuration is invalid")
+	}
+
+	newCfg = removeEnvOverrides(newCfg, s.configNoEnv, s.GetEnvironmentOverrides())
+
+	// 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 managment system is down when a pod starts.
+	if !s.persistFeatureFlags {
+		newCfg.FeatureFlags = nil
+	}
+
+	if err := s.backingStore.Set(newCfg); 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")
+	}
+
+	return oldCfg, nil
+}
+
+func (s *Store) loadLockedWithOld(oldCfg *model.Config, unlockOnce *sync.Once) error {
+	configBytes, err := s.backingStore.Load()
+	if err != nil {
+		return err
+	}
+
+	loadedConfig := &model.Config{}
+	if len(configBytes) != 0 {
+		if err = json.Unmarshal(configBytes, &loadedConfig); err != nil {
+			return jsonutils.HumanizeJsonError(err, configBytes)
+		}
+	}
+
+	loadedConfig.SetDefaults()
+
+	s.configNoEnv = loadedConfig.Clone()
+	fixConfig(s.configNoEnv)
+
+	loadedConfig = applyEnvironmentMap(loadedConfig, GetEnvironment())
+
+	fixConfig(loadedConfig)
+
+	if err := loadedConfig.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)
+	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")
+	}
+	if len(configBytes) == 0 || !bytes.Equal(oldCfgBytes, newCfgBytes) {
+		if err := s.backingStore.Set(s.configNoEnv); err != nil {
+			if !errors.Is(err, ErrReadOnlyConfiguration) {
+				return errors.Wrap(err, "failed to persist")
+			}
+		}
+	}
+
+	s.config = loadedConfig
+
+	unlockOnce.Do(s.configLock.Unlock)
+
+	s.invokeConfigListeners(oldCfg, loadedConfig)
+
+	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) {
+	s.configLock.RLock()
+	defer s.configLock.RUnlock()
+	return s.backingStore.GetFile(name)
+}
+
+// SetFile sets or replaces the contents of a configuration file.
+func (s *Store) SetFile(name string, data []byte) error {
+	s.configLock.Lock()
+	defer s.configLock.Unlock()
+	return s.backingStore.SetFile(name, data)
+}
+
+// HasFile returns true if the given file was previously persisted.
+func (s *Store) HasFile(name string) (bool, error) {
+	s.configLock.RLock()
+	defer s.configLock.RUnlock()
+	return s.backingStore.HasFile(name)
+}
+
+// RemoveFile removes a previously persisted configuration file.
+func (s *Store) RemoveFile(name string) error {
+	s.configLock.Lock()
+	defer s.configLock.Unlock()
+	return s.backingStore.RemoveFile(name)
+}
+
+// String describes the backing store for the config.
+func (s *Store) String() string {
+	return s.backingStore.String()
+}
+
+// Close cleans up resources associated with the store.
+func (s *Store) Close() error {
+	s.configLock.Lock()
+	defer s.configLock.Unlock()
+	return s.backingStore.Close()
+}

config/unmarshal.go

@@ -4,129 +4,12 @@
 package config
 
 import (
-	"bytes"
 	"encoding/json"
-	"io"
-	"io/ioutil"
-	"reflect"
-	"strings"
 
-	"github.com/mattermost/viper"
-	"github.com/pkg/errors"
-
-	"github.com/mattermost/mattermost-server/v5/mlog"
 	"github.com/mattermost/mattermost-server/v5/model"
-	"github.com/mattermost/mattermost-server/v5/utils/jsonutils"
 )
 
-// newViper creates an instance of viper.Viper configured for parsing a configuration.
-func newViper(allowEnvironmentOverrides bool) *viper.Viper {
-	v := viper.New()
-
-	v.SetConfigType("json")
-
-	v.AllowEmptyEnv(true)
-
-	if allowEnvironmentOverrides {
-		v.SetEnvPrefix("mm")
-		v.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
-		v.AutomaticEnv()
-	}
-
-	return v
-}
-
 // marshalConfig converts the given configuration into JSON bytes for persistence.
 func marshalConfig(cfg *model.Config) ([]byte, error) {
 	return json.MarshalIndent(cfg, "", "    ")
 }
-
-// unmarshalConfig unmarshals a raw configuration into a Config model and environment variable overrides.
-func unmarshalConfig(r io.Reader, allowEnvironmentOverrides bool) (*model.Config, map[string]interface{}, error) {
-	// Pre-flight check the syntax of the configuration file to improve error messaging.
-	configData, err := ioutil.ReadAll(r)
-	if err != nil {
-		return nil, nil, errors.Wrapf(err, "failed to read")
-	}
-
-	var rawConfig model.Config
-	if err = json.Unmarshal(configData, &rawConfig); err != nil {
-		return nil, nil, jsonutils.HumanizeJsonError(err, configData)
-	}
-	rawConfig.SetDefaults()
-	dataWithDefaults, err := json.Marshal(rawConfig)
-	if err != nil {
-		return nil, nil, errors.Wrap(err, "failed to re-marshal config")
-	}
-
-	v := newViper(allowEnvironmentOverrides)
-	if err := v.ReadConfig(bytes.NewReader(dataWithDefaults)); err != nil {
-		return nil, nil, err
-	}
-
-	var config model.Config
-	unmarshalErr := v.Unmarshal(&config)
-	// https://github.com/spf13/viper/issues/324
-	// https://github.com/spf13/viper/issues/348
-	if unmarshalErr == nil {
-		config.PluginSettings.Plugins = make(map[string]map[string]interface{})
-		unmarshalErr = v.UnmarshalKey("pluginsettings.plugins", &config.PluginSettings.Plugins)
-	}
-	if unmarshalErr == nil {
-		config.PluginSettings.PluginStates = make(map[string]*model.PluginState)
-		unmarshalErr = v.UnmarshalKey("pluginsettings.pluginstates", &config.PluginSettings.PluginStates)
-	}
-
-	envConfig := v.EnvSettings()
-
-	var envErr error
-	if envConfig, envErr = fixEnvSettingsCase(envConfig); envErr != nil {
-		return nil, nil, envErr
-	}
-
-	return &config, envConfig, unmarshalErr
-}
-
-// Fixes the case of the environment variables sent back from Viper since Viper stores everything
-// as lower case.
-func fixEnvSettingsCase(in map[string]interface{}) (out map[string]interface{}, err error) {
-	defer func() {
-		if r := recover(); r != nil {
-			mlog.Error("Panicked in fixEnvSettingsCase. This should never happen.", mlog.Any("err", r))
-			out = in
-		}
-	}()
-
-	var fixCase func(map[string]interface{}, reflect.Type) map[string]interface{}
-	fixCase = func(in map[string]interface{}, t reflect.Type) map[string]interface{} {
-		if t.Kind() == reflect.Ptr {
-			t = t.Elem()
-		}
-
-		if t.Kind() != reflect.Struct {
-			// Should never hit this, but this will prevent a panic if that does happen somehow
-			return nil
-		}
-
-		fixCaseOut := make(map[string]interface{}, len(in))
-
-		for i := 0; i < t.NumField(); i++ {
-			field := t.Field(i)
-
-			key := field.Name
-			if value, ok := in[strings.ToLower(key)]; ok {
-				if valueAsMap, ok := value.(map[string]interface{}); ok {
-					fixCaseOut[key] = fixCase(valueAsMap, field.Type)
-				} else {
-					fixCaseOut[key] = value
-				}
-			}
-		}
-
-		return fixCaseOut
-	}
-
-	out = fixCase(in, reflect.TypeOf(model.Config{}))
-
-	return
-}

config/unmarshal_test.go

@@ -1,354 +0,0 @@
-// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
-// See LICENSE.txt for license information.
-
-package config
-
-import (
-	"bytes"
-	"os"
-	"strings"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-
-	"github.com/mattermost/mattermost-server/v5/model"
-	"github.com/mattermost/mattermost-server/v5/utils"
-)
-
-func TestUnmarshalConfig(t *testing.T) {
-	_, _, err := unmarshalConfig(bytes.NewReader([]byte(``)), false)
-	require.EqualError(t, err, "parsing error at line 1, character 1: unexpected end of JSON input")
-
-	_, _, err = unmarshalConfig(bytes.NewReader([]byte(`
-		{
-			malformed
-	`)), false)
-	require.EqualError(t, err, "parsing error at line 3, character 5: invalid character 'm' looking for beginning of object key string")
-}
-
-func TestUnmarshalConfig_PluginSettings(t *testing.T) {
-	config, _, err := unmarshalConfig(bytes.NewReader([]byte(`{
-		"PluginSettings": {
-			"Directory": "/temp/mattermost-plugins",
-			"Plugins": {
-				"com.example.plugin": {
-					"number": 1,
-					"string": "abc",
-					"boolean": false,
-					"abc.def.ghi": {
-						"abc": 123,
-						"def": "456"
-					}
-				},
-				"jira": {
-					"number": 2,
-					"string": "123",
-					"boolean": true,
-					"abc.def.ghi": {
-						"abc": 456,
-						"def": "123"
-					}
- 				}
-			},
-			"PluginStates": {
-				"com.example.plugin": {
-					"enable": true
-				},
-				"jira": {
-					"enable": false
- 				}
-			}
-		}
-	}`)), false)
-	require.Nil(t, err)
-
-	assert.Equal(t, "/temp/mattermost-plugins", *config.PluginSettings.Directory)
-
-	if assert.Contains(t, config.PluginSettings.Plugins, "com.example.plugin") {
-		assert.Equal(t, map[string]interface{}{
-			"number":  float64(1),
-			"string":  "abc",
-			"boolean": false,
-			"abc.def.ghi": map[string]interface{}{
-				"abc": float64(123),
-				"def": "456",
-			},
-		}, config.PluginSettings.Plugins["com.example.plugin"])
-	}
-	if assert.Contains(t, config.PluginSettings.PluginStates, "com.example.plugin") {
-		assert.Equal(t, model.PluginState{
-			Enable: true,
-		}, *config.PluginSettings.PluginStates["com.example.plugin"])
-	}
-
-	if assert.Contains(t, config.PluginSettings.Plugins, "jira") {
-		assert.Equal(t, map[string]interface{}{
-			"number":  float64(2),
-			"string":  "123",
-			"boolean": true,
-			"abc.def.ghi": map[string]interface{}{
-				"abc": float64(456),
-				"def": "123",
-			},
-		}, config.PluginSettings.Plugins["jira"])
-	}
-	if assert.Contains(t, config.PluginSettings.PluginStates, "jira") {
-		assert.Equal(t, model.PluginState{
-			Enable: false,
-		}, *config.PluginSettings.PluginStates["jira"])
-	}
-}
-
-func TestConfigFromEnviroVars(t *testing.T) {
-	config := `{
-		"ServiceSettings": {
-			"EnableCommands": true,
-			"ReadTimeout": 100
-		},
-		"TeamSettings": {
-			"SiteName": "Mattermost",
-			"CustomBrandText": ""
-		},
-		"SupportSettings": {
-			"TermsOfServiceLink": "https://about.mattermost.com/default-terms/"
-		},
-		"PluginSettings": {
-			"Enable": true,
-			"Plugins": {
-				"jira": {
-					"enabled": "true",
-					"secret": "config-secret"
-				}
-			},
-			"PluginStates": {
-				"jira": {
-					"Enable": true
-				}
-			}
-		}
-	}`
-
-	t.Run("string settings", func(t *testing.T) {
-		os.Setenv("MM_TEAMSETTINGS_SITENAME", "From Environment")
-		os.Setenv("MM_TEAMSETTINGS_CUSTOMBRANDTEXT", "Custom Brand")
-
-		cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true)
-		require.Nil(t, err)
-
-		assert.Equal(t, "From Environment", *cfg.TeamSettings.SiteName)
-		assert.Equal(t, "Custom Brand", *cfg.TeamSettings.CustomBrandText)
-
-		teamSettings, ok := envCfg["TeamSettings"]
-		require.True(t, ok, "TeamSettings is missing from envConfig")
-
-		teamSettingsAsMap, ok := teamSettings.(map[string]interface{})
-		require.True(t, ok, "TeamSettings is not a map in envConfig")
-
-		siteNameInEnv, ok := teamSettingsAsMap["SiteName"].(bool)
-		require.True(t, ok || siteNameInEnv, "SiteName should be in envConfig")
-
-		customBrandTextInEnv, ok := teamSettingsAsMap["CustomBrandText"].(bool)
-		require.True(t, ok || customBrandTextInEnv, "SiteName should be in envConfig")
-
-		os.Unsetenv("MM_TEAMSETTINGS_SITENAME")
-		os.Unsetenv("MM_TEAMSETTINGS_CUSTOMBRANDTEXT")
-
-		cfg, envCfg, err = unmarshalConfig(strings.NewReader(config), true)
-		require.Nil(t, err)
-
-		assert.Equal(t, "Mattermost", *cfg.TeamSettings.SiteName)
-
-		_, ok = envCfg["TeamSettings"]
-		require.False(t, ok, "TeamSettings should be missing from envConfig")
-	})
-
-	t.Run("boolean setting", func(t *testing.T) {
-		os.Setenv("MM_SERVICESETTINGS_ENABLECOMMANDS", "false")
-		defer os.Unsetenv("MM_SERVICESETTINGS_ENABLECOMMANDS")
-
-		cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true)
-		require.Nil(t, err)
-
-		require.False(t, *cfg.ServiceSettings.EnableCommands, "Couldn't read config from environment var")
-
-		serviceSettings, ok := envCfg["ServiceSettings"]
-		require.True(t, ok, "ServiceSettings is missing from envConfig")
-
-		serviceSettingsAsMap, ok := serviceSettings.(map[string]interface{})
-		require.True(t, ok, "ServiceSettings is not a map in envConfig")
-
-		enableCommandsInEnv, ok := serviceSettingsAsMap["EnableCommands"].(bool)
-		require.True(t, ok || enableCommandsInEnv, "EnableCommands should be in envConfig")
-	})
-
-	t.Run("integer setting", func(t *testing.T) {
-		os.Setenv("MM_SERVICESETTINGS_READTIMEOUT", "400")
-		defer os.Unsetenv("MM_SERVICESETTINGS_READTIMEOUT")
-
-		cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true)
-		require.Nil(t, err)
-
-		assert.Equal(t, 400, *cfg.ServiceSettings.ReadTimeout)
-
-		serviceSettings, ok := envCfg["ServiceSettings"]
-		require.True(t, ok, "ServiceSettings is missing from envConfig")
-
-		serviceSettingsAsMap, ok := serviceSettings.(map[string]interface{})
-		require.True(t, ok, "ServiceSettings is not a map in envConfig")
-
-		readTimeoutInEnv, ok := serviceSettingsAsMap["ReadTimeout"].(bool)
-		require.True(t, ok || readTimeoutInEnv, "ReadTimeout should be in envConfig")
-	})
-
-	t.Run("setting missing from config.json", func(t *testing.T) {
-		os.Setenv("MM_SERVICESETTINGS_SITEURL", "https://example.com")
-		defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL")
-
-		cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true)
-		require.Nil(t, err)
-
-		assert.Equal(t, "https://example.com", *cfg.ServiceSettings.SiteURL)
-
-		serviceSettings, ok := envCfg["ServiceSettings"]
-		require.True(t, ok, "ServiceSettings is missing from envConfig")
-
-		serviceSettingsAsMap, ok := serviceSettings.(map[string]interface{})
-		require.True(t, ok, "ServiceSettings is not a map in envConfig")
-
-		siteURLInEnv, ok := serviceSettingsAsMap["SiteURL"].(bool)
-		require.True(t, ok || siteURLInEnv, "SiteURL should be in envConfig")
-	})
-
-	t.Run("empty string setting", func(t *testing.T) {
-		os.Setenv("MM_SUPPORTSETTINGS_TERMSOFSERVICELINK", "")
-		defer os.Unsetenv("MM_SUPPORTSETTINGS_TERMSOFSERVICELINK")
-
-		cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true)
-		require.Nil(t, err)
-
-		assert.Empty(t, *cfg.SupportSettings.TermsOfServiceLink)
-
-		supportSettings, ok := envCfg["SupportSettings"]
-		require.True(t, ok, "SupportSettings is missing from envConfig")
-
-		supportSettingsAsMap, ok := supportSettings.(map[string]interface{})
-		require.True(t, ok, "SupportSettings is not a map in envConfig")
-
-		termsOfServiceLinkInEnv, ok := supportSettingsAsMap["TermsOfServiceLink"].(bool)
-		require.True(t, ok || termsOfServiceLinkInEnv, "TermsOfServiceLink should be in envConfig")
-	})
-
-	t.Run("feature flag", func(t *testing.T) {
-		os.Setenv("MM_FEATUREFLAGS_TESTFEATURE", "value")
-		defer os.Unsetenv("MM_FEATUREFLAGS_TESTFEATURE")
-
-		cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true)
-		require.Nil(t, err)
-		require.NotNil(t, cfg.FeatureFlags)
-
-		assert.Equal(t, "value", cfg.FeatureFlags.TestFeature)
-
-		featureFlags, ok := envCfg["FeatureFlags"]
-		require.True(t, ok, "FeatureFlags is missing from envConfig")
-
-		featureFlagsAsMap, ok := featureFlags.(map[string]interface{})
-		require.True(t, ok, "FeatureFlags is not a map in envConfig")
-
-		testFeatureInEnv, ok := featureFlagsAsMap["TestFeature"].(bool)
-		require.True(t, ok || testFeatureInEnv, "TestFeature should be in envConfig")
-	})
-
-	t.Run("plugin directory settings", func(t *testing.T) {
-		os.Setenv("MM_PLUGINSETTINGS_ENABLE", "false")
-		os.Setenv("MM_PLUGINSETTINGS_DIRECTORY", "/temp/plugins")
-		os.Setenv("MM_PLUGINSETTINGS_CLIENTDIRECTORY", "/temp/clientplugins")
-		defer os.Unsetenv("MM_PLUGINSETTINGS_ENABLE")
-		defer os.Unsetenv("MM_PLUGINSETTINGS_DIRECTORY")
-		defer os.Unsetenv("MM_PLUGINSETTINGS_CLIENTDIRECTORY")
-
-		cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true)
-		require.Nil(t, err)
-
-		assert.Equal(t, false, *cfg.PluginSettings.Enable)
-		assert.Equal(t, "/temp/plugins", *cfg.PluginSettings.Directory)
-		assert.Equal(t, "/temp/clientplugins", *cfg.PluginSettings.ClientDirectory)
-
-		pluginSettings, ok := envCfg["PluginSettings"]
-		require.True(t, ok, "PluginSettings is missing from envConfig")
-
-		pluginSettingsAsMap, ok := pluginSettings.(map[string]interface{})
-		require.True(t, ok, "PluginSettings is not a map in envConfig")
-
-		directory, ok := pluginSettingsAsMap["Directory"].(bool)
-		require.True(t, ok || directory, "Directory should be in envConfig")
-
-		clientDirectory, ok := pluginSettingsAsMap["ClientDirectory"].(bool)
-		require.True(t, ok || clientDirectory, "ClientDirectory should be in envConfig")
-	})
-
-	t.Run("plugin specific settings cannot be overridden via environment", func(t *testing.T) {
-		os.Setenv("MM_PLUGINSETTINGS_PLUGINS_JIRA_ENABLED", "false")
-		os.Setenv("MM_PLUGINSETTINGS_PLUGINS_JIRA_SECRET", "env-secret")
-		os.Setenv("MM_PLUGINSETTINGS_PLUGINSTATES_JIRA_ENABLE", "false")
-		defer os.Unsetenv("MM_PLUGINSETTINGS_PLUGINS_JIRA_ENABLED")
-		defer os.Unsetenv("MM_PLUGINSETTINGS_PLUGINS_JIRA_SECRET")
-		defer os.Unsetenv("MM_PLUGINSETTINGS_PLUGINSTATES_JIRA_ENABLE")
-
-		cfg, envCfg, err := unmarshalConfig(strings.NewReader(config), true)
-		require.Nil(t, err)
-
-		pluginsJira, ok := cfg.PluginSettings.Plugins["jira"]
-		require.True(t, ok, "PluginSettings.Plugins.jira is missing from config")
-
-		enabled, ok := pluginsJira["enabled"]
-		require.True(t, ok, "PluginSettings.Plugins.jira.enabled is missing from config")
-		assert.Equal(t, "true", enabled)
-
-		secret, ok := pluginsJira["secret"]
-		require.True(t, ok, "PluginSettings.Plugins.jira.secret is missing from config")
-		assert.Equal(t, "config-secret", secret)
-
-		pluginStatesJira, ok := cfg.PluginSettings.PluginStates["jira"]
-		require.True(t, ok, "PluginSettings.PluginStates.jira is missing from config")
-		require.Equal(t, true, pluginStatesJira.Enable)
-
-		pluginSettings, ok := envCfg["PluginSettings"]
-		require.True(t, ok, "PluginSettings is missing from envConfig")
-
-		pluginSettingsAsMap, ok := pluginSettings.(map[string]interface{})
-		require.True(t, ok, "PluginSettings is not a map in envConfig")
-
-		plugins, ok := pluginSettingsAsMap["Plugins"].(map[string]interface{})
-		require.True(t, ok, "PluginSettings.Plugins is not a map in envConfig")
-
-		_, ok = plugins["jira"].(map[string]interface{})
-		require.False(t, ok, "PluginSettings.Plugins.jira should not be a map in envConfig")
-
-		pluginStates, ok := pluginSettingsAsMap["PluginStates"].(map[string]interface{})
-		require.True(t, ok, "PluginSettings.PluginStates is missing from envConfig")
-
-		_, ok = pluginStates["jira"].(map[string]interface{})
-		require.False(t, ok, "PluginSettings.PluginStates.jira should not be a map in envConfig")
-	})
-}
-
-func TestReadConfig_ImageProxySettings(t *testing.T) {
-	utils.TranslationsPreInit()
-
-	t.Run("deprecated settings should still be read properly", func(t *testing.T) {
-		config, _, err := unmarshalConfig(bytes.NewReader([]byte(`{
-			"ServiceSettings": {
-				"ImageProxyType": "OldImageProxyType",
-				"ImageProxyURL": "OldImageProxyURL",
-				"ImageProxyOptions": "OldImageProxyOptions"
-			}
-		}`)), false)
-
-		require.Nil(t, err)
-
-		assert.Equal(t, model.NewString("OldImageProxyType"), config.ServiceSettings.DEPRECATED_DO_NOT_USE_ImageProxyType)
-		assert.Equal(t, model.NewString("OldImageProxyURL"), config.ServiceSettings.DEPRECATED_DO_NOT_USE_ImageProxyURL)
-		assert.Equal(t, model.NewString("OldImageProxyOptions"), config.ServiceSettings.DEPRECATED_DO_NOT_USE_ImageProxyOptions)
-	})
-}

config/utils.go

@@ -76,6 +76,10 @@ func desanitize(actual, target *model.Config) {
 	if target.ServiceSettings.GfycatApiSecret != nil && *target.ServiceSettings.GfycatApiSecret == model.FAKE_SETTING {
 		*target.ServiceSettings.GfycatApiSecret = *actual.ServiceSettings.GfycatApiSecret
 	}
+
+	if *target.ServiceSettings.SplitKey == model.FAKE_SETTING {
+		*target.ServiceSettings.SplitKey = *actual.ServiceSettings.SplitKey
+	}
 }
 
 // fixConfig patches invalid or missing data in the configuration, returning true if changed.

config/utils_test.go

@@ -71,6 +71,7 @@ func TestDesanitize(t *testing.T) {
 	assert.Equal(t, *actual.ElasticsearchSettings.Password, *target.ElasticsearchSettings.Password)
 	assert.Equal(t, actual.SqlSettings.DataSourceReplicas, target.SqlSettings.DataSourceReplicas)
 	assert.Equal(t, actual.SqlSettings.DataSourceSearchReplicas, target.SqlSettings.DataSourceSearchReplicas)
+	assert.Equal(t, actual.ServiceSettings.SplitKey, target.ServiceSettings.SplitKey)
 }
 
 func TestFixInvalidLocales(t *testing.T) {