diff --git a/config/common.go b/config/common.go index 4e0e89b7d3..45d6cf767d 100644 --- a/config/common.go +++ b/config/common.go @@ -4,6 +4,7 @@ package config import ( + "bytes" "io" "sync" @@ -15,9 +16,10 @@ import ( type commonStore struct { emitter - configLock sync.RWMutex - config *model.Config - environmentOverrides map[string]interface{} + configLock sync.RWMutex + config *model.Config + configWithoutOverrides *model.Config + environmentOverrides map[string]interface{} } // Get fetches the current, cached configuration. @@ -67,7 +69,7 @@ func (cs *commonStore) set(newCfg *model.Config, validate func(*model.Config) er } } - if err := persist(newCfg); err != nil { + if err := persist(cs.removeEnvOverrides(newCfg)); err != nil { return nil, errors.Wrap(err, "failed to persist") } @@ -86,10 +88,20 @@ func (cs *commonStore) set(newCfg *model.Config, validate func(*model.Config) er // // 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(f, allowEnvironmentOverrides) + loadedCfg, environmentOverrides, err := unmarshalConfig(tee, allowEnvironmentOverrides) if err != nil { - return errors.Wrapf(err, "failed to unmarshal config") + 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 @@ -114,13 +126,15 @@ func (cs *commonStore) load(f io.ReadCloser, needsSave bool, validate func(*mode defer unlockOnce.Do(cs.configLock.Unlock) if needsSave && persist != nil { - if err = persist(loadedCfg); err != 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) @@ -140,3 +154,8 @@ func (cs *commonStore) validate(cfg *model.Config) error { return nil } + +// removeEnvOverrides returns a new config without the given environment overrides. +func (cs *commonStore) removeEnvOverrides(cfg *model.Config) *model.Config { + return removeEnvOverrides(cfg, cs.configWithoutOverrides, cs.environmentOverrides) +} diff --git a/config/database_test.go b/config/database_test.go index 1e3d89944f..0f090e0af1 100644 --- a/config/database_test.go +++ b/config/database_test.go @@ -407,6 +407,8 @@ func TestDatabaseStoreLoad(t *testing.T) { require.NoError(t, err) defer ds.Close() + assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL) + os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") err = ds.Load() @@ -415,6 +417,26 @@ func TestDatabaseStoreLoad(t *testing.T) { assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, ds.GetEnvironmentOverrides()) }) + t.Run("do not persist environment variables", func(t *testing.T) { + _, tearDown := setupConfigDatabase(t, minimalConfig, nil) + defer tearDown() + + os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://overridePersistEnvVariables") + + ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *sqlSettings.DriverName, *sqlSettings.DataSource)) + require.NoError(t, err) + defer ds.Close() + + _, err = ds.Set(ds.Get()) + require.NoError(t, err) + + assert.Equal(t, "http://overridePersistEnvVariables", *ds.Get().ServiceSettings.SiteURL) + assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, ds.GetEnvironmentOverrides()) + // check that in DB config does not include overwritten variable + _, actualConfig := getActualDatabaseConfig(t) + assert.Equal(t, "http://minimal", *actualConfig.ServiceSettings.SiteURL) + }) + t.Run("invalid", func(t *testing.T) { _, tearDown := setupConfigDatabase(t, emptyConfig, nil) defer tearDown() diff --git a/config/environment.go b/config/environment.go new file mode 100644 index 0000000000..9b0df10bda --- /dev/null +++ b/config/environment.go @@ -0,0 +1,61 @@ +// Copyright (c) 2019-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package config + +import ( + "reflect" + + "github.com/mattermost/mattermost-server/model" +) + +// 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. +func removeEnvOverrides(cfg, cfgWithoutEnv *model.Config, envOverrides map[string]interface{}) *model.Config { + paths := getPaths(envOverrides) + newCfg := cfg.Clone() + for _, path := range paths { + originalVal := getVal(cfgWithoutEnv, path) + getVal(newCfg, path).Set(originalVal) + } + return newCfg +} + +// getPaths turns a nested map into a slice of paths describing the keys of the map. Eg: +// map[string]map[string]map[string]bool{"this":{"is first":{"path":true}, "is second":{"path":true}))) is turned into: +// [][]string{{"this", "is first", "path"}, {"this", "is second", "path"}} +func getPaths(m map[string]interface{}) [][]string { + return getPathsRec(m, nil) +} + +// getPathsRec assembles the paths (see `getPaths` above) +func getPathsRec(src interface{}, curPath []string) [][]string { + if srcMap, ok := src.(map[string]interface{}); ok { + paths := [][]string{} + for k, v := range srcMap { + paths = append(paths, getPathsRec(v, append(curPath, k))...) + } + return paths + } + + return [][]string{curPath} +} + +// getVal walks `src` (here it starts with a model.Config, then recurses into its leaves) +// and returns the reflect.Value of the leaf at the end `path` +func getVal(src interface{}, path []string) reflect.Value { + var val reflect.Value + if reflect.ValueOf(src).Kind() == reflect.Ptr { + val = reflect.ValueOf(src).Elem().FieldByName(path[0]) + } else { + val = reflect.ValueOf(src).FieldByName(path[0]) + } + if val.Kind() == reflect.Ptr { + val = val.Elem() + } + if val.Kind() == reflect.Struct { + return getVal(val.Interface(), path[1:]) + } + return val +} diff --git a/config/file_test.go b/config/file_test.go index 5395b0b8c1..4eaf5a2de0 100644 --- a/config/file_test.go +++ b/config/file_test.go @@ -48,7 +48,7 @@ func setupConfigFile(t *testing.T, cfg *model.Config) (string, func()) { } } -// getActualFileConfig returns the configuration present in the given file without relying a config store. +// getActualFileConfig returns the configuration present in the given file without relying on a config store. func getActualFileConfig(t *testing.T, path string) *model.Config { t.Helper() @@ -428,6 +428,8 @@ func TestFileStoreLoad(t *testing.T) { require.NoError(t, err) defer fs.Close() + assert.Equal(t, "http://minimal", *fs.Get().ServiceSettings.SiteURL) + os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") err = fs.Load() @@ -436,6 +438,28 @@ func TestFileStoreLoad(t *testing.T) { assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, fs.GetEnvironmentOverrides()) }) + t.Run("do not persist environment variables", func(t *testing.T) { + path, tearDown := setupConfigFile(t, minimalConfig) + defer tearDown() + + os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://overridePersistEnvVariables") + + fs, err := config.NewFileStore(path, false) + require.NoError(t, err) + defer fs.Close() + + assert.Equal(t, "http://overridePersistEnvVariables", *fs.Get().ServiceSettings.SiteURL) + + _, err = fs.Set(fs.Get()) + require.NoError(t, err) + + assert.Equal(t, "http://overridePersistEnvVariables", *fs.Get().ServiceSettings.SiteURL) + assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, fs.GetEnvironmentOverrides()) + // check that on disk config does not include overwritten variable + actualConfig := getActualFileConfig(t, path) + assert.Equal(t, "http://minimal", *actualConfig.ServiceSettings.SiteURL) + }) + t.Run("invalid", func(t *testing.T) { path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown()