MakeBucket: Retry with correct region (#665)

api-error-response.go

@@ -48,6 +48,9 @@ type ErrorResponse struct {
 	// Region where the bucket is located. This header is returned
 	// only in HEAD bucket and ListObjects response.
 	Region string
+
+	// Headers of the returned S3 XML error
+	Headers http.Header `xml:"-" json:"-"`
 }
 
 // ToErrorResponse - Returns parsed ErrorResponse struct from body and
@@ -98,6 +101,7 @@ func httpRespToErrorResponse(resp *http.Response, bucketName, objectName string)
 		return ErrInvalidArgument(msg)
 	}
 	var errResp ErrorResponse
+
 	err := xmlDecoder(resp.Body, &errResp)
 	// Xml decoding failed with no body, fall back to HTTP headers.
 	if err != nil {
@@ -108,19 +112,13 @@ func httpRespToErrorResponse(resp *http.Response, bucketName, objectName string)
 					Code:       "NoSuchBucket",
 					Message:    "The specified bucket does not exist.",
 					BucketName: bucketName,
-					RequestID:  resp.Header.Get("x-amz-request-id"),
-					HostID:     resp.Header.Get("x-amz-id-2"),
-					Region:     resp.Header.Get("x-amz-bucket-region"),
 				}
 			} else {
 				errResp = ErrorResponse{
 					Code:       "NoSuchKey",
 					Message:    "The specified key does not exist.",
 					BucketName: bucketName,
 					Key:        objectName,
-					RequestID:  resp.Header.Get("x-amz-request-id"),
-					HostID:     resp.Header.Get("x-amz-id-2"),
-					Region:     resp.Header.Get("x-amz-bucket-region"),
 				}
 			}
 		case http.StatusForbidden:
@@ -129,30 +127,37 @@ func httpRespToErrorResponse(resp *http.Response, bucketName, objectName string)
 				Message:    "Access Denied.",
 				BucketName: bucketName,
 				Key:        objectName,
-				RequestID:  resp.Header.Get("x-amz-request-id"),
-				HostID:     resp.Header.Get("x-amz-id-2"),
-				Region:     resp.Header.Get("x-amz-bucket-region"),
 			}
 		case http.StatusConflict:
 			errResp = ErrorResponse{
 				Code:       "Conflict",
 				Message:    "Bucket not empty.",
 				BucketName: bucketName,
-				RequestID:  resp.Header.Get("x-amz-request-id"),
-				HostID:     resp.Header.Get("x-amz-id-2"),
-				Region:     resp.Header.Get("x-amz-bucket-region"),
 			}
 		default:
 			errResp = ErrorResponse{
 				Code:       resp.Status,
 				Message:    resp.Status,
 				BucketName: bucketName,
-				RequestID:  resp.Header.Get("x-amz-request-id"),
-				HostID:     resp.Header.Get("x-amz-id-2"),
-				Region:     resp.Header.Get("x-amz-bucket-region"),
 			}
 		}
 	}
+
+	// Save hodID, requestID and region information
+	// from headers if not available through error XML.
+	if errResp.RequestID == "" {
+		errResp.RequestID = resp.Header.Get("x-amz-request-id")
+	}
+	if errResp.HostID == "" {
+		errResp.HostID = resp.Header.Get("x-amz-id-2")
+	}
+	if errResp.Region == "" {
+		errResp.Region = resp.Header.Get("x-amz-bucket-region")
+	}
+
+	// Save headers returned in the API XML error
+	errResp.Headers = resp.Header
+
 	return errResp
 }
 

api-error-response_test.go

@@ -72,6 +72,7 @@ func TestHttpRespToErrorResponse(t *testing.T) {
 			RequestID:  resp.Header.Get("x-amz-request-id"),
 			HostID:     resp.Header.Get("x-amz-id-2"),
 			Region:     resp.Header.Get("x-amz-bucket-region"),
+			Headers:    resp.Header,
 		}
 		return errResp
 	}
@@ -172,7 +173,7 @@ func TestHttpRespToErrorResponse(t *testing.T) {
 	for i, testCase := range testCases {
 		actualResult := httpRespToErrorResponse(testCase.inputHTTPResp, testCase.bucketName, testCase.objectName)
 		if !reflect.DeepEqual(testCase.expectedResult, actualResult) {
-			t.Errorf("Test %d: Expected result to be '%+v', but instead got '%+v'", i+1, testCase.expectedResult, actualResult)
+			t.Errorf("Test %d: Expected result to be '%#v', but instead got '%#v'", i+1, testCase.expectedResult, actualResult)
 		}
 	}
 }

api-put-bucket.go

@@ -41,7 +41,14 @@ import (
 //
 // For Amazon S3 for more supported regions - http://docs.aws.amazon.com/general/latest/gr/rande.html
 // For Google Cloud Storage for more supported regions - https://cloud.google.com/storage/docs/bucket-locations
-func (c Client) MakeBucket(bucketName string, location string) error {
+func (c Client) MakeBucket(bucketName string, location string) (err error) {
+	defer func() {
+		// Save the location into cache on a successful makeBucket response.
+		if err == nil {
+			c.bucketLocCache.Set(bucketName, location)
+		}
+	}()
+
 	// Validate the input arguments.
 	if err := isValidBucketName(bucketName); err != nil {
 		return err
@@ -52,45 +59,70 @@ func (c Client) MakeBucket(bucketName string, location string) error {
 		location = "us-east-1"
 	}
 
-	// Instantiate the request.
-	req, err := c.makeBucketRequest(bucketName, location)
-	if err != nil {
-		return err
-	}
+	// Try creating bucket with the provided region, in case of
+	// invalid region error let's guess the appropriate region
+	// from S3 API headers
 
-	// Execute the request.
-	resp, err := c.do(req)
-	defer closeResponse(resp)
-	if err != nil {
-		return err
-	}
+	// Create a done channel to control 'newRetryTimer' go routine.
+	doneCh := make(chan struct{}, 1)
+
+	// Indicate to our routine to exit cleanly upon return.
+	defer close(doneCh)
+
+	// Blank indentifier is kept here on purpose since 'range' without
+	// blank identifiers is only supported since go1.4
+	// https://golang.org/doc/go1.4#forrange.
+	for _ = range c.newRetryTimer(MaxRetry, DefaultRetryUnit, DefaultRetryCap, MaxJitter, doneCh) {
+		// Initialize the makeBucket request.
+		req, err := c.makeBucketRequest(bucketName, location)
+		if err != nil {
+			return err
+		}
+
+		// Execute make bucket request.
+		resp, err := c.do(req)
+		defer closeResponse(resp)
+		if err != nil {
+			return err
+		}
 
-	if resp != nil {
 		if resp.StatusCode != http.StatusOK {
-			return httpRespToErrorResponse(resp, bucketName, "")
+			err := httpRespToErrorResponse(resp, bucketName, "")
+			errResp := ToErrorResponse(err)
+			if errResp.Code == "InvalidRegion" && errResp.Region != "" {
+				// Fetch bucket region found in headers
+				// of S3 error response, attempt bucket
+				// create again.
+				location = errResp.Region
+				continue
+			}
+			// Nothing to retry, fail.
+			return err
 		}
-	}
 
-	// Save the location into cache on a successful makeBucket response.
-	c.bucketLocCache.Set(bucketName, location)
+		// Control reaches here when bucket create was successful,
+		// break out.
+		break
+	}
 
-	// Return.
+	// Success.
 	return nil
 }
 
-// makeBucketRequest constructs request for makeBucket.
+// Low level wrapper API For makeBucketRequest.
 func (c Client) makeBucketRequest(bucketName string, location string) (*http.Request, error) {
 	// Validate input arguments.
 	if err := isValidBucketName(bucketName); err != nil {
 		return nil, err
 	}
 
-	// In case of Amazon S3.  The make bucket issued on already
-	// existing bucket would fail with 'AuthorizationMalformed' error
-	// if virtual style is used. So we default to 'path style' as that
-	// is the preferred method here. The final location of the
-	// 'bucket' is provided through XML LocationConstraint data with
-	// the request.
+	// In case of Amazon S3.  The make bucket issued on
+	// already existing bucket would fail with
+	// 'AuthorizationMalformed' error if virtual style is
+	// used. So we default to 'path style' as that is the
+	// preferred method here. The final location of the
+	// 'bucket' is provided through XML LocationConstraint
+	// data with the request.
 	targetURL := c.endpointURL
 	targetURL.Path = path.Join(bucketName, "") + "/"
 
@@ -103,7 +135,8 @@ func (c Client) makeBucketRequest(bucketName string, location string) (*http.Req
 	// set UserAgent for the request.
 	c.setUserAgent(req)
 
-	// set sha256 sum for signature calculation only with signature version '4'.
+	// set sha256 sum for signature calculation only with
+	// signature version '4'.
 	if c.signature.isV4() {
 		req.Header.Set("X-Amz-Content-Sha256", hex.EncodeToString(sum256([]byte{})))
 	}

api.go

@@ -480,7 +480,7 @@ func (c Client) executeMethod(method string, metadata requestMetadata) (res *htt
 		}
 	}
 
-	// Create a done channel to control 'ListObjects' go routine.
+	// Create a done channel to control 'newRetryTimer' go routine.
 	doneCh := make(chan struct{}, 1)
 
 	// Indicate to our routine to exit cleanly upon return.
@@ -489,7 +489,7 @@ func (c Client) executeMethod(method string, metadata requestMetadata) (res *htt
 	// Blank indentifier is kept here on purpose since 'range' without
 	// blank identifiers is only supported since go1.4
 	// https://golang.org/doc/go1.4#forrange.
-	for _ = range c.newRetryTimer(MaxRetry, time.Second, time.Second*30, MaxJitter, doneCh) {
+	for _ = range c.newRetryTimer(MaxRetry, DefaultRetryUnit, DefaultRetryCap, MaxJitter, doneCh) {
 		// Retry executes the following function body if request has an
 		// error until maxRetries have been exhausted, retry attempts are
 		// performed after waiting for a given period of time in a
@@ -538,14 +538,21 @@ func (c Client) executeMethod(method string, metadata requestMetadata) (res *htt
 		if err != nil {
 			return nil, err
 		}
+
 		// Save the body.
 		errBodySeeker := bytes.NewReader(errBodyBytes)
 		res.Body = ioutil.NopCloser(errBodySeeker)
 
 		// For errors verify if its retryable otherwise fail quickly.
 		errResponse := ToErrorResponse(httpRespToErrorResponse(res, metadata.bucketName, metadata.objectName))
-		// Bucket region if set in error response and the error code dictates invalid region,
-		// we can retry the request with the new region.
+
+		// Save the body back again.
+		errBodySeeker.Seek(0, 0) // Seek back to starting point.
+		res.Body = ioutil.NopCloser(errBodySeeker)
+
+		// Bucket region if set in error response and the error
+		// code dictates invalid region, we can retry the request
+		// with the new region.
 		if errResponse.Code == "InvalidRegion" && errResponse.Region != "" {
 			c.bucketLocCache.Set(metadata.bucketName, errResponse.Region)
 			continue // Retry.
@@ -561,10 +568,6 @@ func (c Client) executeMethod(method string, metadata requestMetadata) (res *htt
 			continue // Retry.
 		}
 
-		// Save the body back again.
-		errBodySeeker.Seek(0, 0) // Seek back to starting point.
-		res.Body = ioutil.NopCloser(errBodySeeker)
-
 		// For all other cases break out of the retry loop.
 		break
 	}

api_functional_v4_test.go

@@ -364,12 +364,11 @@ func TestPutObjectStreaming(t *testing.T) {
 	rand.Seed(time.Now().Unix())
 
 	// Instantiate new minio client object.
-	c, err := NewWithRegion(
+	c, err := NewV4(
 		os.Getenv("S3_ADDRESS"),
 		os.Getenv("ACCESS_KEY"),
 		os.Getenv("SECRET_KEY"),
 		mustParseBool(os.Getenv("S3_SECURE")),
-		"us-east-1",
 	)
 	if err != nil {
 		t.Fatal("Error:", err)
@@ -801,7 +800,7 @@ func TestRemovePartiallyUploaded(t *testing.T) {
 	if err == nil {
 		t.Fatal("Error: PutObject should fail.")
 	}
-	if err.Error() != "Proactively closed to be verified later." {
+	if err.Error() != "proactively closed to be verified later" {
 		t.Fatal("Error:", err)
 	}
 	err = c.RemoveIncompleteUpload(bucketName, objectName)

retry.go

@@ -33,8 +33,16 @@ const MaxJitter = 1.0
 // NoJitter disables the use of jitter for randomizing the exponential backoff time
 const NoJitter = 0.0
 
-// newRetryTimer creates a timer with exponentially increasing delays
-// until the maximum retry attempts are reached.
+// DefaultRetryUnit - default unit multiplicative per retry.
+// defaults to 1 second.
+const DefaultRetryUnit = time.Second
+
+// DefaultRetryCap - Each retry attempt never waits no longer than
+// this maximum time duration.
+const DefaultRetryCap = time.Second * 30
+
+// newRetryTimer creates a timer with exponentially increasing
+// delays until the maximum retry attempts are reached.
 func (c Client) newRetryTimer(maxRetry int, unit time.Duration, cap time.Duration, jitter float64, doneCh chan struct{}) <-chan int {
 	attemptCh := make(chan int)