Lock bucket while modifying its metadata (Fixes #2979) (#3019)

- When modifying notification configuration
- When modifying listener configuration
- When modifying policy configuration

With this change we also stop early checking if the bucket exists, since
that uses a Read-lock and causes a deadlock due to the outer Write-lock.

cmd/bucket-notification-handlers.go

@@ -164,6 +164,13 @@ func PutBucketNotificationConfig(bucket string, ncfg *notificationConfig, objAPI
 		return errInvalidArgument
 	}
 
+	// Acquire a write lock on bucket before modifying its
+	// configuration.
+	opsID := getOpsID()
+	nsMutex.Lock(bucket, "", opsID)
+	// Release lock after notifying peers
+	defer nsMutex.Unlock(bucket, "", opsID)
+
 	// persist config to disk
 	err := persistNotificationConfig(bucket, ncfg, objAPI)
 	if err != nil {
@@ -365,6 +372,13 @@ func AddBucketListenerConfig(bucket string, lcfg *listenerConfig, objAPI ObjectL
 	// add new lid to listeners and persist to object layer.
 	listenerCfgs = append(listenerCfgs, *lcfg)
 
+	// Acquire a write lock on bucket before modifying its
+	// configuration.
+	opsID := getOpsID()
+	nsMutex.Lock(bucket, "", opsID)
+	// Release lock after notifying peers
+	defer nsMutex.Unlock(bucket, "", opsID)
+
 	// update persistent config
 	err := persistListenerConfig(bucket, listenerCfgs, objAPI)
 	if err != nil {
@@ -398,6 +412,13 @@ func RemoveBucketListenerConfig(bucket string, lcfg *listenerConfig, objAPI Obje
 		return
 	}
 
+	// Acquire a write lock on bucket before modifying its
+	// configuration.
+	opsID := getOpsID()
+	nsMutex.Lock(bucket, "", opsID)
+	// Release lock after notifying peers
+	defer nsMutex.Unlock(bucket, "", opsID)
+
 	// update persistent config
 	err := persistListenerConfig(bucket, updatedLcfgs, objAPI)
 	if err != nil {

cmd/bucket-policy-handlers.go

@@ -200,12 +200,20 @@ func (api objectAPIHandlers) PutBucketPolicyHandler(w http.ResponseWriter, r *ht
 // persists it to storage, and notify nodes in the cluster about the
 // change. In-memory state is updated in response to the notification.
 func persistAndNotifyBucketPolicyChange(bucket string, pCh policyChange, objAPI ObjectLayer) error {
-	// FIXME: Race exists between the bucket existence check and
+	// Verify if bucket actually exists. FIXME: Ideally this check
-	// then updating the bucket policy.
+	// should not be used but is kept here to error out for
+	// invalid and non-existent buckets.
 	if err := isBucketExist(bucket, objAPI); err != nil {
 		return err
 	}
 
+	// Acquire a write lock on bucket before modifying its
+	// configuration.
+	opsID := getOpsID()
+	nsMutex.Lock(bucket, "", opsID)
+	// Release lock after notifying peers
+	defer nsMutex.Unlock(bucket, "", opsID)
+
 	if pCh.IsRemove {
 		if err := removeBucketPolicy(bucket, objAPI); err != nil {
 			return err

cmd/bucket-policy.go

@@ -193,11 +193,6 @@ func readBucketPolicy(bucket string, objAPI ObjectLayer) (*bucketPolicy, error)
 // removeBucketPolicy - removes any previously written bucket policy. Returns BucketPolicyNotFound
 // if no policies are found.
 func removeBucketPolicy(bucket string, objAPI ObjectLayer) error {
-	// Verify if bucket actually exists
-	if err := isBucketExist(bucket, objAPI); err != nil {
-		return err
-	}
-
 	policyPath := pathJoin(bucketConfigPrefix, bucket, policyJSON)
 	if err := objAPI.DeleteObject(minioMetaBucket, policyPath); err != nil {
 		errorIf(err, "Unable to remove bucket-policy on bucket %s.", bucket)
@@ -213,11 +208,6 @@ func removeBucketPolicy(bucket string, objAPI ObjectLayer) error {
 // writeBucketPolicy - save a bucket policy that is assumed to be
 // validated.
 func writeBucketPolicy(bucket string, objAPI ObjectLayer, bpy *bucketPolicy) error {
-	// Verify if bucket actually exists
-	if err := isBucketExist(bucket, objAPI); err != nil {
-		return err
-	}
-
 	buf, err := json.Marshal(bpy)
 	if err != nil {
 		errorIf(err, "Unable to marshal bucket policy '%v' to JSON", *bpy)

cmd/event-notifier.go

@@ -399,12 +399,6 @@ func persistNotificationConfig(bucket string, ncfg *notificationConfig, obj Obje
 		return err
 	}
 
-	// verify bucket exists
-	// FIXME: There is a race between this check and PutObject
-	if err = isBucketExist(bucket, obj); err != nil {
-		return err
-	}
-
 	// build path
 	ncPath := path.Join(bucketConfigPrefix, bucket, bucketNotificationConfig)
 	// write object to path
@@ -425,12 +419,6 @@ func persistListenerConfig(bucket string, lcfg []listenerConfig, obj ObjectLayer
 		return err
 	}
 
-	// verify bucket exists
-	// FIXME: There is a race between this check and PutObject
-	if err = isBucketExist(bucket, obj); err != nil {
-		return err
-	}
-
 	// build path
 	lcPath := path.Join(bucketConfigPrefix, bucket, bucketListenerConfig)
 	// write object to path

cmd/event-notifier_test.go

@@ -277,15 +277,6 @@ func TestInitEventNotifier(t *testing.T) {
 		},
 	}
 
-	// write without an existing bucket and check
-	if err := persistNotificationConfig(bucketName, &notificationConfig{}, obj); err == nil {
-		t.Fatalf("Did not get an error though bucket does not exist!")
-	}
-	// no bucket write check for listener
-	if err := persistListenerConfig(bucketName, []listenerConfig{}, obj); err == nil {
-		t.Fatalf("Did not get an error though bucket does not exist!")
-	}
-
 	// create bucket
 	if err := obj.MakeBucket(bucketName); err != nil {
 		t.Fatal("Unexpected error:", err)