Merge pull request #6904 from hashicorp/fix_6890

Fix 6890

builder/amazon/common/access_config.go

@@ -11,6 +11,8 @@ import (
 	"github.com/aws/aws-sdk-go/aws/credentials"
 	"github.com/aws/aws-sdk-go/aws/ec2metadata"
 	"github.com/aws/aws-sdk-go/aws/session"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 	"github.com/hashicorp/go-cleanhttp"
 	"github.com/hashicorp/packer/template/interpolate"
 )
@@ -28,6 +30,8 @@ type AccessConfig struct {
 	SkipMetadataApiCheck bool   `mapstructure:"skip_metadata_api_check"`
 	Token                string `mapstructure:"token"`
 	session              *session.Session
+
+	getEC2Connection func() ec2iface.EC2API
 }
 
 // Config returns a valid aws.Config object for access to AWS services, or
@@ -38,7 +42,6 @@ func (c *AccessConfig) Session() (*session.Session, error) {
 	}
 
 	config := aws.NewConfig().WithCredentialsChainVerboseErrors(true)
-
 	staticCreds := credentials.NewStaticCredentials(c.AccessKey, c.SecretKey, c.Token)
 	if _, err := staticCreds.Get(); err != credentials.ErrStaticCredentialsEmpty {
 		config.WithCredentials(staticCreds)
@@ -148,8 +151,7 @@ func (c *AccessConfig) Prepare(ctx *interpolate.Context) []error {
 	}
 
 	if c.RawRegion != "" && !c.SkipValidation {
-		ec2conn := getValidationSession()
+		err := c.ValidateRegion(c.RawRegion)
-		err := ValidateRegion(c.RawRegion, ec2conn)
 		if err != nil {
 			errs = append(errs, fmt.Errorf("error validating region: %s", err.Error()))
 		}
@@ -157,3 +159,14 @@ func (c *AccessConfig) Prepare(ctx *interpolate.Context) []error {
 
 	return errs
 }
+
+func (c *AccessConfig) NewEC2Connection() (ec2iface.EC2API, error) {
+	if c.getEC2Connection != nil {
+		return c.getEC2Connection(), nil
+	}
+	sess, err := c.Session()
+	if err != nil {
+		return nil, err
+	}
+	return ec2.New(sess), nil
+}

builder/amazon/common/access_config_test.go

@@ -5,31 +5,34 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/session"
+	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 )
 
 func testAccessConfig() *AccessConfig {
-	return &AccessConfig{}
+	return &AccessConfig{
+		getEC2Connection: func() ec2iface.EC2API {
+			return &mockEC2Client{}
+		},
+	}
 }
 
 func TestAccessConfigPrepare_Region(t *testing.T) {
 	c := testAccessConfig()
 
-	mockConn := &mockEC2Client{}
-
 	c.RawRegion = "us-east-12"
-	err := ValidateRegion(c.RawRegion, mockConn)
+	err := c.ValidateRegion(c.RawRegion)
 	if err == nil {
 		t.Fatalf("should have region validation err: %s", c.RawRegion)
 	}
 
 	c.RawRegion = "us-east-1"
-	err = ValidateRegion(c.RawRegion, mockConn)
+	err = c.ValidateRegion(c.RawRegion)
 	if err != nil {
 		t.Fatalf("shouldn't have region validation err: %s", c.RawRegion)
 	}
 
 	c.RawRegion = "custom"
-	err = ValidateRegion(c.RawRegion, mockConn)
+	err = c.ValidateRegion(c.RawRegion)
 	if err == nil {
 		t.Fatalf("should have region validation err: %s", c.RawRegion)
 	}

builder/amazon/common/ami_config.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"log"
 
-	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 	"github.com/hashicorp/packer/template/interpolate"
 )
 
@@ -57,8 +56,7 @@ func (c *AMIConfig) Prepare(accessConfig *AccessConfig, ctx *interpolate.Context
 		}
 	}
 
-	ec2conn := getValidationSession()
+	errs = append(errs, c.prepareRegions(accessConfig)...)
-	errs = c.prepareRegions(ec2conn, accessConfig, errs)
 
 	if len(c.AMIUsers) > 0 && c.AMIEncryptBootVolume {
 		errs = append(errs, fmt.Errorf("Cannot share AMI with encrypted boot volume"))
@@ -96,8 +94,16 @@ func (c *AMIConfig) Prepare(accessConfig *AccessConfig, ctx *interpolate.Context
 	return nil
 }
 
-func (c *AMIConfig) prepareRegions(ec2conn ec2iface.EC2API, accessConfig *AccessConfig, errs []error) []error {
+func (c *AMIConfig) prepareRegions(accessConfig *AccessConfig) (errs []error) {
 	if len(c.AMIRegions) > 0 {
+		if !c.AMISkipRegionValidation {
+			// Verify the regions are real
+			err := accessConfig.ValidateRegion(c.AMIRegions...)
+			if err != nil {
+				errs = append(errs, fmt.Errorf("error validating regions: %v", err))
+			}
+		}
+
 		regionSet := make(map[string]struct{})
 		regions := make([]string, 0, len(c.AMIRegions))
 
@@ -110,14 +116,6 @@ func (c *AMIConfig) prepareRegions(ec2conn ec2iface.EC2API, accessConfig *Access
 			// Mark that we saw the region
 			regionSet[region] = struct{}{}
 
-			if !c.AMISkipRegionValidation {
-				// Verify the region is real
-				err := ValidateRegion(region, ec2conn)
-				if err != nil {
-					errs = append(errs, fmt.Errorf("error validating region: %s", err.Error()))
-				}
-			}
-
 			// Make sure that if we have region_kms_key_ids defined,
 			// the regions in ami_regions are also in region_kms_key_ids
 			if len(c.AMIRegionKMSKeyIDs) > 0 {

builder/amazon/common/ami_config_test.go

@@ -17,20 +17,21 @@ func testAMIConfig() *AMIConfig {
 }
 
 func getFakeAccessConfig(region string) *AccessConfig {
-	return &AccessConfig{
+	c := testAccessConfig()
-		RawRegion: region,
+	c.RawRegion = region
-	}
+	return c
 }
 
 func TestAMIConfigPrepare_name(t *testing.T) {
 	c := testAMIConfig()
+	accessConf := testAccessConfig()
 	c.AMISkipRegionValidation = true
-	if err := c.Prepare(nil, nil); err != nil {
+	if err := c.Prepare(accessConf, nil); err != nil {
 		t.Fatalf("shouldn't have err: %s", err)
 	}
 
 	c.AMIName = ""
-	if err := c.Prepare(nil, nil); err == nil {
+	if err := c.Prepare(accessConf, nil); err == nil {
 		t.Fatal("should have error")
 	}
 }
@@ -56,8 +57,9 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 
 	var errs []error
 	var err error
+	accessConf := testAccessConfig()
 	mockConn := &mockEC2Client{}
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatalf("shouldn't have err: %#v", errs)
 	}
 
@@ -66,18 +68,18 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 	if err != nil {
 		t.Fatalf("shouldn't have err: %s", err.Error())
 	}
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatalf("shouldn't have err: %#v", errs)
 	}
 
 	c.AMIRegions = []string{"foo"}
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) == 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) == 0 {
 		t.Fatal("should have error")
 	}
 	errs = errs[:0]
 
 	c.AMIRegions = []string{"us-east-1", "us-west-1", "us-east-1"}
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatalf("bad: %s", errs[0])
 	}
 
@@ -88,7 +90,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 
 	c.AMIRegions = []string{"custom"}
 	c.AMISkipRegionValidation = true
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatal("shouldn't have error")
 	}
 	c.AMISkipRegionValidation = false
@@ -99,7 +101,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-west-1": "789-012-3456",
 		"us-east-2": "456-789-0123",
 	}
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatal(fmt.Sprintf("shouldn't have error: %s", errs[0]))
 	}
 
@@ -109,7 +111,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-west-1": "789-012-3456",
 		"us-east-2": "",
 	}
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatal("should have passed; we are able to use default KMS key if not sharing")
 	}
 
@@ -120,7 +122,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-west-1": "789-012-3456",
 		"us-east-2": "",
 	}
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatal("should have an error b/c can't use default KMS key if sharing")
 	}
 
@@ -130,7 +132,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-west-1": "789-012-3456",
 		"us-east-2": "456-789-0123",
 	}
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatal("should have error b/c theres a region in the key map that isn't in ami_regions")
 	}
 
@@ -141,7 +143,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 	}
 
 	c.AMISkipRegionValidation = true
-	if err := c.Prepare(nil, nil); err == nil {
+	if err := c.Prepare(accessConf, nil); err == nil {
 		t.Fatal("should have error b/c theres a region in in ami_regions that isn't in the key map")
 	}
 	c.AMISkipRegionValidation = false
@@ -154,15 +156,15 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-east-1": "123-456-7890",
 		"us-west-1": "",
 	}
-	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatal("should have error b/c theres a region in in ami_regions that isn't in the key map")
 	}
 
 	// allow rawregion to exist in ami_regions list.
-	accessConf := getFakeAccessConfig("us-east-1")
+	accessConf = getFakeAccessConfig("us-east-1")
 	c.AMIRegions = []string{"us-east-1", "us-west-1", "us-east-2"}
 	c.AMIRegionKMSKeyIDs = nil
-	if errs = c.prepareRegions(mockConn, accessConf, errs); len(errs) > 0 {
+	if errs = c.prepareRegions(accessConf); len(errs) > 0 {
 		t.Fatal("should allow user to have the raw region in ami_regions")
 	}
 
@@ -170,25 +172,31 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 
 func TestAMIConfigPrepare_Share_EncryptedBoot(t *testing.T) {
 	c := testAMIConfig()
+	c.AMISkipRegionValidation = true
 	c.AMIUsers = []string{"testAccountID"}
 	c.AMIEncryptBootVolume = true
 
+	accessConf := testAccessConfig()
+
 	c.AMIKmsKeyId = ""
-	if err := c.Prepare(nil, nil); err == nil {
+	if err := c.Prepare(accessConf, nil); err == nil {
 		t.Fatal("shouldn't be able to share ami with encrypted boot volume")
 	}
 
 	c.AMIKmsKeyId = "89c3fb9a-de87-4f2a-aedc-fddc5138193c"
-	if err := c.Prepare(nil, nil); err == nil {
+	if err := c.Prepare(accessConf, nil); err == nil {
 		t.Fatal("shouldn't be able to share ami with encrypted boot volume")
 	}
 }
 
 func TestAMINameValidation(t *testing.T) {
 	c := testAMIConfig()
+	c.AMISkipRegionValidation = true
+
+	accessConf := testAccessConfig()
 
 	c.AMIName = "aa"
-	if err := c.Prepare(nil, nil); err == nil {
+	if err := c.Prepare(accessConf, nil); err == nil {
 		t.Fatal("shouldn't be able to have an ami name with less than 3 characters")
 	}
 
@@ -197,22 +205,22 @@ func TestAMINameValidation(t *testing.T) {
 		longAmiName += "a"
 	}
 	c.AMIName = longAmiName
-	if err := c.Prepare(nil, nil); err == nil {
+	if err := c.Prepare(accessConf, nil); err == nil {
 		t.Fatal("shouldn't be able to have an ami name with great than 128 characters")
 	}
 
 	c.AMIName = "+aaa"
-	if err := c.Prepare(nil, nil); err == nil {
+	if err := c.Prepare(accessConf, nil); err == nil {
 		t.Fatal("shouldn't be able to have an ami name with invalid characters")
 	}
 
 	c.AMIName = "fooBAR1()[] ./-'@_"
-	if err := c.Prepare(nil, nil); err != nil {
+	if err := c.Prepare(accessConf, nil); err != nil {
 		t.Fatal("should be able to use all of the allowed AMI characters")
 	}
 
 	c.AMIName = `xyz-base-2017-04-05-1934`
-	if err := c.Prepare(nil, nil); err != nil {
+	if err := c.Prepare(accessConf, nil); err != nil {
 		t.Fatalf("expected `xyz-base-2017-04-05-1934` to pass validation.")
 	}
 

builder/amazon/common/regions.go

@@ -2,20 +2,10 @@ package common
 
 import (
 	"fmt"
-	"github.com/aws/aws-sdk-go/aws/session"
+
-	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 )
 
-func getValidationSession() *ec2.EC2 {
-	sess := session.Must(session.NewSessionWithOptions(session.Options{
-		SharedConfigState: session.SharedConfigEnable,
-	}))
-
-	ec2conn := ec2.New(sess)
-	return ec2conn
-}
-
 func listEC2Regions(ec2conn ec2iface.EC2API) ([]string, error) {
 	var regions []string
 	resultRegions, err := ec2conn.DescribeRegions(nil)
@@ -31,17 +21,33 @@ func listEC2Regions(ec2conn ec2iface.EC2API) ([]string, error) {
 
 // ValidateRegion returns true if the supplied region is a valid AWS
 // region and false if it's not.
-func ValidateRegion(region string, ec2conn ec2iface.EC2API) error {
+func (c *AccessConfig) ValidateRegion(regions ...string) error {
-	regions, err := listEC2Regions(ec2conn)
+	ec2conn, err := c.NewEC2Connection()
 	if err != nil {
 		return err
 	}
 
-	for _, valid := range regions {
+	validRegions, err := listEC2Regions(ec2conn)
-		if region == valid {
+	if err != nil {
-			return nil
+		return err
+	}
+
+	var invalidRegions []string
+	for _, region := range regions {
+		found := false
+		for _, validRegion := range validRegions {
+			if region == validRegion {
+				found = true
+				break
+			}
+		}
+		if !found {
+			invalidRegions = append(invalidRegions, region)
 		}
 	}
 
-	return fmt.Errorf("Invalid region: %s", region)
+	if len(invalidRegions) > 0 {
+		return fmt.Errorf("Invalid region(s): %v", invalidRegions)
+	}
+	return nil
 }