[MM-32390] Config logic refactor (#17578)

* Replace config generator * Cleanup * Some renaming and docs additions to add clarity * Cleanup logging related methods * Cleanup emitter * Fix TestDefaultsGenerator * Move feature flags synchronization logic out of config package * Remove unnecessary util functions * Simplify load/set logic * Refine semantics and add some test to cover them * Remove unnecessary deep copies * Improve logic further * Fix license header * Review file store tests * Fix test * Fix test * Avoid additional write during initialization * More consistent naming * Update app/feature_flags.go Co-authored-by: Christopher Speller <crspeller@gmail.com> * Update config/store.go Co-authored-by: Christopher Speller <crspeller@gmail.com> * Update config/store.go Co-authored-by: Christopher Speller <crspeller@gmail.com> * Update config/store.go Co-authored-by: Ibrahim Serdar Acikgoz <serdaracikgoz86@gmail.com> * Move FF synchronizer to its own package * Remove unidiomatic use of sync.Once * Add some comments * Rename function * More comment Co-authored-by: Christopher Speller <crspeller@gmail.com> Co-authored-by: Ibrahim Serdar Acikgoz <serdaracikgoz86@gmail.com>
mattermost · May 19, 2021 · 3681cd3 · 3681cd3
1 parent b810a40
commit 3681cd3
Show file tree

Hide file tree

Showing 37 changed files with 816 additions and 665 deletions.
diff --git a/Makefile b/Makefile
@@ -549,7 +549,7 @@ config-ldap: ## Configures LDAP.
 config-reset: ## Resets the config/config.json file to the default.
 	@echo Resetting configuration to default
 	rm -f config/config.json
-	OUTPUT_CONFIG=$(PWD)/config/config.json $(GO) generate $(GOFLAGS) ./config
+	OUTPUT_CONFIG=$(PWD)/config/config.json $(GO) $(GOFLAGS) run ./scripts/config_generator
 
 diff-config: ## Compares default configuration between two mattermost versions
 	@./scripts/diff-config.sh

diff --git a/api4/config_test.go b/api4/config_test.go
@@ -721,11 +721,11 @@ func TestMigrateConfig(t *testing.T) {
 	})
 
 	th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) {
-		f, err := config.NewStore("from.json", false, false, nil)
+		f, err := config.NewStoreFromDSN("from.json", false, false, nil)
 		require.NoError(t, err)
 		defer f.RemoveFile("from.json")
 
-		_, err = config.NewStore("to.json", false, false, nil)
+		_, err = config.NewStoreFromDSN("to.json", false, false, nil)
 		require.NoError(t, err)
 		defer f.RemoveFile("to.json")
 

diff --git a/api4/user_test.go b/api4/user_test.go
@@ -5523,6 +5523,9 @@ func TestThreadSocketEvents(t *testing.T) {
 	os.Setenv("MM_FEATUREFLAGS_COLLAPSEDTHREADS", "true")
 	defer os.Unsetenv("MM_FEATUREFLAGS_COLLAPSEDTHREADS")
 
+	th.ConfigStore.SetReadOnlyFF(false)
+	defer th.ConfigStore.SetReadOnlyFF(true)
+
 	th.App.UpdateConfig(func(cfg *model.Config) {
 		*cfg.ServiceSettings.ThreadAutoFollow = true
 		*cfg.ServiceSettings.CollapsedThreads = model.COLLAPSED_THREADS_DEFAULT_ON

diff --git a/app/audit.go b/app/audit.go
@@ -143,15 +143,12 @@ func (s *Server) configureAudit(adt *audit.Audit, bAllowAdvancedLogging bool) er
 	if !bAllowAdvancedLogging || dsn == "" {
 		return errs
 	}
-	isJson := config.IsJsonMap(dsn)
-	cfg, err := config.NewLogConfigSrc(dsn, isJson, s.configStore)
+	cfg, err := config.NewLogConfigSrc(dsn, s.configStore)
 	if err != nil {
 		errs = multierror.Append(fmt.Errorf("invalid config for audit, %w", err))
 		return errs
 	}
-	if !isJson {
-		mlog.Debug("Loaded audit configuration", mlog.String("filename", dsn))
-	}
+	mlog.Debug("Loaded audit configuration", mlog.String("source", dsn))
 
 	for name, t := range cfg.Get() {
 		if len(t.Levels) == 0 {

diff --git a/app/feature_flags.go b/app/feature_flags.go
@@ -8,7 +8,7 @@ import (
 	"os"
 	"time"
 
-	"github.com/mattermost/mattermost-server/v5/config"
+	"github.com/mattermost/mattermost-server/v5/app/featureflag"
 	"github.com/mattermost/mattermost-server/v5/shared/mlog"
 )
 
@@ -21,7 +21,7 @@ func (s *Server) setupFeatureFlags() {
 	splitConfigured := splitKey != ""
 	syncFeatureFlags := splitConfigured && s.IsLeader()
 
-	s.configStore.PersistFeatures(splitConfigured)
+	s.configStore.SetReadOnlyFF(!splitConfigured)
 
 	if syncFeatureFlags {
 		if err := s.startFeatureFlagUpdateJob(); err != nil {
@@ -71,7 +71,7 @@ func (s *Server) startFeatureFlagUpdateJob() error {
 		attributes["group_id"] = groupId
 	}
 
-	synchronizer, err := config.NewFeatureFlagSynchronizer(config.FeatureFlagSyncParams{
+	synchronizer, err := featureflag.NewSynchronizer(featureflag.SyncParams{
 		ServerID:   s.TelemetryId(),
 		SplitKey:   *s.Config().ServiceSettings.SplitKey,
 		Log:        log,

diff --git a/config/feature_flags.go → app/featureflag/feature_flags_sync.go b/config/feature_flags.go → app/featureflag/feature_flags_sync.go
@@ -1,7 +1,7 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
 
-package config
+package featureflag
 
 import (
 	"math"
@@ -17,16 +17,16 @@ import (
 	"github.com/mattermost/mattermost-server/v5/shared/mlog"
 )
 
-type FeatureFlagSyncParams struct {
+type SyncParams struct {
 	ServerID            string
 	SplitKey            string
 	SyncIntervalSeconds int
 	Log                 *mlog.Logger
 	Attributes          map[string]interface{}
 }
 
-type FeatureFlagSynchronizer struct {
-	FeatureFlagSyncParams
+type Synchronizer struct {
+	SyncParams
 
 	client  *client.SplitClient
 	stop    chan struct{}
@@ -35,7 +35,7 @@ type FeatureFlagSynchronizer struct {
 
 var featureNames = getStructFields(model.FeatureFlags{})
 
-func NewFeatureFlagSynchronizer(params FeatureFlagSyncParams) (*FeatureFlagSynchronizer, error) {
+func NewSynchronizer(params SyncParams) (*Synchronizer, error) {
 	cfg := conf.Default()
 	if params.Log != nil {
 		cfg.Logger = &splitLogger{wrappedLog: params.Log.With(mlog.String("service", "split"))}
@@ -47,30 +47,30 @@ func NewFeatureFlagSynchronizer(params FeatureFlagSyncParams) (*FeatureFlagSynch
 		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{}),
+	return &Synchronizer{
+		SyncParams: 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 {
+func (f *Synchronizer) EnsureReady() error {
 	if err := f.client.BlockUntilReady(10); err != nil {
 		return errors.Wrap(err, "split.io client could not initialize")
 	}
 
 	return nil
 }
 
-func (f *FeatureFlagSynchronizer) UpdateFeatureFlagValues(base model.FeatureFlags) model.FeatureFlags {
+func (f *Synchronizer) UpdateFeatureFlagValues(base model.FeatureFlags) model.FeatureFlags {
 	featuresMap := f.client.Treatments(f.ServerID, featureNames, f.Attributes)
 	ffm := featureFlagsFromMap(featuresMap, base)
 	return ffm
 }
 
-func (f *FeatureFlagSynchronizer) Close() {
+func (f *Synchronizer) Close() {
 	f.client.Destroy()
 }
 

diff --git a/config/feature_flags_test.go → app/featureflag/feature_flags_sync_test.go b/config/feature_flags_test.go → app/featureflag/feature_flags_sync_test.go
@@ -1,7 +1,7 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
 
-package config
+package featureflag
 
 import (
 	"testing"

diff --git a/config/split_logger.go → app/featureflag/split_logger.go b/config/split_logger.go → app/featureflag/split_logger.go
@@ -1,7 +1,7 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
 
-package config
+package featureflag
 
 import (
 	"fmt"

diff --git a/app/options.go b/app/options.go
@@ -45,7 +45,7 @@ func StoreOverride(override interface{}) Option {
 // config loaded from the dsn on top of the normal defaults
 func Config(dsn string, watch, readOnly bool, configDefaults *model.Config) Option {
 	return func(s *Server) error {
-		configStore, err := config.NewStore(dsn, watch, readOnly, configDefaults)
+		configStore, err := config.NewStoreFromDSN(dsn, watch, readOnly, configDefaults)
 		if err != nil {
 			return errors.Wrap(err, "failed to apply Config option")
 		}

diff --git a/app/server.go b/app/server.go
@@ -19,7 +19,6 @@ import (
 	"os"
 	"os/exec"
 	"path"
-	"path/filepath"
 	"runtime"
 	"strconv"
 	"strings"
@@ -38,6 +37,7 @@ import (
 	"github.com/rs/cors"
 	"golang.org/x/crypto/acme/autocert"
 
+	"github.com/mattermost/mattermost-server/v5/app/featureflag"
 	"github.com/mattermost/mattermost-server/v5/app/request"
 	"github.com/mattermost/mattermost-server/v5/audit"
 	"github.com/mattermost/mattermost-server/v5/config"
@@ -200,7 +200,7 @@ type Server struct {
 	uploadLockMapMut sync.Mutex
 	uploadLockMap    map[string]bool
 
-	featureFlagSynchronizer      *config.FeatureFlagSynchronizer
+	featureFlagSynchronizer      *featureflag.Synchronizer
 	featureFlagStop              chan struct{}
 	featureFlagStopped           chan struct{}
 	featureFlagSynchronizerMutex sync.Mutex
@@ -805,15 +805,8 @@ func (s *Server) initLogging() error {
 	// shutdown once license is loaded/checked.
 	if *s.Config().LogSettings.AdvancedLoggingConfig != "" {
 		dsn := *s.Config().LogSettings.AdvancedLoggingConfig
-		isJson := config.IsJsonMap(dsn)
 
-		// If this is a file based config we need the full path so it can be watched.
-		if !isJson && strings.HasPrefix(s.configStore.String(), "file://") && !filepath.IsAbs(dsn) {
-			configPath := strings.TrimPrefix(s.configStore.String(), "file://")
-			dsn = filepath.Join(filepath.Dir(configPath), dsn)
-		}
-
-		cfg, err := config.NewLogConfigSrc(dsn, isJson, s.configStore)
+		cfg, err := config.NewLogConfigSrc(dsn, s.configStore)
 		if err != nil {
 			return fmt.Errorf("invalid advanced logging config, %w", err)
 		}
@@ -822,9 +815,7 @@ func (s *Server) initLogging() error {
 			return fmt.Errorf("error configuring advanced logging, %w", err)
 		}
 
-		if !isJson {
-			mlog.Info("Loaded advanced logging config", mlog.String("source", dsn))
-		}
+		mlog.Info("Loaded advanced logging config", mlog.String("source", dsn))
 
 		listenerId := cfg.AddListener(func(_, newCfg mlog.LogTargetCfg) {
 			if err := s.Log.ConfigAdvancedLogging(newCfg); err != nil {

diff --git a/build/release.mk b/build/release.mk
@@ -76,7 +76,7 @@ package:
 	@# Resource directories
 	mkdir -p $(DIST_PATH)/config
 	cp -L config/README.md $(DIST_PATH)/config
-	OUTPUT_CONFIG=$(PWD)/$(DIST_PATH)/config/config.json go generate ./config
+	OUTPUT_CONFIG=$(PWD)/$(DIST_PATH)/config/config.json go run ./scripts/config_generator
 	cp -RL fonts $(DIST_PATH)
 	cp -RL templates $(DIST_PATH)
 	rm -rf $(DIST_PATH)/templates/*.mjml $(DIST_PATH)/templates/partials/

diff --git a/cmd/mattermost/commands/config.go b/cmd/mattermost/commands/config.go
@@ -146,7 +146,7 @@ func getConfigStore(command *cobra.Command) (*config.Store, error) {
 		return nil, errors.Wrap(err, "failed to initialize i18n")
 	}
 
-	configStore, err := config.NewStore(getConfigDSN(command, config.GetEnvironment()), false, false, nil)
+	configStore, err := config.NewStoreFromDSN(getConfigDSN(command, config.GetEnvironment()), false, false, nil)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to initialize config store")
 	}

diff --git a/cmd/mattermost/commands/config_test.go b/cmd/mattermost/commands/config_test.go
@@ -543,9 +543,9 @@ func TestConfigMigrate(t *testing.T) {
 	sqlDSN := getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource)
 	fileDSN := "config.json"
 
-	ds, err := config.NewStore(sqlDSN, false, false, nil)
+	ds, err := config.NewStoreFromDSN(sqlDSN, false, false, nil)
 	require.NoError(t, err)
-	fs, err := config.NewStore(fileDSN, false, false, nil)
+	fs, err := config.NewStoreFromDSN(fileDSN, false, false, nil)
 	require.NoError(t, err)
 
 	defer ds.Close()

diff --git a/cmd/mattermost/commands/db.go b/cmd/mattermost/commands/db.go
@@ -57,7 +57,7 @@ func initDbCmdF(command *cobra.Command, _ []string) error {
 		return errors.Wrap(err, "error loading custom configuration defaults")
 	}
 
-	configStore, err := config.NewStore(getConfigDSN(command, config.GetEnvironment()), false, false, customDefaults)
+	configStore, err := config.NewStoreFromDSN(getConfigDSN(command, config.GetEnvironment()), false, false, customDefaults)
 	if err != nil {
 		return errors.Wrap(err, "failed to load configuration")
 	}

diff --git a/cmd/mattermost/commands/server.go b/cmd/mattermost/commands/server.go
@@ -52,7 +52,7 @@ func serverCmdF(command *cobra.Command, args []string) error {
 		mlog.Warn("Error loading custom configuration defaults: " + err.Error())
 	}
 
-	configStore, err := config.NewStore(getConfigDSN(command, config.GetEnvironment()), !disableConfigWatch, false, customDefaults)
+	configStore, err := config.NewStoreFromDSN(getConfigDSN(command, config.GetEnvironment()), !disableConfigWatch, false, customDefaults)
 	if err != nil {
 		return errors.Wrap(err, "failed to load configuration")
 	}