MM-13893: introduce file store (#10243)

* config file store Introduce an interface and concrete implementation for accessing the config. This mostly maps 1:1 with the exiting usage in `App`, except for internalizing the watcher. A future change will likely eliminate `App.PersistConfig()` and make this implicit on `Set` or `Patch` * experimental file test changes * emoji: move file driver checks from api4 to app It is no longer possible to app.UpdateConfig and provide an invalid configuration, making it hard to test this case. This check doesn't really belong in the api anyway, since it's a configuration validity check and not a permissions check. Either way, the check now occurs at the App level. * api4: generate valid public link salts for test * TestStartServerRateLimiterCriticalError: use mock store to test invalid config * remove config_test.go * remove needsSave, and have Load() save to the backing store as necessary * restore README.md * move ldap UserFilter check to model isValid checks * remove databaseStore until ready * remove unimplemented Patch * simplify unlockOnce implementation * revert forgetting to set s.Ldap * config/file.go: rename ReadOnlyConfigurationError to ErrReadOnlyConfiguration * config: export FileStore * add TestFileStoreSave * improved config/utils test coverage * restore config/README.md copy * tweaks * file store: acquire a write lock on Save/Close to safely close watcher * fix unmarshal_test.go
2019-02-12 14:19:01 -04:00
--- a/app/admin.go
+++ b/app/admin.go
@@ -12,7 +12,7 @@ import (

 	"net/http"

-	"github.com/mattermost/mattermost-server/einterfaces"
+	"github.com/mattermost/mattermost-server/config"
 	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/services/mailservice"
@@ -160,40 +160,15 @@ func (a *App) GetEnvironmentConfig() map[string]interface{} {
 	return a.EnvironmentConfig()
 }

-func validateLdapFilter(cfg *model.Config, ldap einterfaces.LdapInterface) *model.AppError {
-	if !*cfg.LdapSettings.Enable || ldap == nil || *cfg.LdapSettings.UserFilter == "" {
-		return nil
+func (a *App) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bool) *model.AppError {
+	oldCfg, err := a.Srv.configStore.Set(newCfg)
+	if err == config.ErrReadOnlyConfiguration {
+		return model.NewAppError("saveConfig", "ent.cluster.save_config.error", nil, err.Error(), http.StatusForbidden)
+	} else if err != nil {
+		return model.NewAppError("saveConfig", "app.save_config.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}

-	return ldap.ValidateFilter(*cfg.LdapSettings.UserFilter)
-}
-
-func (a *App) SaveConfig(cfg *model.Config, sendConfigChangeClusterMessage bool) *model.AppError {
-	oldCfg := a.Config()
-	cfg.SetDefaults()
-	a.Desanitize(cfg)
-
-	if err := cfg.IsValid(); err != nil {
-		return err
-	}
-
-	if err := validateLdapFilter(cfg, a.Ldap); err != nil {
-		return err
-	}
-
-	if *a.Config().ClusterSettings.Enable && *a.Config().ClusterSettings.ReadOnlyConfig {
-		return model.NewAppError("saveConfig", "ent.cluster.save_config.error", nil, "", http.StatusForbidden)
-	}
-
-	a.DisableConfigWatch()
-
-	a.UpdateConfig(func(update *model.Config) {
-		*update = *cfg
-	})
 	a.PersistConfig()
-	a.ReloadConfig()
-	a.EnableConfigWatch()
-
 	if a.Metrics != nil {
 		if *a.Config().MetricsSettings.Enable {
 			a.Metrics.StartServer()
@@ -203,7 +178,7 @@ func (a *App) SaveConfig(cfg *model.Config, sendConfigChangeClusterMessage bool)
 	}

 	if a.Cluster != nil {
-		err := a.Cluster.ConfigChanged(cfg, oldCfg, sendConfigChangeClusterMessage)
+		err := a.Cluster.ConfigChanged(newCfg, oldCfg, sendConfigChangeClusterMessage)
 		if err != nil {
 			return err
 		}
--- a/app/config.go
+++ b/app/config.go
@@ -15,7 +15,6 @@ import (
 	"net/url"
 	"runtime/debug"
 	"strconv"
-	"strings"
 	"time"

 	"github.com/mattermost/mattermost-server/config"
@@ -29,10 +28,7 @@ const (
 )

 func (s *Server) Config() *model.Config {
-	if cfg := s.config.Load(); cfg != nil {
-		return cfg.(*model.Config)
-	}
-	return &model.Config{}
+	return s.configStore.Get()
 }

 func (a *App) Config() *model.Config {
@@ -40,10 +36,7 @@ func (a *App) Config() *model.Config {
 }

 func (s *Server) EnvironmentConfig() map[string]interface{} {
-	if s.envConfig != nil {
-		return s.envConfig
-	}
-	return map[string]interface{}{}
+	return s.configStore.GetEnvironmentOverrides()
 }

 func (a *App) EnvironmentConfig() map[string]interface{} {
@@ -54,9 +47,9 @@ func (s *Server) UpdateConfig(f func(*model.Config)) {
 	old := s.Config()
 	updated := old.Clone()
 	f(updated)
-	s.config.Store(updated)
-
-	s.InvokeConfigListeners(old, updated)
+	if _, err := s.configStore.Set(updated); err != nil {
+		mlog.Error("Failed to update config", mlog.Err(err))
+	}
 }

 func (a *App) UpdateConfig(f func(*model.Config)) {
@@ -64,46 +57,23 @@ func (a *App) UpdateConfig(f func(*model.Config)) {
 }

 func (a *App) PersistConfig() {
-	config.SaveConfig(a.ConfigFileName(), a.Config())
-}
-
-func (s *Server) LoadConfig(configFile string) *model.AppError {
-	old := s.Config()
-
-	cfg, configPath, envConfig, err := config.LoadConfig(configFile)
-	if err != nil {
-		return err
+	if err := a.Srv.configStore.Save(); err != nil {
+		mlog.Error("Failed to persist config", mlog.Err(err))
 	}
-	*cfg.ServiceSettings.SiteURL = strings.TrimRight(*cfg.ServiceSettings.SiteURL, "/")
-	s.config.Store(cfg)
-
-	s.configFile = configPath
-	s.envConfig = envConfig
-
-	s.InvokeConfigListeners(old, cfg)
-	return nil
 }

-func (a *App) LoadConfig(configFile string) *model.AppError {
-	return a.Srv.LoadConfig(configFile)
-}
-
-func (s *Server) ReloadConfig() *model.AppError {
+func (s *Server) ReloadConfig() error {
 	debug.FreeOSMemory()
-	if err := s.LoadConfig(s.configFile); err != nil {
+	if err := s.configStore.Load(); err != nil {
 		return err
 	}
 	return nil
 }

-func (a *App) ReloadConfig() *model.AppError {
+func (a *App) ReloadConfig() error {
 	return a.Srv.ReloadConfig()
 }

-func (a *App) ConfigFileName() string {
-	return a.Srv.configFile
-}
-
 func (a *App) ClientConfig() map[string]string {
 	return a.Srv.clientConfig
 }
@@ -116,40 +86,11 @@ func (a *App) LimitedClientConfig() map[string]string {
 	return a.Srv.limitedClientConfig
 }

-func (s *Server) EnableConfigWatch() {
-	if s.configWatcher == nil && !s.disableConfigWatch {
-		configWatcher, err := config.NewConfigWatcher(s.configFile, func() {
-			s.ReloadConfig()
-		})
-		if err != nil {
-			mlog.Error(fmt.Sprint(err))
-		}
-		s.configWatcher = configWatcher
-	}
-}
-
-func (a *App) EnableConfigWatch() {
-	a.Srv.EnableConfigWatch()
-}
-
-func (s *Server) DisableConfigWatch() {
-	if s.configWatcher != nil {
-		s.configWatcher.Close()
-		s.configWatcher = nil
-	}
-}
-
-func (a *App) DisableConfigWatch() {
-	a.Srv.DisableConfigWatch()
-}
-
 // Registers a function with a given to be called when the config is reloaded and may have changed. The function
 // will be called with two arguments: the old config and the new config. AddConfigListener returns a unique ID
 // for the listener that can later be used to remove it.
 func (s *Server) AddConfigListener(listener func(*model.Config, *model.Config)) string {
-	id := model.NewId()
-	s.configListeners[id] = listener
-	return id
+	return s.configStore.AddListener(listener)
 }

 func (a *App) AddConfigListener(listener func(*model.Config, *model.Config)) string {
@@ -158,19 +99,13 @@ func (a *App) AddConfigListener(listener func(*model.Config, *model.Config)) str

 // Removes a listener function by the unique ID returned when AddConfigListener was called
 func (s *Server) RemoveConfigListener(id string) {
-	delete(s.configListeners, id)
+	s.configStore.RemoveListener(id)
 }

 func (a *App) RemoveConfigListener(id string) {
 	a.Srv.RemoveConfigListener(id)
 }

-func (s *Server) InvokeConfigListeners(old, current *model.Config) {
-	for _, listener := range s.configListeners {
-		listener(old, current)
-	}
-}
-
 // EnsureAsymmetricSigningKey ensures that an asymmetric signing key exists and future calls to
 // AsymmetricSigningKey will always return a valid signing key.
 func (a *App) ensureAsymmetricSigningKey() error {
@@ -306,51 +241,6 @@ func (a *App) regenerateClientConfig() {
 	a.Srv.clientConfigHash = fmt.Sprintf("%x", md5.Sum(clientConfigJSON))
 }

-func (a *App) Desanitize(cfg *model.Config) {
-	actual := a.Config()
-
-	if cfg.LdapSettings.BindPassword != nil && *cfg.LdapSettings.BindPassword == model.FAKE_SETTING {
-		*cfg.LdapSettings.BindPassword = *actual.LdapSettings.BindPassword
-	}
-
-	if *cfg.FileSettings.PublicLinkSalt == model.FAKE_SETTING {
-		*cfg.FileSettings.PublicLinkSalt = *actual.FileSettings.PublicLinkSalt
-	}
-	if *cfg.FileSettings.AmazonS3SecretAccessKey == model.FAKE_SETTING {
-		cfg.FileSettings.AmazonS3SecretAccessKey = actual.FileSettings.AmazonS3SecretAccessKey
-	}
-
-	if *cfg.EmailSettings.InviteSalt == model.FAKE_SETTING {
-		cfg.EmailSettings.InviteSalt = actual.EmailSettings.InviteSalt
-	}
-	if *cfg.EmailSettings.SMTPPassword == model.FAKE_SETTING {
-		cfg.EmailSettings.SMTPPassword = actual.EmailSettings.SMTPPassword
-	}
-
-	if *cfg.GitLabSettings.Secret == model.FAKE_SETTING {
-		*cfg.GitLabSettings.Secret = *actual.GitLabSettings.Secret
-	}
-
-	if *cfg.SqlSettings.DataSource == model.FAKE_SETTING {
-		*cfg.SqlSettings.DataSource = *actual.SqlSettings.DataSource
-	}
-	if *cfg.SqlSettings.AtRestEncryptKey == model.FAKE_SETTING {
-		cfg.SqlSettings.AtRestEncryptKey = actual.SqlSettings.AtRestEncryptKey
-	}
-
-	if *cfg.ElasticsearchSettings.Password == model.FAKE_SETTING {
-		*cfg.ElasticsearchSettings.Password = *actual.ElasticsearchSettings.Password
-	}
-
-	for i := range cfg.SqlSettings.DataSourceReplicas {
-		cfg.SqlSettings.DataSourceReplicas[i] = actual.SqlSettings.DataSourceReplicas[i]
-	}
-
-	for i := range cfg.SqlSettings.DataSourceSearchReplicas {
-		cfg.SqlSettings.DataSourceSearchReplicas[i] = actual.SqlSettings.DataSourceSearchReplicas[i]
-	}
-}
-
 func (a *App) GetCookieDomain() string {
 	if *a.Config().ServiceSettings.AllowCookiesForSubdomains {
 		if siteURL, err := url.Parse(*a.Config().ServiceSettings.SiteURL); err == nil {
--- a/app/config_test.go
+++ b/app/config_test.go
@@ -4,67 +4,29 @@
 package app

 import (
-	"io/ioutil"
 	"strconv"
-	"strings"
 	"testing"
 	"time"

 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"

 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/store/sqlstore"
 	"github.com/mattermost/mattermost-server/utils"
-	"github.com/mattermost/mattermost-server/utils/fileutils"
 )

-func TestLoadConfig(t *testing.T) {
-	tempConfig, err := ioutil.TempFile("", "")
-	require.Nil(t, err)
-
-	input, err := ioutil.ReadFile(fileutils.FindConfigFile("config.json"))
-	require.Nil(t, err)
-	lines := strings.Split(string(input), "\n")
-	for i, line := range lines {
-		if strings.Contains(line, "SiteURL") {
-			lines[i] = `        "SiteURL": "http://localhost:8065/",`
-		}
-	}
-	output := strings.Join(lines, "\n")
-	err = ioutil.WriteFile(tempConfig.Name(), []byte(output), 0644)
-	require.Nil(t, err)
-	tempConfig.Close()
-
-	a := App{
-		Srv: &Server{},
-	}
-	appErr := a.LoadConfig(tempConfig.Name())
-	require.Nil(t, appErr)
-
-	assert.Equal(t, "http://localhost:8065", *a.Config().ServiceSettings.SiteURL)
-}
-
 func TestConfigListener(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()

 	originalSiteName := th.App.Config().TeamSettings.SiteName
-	th.App.UpdateConfig(func(cfg *model.Config) {
-		*cfg.TeamSettings.SiteName = "test123"
-	})

 	listenerCalled := false
 	listener := func(oldConfig *model.Config, newConfig *model.Config) {
-		if listenerCalled {
-			t.Fatal("listener called twice")
-		}
+		assert.False(t, listenerCalled, "listener called twice")

-		if *oldConfig.TeamSettings.SiteName != "test123" {
-			t.Fatal("old config contains incorrect site name")
-		} else if *newConfig.TeamSettings.SiteName != *originalSiteName {
-			t.Fatal("new config contains incorrect site name")
-		}
+		assert.Equal(t, *originalSiteName, *oldConfig.TeamSettings.SiteName, "old config contains incorrect site name")
+		assert.Equal(t, "test123", *newConfig.TeamSettings.SiteName, "new config contains incorrect site name")

 		listenerCalled = true
 	}
@@ -73,22 +35,19 @@ func TestConfigListener(t *testing.T) {

 	listener2Called := false
 	listener2 := func(oldConfig *model.Config, newConfig *model.Config) {
-		if listener2Called {
-			t.Fatal("listener2 called twice")
-		}
+		assert.False(t, listener2Called, "listener2 called twice")

 		listener2Called = true
 	}
 	listener2Id := th.App.AddConfigListener(listener2)
 	defer th.App.RemoveConfigListener(listener2Id)

-	th.App.ReloadConfig()
+	th.App.UpdateConfig(func(cfg *model.Config) {
+		*cfg.TeamSettings.SiteName = "test123"
+	})

-	if !listenerCalled {
-		t.Fatal("listener should've been called")
-	} else if !listener2Called {
-		t.Fatal("listener 2 should've been called")
-	}
+	assert.True(t, listenerCalled, "listener should've been called")
+	assert.True(t, listener2Called, "listener 2 should've been called")
 }

 func TestAsymmetricSigningKey(t *testing.T) {
--- a/app/emoji.go
+++ b/app/emoji.go
@@ -31,6 +31,14 @@ const (
 )

 func (a *App) CreateEmoji(sessionUserId string, emoji *model.Emoji, multiPartImageData *multipart.Form) (*model.Emoji, *model.AppError) {
+	if !*a.Config().ServiceSettings.EnableCustomEmoji {
+		return nil, model.NewAppError("UploadEmojiImage", "api.emoji.disabled.app_error", nil, "", http.StatusNotImplemented)
+	}
+
+	if len(*a.Config().FileSettings.DriverName) == 0 {
+		return nil, model.NewAppError("GetEmoji", "api.emoji.storage.app_error", nil, "", http.StatusNotImplemented)
+	}
+
 	// wipe the emoji id so that existing emojis can't get overwritten
 	emoji.Id = ""

@@ -79,6 +87,14 @@ func (a *App) GetEmojiList(page, perPage int, sort string) ([]*model.Emoji, *mod
 }

 func (a *App) UploadEmojiImage(id string, imageData *multipart.FileHeader) *model.AppError {
+	if !*a.Config().ServiceSettings.EnableCustomEmoji {
+		return model.NewAppError("UploadEmojiImage", "api.emoji.disabled.app_error", nil, "", http.StatusNotImplemented)
+	}
+
+	if len(*a.Config().FileSettings.DriverName) == 0 {
+		return model.NewAppError("UploadEmojiImage", "api.emoji.storage.app_error", nil, "", http.StatusNotImplemented)
+	}
+
 	file, err := imageData.Open()
 	if err != nil {
 		return model.NewAppError("uploadEmojiImage", "api.emoji.upload.open.app_error", nil, "", http.StatusBadRequest)
--- a/app/enterprise.go
+++ b/app/enterprise.go
@@ -8,7 +8,6 @@ import (
 	ejobs "github.com/mattermost/mattermost-server/einterfaces/jobs"
 	tjobs "github.com/mattermost/mattermost-server/jobs/interfaces"
 	"github.com/mattermost/mattermost-server/model"
-	"github.com/mattermost/mattermost-server/utils"
 )

 var accountMigrationInterface func(*App) einterfaces.AccountMigrationInterface
@@ -119,11 +118,6 @@ func (s *Server) initEnterprise() {
 	}
 	if ldapInterface != nil {
 		s.Ldap = ldapInterface(s.FakeApp())
-		s.AddConfigListener(func(_, cfg *model.Config) {
-			if err := validateLdapFilter(cfg, s.Ldap); err != nil {
-				panic(utils.T(err.Id))
-			}
-		})
 	}
 	if messageExportInterface != nil {
 		s.MessageExport = messageExportInterface(s.FakeApp())
--- a/app/options.go
+++ b/app/options.go
@@ -6,6 +6,7 @@ package app
 import (
 	"github.com/pkg/errors"

+	"github.com/mattermost/mattermost-server/config"
 	"github.com/mattermost/mattermost-server/store"
 )

@@ -38,8 +39,19 @@ func StoreOverride(override interface{}) Option {

 func ConfigFile(file string, watch bool) Option {
 	return func(s *Server) error {
-		s.configFile = file
-		s.disableConfigWatch = !watch
+		configStore, err := config.NewFileStore(file, watch)
+		if err != nil {
+			return errors.Wrap(err, "failed to apply ConfigFile option")
+		}
+
+		s.configStore = configStore
+		return nil
+	}
+}
+
+func ConfigStore(configStore config.Store) Option {
+	return func(s *Server) error {
+		s.configStore = configStore

 		return nil
 	}
--- a/app/post_test.go
+++ b/app/post_test.go
@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"net/http"
 	"sync"
-	"sync/atomic"
 	"testing"
 	"time"

@@ -533,8 +532,7 @@ func TestMaxPostSize(t *testing.T) {

 			app := App{
 				Srv: &Server{
-					Store:  mockStore,
-					config: atomic.Value{},
+					Store: mockStore,
 				},
 			}

--- a/app/server.go
+++ b/app/server.go
@@ -35,7 +35,6 @@ import (
 	"github.com/mattermost/mattermost-server/services/timezones"
 	"github.com/mattermost/mattermost-server/store"
 	"github.com/mattermost/mattermost-server/utils"
-	"github.com/mattermost/mattermost-server/utils/fileutils"
 )

 var MaxNotificationsPerChannelDefault int64 = 1000000
@@ -75,10 +74,6 @@ type Server struct {
 	runjobs bool
 	Jobs    *jobs.JobServer

-	config                 atomic.Value
-	envConfig              map[string]interface{}
-	configFile             string
-	configListeners        map[string]func(*model.Config, *model.Config)
 	clusterLeaderListeners sync.Map

 	licenseValue       atomic.Value
@@ -96,8 +91,7 @@ type Server struct {
 	licenseListenerId       string
 	logListenerId           string
 	clusterLeaderListenerId string
-	disableConfigWatch      bool
-	configWatcher           *config.ConfigWatcher
+	configStore             config.Store
 	asymmetricSigningKey    *ecdsa.PrivateKey

 	pluginCommands     []*PluginCommand
@@ -137,8 +131,6 @@ func NewServer(options ...Option) (*Server, error) {
 	s := &Server{
 		goroutineExitSignal:     make(chan struct{}, 1),
 		RootRouter:              rootRouter,
-		configFile:              "config.json",
-		configListeners:         make(map[string]func(*model.Config, *model.Config)),
 		licenseListeners:        map[string]func(){},
 		sessionCache:            utils.NewLru(model.SESSION_CACHE_SIZE),
 		seenPendingPostIdsCache: utils.NewLru(PENDING_POST_IDS_CACHE_SIZE),
@@ -150,11 +142,14 @@ func NewServer(options ...Option) (*Server, error) {
 		}
 	}

-	if err := s.LoadConfig(s.configFile); err != nil {
-		return nil, err
-	}
+	if s.configStore == nil {
+		configStore, err := config.NewFileStore("config.json", true)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to load config")
+		}

-	s.EnableConfigWatch()
+		s.configStore = configStore
+	}

 	// Initalize logging
 	s.Log = mlog.NewLogger(utils.MloggerConfigFromLoggerConfig(&s.Config().LogSettings))
@@ -198,7 +193,7 @@ func NewServer(options ...Option) (*Server, error) {
 	mlog.Info(fmt.Sprintf("Enterprise Enabled: %v", model.BuildEnterpriseReady))
 	pwd, _ := os.Getwd()
 	mlog.Info(fmt.Sprintf("Current working directory is %v", pwd))
-	mlog.Info(fmt.Sprintf("Loaded config file from %v", fileutils.FindConfigFile(s.configFile)))
+	mlog.Info("Loaded config", mlog.String("source", s.configStore.String()))

 	license := s.License()

@@ -323,7 +318,7 @@ func (s *Server) Shutdown() error {
 	s.RemoveConfigListener(s.configListenerId)
 	s.RemoveConfigListener(s.logListenerId)

-	s.DisableConfigWatch()
+	s.configStore.Close()

 	if s.Cluster != nil {
 		s.Cluster.StopInterNodeCommunication()
--- a/app/server_test.go
+++ b/app/server_test.go
@@ -12,6 +12,7 @@ import (
 	"strings"
 	"testing"

+	"github.com/mattermost/mattermost-server/config"
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/utils/fileutils"
 	"github.com/stretchr/testify/require"
@@ -32,14 +33,14 @@ func TestStartServerSuccess(t *testing.T) {
 }

 func TestStartServerRateLimiterCriticalError(t *testing.T) {
-	s, err := NewServer()
-	require.NoError(t, err)
-
 	// Attempt to use Rate Limiter with an invalid config
-	s.UpdateConfig(func(cfg *model.Config) {
-		*cfg.RateLimitSettings.Enable = true
-		*cfg.RateLimitSettings.MaxBurst = -100
-	})
+	ms, err := config.NewMemoryStore(true)
+	require.NoError(t, err)
+	*ms.Config.RateLimitSettings.Enable = true
+	*ms.Config.RateLimitSettings.MaxBurst = -100
+
+	s, err := NewServer(ConfigStore(ms))
+	require.NoError(t, err)

 	serverErr := s.Start()
 	s.Shutdown()