From 7f7f511d1ca52bd1b02f8211ad1a18bf661f3559 Mon Sep 17 00:00:00 2001 From: Christopher Poile Date: Tue, 26 Mar 2019 16:28:41 -0400 Subject: [PATCH] MM-11697: Environment overrides do not overwrite config.json on save (#10413) * MM-11697: Environment overrides do not overwrite config.json on save #10388 The config store now keeps a copy of the config as loaded from the store without environment overrides. Whenever persisting, we now check if the current setting is different from the loaded setting. If it is, then use the loaded setting instead. As described in the comments to `removeEnvOverrides` in `common.go`, this behavior will have to change if we ever let the user change a setting that has been environmentally overriden. This was interesting because the `load` function in `common.go` also persists, so we have to tee the provided `io.ReadCloser` and construct a config that doesn't have the environment overrides. And then we have to find the path to the (maybe) changed variable in the config struct using reflection. Possible WIP: I had to expose a `GetWithoutEnvOverrides` function in the Store interface just for the tests -- this is because the `file_test` and `database_test`s are in the config_test package instead of the `config` package. * added function documentation * fixed a small problem with tests * MM-11697: big cleanup based on Jesse's PR comments * MM-11697: edits per PR feedback * MM-11697: licence header * MM-11697: now testing that on disk config is not changed by env overrides * MM-11697: remove unneeded exports --- config/common.go | 33 +++++++++++++++++----- config/database_test.go | 22 +++++++++++++++ config/environment.go | 61 +++++++++++++++++++++++++++++++++++++++++ config/file_test.go | 26 +++++++++++++++++- 4 files changed, 134 insertions(+), 8 deletions(-) create mode 100644 config/environment.go 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()