From 142a84ef43c653eed7e52fffba49a6a473f04203 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Mon, 25 Feb 2019 10:31:42 -0800 Subject: [PATCH 1/3] amazon/chroot: Refactor step_register_ami for testing EBS-related behaviour There is some logic around how EBS mappings are handled / generated depending on whether an image if from scratch or not. There was no testing around this behaviour before. This strips that logic out into a separate function, to enable testing it independentlhy. --- builder/amazon/chroot/step_register_ami.go | 107 ++++++++++-------- .../amazon/chroot/step_register_ami_test.go | 75 ++++++++++++ 2 files changed, 134 insertions(+), 48 deletions(-) diff --git a/builder/amazon/chroot/step_register_ami.go b/builder/amazon/chroot/step_register_ami.go index dbb84a5be..c3eac6c14 100644 --- a/builder/amazon/chroot/step_register_ami.go +++ b/builder/amazon/chroot/step_register_ami.go @@ -21,61 +21,19 @@ type StepRegisterAMI struct { func (s *StepRegisterAMI) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { config := state.Get("config").(*Config) ec2conn := state.Get("ec2").(*ec2.EC2) - snapshotId := state.Get("snapshot_id").(string) + snapshotID := state.Get("snapshot_id").(string) ui := state.Get("ui").(packer.Ui) ui.Say("Registering the AMI...") - var ( - registerOpts *ec2.RegisterImageInput - mappings []*ec2.BlockDeviceMapping - image *ec2.Image - rootDeviceName string - ) + var registerOpts *ec2.RegisterImageInput + // Source Image is only required to be passed if the image is not from scratch if config.FromScratch { - mappings = config.AMIBlockDevices.BuildAMIDevices() - rootDeviceName = config.RootDeviceName + registerOpts = buildBaseRegisterOpts(config, nil, s.RootVolumeSize, snapshotID) } else { - image = state.Get("source_image").(*ec2.Image) - mappings = image.BlockDeviceMappings - rootDeviceName = *image.RootDeviceName - } - - newMappings := make([]*ec2.BlockDeviceMapping, len(mappings)) - for i, device := range mappings { - newDevice := device - if *newDevice.DeviceName == rootDeviceName { - if newDevice.Ebs != nil { - newDevice.Ebs.SnapshotId = aws.String(snapshotId) - } else { - newDevice.Ebs = &ec2.EbsBlockDevice{SnapshotId: aws.String(snapshotId)} - } - - if config.FromScratch || s.RootVolumeSize > *newDevice.Ebs.VolumeSize { - newDevice.Ebs.VolumeSize = aws.Int64(s.RootVolumeSize) - } - } - - // assume working from a snapshot, so we unset the Encrypted field if set, - // otherwise AWS API will return InvalidParameter - if newDevice.Ebs != nil && newDevice.Ebs.Encrypted != nil { - newDevice.Ebs.Encrypted = nil - } - - newMappings[i] = newDevice - } - - if config.FromScratch { - registerOpts = &ec2.RegisterImageInput{ - Name: &config.AMIName, - Architecture: aws.String(ec2.ArchitectureValuesX8664), - RootDeviceName: aws.String(rootDeviceName), - VirtualizationType: aws.String(config.AMIVirtType), - BlockDeviceMappings: newMappings, - } - } else { - registerOpts = buildRegisterOpts(config, image, newMappings) + image := state.Get("source_image").(*ec2.Image) + registerOpts = buildBaseRegisterOpts(config, image, s.RootVolumeSize, snapshotID) } if s.EnableAMISriovNetSupport { @@ -115,6 +73,59 @@ func (s *StepRegisterAMI) Run(ctx context.Context, state multistep.StateBag) mul func (s *StepRegisterAMI) Cleanup(state multistep.StateBag) {} +// Builds the base register opts with architecture, name, root block device, mappings, virtualizationtype +func buildBaseRegisterOpts(config *Config, sourceImage *ec2.Image, rootVolumeSize int64, snapshotID string) *ec2.RegisterImageInput { + var ( + mappings []*ec2.BlockDeviceMapping + rootDeviceName string + ) + + if config.FromScratch { + mappings = config.AMIBlockDevices.BuildAMIDevices() + rootDeviceName = config.RootDeviceName + } else { + // If config.FromScratch is false, source image must be set + mappings = sourceImage.BlockDeviceMappings + rootDeviceName = *sourceImage.RootDeviceName + } + + newMappings := make([]*ec2.BlockDeviceMapping, len(mappings)) + for i, device := range mappings { + newDevice := device + if *newDevice.DeviceName == rootDeviceName { + if newDevice.Ebs != nil { + newDevice.Ebs.SnapshotId = aws.String(snapshotID) + } else { + newDevice.Ebs = &ec2.EbsBlockDevice{SnapshotId: aws.String(snapshotID)} + } + + if config.FromScratch || rootVolumeSize > *newDevice.Ebs.VolumeSize { + newDevice.Ebs.VolumeSize = aws.Int64(rootVolumeSize) + } + } + + // assume working from a snapshot, so we unset the Encrypted field if set, + // otherwise AWS API will return InvalidParameter + if newDevice.Ebs != nil && newDevice.Ebs.Encrypted != nil { + newDevice.Ebs.Encrypted = nil + } + + newMappings[i] = newDevice + } + + if config.FromScratch { + return &ec2.RegisterImageInput{ + Name: &config.AMIName, + Architecture: aws.String(ec2.ArchitectureValuesX8664), + RootDeviceName: aws.String(rootDeviceName), + VirtualizationType: aws.String(config.AMIVirtType), + BlockDeviceMappings: newMappings, + } + } + + return buildRegisterOpts(config, sourceImage, newMappings) +} + func buildRegisterOpts(config *Config, image *ec2.Image, mappings []*ec2.BlockDeviceMapping) *ec2.RegisterImageInput { registerOpts := &ec2.RegisterImageInput{ Name: &config.AMIName, diff --git a/builder/amazon/chroot/step_register_ami_test.go b/builder/amazon/chroot/step_register_ami_test.go index 0cc3bc912..7187ff674 100644 --- a/builder/amazon/chroot/step_register_ami_test.go +++ b/builder/amazon/chroot/step_register_ami_test.go @@ -1,6 +1,8 @@ package chroot import ( + amazon "github.com/hashicorp/packer/builder/amazon/common" + "github.com/hashicorp/packer/common" "testing" "github.com/aws/aws-sdk-go/aws" @@ -71,3 +73,76 @@ func TestStepRegisterAmi_buildRegisterOpts_hvm(t *testing.T) { t.Fatalf("Unexpected KernelId value: expected nil got %s\n", *opts.KernelId) } } + +func TestStepRegisterAmi_buildRegisterOptsFromScratch(t *testing.T) { + rootDeviceName := "/dev/sda" + snapshotID := "foo" + config := Config{ + FromScratch: true, + PackerConfig: common.PackerConfig{}, + AMIBlockDevices: amazon.AMIBlockDevices{ + AMIMappings: []amazon.BlockDevice{ + { + DeviceName: rootDeviceName, + }, + }, + }, + RootDeviceName: rootDeviceName, + } + registerOpts := buildBaseRegisterOpts(&config, nil, 10, snapshotID) + + if len(registerOpts.BlockDeviceMappings) != 1 { + t.Fatal("Expected block device mapping of length 1") + } + + if *registerOpts.BlockDeviceMappings[0].Ebs.SnapshotId != snapshotID { + t.Fatalf("Snapshot ID of root disk not set to snapshot id %s", rootDeviceName) + } +} + +func TestStepRegisterAmi_buildRegisterOptFromExistingImage(t *testing.T) { + rootDeviceName := "/dev/sda" + snapshotID := "foo" + + config := Config{ + FromScratch: false, + PackerConfig: common.PackerConfig{}, + } + sourceImage := ec2.Image{ + RootDeviceName: &rootDeviceName, + BlockDeviceMappings: []*ec2.BlockDeviceMapping{ + { + DeviceName: &rootDeviceName, + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: aws.Int64(10), + }, + }, + // Throw in an ephemeral device, it seems like all devices in the return struct in a source AMI have + // a size, even if it's for ephemeral + { + DeviceName: aws.String("/dev/sdb"), + VirtualName: aws.String("ephemeral0"), + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: aws.Int64(0), + }, + }, + }, + } + registerOpts := buildBaseRegisterOpts(&config, &sourceImage, 15, snapshotID) + + if len(registerOpts.BlockDeviceMappings) != 2 { + t.Fatal("Expected block device mapping of length 2") + } + + for _, dev := range registerOpts.BlockDeviceMappings { + if dev.Ebs.SnapshotId != nil && *dev.Ebs.SnapshotId == snapshotID { + // Even though root volume size is in config, it isn't used, instead we use the root volume size + // that's derived when we build the step + if *dev.Ebs.VolumeSize != 15 { + t.Fatalf("Root volume size not 15 GB instead %d", *dev.Ebs.VolumeSize) + } + return + } + } + t.Fatalf("Could not find device with snapshot ID %s", snapshotID) +} From 601e7544381118769086d2bfdd5a4ad24b227329 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Mon, 25 Feb 2019 10:55:10 -0800 Subject: [PATCH 2/3] amazon/chroot: Allow creating new block device mappings !not fromScratch Previously, when you built from an existing image, you were unable to reconfigure block device mappings, as it just took them and copied them over. This allows users to specify new, custom block device mappings, even when building from an existing image. --- builder/amazon/chroot/builder.go | 19 +++-- builder/amazon/chroot/builder_test.go | 46 ++++++++++++ builder/amazon/chroot/step_register_ami.go | 11 +-- .../amazon/chroot/step_register_ami_test.go | 75 ++++++++++++++++++- 4 files changed, 139 insertions(+), 12 deletions(-) diff --git a/builder/amazon/chroot/builder.go b/builder/amazon/chroot/builder.go index 3b1b4609e..aa9e7ddd7 100644 --- a/builder/amazon/chroot/builder.go +++ b/builder/amazon/chroot/builder.go @@ -167,11 +167,20 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, error) { errs = packer.MultiErrorAppend( errs, errors.New("source_ami or source_ami_filter is required.")) } - if len(b.config.AMIMappings) != 0 { - warns = append(warns, "ami_block_device_mappings are unused when from_scratch is false") - } - if b.config.RootDeviceName != "" { - warns = append(warns, "root_device_name is unused when from_scratch is false") + if len(b.config.AMIMappings) > 0 && b.config.RootDeviceName != "" { + if b.config.RootVolumeSize == 0 { + // Although, they can specify the device size in the block device mapping, it's easier to + // be specific here. + errs = packer.MultiErrorAppend( + errs, errors.New("root_volume_size is required if ami_block_device_mappings is specified")) + } + warns = append(warns, "ami_block_device_mappings from source image will be completely overwritten") + } else if len(b.config.AMIMappings) > 0 { + errs = packer.MultiErrorAppend( + errs, errors.New("If ami_block_device_mappings is specified, root_device_name must be specified")) + } else if b.config.RootDeviceName != "" { + errs = packer.MultiErrorAppend( + errs, errors.New("If root_device_name is specified, ami_block_device_mappings must be specified")) } } diff --git a/builder/amazon/chroot/builder_test.go b/builder/amazon/chroot/builder_test.go index b4bb0ab76..1fcc905f1 100644 --- a/builder/amazon/chroot/builder_test.go +++ b/builder/amazon/chroot/builder_test.go @@ -162,3 +162,49 @@ func TestBuilderPrepare_CopyFilesNoDefault(t *testing.T) { t.Errorf("Was expecting no default value for copy_files.") } } + +func TestBuilderPrepare_RootDeviceNameAndAMIMappings(t *testing.T) { + var b Builder + config := testConfig() + + config["root_device_name"] = "/dev/sda" + config["ami_block_device_mappings"] = []interface{}{map[string]string{}} + config["root_volume_size"] = 15 + warnings, err := b.Prepare(config) + if len(warnings) == 0 { + t.Fatal("Missing warning, stating block device mappings will be overwritten") + } else if len(warnings) > 1 { + t.Fatalf("excessive warnings: %#v", warnings) + } + if err != nil { + t.Fatalf("should not have error: %s", err) + } +} + +func TestBuilderPrepare_AMIMappingsNoRootDeviceName(t *testing.T) { + var b Builder + config := testConfig() + + config["ami_block_device_mappings"] = []interface{}{map[string]string{}} + warnings, err := b.Prepare(config) + if len(warnings) > 0 { + t.Fatalf("bad: %#v", warnings) + } + if err == nil { + t.Fatalf("should have error") + } +} + +func TestBuilderPrepare_RootDeviceNameNoAMIMappings(t *testing.T) { + var b Builder + config := testConfig() + + config["root_device_name"] = "/dev/sda" + warnings, err := b.Prepare(config) + if len(warnings) > 0 { + t.Fatalf("bad: %#v", warnings) + } + if err == nil { + t.Fatalf("should have error") + } +} diff --git a/builder/amazon/chroot/step_register_ami.go b/builder/amazon/chroot/step_register_ami.go index c3eac6c14..da60e917a 100644 --- a/builder/amazon/chroot/step_register_ami.go +++ b/builder/amazon/chroot/step_register_ami.go @@ -80,7 +80,8 @@ func buildBaseRegisterOpts(config *Config, sourceImage *ec2.Image, rootVolumeSiz rootDeviceName string ) - if config.FromScratch { + generatingNewBlockDeviceMappings := config.FromScratch || len(config.AMIMappings) > 0 + if generatingNewBlockDeviceMappings { mappings = config.AMIBlockDevices.BuildAMIDevices() rootDeviceName = config.RootDeviceName } else { @@ -99,7 +100,7 @@ func buildBaseRegisterOpts(config *Config, sourceImage *ec2.Image, rootVolumeSiz newDevice.Ebs = &ec2.EbsBlockDevice{SnapshotId: aws.String(snapshotID)} } - if config.FromScratch || rootVolumeSize > *newDevice.Ebs.VolumeSize { + if generatingNewBlockDeviceMappings || rootVolumeSize > *newDevice.Ebs.VolumeSize { newDevice.Ebs.VolumeSize = aws.Int64(rootVolumeSize) } } @@ -123,14 +124,14 @@ func buildBaseRegisterOpts(config *Config, sourceImage *ec2.Image, rootVolumeSiz } } - return buildRegisterOpts(config, sourceImage, newMappings) + return buildRegisterOptsFromExistingImage(config, sourceImage, newMappings, rootDeviceName) } -func buildRegisterOpts(config *Config, image *ec2.Image, mappings []*ec2.BlockDeviceMapping) *ec2.RegisterImageInput { +func buildRegisterOptsFromExistingImage(config *Config, image *ec2.Image, mappings []*ec2.BlockDeviceMapping, rootDeviceName string) *ec2.RegisterImageInput { registerOpts := &ec2.RegisterImageInput{ Name: &config.AMIName, Architecture: image.Architecture, - RootDeviceName: image.RootDeviceName, + RootDeviceName: &rootDeviceName, BlockDeviceMappings: mappings, VirtualizationType: image.VirtualizationType, } diff --git a/builder/amazon/chroot/step_register_ami_test.go b/builder/amazon/chroot/step_register_ami_test.go index 7187ff674..8c686b4bc 100644 --- a/builder/amazon/chroot/step_register_ami_test.go +++ b/builder/amazon/chroot/step_register_ami_test.go @@ -23,12 +23,13 @@ func TestStepRegisterAmi_buildRegisterOpts_pv(t *testing.T) { config.AMIName = "test_ami_name" config.AMIDescription = "test_ami_description" config.AMIVirtType = "paravirtual" + rootDeviceName := "foo" image := testImage() blockDevices := []*ec2.BlockDeviceMapping{} - opts := buildRegisterOpts(&config, &image, blockDevices) + opts := buildRegisterOptsFromExistingImage(&config, &image, blockDevices, rootDeviceName) expected := config.AMIVirtType if *opts.VirtualizationType != expected { @@ -45,6 +46,10 @@ func TestStepRegisterAmi_buildRegisterOpts_pv(t *testing.T) { t.Fatalf("Unexpected KernelId value: expected %s got %s\n", expected, *opts.KernelId) } + expected = rootDeviceName + if *opts.RootDeviceName != expected { + t.Fatalf("Unexpected RootDeviceName value: expected %s got %s\n", expected, *opts.RootDeviceName) + } } func TestStepRegisterAmi_buildRegisterOpts_hvm(t *testing.T) { @@ -52,12 +57,13 @@ func TestStepRegisterAmi_buildRegisterOpts_hvm(t *testing.T) { config.AMIName = "test_ami_name" config.AMIDescription = "test_ami_description" config.AMIVirtType = "hvm" + rootDeviceName := "foo" image := testImage() blockDevices := []*ec2.BlockDeviceMapping{} - opts := buildRegisterOpts(&config, &image, blockDevices) + opts := buildRegisterOptsFromExistingImage(&config, &image, blockDevices, rootDeviceName) expected := config.AMIVirtType if *opts.VirtualizationType != expected { @@ -72,6 +78,11 @@ func TestStepRegisterAmi_buildRegisterOpts_hvm(t *testing.T) { if opts.KernelId != nil { t.Fatalf("Unexpected KernelId value: expected nil got %s\n", *opts.KernelId) } + + expected = rootDeviceName + if *opts.RootDeviceName != expected { + t.Fatalf("Unexpected RootDeviceName value: expected %s got %s\n", expected, *opts.RootDeviceName) + } } func TestStepRegisterAmi_buildRegisterOptsFromScratch(t *testing.T) { @@ -146,3 +157,63 @@ func TestStepRegisterAmi_buildRegisterOptFromExistingImage(t *testing.T) { } t.Fatalf("Could not find device with snapshot ID %s", snapshotID) } + +func TestStepRegisterAmi_buildRegisterOptFromExistingImageWithBlockDeviceMappings(t *testing.T) { + const ( + rootDeviceName = "/dev/xvda" + oldRootDevice = "/dev/sda1" + ) + snapshotId := "foo" + + config := Config{ + FromScratch: false, + PackerConfig: common.PackerConfig{}, + AMIBlockDevices: amazon.AMIBlockDevices{ + AMIMappings: []amazon.BlockDevice{ + { + DeviceName: rootDeviceName, + }, + }, + }, + RootDeviceName: rootDeviceName, + } + + // Intentionally try to use a different root devicename + sourceImage := ec2.Image{ + RootDeviceName: aws.String(oldRootDevice), + BlockDeviceMappings: []*ec2.BlockDeviceMapping{ + { + DeviceName: aws.String(oldRootDevice), + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: aws.Int64(10), + }, + }, + // Throw in an ephemeral device, it seems like all devices in the return struct in a source AMI have + // a size, even if it's for ephemeral + { + DeviceName: aws.String("/dev/sdb"), + VirtualName: aws.String("ephemeral0"), + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: aws.Int64(0), + }, + }, + }, + } + registerOpts := buildBaseRegisterOpts(&config, &sourceImage, 15, snapshotId) + + if len(registerOpts.BlockDeviceMappings) != 1 { + t.Fatal("Expected block device mapping of length 1") + } + + if *registerOpts.BlockDeviceMappings[0].Ebs.SnapshotId != snapshotId { + t.Fatalf("Snapshot ID of root disk set to '%s' expected '%s'", *registerOpts.BlockDeviceMappings[0].Ebs.SnapshotId, rootDeviceName) + } + + if *registerOpts.RootDeviceName != rootDeviceName { + t.Fatalf("Root device set to '%s' expected %s", *registerOpts.RootDeviceName, rootDeviceName) + } + + if *registerOpts.BlockDeviceMappings[0].Ebs.VolumeSize != 15 { + t.Fatalf("Size of root disk not set to 15 GB, instead %d", *registerOpts.BlockDeviceMappings[0].Ebs.VolumeSize) + } +} From 0e8eb61699471f0cdeab6e1e1033c6716413f77b Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Mon, 11 Mar 2019 11:40:49 -0700 Subject: [PATCH 3/3] Add documentation around new behaviour of ami_block_device_mappings Previously, if ami_block_device_mappings was set, and you were building from an existing image, it would get silently ignored, and changing root_device_name would be ignored. This changes that behaviour, so if ami_block_device_mappings is specified, it's respected. --- website/source/docs/builders/amazon-chroot.html.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/website/source/docs/builders/amazon-chroot.html.md b/website/source/docs/builders/amazon-chroot.html.md index 3b97c38eb..f62914808 100644 --- a/website/source/docs/builders/amazon-chroot.html.md +++ b/website/source/docs/builders/amazon-chroot.html.md @@ -173,8 +173,11 @@ each category, the available configuration keys are alphabetized. more [block device mappings](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/block-device-mapping-concepts.html) to the AMI. These will be attached when booting a new instance from your - AMI. Your options here may vary depending on the type of VM you use. The - block device mappings allow for the following configuration: + AMI. If this field is populated, and you are building from an existing source image, + the block device mappings in the source image will be overwritten. This means you + must have a block device mapping entry for your root volume, `root_volume_size `, + and `root_device_name`. `Your options here may vary depending on the type of VM + you use. The block device mappings allow for the following configuration: - `delete_on_termination` (boolean) - Indicates whether the EBS volume is deleted on instance termination. Default `false`. **NOTE**: If this