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

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

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

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,
 	}

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