From 018813b98f4fb5e1f0572c820708bd6112e97b8f Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Wed, 31 Jan 2018 08:15:54 -0800 Subject: [PATCH] Fix configuration handling bugs: (#5473) * Update the GetConfig admin API to use the latest version of configuration, along with fixes to the corresponding RPCs. * Remove mutex inside the configuration struct, and inside notification struct. * Use global config mutex where needed. * Add `serverConfig.ConfigDiff()` that provides a more granular diff of what is different between two configurations. --- cmd/admin-handlers.go | 6 ++- cmd/admin-rpc-client.go | 9 ++-- cmd/admin-rpc-client_test.go | 8 ++-- cmd/browser-peer-rpc.go | 4 ++ cmd/config-current.go | 77 +++++++++++++++++------------- cmd/config-current_test.go | 90 ++++++++++++++++++++++++++++++++++++ cmd/config-migrate.go | 2 +- cmd/config-versions.go | 8 ++-- cmd/notifier-config.go | 60 ++---------------------- cmd/web-handlers.go | 4 ++ 10 files changed, 166 insertions(+), 102 deletions(-) diff --git a/cmd/admin-handlers.go b/cmd/admin-handlers.go index d3f541333..1a5cdff84 100644 --- a/cmd/admin-handlers.go +++ b/cmd/admin-handlers.go @@ -795,7 +795,7 @@ func (a adminAPIHandlers) UpdateCredentialsHandler(w http.ResponseWriter, } // Take a lock on minio/config.json. Prevents concurrent - // config file/credentials updates. + // config file updates. configLock := globalNSMutex.NewNSLock(minioReservedBucket, minioConfigFile) if configLock.GetLock(globalObjectTimeout) != nil { writeErrorResponseJSON(w, ErrOperationTimedOut, r.URL) @@ -803,6 +803,10 @@ func (a adminAPIHandlers) UpdateCredentialsHandler(w http.ResponseWriter, } defer configLock.Unlock() + // Acquire lock before updating global configuration. + globalServerConfigMu.Lock() + defer globalServerConfigMu.Unlock() + // Notify all other Minio peers to update credentials updateErrs := updateCredsOnPeers(creds) for peer, err := range updateErrs { diff --git a/cmd/admin-rpc-client.go b/cmd/admin-rpc-client.go index ed3be9911..bd416ffe7 100644 --- a/cmd/admin-rpc-client.go +++ b/cmd/admin-rpc-client.go @@ -23,7 +23,6 @@ import ( "os" "path" "path/filepath" - "reflect" "sort" "sync" "time" @@ -478,7 +477,7 @@ func getPeerConfig(peers adminPeers) ([]byte, error) { // Find the maximally occurring config among peers in a // distributed setup. - serverConfigs := make([]serverConfigV13, len(peers)) + serverConfigs := make([]serverConfig, len(peers)) for i, configBytes := range configs { if errs[i] != nil { continue @@ -505,7 +504,7 @@ func getPeerConfig(peers adminPeers) ([]byte, error) { // getValidServerConfig - finds the server config that is present in // quorum or more number of servers. -func getValidServerConfig(serverConfigs []serverConfigV13, errs []error) (scv serverConfigV13, e error) { +func getValidServerConfig(serverConfigs []serverConfig, errs []error) (scv serverConfig, e error) { // majority-based quorum quorum := len(serverConfigs)/2 + 1 @@ -548,7 +547,7 @@ func getValidServerConfig(serverConfigs []serverConfigV13, errs []error) (scv se // seen. See example above for // clarity. continue - } else if j < i && reflect.DeepEqual(serverConfigs[i], serverConfigs[j]) { + } else if j < i && serverConfigs[i].ConfigDiff(&serverConfigs[j]) == "" { // serverConfigs[i] is equal to // serverConfigs[j], update // serverConfigs[j]'s counter since it @@ -567,7 +566,7 @@ func getValidServerConfig(serverConfigs []serverConfigV13, errs []error) (scv se // We find the maximally occurring server config and check if // there is quorum. - var configJSON serverConfigV13 + var configJSON serverConfig maxOccurrence := 0 for i, count := range configCounter { if maxOccurrence < count { diff --git a/cmd/admin-rpc-client_test.go b/cmd/admin-rpc-client_test.go index fa6ecda94..cb0a54846 100644 --- a/cmd/admin-rpc-client_test.go +++ b/cmd/admin-rpc-client_test.go @@ -220,7 +220,7 @@ var ( // TestGetValidServerConfig - test for getValidServerConfig. func TestGetValidServerConfig(t *testing.T) { - var c1, c2 serverConfigV13 + var c1, c2 serverConfig err := json.Unmarshal(config1, &c1) if err != nil { t.Fatalf("json unmarshal of %s failed: %v", string(config1), err) @@ -233,7 +233,7 @@ func TestGetValidServerConfig(t *testing.T) { // Valid config. noErrs := []error{nil, nil, nil, nil} - serverConfigs := []serverConfigV13{c1, c2, c1, c1} + serverConfigs := []serverConfig{c1, c2, c1, c1} validConfig, err := getValidServerConfig(serverConfigs, noErrs) if err != nil { t.Errorf("Expected a valid config but received %v instead", err) @@ -244,7 +244,7 @@ func TestGetValidServerConfig(t *testing.T) { } // Invalid config - no quorum. - serverConfigs = []serverConfigV13{c1, c2, c2, c1} + serverConfigs = []serverConfig{c1, c2, c2, c1} _, err = getValidServerConfig(serverConfigs, noErrs) if err != errXLWriteQuorum { t.Errorf("Expected to fail due to lack of quorum but received %v", err) @@ -252,7 +252,7 @@ func TestGetValidServerConfig(t *testing.T) { // All errors allErrs := []error{errDiskNotFound, errDiskNotFound, errDiskNotFound, errDiskNotFound} - serverConfigs = []serverConfigV13{{}, {}, {}, {}} + serverConfigs = []serverConfig{{}, {}, {}, {}} _, err = getValidServerConfig(serverConfigs, allErrs) if err != errXLWriteQuorum { t.Errorf("Expected to fail due to lack of quorum but received %v", err) diff --git a/cmd/browser-peer-rpc.go b/cmd/browser-peer-rpc.go index 0000e6539..1a3f7d363 100644 --- a/cmd/browser-peer-rpc.go +++ b/cmd/browser-peer-rpc.go @@ -50,6 +50,10 @@ func (br *browserPeerAPIHandlers) SetAuthPeer(args SetAuthPeerArgs, reply *AuthR return fmt.Errorf("Invalid credential passed") } + // Acquire lock before updating global configuration. + globalServerConfigMu.Lock() + defer globalServerConfigMu.Unlock() + // Update credentials in memory prevCred := globalServerConfig.SetCredential(args.Creds) diff --git a/cmd/config-current.go b/cmd/config-current.go index ea40c0f2d..66cd3cdb9 100644 --- a/cmd/config-current.go +++ b/cmd/config-current.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "io/ioutil" + "reflect" "sync" "github.com/minio/minio/pkg/auth" @@ -48,34 +49,22 @@ var ( // GetVersion get current config version. func (s *serverConfig) GetVersion() string { - s.RLock() - defer s.RUnlock() - return s.Version } // SetRegion set a new region. func (s *serverConfig) SetRegion(region string) { - s.Lock() - defer s.Unlock() - // Save new region. s.Region = region } // GetRegion get current region. func (s *serverConfig) GetRegion() string { - s.RLock() - defer s.RUnlock() - return s.Region } // SetCredential sets new credential and returns the previous credential. func (s *serverConfig) SetCredential(creds auth.Credentials) (prevCred auth.Credentials) { - s.Lock() - defer s.Unlock() - // Save previous credential. prevCred = s.Credential @@ -88,25 +77,16 @@ func (s *serverConfig) SetCredential(creds auth.Credentials) (prevCred auth.Cred // GetCredentials get current credentials. func (s *serverConfig) GetCredential() auth.Credentials { - s.RLock() - defer s.RUnlock() - return s.Credential } // SetBrowser set if browser is enabled. func (s *serverConfig) SetBrowser(b bool) { - s.Lock() - defer s.Unlock() - // Set the new value. s.Browser = BrowserFlag(b) } func (s *serverConfig) SetStorageClass(standardClass, rrsClass storageClass) { - s.Lock() - defer s.Unlock() - s.StorageClass.Standard = standardClass s.StorageClass.RRS = rrsClass } @@ -114,9 +94,6 @@ func (s *serverConfig) SetStorageClass(standardClass, rrsClass storageClass) { // GetStorageClass reads storage class fields from current config, parses and validates it. // It returns the standard and reduced redundancy storage class struct func (s *serverConfig) GetStorageClass() (storageClass, storageClass) { - s.RLock() - defer s.RUnlock() - var err error // Storage Class from config.json is already parsed and stored in s.StorageClass // Now validate the storage class fields @@ -140,21 +117,59 @@ func (s *serverConfig) GetStorageClass() (storageClass, storageClass) { // GetCredentials get current credentials. func (s *serverConfig) GetBrowser() bool { - s.RLock() - defer s.RUnlock() - return bool(s.Browser) } // Save config. func (s *serverConfig) Save() error { - s.RLock() - defer s.RUnlock() - // Save config file. return quick.Save(getConfigFile(), s) } +// Returns the string describing a difference with the given +// configuration object. If the given configuration object is +// identical, an empty string is returned. +func (s *serverConfig) ConfigDiff(t *serverConfig) string { + switch { + case t == nil: + return "Given configuration is empty" + case s.Credential != t.Credential: + return "Credential configuration differs" + case s.Region != t.Region: + return "Region configuration differs" + case s.Browser != t.Browser: + return "Browser configuration differs" + case s.Domain != t.Domain: + return "Domain configuration differs" + case s.StorageClass != t.StorageClass: + return "StorageClass configuration differs" + case !reflect.DeepEqual(s.Notify.AMQP, t.Notify.AMQP): + return "AMQP Notification configuration differs" + case !reflect.DeepEqual(s.Notify.NATS, t.Notify.NATS): + return "NATS Notification configuration differs" + case !reflect.DeepEqual(s.Notify.ElasticSearch, t.Notify.ElasticSearch): + return "ElasticSearch Notification configuration differs" + case !reflect.DeepEqual(s.Notify.Redis, t.Notify.Redis): + return "Redis Notification configuration differs" + case !reflect.DeepEqual(s.Notify.PostgreSQL, t.Notify.PostgreSQL): + return "PostgreSQL Notification configuration differs" + case !reflect.DeepEqual(s.Notify.Kafka, t.Notify.Kafka): + return "Kafka Notification configuration differs" + case !reflect.DeepEqual(s.Notify.Webhook, t.Notify.Webhook): + return "Webhook Notification configuration differs" + case !reflect.DeepEqual(s.Notify.MySQL, t.Notify.MySQL): + return "MySQL Notification configuration differs" + case !reflect.DeepEqual(s.Notify.MQTT, t.Notify.MQTT): + return "MQTT Notification configuration differs" + case reflect.DeepEqual(s, t): + return "" + default: + // This case will not happen unless this comparison + // function has become stale. + return "Configuration differs" + } +} + func newServerConfig() *serverConfig { srvCfg := &serverConfig{ Version: serverConfigVersion, @@ -162,7 +177,7 @@ func newServerConfig() *serverConfig { Region: globalMinioDefaultRegion, Browser: true, StorageClass: storageClassConfig{}, - Notify: ¬ifier{}, + Notify: notifier{}, } // Make sure to initialize notification configs. diff --git a/cmd/config-current_test.go b/cmd/config-current_test.go index b03314430..257a55cc2 100644 --- a/cmd/config-current_test.go +++ b/cmd/config-current_test.go @@ -23,6 +23,7 @@ import ( "reflect" "testing" + "github.com/minio/minio/pkg/auth" "github.com/tidwall/gjson" ) @@ -316,3 +317,92 @@ func TestValidateConfig(t *testing.T) { } } + +func TestConfigDiff(t *testing.T) { + testCases := []struct { + s, t *serverConfig + diff string + }{ + // 1 + {&serverConfig{}, nil, "Given configuration is empty"}, + // 2 + { + &serverConfig{Credential: auth.Credentials{"u1", "p1"}}, + &serverConfig{Credential: auth.Credentials{"u1", "p2"}}, + "Credential configuration differs", + }, + // 3 + {&serverConfig{Region: "us-east-1"}, &serverConfig{Region: "us-west-1"}, "Region configuration differs"}, + // 4 + {&serverConfig{Browser: false}, &serverConfig{Browser: true}, "Browser configuration differs"}, + // 5 + {&serverConfig{Domain: "domain1"}, &serverConfig{Domain: "domain2"}, "Domain configuration differs"}, + // 6 + { + &serverConfig{StorageClass: storageClassConfig{storageClass{"1", 8}, storageClass{"2", 6}}}, + &serverConfig{StorageClass: storageClassConfig{storageClass{"1", 8}, storageClass{"2", 4}}}, + "StorageClass configuration differs", + }, + // 7 + { + &serverConfig{Notify: notifier{AMQP: map[string]amqpNotify{"1": {Enable: true}}}}, + &serverConfig{Notify: notifier{AMQP: map[string]amqpNotify{"1": {Enable: false}}}}, + "AMQP Notification configuration differs", + }, + // 8 + { + &serverConfig{Notify: notifier{NATS: map[string]natsNotify{"1": {Enable: true}}}}, + &serverConfig{Notify: notifier{NATS: map[string]natsNotify{"1": {Enable: false}}}}, + "NATS Notification configuration differs", + }, + // 9 + { + &serverConfig{Notify: notifier{ElasticSearch: map[string]elasticSearchNotify{"1": {Enable: true}}}}, + &serverConfig{Notify: notifier{ElasticSearch: map[string]elasticSearchNotify{"1": {Enable: false}}}}, + "ElasticSearch Notification configuration differs", + }, + // 10 + { + &serverConfig{Notify: notifier{Redis: map[string]redisNotify{"1": {Enable: true}}}}, + &serverConfig{Notify: notifier{Redis: map[string]redisNotify{"1": {Enable: false}}}}, + "Redis Notification configuration differs", + }, + // 11 + { + &serverConfig{Notify: notifier{PostgreSQL: map[string]postgreSQLNotify{"1": {Enable: true}}}}, + &serverConfig{Notify: notifier{PostgreSQL: map[string]postgreSQLNotify{"1": {Enable: false}}}}, + "PostgreSQL Notification configuration differs", + }, + // 12 + { + &serverConfig{Notify: notifier{Kafka: map[string]kafkaNotify{"1": {Enable: true}}}}, + &serverConfig{Notify: notifier{Kafka: map[string]kafkaNotify{"1": {Enable: false}}}}, + "Kafka Notification configuration differs", + }, + // 13 + { + &serverConfig{Notify: notifier{Webhook: map[string]webhookNotify{"1": {Enable: true}}}}, + &serverConfig{Notify: notifier{Webhook: map[string]webhookNotify{"1": {Enable: false}}}}, + "Webhook Notification configuration differs", + }, + // 14 + { + &serverConfig{Notify: notifier{MySQL: map[string]mySQLNotify{"1": {Enable: true}}}}, + &serverConfig{Notify: notifier{MySQL: map[string]mySQLNotify{"1": {Enable: false}}}}, + "MySQL Notification configuration differs", + }, + // 15 + { + &serverConfig{Notify: notifier{MQTT: map[string]mqttNotify{"1": {Enable: true}}}}, + &serverConfig{Notify: notifier{MQTT: map[string]mqttNotify{"1": {Enable: false}}}}, + "MQTT Notification configuration differs", + }, + } + + for i, testCase := range testCases { + got := testCase.s.ConfigDiff(testCase.t) + if got != testCase.diff { + t.Errorf("Test %d: got %s expected %s", i+1, got, testCase.diff) + } + } +} diff --git a/cmd/config-migrate.go b/cmd/config-migrate.go index 583db2838..ac17fcbcf 100644 --- a/cmd/config-migrate.go +++ b/cmd/config-migrate.go @@ -1717,7 +1717,7 @@ func migrateV21ToV22() error { // Copy over fields from V21 into V22 config struct srvConfig := &serverConfigV22{ - Notify: ¬ifier{}, + Notify: notifier{}, } srvConfig.Version = serverConfigVersion srvConfig.Credential = cv21.Credential diff --git a/cmd/config-versions.go b/cmd/config-versions.go index 58c9da2ac..831f2d80b 100644 --- a/cmd/config-versions.go +++ b/cmd/config-versions.go @@ -547,9 +547,11 @@ type serverConfigV21 struct { } // serverConfigV22 is just like version '21' with added support -// for StorageClass +// for StorageClass. +// +// IMPORTANT NOTE: When updating this struct make sure that +// serverConfig.ConfigDiff() is updated as necessary. type serverConfigV22 struct { - sync.RWMutex Version string `json:"version"` // S3 API configuration. @@ -562,5 +564,5 @@ type serverConfigV22 struct { StorageClass storageClassConfig `json:"storageclass"` // Notification queue configuration. - Notify *notifier `json:"notify"` + Notify notifier `json:"notify"` } diff --git a/cmd/notifier-config.go b/cmd/notifier-config.go index e46c16eb8..d87c72ec8 100644 --- a/cmd/notifier-config.go +++ b/cmd/notifier-config.go @@ -18,12 +18,10 @@ package cmd import ( "fmt" - "sync" ) // Notifier represents collection of supported notification queues. type notifier struct { - sync.RWMutex AMQP amqpConfigs `json:"amqp"` NATS natsConfigs `json:"nats"` ElasticSearch elasticSearchConfigs `json:"elasticsearch"` @@ -33,7 +31,9 @@ type notifier struct { Webhook webhookConfigs `json:"webhook"` MySQL mySQLConfigs `json:"mysql"` MQTT mqttConfigs `json:"mqtt"` - // Add new notification queues. + // Add new notification queues. IMPORTANT: When new queues are + // added, update `serverConfig.ConfigDiff()` to reflect the + // change. } type amqpConfigs map[string]amqpNotify @@ -239,163 +239,109 @@ func (n *notifier) Validate() error { } func (n *notifier) SetAMQPByID(accountID string, amqpn amqpNotify) { - n.Lock() - defer n.Unlock() n.AMQP[accountID] = amqpn } func (n *notifier) GetAMQP() map[string]amqpNotify { - n.RLock() - defer n.RUnlock() return n.AMQP.Clone() } func (n *notifier) GetAMQPByID(accountID string) amqpNotify { - n.RLock() - defer n.RUnlock() return n.AMQP[accountID] } func (n *notifier) SetMQTTByID(accountID string, mqttn mqttNotify) { - n.Lock() - defer n.Unlock() n.MQTT[accountID] = mqttn } func (n *notifier) GetMQTT() map[string]mqttNotify { - n.RLock() - defer n.RUnlock() return n.MQTT.Clone() } func (n *notifier) GetMQTTByID(accountID string) mqttNotify { - n.RLock() - defer n.RUnlock() return n.MQTT[accountID] } func (n *notifier) SetNATSByID(accountID string, natsn natsNotify) { - n.Lock() - defer n.Unlock() n.NATS[accountID] = natsn } func (n *notifier) GetNATS() map[string]natsNotify { - n.RLock() - defer n.RUnlock() return n.NATS.Clone() } func (n *notifier) GetNATSByID(accountID string) natsNotify { - n.RLock() - defer n.RUnlock() return n.NATS[accountID] } func (n *notifier) SetElasticSearchByID(accountID string, es elasticSearchNotify) { - n.Lock() - defer n.Unlock() n.ElasticSearch[accountID] = es } func (n *notifier) GetElasticSearchByID(accountID string) elasticSearchNotify { - n.RLock() - defer n.RUnlock() return n.ElasticSearch[accountID] } func (n *notifier) GetElasticSearch() map[string]elasticSearchNotify { - n.RLock() - defer n.RUnlock() return n.ElasticSearch.Clone() } func (n *notifier) SetRedisByID(accountID string, r redisNotify) { - n.Lock() - defer n.Unlock() n.Redis[accountID] = r } func (n *notifier) GetRedis() map[string]redisNotify { - n.RLock() - defer n.RUnlock() return n.Redis.Clone() } func (n *notifier) GetRedisByID(accountID string) redisNotify { - n.RLock() - defer n.RUnlock() return n.Redis[accountID] } func (n *notifier) GetWebhook() map[string]webhookNotify { - n.RLock() - defer n.RUnlock() return n.Webhook.Clone() } func (n *notifier) GetWebhookByID(accountID string) webhookNotify { - n.RLock() - defer n.RUnlock() return n.Webhook[accountID] } func (n *notifier) SetWebhookByID(accountID string, pgn webhookNotify) { - n.Lock() - defer n.Unlock() n.Webhook[accountID] = pgn } func (n *notifier) SetPostgreSQLByID(accountID string, pgn postgreSQLNotify) { - n.Lock() - defer n.Unlock() n.PostgreSQL[accountID] = pgn } func (n *notifier) GetPostgreSQL() map[string]postgreSQLNotify { - n.RLock() - defer n.RUnlock() return n.PostgreSQL.Clone() } func (n *notifier) GetPostgreSQLByID(accountID string) postgreSQLNotify { - n.RLock() - defer n.RUnlock() return n.PostgreSQL[accountID] } func (n *notifier) SetMySQLByID(accountID string, pgn mySQLNotify) { - n.Lock() - defer n.Unlock() n.MySQL[accountID] = pgn } func (n *notifier) GetMySQL() map[string]mySQLNotify { - n.RLock() - defer n.RUnlock() return n.MySQL.Clone() } func (n *notifier) GetMySQLByID(accountID string) mySQLNotify { - n.RLock() - defer n.RUnlock() return n.MySQL[accountID] } func (n *notifier) SetKafkaByID(accountID string, kn kafkaNotify) { - n.Lock() - defer n.Unlock() n.Kafka[accountID] = kn } func (n *notifier) GetKafka() map[string]kafkaNotify { - n.RLock() - defer n.RUnlock() return n.Kafka.Clone() } func (n *notifier) GetKafkaByID(accountID string) kafkaNotify { - n.RLock() - defer n.RUnlock() return n.Kafka[accountID] } diff --git a/cmd/web-handlers.go b/cmd/web-handlers.go index f81fd0382..aabad1c82 100644 --- a/cmd/web-handlers.go +++ b/cmd/web-handlers.go @@ -423,6 +423,10 @@ func (web *webAPIHandlers) SetAuth(r *http.Request, args *SetAuthArgs, reply *Se return toJSONError(err) } + // Acquire lock before updating global configuration. + globalServerConfigMu.Lock() + defer globalServerConfigMu.Unlock() + // Notify all other Minio peers to update credentials errsMap := updateCredsOnPeers(creds)