Merge pull request #1356 from bgilbert/region

internal/resource: derive AWS region hint from ARN partition field

Author: bgilbert

internal/resource/url.go

@@ -63,6 +63,15 @@ var (
 		"Accept-Encoding": []string{"identity"},
 		"Accept":          []string{"application/vnd.coreos.ignition+json;version=3.3.0, */*;q=0.1"},
 	}
+
+	// We could derive this info from aws-sdk-go/aws/endpoints/defaults.go,
+	// but hardcoding it allows us to unit-test that specific regions
+	// are used for hinting
+	awsPartitionRegionHints = map[string]string{
+		"aws":        "us-east-1",
+		"aws-cn":     "cn-north-1",
+		"aws-us-gov": "us-gov-west-1",
+	}
 )
 
 // Fetcher holds settings for fetching resources from URLs
@@ -409,7 +418,7 @@ func (f *Fetcher) fetchFromS3(u url.URL, dest s3target, opts FetchOptions) error
 	sess := f.AWSSession.Copy()
 
 	// Determine the bucket and key based on the URL scheme
-	var bucket, key, region string
+	var bucket, key, region, regionHint string
 	var err error
 	switch u.Scheme {
 	case "s3":
@@ -420,7 +429,7 @@ func (f *Fetcher) fetchFromS3(u url.URL, dest s3target, opts FetchOptions) error
 		// Parse the bucket and key from the ARN Resource.
 		// Also set the region for accesspoints.
 		// S3 bucket ARNs don't include the region field.
-		bucket, key, region, err = f.parseARN(fullURL)
+		bucket, key, region, regionHint, err = f.parseARN(fullURL)
 		if err != nil {
 			return err
 		}
@@ -430,14 +439,25 @@ func (f *Fetcher) fetchFromS3(u url.URL, dest s3target, opts FetchOptions) error
 
 	// Determine the partition and region this bucket is in
 	if region == "" {
-		regionHint := "us-east-1"
-		if f.S3RegionHint != "" {
+		// We didn't get an accesspoint ARN, so we don't know the
+		// region directly.  Maybe we computed a region hint from
+		// the ARN partition field?
+		if regionHint == "" {
+			// Nope; we got an unknown ARN partition value or an
+			// s3:// URL.  Maybe we're running in AWS and can
+			// assume the same partition we're running in?
 			regionHint = f.S3RegionHint
 		}
+		if regionHint == "" {
+			// Nope; assume aws partition.
+			regionHint = "us-east-1"
+		}
+		// Use the region hint to ask the correct partition for the
+		// bucket's region.
 		region, err = s3manager.GetBucketRegion(ctx, sess, bucket, regionHint)
 		if err != nil {
 			if aerr, ok := err.(awserr.Error); ok && aerr.Code() == "NotFound" {
-				return fmt.Errorf("couldn't determine the region for bucket %q: %v", u.Host, err)
+				return fmt.Errorf("couldn't determine the region for bucket %q: %v", bucket, err)
 			}
 			return err
 		}
@@ -547,21 +567,24 @@ func (f *Fetcher) decompressCopyHashAndVerify(dest io.Writer, src io.Reader, opt
 }
 
 // parseARN is a custom wrapper around arn.Parse(); it takes arnURL, a full ARN URL,
-// and returns a bucket, a key, a potentially empty region, or an error if the ARN
-// is invalid or not for an S3 object.
+// and returns a bucket, a key, a potentially empty region, and a
+// potentially empty region hint for use in region detection; or an error if
+// the ARN is invalid or not for an S3 object.
 // If the given arnURL is an accesspoint ARN, the region is set.
 // The region is empty for S3 bucket ARNs because they don't include the region field.
-func (f *Fetcher) parseARN(arnURL string) (string, string, string, error) {
+func (f *Fetcher) parseARN(arnURL string) (string, string, string, string, error) {
 	if !arn.IsARN(arnURL) {
-		return "", "", "", configErrors.ErrInvalidS3ARN
+		return "", "", "", "", configErrors.ErrInvalidS3ARN
 	}
 	s3arn, err := arn.Parse(arnURL)
 	if err != nil {
-		return "", "", "", err
+		return "", "", "", "", err
 	}
 	if s3arn.Service != "s3" {
-		return "", "", "", configErrors.ErrInvalidS3ARN
+		return "", "", "", "", configErrors.ErrInvalidS3ARN
 	}
+	// empty if unrecognized partition
+	regionHint := awsPartitionRegionHints[s3arn.Partition]
 	// Split the ARN bucket (or accesspoint) and key by separating on slashes.
 	// See https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#arns-paths for more info.
 	urlSplit := strings.Split(arnURL, "/")
@@ -570,7 +593,7 @@ func (f *Fetcher) parseARN(arnURL string) (string, string, string, error) {
 	if strings.HasPrefix(s3arn.Resource, "accesspoint/") {
 		// urlSplit must consist of arn, name of accesspoint, and key
 		if len(urlSplit) < 3 {
-			return "", "", "", configErrors.ErrInvalidS3ARN
+			return "", "", "", "", configErrors.ErrInvalidS3ARN
 		}
 
 		// When using GetObjectInput with an access point,
@@ -579,11 +602,11 @@ func (f *Fetcher) parseARN(arnURL string) (string, string, string, error) {
 		// https://docs.aws.amazon.com/AmazonS3/latest/userguide/using-access-points.html
 		bucket := strings.Join(urlSplit[:2], "/")
 		key := strings.Join(urlSplit[2:], "/")
-		return bucket, key, s3arn.Region, nil
+		return bucket, key, s3arn.Region, regionHint, nil
 	}
 	// urlSplit must consist of name of bucket and key
 	if len(urlSplit) < 2 {
-		return "", "", "", configErrors.ErrInvalidS3ARN
+		return "", "", "", "", configErrors.ErrInvalidS3ARN
 	}
 
 	// Parse out the bucket name in order to find the region with s3manager.GetBucketRegion.
@@ -592,5 +615,5 @@ func (f *Fetcher) parseARN(arnURL string) (string, string, string, error) {
 	bucketUrlSplit := strings.Split(urlSplit[0], ":")
 	bucket := bucketUrlSplit[len(bucketUrlSplit)-1]
 	key := strings.Join(urlSplit[1:], "/")
-	return bucket, key, "", nil
+	return bucket, key, "", regionHint, nil
 }

internal/resource/url_test.go

@@ -21,6 +21,8 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/coreos/ignition/v2/config/shared/errors"
 	"github.com/coreos/ignition/v2/internal/log"
 	"github.com/coreos/ignition/v2/internal/util"
@@ -243,50 +245,74 @@ func TestFetchOffline(t *testing.T) {
 }
 
 func TestParseARN(t *testing.T) {
-	type in struct {
-		url string
-	}
-	type out struct {
-		bucket string
-		key    string
-		region string
-		err    error
-	}
 	tests := []struct {
-		in  in
-		out out
+		url        string
+		bucket     string
+		key        string
+		region     string
+		regionHint string
+		err        error
 	}{
 		{
-			in: in{
-				"arn:aws:iam:us-west-2:123456789012:resource",
-			},
-			out: out{
-				err: errors.ErrInvalidS3ARN,
-			},
+			url: "arn:aws:iam:us-west-2:123456789012:resource",
+			err: errors.ErrInvalidS3ARN,
 		},
 		{
-			in: in{
-				"arn:aws:s3:::kola-fixtures/resources/anonymous",
-			},
-			out: out{
-				bucket: "kola-fixtures", key: "resources/anonymous",
-			},
+			url:        "arn:aws:s3:::kola-fixtures/resources/anonymous",
+			bucket:     "kola-fixtures",
+			key:        "resources/anonymous",
+			regionHint: "us-east-1",
 		},
 		{
-			in: in{
-				"arn:aws:s3:us-west-2:123456789012:accesspoint/test/object",
-			},
-			out: out{
-				bucket: "arn:aws:s3:us-west-2:123456789012:accesspoint/test", key: "object", region: "us-west-2",
-			},
+			url:        "arn:aws-cn:s3:::kola-fixtures/resources/anonymous",
+			bucket:     "kola-fixtures",
+			key:        "resources/anonymous",
+			regionHint: "cn-north-1",
 		},
 		{
-			in: in{
-				"arn:aws:s3:us-west-2:123456789012:accesspoint/test/path/object",
-			},
-			out: out{
-				bucket: "arn:aws:s3:us-west-2:123456789012:accesspoint/test", key: "path/object", region: "us-west-2",
-			},
+			url:        "arn:aws-us-gov:s3:::kola-fixtures/resources/anonymous",
+			bucket:     "kola-fixtures",
+			key:        "resources/anonymous",
+			regionHint: "us-gov-west-1",
+		},
+		{
+			url:    "arn:invalid:s3:::kola-fixtures/resources/anonymous",
+			bucket: "kola-fixtures",
+			key:    "resources/anonymous",
+		},
+		{
+			url:        "arn:aws:s3:us-west-2:123456789012:accesspoint/test/object",
+			bucket:     "arn:aws:s3:us-west-2:123456789012:accesspoint/test",
+			key:        "object",
+			region:     "us-west-2",
+			regionHint: "us-east-1",
+		},
+		{
+			url:        "arn:aws-cn:s3:cn-northwest-1:123456789012:accesspoint/test/object",
+			bucket:     "arn:aws-cn:s3:cn-northwest-1:123456789012:accesspoint/test",
+			key:        "object",
+			region:     "cn-northwest-1",
+			regionHint: "cn-north-1",
+		},
+		{
+			url:        "arn:aws-us-gov:s3:us-gov-east-1:123456789012:accesspoint/test/object",
+			bucket:     "arn:aws-us-gov:s3:us-gov-east-1:123456789012:accesspoint/test",
+			key:        "object",
+			region:     "us-gov-east-1",
+			regionHint: "us-gov-west-1",
+		},
+		{
+			url:    "arn:invalid:s3:us-west-2:123456789012:accesspoint/test/object",
+			bucket: "arn:invalid:s3:us-west-2:123456789012:accesspoint/test",
+			key:    "object",
+			region: "us-west-2",
+		},
+		{
+			url:        "arn:aws:s3:us-west-2:123456789012:accesspoint/test/path/object",
+			bucket:     "arn:aws:s3:us-west-2:123456789012:accesspoint/test",
+			key:        "path/object",
+			region:     "us-west-2",
+			regionHint: "us-east-1",
 		},
 	}
 
@@ -296,22 +322,11 @@ func TestParseARN(t *testing.T) {
 	}
 
 	for i, test := range tests {
-		bucket, key, region, err := f.parseARN(test.in.url)
-		if !reflect.DeepEqual(test.out.err, err) {
-			t.Errorf("#%d: expected error %+v, got %+v", i, test.out.err, err)
-			continue
-		}
-		if test.out.err == nil && !reflect.DeepEqual(test.out.bucket, bucket) {
-			t.Errorf("#%d: expected output %+v, got %+v", i, test.out.bucket, bucket)
-			continue
-		}
-		if test.out.err == nil && !reflect.DeepEqual(test.out.key, key) {
-			t.Errorf("#%d: expected output %+v, got %+v", i, test.out.key, key)
-			continue
-		}
-		if test.out.err == nil && !reflect.DeepEqual(test.out.region, region) {
-			t.Errorf("#%d: expected output %+v, got %+v", i, test.out.region, region)
-			continue
-		}
+		bucket, key, region, regionHint, err := f.parseARN(test.url)
+		assert.Equal(t, test.err, err, "#%d: bad err", i)
+		assert.Equal(t, test.bucket, bucket, "#%d: bad bucket", i)
+		assert.Equal(t, test.key, key, "#%d: bad key", i)
+		assert.Equal(t, test.region, region, "#%d: bad region", i)
+		assert.Equal(t, test.regionHint, regionHint, "#%d: bad region hint", i)
 	}
 }