aherman
/
packer
mirror of https://github.com/hashicorp/packer
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

refactored step_ami_region_copy to fix bugs and clarify assumptions; added more tests for that step.

Browse Source 
fix race condition caused by variable declaration outside of loop
pull/7870/head
Megan Marsh 7 years ago
parent 39a4da4d07
commit 54d2ad5028
8 changed files with 278 additions and 51 deletions
Show Stats Download Patch File Download Diff File

2
builder/amazon/common/block_device.go
Unescape View File

@ -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)

59
builder/amazon/common/step_ami_region_copy.go
Unescape View File

@ -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) {

237
builder/amazon/common/step_ami_region_copy_test.go
Unescape View File

@ -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)
}
}

15
builder/amazon/ebs/step_create_ami.go
Unescape View File

@ -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)
}

4
website/source/docs/builders/amazon-chroot.html.md.erb
Unescape View File

@ -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

4
website/source/docs/builders/amazon-ebs.html.md.erb
Unescape View File

@ -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

4
website/source/docs/builders/amazon-ebssurrogate.html.md.erb
Unescape View File

@ -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

4
website/source/docs/builders/amazon-instance.html.md.erb
Unescape View File

@ -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

Write Preview
Loading…
Cancel
Save