diff --git a/builder/amazon/common/block_device.go b/builder/amazon/common/block_device.go index 06fda046b..88e066031 100644 --- a/builder/amazon/common/block_device.go +++ b/builder/amazon/common/block_device.go @@ -93,7 +93,7 @@ func (b *BlockDevice) Prepare(ctx *interpolate.Context) error { return fmt.Errorf("The `device_name` must be specified " + "for every device in the block device mapping.") } - // Warn that encrypted must be true when setting kms_key_id + // Warn that encrypted must be true or nil when setting kms_key_id if b.KmsKeyId != "" && b.Encrypted != nil && *b.Encrypted == false { return fmt.Errorf("The device %v, must also have `encrypted: "+ "true` when setting a kms_key_id.", b.DeviceName) diff --git a/builder/amazon/common/step_ami_region_copy.go b/builder/amazon/common/step_ami_region_copy.go index 36ae711fe..077459f5d 100644 --- a/builder/amazon/common/step_ami_region_copy.go +++ b/builder/amazon/common/step_ami_region_copy.go @@ -26,7 +26,7 @@ type StepAMIRegionCopy struct { AMISkipBuildRegion bool } -func (s *StepAMIRegionCopy) DeduplicateRegions() { +func (s *StepAMIRegionCopy) DeduplicateRegions(intermediary bool) { // Deduplicates regions by looping over the list of regions and storing // the regions as keys in a map. This saves users from accidentally copying // regions twice if they've added a region to a map twice. @@ -34,10 +34,24 @@ func (s *StepAMIRegionCopy) DeduplicateRegions() { RegionMap := map[string]bool{} RegionSlice := []string{} + // Original build region may or may not be present in the Regions list, so + // let's make absolutely sure it's in our map. + RegionMap[s.OriginalRegion] = true for _, r := range s.Regions { RegionMap[r] = true } + if !intermediary || s.AMISkipBuildRegion { + // We don't want to copy back into the original region if we aren't + // using an intermediary image, so remove the original region from our + // map. + + // We also don't want to copy back into the original region if the + // intermediary image is because we're skipping the build region. + delete(RegionMap, s.OriginalRegion) + + } + // Now print all those keys into the region slice again for k, _ := range RegionMap { RegionSlice = append(RegionSlice, k) @@ -50,35 +64,27 @@ func (s *StepAMIRegionCopy) Run(ctx context.Context, state multistep.StateBag) m ui := state.Get("ui").(packer.Ui) amis := state.Get("amis").(map[string]string) snapshots := state.Get("snapshots").(map[string][]string) + intermediary := state.Get("intermediary_image").(bool) + s.DeduplicateRegions(intermediary) ami := amis[s.OriginalRegion] - // Always copy back into original region to preserve the ami name - 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 + + // Make a note to delete the intermediary AMI if necessary. + if intermediary { s.toDelete = ami } - if s.EncryptBootVolume != nil { - 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 + // AMI with required encryption setting. + // temp image was created by stepCreateAMI. + if s.RegionKeyIds == nil { + s.RegionKeyIds = make(map[string]string) } - // Now that we've added OriginalRegion, best to make sure there aren't - // any duplicates hanging around; duplicates will waste time. - s.DeduplicateRegions() - - 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) - } - // Make sure the kms_key_id for the original region is in the map - if _, ok := s.RegionKeyIds[s.OriginalRegion]; !ok { - s.RegionKeyIds[s.OriginalRegion] = s.AMIKmsKeyId - } + // Make sure the kms_key_id for the original region is in the map + if _, ok := s.RegionKeyIds[s.OriginalRegion]; !ok { + s.RegionKeyIds[s.OriginalRegion] = s.AMIKmsKeyId } } @@ -90,15 +96,18 @@ func (s *StepAMIRegionCopy) Run(ctx context.Context, state multistep.StateBag) m var lock sync.Mutex var wg sync.WaitGroup - var regKeyID string errs := new(packer.MultiError) - wg.Add(len(s.Regions)) for _, region := range s.Regions { + var regKeyID string ui.Message(fmt.Sprintf("Copying to: %s", region)) if s.EncryptBootVolume != nil && *s.EncryptBootVolume { + // Encrypt is true, explicitly regKeyID = s.RegionKeyIds[region] + } else { + // Encrypt is nil or false; Make sure region key is empty + regKeyID = "" } go func(region string) { diff --git a/builder/amazon/common/step_ami_region_copy_test.go b/builder/amazon/common/step_ami_region_copy_test.go index 03841b67d..d434b777f 100644 --- a/builder/amazon/common/step_ami_region_copy_test.go +++ b/builder/amazon/common/step_ami_region_copy_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + "sync" "testing" "github.com/aws/aws-sdk-go/aws" @@ -25,10 +26,14 @@ type mockEC2Conn struct { deregisterImageCount int deleteSnapshotCount int waitCount int + + lock sync.Mutex } func (m *mockEC2Conn) CopyImage(copyInput *ec2.CopyImageInput) (*ec2.CopyImageOutput, error) { + m.lock.Lock() m.copyImageCount++ + m.lock.Unlock() copiedImage := fmt.Sprintf("%s-copied-%d", *copyInput.SourceImageId, m.copyImageCount) output := &ec2.CopyImageOutput{ ImageId: &copiedImage, @@ -38,7 +43,9 @@ func (m *mockEC2Conn) CopyImage(copyInput *ec2.CopyImageInput) (*ec2.CopyImageOu // functions we have to create mock responses for in order for test to run func (m *mockEC2Conn) DescribeImages(*ec2.DescribeImagesInput) (*ec2.DescribeImagesOutput, error) { + m.lock.Lock() m.describeImagesCount++ + m.lock.Unlock() output := &ec2.DescribeImagesOutput{ Images: []*ec2.Image{{}}, } @@ -46,19 +53,25 @@ func (m *mockEC2Conn) DescribeImages(*ec2.DescribeImagesInput) (*ec2.DescribeIma } func (m *mockEC2Conn) DeregisterImage(*ec2.DeregisterImageInput) (*ec2.DeregisterImageOutput, error) { + m.lock.Lock() m.deregisterImageCount++ + m.lock.Unlock() output := &ec2.DeregisterImageOutput{} return output, nil } func (m *mockEC2Conn) DeleteSnapshot(*ec2.DeleteSnapshotInput) (*ec2.DeleteSnapshotOutput, error) { + m.lock.Lock() m.deleteSnapshotCount++ + m.lock.Unlock() output := &ec2.DeleteSnapshotOutput{} return output, nil } func (m *mockEC2Conn) WaitUntilImageAvailableWithContext(aws.Context, *ec2.DescribeImagesInput, ...request.WaiterOption) error { + m.lock.Lock() m.waitCount++ + m.lock.Unlock() return nil } @@ -84,14 +97,19 @@ func tState() multistep.StateBag { return state } -func TestStepAmiRegionCopy_nil_encryption(t *testing.T) { - // create step +func TestStepAMIRegionCopy_duplicates(t *testing.T) { + // ------------------------------------------------------------------------ + // Test that if the original region is added to both Regions and Region, + // the ami is only copied once (with encryption). + // ------------------------------------------------------------------------ + stepAMIRegionCopy := StepAMIRegionCopy{ - AccessConfig: testAccessConfig(), - Regions: make([]string, 0), - AMIKmsKeyId: "", - RegionKeyIds: make(map[string]string), - EncryptBootVolume: nil, + AccessConfig: testAccessConfig(), + Regions: []string{"us-east-1"}, + AMIKmsKeyId: "12345", + // Original region key in regionkeyids is different than in amikmskeyid + RegionKeyIds: map[string]string{"us-east-1": "12345"}, + EncryptBootVolume: aws.Bool(true), Name: "fake-ami-name", OriginalRegion: "us-east-1", } @@ -99,24 +117,116 @@ func TestStepAmiRegionCopy_nil_encryption(t *testing.T) { stepAMIRegionCopy.getRegionConn = getMockConn state := tState() + state.Put("intermediary_image", true) stepAMIRegionCopy.Run(context.Background(), state) - if stepAMIRegionCopy.toDelete != "" { - t.Fatalf("Shouldn't have an intermediary ami if encrypt is nil") + if len(stepAMIRegionCopy.Regions) != 1 { + t.Fatalf("Should have added original ami to Regions one time only") + } + + // ------------------------------------------------------------------------ + // Both Region and Regions set, but no encryption - shouldn't copy anything + // ------------------------------------------------------------------------ + + // the ami is only copied once. + stepAMIRegionCopy = StepAMIRegionCopy{ + AccessConfig: testAccessConfig(), + Regions: []string{"us-east-1"}, + Name: "fake-ami-name", + OriginalRegion: "us-east-1", } + // mock out the region connection code + state.Put("intermediary_image", false) + stepAMIRegionCopy.getRegionConn = getMockConn + stepAMIRegionCopy.Run(context.Background(), state) + if len(stepAMIRegionCopy.Regions) != 0 { - t.Fatalf("Should not have added original ami to original region") + t.Fatalf("Should not have added original ami to Regions; not encrypting") + } + + // ------------------------------------------------------------------------ + // Both Region and Regions set, but no encryption - shouldn't copy anything, + // this tests false as opposed to nil value above. + // ------------------------------------------------------------------------ + + // the ami is only copied once. + stepAMIRegionCopy = StepAMIRegionCopy{ + AccessConfig: testAccessConfig(), + Regions: []string{"us-east-1"}, + EncryptBootVolume: aws.Bool(false), + Name: "fake-ami-name", + OriginalRegion: "us-east-1", + } + // mock out the region connection code + state.Put("intermediary_image", false) + stepAMIRegionCopy.getRegionConn = getMockConn + stepAMIRegionCopy.Run(context.Background(), state) + + if len(stepAMIRegionCopy.Regions) != 0 { + t.Fatalf("Should not have added original ami to Regions once; not" + + "encrypting") + } + + // ------------------------------------------------------------------------ + // Multiple regions, many duplicates, and encryption (this shouldn't ever + // happen because of our template validation, but good to test it.) + // ------------------------------------------------------------------------ + + stepAMIRegionCopy = StepAMIRegionCopy{ + AccessConfig: testAccessConfig(), + // Many duplicates for only 3 actual values + Regions: []string{"us-east-1", "us-west-2", "us-west-2", "ap-east-1", "ap-east-1", "ap-east-1"}, + AMIKmsKeyId: "IlikePancakes", + // Original region key in regionkeyids is different than in amikmskeyid + RegionKeyIds: map[string]string{"us-east-1": "12345", "us-west-2": "abcde", "ap-east-1": "xyz"}, + EncryptBootVolume: aws.Bool(true), + Name: "fake-ami-name", + OriginalRegion: "us-east-1", + } + // mock out the region connection code + stepAMIRegionCopy.getRegionConn = getMockConn + state.Put("intermediary_image", true) + stepAMIRegionCopy.Run(context.Background(), state) + + if len(stepAMIRegionCopy.Regions) != 3 { + t.Fatalf("Each AMI should have been added to Regions one time only.") + } + + // Also verify that we respect RegionKeyIds over AMIKmsKeyIds: + if stepAMIRegionCopy.RegionKeyIds["us-east-1"] != "12345" { + t.Fatalf("RegionKeyIds should take precedence over AmiKmsKeyIds") + } + + // ------------------------------------------------------------------------ + // Multiple regions, many duplicates, NO encryption + // ------------------------------------------------------------------------ + + stepAMIRegionCopy = StepAMIRegionCopy{ + AccessConfig: testAccessConfig(), + // Many duplicates for only 3 actual values + Regions: []string{"us-east-1", "us-west-2", "us-west-2", "ap-east-1", "ap-east-1", "ap-east-1"}, + Name: "fake-ami-name", + OriginalRegion: "us-east-1", + } + // mock out the region connection code + stepAMIRegionCopy.getRegionConn = getMockConn + state.Put("intermediary_image", false) + stepAMIRegionCopy.Run(context.Background(), state) + + if len(stepAMIRegionCopy.Regions) != 2 { + t.Fatalf("Each AMI should have been added to Regions one time only, " + + "and original region shouldn't be added at all") } } -func TestStepAmiRegionCopy_false_encryption(t *testing.T) { +func TestStepAmiRegionCopy_nil_encryption(t *testing.T) { // create step stepAMIRegionCopy := StepAMIRegionCopy{ AccessConfig: testAccessConfig(), Regions: make([]string, 0), AMIKmsKeyId: "", RegionKeyIds: make(map[string]string), - EncryptBootVolume: aws.Bool(false), + EncryptBootVolume: nil, Name: "fake-ami-name", OriginalRegion: "us-east-1", } @@ -124,13 +234,14 @@ func TestStepAmiRegionCopy_false_encryption(t *testing.T) { stepAMIRegionCopy.getRegionConn = getMockConn state := tState() + state.Put("intermediary_image", false) stepAMIRegionCopy.Run(context.Background(), state) - if stepAMIRegionCopy.toDelete != "ami-12345" { - t.Fatalf("should be deleting the original intermediary ami") + 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 Regions") + if len(stepAMIRegionCopy.Regions) != 0 { + t.Fatalf("Should not have added original ami to original region") } } @@ -149,6 +260,7 @@ func TestStepAmiRegionCopy_true_encryption(t *testing.T) { stepAMIRegionCopy.getRegionConn = getMockConn state := tState() + state.Put("intermediary_image", true) stepAMIRegionCopy.Run(context.Background(), state) if stepAMIRegionCopy.toDelete == "" { @@ -159,13 +271,16 @@ func TestStepAmiRegionCopy_true_encryption(t *testing.T) { } } -func TestStepAmiRegionCopy_true_AMISkipBuildRegion(t *testing.T) { - // create step +func TestStepAmiRegionCopy_AMISkipBuildRegion(t *testing.T) { + // ------------------------------------------------------------------------ + // skip build region is true + // ------------------------------------------------------------------------ + stepAMIRegionCopy := StepAMIRegionCopy{ AccessConfig: testAccessConfig(), - Regions: make([]string, 0), + Regions: []string{"us-west-1"}, AMIKmsKeyId: "", - RegionKeyIds: make(map[string]string), + RegionKeyIds: map[string]string{"us-west-1": "abcde"}, Name: "fake-ami-name", OriginalRegion: "us-east-1", AMISkipBuildRegion: true, @@ -174,12 +289,90 @@ func TestStepAmiRegionCopy_true_AMISkipBuildRegion(t *testing.T) { stepAMIRegionCopy.getRegionConn = getMockConn state := tState() + state.Put("intermediary_image", true) 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") + if len(stepAMIRegionCopy.Regions) != 1 { + t.Fatalf("Should not have added original ami to Regions; Regions: %#v", stepAMIRegionCopy.Regions) + } + + // ------------------------------------------------------------------------ + // skip build region is false. + // ------------------------------------------------------------------------ + stepAMIRegionCopy = StepAMIRegionCopy{ + AccessConfig: testAccessConfig(), + Regions: []string{"us-west-1"}, + AMIKmsKeyId: "", + RegionKeyIds: make(map[string]string), + Name: "fake-ami-name", + OriginalRegion: "us-east-1", + AMISkipBuildRegion: false, + } + // mock out the region connection code + stepAMIRegionCopy.getRegionConn = getMockConn + + state.Put("intermediary_image", false) // not encrypted + stepAMIRegionCopy.Run(context.Background(), state) + + if stepAMIRegionCopy.toDelete != "" { + t.Fatalf("Shouldn't have an intermediary AMI, so dont delete original ami") + } + if len(stepAMIRegionCopy.Regions) != 1 { + t.Fatalf("Should not have added original ami to Regions; Regions: %#v", stepAMIRegionCopy.Regions) + } + + // ------------------------------------------------------------------------ + // skip build region is false, but encrypt is true + // ------------------------------------------------------------------------ + stepAMIRegionCopy = StepAMIRegionCopy{ + AccessConfig: testAccessConfig(), + Regions: []string{"us-west-1"}, + AMIKmsKeyId: "", + RegionKeyIds: map[string]string{"us-west-1": "abcde"}, + Name: "fake-ami-name", + OriginalRegion: "us-east-1", + AMISkipBuildRegion: false, + EncryptBootVolume: aws.Bool(true), + } + // mock out the region connection code + stepAMIRegionCopy.getRegionConn = getMockConn + + state.Put("intermediary_image", true) //encrypted + stepAMIRegionCopy.Run(context.Background(), state) + + if stepAMIRegionCopy.toDelete == "" { + t.Fatalf("Have to delete intermediary AMI") + } + if len(stepAMIRegionCopy.Regions) != 2 { + t.Fatalf("Should have added original ami to Regions; Regions: %#v", stepAMIRegionCopy.Regions) + } + + // ------------------------------------------------------------------------ + // skip build region is true, and encrypt is true + // ------------------------------------------------------------------------ + stepAMIRegionCopy = StepAMIRegionCopy{ + AccessConfig: testAccessConfig(), + Regions: []string{"us-west-1"}, + AMIKmsKeyId: "", + RegionKeyIds: map[string]string{"us-west-1": "abcde"}, + Name: "fake-ami-name", + OriginalRegion: "us-east-1", + AMISkipBuildRegion: true, + EncryptBootVolume: aws.Bool(true), + } + // mock out the region connection code + stepAMIRegionCopy.getRegionConn = getMockConn + + state.Put("intermediary_image", true) //encrypted + stepAMIRegionCopy.Run(context.Background(), state) + + if stepAMIRegionCopy.toDelete == "" { + t.Fatalf("Have to delete intermediary AMI") + } + if len(stepAMIRegionCopy.Regions) != 1 { + t.Fatalf("Should not have added original ami to Regions; Regions: %#v", stepAMIRegionCopy.Regions) } } diff --git a/builder/amazon/ebs/step_create_ami.go b/builder/amazon/ebs/step_create_ami.go index c1d0da0b8..d4d35638f 100644 --- a/builder/amazon/ebs/step_create_ami.go +++ b/builder/amazon/ebs/step_create_ami.go @@ -26,9 +26,18 @@ func (s *stepCreateAMI) Run(ctx context.Context, state multistep.StateBag) multi // Create the image amiName := config.AMIName - if config.AMIEncryptBootVolume != nil || s.AMISkipBuildRegion { - // Create a temporary image and then create a copy of it with the - // correct encrypt_boot + state.Put("intermediary_image", false) + if config.AMIEncryptBootVolume != nil && *config.AMIEncryptBootVolume != false || s.AMISkipBuildRegion { + state.Put("intermediary_image", true) + + // From AWS SDK docs: You can encrypt a copy of an unencrypted snapshot, + // but you cannot use it to create an unencrypted copy of an encrypted + // snapshot. Your default CMK for EBS is used unless you specify a + // non-default key using KmsKeyId. + + // If encrypt_boot is nil or true, we need to create a temporary image + // so that in step_region_copy, we can copy it with the correct + // encryption amiName = random.AlphaNum(7) } diff --git a/website/source/docs/builders/amazon-chroot.html.md.erb b/website/source/docs/builders/amazon-chroot.html.md.erb index 14bc59195..635b3e505 100644 --- a/website/source/docs/builders/amazon-chroot.html.md.erb +++ b/website/source/docs/builders/amazon-chroot.html.md.erb @@ -198,6 +198,10 @@ each category, the available configuration keys are alphabetized. *KmsKeyId* in the [AWS API docs - CopyImage](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_CopyImage.html). + This option supercedes the `kms_key_id` option -- if you set both, and they + are different, Packer will respect the value in `region_kms_key_ids` for + your build region and silently disregard the value provided in `kms_key_id`. + - `root_device_name` (string) - The root device name. For example, `xvda`. - `mfa_code` (string) - The MFA diff --git a/website/source/docs/builders/amazon-ebs.html.md.erb b/website/source/docs/builders/amazon-ebs.html.md.erb index 4825ccaca..3e7b6fe6d 100644 --- a/website/source/docs/builders/amazon-ebs.html.md.erb +++ b/website/source/docs/builders/amazon-ebs.html.md.erb @@ -244,6 +244,10 @@ builder. *KmsKeyId* in the [AWS API docs - CopyImage](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_CopyImage.html). + This option supercedes the `kms_key_id` option -- if you set both, and they + are different, Packer will respect the value in `region_kms_key_ids` for + your build region and silently disregard the value provided in `kms_key_id`. + - `run_tags` (object of key/value strings) - Tags to apply to the instance that is *launched* to create the AMI. These tags are *not* applied to the resulting AMI unless they're duplicated in `tags`. This is a [template diff --git a/website/source/docs/builders/amazon-ebssurrogate.html.md.erb b/website/source/docs/builders/amazon-ebssurrogate.html.md.erb index cc1f7fcdb..e34e9ed1b 100644 --- a/website/source/docs/builders/amazon-ebssurrogate.html.md.erb +++ b/website/source/docs/builders/amazon-ebssurrogate.html.md.erb @@ -245,6 +245,10 @@ builder. *KmsKeyId* in the [AWS API docs - CopyImage](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_CopyImage.html). + This option supercedes the `kms_key_id` option -- if you set both, and they + are different, Packer will respect the value in `region_kms_key_ids` for + your build region and silently disregard the value provided in `kms_key_id`. + - `run_tags` (object of key/value strings) - Tags to apply to the instance that is *launched* to create the AMI. These tags are *not* applied to the resulting AMI unless they're duplicated in `tags`. This is a [template diff --git a/website/source/docs/builders/amazon-instance.html.md.erb b/website/source/docs/builders/amazon-instance.html.md.erb index 7c8048a32..f0d61bdd0 100644 --- a/website/source/docs/builders/amazon-instance.html.md.erb +++ b/website/source/docs/builders/amazon-instance.html.md.erb @@ -247,6 +247,10 @@ builder. *KmsKeyId* in the [AWS API docs - CopyImage](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_CopyImage.html). + This option supercedes the `kms_key_id` option -- if you set both, and they + are different, Packer will respect the value in `region_kms_key_ids` for + your build region and silently disregard the value provided in `kms_key_id`. + - `run_tags` (object of key/value strings) - Tags to apply to the instance that is *launched* to create the AMI. These tags are *not* applied to the resulting AMI unless they're duplicated in `tags`. This is a [template