From 79093da6adc6cb35221ea4a170187357d133e923 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Tue, 18 Sep 2018 08:29:20 -0700 Subject: [PATCH] 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 | 1 + builder/amazon/common/ami_config.go | 83 +++++++++++++----------- builder/amazon/common/ami_config_test.go | 52 +++++++++------ builder/amazon/ebs/builder_test.go | 2 + builder/amazon/ebsvolume/builder_test.go | 1 + builder/amazon/instance/builder_test.go | 9 +++ 6 files changed, 90 insertions(+), 58 deletions(-) diff --git a/builder/amazon/chroot/builder_test.go b/builder/amazon/chroot/builder_test.go index f7c6c8d43..b4bb0ab76 100644 --- a/builder/amazon/chroot/builder_test.go +++ b/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) diff --git a/builder/amazon/common/ami_config.go b/builder/amazon/common/ami_config.go index b26ce9320..58c6b8827 100644 --- a/builder/amazon/common/ami_config.go +++ b/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 { - 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, 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 - } + ec2conn := getValidationSession() + 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")) @@ -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 +} diff --git a/builder/amazon/common/ami_config_test.go b/builder/amazon/common/ami_config_test.go index df66d6feb..0e4f28e22 100644 --- a/builder/amazon/common/ami_config_test.go +++ b/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: ®ionString} return &ec2.DescribeRegionsOutput{ - Regions: []*ec2.Region{®}, + 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 { - t.Fatalf("shouldn't have err: %s", err) + c.AMISkipRegionValidation = true + + 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 { - t.Fatalf("shouldn't have err: %s", err) + if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 { + 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 { - t.Fatalf("bad: %s", err) + if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 { + 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 { - t.Fatal("shouldn't have error") + if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 { + 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") } diff --git a/builder/amazon/ebs/builder_test.go b/builder/amazon/ebs/builder_test.go index 4630a36cb..a8200b5a8 100644 --- a/builder/amazon/ebs/builder_test.go +++ b/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) diff --git a/builder/amazon/ebsvolume/builder_test.go b/builder/amazon/ebsvolume/builder_test.go index 7c209f0a4..85c8ae833 100644 --- a/builder/amazon/ebsvolume/builder_test.go +++ b/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) diff --git a/builder/amazon/instance/builder_test.go b/builder/amazon/instance/builder_test.go index 1ad77d82b..0c2a05145 100644 --- a/builder/amazon/instance/builder_test.go +++ b/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()