From 2a94b596ce93237517026ddd220da8b12f0132c7 Mon Sep 17 00:00:00 2001
From: Or Cohen <orc@fewbytes.com>
Date: Tue, 25 Aug 2015 00:19:11 +0300
Subject: [PATCH] Fix and refactor block device mapping builder

Fix NoDevice not properly configured #2398.
Refactor the mapping builder to match BlockDeviceMapping from AWS SDK:

* If NoDevice is specified, include NoDevice only.
* If VirtualName starts with ephemeral, don't create Ebs (they are mutually
  exclusive anyway)
* Otherwise, assume Ebs and create it with the exact specified attributes by
  the user.

Change/add tests to reflect these changes.
---
 builder/amazon/common/block_device.go      | 51 ++++++++++++----------
 builder/amazon/common/block_device_test.go | 34 +++++++++++++--
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/builder/amazon/common/block_device.go b/builder/amazon/common/block_device.go
index 094738869..73d17c45f 100644
--- a/builder/amazon/common/block_device.go
+++ b/builder/amazon/common/block_device.go
@@ -30,35 +30,40 @@ func buildBlockDevices(b []BlockDevice) []*ec2.BlockDeviceMapping {
 	var blockDevices []*ec2.BlockDeviceMapping
 
 	for _, blockDevice := range b {
-		ebsBlockDevice := &ec2.EbsBlockDevice{
-			VolumeType:          aws.String(blockDevice.VolumeType),
-			VolumeSize:          aws.Int64(blockDevice.VolumeSize),
-			DeleteOnTermination: aws.Bool(blockDevice.DeleteOnTermination),
+		mapping := &ec2.BlockDeviceMapping {
+			DeviceName: aws.String(blockDevice.DeviceName),
 		}
 
-		// IOPS is only valid for SSD Volumes
-		if blockDevice.VolumeType != "" && blockDevice.VolumeType != "standard" && blockDevice.VolumeType != "gp2" {
-			ebsBlockDevice.Iops = aws.Int64(blockDevice.IOPS)
-		}
+		if blockDevice.NoDevice {
+			mapping.NoDevice = aws.String("")
+		} else if strings.HasPrefix(blockDevice.VirtualName, "ephemeral") {
+			mapping.VirtualName = aws.String(blockDevice.VirtualName)
+		} else {
+			ebsBlockDevice := &ec2.EbsBlockDevice{
+				DeleteOnTermination: aws.Bool(blockDevice.DeleteOnTermination),
+			}
 
-		// You cannot specify Encrypted if you specify a Snapshot ID
-		if blockDevice.SnapshotId != "" {
-			ebsBlockDevice.SnapshotId = aws.String(blockDevice.SnapshotId)
-		} else if blockDevice.Encrypted {
-			ebsBlockDevice.Encrypted = aws.Bool(blockDevice.Encrypted)
-		}
+			if blockDevice.VolumeType != "" {
+				ebsBlockDevice.VolumeType = aws.String(blockDevice.VolumeType)
+			}
 
-		mapping := &ec2.BlockDeviceMapping{
-			DeviceName:  aws.String(blockDevice.DeviceName),
-			VirtualName: aws.String(blockDevice.VirtualName),
-		}
+			if blockDevice.VolumeSize > 0 {
+				ebsBlockDevice.VolumeSize = aws.Int64(blockDevice.VolumeSize)
+			}
 
-		if !strings.HasPrefix(blockDevice.VirtualName, "ephemeral") {
-			mapping.Ebs = ebsBlockDevice
-		}
+			// IOPS is only valid for io1 type
+			if blockDevice.VolumeType == "io1" {
+				ebsBlockDevice.Iops = aws.Int64(blockDevice.IOPS)
+			}
 
-		if blockDevice.NoDevice {
-			mapping.NoDevice = aws.String("")
+			// You cannot specify Encrypted if you specify a Snapshot ID
+			if blockDevice.SnapshotId != "" {
+				ebsBlockDevice.SnapshotId = aws.String(blockDevice.SnapshotId)
+			} else if blockDevice.Encrypted {
+				ebsBlockDevice.Encrypted = aws.Bool(blockDevice.Encrypted)
+			}
+
+			mapping.Ebs = ebsBlockDevice
 		}
 
 		blockDevices = append(blockDevices, mapping)
diff --git a/builder/amazon/common/block_device_test.go b/builder/amazon/common/block_device_test.go
index 99514009b..b73f4da36 100644
--- a/builder/amazon/common/block_device_test.go
+++ b/builder/amazon/common/block_device_test.go
@@ -24,7 +24,6 @@ func TestBlockDevice(t *testing.T) {
 
 			Result: &ec2.BlockDeviceMapping{
 				DeviceName:  aws.String("/dev/sdb"),
-				VirtualName: aws.String(""),
 				Ebs: &ec2.EbsBlockDevice{
 					SnapshotId:          aws.String("snap-1234"),
 					VolumeType:          aws.String("standard"),
@@ -41,9 +40,7 @@ func TestBlockDevice(t *testing.T) {
 
 			Result: &ec2.BlockDeviceMapping{
 				DeviceName:  aws.String("/dev/sdb"),
-				VirtualName: aws.String(""),
 				Ebs: &ec2.EbsBlockDevice{
-					VolumeType:          aws.String(""),
 					VolumeSize:          aws.Int64(8),
 					DeleteOnTermination: aws.Bool(false),
 				},
@@ -60,7 +57,6 @@ func TestBlockDevice(t *testing.T) {
 
 			Result: &ec2.BlockDeviceMapping{
 				DeviceName:  aws.String("/dev/sdb"),
-				VirtualName: aws.String(""),
 				Ebs: &ec2.EbsBlockDevice{
 					VolumeType:          aws.String("io1"),
 					VolumeSize:          aws.Int64(8),
@@ -69,6 +65,25 @@ func TestBlockDevice(t *testing.T) {
 				},
 			},
 		},
+		{
+			Config: &BlockDevice{
+				DeviceName:          "/dev/sdb",
+				VolumeType:          "gp2",
+				VolumeSize:          8,
+				DeleteOnTermination: true,
+				Encrypted:           true,
+			},
+
+			Result: &ec2.BlockDeviceMapping{
+				DeviceName:  aws.String("/dev/sdb"),
+				Ebs: &ec2.EbsBlockDevice{
+					VolumeType:          aws.String("gp2"),
+					VolumeSize:          aws.Int64(8),
+					DeleteOnTermination: aws.Bool(true),
+					Encrypted:           aws.Bool(true),
+				},
+			},
+		},
 		{
 			Config: &BlockDevice{
 				DeviceName:  "/dev/sdb",
@@ -80,6 +95,17 @@ func TestBlockDevice(t *testing.T) {
 				VirtualName: aws.String("ephemeral0"),
 			},
 		},
+		{
+			Config: &BlockDevice{
+				DeviceName: "/dev/sdb",
+				NoDevice:   true,
+			},
+
+			Result: &ec2.BlockDeviceMapping{
+				DeviceName:  aws.String("/dev/sdb"),
+				NoDevice:    aws.String(""),
+			},
+		},
 	}
 
 	for _, tc := range cases {