From 6e9c91f43a9275de56d6abdbb9528a0338b37168 Mon Sep 17 00:00:00 2001 From: Bala FA Date: Fri, 31 Mar 2017 10:56:24 +0530 Subject: [PATCH] fix: use its own lock in serverConfigV17 (#4014) Previously serverConfigV17 used a global lock that made any instance of serverConfigV17 depended on single global serverConfigMu. This patch fixes by having individual lock per instances. --- cmd/config-v17.go | 257 +++++++++++++++++++---------------------- cmd/config-v17_test.go | 2 +- cmd/gateway-main.go | 14 +-- cmd/server-main.go | 3 +- 4 files changed, 122 insertions(+), 154 deletions(-) diff --git a/cmd/config-v17.go b/cmd/config-v17.go index 1faa64578..085871cbd 100644 --- a/cmd/config-v17.go +++ b/cmd/config-v17.go @@ -26,17 +26,21 @@ import ( "github.com/tidwall/gjson" ) -// Read Write mutex for safe access to ServerConfig. -var serverConfigMu sync.RWMutex - // Config version const v17 = "17" +var ( + // serverConfig server config. + serverConfig *serverConfigV17 + serverConfigMu sync.RWMutex +) + // serverConfigV17 server configuration version '17' which is like // version '16' except it adds support for "format" parameter in // database event notification targets: PostgreSQL, MySQL, Redis and // Elasticsearch. type serverConfigV17 struct { + sync.RWMutex Version string `json:"version"` // S3 API configuration. @@ -51,6 +55,73 @@ type serverConfigV17 struct { Notify *notifier `json:"notify"` } +// GetVersion get current config version. +func (s *serverConfigV17) GetVersion() string { + s.RLock() + defer s.RUnlock() + + return s.Version +} + +// SetRegion set new region. +func (s *serverConfigV17) SetRegion(region string) { + s.Lock() + defer s.Unlock() + + s.Region = region +} + +// GetRegion get current region. +func (s *serverConfigV17) GetRegion() string { + s.RLock() + defer s.RUnlock() + + return s.Region +} + +// SetCredentials set new credentials. +func (s *serverConfigV17) SetCredential(creds credential) { + s.Lock() + defer s.Unlock() + + // Set updated credential. + s.Credential = creds +} + +// GetCredentials get current credentials. +func (s *serverConfigV17) GetCredential() credential { + s.RLock() + defer s.RUnlock() + + return s.Credential +} + +// SetBrowser set if browser is enabled. +func (s *serverConfigV17) SetBrowser(b bool) { + s.Lock() + defer s.Unlock() + + // Set the new value. + s.Browser = BrowserFlag(b) +} + +// GetCredentials get current credentials. +func (s *serverConfigV17) GetBrowser() bool { + s.RLock() + defer s.RUnlock() + + return bool(s.Browser) +} + +// Save config. +func (s *serverConfigV17) Save() error { + s.RLock() + defer s.RUnlock() + + // Save config file. + return quick.Save(getConfigFile(), s) +} + func newServerConfigV17() *serverConfigV17 { srvCfg := &serverConfigV17{ Version: v17, @@ -91,20 +162,14 @@ func newConfig() error { // Initialize server config. srvCfg := newServerConfigV17() - // If env is set for a fresh start, save them to config file. + // If env is set override the credentials from config file. if globalIsEnvCreds { srvCfg.SetCredential(globalActiveCred) } - if globalIsEnvBrowser { srvCfg.SetBrowser(globalIsBrowserEnabled) } - // Create config path. - if err := createConfigDir(); err != nil { - return err - } - // hold the mutex lock before a new config is assigned. // Save the new config globally. // unlock the mutex. @@ -116,42 +181,6 @@ func newConfig() error { return serverConfig.Save() } -// loadConfig - loads a new config from disk, overrides params from env -// if found and valid -func loadConfig() error { - srvCfg := &serverConfigV17{ - Region: globalMinioDefaultRegion, - Browser: true, - } - - if _, err := quick.Load(getConfigFile(), srvCfg); err != nil { - return err - } - if srvCfg.Version != v17 { - return fmt.Errorf("configuration version mismatch. Expected: ‘%s’, Got: ‘%s’", srvCfg.Version, v17) - } - - // If env is set override the credentials from config file. - if globalIsEnvCreds { - srvCfg.SetCredential(globalActiveCred) - } else { - globalActiveCred = srvCfg.GetCredential() - } - - if globalIsEnvBrowser { - srvCfg.SetBrowser(globalIsBrowserEnabled) - } else { - globalIsBrowserEnabled = srvCfg.GetBrowser() - } - - // hold the mutex lock before a new config is assigned. - serverConfigMu.Lock() - // Save the loaded config globally. - serverConfig = srvCfg - serverConfigMu.Unlock() - return nil -} - // doCheckDupJSONKeys recursively detects duplicate json keys func doCheckDupJSONKeys(key, value gjson.Result) error { // Key occurrences map of the current scope to count @@ -203,8 +232,8 @@ func checkDupJSONKeys(json string) error { return doCheckDupJSONKeys(rootKey, config) } -// validateConfig checks for -func validateConfig() error { +// getValidConfig - returns valid server configuration +func getValidConfig() (*serverConfigV17, error) { srvCfg := &serverConfigV17{ Region: globalMinioDefaultRegion, Browser: true, @@ -212,128 +241,74 @@ func validateConfig() error { configFile := getConfigFile() if _, err := quick.Load(configFile, srvCfg); err != nil { - return err + return nil, err } if srvCfg.Version != v17 { - // Older binary but newer config version. - // Config version can never be older as it would have migrated. - return fmt.Errorf("Expected config version: %s, newer config version found: %s", v17, srvCfg.Version) + return nil, fmt.Errorf("configuration version mismatch. Expected: ‘%s’, Got: ‘%s’", v17, srvCfg.Version) } // Load config file json and check for duplication json keys jsonBytes, err := ioutil.ReadFile(configFile) if err != nil { - return err + return nil, err } - if err := checkDupJSONKeys(string(jsonBytes)); err != nil { - return err + if err = checkDupJSONKeys(string(jsonBytes)); err != nil { + return nil, err } // Validate region field - if srvCfg.GetRegion() == "" { - return errors.New("Region config value cannot be empty") + if srvCfg.Region == "" { + return nil, errors.New("Region config value cannot be empty") } // Validate credential fields only when // they are not set via the environment - if !globalIsEnvCreds { - if !srvCfg.Credential.IsValid() { - return errors.New("invalid credential") - } + + // Error out if global is env credential is not set and config has invalid credential + if !globalIsEnvCreds && !srvCfg.Credential.IsValid() { + return nil, errors.New("invalid credential in config file " + configFile) } // Validate logger field - if err := srvCfg.Logger.Validate(); err != nil { - return err + if err = srvCfg.Logger.Validate(); err != nil { + return nil, err } // Validate notify field - if err := srvCfg.Notify.Validate(); err != nil { - return err + if err = srvCfg.Notify.Validate(); err != nil { + return nil, err } - return nil -} - -// serverConfig server config. -var serverConfig *serverConfigV17 - -// GetVersion get current config version. -func (s serverConfigV17) GetVersion() string { - serverConfigMu.RLock() - defer serverConfigMu.RUnlock() - - return s.Version + return srvCfg, nil } -// SetRegion set new region. -func (s *serverConfigV17) SetRegion(region string) { - serverConfigMu.Lock() - defer serverConfigMu.Unlock() - - // Empty region means "us-east-1" by default from S3 spec. - if region == "" { - region = "us-east-1" +// loadConfig - loads a new config from disk, overrides params from env +// if found and valid +func loadConfig() error { + srvCfg, err := getValidConfig() + if err != nil { + return err } - s.Region = region -} - -// GetRegion get current region. -func (s serverConfigV17) GetRegion() string { - serverConfigMu.RLock() - defer serverConfigMu.RUnlock() - - if s.Region != "" { - return s.Region - } // region empty - - // Empty region means "us-east-1" by default from S3 spec. - return "us-east-1" -} - -// SetCredentials set new credentials. -func (s *serverConfigV17) SetCredential(creds credential) { - serverConfigMu.Lock() - defer serverConfigMu.Unlock() - // Set updated credential. - s.Credential = creds -} - -// GetCredentials get current credentials. -func (s serverConfigV17) GetCredential() credential { - serverConfigMu.RLock() - defer serverConfigMu.RUnlock() - - return s.Credential -} + // If env is set override the credentials from config file. + if globalIsEnvCreds { + srvCfg.SetCredential(globalActiveCred) + } + if globalIsEnvBrowser { + srvCfg.SetBrowser(globalIsBrowserEnabled) + } -// SetBrowser set if browser is enabled. -func (s *serverConfigV17) SetBrowser(b bool) { + // hold the mutex lock before a new config is assigned. serverConfigMu.Lock() - defer serverConfigMu.Unlock() - - // Set the new value. - s.Browser = BrowserFlag(b) -} - -// GetCredentials get current credentials. -func (s serverConfigV17) GetBrowser() bool { - serverConfigMu.RLock() - defer serverConfigMu.RUnlock() - - return bool(s.Browser) -} - -// Save config. -func (s serverConfigV17) Save() error { - serverConfigMu.RLock() - defer serverConfigMu.RUnlock() - - // get config file. - configFile := getConfigFile() + serverConfig = srvCfg + if !globalIsEnvCreds { + globalActiveCred = serverConfig.GetCredential() + } + if !globalIsEnvBrowser { + globalIsBrowserEnabled = serverConfig.GetBrowser() + } + serverConfigMu.Unlock() - // Save config file. - return quick.Save(configFile, &s) + return nil } diff --git a/cmd/config-v17_test.go b/cmd/config-v17_test.go index 2f244ec90..c15d8a3b7 100644 --- a/cmd/config-v17_test.go +++ b/cmd/config-v17_test.go @@ -309,7 +309,7 @@ func TestValidateConfig(t *testing.T) { if werr := ioutil.WriteFile(configPath, []byte(testCase.configData), 0700); werr != nil { t.Fatal(werr) } - verr := validateConfig() + _, verr := getValidConfig() if testCase.shouldPass && verr != nil { t.Errorf("Test %d, should pass but it failed with err = %v", i+1, verr) } diff --git a/cmd/gateway-main.go b/cmd/gateway-main.go index dbd444532..542016975 100644 --- a/cmd/gateway-main.go +++ b/cmd/gateway-main.go @@ -104,17 +104,9 @@ func newGatewayConfig(accessKey, secretKey, region string) error { SecretKey: secretKey, }) - // Set default printing to console. - srvCfg.Logger.SetConsole(NewConsoleLogger()) - // Set custom region. srvCfg.SetRegion(region) - // Create certs path for SSL configuration. - if err := createConfigDir(); err != nil { - return err - } - // hold the mutex lock before a new config is assigned. // Save the new config globally. // unlock the mutex. @@ -139,8 +131,7 @@ func gatewayMain(ctx *cli.Context) { // TODO: add support for custom region when we add // support for S3 backend storage, currently this can // default to "us-east-1" - err := newGatewayConfig(accessKey, secretKey, "us-east-1") - fatalIf(err, "Unable to initialize gateway config") + newGatewayConfig(accessKey, secretKey, globalMinioDefaultRegion) // Get quiet flag from command line argument. quietFlag := ctx.Bool("quiet") || ctx.GlobalBool("quiet") @@ -151,6 +142,9 @@ func gatewayMain(ctx *cli.Context) { // First argument is selected backend type. backendType := ctx.Args().First() + // Create certs path for SSL configuration. + fatalIf(createConfigDir(), "Unable to create configuration directory") + newObject, err := newGatewayLayer(backendType, accessKey, secretKey) fatalIf(err, "Unable to initialize gateway layer") diff --git a/cmd/server-main.go b/cmd/server-main.go index 2e89b5869..13d4f76b2 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -357,8 +357,7 @@ func initConfig() { // Config file does not exist, we create it fresh and return upon success. if isFile(getConfigFile()) { fatalIf(migrateConfig(), "Config migration failed.") - fatalIf(validateConfig(), "Unable to validate configuration file") - fatalIf(loadConfig(), "Unable to initialize minio config") + fatalIf(loadConfig(), "Unable to load minio config file") } else { fatalIf(newConfig(), "Unable to initialize minio config for the first time.") log.Println("Created minio configuration file successfully at " + getConfigDir())