From 398421b9f5a31b3c0943b6ece3cc6f1769bf3c18 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 14 Nov 2016 15:45:00 -0800 Subject: [PATCH] xl/bootup: Server bootup shouldn't return for missing buckets. (#3255) Ref #3196 --- cmd/bucket-notification-handlers.go | 11 +++++++-- cmd/bucket-policy-handlers.go | 35 ++++++++++++++++++++--------- cmd/bucket-policy-handlers_test.go | 15 ++++++++----- cmd/bucket-policy.go | 5 ----- cmd/event-notifier.go | 1 - cmd/web-handlers_test.go | 2 -- 6 files changed, 43 insertions(+), 26 deletions(-) diff --git a/cmd/bucket-notification-handlers.go b/cmd/bucket-notification-handlers.go index 376022c49..e7a8dce5f 100644 --- a/cmd/bucket-notification-handlers.go +++ b/cmd/bucket-notification-handlers.go @@ -54,6 +54,14 @@ func (api objectAPIHandlers) GetBucketNotificationHandler(w http.ResponseWriter, } vars := mux.Vars(r) bucket := vars["bucket"] + + _, err := objAPI.GetBucketInfo(bucket) + if err != nil { + errorIf(err, "Unable to find bucket info.") + writeErrorResponse(w, r, toAPIErrorCode(err), r.URL.Path) + return + } + // Attempt to successfully load notification config. nConfig, err := loadNotificationConfig(bucket, objAPI) if err != nil && err != errNoSuchNotifications { @@ -177,8 +185,7 @@ func PutBucketNotificationConfig(bucket string, ncfg *notificationConfig, objAPI return fmt.Errorf("Unable to persist Bucket notification config to object layer - config=%v errMsg=%v", *ncfg, err) } - // All servers (including local) are told to update in-memory - // config + // All servers (including local) are told to update in-memory config S3PeersUpdateBucketNotification(bucket, ncfg) return nil diff --git a/cmd/bucket-policy-handlers.go b/cmd/bucket-policy-handlers.go index 0ada4f934..c73c6eadd 100644 --- a/cmd/bucket-policy-handlers.go +++ b/cmd/bucket-policy-handlers.go @@ -142,6 +142,14 @@ func (api objectAPIHandlers) PutBucketPolicyHandler(w http.ResponseWriter, r *ht vars := mux.Vars(r) bucket := vars["bucket"] + // Before proceeding validate if bucket exists. + _, err := objAPI.GetBucketInfo(bucket) + if err != nil { + errorIf(err, "Unable to find bucket info.") + writeErrorResponse(w, r, toAPIErrorCode(err), r.URL.Path) + return + } + // If Content-Length is unknown or zero, deny the // request. PutBucketPolicy always needs a Content-Length if // incoming request is not chunked. @@ -200,13 +208,6 @@ 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 { - // Verify if bucket actually exists. FIXME: Ideally this check - // 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. bucketLock := nsMutex.NewNSLock(bucket, "") @@ -253,12 +254,18 @@ func (api objectAPIHandlers) DeleteBucketPolicyHandler(w http.ResponseWriter, r vars := mux.Vars(r) bucket := vars["bucket"] + // Before proceeding validate if bucket exists. + _, err := objAPI.GetBucketInfo(bucket) + if err != nil { + errorIf(err, "Unable to find bucket info.") + writeErrorResponse(w, r, toAPIErrorCode(err), r.URL.Path) + return + } + // Delete bucket access policy, by passing an empty policy // struct. if err := persistAndNotifyBucketPolicyChange(bucket, policyChange{true, nil}, objAPI); err != nil { switch err.(type) { - case BucketNameInvalid: - writeErrorResponse(w, r, ErrInvalidBucketName, r.URL.Path) case BucketPolicyNotFound: writeErrorResponse(w, r, ErrNoSuchBucketPolicy, r.URL.Path) default: @@ -292,13 +299,19 @@ func (api objectAPIHandlers) GetBucketPolicyHandler(w http.ResponseWriter, r *ht vars := mux.Vars(r) bucket := vars["bucket"] + // Before proceeding validate if bucket exists. + _, err := objAPI.GetBucketInfo(bucket) + if err != nil { + errorIf(err, "Unable to find bucket info.") + writeErrorResponse(w, r, toAPIErrorCode(err), r.URL.Path) + return + } + // Read bucket access policy. policy, err := readBucketPolicy(bucket, objAPI) if err != nil { errorIf(err, "Unable to read bucket policy.") switch err.(type) { - case BucketNameInvalid: - writeErrorResponse(w, r, ErrInvalidBucketName, r.URL.Path) case BucketPolicyNotFound: writeErrorResponse(w, r, ErrNoSuchBucketPolicy, r.URL.Path) default: diff --git a/cmd/bucket-policy-handlers_test.go b/cmd/bucket-policy-handlers_test.go index 25c9fbe98..5b89e5289 100644 --- a/cmd/bucket-policy-handlers_test.go +++ b/cmd/bucket-policy-handlers_test.go @@ -250,6 +250,11 @@ func testPutBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName string credentials credential, t *testing.T) { initBucketPolicies(obj) + bucketName1 := fmt.Sprintf("%s-1", bucketName) + if err := obj.MakeBucket(bucketName1); err != nil { + t.Fatal(err) + } + // template for constructing HTTP request body for PUT bucket policy. bucketPolicyTemplate := `{"Version":"2012-10-17","Statement":[{"Sid":"","Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetBucketLocation","s3:ListBucket"],"Resource":["arn:aws:s3:::%s"]},{"Sid":"","Effect":"Allow","Principal":{"AWS":["*"]},"Action":["s3:GetObject"],"Resource":["arn:aws:s3:::%s/this*"]}]}` @@ -327,7 +332,7 @@ func testPutBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName string // setting an invalid bucket policy. // the bucket policy parser will fail. { - bucketName: "non-existent-bucket", + bucketName: bucketName, bucketPolicyReader: bytes.NewReader([]byte("dummy-policy")), policyLen: len([]byte("dummy-policy")), @@ -339,7 +344,7 @@ func testPutBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName string // Different bucket name used in the HTTP request and the policy string. // checkBucketPolicyResources should fail. { - bucketName: "different-bucket", + bucketName: bucketName1, bucketPolicyReader: bytes.NewReader([]byte(fmt.Sprintf(bucketPolicyTemplate, bucketName, bucketName))), policyLen: len(fmt.Sprintf(bucketPolicyTemplate, bucketName, bucketName)), @@ -358,7 +363,7 @@ func testPutBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName string policyLen: len(fmt.Sprintf(bucketPolicyTemplate, bucketName, bucketName)), accessKey: credentials.AccessKeyID, secretKey: credentials.SecretAccessKey, - expectedRespStatus: http.StatusInternalServerError, + expectedRespStatus: http.StatusNotFound, }, // Test case - 9. // invalid bucket name is used. @@ -527,7 +532,7 @@ func testGetBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName string accessKey: credentials.AccessKeyID, secretKey: credentials.SecretAccessKey, expectedBucketPolicy: bucketPolicyTemplate, - expectedRespStatus: http.StatusInternalServerError, + expectedRespStatus: http.StatusNotFound, }, // Test case - 3. // Case with invalid bucket name. @@ -736,7 +741,7 @@ func testDeleteBucketPolicyHandler(obj ObjectLayer, instanceType, bucketName str bucketName: "non-existent-bucket", accessKey: credentials.AccessKeyID, secretKey: credentials.SecretAccessKey, - expectedRespStatus: http.StatusInternalServerError, + expectedRespStatus: http.StatusNotFound, }, // Test case - 3. // Case with invalid bucket name. diff --git a/cmd/bucket-policy.go b/cmd/bucket-policy.go index 3b3155e19..44319da90 100644 --- a/cmd/bucket-policy.go +++ b/cmd/bucket-policy.go @@ -142,11 +142,6 @@ func getOldBucketsConfigPath() (string, error) { // readBucketPolicyJSON - reads bucket policy for an input bucket, returns BucketPolicyNotFound // if bucket policy is not found. func readBucketPolicyJSON(bucket string, objAPI ObjectLayer) (bucketPolicyReader io.Reader, err error) { - // Verify if bucket actually exists - if err = isBucketExist(bucket, objAPI); err != nil { - return nil, err - } - policyPath := pathJoin(bucketConfigPrefix, bucket, policyJSON) objInfo, err := objAPI.GetObjectInfo(minioMetaBucket, policyPath) err = errorCause(err) diff --git a/cmd/event-notifier.go b/cmd/event-notifier.go index be3612b7f..5c1b1a061 100644 --- a/cmd/event-notifier.go +++ b/cmd/event-notifier.go @@ -466,7 +466,6 @@ func loadAllBucketNotifications(objAPI ObjectLayer) (map[string]*notificationCon } else { nConfigs[bucket.Name] = nCfg } - lCfg, lErr := loadListenerConfig(bucket.Name, objAPI) if lErr != nil { if lErr != errNoSuchNotifications { diff --git a/cmd/web-handlers_test.go b/cmd/web-handlers_test.go index fcc1747ee..b2f89a49a 100644 --- a/cmd/web-handlers_test.go +++ b/cmd/web-handlers_test.go @@ -1118,8 +1118,6 @@ func testWebSetBucketPolicyHandler(obj ObjectLayer, instanceType string, t TestE policy string pass bool }{ - // Inexistent bucket - {"fooo", "", "readonly", false}, // Invalid bucket name {"", "", "readonly", false}, // Invalid policy