Object Store: Make region optional in S3 provider (#4265)

* Object Store: Make region optional in S3 provider

* Skip region validation if endpoint is set.

* Check for gcs s3 not found errors

* Handle nil errors

* Move const to const group

Was masking errors where we were issuing queries to AWS
for GCS (in s3 interoperability mode) and they were
succeeding because the bucket also existed in AWS

pkg/objectstore/aws.go

@@ -2,6 +2,7 @@ package objectstore
 
 import (
 	"context"
+	"strings"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
@@ -14,6 +15,7 @@ import (
 const (
 	bucketNotFound = "NotFound"
 	noSuchBucket   = s3.ErrCodeNoSuchBucket
+	gcsS3NotFound  = "not found"
 )
 
 func config(region string) *aws.Config {
@@ -25,11 +27,14 @@ func config(region string) *aws.Config {
 }
 
 func isBucketNotFoundError(err error) bool {
+	if err == nil {
+		return false
+	}
 	if awsErr, ok := errors.Cause(err).(awserr.Error); ok {
 		code := awsErr.Code()
 		return code == bucketNotFound || code == noSuchBucket
 	}
-	return false
+	return strings.Contains(err.Error(), gcsS3NotFound)
 }
 
 func GetS3BucketRegion(ctx context.Context, bucketName, regionHint string) (string, error) {

pkg/objectstore/bucket.go

@@ -4,6 +4,7 @@ package objectstore
 
 import (
 	"context"
+	"path/filepath"
 
 	"github.com/graymeta/stow"
 	"github.com/pkg/errors"
@@ -34,7 +35,6 @@ type bucket struct {
 // CreateBucket creates the bucket. Bucket naming rules are provider dependent.
 func (p *provider) CreateBucket(ctx context.Context, bucketName, region string) (Bucket, error) {
 	location, err := getStowLocation(ctx, p.config, p.secret, region)
-	p.hostEndPoint = checkHostURI(p.config, p.hostEndPoint, region)
 	if err != nil {
 		return nil, err
 	}
@@ -49,7 +49,7 @@ func (p *provider) CreateBucket(ctx context.Context, bucketName, region string)
 		directory:    dir,
 		container:    c,
 		location:     location,
-		hostEndPoint: getHostEndPoint(p.hostEndPoint, c.ID()),
+		hostEndPoint: filepath.Join(p.hostEndPoint, c.ID()),
 	}
 	dir.bucket = bucket
 	return bucket, nil
@@ -73,7 +73,7 @@ func (p *provider) GetBucket(ctx context.Context, bucketName string) (Bucket, er
 		directory:    dir,
 		container:    c,
 		location:     location,
-		hostEndPoint: getHostEndPoint(p.hostEndPoint, c.ID()),
+		hostEndPoint: filepath.Join(p.hostEndPoint, c.ID()),
 	}
 	dir.bucket = bucket
 	return bucket, nil
@@ -101,7 +101,7 @@ func (p *provider) ListBuckets(ctx context.Context) (map[string]Bucket, error) {
 				directory:    dir,
 				container:    c,
 				location:     location,
-				hostEndPoint: getHostEndPoint(p.hostEndPoint, c.ID()),
+				hostEndPoint: filepath.Join(p.hostEndPoint, c.ID()),
 			}
 			dir.bucket = bucket
 			buckets[c.ID()] = bucket
@@ -138,11 +138,11 @@ type s3Provider struct {
 }
 
 func (p *s3Provider) GetBucket(ctx context.Context, bucketName string) (Bucket, error) {
-	region, err := p.getRegionForBucket(ctx, bucketName)
-	if err != nil {
-		return nil, errors.Wrapf(err, "could not get region for bucket %s", bucketName)
+	region, _ := p.getRegionForBucket(ctx, bucketName)
+	hostEndPoint := p.hostEndPoint
+	if hostEndPoint != "" {
+		hostEndPoint = awsS3Endpoint(region)
 	}
-	p.hostEndPoint = checkHostURI(p.config, p.hostEndPoint, region)
 	location, err := getStowLocation(ctx, p.config, p.secret, region)
 	if err != nil {
 		return nil, err
@@ -158,28 +158,23 @@ func (p *s3Provider) GetBucket(ctx context.Context, bucketName string) (Bucket,
 		directory:    dir,
 		container:    c,
 		location:     location,
-		hostEndPoint: getHostEndPoint(p.hostEndPoint, c.ID()),
+		hostEndPoint: filepath.Join(hostEndPoint, c.ID()),
 	}
 	dir.bucket = bucket
 	return bucket, nil
 }
 
 func (p *s3Provider) DeleteBucket(ctx context.Context, bucketName string) error {
-	region, err := p.getRegionForBucket(ctx, bucketName)
+	region, _ := p.getRegionForBucket(ctx, bucketName)
+	location, err := getStowLocation(ctx, p.config, p.secret, region)
 	if err != nil {
-		return errors.Wrapf(err, "could not get region for bucket %s", bucketName)
+		return errors.Wrapf(err, "Failed to get location for bucket deletion. bucket: %s", bucketName)
 	}
-	p.hostEndPoint = checkHostURI(p.config, p.hostEndPoint, region)
-	location, err := getStowLocation(ctx, p.config, p.secret, region)
 	return location.RemoveContainer(bucketName)
 }
 
 // returns the region for a particular bucket
 func (p *s3Provider) getRegionForBucket(ctx context.Context, bucketName string) (string, error) {
-	// Only AWS S3 requires this check
-	if p.config.Type != ProviderTypeS3 {
-		return "", errors.New("getRegionForBucket can be used only for S3")
-	}
 	return GetS3BucketRegion(ctx, bucketName, "")
 }
 

pkg/objectstore/const.go

@@ -1,9 +1,8 @@
 package objectstore
 
 const (
-	awsS3HostFmt = "https://s3-%s.amazonaws.com"
-
-	googleGCSHostFmt = "https://storage.googleapis.com"
+	awsS3HostFmt  = "https://s3-%s.amazonaws.com"
+	googleGCSHost = "https://storage.googleapis.com"
 )
 
 // ProviderType enum for different providers

pkg/objectstore/objectstore.go

@@ -205,21 +205,12 @@ func getStowLocation(ctx context.Context, config ProviderConfig, secret *Secret,
 func getHostURI(config ProviderConfig) string {
 	switch config.Type {
 	case ProviderTypeGCS:
-		return fmt.Sprintf(googleGCSHostFmt)
+		return googleGCSHost
 	default:
 		return config.Endpoint
 	}
 }
 
-func checkHostURI(config ProviderConfig, hostEndpoint, region string) string {
-	if config.Type == ProviderTypeS3 {
-		if expectedEndpoint := fmt.Sprintf(awsS3HostFmt, region); expectedEndpoint != hostEndpoint {
-			return expectedEndpoint
-		}
-	}
-	return hostEndpoint
-}
-
-func getHostEndPoint(host, path string) string {
-	return fmt.Sprintf("%s/%s", host, path)
+func awsS3Endpoint(region string) string {
+	return fmt.Sprintf(awsS3HostFmt, region)
 }