diff --git a/builder/amazon/chroot/builder.go b/builder/amazon/chroot/builder.go index 78072ecd5..8a4d414c3 100644 --- a/builder/amazon/chroot/builder.go +++ b/builder/amazon/chroot/builder.go @@ -165,11 +165,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 dbb84a5be..da60e917a 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,11 +73,65 @@ func (s *StepRegisterAMI) Run(ctx context.Context, state multistep.StateBag) mul func (s *StepRegisterAMI) Cleanup(state multistep.StateBag) {} -func buildRegisterOpts(config *Config, image *ec2.Image, mappings []*ec2.BlockDeviceMapping) *ec2.RegisterImageInput { +// 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 + ) + + generatingNewBlockDeviceMappings := config.FromScratch || len(config.AMIMappings) > 0 + if generatingNewBlockDeviceMappings { + 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 generatingNewBlockDeviceMappings || 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 buildRegisterOptsFromExistingImage(config, sourceImage, newMappings, rootDeviceName) +} + +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 0cc3bc912..8c686b4bc 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" @@ -21,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 { @@ -43,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) { @@ -50,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 { @@ -70,4 +78,142 @@ 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) { + 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) +} + +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) + } } diff --git a/website/source/docs/builders/amazon-chroot.html.md b/website/source/docs/builders/amazon-chroot.html.md index ccc44f100..cba9a0b1e 100644 --- a/website/source/docs/builders/amazon-chroot.html.md +++ b/website/source/docs/builders/amazon-chroot.html.md @@ -172,8 +172,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