diff --git a/Makefile b/Makefile index 0a18580077..10b3f8f32d 100644 --- 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 index 31f2a1d379..cacdaa3d41 100644 --- 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 index b1dc511619..75af85c9e8 100644 --- 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 index ffd21df768..ac0f9d5b38 100644 --- 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 index 0534714fb3..00b7217eb0 100644 --- 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 b/app/featureflag/feature_flags_sync.go similarity index 82% rename from config/feature_flags.go rename to app/featureflag/feature_flags_sync.go index 4d8b1e13f8..0f1ad5c08b 100644 --- a/config/feature_flags.go +++ b/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,7 +17,7 @@ import ( "github.com/mattermost/mattermost-server/v5/shared/mlog" ) -type FeatureFlagSyncParams struct { +type SyncParams struct { ServerID string SplitKey string SyncIntervalSeconds int @@ -25,8 +25,8 @@ type FeatureFlagSyncParams struct { 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,16 +47,16 @@ 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") } @@ -64,13 +64,13 @@ func (f *FeatureFlagSynchronizer) EnsureReady() error { 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 b/app/featureflag/feature_flags_sync_test.go similarity index 99% rename from config/feature_flags_test.go rename to app/featureflag/feature_flags_sync_test.go index cb1cf50471..1394be7ab9 100644 --- a/config/feature_flags_test.go +++ b/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 b/app/featureflag/split_logger.go similarity index 97% rename from config/split_logger.go rename to app/featureflag/split_logger.go index 1025f4ba05..94040cc4f9 100644 --- a/config/split_logger.go +++ b/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 index 4ce8c78ea4..7028cd61fd 100644 --- 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 index d3e44db352..319c17dff7 100644 --- 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 index 3b857b2e34..1530e75c66 100644 --- 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 index 99c4e97271..dfe1cc4006 100644 --- 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 index 0e8e8677ef..03b8bf25c0 100644 --- 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 index 511d996308..cc8a9ba126 100644 --- 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 index abfc4ef7b8..ce4e538783 100644 --- 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") } diff --git a/config/client_test.go b/config/client_test.go index 31d8d733e8..1a9abe6212 100644 --- a/config/client_test.go +++ b/config/client_test.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config_test +package config import ( "fmt" @@ -9,7 +9,6 @@ import ( "github.com/stretchr/testify/assert" - "github.com/mattermost/mattermost-server/v5/config" "github.com/mattermost/mattermost-server/v5/model" ) @@ -26,16 +25,16 @@ func TestGetClientConfig(t *testing.T) { "unlicensed", &model.Config{ EmailSettings: model.EmailSettings{ - EmailNotificationContentsType: sToP(model.EMAIL_NOTIFICATION_CONTENTS_FULL), + EmailNotificationContentsType: model.NewString(model.EMAIL_NOTIFICATION_CONTENTS_FULL), }, ThemeSettings: model.ThemeSettings{ // Ignored, since not licensed. - AllowCustomThemes: bToP(false), + AllowCustomThemes: model.NewBool(false), }, ServiceSettings: model.ServiceSettings{ - WebsocketURL: sToP("ws://mattermost.example.com:8065"), - WebsocketPort: iToP(80), - WebsocketSecurePort: iToP(443), + WebsocketURL: model.NewString("ws://mattermost.example.com:8065"), + WebsocketPort: model.NewInt(80), + WebsocketSecurePort: model.NewInt(443), }, }, "", @@ -54,17 +53,17 @@ func TestGetClientConfig(t *testing.T) { "licensed, but not for theme management", &model.Config{ EmailSettings: model.EmailSettings{ - EmailNotificationContentsType: sToP(model.EMAIL_NOTIFICATION_CONTENTS_FULL), + EmailNotificationContentsType: model.NewString(model.EMAIL_NOTIFICATION_CONTENTS_FULL), }, ThemeSettings: model.ThemeSettings{ // Ignored, since not licensed. - AllowCustomThemes: bToP(false), + AllowCustomThemes: model.NewBool(false), }, }, "tag1", &model.License{ Features: &model.Features{ - ThemeManagement: bToP(false), + ThemeManagement: model.NewBool(false), }, }, map[string]string{ @@ -77,16 +76,16 @@ func TestGetClientConfig(t *testing.T) { "licensed for theme management", &model.Config{ EmailSettings: model.EmailSettings{ - EmailNotificationContentsType: sToP(model.EMAIL_NOTIFICATION_CONTENTS_FULL), + EmailNotificationContentsType: model.NewString(model.EMAIL_NOTIFICATION_CONTENTS_FULL), }, ThemeSettings: model.ThemeSettings{ - AllowCustomThemes: bToP(false), + AllowCustomThemes: model.NewBool(false), }, }, "tag2", &model.License{ Features: &model.Features{ - ThemeManagement: bToP(true), + ThemeManagement: model.NewBool(true), }, }, map[string]string{ @@ -99,13 +98,13 @@ func TestGetClientConfig(t *testing.T) { "licensed for enforcement", &model.Config{ ServiceSettings: model.ServiceSettings{ - EnforceMultifactorAuthentication: bToP(true), + EnforceMultifactorAuthentication: model.NewBool(true), }, }, "tag1", &model.License{ Features: &model.Features{ - MFA: bToP(true), + MFA: model.NewBool(true), }, }, map[string]string{ @@ -116,7 +115,7 @@ func TestGetClientConfig(t *testing.T) { "experimental channel organization enabled", &model.Config{ ServiceSettings: model.ServiceSettings{ - ExperimentalChannelOrganization: bToP(true), + ExperimentalChannelOrganization: model.NewBool(true), }, }, "tag1", @@ -129,8 +128,8 @@ func TestGetClientConfig(t *testing.T) { "experimental channel organization disabled, but experimental group unread channels on", &model.Config{ ServiceSettings: model.ServiceSettings{ - ExperimentalChannelOrganization: bToP(false), - ExperimentalGroupUnreadChannels: sToP(model.GROUP_UNREAD_CHANNELS_DEFAULT_ON), + ExperimentalChannelOrganization: model.NewBool(false), + ExperimentalGroupUnreadChannels: model.NewString(model.GROUP_UNREAD_CHANNELS_DEFAULT_ON), }, }, "tag1", @@ -143,7 +142,7 @@ func TestGetClientConfig(t *testing.T) { "default marketplace", &model.Config{ PluginSettings: model.PluginSettings{ - MarketplaceUrl: sToP(model.PLUGIN_SETTINGS_DEFAULT_MARKETPLACE_URL), + MarketplaceUrl: model.NewString(model.PLUGIN_SETTINGS_DEFAULT_MARKETPLACE_URL), }, }, "tag1", @@ -156,7 +155,7 @@ func TestGetClientConfig(t *testing.T) { "non-default marketplace", &model.Config{ PluginSettings: model.PluginSettings{ - MarketplaceUrl: sToP("http://example.com"), + MarketplaceUrl: model.NewString("http://example.com"), }, }, "tag1", @@ -169,7 +168,7 @@ func TestGetClientConfig(t *testing.T) { "enable ShowFullName prop", &model.Config{ PrivacySettings: model.PrivacySettings{ - ShowFullName: bToP(true), + ShowFullName: model.NewBool(true), }, }, "tag1", @@ -190,7 +189,7 @@ func TestGetClientConfig(t *testing.T) { testCase.license.Features.SetDefaults() } - configMap := config.GenerateClientConfig(testCase.config, testCase.telemetryID, testCase.license) + configMap := GenerateClientConfig(testCase.config, testCase.telemetryID, testCase.license) for expectedField, expectedValue := range testCase.expectedFields { actualValue, ok := configMap[expectedField] if assert.True(t, ok, fmt.Sprintf("config does not contain %v", expectedField)) { @@ -214,16 +213,16 @@ func TestGetLimitedClientConfig(t *testing.T) { "unlicensed", &model.Config{ EmailSettings: model.EmailSettings{ - EmailNotificationContentsType: sToP(model.EMAIL_NOTIFICATION_CONTENTS_FULL), + EmailNotificationContentsType: model.NewString(model.EMAIL_NOTIFICATION_CONTENTS_FULL), }, ThemeSettings: model.ThemeSettings{ // Ignored, since not licensed. - AllowCustomThemes: bToP(false), + AllowCustomThemes: model.NewBool(false), }, ServiceSettings: model.ServiceSettings{ - WebsocketURL: sToP("ws://mattermost.example.com:8065"), - WebsocketPort: iToP(80), - WebsocketSecurePort: iToP(443), + WebsocketURL: model.NewString("ws://mattermost.example.com:8065"), + WebsocketPort: model.NewInt(80), + WebsocketSecurePort: model.NewInt(443), }, }, "", @@ -240,11 +239,11 @@ func TestGetLimitedClientConfig(t *testing.T) { "password settings", &model.Config{ PasswordSettings: model.PasswordSettings{ - MinimumLength: iToP(15), - Lowercase: bToP(true), - Uppercase: bToP(true), - Number: bToP(true), - Symbol: bToP(false), + MinimumLength: model.NewInt(15), + Lowercase: model.NewBool(true), + Uppercase: model.NewBool(true), + Number: model.NewBool(true), + Symbol: model.NewBool(false), }, }, "", @@ -282,7 +281,7 @@ func TestGetLimitedClientConfig(t *testing.T) { testCase.license.Features.SetDefaults() } - configMap := config.GenerateLimitedClientConfig(testCase.config, testCase.telemetryID, testCase.license) + configMap := GenerateLimitedClientConfig(testCase.config, testCase.telemetryID, testCase.license) for expectedField, expectedValue := range testCase.expectedFields { actualValue, ok := configMap[expectedField] if assert.True(t, ok, fmt.Sprintf("config does not contain %v", expectedField)) { @@ -292,15 +291,3 @@ func TestGetLimitedClientConfig(t *testing.T) { }) } } - -func sToP(s string) *string { - return &s -} - -func bToP(b bool) *bool { - return &b -} - -func iToP(i int) *int { - return &i -} diff --git a/config/common_test.go b/config/common_test.go index d5be55252d..ba56cc311e 100644 --- a/config/common_test.go +++ b/config/common_test.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config_test +package config import ( "os" @@ -10,7 +10,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/mattermost/mattermost-server/v5/config" "github.com/mattermost/mattermost-server/v5/model" ) @@ -20,23 +19,23 @@ func init() { emptyConfig = &model.Config{} readOnlyConfig = &model.Config{ ClusterSettings: model.ClusterSettings{ - Enable: bToP(true), - ReadOnlyConfig: bToP(true), + Enable: model.NewBool(true), + ReadOnlyConfig: model.NewBool(true), }, } minimalConfig = &model.Config{ ServiceSettings: model.ServiceSettings{ - SiteURL: sToP("http://minimal"), + SiteURL: model.NewString("http://minimal"), }, SqlSettings: model.SqlSettings{ - AtRestEncryptKey: sToP("abcdefghijklmnopqrstuvwxyz0123456789"), + AtRestEncryptKey: model.NewString("abcdefghijklmnopqrstuvwxyz0123456789"), }, FileSettings: model.FileSettings{ - PublicLinkSalt: sToP("abcdefghijklmnopqrstuvwxyz0123456789"), + PublicLinkSalt: model.NewString("abcdefghijklmnopqrstuvwxyz0123456789"), }, LocalizationSettings: model.LocalizationSettings{ - DefaultServerLocale: sToP("en"), - DefaultClientLocale: sToP("en"), + DefaultServerLocale: model.NewString("en"), + DefaultClientLocale: model.NewString("en"), }, } @@ -47,34 +46,34 @@ func init() { invalidConfig = &model.Config{ ServiceSettings: model.ServiceSettings{ - SiteURL: sToP("invalid"), + SiteURL: model.NewString("invalid"), }, } fixesRequiredConfig = &model.Config{ ServiceSettings: model.ServiceSettings{ - SiteURL: sToP("http://trailingslash/"), + SiteURL: model.NewString("http://trailingslash/"), }, SqlSettings: model.SqlSettings{ - AtRestEncryptKey: sToP("abcdefghijklmnopqrstuvwxyz0123456789"), + AtRestEncryptKey: model.NewString("abcdefghijklmnopqrstuvwxyz0123456789"), }, FileSettings: model.FileSettings{ - DriverName: sToP(model.IMAGE_DRIVER_LOCAL), - Directory: sToP("/path/to/directory"), - PublicLinkSalt: sToP("abcdefghijklmnopqrstuvwxyz0123456789"), + DriverName: model.NewString(model.IMAGE_DRIVER_LOCAL), + Directory: model.NewString("/path/to/directory"), + PublicLinkSalt: model.NewString("abcdefghijklmnopqrstuvwxyz0123456789"), }, LocalizationSettings: model.LocalizationSettings{ - DefaultServerLocale: sToP("garbage"), - DefaultClientLocale: sToP("garbage"), + DefaultServerLocale: model.NewString("garbage"), + DefaultClientLocale: model.NewString("garbage"), }, } ldapConfig = &model.Config{ LdapSettings: model.LdapSettings{ - BindPassword: sToP("password"), + BindPassword: model.NewString("password"), }, } testConfig = &model.Config{ ServiceSettings: model.ServiceSettings{ - SiteURL: sToP("http://TestStoreNew"), + SiteURL: model.NewString("http://TestStoreNew"), }, } customConfigDefaults = &model.Config{ @@ -94,7 +93,7 @@ func TestMergeConfigs(t *testing.T) { patch := &model.Config{} patch.SetDefaults() - merged, err := config.Merge(base, patch, nil) + merged, err := Merge(base, patch, nil) require.NoError(t, err) assert.Equal(t, patch, merged) @@ -104,7 +103,7 @@ func TestMergeConfigs(t *testing.T) { base.SetDefaults() patch := base.Clone() - merged, err := config.Merge(base, patch, nil) + merged, err := Merge(base, patch, nil) require.NoError(t, err) assert.Equal(t, base, merged) @@ -114,9 +113,9 @@ func TestMergeConfigs(t *testing.T) { base := &model.Config{} base.SetDefaults() patch := base.Clone() - patch.ServiceSettings.SiteURL = newString("http://newhost.ca") + patch.ServiceSettings.SiteURL = model.NewString("http://newhost.ca") - merged, err := config.Merge(base, patch, nil) + merged, err := Merge(base, patch, nil) require.NoError(t, err) assert.NotEqual(t, base, merged) @@ -126,14 +125,14 @@ func TestMergeConfigs(t *testing.T) { base := &model.Config{} base.SetDefaults() patch := &model.Config{} - patch.ServiceSettings.SiteURL = newString("http://newhost.ca") - patch.GoogleSettings.Enable = newBool(true) + patch.ServiceSettings.SiteURL = model.NewString("http://newhost.ca") + patch.GoogleSettings.Enable = model.NewBool(true) expected := base.Clone() - expected.ServiceSettings.SiteURL = newString("http://newhost.ca") - expected.GoogleSettings.Enable = newBool(true) + expected.ServiceSettings.SiteURL = model.NewString("http://newhost.ca") + expected.GoogleSettings.Enable = model.NewBool(true) - merged, err := config.Merge(base, patch, nil) + merged, err := Merge(base, patch, nil) require.NoError(t, err) assert.NotEqual(t, base, merged) @@ -143,12 +142,12 @@ func TestMergeConfigs(t *testing.T) { } func TestConfigEnvironmentOverrides(t *testing.T) { - memstore, err := config.NewMemoryStore() + memstore, err := NewMemoryStore() require.NoError(t, err) - base, err := config.NewStoreFromBacking(memstore, nil, false) + base, err := NewStoreFromBacking(memstore, nil, false) require.NoError(t, err) originalConfig := &model.Config{} - originalConfig.ServiceSettings.SiteURL = newString("http://notoverriden.ca") + originalConfig.ServiceSettings.SiteURL = model.NewString("http://notoverriden.ca") os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://overridden.ca") defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") @@ -172,15 +171,12 @@ func TestRemoveEnvironmentOverrides(t *testing.T) { os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://overridden.ca") defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") - memstore, err := config.NewMemoryStore() + memstore, err := NewMemoryStore() require.NoError(t, err) - base, err := config.NewStoreFromBacking(memstore, nil, false) + base, err := NewStoreFromBacking(memstore, nil, false) require.NoError(t, err) oldCfg := base.Get() assert.Equal(t, "http://overridden.ca", *oldCfg.ServiceSettings.SiteURL) newCfg := base.RemoveEnvironmentOverrides(oldCfg) assert.Equal(t, "", *newCfg.ServiceSettings.SiteURL) } - -func newBool(b bool) *bool { return &b } -func newString(s string) *string { return &s } diff --git a/config/config_generator/generator/generator.go b/config/config_generator/generator/generator.go deleted file mode 100644 index 9a531311eb..0000000000 --- a/config/config_generator/generator/generator.go +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package generator - -import ( - "encoding/json" - "os" - - "github.com/mattermost/mattermost-server/v5/model" -) - -// GenerateDefaultConfig writes default config to outputFile. -func GenerateDefaultConfig(outputFile *os.File) error { - defaultCfg := &model.Config{} - defaultCfg.SetDefaults() - if data, err := json.MarshalIndent(defaultCfg, "", " "); err != nil { - return err - } else if _, err := outputFile.Write(data); err != nil { - return err - } - return nil -} diff --git a/config/database_test.go b/config/database_test.go index 3326b882fc..ad0eb02d31 100644 --- a/config/database_test.go +++ b/config/database_test.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config_test +package config import ( "bytes" @@ -16,7 +16,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/mattermost/mattermost-server/v5/config" "github.com/mattermost/mattermost-server/v5/model" ) @@ -35,11 +34,11 @@ func setupConfigDatabase(t *testing.T, cfg *model.Config, files map[string][]byt os.Clearenv() truncateTables(t) - cfgData, err := config.MarshalConfig(cfg) + cfgData, err := marshalConfig(cfg) require.NoError(t, err) db := sqlx.NewDb(mainHelper.GetSQLStore().GetMaster().Db, *mainHelper.GetSQLSettings().DriverName) - err = config.InitializeConfigurationsTable(db) + err = initializeConfigurationsTable(db) require.NoError(t, err) id := model.NewId() @@ -115,14 +114,14 @@ func assertDatabaseNotEqualsConfig(t *testing.T, expectedCfg *model.Config) { assert.NotEqual(t, expectedCfg, actualCfg) } -func newTestDatabaseStore(customDefaults *model.Config) (*config.Store, error) { +func newTestDatabaseStore(customDefaults *model.Config) (*Store, error) { sqlSettings := mainHelper.GetSQLSettings() - dss, err := config.NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource)) + dss, err := NewDatabaseStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource)) if err != nil { return nil, err } - cStore, err := config.NewStoreFromBacking(dss, customDefaults, false) + cStore, err := NewStoreFromBacking(dss, customDefaults, false) if err != nil { return nil, err } @@ -211,20 +210,20 @@ func TestDatabaseStoreNew(t *testing.T) { }) t.Run("invalid url", func(t *testing.T) { - _, err := config.NewDatabaseStore("") + _, err := NewDatabaseStore("") require.Error(t, err) - _, err = config.NewDatabaseStore("mysql") + _, err = NewDatabaseStore("mysql") require.Error(t, err) }) t.Run("unsupported scheme", func(t *testing.T) { - _, err := config.NewDatabaseStore("invalid") + _, err := NewDatabaseStore("invalid") require.Error(t, err) }) t.Run("unsupported scheme with valid data source", func(t *testing.T) { - _, err := config.NewDatabaseStore(fmt.Sprintf("invalid://%s", *sqlSettings.DataSource)) + _, err := NewDatabaseStore(fmt.Sprintf("invalid://%s", *sqlSettings.DataSource)) require.Error(t, err) }) } @@ -452,7 +451,7 @@ func TestDatabaseStoreSet(t *testing.T) { defer ds.Close() newCfg := &model.Config{} - newCfg.LdapSettings.BindPassword = sToP(model.FAKE_SETTING) + newCfg.LdapSettings.BindPassword = model.NewString(model.FAKE_SETTING) _, err = ds.Set(newCfg) require.NoError(t, err) @@ -469,7 +468,7 @@ func TestDatabaseStoreSet(t *testing.T) { defer ds.Close() newCfg := &model.Config{} - newCfg.ServiceSettings.SiteURL = sToP("invalid") + newCfg.ServiceSettings.SiteURL = model.NewString("invalid") _, err = ds.Set(newCfg) if assert.Error(t, err) { @@ -508,7 +507,7 @@ func TestDatabaseStoreSet(t *testing.T) { newCfg := &model.Config{ ServiceSettings: model.ServiceSettings{ - SiteURL: sToP("http://new"), + SiteURL: model.NewString("http://new"), }, } @@ -528,7 +527,7 @@ func TestDatabaseStoreSet(t *testing.T) { newCfg := &model.Config{ ServiceSettings: model.ServiceSettings{ - SiteURL: sToP("http://new"), + SiteURL: model.NewString("http://new"), }, } @@ -554,7 +553,7 @@ func TestDatabaseStoreSet(t *testing.T) { _, err = db.Exec("DROP TABLE Configurations") require.NoError(t, err) - newCfg := &model.Config{} + newCfg := minimalConfig _, err = ds.Set(newCfg) require.Error(t, err) @@ -574,9 +573,9 @@ func TestDatabaseStoreSet(t *testing.T) { require.NoError(t, err) defer ds.Close() - longSiteURL := fmt.Sprintf("http://%s", strings.Repeat("a", config.MaxWriteLength)) + longSiteURL := fmt.Sprintf("http://%s", strings.Repeat("a", MaxWriteLength)) newCfg := emptyConfig.Clone() - newCfg.ServiceSettings.SiteURL = sToP(longSiteURL) + newCfg.ServiceSettings.SiteURL = model.NewString(longSiteURL) _, err = ds.Set(newCfg) require.Error(t, err) @@ -597,7 +596,7 @@ func TestDatabaseStoreSet(t *testing.T) { } ds.AddListener(callback) - newCfg := &model.Config{} + newCfg := minimalConfig _, err = ds.Set(newCfg) require.NoError(t, err) @@ -616,7 +615,7 @@ func TestDatabaseStoreSet(t *testing.T) { require.NoError(t, err) defer ds.Close() - ds.PersistFeatures(false) + ds.SetReadOnlyFF(true) _, err = ds.Set(minimalConfig) require.NoError(t, err) @@ -632,7 +631,7 @@ func TestDatabaseStoreSet(t *testing.T) { require.NoError(t, err) defer ds.Close() - ds.PersistFeatures(true) + ds.SetReadOnlyFF(false) _, err = ds.Set(minimalConfig) require.NoError(t, err) @@ -828,7 +827,7 @@ func TestDatabaseStoreLoad(t *testing.T) { require.NoError(t, err) defer ds.Close() - cfgData, err := config.MarshalConfig(invalidConfig) + cfgData, err := marshalConfig(invalidConfig) require.NoError(t, err) sqlSettings := mainHelper.GetSQLSettings() @@ -862,7 +861,7 @@ func TestDatabaseStoreLoad(t *testing.T) { assert.Equal(t, "http://trailingslash", *ds.Get().ServiceSettings.SiteURL) }) - t.Run("listeners notifed", func(t *testing.T) { + t.Run("listeners notifed on change", func(t *testing.T) { _, tearDown := setupConfigDatabase(t, emptyConfig, nil) defer tearDown() @@ -876,10 +875,16 @@ func TestDatabaseStoreLoad(t *testing.T) { } ds.AddListener(callback) + newCfg := minimalConfig.Clone() + dbStore, ok := ds.backingStore.(*DatabaseStore) + require.True(t, ok) + err = dbStore.persist(newCfg) + require.NoError(t, err) + err = ds.Load() require.NoError(t, err) - require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config loaded") + require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config changed on load") }) } @@ -950,7 +955,7 @@ func TestDatabaseSetFile(t *testing.T) { if *mainHelper.Settings.DriverName == "postgres" { t.Skip("No limit for postgres") } - longFile := bytes.Repeat([]byte("a"), config.MaxWriteLength) + longFile := bytes.Repeat([]byte("a"), MaxWriteLength) err := ds.SetFile("toolong", longFile) require.NoError(t, err) @@ -960,7 +965,7 @@ func TestDatabaseSetFile(t *testing.T) { if *mainHelper.Settings.DriverName == "postgres" { t.Skip("No limit for postgres") } - longFile := bytes.Repeat([]byte("a"), config.MaxWriteLength+1) + longFile := bytes.Repeat([]byte("a"), MaxWriteLength+1) err := ds.SetFile("toolong", longFile) if assert.Error(t, err) { diff --git a/config/default.go b/config/default.go deleted file mode 100644 index 576cde0a76..0000000000 --- a/config/default.go +++ /dev/null @@ -1,6 +0,0 @@ -//go:generate go run config_generator/main.go - -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package config diff --git a/config/emitter.go b/config/emitter.go index cd457a6bbf..a058a497fb 100644 --- a/config/emitter.go +++ b/config/emitter.go @@ -10,6 +10,9 @@ import ( "github.com/mattermost/mattermost-server/v5/shared/mlog" ) +// Listener is a callback function invoked when the configuration changes. +type Listener func(oldCfg, newCfg *model.Config) + // emitter enables threadsafe registration and broadcasting to configuration listeners type emitter struct { listeners sync.Map @@ -18,9 +21,7 @@ type emitter struct { // AddListener adds a callback function to invoke when the configuration is modified. func (e *emitter) AddListener(listener Listener) string { id := model.NewId() - e.listeners.Store(id, listener) - return id } @@ -34,7 +35,6 @@ func (e *emitter) invokeConfigListeners(oldCfg, newCfg *model.Config) { e.listeners.Range(func(key, value interface{}) bool { listener := value.(Listener) listener(oldCfg, newCfg) - return true }) } diff --git a/config/export_test.go b/config/export_test.go deleted file mode 100644 index d0a46eba35..0000000000 --- a/config/export_test.go +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package config - -import ( - "github.com/jmoiron/sqlx" - - "github.com/mattermost/mattermost-server/v5/model" -) - -// MarshalConfig exposes the internal marshalConfig to tests only. -func MarshalConfig(cfg *model.Config) ([]byte, error) { - return marshalConfig(cfg) -} - -// InitializeConfigurationsTable exposes the internal initializeConfigurationsTable to test only. -func InitializeConfigurationsTable(db *sqlx.DB) error { - return initializeConfigurationsTable(db) -} diff --git a/config/file_test.go b/config/file_test.go index f286935d54..e8f234b698 100644 --- a/config/file_test.go +++ b/config/file_test.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config_test +package config import ( "encoding/json" @@ -16,7 +16,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/mattermost/mattermost-server/v5/config" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/utils" ) @@ -36,7 +35,7 @@ func setupConfigFile(t *testing.T, cfg *model.Config) (string, func()) { f, err := ioutil.TempFile(tempDir, "setupConfigFile") require.NoError(t, err) - cfgData, err := config.MarshalConfig(cfg) + cfgData, err := marshalConfig(cfg) require.NoError(t, err) ioutil.WriteFile(f.Name(), cfgData, 0644) @@ -49,6 +48,19 @@ func setupConfigFile(t *testing.T, cfg *model.Config) (string, func()) { } } +func setupConfigFileStore(t *testing.T, cfg *model.Config) (*Store, func()) { + t.Helper() + path, tearDown := setupConfigFile(t, cfg) + fs, err := NewFileStore(path, false) + require.NoError(t, err) + configStore, err := NewStoreFromBacking(fs, nil, false) + require.NoError(t, err) + return configStore, func() { + tearDown() + configStore.Close() + } +} + // 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() @@ -64,7 +76,7 @@ func getActualFileConfig(t *testing.T, path string) *model.Config { return actualCfg } -// assertFileEqualsConfig verifies the on disk contents of the given path equal the given config. +// assertFileEqualsConfig verifies the on disk contents of the given path equal the given func assertFileEqualsConfig(t *testing.T, expectedCfg *model.Config, path string) { t.Helper() @@ -73,7 +85,7 @@ func assertFileEqualsConfig(t *testing.T, expectedCfg *model.Config, path string assert.Equal(t, expectedCfg, actualCfg) } -// assertFileNotEqualsConfig verifies the on disk contents of the given path does not equal the given config. +// assertFileNotEqualsConfig verifies the on disk contents of the given path does not equal the given func assertFileNotEqualsConfig(t *testing.T, expectedCfg *model.Config, path string) { t.Helper() @@ -89,9 +101,9 @@ func TestFileStoreNew(t *testing.T) { path, tearDown := setupConfigFile(t, testConfig) defer tearDown() - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, nil, false) + configStore, err := NewStoreFromBacking(fs, nil, false) require.NoError(t, err) defer configStore.Close() @@ -103,9 +115,9 @@ func TestFileStoreNew(t *testing.T) { path, tearDown := setupConfigFile(t, testConfig) defer tearDown() - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, customConfigDefaults, false) + configStore, err := NewStoreFromBacking(fs, customConfigDefaults, false) require.NoError(t, err) defer configStore.Close() @@ -122,9 +134,9 @@ func TestFileStoreNew(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfigNoFF) defer tearDown() - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, nil, false) + configStore, err := NewStoreFromBacking(fs, nil, false) require.NoError(t, err) defer configStore.Close() @@ -136,9 +148,9 @@ func TestFileStoreNew(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfigNoFF) defer tearDown() - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, customConfigDefaults, false) + configStore, err := NewStoreFromBacking(fs, customConfigDefaults, false) require.NoError(t, err) defer configStore.Close() @@ -158,9 +170,9 @@ func TestFileStoreNew(t *testing.T) { defer os.RemoveAll(tempDir) path := filepath.Join(tempDir, "does_not_exist") - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, nil, false) + configStore, err := NewStoreFromBacking(fs, nil, false) require.NoError(t, err) defer configStore.Close() @@ -177,9 +189,9 @@ func TestFileStoreNew(t *testing.T) { defer os.RemoveAll(tempDir) path := filepath.Join(tempDir, "does_not_exist") - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, customConfigDefaults, false) + configStore, err := NewStoreFromBacking(fs, customConfigDefaults, false) require.NoError(t, err) defer configStore.Close() @@ -196,9 +208,9 @@ func TestFileStoreNew(t *testing.T) { defer os.RemoveAll(tempDir) path := filepath.Join(tempDir, "does/not/exist") - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, nil, false) + configStore, err := NewStoreFromBacking(fs, nil, false) require.Nil(t, configStore) require.Error(t, err) }) @@ -213,14 +225,14 @@ func TestFileStoreNew(t *testing.T) { path := "TestFileStoreNew/a/b/c/config.json" - cfgData, err := config.MarshalConfig(testConfig) + cfgData, err := marshalConfig(testConfig) require.NoError(t, err) ioutil.WriteFile(path, cfgData, 0644) - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, nil, false) + configStore, err := NewStoreFromBacking(fs, nil, false) require.NoError(t, err) defer configStore.Close() @@ -237,9 +249,9 @@ func TestFileStoreNew(t *testing.T) { defer os.RemoveAll("config/TestFileStoreNew") path := "TestFileStoreNew/a/b/c/config.json" - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, nil, false) + configStore, err := NewStoreFromBacking(fs, nil, false) require.NoError(t, err) defer configStore.Close() @@ -249,15 +261,9 @@ func TestFileStoreNew(t *testing.T) { } func TestFileStoreGet(t *testing.T) { - path, tearDown := setupConfigFile(t, testConfig) + configStore, tearDown := setupConfigFileStore(t, testConfig) defer tearDown() - fs, err := config.NewFileStore(path, false) - require.NoError(t, err) - configStore, err := config.NewStoreFromBacking(fs, nil, false) - require.NoError(t, err) - defer configStore.Close() - cfg := configStore.Get() assert.Equal(t, "http://TestStoreNew", *cfg.ServiceSettings.SiteURL) @@ -267,7 +273,7 @@ func TestFileStoreGet(t *testing.T) { assert.True(t, cfg == cfg2, "Get() returned different configuration instances") newCfg := &model.Config{} - _, err = configStore.Set(newCfg) + _, err := configStore.Set(newCfg) require.NoError(t, err) assert.False(t, newCfg == cfg, "returned config should have been different from original") @@ -278,9 +284,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { path, tearDown := setupConfigFile(t, testConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -290,9 +296,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") - fsInner, err = config.NewFileStore(path, false) + fsInner, err = NewFileStore(path, false) require.NoError(t, err) - fs, err = config.NewStoreFromBacking(fsInner, nil, false) + fs, err = NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -304,9 +310,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { path, tearDown := setupConfigFile(t, testConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, customConfigDefaults, false) + fs, err := NewStoreFromBacking(fsInner, customConfigDefaults, false) require.NoError(t, err) defer fs.Close() @@ -316,9 +322,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") - fsInner, err = config.NewFileStore(path, false) + fsInner, err = NewFileStore(path, false) require.NoError(t, err) - fs, err = config.NewStoreFromBacking(fsInner, customConfigDefaults, false) + fs, err = NewStoreFromBacking(fsInner, customConfigDefaults, false) require.NoError(t, err) defer fs.Close() @@ -331,9 +337,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { path, tearDown := setupConfigFile(t, testConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -343,9 +349,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { os.Setenv("MM_PLUGINSETTINGS_ENABLEUPLOADS", "true") defer os.Unsetenv("MM_PLUGINSETTINGS_ENABLEUPLOADS") - fsInner, err = config.NewFileStore(path, false) + fsInner, err = NewFileStore(path, false) require.NoError(t, err) - fs, err = config.NewStoreFromBacking(fsInner, nil, false) + fs, err = NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -357,9 +363,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { path, tearDown := setupConfigFile(t, testConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -369,9 +375,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { os.Setenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM", "3000") defer os.Unsetenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM") - fsInner, err = config.NewFileStore(path, false) + fsInner, err = NewFileStore(path, false) require.NoError(t, err) - fs, err = config.NewStoreFromBacking(fsInner, nil, false) + fs, err = NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -383,9 +389,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { path, tearDown := setupConfigFile(t, testConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -395,9 +401,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { os.Setenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE", "123456") defer os.Unsetenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE") - fsInner, err = config.NewFileStore(path, false) + fsInner, err = NewFileStore(path, false) require.NoError(t, err) - fs, err = config.NewStoreFromBacking(fsInner, nil, false) + fs, err = NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -409,9 +415,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { path, tearDown := setupConfigFile(t, testConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -421,9 +427,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db") defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS") - fsInner, err = config.NewFileStore(path, false) + fsInner, err = NewFileStore(path, false) require.NoError(t, err) - fs, err = config.NewStoreFromBacking(fsInner, nil, false) + fs, err = NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -432,15 +438,12 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { }) t.Run("get override for a slice variable - three values", func(t *testing.T) { - // This should work, but Viper (or we) don't parse environment variables to turn strings with spaces into slices. - t.Skip("not implemented yet") - path, tearDown := setupConfigFile(t, testConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -450,9 +453,9 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db user:pwd@db2:5433/test-db2 user:pwd@db3:5434/test-db3") defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS") - fsInner, err = config.NewFileStore(path, false) + fsInner, err = NewFileStore(path, false) require.NoError(t, err) - fs, err = config.NewStoreFromBacking(fsInner, nil, false) + fs, err = NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -463,96 +466,74 @@ func TestFileStoreGetEnivironmentOverrides(t *testing.T) { func TestFileStoreSet(t *testing.T) { t.Run("defaults required", func(t *testing.T) { - path, tearDown := setupConfigFile(t, minimalConfig) + configStore, tearDown := setupConfigFileStore(t, minimalConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) - require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) - require.NoError(t, err) - defer fs.Close() - - oldCfg := fs.Get().Clone() - + oldCfg := configStore.Get().Clone() newCfg := &model.Config{} - retCfg, err := fs.Set(newCfg) + retCfg, err := configStore.Set(newCfg) require.NoError(t, err) require.Equal(t, oldCfg, retCfg) - assert.Equal(t, "", *fs.Get().ServiceSettings.SiteURL) + assert.Equal(t, "", *configStore.Get().ServiceSettings.SiteURL) }) t.Run("desanitization required", func(t *testing.T) { - path, tearDown := setupConfigFile(t, ldapConfig) + configStore, tearDown := setupConfigFileStore(t, ldapConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) - require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) - require.NoError(t, err) - defer fs.Close() - newCfg := &model.Config{} - newCfg.LdapSettings.BindPassword = sToP(model.FAKE_SETTING) + newCfg.LdapSettings.BindPassword = model.NewString(model.FAKE_SETTING) - _, err = fs.Set(newCfg) + _, err := configStore.Set(newCfg) require.NoError(t, err) - assert.Equal(t, "password", *fs.Get().LdapSettings.BindPassword) + assert.Equal(t, "password", *configStore.Get().LdapSettings.BindPassword) }) t.Run("invalid", func(t *testing.T) { - path, tearDown := setupConfigFile(t, emptyConfig) + configStore, tearDown := setupConfigFileStore(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) - require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) - require.NoError(t, err) - defer fs.Close() - newCfg := &model.Config{} - newCfg.ServiceSettings.SiteURL = sToP("invalid") + newCfg.ServiceSettings.SiteURL = model.NewString("invalid") - _, err = fs.Set(newCfg) + _, err := configStore.Set(newCfg) if assert.Error(t, err) { assert.EqualError(t, err, "new configuration is invalid: Config.IsValid: model.config.is_valid.site_url.app_error, ") } - assert.Equal(t, "", *fs.Get().ServiceSettings.SiteURL) + assert.Equal(t, "", *configStore.Get().ServiceSettings.SiteURL) }) t.Run("read-only", func(t *testing.T) { - path, tearDown := setupConfigFile(t, readOnlyConfig) + configStore, tearDown := setupConfigFileStore(t, readOnlyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) - require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) - require.NoError(t, err) - defer fs.Close() - - _, err = fs.Set(readOnlyConfig) + newReadOnlyConfig := readOnlyConfig.Clone() + newReadOnlyConfig.ServiceSettings = model.ServiceSettings{ + SiteURL: model.NewString("http://test"), + } + _, err := configStore.Set(newReadOnlyConfig) if assert.Error(t, err) { - assert.Equal(t, config.ErrReadOnlyConfiguration, errors.Cause(err)) + assert.Equal(t, ErrReadOnlyConfiguration, errors.Cause(err)) } - assert.Equal(t, "", *fs.Get().ServiceSettings.SiteURL) + assert.Equal(t, "", *configStore.Get().ServiceSettings.SiteURL) }) t.Run("persist failed", func(t *testing.T) { - t.Skip("skipping persistence test inside Set") path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() - os.Chmod(path, 0500) + fsInner.path = "" newCfg := &model.Config{} @@ -565,29 +546,82 @@ func TestFileStoreSet(t *testing.T) { }) t.Run("listeners notified", func(t *testing.T) { - path, tearDown := setupConfigFile(t, emptyConfig) + configStore, tearDown := setupConfigFileStore(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) - require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) - require.NoError(t, err) - defer fs.Close() - called := make(chan bool, 1) - callback := func(oldfg, newCfg *model.Config) { + callback := func(oldCfg, newCfg *model.Config) { + require.NotEqual(t, oldCfg, newCfg) called <- true } - fs.AddListener(callback) + configStore.AddListener(callback) - newCfg := &model.Config{} + newCfg := minimalConfig - _, err = fs.Set(newCfg) + _, err := configStore.Set(newCfg) require.NoError(t, err) require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config written") }) + t.Run("listeners notified, only env change", func(t *testing.T) { + configStore, tearDown := setupConfigFileStore(t, minimalConfig) + defer tearDown() + + called := make(chan bool, 1) + callback := func(oldCfg, newCfg *model.Config) { + require.NotEqual(t, oldCfg, newCfg) + expectedConfig := minimalConfig.Clone() + expectedConfig.ServiceSettings.SiteURL = model.NewString("http://override") + require.Equal(t, minimalConfig, oldCfg) + require.Equal(t, expectedConfig, newCfg) + called <- true + } + configStore.AddListener(callback) + + newCfg := minimalConfig + os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") + defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") + + _, err := configStore.Set(newCfg) + require.NoError(t, err) + + require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config changed") + }) + + t.Run("listeners notified, feature flags change only", func(t *testing.T) { + configStore, tearDown := setupConfigFileStore(t, minimalConfig) + defer tearDown() + + expectedOldConfig := minimalConfig.Clone() + var expectedNewConfig *model.Config + called := make(chan bool, 1) + callback := func(oldCfg, newCfg *model.Config) { + require.NotEqual(t, oldCfg, newCfg) + require.Equal(t, expectedOldConfig, oldCfg) + require.Equal(t, expectedNewConfig, newCfg) + called <- true + } + configStore.AddListener(callback) + + configStore.SetReadOnlyFF(true) + + expectedNewConfig = minimalConfig.Clone() + expectedNewConfig.FeatureFlags.TestFeature = "test" + _, err := configStore.Set(expectedNewConfig) + require.NoError(t, err) + + require.False(t, wasCalled(called, 5*time.Second)) + + configStore.SetReadOnlyFF(false) + + expectedNewConfig.FeatureFlags.TestFeature = "test2" + _, err = configStore.Set(expectedNewConfig) + require.NoError(t, err) + + require.True(t, wasCalled(called, 5*time.Second)) + }) + t.Run("watcher restarted", func(t *testing.T) { if testing.Short() { t.Skip("skipping watcher test in short mode") @@ -596,26 +630,26 @@ func TestFileStoreSet(t *testing.T) { path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, true) + fsInner, err := NewFileStore(path, true) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() - _, err = fs.Set(&model.Config{}) + _, err = fs.Set(minimalConfig) require.NoError(t, err) // Let the initial call to invokeConfigListeners finish. time.Sleep(1 * time.Second) called := make(chan bool, 1) - callback := func(oldfg, newCfg *model.Config) { + callback := func(oldCfg, newCfg *model.Config) { called <- true } fs.AddListener(callback) // Rewrite the config to the file on disk - cfgData, err := config.MarshalConfig(emptyConfig) + cfgData, err := marshalConfig(emptyConfig) require.NoError(t, err) ioutil.WriteFile(path, cfgData, 0644) @@ -628,9 +662,9 @@ func TestFileStoreLoad(t *testing.T) { path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -642,24 +676,18 @@ func TestFileStoreLoad(t *testing.T) { }) t.Run("honour environment", func(t *testing.T) { - path, tearDown := setupConfigFile(t, minimalConfig) + configStore, tearDown := setupConfigFileStore(t, minimalConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) - require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) - require.NoError(t, err) - defer fs.Close() - - assert.Equal(t, "http://minimal", *fs.Get().ServiceSettings.SiteURL) + assert.Equal(t, "http://minimal", *configStore.Get().ServiceSettings.SiteURL) os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") - err = fs.Load() + err := configStore.Load() require.NoError(t, err) - assert.Equal(t, "http://override", *fs.Get().ServiceSettings.SiteURL) - assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, fs.GetEnvironmentOverrides()) + assert.Equal(t, "http://override", *configStore.Get().ServiceSettings.SiteURL) + assert.Equal(t, map[string]interface{}{"ServiceSettings": map[string]interface{}{"SiteURL": true}}, configStore.GetEnvironmentOverrides()) }) t.Run("do not persist environment variables - string", func(t *testing.T) { @@ -669,9 +697,9 @@ func TestFileStoreLoad(t *testing.T) { os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://overridePersistEnvVariables") defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -694,9 +722,9 @@ func TestFileStoreLoad(t *testing.T) { os.Setenv("MM_PLUGINSETTINGS_ENABLEUPLOADS", "true") defer os.Unsetenv("MM_PLUGINSETTINGS_ENABLEUPLOADS") - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -719,9 +747,9 @@ func TestFileStoreLoad(t *testing.T) { os.Setenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM", "3000") defer os.Unsetenv("MM_TEAMSETTINGS_MAXUSERSPERTEAM") - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -744,9 +772,9 @@ func TestFileStoreLoad(t *testing.T) { os.Setenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE", "123456") defer os.Unsetenv("MM_SERVICESETTINGS_TLSSTRICTTRANSPORTMAXAGE") - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -769,9 +797,9 @@ func TestFileStoreLoad(t *testing.T) { os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db") defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS") - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -796,9 +824,9 @@ func TestFileStoreLoad(t *testing.T) { os.Setenv("MM_SQLSETTINGS_DATASOURCEREPLICAS", "user:pwd@db:5432/test-db") defer os.Unsetenv("MM_SQLSETTINGS_DATASOURCEREPLICAS") - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -818,13 +846,13 @@ func TestFileStoreLoad(t *testing.T) { path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() - cfgData, err := config.MarshalConfig(invalidConfig) + cfgData, err := marshalConfig(invalidConfig) require.NoError(t, err) ioutil.WriteFile(path, cfgData, 0644) @@ -835,13 +863,26 @@ func TestFileStoreLoad(t *testing.T) { } }) + t.Run("invalid environment value", func(t *testing.T) { + configStore, tearDown := setupConfigFileStore(t, emptyConfig) + defer tearDown() + + os.Setenv("MM_SERVICESETTINGS_SITEURL", "invalid_url") + defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") + + newCfg := minimalConfig + _, err := configStore.Set(newCfg) + require.Error(t, err) + require.EqualError(t, err, "new configuration is invalid: Config.IsValid: model.config.is_valid.site_url.app_error, ") + }) + t.Run("fixes required", func(t *testing.T) { path, tearDown := setupConfigFile(t, fixesRequiredConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -858,22 +899,70 @@ func TestFileStoreLoad(t *testing.T) { path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() called := make(chan bool, 1) - callback := func(oldfg, newCfg *model.Config) { + callback := func(oldCfg, newCfg *model.Config) { called <- true } fs.AddListener(callback) + cfgData, err := marshalConfig(minimalConfig) + require.NoError(t, err) + + err = ioutil.WriteFile(path, cfgData, 0644) + require.NoError(t, err) + err = fs.Load() require.NoError(t, err) - require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config loaded") + require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config changed on load") + }) + + t.Run("no change", func(t *testing.T) { + configStore, tearDown := setupConfigFileStore(t, testConfig) + defer tearDown() + + called := make(chan bool, 1) + callback := func(oldCfg, newCfg *model.Config) { + called <- true + } + configStore.AddListener(callback) + + err := configStore.Load() + require.NoError(t, err) + + require.False(t, wasCalled(called, 5*time.Second), "callback should not have been called if nothing changed") + }) + + t.Run("listeners notified, only env change", func(t *testing.T) { + configStore, tearDown := setupConfigFileStore(t, minimalConfig) + defer tearDown() + + time.Sleep(1 * time.Second) + + called := make(chan bool, 1) + callback := func(oldCfg, newCfg *model.Config) { + require.NotEqual(t, oldCfg, newCfg) + expectedConfig := minimalConfig.Clone() + expectedConfig.ServiceSettings.SiteURL = model.NewString("http://override") + require.Equal(t, minimalConfig, oldCfg) + require.Equal(t, expectedConfig, newCfg) + called <- true + } + configStore.AddListener(callback) + + os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") + defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") + + err := configStore.Load() + require.NoError(t, err) + + require.True(t, wasCalled(called, 5*time.Second), "callback should have been called when config changed") }) } @@ -885,9 +974,9 @@ func TestFileStoreWatcherEmitter(t *testing.T) { t.Run("disabled", func(t *testing.T) { path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, false) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -895,13 +984,13 @@ func TestFileStoreWatcherEmitter(t *testing.T) { time.Sleep(1 * time.Second) called := make(chan bool, 1) - callback := func(oldfg, newCfg *model.Config) { + callback := func(oldCfg, newCfg *model.Config) { called <- true } fs.AddListener(callback) // Rewrite the config to the file on disk - cfgData, err := config.MarshalConfig(emptyConfig) + cfgData, err := marshalConfig(emptyConfig) require.NoError(t, err) ioutil.WriteFile(path, cfgData, 0644) @@ -911,20 +1000,20 @@ func TestFileStoreWatcherEmitter(t *testing.T) { t.Run("enabled", func(t *testing.T) { path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, true) + fsInner, err := NewFileStore(path, true) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() called := make(chan bool, 1) - callback := func(oldfg, newCfg *model.Config) { + callback := func(oldCCfg, newCfg *model.Config) { called <- true } fs.AddListener(callback) // Rewrite the config to the file on disk - cfgData, err := config.MarshalConfig(minimalConfig) + cfgData, err := marshalConfig(minimalConfig) require.NoError(t, err) f, err := os.OpenFile(path, os.O_WRONLY, 0644) @@ -939,9 +1028,9 @@ func TestFileStoreWatcherEmitter(t *testing.T) { t.Run("no change", func(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) + fsInner, err := NewFileStore(path, true) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) + fs, err := NewStoreFromBacking(fsInner, nil, false) require.NoError(t, err) defer fs.Close() @@ -949,7 +1038,7 @@ func TestFileStoreWatcherEmitter(t *testing.T) { time.Sleep(1 * time.Second) called := make(chan bool, 1) - callback := func(oldfg, newCfg *model.Config) { + callback := func(oldCfg, newCfg *model.Config) { called <- true } fs.AddListener(callback) @@ -959,32 +1048,57 @@ func TestFileStoreWatcherEmitter(t *testing.T) { require.False(t, wasCalled(called, 1*time.Second), "callback should not have been called since no change has happened") }) + + t.Run("env only change", func(t *testing.T) { + path, tearDown := setupConfigFile(t, minimalConfig) + defer tearDown() + fsInner, err := NewFileStore(path, true) + require.NoError(t, err) + fs, err := NewStoreFromBacking(fsInner, nil, false) + require.NoError(t, err) + defer fs.Close() + + // Let the initial call to invokeConfigListeners finish. + time.Sleep(1 * time.Second) + + called := make(chan bool, 1) + callback := func(oldCfg, newCfg *model.Config) { + require.NotEqual(t, oldCfg, newCfg) + expectedConfig := minimalConfig.Clone() + expectedConfig.ServiceSettings.SiteURL = model.NewString("http://override") + require.Equal(t, minimalConfig, oldCfg) + require.Equal(t, expectedConfig, newCfg) + called <- true + } + fs.AddListener(callback) + + os.Setenv("MM_SERVICESETTINGS_SITEURL", "http://override") + defer os.Unsetenv("MM_SERVICESETTINGS_SITEURL") + _, err = fs.Set(minimalConfig) + require.NoError(t, err) + + require.True(t, wasCalled(called, 5*time.Second), "callback should have been called since no change has happened") + }) } func TestFileStoreSave(t *testing.T) { - path, tearDown := setupConfigFile(t, minimalConfig) + store, tearDown := setupConfigFileStore(t, minimalConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, false) - require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, false) - require.NoError(t, err) - defer fs.Close() - newCfg := &model.Config{ ServiceSettings: model.ServiceSettings{ - SiteURL: sToP("http://new"), + SiteURL: model.NewString("http://new"), }, } t.Run("set with automatic save", func(t *testing.T) { - _, err = fs.Set(newCfg) + _, err := store.Set(newCfg) require.NoError(t, err) - err = fs.Load() + err = store.Load() require.NoError(t, err) - assert.Equal(t, "http://new", *fs.Get().ServiceSettings.SiteURL) + assert.Equal(t, "http://new", *store.Get().ServiceSettings.SiteURL) }) } @@ -992,7 +1106,7 @@ func TestFileGetFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1054,7 +1168,7 @@ func TestFileSetFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1105,7 +1219,7 @@ func TestFileHasFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1118,7 +1232,7 @@ func TestFileHasFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1134,7 +1248,7 @@ func TestFileHasFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1157,7 +1271,7 @@ func TestFileHasFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1170,7 +1284,7 @@ func TestFileHasFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1189,7 +1303,7 @@ func TestFileRemoveFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1201,7 +1315,7 @@ func TestFileRemoveFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1223,7 +1337,7 @@ func TestFileRemoveFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1252,7 +1366,7 @@ func TestFileRemoveFile(t *testing.T) { path, tearDown := setupConfigFile(t, minimalConfig) defer tearDown() - fs, err := config.NewFileStore(path, true) + fs, err := NewFileStore(path, true) require.NoError(t, err) defer fs.Close() @@ -1274,7 +1388,7 @@ func TestFileStoreString(t *testing.T) { path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown() - fs, err := config.NewFileStore(path, false) + fs, err := NewFileStore(path, false) require.NoError(t, err) defer fs.Close() @@ -1295,21 +1409,58 @@ func wasCalled(c chan bool, duration time.Duration) bool { func TestFileStoreReadOnly(t *testing.T) { path, tearDown := setupConfigFile(t, emptyConfig) defer tearDown() - fsInner, err := config.NewFileStore(path, true) + fsInner, err := NewFileStore(path, true) require.NoError(t, err) - fs, err := config.NewStoreFromBacking(fsInner, nil, true) + fs, err := NewStoreFromBacking(fsInner, nil, true) require.NoError(t, err) defer fs.Close() called := make(chan bool, 1) - callback := func(oldfg, newCfg *model.Config) { + callback := func(oldCfg, newCfg *model.Config) { called <- true } fs.AddListener(callback) cfg, err := fs.Set(minimalConfig) require.Nil(t, cfg) - require.Equal(t, config.ErrReadOnlyStore, err) + require.Equal(t, ErrReadOnlyStore, err) require.False(t, wasCalled(called, 1*time.Second), "callback should not have been called since config is read-only") } + +func TestFileStoreSetReadOnlyFF(t *testing.T) { + t.Run("read only true", func(t *testing.T) { + store, tearDown := setupConfigFileStore(t, minimalConfig) + defer tearDown() + config := store.Get() + require.Equal(t, minimalConfig.FeatureFlags, config.FeatureFlags) + + newCfg := config.Clone() + newCfg.FeatureFlags.TestFeature = "test" + + // store has read-only FF by default. + _, err := store.Set(newCfg) + require.NoError(t, err) + + config = store.Get() + require.Equal(t, minimalConfig.FeatureFlags, config.FeatureFlags) + }) + + t.Run("read only false", func(t *testing.T) { + store, tearDown := setupConfigFileStore(t, minimalConfig) + defer tearDown() + config := store.Get() + require.Equal(t, minimalConfig.FeatureFlags, config.FeatureFlags) + + newCfg := config.Clone() + newCfg.FeatureFlags.TestFeature = "test" + + store.SetReadOnlyFF(false) + + _, err := store.Set(newCfg) + require.NoError(t, err) + + config = store.Get() + require.Equal(t, newCfg.FeatureFlags, config.FeatureFlags) + }) +} diff --git a/config/logging.go b/config/logging.go index 367ff95b29..dc79264e6a 100644 --- a/config/logging.go +++ b/config/logging.go @@ -4,7 +4,10 @@ package config import ( + "encoding/json" + "errors" "os" + "path/filepath" "strings" "sync" @@ -13,10 +16,6 @@ import ( type LogSrcListener func(old, new mlog.LogTargetCfg) -type FileGetter interface { - GetFile(name string) ([]byte, error) -} - // LogConfigSrc abstracts the Advanced Logging configuration so that implementations can // fetch from file, database, etc. type LogConfigSrc interface { @@ -24,7 +23,7 @@ type LogConfigSrc interface { Get() mlog.LogTargetCfg // Set updates the dsn specifying the source and reloads - Set(dsn string, fget FileGetter) (err error) + Set(dsn string, configStore *Store) (err error) // AddListener adds a callback function to invoke when the configuration is modified. AddListener(listener LogSrcListener) string @@ -38,13 +37,25 @@ type LogConfigSrc interface { // NewLogConfigSrc creates an advanced logging configuration source, backed by a // file, JSON string, or database. -func NewLogConfigSrc(dsn string, isJSON bool, fget FileGetter) (LogConfigSrc, error) { +func NewLogConfigSrc(dsn string, configStore *Store) (LogConfigSrc, error) { + if configStore == nil { + return nil, errors.New("configStore should not be nil") + } + dsn = strings.TrimSpace(dsn) - if isJSON { + if isJSONMap(dsn) { return newJSONSrc(dsn) } - return newFileSrc(dsn, fget) + + path := dsn + // If this is a file based config we need the full path so it can be watched. + if strings.HasPrefix(configStore.String(), "file://") && !filepath.IsAbs(dsn) { + configPath := strings.TrimPrefix(configStore.String(), "file://") + path = filepath.Join(filepath.Dir(configPath), dsn) + } + + return newFileSrc(path, configStore) } // jsonSrc @@ -68,8 +79,8 @@ func (src *jsonSrc) Get() mlog.LogTargetCfg { } // Set updates the JSON specifying the source and reloads -func (src *jsonSrc) Set(data string, _ FileGetter) error { - cfg, err := JSONToLogTargetCfg([]byte(data)) +func (src *jsonSrc) Set(data string, _ *Store) error { + cfg, err := logTargetCfgFromJSON([]byte(data)) if err != nil { return err } @@ -103,11 +114,11 @@ type fileSrc struct { watcher *watcher } -func newFileSrc(path string, fget FileGetter) (*fileSrc, error) { +func newFileSrc(path string, configStore *Store) (*fileSrc, error) { src := &fileSrc{ path: path, } - if err := src.Set(path, fget); err != nil { + if err := src.Set(path, configStore); err != nil { return nil, err } return src, nil @@ -123,13 +134,13 @@ func (src *fileSrc) Get() mlog.LogTargetCfg { // Set updates the dsn specifying the file source and reloads. // The file will be watched for changes and reloaded as needed, // and all listeners notified. -func (src *fileSrc) Set(path string, fget FileGetter) error { - data, err := fget.GetFile(path) +func (src *fileSrc) Set(path string, configStore *Store) error { + data, err := configStore.GetFile(path) if err != nil { return err } - cfg, err := JSONToLogTargetCfg(data) + cfg, err := logTargetCfgFromJSON(data) if err != nil { return err } @@ -153,7 +164,7 @@ func (src *fileSrc) Set(path string, fget FileGetter) error { } watcher, err := newWatcher(path, func() { - if serr := src.Set(path, fget); serr != nil { + if serr := src.Set(path, configStore); serr != nil { mlog.Error("Failed to reload file on change", mlog.String("path", path), mlog.Err(serr)) } }) @@ -186,3 +197,12 @@ func (src *fileSrc) Close() error { } return err } + +func logTargetCfgFromJSON(data []byte) (mlog.LogTargetCfg, error) { + cfg := make(mlog.LogTargetCfg) + err := json.Unmarshal(data, &cfg) + if err != nil { + return nil, err + } + return cfg, nil +} diff --git a/config/logging_test.go b/config/logging_test.go index bc6f41e5e6..50e90ec4a7 100644 --- a/config/logging_test.go +++ b/config/logging_test.go @@ -4,10 +4,10 @@ package config import ( - "os" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const ( @@ -15,38 +15,29 @@ const ( badJSON = `{"file":{ Type="file"}}` ) -type fgetFunc func(string) ([]byte, error) - -func (f fgetFunc) GetFile(path string) ([]byte, error) { - return f(path) -} - -func getValidFile(path string) ([]byte, error) { - return []byte(validJSON), nil -} - -func getInvalidFile(path string) ([]byte, error) { - return nil, os.ErrNotExist -} - func TestNewLogConfigSrc(t *testing.T) { + store := NewTestMemoryStore() + require.NotNil(t, store) + err := store.SetFile("advancedlogging.conf", []byte(validJSON)) + require.NoError(t, err) + tests := []struct { - name string - dsn string - fget FileGetter - wantErr bool - wantType LogConfigSrc + name string + dsn string + configStore *Store + wantErr bool + wantType LogConfigSrc }{ - {name: "empty dsn", dsn: "", fget: fgetFunc(getInvalidFile), wantErr: true, wantType: nil}, - {name: "garbage dsn", dsn: "!@wfejwcevioj", fget: fgetFunc(getInvalidFile), wantErr: true, wantType: nil}, - {name: "valid json dsn", dsn: validJSON, fget: fgetFunc(getInvalidFile), wantErr: false, wantType: &jsonSrc{}}, - {name: "invalid json dsn", dsn: badJSON, fget: fgetFunc(getInvalidFile), wantErr: true, wantType: nil}, - {name: "valid filespec dsn", dsn: "advancedlogging.conf", fget: fgetFunc(getValidFile), wantErr: false, wantType: &fileSrc{}}, - {name: "invalid filespec dsn", dsn: "/nobody/here.conf", fget: fgetFunc(getInvalidFile), wantErr: true, wantType: nil}, + {name: "empty dsn", dsn: "", configStore: store, wantErr: true, wantType: nil}, + {name: "garbage dsn", dsn: "!@wfejwcevioj", configStore: store, wantErr: true, wantType: nil}, + {name: "valid json dsn", dsn: validJSON, configStore: store, wantErr: false, wantType: &jsonSrc{}}, + {name: "invalid json dsn", dsn: badJSON, configStore: store, wantErr: true, wantType: nil}, + {name: "valid filespec dsn", dsn: "advancedlogging.conf", configStore: store, wantErr: false, wantType: &fileSrc{}}, + {name: "invalid filespec dsn", dsn: "/nobody/here.conf", configStore: store, wantErr: true, wantType: nil}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := NewLogConfigSrc(tt.dsn, IsJsonMap(tt.dsn), tt.fget) + got, err := NewLogConfigSrc(tt.dsn, tt.configStore) if tt.wantErr { assert.Error(t, err) } else { diff --git a/config/main_test.go b/config/main_test.go index 40a952712e..a36c5a3de0 100644 --- a/config/main_test.go +++ b/config/main_test.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config_test +package config import ( "fmt" diff --git a/config/memory_test.go b/config/memory_test.go index 38f7565ffc..4463d0094d 100644 --- a/config/memory_test.go +++ b/config/memory_test.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config_test +package config import ( "os" @@ -9,8 +9,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "github.com/mattermost/mattermost-server/v5/config" ) func setupConfigMemory(t *testing.T) { @@ -21,7 +19,7 @@ func setupConfigMemory(t *testing.T) { func TestMemoryGetFile(t *testing.T) { setupConfigMemory(t) - ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{ + ms, err := NewMemoryStoreWithOptions(&MemoryStoreOptions{ InitialConfig: minimalConfig, InitialFiles: map[string][]byte{ "empty-file": {}, @@ -57,7 +55,7 @@ func TestMemoryGetFile(t *testing.T) { func TestMemorySetFile(t *testing.T) { setupConfigMemory(t) - ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{ + ms, err := NewMemoryStoreWithOptions(&MemoryStoreOptions{ InitialConfig: minimalConfig, }) require.NoError(t, err) @@ -89,7 +87,7 @@ func TestMemoryHasFile(t *testing.T) { t.Run("has non-existent", func(t *testing.T) { setupConfigMemory(t) - ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{ + ms, err := NewMemoryStoreWithOptions(&MemoryStoreOptions{ InitialConfig: minimalConfig, }) require.NoError(t, err) @@ -103,7 +101,7 @@ func TestMemoryHasFile(t *testing.T) { t.Run("has existing", func(t *testing.T) { setupConfigMemory(t) - ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{ + ms, err := NewMemoryStoreWithOptions(&MemoryStoreOptions{ InitialConfig: minimalConfig, }) require.NoError(t, err) @@ -120,7 +118,7 @@ func TestMemoryHasFile(t *testing.T) { t.Run("has manually created file", func(t *testing.T) { setupConfigMemory(t) - ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{ + ms, err := NewMemoryStoreWithOptions(&MemoryStoreOptions{ InitialConfig: minimalConfig, InitialFiles: map[string][]byte{ "manual": []byte("manual file"), @@ -139,7 +137,7 @@ func TestMemoryRemoveFile(t *testing.T) { t.Run("remove non-existent", func(t *testing.T) { setupConfigMemory(t) - ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{ + ms, err := NewMemoryStoreWithOptions(&MemoryStoreOptions{ InitialConfig: minimalConfig, }) require.NoError(t, err) @@ -152,7 +150,7 @@ func TestMemoryRemoveFile(t *testing.T) { t.Run("remove existing", func(t *testing.T) { setupConfigMemory(t) - ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{ + ms, err := NewMemoryStoreWithOptions(&MemoryStoreOptions{ InitialConfig: minimalConfig, }) require.NoError(t, err) @@ -175,7 +173,7 @@ func TestMemoryRemoveFile(t *testing.T) { t.Run("remove manually created file", func(t *testing.T) { setupConfigMemory(t) - ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{ + ms, err := NewMemoryStoreWithOptions(&MemoryStoreOptions{ InitialConfig: minimalConfig, InitialFiles: map[string][]byte{ "manual": []byte("manual file"), @@ -199,7 +197,7 @@ func TestMemoryRemoveFile(t *testing.T) { func TestMemoryStoreString(t *testing.T) { setupConfigMemory(t) - ms, err := config.NewMemoryStoreWithOptions(&config.MemoryStoreOptions{InitialConfig: emptyConfig}) + ms, err := NewMemoryStoreWithOptions(&MemoryStoreOptions{InitialConfig: emptyConfig}) require.NoError(t, err) defer ms.Close() diff --git a/config/migrate.go b/config/migrate.go index fac71f72e2..5e53685465 100644 --- a/config/migrate.go +++ b/config/migrate.go @@ -9,13 +9,13 @@ import ( // Migrate migrates SAML keys, certificates, and other config files from one store to another given their data source names. func Migrate(from, to string) error { - source, err := NewStore(from, false, false, nil) + source, err := NewStoreFromDSN(from, false, false, nil) if err != nil { return errors.Wrapf(err, "failed to access source config %s", from) } defer source.Close() - destination, err := NewStore(to, false, false, nil) + destination, err := NewStoreFromDSN(to, false, false, nil) if err != nil { return errors.Wrapf(err, "failed to access destination config %s", to) } @@ -33,7 +33,7 @@ func Migrate(from, to string) error { } // Only migrate advanced logging config if it is not embedded JSON. - if !IsJsonMap(*sourceConfig.LogSettings.AdvancedLoggingConfig) { + if !isJSONMap(*sourceConfig.LogSettings.AdvancedLoggingConfig) { files = append(files, *sourceConfig.LogSettings.AdvancedLoggingConfig) } diff --git a/config/migrate_test.go b/config/migrate_test.go index 85e1525585..b36601efc9 100644 --- a/config/migrate_test.go +++ b/config/migrate_test.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config_test +package config import ( "io/ioutil" @@ -12,11 +12,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/mattermost/mattermost-server/v5/config" "github.com/mattermost/mattermost-server/v5/model" ) -type cleanUpFn func(store *config.Store) +type cleanUpFn func(store *Store) func TestMigrate(t *testing.T) { files := []string{ @@ -49,12 +48,12 @@ func TestMigrate(t *testing.T) { truncateTables(t) } - setupSource := func(t *testing.T, source *config.Store) cleanUpFn { + setupSource := func(t *testing.T, source *Store) cleanUpFn { t.Helper() cfg := source.Get() originalCfg := cfg.Clone() - cfg.ServiceSettings.SiteURL = sToP("http://example.com") + cfg.ServiceSettings.SiteURL = model.NewString("http://example.com") cfg.SamlSettings.IdpCertificateFile = &files[0] cfg.SamlSettings.PublicCertificateFile = &files[1] cfg.SamlSettings.PrivateKeyFile = &files[2] @@ -77,13 +76,13 @@ func TestMigrate(t *testing.T) { require.NoError(t, err) } - return func(store *config.Store) { + return func(store *Store) { _, err := store.Set(originalCfg) require.NoError(t, err) } } - assertDestination := func(t *testing.T, destination *config.Store, source *config.Store) { + assertDestination := func(t *testing.T, destination *Store, source *Store) { t.Helper() for i, file := range files { @@ -109,19 +108,19 @@ func TestMigrate(t *testing.T) { destinationDSN := path.Join(pwd, "config-custom.json") sourceDSN := getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource) - sourcedb, err := config.NewDatabaseStore(sourceDSN) + sourcedb, err := NewDatabaseStore(sourceDSN) require.NoError(t, err) - source, err := config.NewStoreFromBacking(sourcedb, nil, false) + source, err := NewStoreFromBacking(sourcedb, nil, false) require.NoError(t, err) defer source.Close() cleanUp := setupSource(t, source) - err = config.Migrate(sourceDSN, destinationDSN) + err = Migrate(sourceDSN, destinationDSN) require.NoError(t, err) - destinationfile, err := config.NewFileStore(destinationDSN, false) + destinationfile, err := NewFileStore(destinationDSN, false) require.NoError(t, err) - destination, err := config.NewStoreFromBacking(destinationfile, nil, false) + destination, err := NewStoreFromBacking(destinationfile, nil, false) require.NoError(t, err) defer destination.Close() defer cleanUp(destination) @@ -139,19 +138,19 @@ func TestMigrate(t *testing.T) { sourceDSN := path.Join(pwd, "config-custom.json") destinationDSN := getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource) - sourcefile, err := config.NewFileStore(sourceDSN, false) + sourcefile, err := NewFileStore(sourceDSN, false) require.NoError(t, err) - source, err := config.NewStoreFromBacking(sourcefile, nil, false) + source, err := NewStoreFromBacking(sourcefile, nil, false) require.NoError(t, err) defer source.Close() cleanUp := setupSource(t, source) - err = config.Migrate(sourceDSN, destinationDSN) + err = Migrate(sourceDSN, destinationDSN) require.NoError(t, err) - destinationdb, err := config.NewDatabaseStore(destinationDSN) + destinationdb, err := NewDatabaseStore(destinationDSN) require.NoError(t, err) - destination, err := config.NewStoreFromBacking(destinationdb, nil, false) + destination, err := NewStoreFromBacking(destinationdb, nil, false) require.NoError(t, err) defer destination.Close() defer cleanUp(destination) diff --git a/config/store.go b/config/store.go index d96db5ec45..3a96529a10 100644 --- a/config/store.go +++ b/config/store.go @@ -4,7 +4,6 @@ package config import ( - "bytes" "encoding/json" "reflect" "sync" @@ -21,9 +20,23 @@ var ( ErrReadOnlyStore = errors.New("configuration store is read-only") ) -// Listener is a callback function invoked when the configuration changes. -type Listener func(oldConfig *model.Config, newConfig *model.Config) +// Store is the higher level object that handles storing and retrieval of config data. +// To do so it relies on a variety of backing stores (e.g. file, database, memory). +type Store struct { + emitter + backingStore BackingStore + configLock sync.RWMutex + config *model.Config + configNoEnv *model.Config + configCustomDefaults *model.Config + + readOnly bool + readOnlyFF bool +} + +// BackingStore defines the behaviour exposed by the underlying store +// implementation (e.g. file, database). type BackingStore interface { // Set replaces the current configuration in its entirety and updates the backing store. Set(*model.Config) error @@ -54,27 +67,13 @@ type BackingStore interface { Close() error } -// NewStore creates a database or file store given a data source name by which to connect. -func NewStore(dsn string, watch, readOnly bool, customDefaults *model.Config) (*Store, error) { - backingStore, err := getBackingStore(dsn, watch) - if err != nil { - return nil, err - } - - store, err := NewStoreFromBacking(backingStore, customDefaults, readOnly) - if err != nil { - backingStore.Close() - return nil, errors.Wrap(err, "failed to create store") - } - - return store, nil -} - +// NewStoreFromBacking creates and returns a new config store given a backing store. func NewStoreFromBacking(backingStore BackingStore, customDefaults *model.Config, readOnly bool) (*Store, error) { store := &Store{ backingStore: backingStore, configCustomDefaults: customDefaults, readOnly: readOnly, + readOnlyFF: true, } if err := store.Load(); err != nil { @@ -90,14 +89,31 @@ func NewStoreFromBacking(backingStore BackingStore, customDefaults *model.Config return store, nil } -func getBackingStore(dsn string, watch bool) (BackingStore, error) { +// NewStoreFromDSN creates and returns a new config store backed by either a database or file store +// depending on the value of the given data source name string. +func NewStoreFromDSN(dsn string, watch, readOnly bool, customDefaults *model.Config) (*Store, error) { + var err error + var backingStore BackingStore if IsDatabaseDSN(dsn) { - return NewDatabaseStore(dsn) + backingStore, err = NewDatabaseStore(dsn) + } else { + backingStore, err = NewFileStore(dsn, watch) + } + if err != nil { + return nil, err } - return NewFileStore(dsn, watch) + store, err := NewStoreFromBacking(backingStore, customDefaults, readOnly) + if err != nil { + backingStore.Close() + return nil, errors.Wrap(err, "failed to create store") + } + + return store, nil } +// NewTestMemoryStore returns a new config store backed by a memory store +// to be used for testing purposes. func NewTestMemoryStore() *Store { memoryStore, err := NewMemoryStore() if err != nil { @@ -112,19 +128,6 @@ func NewTestMemoryStore() *Store { return configStore } -type Store struct { - emitter - backingStore BackingStore - - configLock sync.RWMutex - config *model.Config - configNoEnv *model.Config - configCustomDefaults *model.Config - - persistFeatureFlags bool - readOnly bool -} - // Get fetches the current, cached configuration. func (s *Store) Get() *model.Config { s.configLock.RLock() @@ -132,7 +135,7 @@ func (s *Store) Get() *model.Config { return s.config } -// Get fetches the current, cached configuration without environment variable overrides. +// GetNoEnv fetches the current cached configuration without environment variable overrides. func (s *Store) GetNoEnv() *model.Config { s.configLock.RLock() defer s.configLock.RUnlock() @@ -151,33 +154,37 @@ func (s *Store) GetEnvironmentOverridesWithFilter(filter func(reflect.StructFiel } // RemoveEnvironmentOverrides returns a new config without the environment -// overrides +// overrides. func (s *Store) RemoveEnvironmentOverrides(cfg *model.Config) *model.Config { s.configLock.RLock() defer s.configLock.RUnlock() return removeEnvOverrides(cfg, s.configNoEnv, s.GetEnvironmentOverrides()) } -// PersistFeatures sets if the store should persist feature flags. -func (s *Store) PersistFeatures(persist bool) { +// SetReadOnlyFF sets whether feature flags should be written out to +// config or treated as read-only. +func (s *Store) SetReadOnlyFF(readOnly bool) { s.configLock.Lock() defer s.configLock.Unlock() - s.persistFeatureFlags = persist + s.readOnlyFF = readOnly } // Set replaces the current configuration in its entirety and updates the backing store. func (s *Store) Set(newCfg *model.Config) (*model.Config, error) { s.configLock.Lock() - var unlockOnce sync.Once - defer unlockOnce.Do(s.configLock.Unlock) + defer s.configLock.Unlock() if s.readOnly { return nil, ErrReadOnlyStore } - oldCfg := s.config.Clone() + newCfg = newCfg.Clone() + // no need to clone these as cached configs are getting replaced + // with brand new objects. + oldCfg := s.config + oldCfgNoEnv := s.configNoEnv - // Really just for some tests we need to set defaults here + // Setting defaults allows us to accept partial config objects. newCfg.SetDefaults() // Sometimes the config is received with "fake" data in sensitive fields. Apply the real @@ -188,124 +195,159 @@ func (s *Store) Set(newCfg *model.Config) (*model.Config, error) { return nil, errors.Wrap(err, "new configuration is invalid") } - newCfg = removeEnvOverrides(newCfg, s.configNoEnv, s.GetEnvironmentOverrides()) + // We attempt to remove any environment override that may be present in the input config. + newCfgNoEnv := removeEnvOverrides(newCfg, oldCfgNoEnv, s.GetEnvironmentOverrides()) - // Don't persist feature flags unless we are on MM cloud + // Don't store feature flags unless we are on MM cloud // MM cloud uses config in the DB as a cache of the feature flag // settings in case the management system is down when a pod starts. - if !s.persistFeatureFlags { + + // Backing up feature flags section in case we need to restore them later on. + oldCfgFF := oldCfg.FeatureFlags + oldCfgNoEnvFF := oldCfgNoEnv.FeatureFlags + // Clearing FF sections to avoid both comparing and persisting them. + if s.readOnlyFF { + oldCfg.FeatureFlags = nil newCfg.FeatureFlags = nil + newCfgNoEnv.FeatureFlags = nil } - if err := s.backingStore.Set(newCfg); err != nil { + if err := s.backingStore.Set(newCfgNoEnv); err != nil { return nil, errors.Wrap(err, "failed to persist") } - if err := s.loadLockedWithOld(oldCfg, &unlockOnce); err != nil { - return nil, errors.Wrap(err, "failed to load on save") + // We apply back environment overrides since the input config may or + // may not have them applied. + newCfg = applyEnvironmentMap(newCfgNoEnv, GetEnvironment()) + fixConfig(newCfg) + if err := newCfg.IsValid(); err != nil { + return nil, errors.Wrap(err, "new configuration is invalid") + } + + hasChanged, err := equal(oldCfg, newCfg) + if err != nil { + return nil, errors.Wrap(err, "failed to compare configs") + } + + // We restore the previously cleared feature flags sections back. + if s.readOnlyFF { + oldCfg.FeatureFlags = oldCfgFF + newCfg.FeatureFlags = oldCfgFF + newCfgNoEnv.FeatureFlags = oldCfgNoEnvFF + } + + s.configNoEnv = newCfgNoEnv + s.config = newCfg + + if hasChanged { + s.configLock.Unlock() + s.invokeConfigListeners(oldCfg, newCfg.Clone()) + s.configLock.Lock() } return oldCfg, nil } -func (s *Store) loadLockedWithOld(oldCfg *model.Config, unlockOnce *sync.Once) error { +// Load updates the current configuration from the backing store, possibly initializing. +func (s *Store) Load() error { + s.configLock.Lock() + defer s.configLock.Unlock() + + oldCfg := &model.Config{} + if s.config != nil { + oldCfg = s.config + } + configBytes, err := s.backingStore.Load() if err != nil { return err } - loadedConfig := &model.Config{} + loadedCfg := &model.Config{} if len(configBytes) != 0 { - if err = json.Unmarshal(configBytes, &loadedConfig); err != nil { + if err = json.Unmarshal(configBytes, &loadedCfg); err != nil { return jsonutils.HumanizeJSONError(err, configBytes) } } - loadedFeatureFlags := loadedConfig.FeatureFlags - // If we have custom defaults set, the initial config is merged on // top of them and we delete them not to be used again in the // configuration reloads if s.configCustomDefaults != nil { var mErr error - loadedConfig, mErr = Merge(s.configCustomDefaults, loadedConfig, nil) + loadedCfg, mErr = Merge(s.configCustomDefaults, loadedCfg, nil) if mErr != nil { return errors.Wrap(mErr, "failed to merge custom config defaults") } s.configCustomDefaults = nil } - loadedConfig.SetDefaults() + // We set the SiteURL to empty (if nil) so that the following call to + // SetDefaults() will generate missing data. This avoids an additional write + // to the backing store. + if loadedCfg.ServiceSettings.SiteURL == nil { + loadedCfg.ServiceSettings.SiteURL = model.NewString("") + } - s.configNoEnv = loadedConfig.Clone() - fixConfig(s.configNoEnv) + // Setting defaults allows us to accept partial config objects. + loadedCfg.SetDefaults() - loadedConfig = applyEnvironmentMap(loadedConfig, GetEnvironment()) + // No need to clone here since the below call to applyEnvironmentMap + // already does that internally. + loadedCfgNoEnv := loadedCfg + fixConfig(loadedCfgNoEnv) - fixConfig(loadedConfig) - - if err := loadedConfig.IsValid(); err != nil { + loadedCfg = applyEnvironmentMap(loadedCfg, GetEnvironment()) + fixConfig(loadedCfg) + if err := loadedCfg.IsValid(); err != nil { return errors.Wrap(err, "invalid config") } - // Apply changes that may have happened on load to the backing store. - oldCfgBytes, err := json.Marshal(oldCfg) + // Backing up feature flags section in case we need to restore them later on. + oldCfgFF := oldCfg.FeatureFlags + loadedCfgFF := loadedCfg.FeatureFlags + loadedCfgNoEnvFF := loadedCfgNoEnv.FeatureFlags + // Clearing FF sections to avoid both comparing and persisting them. + if s.readOnlyFF { + oldCfg.FeatureFlags = nil + loadedCfg.FeatureFlags = nil + loadedCfgNoEnv.FeatureFlags = nil + } + + // Check for changes that may have happened on load to the backing store. + hasChanged, err := equal(oldCfg, loadedCfg) if err != nil { - return errors.Wrap(err, "failed to marshal old config") - } - newCfgBytes, err := json.Marshal(loadedConfig) - if err != nil { - return errors.Wrap(err, "failed to marshal loaded config") + return errors.Wrap(err, "failed to compare configs") } - var shouldStore bool - hasChanged := len(configBytes) == 0 || !bytes.Equal(oldCfgBytes, newCfgBytes) - if hasChanged { - featureFlags := s.configNoEnv.FeatureFlags - // Don't persist feature flags unless we are on MM cloud - // MM cloud uses config in the DB as a cache of the feature flag - // settings in case the management system is down when a pod starts. - if !s.persistFeatureFlags { - s.configNoEnv.FeatureFlags = loadedFeatureFlags - } - toStoreBytes, err := json.Marshal(s.configNoEnv) - if err != nil { - return errors.Wrap(err, "failed to marshal old config") - } - shouldStore = !bytes.Equal(toStoreBytes, configBytes) - // We write back to the backing store only if - // the config has changed and the store is not read-only. - if !s.readOnly && shouldStore { - err := s.backingStore.Set(s.configNoEnv) - s.configNoEnv.FeatureFlags = featureFlags - if err != nil && !errors.Is(err, ErrReadOnlyConfiguration) { - return errors.Wrap(err, "failed to persist") - } + // We write back to the backing store only if the store is not read-only + // and the config has either changed or is missing. + if !s.readOnly && (hasChanged || len(configBytes) == 0) { + err := s.backingStore.Set(loadedCfgNoEnv) + if err != nil && !errors.Is(err, ErrReadOnlyConfiguration) { + return errors.Wrap(err, "failed to persist") } } - s.config = loadedConfig + // We restore the previously cleared feature flags sections back. + if s.readOnlyFF { + oldCfg.FeatureFlags = oldCfgFF + loadedCfg.FeatureFlags = loadedCfgFF + loadedCfgNoEnv.FeatureFlags = loadedCfgNoEnvFF + } - unlockOnce.Do(s.configLock.Unlock) + s.config = loadedCfg + s.configNoEnv = loadedCfgNoEnv if hasChanged { - s.invokeConfigListeners(oldCfg, loadedConfig) + s.configLock.Unlock() + s.invokeConfigListeners(oldCfg, loadedCfg.Clone()) + s.configLock.Lock() } return nil } -// Load updates the current configuration from the backing store, possibly initializing. -func (s *Store) Load() error { - s.configLock.Lock() - var unlockOnce sync.Once - defer unlockOnce.Do(s.configLock.Unlock) - - oldCfg := s.config.Clone() - - return s.loadLockedWithOld(oldCfg, &unlockOnce) -} - // GetFile fetches the contents of a previously persisted configuration file. // If no such file exists, an empty byte array will be returned without error. func (s *Store) GetFile(name string) ([]byte, error) { diff --git a/config/store_test.go b/config/store_test.go index 998d097845..da5e20c751 100644 --- a/config/store_test.go +++ b/config/store_test.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config_test +package config import ( "io/ioutil" @@ -10,11 +10,9 @@ import ( "testing" "github.com/stretchr/testify/require" - - "github.com/mattermost/mattermost-server/v5/config" ) -func TestNewStore(t *testing.T) { +func TestNewStoreFromDSN(t *testing.T) { if testing.Short() { t.SkipNow() } @@ -29,25 +27,25 @@ func TestNewStore(t *testing.T) { require.NoError(t, os.Mkdir(filepath.Join(tempDir, "config"), 0700)) t.Run("database dsn", func(t *testing.T) { - ds, err := config.NewStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource), false, false, nil) + ds, err := NewStoreFromDSN(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource), false, false, nil) require.NoError(t, err) ds.Close() }) t.Run("database dsn, watch ignored", func(t *testing.T) { - ds, err := config.NewStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource), true, false, nil) + ds, err := NewStoreFromDSN(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource), true, false, nil) require.NoError(t, err) ds.Close() }) t.Run("file dsn", func(t *testing.T) { - fs, err := config.NewStore("config.json", false, false, nil) + fs, err := NewStoreFromDSN("config.json", false, false, nil) require.NoError(t, err) fs.Close() }) t.Run("file dsn, watch", func(t *testing.T) { - fs, err := config.NewStore("config.json", true, false, nil) + fs, err := NewStoreFromDSN("config.json", true, false, nil) require.NoError(t, err) fs.Close() }) @@ -68,46 +66,46 @@ func TestNewStoreReadOnly(t *testing.T) { require.NoError(t, os.Mkdir(filepath.Join(tempDir, "config"), 0700)) t.Run("database dsn", func(t *testing.T) { - ds, err := config.NewStore(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource), false, true, nil) + ds, err := NewStoreFromDSN(getDsn(*sqlSettings.DriverName, *sqlSettings.DataSource), false, true, nil) require.NoError(t, err) t.Run("Set", func(t *testing.T) { cfg, err := ds.Set(emptyConfig) require.Nil(t, cfg) - require.Equal(t, config.ErrReadOnlyStore, err) + require.Equal(t, ErrReadOnlyStore, err) }) t.Run("SetFile", func(t *testing.T) { err := ds.SetFile("config.json", []byte{}) - require.Equal(t, config.ErrReadOnlyStore, err) + require.Equal(t, ErrReadOnlyStore, err) }) t.Run("RemoveFile", func(t *testing.T) { err := ds.RemoveFile("config.json") - require.Equal(t, config.ErrReadOnlyStore, err) + require.Equal(t, ErrReadOnlyStore, err) }) ds.Close() }) t.Run("file dsn", func(t *testing.T) { - fs, err := config.NewStore("config.json", false, true, nil) + fs, err := NewStoreFromDSN("config.json", false, true, nil) require.NoError(t, err) t.Run("Set", func(t *testing.T) { cfg, err := fs.Set(emptyConfig) require.Nil(t, cfg) - require.Equal(t, config.ErrReadOnlyStore, err) + require.Equal(t, ErrReadOnlyStore, err) }) t.Run("SetFile", func(t *testing.T) { err := fs.SetFile("config.json", []byte{}) - require.Equal(t, config.ErrReadOnlyStore, err) + require.Equal(t, ErrReadOnlyStore, err) }) t.Run("RemoveFile", func(t *testing.T) { err := fs.RemoveFile("config.json") - require.Equal(t, config.ErrReadOnlyStore, err) + require.Equal(t, ErrReadOnlyStore, err) }) fs.Close() diff --git a/config/unmarshal.go b/config/unmarshal.go deleted file mode 100644 index e751e9ac57..0000000000 --- a/config/unmarshal.go +++ /dev/null @@ -1,15 +0,0 @@ -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - -package config - -import ( - "encoding/json" - - "github.com/mattermost/mattermost-server/v5/model" -) - -// marshalConfig converts the given configuration into JSON bytes for persistence. -func marshalConfig(cfg *model.Config) ([]byte, error) { - return json.MarshalIndent(cfg, "", " ") -} diff --git a/config/utils.go b/config/utils.go index dd5cb50f2d..e696dad26a 100644 --- a/config/utils.go +++ b/config/utils.go @@ -4,7 +4,9 @@ package config import ( + "bytes" "encoding/json" + "fmt" "reflect" "strings" @@ -14,6 +16,11 @@ import ( "github.com/mattermost/mattermost-server/v5/utils" ) +// marshalConfig converts the given configuration into JSON bytes for persistence. +func marshalConfig(cfg *model.Config) ([]byte, error) { + return json.MarshalIndent(cfg, "", " ") +} + // desanitize replaces fake settings with their actual values. func desanitize(actual, target *model.Config) { if target.LdapSettings.BindPassword != nil && *target.LdapSettings.BindPassword == model.FAKE_SETTING { @@ -191,20 +198,11 @@ func stripPassword(dsn, schema string) string { return prefix + dsn[:i+1] + dsn[j:] } -func IsJsonMap(data string) bool { +func isJSONMap(data string) bool { var m map[string]interface{} return json.Unmarshal([]byte(data), &m) == nil } -func JSONToLogTargetCfg(data []byte) (mlog.LogTargetCfg, error) { - cfg := make(mlog.LogTargetCfg) - err := json.Unmarshal(data, &cfg) - if err != nil { - return nil, err - } - return cfg, nil -} - func GetValueByPath(path []string, obj interface{}) (interface{}, bool) { r := reflect.ValueOf(obj) var val reflect.Value @@ -249,3 +247,15 @@ func GetValueByPath(path []string, obj interface{}) (interface{}, bool) { } return nil, false } + +func equal(oldCfg, newCfg *model.Config) (bool, error) { + oldCfgBytes, err := json.Marshal(oldCfg) + if err != nil { + return false, fmt.Errorf("failed to marshal old config: %w", err) + } + newCfgBytes, err := json.Marshal(newCfg) + if err != nil { + return false, fmt.Errorf("failed to marshal new config: %w", err) + } + return !bytes.Equal(oldCfgBytes, newCfgBytes), nil +} diff --git a/config/utils_test.go b/config/utils_test.go index c5d63eba21..c948339918 100644 --- a/config/utils_test.go +++ b/config/utils_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/utils" @@ -17,19 +18,19 @@ func TestDesanitize(t *testing.T) { actual.SetDefaults() // These setting should be ignored - actual.LdapSettings.Enable = bToP(false) - actual.FileSettings.DriverName = sToP("s3") + actual.LdapSettings.Enable = model.NewBool(false) + actual.FileSettings.DriverName = model.NewString("s3") // These settings should be desanitized into target. - actual.LdapSettings.BindPassword = sToP("bind_password") - actual.FileSettings.PublicLinkSalt = sToP("public_link_salt") - actual.FileSettings.AmazonS3SecretAccessKey = sToP("amazon_s3_secret_access_key") - actual.EmailSettings.SMTPPassword = sToP("smtp_password") - actual.GitLabSettings.Secret = sToP("secret") - actual.OpenIdSettings.Secret = sToP("secret") - actual.SqlSettings.DataSource = sToP("data_source") - actual.SqlSettings.AtRestEncryptKey = sToP("at_rest_encrypt_key") - actual.ElasticsearchSettings.Password = sToP("password") + actual.LdapSettings.BindPassword = model.NewString("bind_password") + actual.FileSettings.PublicLinkSalt = model.NewString("public_link_salt") + actual.FileSettings.AmazonS3SecretAccessKey = model.NewString("amazon_s3_secret_access_key") + actual.EmailSettings.SMTPPassword = model.NewString("smtp_password") + actual.GitLabSettings.Secret = model.NewString("secret") + actual.OpenIdSettings.Secret = model.NewString("secret") + actual.SqlSettings.DataSource = model.NewString("data_source") + actual.SqlSettings.AtRestEncryptKey = model.NewString("at_rest_encrypt_key") + actual.ElasticsearchSettings.Password = model.NewString("password") actual.SqlSettings.DataSourceReplicas = append(actual.SqlSettings.DataSourceReplicas, "replica0") actual.SqlSettings.DataSourceReplicas = append(actual.SqlSettings.DataSourceReplicas, "replica1") actual.SqlSettings.DataSourceSearchReplicas = append(actual.SqlSettings.DataSourceSearchReplicas, "search_replica0") @@ -39,19 +40,19 @@ func TestDesanitize(t *testing.T) { target.SetDefaults() // These setting should be ignored - target.LdapSettings.Enable = bToP(true) - target.FileSettings.DriverName = sToP("file") + target.LdapSettings.Enable = model.NewBool(true) + target.FileSettings.DriverName = model.NewString("file") // These settings should be updated from actual - target.LdapSettings.BindPassword = sToP(model.FAKE_SETTING) - target.FileSettings.PublicLinkSalt = sToP(model.FAKE_SETTING) - target.FileSettings.AmazonS3SecretAccessKey = sToP(model.FAKE_SETTING) - target.EmailSettings.SMTPPassword = sToP(model.FAKE_SETTING) - target.GitLabSettings.Secret = sToP(model.FAKE_SETTING) - target.OpenIdSettings.Secret = sToP(model.FAKE_SETTING) - target.SqlSettings.DataSource = sToP(model.FAKE_SETTING) - target.SqlSettings.AtRestEncryptKey = sToP(model.FAKE_SETTING) - target.ElasticsearchSettings.Password = sToP(model.FAKE_SETTING) + target.LdapSettings.BindPassword = model.NewString(model.FAKE_SETTING) + target.FileSettings.PublicLinkSalt = model.NewString(model.FAKE_SETTING) + target.FileSettings.AmazonS3SecretAccessKey = model.NewString(model.FAKE_SETTING) + target.EmailSettings.SMTPPassword = model.NewString(model.FAKE_SETTING) + target.GitLabSettings.Secret = model.NewString(model.FAKE_SETTING) + target.OpenIdSettings.Secret = model.NewString(model.FAKE_SETTING) + target.SqlSettings.DataSource = model.NewString(model.FAKE_SETTING) + target.SqlSettings.AtRestEncryptKey = model.NewString(model.FAKE_SETTING) + target.ElasticsearchSettings.Password = model.NewString(model.FAKE_SETTING) target.SqlSettings.DataSourceReplicas = []string{model.FAKE_SETTING, model.FAKE_SETTING} target.SqlSettings.DataSourceSearchReplicas = []string{model.FAKE_SETTING, model.FAKE_SETTING} @@ -246,15 +247,7 @@ func TestStripPassword(t *testing.T) { } } -func sToP(s string) *string { - return &s -} - -func bToP(b bool) *bool { - return &b -} - -func TestIsJsonMap(t *testing.T) { +func TestIsJSONMap(t *testing.T) { tests := []struct { name string data string @@ -278,9 +271,34 @@ func TestIsJsonMap(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := IsJsonMap(tt.data); got != tt.want { - t.Errorf("IsJsonMap() = %v, want %v", got, tt.want) + if got := isJSONMap(tt.data); got != tt.want { + t.Errorf("isJSONMap() = %v, want %v", got, tt.want) } }) } } + +func TestEqual(t *testing.T) { + t.Run("nil", func(t *testing.T) { + diff, err := equal(nil, nil) + require.NoError(t, err) + require.False(t, diff) + }) + + t.Run("no diff", func(t *testing.T) { + old := minimalConfig.Clone() + new := minimalConfig.Clone() + diff, err := equal(old, new) + require.NoError(t, err) + require.False(t, diff) + }) + + t.Run("diff", func(t *testing.T) { + old := minimalConfig.Clone() + new := minimalConfig.Clone() + new.SqlSettings = model.SqlSettings{} + diff, err := equal(old, new) + require.NoError(t, err) + require.True(t, diff) + }) +} diff --git a/config/watcher.go b/config/watcher.go index a7728e9bc2..6fc6a445cd 100644 --- a/config/watcher.go +++ b/config/watcher.go @@ -14,8 +14,6 @@ import ( // watcher monitors a file for changes type watcher struct { - emitter - fsWatcher *fsnotify.Watcher close chan struct{} closed chan struct{} diff --git a/config/config_generator/main.go b/scripts/config_generator/main.go similarity index 58% rename from config/config_generator/main.go rename to scripts/config_generator/main.go index ae0326f147..9c7b90a9ef 100644 --- a/config/config_generator/main.go +++ b/scripts/config_generator/main.go @@ -4,12 +4,25 @@ package main import ( + "encoding/json" "fmt" "os" - "github.com/mattermost/mattermost-server/v5/config/config_generator/generator" + "github.com/mattermost/mattermost-server/v5/model" ) +// generateDefaultConfig writes default config to outputFile. +func generateDefaultConfig(outputFile *os.File) error { + defaultCfg := &model.Config{} + defaultCfg.SetDefaults() + if data, err := json.MarshalIndent(defaultCfg, "", " "); err != nil { + return err + } else if _, err := outputFile.Write(data); err != nil { + return err + } + return nil +} + func main() { outputFile := os.Getenv("OUTPUT_CONFIG") if outputFile == "" { @@ -22,7 +35,7 @@ func main() { } if file, err := os.Create(outputFile); err == nil { - err = generator.GenerateDefaultConfig(file) + err = generateDefaultConfig(file) _ = file.Close() if err != nil { panic(err) @@ -30,5 +43,4 @@ func main() { } else { panic(err) } - } diff --git a/config/default_test.go b/scripts/config_generator/main_test.go similarity index 90% rename from config/default_test.go rename to scripts/config_generator/main_test.go index 1d5315ae1f..6915cd9905 100644 --- a/config/default_test.go +++ b/scripts/config_generator/main_test.go @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -package config +package main import ( "encoding/json" @@ -9,17 +9,16 @@ import ( "os" "testing" - "github.com/stretchr/testify/require" - - "github.com/mattermost/mattermost-server/v5/config/config_generator/generator" "github.com/mattermost/mattermost-server/v5/model" + + "github.com/stretchr/testify/require" ) func TestDefaultsGenerator(t *testing.T) { tmpFile, err := ioutil.TempFile("", "tempconfig") defer os.Remove(tmpFile.Name()) require.NoError(t, err) - require.NoError(t, generator.GenerateDefaultConfig(tmpFile)) + require.NoError(t, generateDefaultConfig(tmpFile)) _ = tmpFile.Close() var config model.Config