From 0d55bc46ee6d141976f717996aaa5036ef861cef Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Mon, 17 Jun 2019 14:39:11 -0700 Subject: [PATCH 1/4] add skip_save_build_region flag to fix naming conflicts when building for multiple regions --- builder/amazon/common/ami_config.go | 1 + builder/amazon/common/step_ami_region_copy.go | 9 ++++--- builder/amazon/common/step_pre_validate.go | 9 +++++-- builder/amazon/ebs/builder.go | 24 +++++++++++-------- builder/amazon/ebs/step_create_ami.go | 9 +++---- 5 files changed, 33 insertions(+), 19 deletions(-) diff --git a/builder/amazon/common/ami_config.go b/builder/amazon/common/ami_config.go index 343d7f1bd..2069ab23f 100644 --- a/builder/amazon/common/ami_config.go +++ b/builder/amazon/common/ami_config.go @@ -29,6 +29,7 @@ type AMIConfig struct { SnapshotTags TagMap `mapstructure:"snapshot_tags"` SnapshotUsers []string `mapstructure:"snapshot_users"` SnapshotGroups []string `mapstructure:"snapshot_groups"` + AMISkipBuildRegion bool `mapstructure:"skip_save_build_region"` } func stringInSlice(s []string, searchstr string) bool { diff --git a/builder/amazon/common/step_ami_region_copy.go b/builder/amazon/common/step_ami_region_copy.go index 620715d27..bfc582e5a 100644 --- a/builder/amazon/common/step_ami_region_copy.go +++ b/builder/amazon/common/step_ami_region_copy.go @@ -21,8 +21,9 @@ type StepAMIRegionCopy struct { Name string OriginalRegion string - toDelete string - getRegionConn func(*AccessConfig, string) (ec2iface.EC2API, error) + toDelete string + getRegionConn func(*AccessConfig, string) (ec2iface.EC2API, error) + AMISkipBuildRegion bool } func (s *StepAMIRegionCopy) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { @@ -33,7 +34,9 @@ func (s *StepAMIRegionCopy) Run(ctx context.Context, state multistep.StateBag) m ami := amis[s.OriginalRegion] // Always copy back into original region to preserve the ami name s.toDelete = ami - s.Regions = append(s.Regions, s.OriginalRegion) + if !s.AMISkipBuildRegion { + s.Regions = append(s.Regions, s.OriginalRegion) + } if s.EncryptBootVolume != nil && *s.EncryptBootVolume { // encrypt_boot is true, so we have to copy the temporary diff --git a/builder/amazon/common/step_pre_validate.go b/builder/amazon/common/step_pre_validate.go index f8ba32b16..592f9d539 100644 --- a/builder/amazon/common/step_pre_validate.go +++ b/builder/amazon/common/step_pre_validate.go @@ -18,8 +18,9 @@ import ( // the build before actually doing any time consuming work // type StepPreValidate struct { - DestAmiName string - ForceDeregister bool + DestAmiName string + ForceDeregister bool + AMISkipBuildRegion bool } func (s *StepPreValidate) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { @@ -76,6 +77,10 @@ func (s *StepPreValidate) Run(ctx context.Context, state multistep.StateBag) mul ui.Say("Force Deregister flag found, skipping prevalidating AMI Name") return multistep.ActionContinue } + if s.AMISkipBuildRegion { + ui.Say("skip_build_region was set; not prevalidating AMI name") + return multistep.ActionContinue + } ec2conn := state.Get("ec2").(*ec2.EC2) diff --git a/builder/amazon/ebs/builder.go b/builder/amazon/ebs/builder.go index d847f9d57..fef470ef5 100644 --- a/builder/amazon/ebs/builder.go +++ b/builder/amazon/ebs/builder.go @@ -154,8 +154,9 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack // Build the steps steps := []multistep.Step{ &awscommon.StepPreValidate{ - DestAmiName: b.config.AMIName, - ForceDeregister: b.config.AMIForceDeregister, + DestAmiName: b.config.AMIName, + ForceDeregister: b.config.AMIForceDeregister, + AMISkipBuildRegion: b.config.AMISkipBuildRegion, }, &awscommon.StepSourceAMIInfo{ SourceAmi: b.config.SourceAmi, @@ -220,15 +221,18 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack AMIName: b.config.AMIName, Regions: b.config.AMIRegions, }, - &stepCreateAMI{}, + &stepCreateAMI{ + AMISkipBuildRegion: b.config.AMISkipBuildRegion, + }, &awscommon.StepAMIRegionCopy{ - AccessConfig: &b.config.AccessConfig, - Regions: b.config.AMIRegions, - AMIKmsKeyId: b.config.AMIKmsKeyId, - RegionKeyIds: b.config.AMIRegionKMSKeyIDs, - EncryptBootVolume: b.config.AMIEncryptBootVolume, - Name: b.config.AMIName, - OriginalRegion: *ec2conn.Config.Region, + AccessConfig: &b.config.AccessConfig, + Regions: b.config.AMIRegions, + AMIKmsKeyId: b.config.AMIKmsKeyId, + RegionKeyIds: b.config.AMIRegionKMSKeyIDs, + EncryptBootVolume: b.config.AMIEncryptBootVolume, + Name: b.config.AMIName, + OriginalRegion: *ec2conn.Config.Region, + AMISkipBuildRegion: b.config.AMISkipBuildRegion, }, &awscommon.StepModifyAMIAttributes{ Description: b.config.AMIDescription, diff --git a/builder/amazon/ebs/step_create_ami.go b/builder/amazon/ebs/step_create_ami.go index 68d8c7084..c1d0da0b8 100644 --- a/builder/amazon/ebs/step_create_ami.go +++ b/builder/amazon/ebs/step_create_ami.go @@ -14,7 +14,8 @@ import ( ) type stepCreateAMI struct { - image *ec2.Image + image *ec2.Image + AMISkipBuildRegion bool } func (s *stepCreateAMI) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { @@ -25,9 +26,9 @@ func (s *stepCreateAMI) Run(ctx context.Context, state multistep.StateBag) multi // Create the image amiName := config.AMIName - if config.AMIEncryptBootVolume != nil { - // encrypt_boot was set, so we will create a temporary image - // and then create a copy of it with the correct encrypt_boot + if config.AMIEncryptBootVolume != nil || s.AMISkipBuildRegion { + // Create a temporary image and then create a copy of it with the + // correct encrypt_boot amiName = random.AlphaNum(7) } From d460b9afd4668f68b1f84ce342967b0ad9e50ff1 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Mon, 17 Jun 2019 14:48:58 -0700 Subject: [PATCH 2/4] add docs for skip_save_build_region --- website/source/docs/builders/amazon-ebs.html.md.erb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/website/source/docs/builders/amazon-ebs.html.md.erb b/website/source/docs/builders/amazon-ebs.html.md.erb index 27b40f803..4825ccaca 100644 --- a/website/source/docs/builders/amazon-ebs.html.md.erb +++ b/website/source/docs/builders/amazon-ebs.html.md.erb @@ -295,6 +295,12 @@ builder. - `skip_region_validation` (boolean) - Set to true if you want to skip validation of the region configuration option. Default `false`. +- `skip_save_build_region` (boolean) - If true, Packer will not check whether + an AMI with the `ami_name` exists in the region it is building in. It will + use an intermediary AMI name, which it will not convert to an AMI in the + build region. It will copy the intermediary AMI into any regions provided + in `ami_regions`, then delete the intermediary AMI. Default `false`. + - `snapshot_groups` (array of strings) - A list of groups that have access to create volumes from the snapshot(s). By default no groups have permission to create volumes from the snapshot(s). `all` will make the snapshot From 8cc82ca8d23c9af8017845fbe0829991ff71caf2 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Mon, 17 Jun 2019 15:04:19 -0700 Subject: [PATCH 3/4] add broken test --- .../common/step_ami_region_copy_test.go | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/builder/amazon/common/step_ami_region_copy_test.go b/builder/amazon/common/step_ami_region_copy_test.go index bd3c4fed0..26f35f7b4 100644 --- a/builder/amazon/common/step_ami_region_copy_test.go +++ b/builder/amazon/common/step_ami_region_copy_test.go @@ -158,3 +158,28 @@ func TestStepAmiRegionCopy_true_encryption(t *testing.T) { t.Fatalf("Should have added original ami to Regions") } } + +func TestStepAmiRegionCopy_true_AMISkipBuildRegion(t *testing.T) { + // create step + stepAMIRegionCopy := StepAMIRegionCopy{ + AccessConfig: testAccessConfig(), + Regions: make([]string, 0), + AMIKmsKeyId: "", + RegionKeyIds: make(map[string]string), + Name: "fake-ami-name", + OriginalRegion: "us-east-1", + AMISkipBuildRegion: true, + } + // mock out the region connection code + stepAMIRegionCopy.getRegionConn = getMockConn + + state := tState() + stepAMIRegionCopy.Run(context.Background(), state) + + if stepAMIRegionCopy.toDelete == "" { + t.Fatalf("Should delete original AMI if skip_save_build_region=true") + } + if len(stepAMIRegionCopy.Regions) == 0 { + t.Fatalf("Should not have added original ami to Regions") + } +} From 305592d8edfb498b1e456d96dd9f6e7c16441d90 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Mon, 17 Jun 2019 15:38:28 -0700 Subject: [PATCH 4/4] fix copy logic and tests --- builder/amazon/common/step_ami_region_copy.go | 30 ++++++++++++------- .../common/step_ami_region_copy_test.go | 10 +++---- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/builder/amazon/common/step_ami_region_copy.go b/builder/amazon/common/step_ami_region_copy.go index bfc582e5a..4a1f2d392 100644 --- a/builder/amazon/common/step_ami_region_copy.go +++ b/builder/amazon/common/step_ami_region_copy.go @@ -33,19 +33,25 @@ func (s *StepAMIRegionCopy) Run(ctx context.Context, state multistep.StateBag) m ami := amis[s.OriginalRegion] // Always copy back into original region to preserve the ami name - s.toDelete = ami - if !s.AMISkipBuildRegion { - s.Regions = append(s.Regions, s.OriginalRegion) + if s.EncryptBootVolume != nil || s.AMISkipBuildRegion { + // if we haven't specificed encryption and we aren't skipping the save + // to the build region, we don't have anything to delete + s.toDelete = ami } - if s.EncryptBootVolume != nil && *s.EncryptBootVolume { - // encrypt_boot is true, so we have to copy the temporary - // AMI with required encryption setting. - // temp image was created by stepCreateAMI. - if s.RegionKeyIds == nil { - s.RegionKeyIds = make(map[string]string) + if s.EncryptBootVolume != nil { + if !s.AMISkipBuildRegion { + s.Regions = append(s.Regions, s.OriginalRegion) + } + if *s.EncryptBootVolume { + // encrypt_boot is true, so we have to copy the temporary + // AMI with required encryption setting. + // temp image was created by stepCreateAMI. + if s.RegionKeyIds == nil { + s.RegionKeyIds = make(map[string]string) + } + s.RegionKeyIds[s.OriginalRegion] = s.AMIKmsKeyId } - s.RegionKeyIds[s.OriginalRegion] = s.AMIKmsKeyId } if len(s.Regions) == 0 { @@ -99,6 +105,10 @@ func (s *StepAMIRegionCopy) Cleanup(state multistep.StateBag) { ec2conn := state.Get("ec2").(*ec2.EC2) ui := state.Get("ui").(packer.Ui) + if len(s.toDelete) == 0 { + return + } + // Delete the unencrypted amis and snapshots ui.Say("Deregistering the AMI and deleting unencrypted temporary " + "AMIs and snapshots") diff --git a/builder/amazon/common/step_ami_region_copy_test.go b/builder/amazon/common/step_ami_region_copy_test.go index 26f35f7b4..03841b67d 100644 --- a/builder/amazon/common/step_ami_region_copy_test.go +++ b/builder/amazon/common/step_ami_region_copy_test.go @@ -101,11 +101,11 @@ func TestStepAmiRegionCopy_nil_encryption(t *testing.T) { state := tState() stepAMIRegionCopy.Run(context.Background(), state) - if stepAMIRegionCopy.toDelete != "ami-12345" { - t.Fatalf("Should delete original intermediary ami even if not encrypted") + if stepAMIRegionCopy.toDelete != "" { + t.Fatalf("Shouldn't have an intermediary ami if encrypt is nil") } - if len(stepAMIRegionCopy.Regions) == 0 { - t.Fatalf("Should have added original ami to original region") + if len(stepAMIRegionCopy.Regions) != 0 { + t.Fatalf("Should not have added original ami to original region") } } @@ -179,7 +179,7 @@ func TestStepAmiRegionCopy_true_AMISkipBuildRegion(t *testing.T) { if stepAMIRegionCopy.toDelete == "" { t.Fatalf("Should delete original AMI if skip_save_build_region=true") } - if len(stepAMIRegionCopy.Regions) == 0 { + if len(stepAMIRegionCopy.Regions) != 0 { t.Fatalf("Should not have added original ami to Regions") } }