From e974b7b9be5ec499b2e48a86038b6edbb1fa8bbd Mon Sep 17 00:00:00 2001 From: Christopher Speller Date: Tue, 29 Sep 2020 21:41:22 -0700 Subject: [PATCH] MM28858 Basic framework for use of feature flags. Enviroment variable overrides. (#15544) * Basic framework for use of feature flags. Enviroment variable overrides. * Use Apperr instead of error number increments. * Undo random viper change. * Update model/config_test.go Co-authored-by: Jesse Hallam Co-authored-by: Jesse Hallam --- api4/system.go | 5 ++ api4/system_test.go | 31 ++++++++++++ config/common.go | 9 +++- config/common_test.go | 28 +++++++---- config/database_test.go | 2 - config/file_test.go | 2 - config/unmarshal.go | 103 +++++---------------------------------- config/unmarshal_test.go | 42 ++++++++-------- model/config.go | 5 ++ model/config_test.go | 18 +++++++ model/feature_flags.go | 14 ++++++ 11 files changed, 130 insertions(+), 129 deletions(-) create mode 100644 model/feature_flags.go diff --git a/api4/system.go b/api4/system.go index ab83a260a8f..da1edfd8bab 100644 --- a/api4/system.go +++ b/api4/system.go @@ -79,6 +79,11 @@ func getSystemPing(c *Context, w http.ResponseWriter, r *http.Request) { s["IosLatestVersion"] = reqs.IosLatestVersion s["IosMinVersion"] = reqs.IosMinVersion + testflag := c.App.Config().FeatureFlags.TestFeature + if testflag != "off" { + s["TestFeatureFlag"] = testflag + } + actualGoroutines := runtime.NumGoroutine() if *c.App.Config().ServiceSettings.GoroutineHealthThreshold > 0 && actualGoroutines >= *c.App.Config().ServiceSettings.GoroutineHealthThreshold { mlog.Warn("The number of running goroutines is over the health threshold", mlog.Int("goroutines", actualGoroutines), mlog.Int("health_threshold", *c.App.Config().ServiceSettings.GoroutineHealthThreshold)) diff --git a/api4/system_test.go b/api4/system_test.go index 3dc37711d1e..078f89e2ddb 100644 --- a/api4/system_test.go +++ b/api4/system_test.go @@ -14,6 +14,7 @@ import ( "testing" "time" + "github.com/mattermost/mattermost-server/v5/config" "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" "github.com/stretchr/testify/assert" @@ -64,6 +65,36 @@ func TestGetPing(t *testing.T) { }) }) + + t.Run("ping feature flag test", func(t *testing.T) { + resp, appErr := th.Client.DoApiGet(th.Client.GetSystemRoute()+"/ping", "") + require.Nil(t, appErr) + require.Equal(t, http.StatusOK, resp.StatusCode) + respBytes, err := ioutil.ReadAll(resp.Body) + require.Nil(t, err) + respString := string(respBytes) + require.NotContains(t, respString, "TestFeatureFlag") + + // Run the enviroment variable override code to test + os.Setenv("MM_FEATUREFLAGS_TESTFEATURE", "testvalue") + defer os.Unsetenv("MM_FEATUREFLAGS_TESTFEATURE") + memoryStore, err := config.NewMemoryStore() + require.Nil(t, err) + retrievedConfig := memoryStore.Get() + + // replace config with generated config + oldConfig := th.App.Config().Clone() + th.App.UpdateConfig(func(cfg *model.Config) { *cfg = *retrievedConfig }) + + resp, appErr = th.Client.DoApiGet(th.Client.GetSystemRoute()+"/ping", "") + require.Nil(t, appErr) + require.Equal(t, http.StatusOK, resp.StatusCode) + respBytes, err = ioutil.ReadAll(resp.Body) + require.Nil(t, err) + respString = string(respBytes) + require.Contains(t, respString, "testvalue") + th.App.UpdateConfig(func(cfg *model.Config) { *cfg = *oldConfig }) + }) } func TestGetAudits(t *testing.T) { diff --git a/config/common.go b/config/common.go index c69ebbc8f27..40df705a6a0 100644 --- a/config/common.go +++ b/config/common.go @@ -77,7 +77,7 @@ func (cs *commonStore) set(newCfg *model.Config, allowEnvironmentOverrides bool, } } - if err := persist(cs.RemoveEnvironmentOverrides(newCfg)); err != nil { + if err := persist(cs.RemoveNonPersistable(newCfg)); err != nil { return nil, errors.Wrap(err, "failed to persist") } @@ -168,3 +168,10 @@ func (cs *commonStore) validate(cfg *model.Config) error { 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 +} diff --git a/config/common_test.go b/config/common_test.go index 64e2b8897eb..af5c6bd9ba1 100644 --- a/config/common_test.go +++ b/config/common_test.go @@ -39,6 +39,7 @@ func init() { DefaultClientLocale: sToP("en"), }, } + minimalConfig.SetDefaults() invalidConfig = &model.Config{ ServiceSettings: model.ServiceSettings{ SiteURL: sToP("invalid"), @@ -73,16 +74,6 @@ func init() { } } -func prepareExpectedConfig(t *testing.T, expectedCfg *model.Config) *model.Config { - // These fields require special initialization for our tests. - expectedCfg = expectedCfg.Clone() - expectedCfg.MessageExportSettings.GlobalRelaySettings = &model.GlobalRelayMessageExportSettings{} - expectedCfg.PluginSettings.Plugins = make(map[string]map[string]interface{}) - expectedCfg.PluginSettings.PluginStates = make(map[string]*model.PluginState) - - return expectedCfg -} - func TestMergeConfigs(t *testing.T) { t.Run("merge two default configs with different salts/keys", func(t *testing.T) { base, err := config.NewMemoryStore() @@ -174,5 +165,22 @@ 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 } diff --git a/config/database_test.go b/config/database_test.go index 3e58a0fcd43..132ac5d3261 100644 --- a/config/database_test.go +++ b/config/database_test.go @@ -100,7 +100,6 @@ func getActualDatabaseConfig(t *testing.T) (string, *model.Config) { func assertDatabaseEqualsConfig(t *testing.T, expectedCfg *model.Config) { t.Helper() - expectedCfg = prepareExpectedConfig(t, expectedCfg) _, actualCfg := getActualDatabaseConfig(t) assert.Equal(t, expectedCfg, actualCfg) } @@ -109,7 +108,6 @@ func assertDatabaseEqualsConfig(t *testing.T, expectedCfg *model.Config) { func assertDatabaseNotEqualsConfig(t *testing.T, expectedCfg *model.Config) { t.Helper() - expectedCfg = prepareExpectedConfig(t, expectedCfg) _, actualCfg := getActualDatabaseConfig(t) assert.NotEqual(t, expectedCfg, actualCfg) } diff --git a/config/file_test.go b/config/file_test.go index ae5fd29edbe..b775c7fea0e 100644 --- a/config/file_test.go +++ b/config/file_test.go @@ -66,7 +66,6 @@ func getActualFileConfig(t *testing.T, path string) *model.Config { func assertFileEqualsConfig(t *testing.T, expectedCfg *model.Config, path string) { t.Helper() - expectedCfg = prepareExpectedConfig(t, expectedCfg) actualCfg := getActualFileConfig(t, path) assert.Equal(t, expectedCfg, actualCfg) @@ -76,7 +75,6 @@ func assertFileEqualsConfig(t *testing.T, expectedCfg *model.Config, path string func assertFileNotEqualsConfig(t *testing.T, expectedCfg *model.Config, path string) { t.Helper() - expectedCfg = prepareExpectedConfig(t, expectedCfg) actualCfg := getActualFileConfig(t, path) assert.NotEqual(t, expectedCfg, actualCfg) diff --git a/config/unmarshal.go b/config/unmarshal.go index 07fd6afef0b..a103fb255dc 100644 --- a/config/unmarshal.go +++ b/config/unmarshal.go @@ -33,99 +33,9 @@ func newViper(allowEnvironmentOverrides bool) *viper.Viper { v.AutomaticEnv() } - // Set zeroed defaults for all the config settings so that Viper knows what environment variables - // it needs to be looking for. The correct defaults will later be applied using Config.SetDefaults. - defaults := getDefaultsFromStruct(model.Config{}) - - for key, value := range defaults { - if key == "PluginSettings.Plugins" || key == "PluginSettings.PluginStates" { - continue - } - - v.SetDefault(key, value) - } - return v } -func getDefaultsFromStruct(s interface{}) map[string]interface{} { - return flattenStructToMap(structToMap(reflect.TypeOf(s))) -} - -// Converts a struct type into a nested map with keys matching the struct's fields and values -// matching the zeroed value of the corresponding field. -func structToMap(t reflect.Type) (out map[string]interface{}) { - defer func() { - if r := recover(); r != nil { - mlog.Error("Panicked in structToMap. This should never happen.", mlog.Any("err", r)) - } - }() - - if t.Kind() != reflect.Struct { - // Should never hit this, but this will prevent a panic if that does happen somehow - return nil - } - - out = map[string]interface{}{} - - for i := 0; i < t.NumField(); i++ { - field := t.Field(i) - - var value interface{} - - switch field.Type.Kind() { - case reflect.Struct: - value = structToMap(field.Type) - case reflect.Ptr: - indirectType := field.Type.Elem() - - if indirectType.Kind() == reflect.Struct { - // Follow pointers to structs since we need to define defaults for their fields - value = structToMap(indirectType) - } else { - value = nil - } - default: - value = reflect.Zero(field.Type).Interface() - } - - out[field.Name] = value - } - - return -} - -// Flattens a nested map so that the result is a single map with keys corresponding to the -// path through the original map. For example, -// { -// "a": { -// "b": 1 -// }, -// "c": "sea" -// } -// would flatten to -// { -// "a.b": 1, -// "c": "sea" -// } -func flattenStructToMap(in map[string]interface{}) map[string]interface{} { - out := make(map[string]interface{}) - - for key, value := range in { - if valueAsMap, ok := value.(map[string]interface{}); ok { - sub := flattenStructToMap(valueAsMap) - - for subKey, subValue := range sub { - out[key+"."+subKey] = subValue - } - } else { - out[key] = value - } - } - - return out -} - // marshalConfig converts the given configuration into JSON bytes for persistence. func marshalConfig(cfg *model.Config) ([]byte, error) { return json.MarshalIndent(cfg, "", " ") @@ -139,13 +49,18 @@ func unmarshalConfig(r io.Reader, allowEnvironmentOverrides bool) (*model.Config return nil, nil, errors.Wrapf(err, "failed to read") } - var rawConfig interface{} + 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(configData)); err != nil { + if err := v.ReadConfig(bytes.NewReader(dataWithDefaults)); err != nil { return nil, nil, err } @@ -184,6 +99,10 @@ func fixEnvSettingsCase(in map[string]interface{}) (out map[string]interface{}, 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 diff --git a/config/unmarshal_test.go b/config/unmarshal_test.go index cd6a2e2f4b5..bde4e27de61 100644 --- a/config/unmarshal_test.go +++ b/config/unmarshal_test.go @@ -16,28 +16,6 @@ import ( "github.com/mattermost/mattermost-server/v5/utils" ) -func TestGetDefaultsFromStruct(t *testing.T) { - s := struct { - TestSettings struct { - IntValue int - BoolValue bool - StringValue string - } - PointerToTestSettings *struct { - Value int - } - }{} - - defaults := getDefaultsFromStruct(s) - - assert.Equal(t, defaults["TestSettings.IntValue"], 0) - assert.Equal(t, defaults["TestSettings.BoolValue"], false) - assert.Equal(t, defaults["TestSettings.StringValue"], "") - assert.Equal(t, defaults["PointerToTestSettings.Value"], 0) - assert.NotContains(t, defaults, "PointerToTestSettings") - assert.Len(t, defaults, 4) -} - 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") @@ -261,6 +239,26 @@ func TestConfigFromEnviroVars(t *testing.T) { 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") diff --git a/model/config.go b/model/config.go index 02830b67e84..7d46ad318bf 100644 --- a/model/config.go +++ b/model/config.go @@ -2882,6 +2882,7 @@ type Config struct { GuestAccountsSettings GuestAccountsSettings ImageProxySettings ImageProxySettings CloudSettings CloudSettings + FeatureFlags *FeatureFlags `json:",omitempty"` } func (o *Config) Clone() *Config { @@ -2969,6 +2970,10 @@ func (o *Config) SetDefaults() { o.GuestAccountsSettings.SetDefaults() o.ImageProxySettings.SetDefaults(o.ServiceSettings) o.CloudSettings.SetDefaults() + if o.FeatureFlags == nil { + o.FeatureFlags = &FeatureFlags{} + o.FeatureFlags.SetDefaults() + } } func (o *Config) IsValid() *AppError { diff --git a/model/config_test.go b/model/config_test.go index 115ff492988..41c580f3fc7 100644 --- a/model/config_test.go +++ b/model/config_test.go @@ -1326,3 +1326,21 @@ func TestConfigMarketplaceDefaults(t *testing.T) { require.Equal(t, "https://marketplace.example.com", *c.PluginSettings.MarketplaceUrl) }) } + +func TestSetDefaultFeatureFlagBehaviour(t *testing.T) { + cfg := Config{} + cfg.SetDefaults() + + require.NotNil(t, cfg.FeatureFlags) + require.Equal(t, "off", cfg.FeatureFlags.TestFeature) + + cfg = Config{ + FeatureFlags: &FeatureFlags{ + TestFeature: "somevalue", + }, + } + cfg.SetDefaults() + require.NotNil(t, cfg.FeatureFlags) + require.Equal(t, "somevalue", cfg.FeatureFlags.TestFeature) + +} diff --git a/model/feature_flags.go b/model/feature_flags.go new file mode 100644 index 00000000000..2646da2c0c8 --- /dev/null +++ b/model/feature_flags.go @@ -0,0 +1,14 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package model + +type FeatureFlags struct { + // Exists only for unit and manual testing. + // When set to a value, will be returned by the ping endpoint. + TestFeature string +} + +func (f *FeatureFlags) SetDefaults() { + f.TestFeature = "off" +}