From bda09bf7d571699a85538cdb41a9c6fda11dada8 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Fri, 24 May 2019 15:08:20 -0700 Subject: [PATCH 1/2] the build ami is made with an intermediary name, which means that we can't skip copying even from the original build region when unencrypted --- builder/amazon/common/step_ami_region_copy.go | 25 +++---------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/builder/amazon/common/step_ami_region_copy.go b/builder/amazon/common/step_ami_region_copy.go index c289cf4d1..620715d27 100644 --- a/builder/amazon/common/step_ami_region_copy.go +++ b/builder/amazon/common/step_ami_region_copy.go @@ -31,17 +31,18 @@ func (s *StepAMIRegionCopy) Run(ctx context.Context, state multistep.StateBag) m snapshots := state.Get("snapshots").(map[string][]string) 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.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. - s.Regions = append(s.Regions, s.OriginalRegion) if s.RegionKeyIds == nil { s.RegionKeyIds = make(map[string]string) } s.RegionKeyIds[s.OriginalRegion] = s.AMIKmsKeyId - s.toDelete = ami } if len(s.Regions) == 0 { @@ -57,20 +58,7 @@ func (s *StepAMIRegionCopy) Run(ctx context.Context, state multistep.StateBag) m wg.Add(len(s.Regions)) for _, region := range s.Regions { - if region == s.OriginalRegion { - if s.EncryptBootVolume == nil || *s.EncryptBootVolume == false { - ui.Message(fmt.Sprintf( - "Avoiding copying AMI to duplicate region %s", region)) - wg.Done() - continue - } else { - // encryption is true and we're in the original region - ui.Message(fmt.Sprintf("Creating encrypted copy in build region: %s", region)) - } - } else { - // in non-build region - ui.Message(fmt.Sprintf("Copying to: %s", region)) - } + ui.Message(fmt.Sprintf("Copying to: %s", region)) if s.EncryptBootVolume != nil && *s.EncryptBootVolume { regKeyID = s.RegionKeyIds[region] @@ -108,11 +96,6 @@ func (s *StepAMIRegionCopy) Cleanup(state multistep.StateBag) { ec2conn := state.Get("ec2").(*ec2.EC2) ui := state.Get("ui").(packer.Ui) - // cleanup is only for encrypted copies. - if s.EncryptBootVolume == nil || !*s.EncryptBootVolume { - return - } - // Delete the unencrypted amis and snapshots ui.Say("Deregistering the AMI and deleting unencrypted temporary " + "AMIs and snapshots") From 6786c0d6415b0348ebfce22a93be7bf51ad4d212 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Thu, 30 May 2019 16:17:23 -0500 Subject: [PATCH 2/2] update tests to reflect new behavior --- .../amazon/common/step_ami_region_copy_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/builder/amazon/common/step_ami_region_copy_test.go b/builder/amazon/common/step_ami_region_copy_test.go index 7428d63b5..bd3c4fed0 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 != "" { - t.Fatalf("Shouldn't delete original AMI if not encrypted") + if stepAMIRegionCopy.toDelete != "ami-12345" { + t.Fatalf("Should delete original intermediary ami even if not encrypted") } - if len(stepAMIRegionCopy.Regions) > 0 { - t.Fatalf("Shouldn't have added original ami to original region") + if len(stepAMIRegionCopy.Regions) == 0 { + t.Fatalf("Should have added original ami to original region") } } @@ -126,11 +126,11 @@ func TestStepAmiRegionCopy_false_encryption(t *testing.T) { state := tState() stepAMIRegionCopy.Run(context.Background(), state) - if stepAMIRegionCopy.toDelete != "" { - t.Fatalf("Shouldn't delete original AMI if not encrypted") + if stepAMIRegionCopy.toDelete != "ami-12345" { + t.Fatalf("should be deleting the original intermediary ami") } - if len(stepAMIRegionCopy.Regions) > 0 { - t.Fatalf("Shouldn't have added original ami to Regions") + if len(stepAMIRegionCopy.Regions) == 0 { + t.Fatalf("Should have added original ami to Regions") } }