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 <jesse.hallam@gmail.com>

Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

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

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

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
+}

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 }

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

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)

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

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

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 {

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)
+
+}

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"
+}