Merge pull request #7370 from sargun/allow-users-to-specify-custom-block-device-mapping

amazon-chroot: Allow users to specify custom block device mapping
2019-03-12 14:21:23 -07:00 · 2019-03-12 14:21:23 -07:00 · 0196d7f835
parent fe8f9e98eb 0e8eb61699
commit 0196d7f835
5 changed files with 275 additions and 59 deletions
--- 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"))
 		}
 	}

--- 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")
+	}
+}
--- 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,
 	}
--- 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)
+	}
 }
--- 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