skip region validation in tests that don't care; refactor Prepare func so we can test region validation logic with a mock

builder/amazon/chroot/builder_test.go

@@ -30,6 +30,7 @@ func TestBuilderPrepare_AMIName(t *testing.T) {
 
 	// Test good
 	config["ami_name"] = "foo"
+	config["skip_region_validation"] = true
 	warnings, err := b.Prepare(config)
 	if len(warnings) > 0 {
 		t.Fatalf("bad: %#v", warnings)

builder/amazon/common/ami_config.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"log"
 
+	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 	"github.com/hashicorp/packer/template/interpolate"
 )
 
@@ -56,44 +57,8 @@ func (c *AMIConfig) Prepare(accessConfig *AccessConfig, ctx *interpolate.Context
 		}
 	}
 
-	if len(c.AMIRegions) > 0 {
+	ec2conn := getValidationSession()
-		regionSet := make(map[string]struct{})
+	errs = c.prepareRegions(ec2conn, accessConfig, errs)
-		regions := make([]string, 0, len(c.AMIRegions))
-
-		for _, region := range c.AMIRegions {
-			// If we already saw the region, then don't look again
-			if _, ok := regionSet[region]; ok {
-				continue
-			}
-
-			// Mark that we saw the region
-			regionSet[region] = struct{}{}
-
-			if !c.AMISkipRegionValidation {
-				// Verify the region is real
-				if valid := ValidateRegion(region, getValidationSession()); !valid {
-					errs = append(errs, fmt.Errorf("Unknown region: %s", region))
-				}
-			}
-
-			// 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 {
-				if _, ok := c.AMIRegionKMSKeyIDs[region]; !ok {
-					errs = append(errs, fmt.Errorf("Region %s is in ami_regions but not in region_kms_key_ids", region))
-				}
-			}
-			if (accessConfig != nil) && (region == accessConfig.RawRegion) {
-				// make sure we don't try to copy to the region we originally
-				// create the AMI in.
-				log.Printf("Cannot copy AMI to AWS session region '%s', deleting it from `ami_regions`.", region)
-				continue
-			}
-			regions = append(regions, region)
-		}
-
-		c.AMIRegions = regions
-	}
 
 	if len(c.AMIUsers) > 0 && c.AMIEncryptBootVolume {
 		errs = append(errs, fmt.Errorf("Cannot share AMI with encrypted boot volume"))
@@ -130,3 +95,45 @@ func (c *AMIConfig) Prepare(accessConfig *AccessConfig, ctx *interpolate.Context
 
 	return nil
 }
+
+func (c *AMIConfig) prepareRegions(ec2conn ec2iface.EC2API, accessConfig *AccessConfig, errs []error) []error {
+	if len(c.AMIRegions) > 0 {
+		regionSet := make(map[string]struct{})
+		regions := make([]string, 0, len(c.AMIRegions))
+
+		for _, region := range c.AMIRegions {
+			// If we already saw the region, then don't look again
+			if _, ok := regionSet[region]; ok {
+				continue
+			}
+
+			// Mark that we saw the region
+			regionSet[region] = struct{}{}
+
+			if !c.AMISkipRegionValidation {
+				// Verify the region is real
+				if valid := ValidateRegion(region, ec2conn); !valid {
+					errs = append(errs, fmt.Errorf("Unknown region: %s", region))
+				}
+			}
+
+			// 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 {
+				if _, ok := c.AMIRegionKMSKeyIDs[region]; !ok {
+					errs = append(errs, fmt.Errorf("Region %s is in ami_regions but not in region_kms_key_ids", region))
+				}
+			}
+			if (accessConfig != nil) && (region == accessConfig.RawRegion) {
+				// make sure we don't try to copy to the region we originally
+				// create the AMI in.
+				log.Printf("Cannot copy AMI to AWS session region '%s', deleting it from `ami_regions`.", region)
+				continue
+			}
+			regions = append(regions, region)
+		}
+
+		c.AMIRegions = regions
+	}
+	return errs
+}

builder/amazon/common/ami_config_test.go

@@ -1,9 +1,11 @@
 package common
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/ec2/ec2iface"
 )
@@ -22,6 +24,7 @@ func getFakeAccessConfig(region string) *AccessConfig {
 
 func TestAMIConfigPrepare_name(t *testing.T) {
 	c := testAMIConfig()
+	c.AMISkipRegionValidation = true
 	if err := c.Prepare(nil, nil); err != nil {
 		t.Fatalf("shouldn't have err: %s", err)
 	}
@@ -37,35 +40,41 @@ type mockEC2Client struct {
 }
 
 func (m *mockEC2Client) DescribeRegions(*ec2.DescribeRegionsInput) (*ec2.DescribeRegionsOutput, error) {
-	// Describes a region.
-	regionString := "us-east-1"
-	reg := ec2.Region{RegionName: &regionString}
 	return &ec2.DescribeRegionsOutput{
-		Regions: []*ec2.Region{&reg},
+		Regions: []*ec2.Region{
+			{RegionName: aws.String("us-east-1")},
+			{RegionName: aws.String("us-east-2")},
+			{RegionName: aws.String("us-west-1")},
+		},
 	}, nil
 }
 
 func TestAMIConfigPrepare_regions(t *testing.T) {
 	c := testAMIConfig()
 	c.AMIRegions = nil
-	if err := c.Prepare(nil, nil); err != nil {
+	c.AMISkipRegionValidation = true
-		t.Fatalf("shouldn't have err: %s", err)
+
+	var errs []error
+	mockConn := &mockEC2Client{}
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
+		t.Fatalf("shouldn't have err: %#v", errs)
 	}
 
-	mockConn := &mockEC2Client{}
+	c.AMISkipRegionValidation = false
 	c.AMIRegions = listEC2Regions(mockConn)
-	if err := c.Prepare(nil, nil); err != nil {
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
-		t.Fatalf("shouldn't have err: %s", err)
+		t.Fatalf("shouldn't have err: %#v", errs)
 	}
 
 	c.AMIRegions = []string{"foo"}
-	if err := c.Prepare(nil, nil); err == nil {
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) == 0 {
 		t.Fatal("should have error")
 	}
+	errs = errs[:0]
 
 	c.AMIRegions = []string{"us-east-1", "us-west-1", "us-east-1"}
-	if err := c.Prepare(nil, nil); err != nil {
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
-		t.Fatalf("bad: %s", err)
+		t.Fatalf("bad: %s", errs[0])
 	}
 
 	expected := []string{"us-east-1", "us-west-1"}
@@ -75,7 +84,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 
 	c.AMIRegions = []string{"custom"}
 	c.AMISkipRegionValidation = true
-	if err := c.Prepare(nil, nil); err != nil {
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
 		t.Fatal("shouldn't have error")
 	}
 	c.AMISkipRegionValidation = false
@@ -86,8 +95,8 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-west-1": "789-012-3456",
 		"us-east-2": "456-789-0123",
 	}
-	if err := c.Prepare(nil, nil); err != nil {
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
-		t.Fatal("shouldn't have error")
+		t.Fatal(fmt.Sprintf("shouldn't have error: %s", errs[0]))
 	}
 
 	c.AMIRegions = []string{"us-east-1", "us-east-2", "us-west-1"}
@@ -96,7 +105,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-west-1": "789-012-3456",
 		"us-east-2": "",
 	}
-	if err := c.Prepare(nil, nil); err != nil {
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
 		t.Fatal("should have passed; we are able to use default KMS key if not sharing")
 	}
 
@@ -107,7 +116,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-west-1": "789-012-3456",
 		"us-east-2": "",
 	}
-	if err := c.Prepare(nil, nil); err == nil {
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
 		t.Fatal("should have an error b/c can't use default KMS key if sharing")
 	}
 
@@ -117,7 +126,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-west-1": "789-012-3456",
 		"us-east-2": "456-789-0123",
 	}
-	if err := c.Prepare(nil, nil); err == nil {
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
 		t.Fatal("should have error b/c theres a region in the key map that isn't in ami_regions")
 	}
 
@@ -126,9 +135,12 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-east-1": "123-456-7890",
 		"us-west-1": "789-012-3456",
 	}
+
+	c.AMISkipRegionValidation = true
 	if err := c.Prepare(nil, 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
 
 	c.SnapshotUsers = []string{"foo", "bar"}
 	c.AMIKmsKeyId = "123-abc-456"
@@ -138,7 +150,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 		"us-east-1": "123-456-7890",
 		"us-west-1": "",
 	}
-	if err := c.Prepare(nil, nil); err == nil {
+	if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 {
 		t.Fatal("should have error b/c theres a region in in ami_regions that isn't in the key map")
 	}
 
@@ -146,7 +158,7 @@ func TestAMIConfigPrepare_regions(t *testing.T) {
 	accessConf := getFakeAccessConfig("us-east-1")
 	c.AMIRegions = []string{"us-east-1", "us-west-1", "us-east-2"}
 	c.AMIRegionKMSKeyIDs = nil
-	if err := c.Prepare(accessConf, nil); err != nil {
+	if errs = c.prepareRegions(mockConn, accessConf, errs); len(errs) > 0 {
 		t.Fatal("should allow user to have the raw region in ami_regions")
 	}
 

builder/amazon/ebs/builder_test.go

@@ -47,6 +47,7 @@ func TestBuilderPrepare_AMIName(t *testing.T) {
 
 	// Test good
 	config["ami_name"] = "foo"
+	config["skip_region_validation"] = true
 	warnings, err := b.Prepare(config)
 	if len(warnings) > 0 {
 		t.Fatalf("bad: %#v", warnings)
@@ -99,6 +100,7 @@ func TestBuilderPrepare_InvalidShutdownBehavior(t *testing.T) {
 
 	// Test good
 	config["shutdown_behavior"] = "terminate"
+	config["skip_region_validation"] = true
 	warnings, err := b.Prepare(config)
 	if len(warnings) > 0 {
 		t.Fatalf("bad: %#v", warnings)

builder/amazon/ebsvolume/builder_test.go

@@ -61,6 +61,7 @@ func TestBuilderPrepare_InvalidShutdownBehavior(t *testing.T) {
 
 	// Test good
 	config["shutdown_behavior"] = "terminate"
+	config["skip_region_validation"] = true
 	warnings, err := b.Prepare(config)
 	if len(warnings) > 0 {
 		t.Fatalf("bad: %#v", warnings)

builder/amazon/instance/builder_test.go

@@ -41,6 +41,8 @@ func TestBuilder_ImplementsBuilder(t *testing.T) {
 func TestBuilderPrepare_AccountId(t *testing.T) {
 	b := &Builder{}
 	config, tempfile := testConfig()
+	config["skip_region_validation"] = true
+
 	defer os.Remove(tempfile.Name())
 	defer tempfile.Close()
 
@@ -84,6 +86,7 @@ func TestBuilderPrepare_AMIName(t *testing.T) {
 
 	// Test good
 	config["ami_name"] = "foo"
+	config["skip_region_validation"] = true
 	warnings, err := b.Prepare(config)
 	if len(warnings) > 0 {
 		t.Fatalf("bad: %#v", warnings)
@@ -118,6 +121,7 @@ func TestBuilderPrepare_AMIName(t *testing.T) {
 func TestBuilderPrepare_BundleDestination(t *testing.T) {
 	b := &Builder{}
 	config, tempfile := testConfig()
+	config["skip_region_validation"] = true
 	defer os.Remove(tempfile.Name())
 	defer tempfile.Close()
 
@@ -138,6 +142,7 @@ func TestBuilderPrepare_BundleDestination(t *testing.T) {
 func TestBuilderPrepare_BundlePrefix(t *testing.T) {
 	b := &Builder{}
 	config, tempfile := testConfig()
+	config["skip_region_validation"] = true
 	defer os.Remove(tempfile.Name())
 	defer tempfile.Close()
 
@@ -174,6 +179,7 @@ func TestBuilderPrepare_InvalidKey(t *testing.T) {
 func TestBuilderPrepare_S3Bucket(t *testing.T) {
 	b := &Builder{}
 	config, tempfile := testConfig()
+	config["skip_region_validation"] = true
 	defer os.Remove(tempfile.Name())
 	defer tempfile.Close()
 
@@ -199,6 +205,7 @@ func TestBuilderPrepare_S3Bucket(t *testing.T) {
 func TestBuilderPrepare_X509CertPath(t *testing.T) {
 	b := &Builder{}
 	config, tempfile := testConfig()
+	config["skip_region_validation"] = true
 	defer os.Remove(tempfile.Name())
 	defer tempfile.Close()
 
@@ -240,6 +247,7 @@ func TestBuilderPrepare_X509CertPath(t *testing.T) {
 func TestBuilderPrepare_X509KeyPath(t *testing.T) {
 	b := &Builder{}
 	config, tempfile := testConfig()
+	config["skip_region_validation"] = true
 	defer os.Remove(tempfile.Name())
 	defer tempfile.Close()
 
@@ -281,6 +289,7 @@ func TestBuilderPrepare_X509KeyPath(t *testing.T) {
 func TestBuilderPrepare_X509UploadPath(t *testing.T) {
 	b := &Builder{}
 	config, tempfile := testConfig()
+	config["skip_region_validation"] = true
 	defer os.Remove(tempfile.Name())
 	defer tempfile.Close()