aws/endpoints: Fix resolve endpoint with empty region (#2911)

Fixes the SDK's behavior when attempting to resolve a service's endpoint when no region was provided. Adds legacy support for services that were able to resolve a valid endpoint. No new service will support resolving an endpoint without an region. Fixes #2909
aws · Oct 25, 2019 · 506b1fd · 506b1fd
1 parent 6c4f82c
commit 506b1fd
Show file tree

Hide file tree

Showing 6 changed files with 161 additions and 45 deletions.
diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md
@@ -6,3 +6,6 @@
 * `aws/endpoints`: Add PartitionID to ResolvedEndpoint ([#2902](https://github.com/aws/aws-sdk-go/pull/2902))
 
 ### SDK Bugs
+* `aws/endpoints`: Fix resolve endpoint with empty region ([#2911](https://github.com/aws/aws-sdk-go/pull/2911))
+  * Fixes the SDK's behavior when attempting to resolve a service's endpoint when no region was provided. Adds legacy support for services that were able to resolve a valid endpoint. No new service will support resolving an endpoint without an region.
+  * Fixes [#2909](https://github.com/aws/aws-sdk-go/issues/2909)
diff --git a/aws/endpoints/sts_legacy_regions.go b/aws/endpoints/sts_legacy_regions.go
@@ -5,7 +5,6 @@ var stsLegacyGlobalRegions = map[string]struct{}{
 	"ap-south-1":     {},
 	"ap-southeast-1": {},
 	"ap-southeast-2": {},
-	"aws-global":     {},
 	"ca-central-1":   {},
 	"eu-central-1":   {},
 	"eu-north-1":     {},

diff --git a/aws/endpoints/v3model.go b/aws/endpoints/v3model.go
@@ -75,25 +75,49 @@ func (p partition) canResolveEndpoint(service, region string, strictMatch bool)
 	return p.RegionRegex.MatchString(region)
 }
 
+func allowLegacyEmptyRegion(service string) bool {
+	legacy := map[string]struct{}{
+		"budgets":       {},
+		"ce":            {},
+		"chime":         {},
+		"cloudfront":    {},
+		"ec2metadata":   {},
+		"iam":           {},
+		"importexport":  {},
+		"organizations": {},
+		"route53":       {},
+		"sts":           {},
+		"support":       {},
+		"waf":           {},
+	}
+
+	_, allowed := legacy[service]
+	return allowed
+}
+
 func (p partition) EndpointFor(service, region string, opts ...func(*Options)) (resolved ResolvedEndpoint, err error) {
 	var opt Options
 	opt.Set(opts...)
 
-	if service == "sts" && opt.STSRegionalEndpoint != RegionalSTSEndpoint {
-		if _, ok := stsLegacyGlobalRegions[region]; ok {
-			region = "aws-global"
-		}
-	}
-
 	s, hasService := p.Services[service]
-	if !(hasService || opt.ResolveUnknownService) {
+	if len(service) == 0 || !(hasService || opt.ResolveUnknownService) {
 		// Only return error if the resolver will not fallback to creating
 		// endpoint based on service endpoint ID passed in.
 		return resolved, NewUnknownServiceError(p.ID, service, serviceList(p.Services))
 	}
 
+	if len(region) == 0 && allowLegacyEmptyRegion(service) && len(s.PartitionEndpoint) != 0 {
+		region = s.PartitionEndpoint
+	}
+
+	if service == "sts" && opt.STSRegionalEndpoint != RegionalSTSEndpoint {
+		if _, ok := stsLegacyGlobalRegions[region]; ok {
+			region = "aws-global"
+		}
+	}
+
 	e, hasEndpoint := s.endpointForRegion(region)
-	if !hasEndpoint && opt.StrictMatching {
+	if len(region) == 0 || (!hasEndpoint && opt.StrictMatching) {
 		return resolved, NewUnknownEndpointError(p.ID, service, region, endpointList(s.Endpoints))
 	}
 

diff --git a/aws/endpoints/v3model_test.go b/aws/endpoints/v3model_test.go
@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"reflect"
 	"regexp"
+	"strings"
 	"testing"
 )
 
@@ -539,3 +540,56 @@ func TestEndpointFor_RegionalFlag(t *testing.T) {
 		})
 	}
 }
+
+func TestEndpointFor_EmptyRegion(t *testing.T) {
+	cases := map[string]struct {
+		Service    string
+		Region     string
+		RealRegion string
+		ExpectErr  string
+	}{
+		// Legacy services that previous accepted empty region
+		"budgets":       {Service: "budgets", RealRegion: "aws-global"},
+		"ce":            {Service: "ce", RealRegion: "aws-global"},
+		"chime":         {Service: "chime", RealRegion: "aws-global"},
+		"ec2metadata":   {Service: "ec2metadata", RealRegion: "aws-global"},
+		"iam":           {Service: "iam", RealRegion: "aws-global"},
+		"importexport":  {Service: "importexport", RealRegion: "aws-global"},
+		"organizations": {Service: "organizations", RealRegion: "aws-global"},
+		"route53":       {Service: "route53", RealRegion: "aws-global"},
+		"sts":           {Service: "sts", RealRegion: "aws-global"},
+		"support":       {Service: "support", RealRegion: "aws-global"},
+		"waf":           {Service: "waf", RealRegion: "aws-global"},
+
+		// Other services
+		"s3":           {Service: "s3", Region: "us-east-1", RealRegion: "us-east-1"},
+		"s3 no region": {Service: "s3", ExpectErr: "could not resolve endpoint"},
+	}
+
+	for name, c := range cases {
+		t.Run(name, func(t *testing.T) {
+			actual, err := DefaultResolver().EndpointFor(c.Service, c.Region)
+			if len(c.ExpectErr) != 0 {
+				if e, a := c.ExpectErr, err.Error(); !strings.Contains(a, e) {
+					t.Errorf("expect %q error in %q", e, a)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("expect no error got, %v", err)
+			}
+
+			expect, err := DefaultResolver().EndpointFor(c.Service, c.RealRegion)
+			if err != nil {
+				t.Fatalf("failed to get endpoint for default resolver")
+			}
+			if e, a := expect.URL, actual.URL; e != a {
+				t.Errorf("expect %v URL, got %v", e, a)
+			}
+			if e, a := expect.SigningRegion, actual.SigningRegion; e != a {
+				t.Errorf("expect %v signing region, got %v", e, a)
+			}
+
+		})
+	}
+}
diff --git a/aws/session/session.go b/aws/session/session.go
@@ -614,40 +614,15 @@ func (s *Session) Copy(cfgs ...*aws.Config) *Session {
 // ClientConfig satisfies the client.ConfigProvider interface and is used to
 // configure the service client instances. Passing the Session to the service
 // client's constructor (New) will use this method to configure the client.
-func (s *Session) ClientConfig(serviceName string, cfgs ...*aws.Config) client.Config {
-	// Backwards compatibility, the error will be eaten if user calls ClientConfig
-	// directly. All SDK services will use ClientconfigWithError.
-	cfg, _ := s.clientConfigWithErr(serviceName, cfgs...)
-
-	return cfg
-}
-
-func (s *Session) clientConfigWithErr(serviceName string, cfgs ...*aws.Config) (client.Config, error) {
+func (s *Session) ClientConfig(service string, cfgs ...*aws.Config) client.Config {
 	s = s.Copy(cfgs...)
 
-	var resolved endpoints.ResolvedEndpoint
-	var err error
-
 	region := aws.StringValue(s.Config.Region)
-
-	if endpoint := aws.StringValue(s.Config.Endpoint); len(endpoint) != 0 {
-		resolved.URL = endpoints.AddScheme(endpoint, aws.BoolValue(s.Config.DisableSSL))
-		resolved.SigningRegion = region
-	} else {
-		resolved, err = s.Config.EndpointResolver.EndpointFor(
-			serviceName, region,
-			func(opt *endpoints.Options) {
-				opt.DisableSSL = aws.BoolValue(s.Config.DisableSSL)
-				opt.UseDualStack = aws.BoolValue(s.Config.UseDualStack)
-				// Support for STSRegionalEndpoint where the STSRegionalEndpoint is
-				// provided in envconfig or sharedconfig with envconfig getting precedence.
-				opt.STSRegionalEndpoint = s.Config.STSRegionalEndpoint
-
-				// Support the condition where the service is modeled but its
-				// endpoint metadata is not available.
-				opt.ResolveUnknownService = true
-			},
-		)
+	resolved, err := s.resolveEndpoint(service, region, s.Config)
+	if err != nil && s.Config.Logger != nil {
+		s.Config.Logger.Log(fmt.Sprintf(
+			"ERROR: unable to resolve endpoint for service %q, region %q, err: %v",
+			service, region, err))
 	}
 
 	return client.Config{
@@ -657,7 +632,37 @@ func (s *Session) clientConfigWithErr(serviceName string, cfgs ...*aws.Config) (
 		SigningRegion:      resolved.SigningRegion,
 		SigningNameDerived: resolved.SigningNameDerived,
 		SigningName:        resolved.SigningName,
-	}, err
+	}
+}
+
+func (s *Session) resolveEndpoint(service, region string, cfg *aws.Config) (endpoints.ResolvedEndpoint, error) {
+
+	if ep := aws.StringValue(cfg.Endpoint); len(ep) != 0 {
+		return endpoints.ResolvedEndpoint{
+			URL:           endpoints.AddScheme(ep, aws.BoolValue(cfg.DisableSSL)),
+			SigningRegion: region,
+		}, nil
+	}
+
+	resolved, err := cfg.EndpointResolver.EndpointFor(service, region,
+		func(opt *endpoints.Options) {
+			opt.DisableSSL = aws.BoolValue(cfg.DisableSSL)
+			opt.UseDualStack = aws.BoolValue(cfg.UseDualStack)
+			// Support for STSRegionalEndpoint where the STSRegionalEndpoint is
+			// provided in envConfig or sharedConfig with envConfig getting
+			// precedence.
+			opt.STSRegionalEndpoint = cfg.STSRegionalEndpoint
+
+			// Support the condition where the service is modeled but its
+			// endpoint metadata is not available.
+			opt.ResolveUnknownService = true
+		},
+	)
+	if err != nil {
+		return endpoints.ResolvedEndpoint{}, err
+	}
+
+	return resolved, nil
 }
 
 // ClientConfigNoResolveEndpoint is the same as ClientConfig with the exception
@@ -667,12 +672,9 @@ func (s *Session) ClientConfigNoResolveEndpoint(cfgs ...*aws.Config) client.Conf
 	s = s.Copy(cfgs...)
 
 	var resolved endpoints.ResolvedEndpoint
-
-	region := aws.StringValue(s.Config.Region)
-
 	if ep := aws.StringValue(s.Config.Endpoint); len(ep) > 0 {
 		resolved.URL = endpoints.AddScheme(ep, aws.BoolValue(s.Config.DisableSSL))
-		resolved.SigningRegion = region
+		resolved.SigningRegion = aws.StringValue(s.Config.Region)
 	}
 
 	return client.Config{

diff --git a/aws/session/session_test.go b/aws/session/session_test.go
@@ -15,6 +15,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws/credentials"
 	"github.com/aws/aws-sdk-go/aws/defaults"
 	"github.com/aws/aws-sdk-go/aws/endpoints"
+	"github.com/aws/aws-sdk-go/aws/request"
 	"github.com/aws/aws-sdk-go/service/s3"
 )
 
@@ -149,6 +150,39 @@ func TestSessionClientConfig(t *testing.T) {
 	}
 }
 
+func TestNewSession_ResolveEndpointError(t *testing.T) {
+	logger := mockLogger{Buffer: bytes.NewBuffer(nil)}
+	sess, err := NewSession(defaults.Config(), &aws.Config{
+		Region: aws.String(""),
+		Logger: logger,
+		EndpointResolver: endpoints.ResolverFunc(
+			func(service, region string, opts ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) {
+				return endpoints.ResolvedEndpoint{}, fmt.Errorf("mock error")
+			},
+		),
+	})
+	if err != nil {
+		t.Fatalf("expect no error got %v", err)
+	}
+
+	cfg := sess.ClientConfig("mock service")
+
+	var r request.Request
+	cfg.Handlers.Validate.Run(&r)
+
+	if r.Error == nil {
+		t.Fatalf("expect validation error, got none")
+	}
+
+	if e, a := aws.ErrMissingRegion.Error(), r.Error.Error(); !strings.Contains(a, e) {
+		t.Errorf("expect %v validation error, got %v", e, a)
+	}
+
+	if e, a := "unable to resolve", logger.Buffer.String(); !strings.Contains(a, e) {
+		t.Errorf("expect %v logged, got %v", e, a)
+	}
+}
+
 func TestNewSession_NoCredentials(t *testing.T) {
 	restoreEnvFn := initSessionTestEnv()
 	defer restoreEnvFn()