fix: select the root block device mapping by name and refuse the rest (#1081)

RunInstances read BlockDeviceMappings[0] and nothing else, so a request carrying a root mapping and a data mapping created one volume sized from whichever happened to be first, and the first mapping became the root whatever device it named. A module emitting the data mapping first produced a root volume of the data volume's size and no data volume at all, with no error.

The root is now the mapping naming the AMI root device, with position deciding only when nothing names it, so the single /dev/vda mapping ECS and EKS launch with still works. Any further EBS mapping is refused in ValidateRunInstancesInput with InvalidParameterValue naming BlockDeviceMappings and the devices it cannot attach, rather than being discarded silently. Ephemeral mappings carry no volume and are ignored.

Creating the additional volumes at launch remains unimplemented; this makes its absence visible.

spinifex/ebsmetadata/metadata.go

@@ -85,6 +85,11 @@ type Snapshot struct {
 	ProviderHandle   string            `json:"provider_handle,omitempty"`
 }
 
+// RootDeviceName is the device every image this platform serves reports as its
+// root. DescribeImages returns it, so it is also the name a caller's root block
+// device mapping carries, and the name that says which mapping is the root.
+const RootDeviceName = "/dev/sda1"
+
 // AMI is the control-plane record used for EC2 image operations.
 type AMI struct {
 	SchemaVersion   uint16            `json:"schema_version"`

spinifex/gateway/ec2/instance/RunInstances.go

@@ -10,6 +10,8 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/mulgadc/spinifex/spinifex/awserrors"
+	"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
+	handlers_ec2_instance "github.com/mulgadc/spinifex/spinifex/handlers/ec2/instance"
 	handlers_ec2_launchtemplate "github.com/mulgadc/spinifex/spinifex/handlers/ec2/launchtemplate"
 	handlers_ec2_placementgroup "github.com/mulgadc/spinifex/spinifex/handlers/ec2/placementgroup"
 	handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam"
@@ -72,6 +74,12 @@ func ValidateRunInstancesInput(input *ec2.RunInstancesInput) (err error) {
 		return errors.New(awserrors.ErrorInvalidParameterValue)
 	}
 
+	// The launch path creates the root volume and nothing else, so a mapping
+	// for any other device is refused by name rather than silently dropped.
+	if unserved := handlers_ec2_instance.UnservedBlockDeviceMappings(input.BlockDeviceMappings); len(unserved) > 0 {
+		return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "BlockDeviceMappings: only the root device %s is created at launch; cannot attach %s", ebsmetadata.RootDeviceName, strings.Join(unserved, ", "))
+	}
+
 	return err
 }
 

spinifex/gateway/ec2/instance/RunInstances_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/mulgadc/spinifex/spinifex/awserrors"
+	"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
 	handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -241,6 +242,41 @@ func TestParseRunInstances(t *testing.T) {
 		assert.NoError(t, ValidateRunInstancesInput(&input))
 	})
 
+	t.Run("RootOnlyBlockDeviceMappingIsValid", func(t *testing.T) {
+		t.Parallel()
+		input := defaults
+		input.BlockDeviceMappings = []*ec2.BlockDeviceMapping{
+			{DeviceName: aws.String(ebsmetadata.RootDeviceName), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8)}},
+		}
+		assert.NoError(t, ValidateRunInstancesInput(&input))
+	})
+
+	t.Run("EphemeralBlockDeviceMappingIsValid", func(t *testing.T) {
+		t.Parallel()
+		input := defaults
+		input.BlockDeviceMappings = []*ec2.BlockDeviceMapping{
+			{DeviceName: aws.String(ebsmetadata.RootDeviceName), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8)}},
+			{DeviceName: aws.String("/dev/sdb"), VirtualName: aws.String("ephemeral0")},
+		}
+		assert.NoError(t, ValidateRunInstancesInput(&input))
+	})
+
+	t.Run("AdditionalEBSBlockDeviceMappingIsRefusedByName", func(t *testing.T) {
+		t.Parallel()
+		input := defaults
+		input.BlockDeviceMappings = []*ec2.BlockDeviceMapping{
+			{DeviceName: aws.String("/dev/sdf"), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(4)}},
+			{DeviceName: aws.String(ebsmetadata.RootDeviceName), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8)}},
+		}
+		err := ValidateRunInstancesInput(&input)
+		require.Error(t, err)
+		code, message, ok := awserrors.ResolveErrorDetail(err)
+		require.True(t, ok)
+		assert.Equal(t, awserrors.ErrorInvalidParameterValue, code)
+		assert.Contains(t, message, "BlockDeviceMappings")
+		assert.Contains(t, message, "/dev/sdf")
+	})
+
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()

spinifex/handlers/ec2/instance/service_impl.go

@@ -82,6 +82,52 @@ func floorVolumeSizeToAMI(ctx context.Context, loader AMIMetaLoader, imageID str
 	return int(amiSize)
 }
 
+// isEphemeralMapping reports whether a mapping names an instance store device
+// rather than an EBS volume. These carry a VirtualName and no Ebs block, so the
+// launch path neither creates nor owes them a volume.
+func isEphemeralMapping(bdm *ec2.BlockDeviceMapping) bool {
+	return bdm != nil && bdm.Ebs == nil && aws.StringValue(bdm.VirtualName) != ""
+}
+
+// SelectRootBlockDeviceMapping returns the index of the mapping addressing the
+// root device, which is the one naming ebsmetadata.RootDeviceName. Position
+// decides only when no mapping names it, so a caller addressing a device this
+// platform's images do not have still gets the volume they sized.
+func SelectRootBlockDeviceMapping(mappings []*ec2.BlockDeviceMapping) int {
+	first := -1
+	for i, bdm := range mappings {
+		if bdm == nil || isEphemeralMapping(bdm) {
+			continue
+		}
+		if aws.StringValue(bdm.DeviceName) == ebsmetadata.RootDeviceName {
+			return i
+		}
+		if first < 0 {
+			first = i
+		}
+	}
+	return first
+}
+
+// UnservedBlockDeviceMappings names the EBS mappings the launch path will not
+// create a volume for. Only the root is served, so any other mapping is a
+// request the platform cannot meet and must refuse rather than discard.
+func UnservedBlockDeviceMappings(mappings []*ec2.BlockDeviceMapping) []string {
+	root := SelectRootBlockDeviceMapping(mappings)
+	var unserved []string
+	for i, bdm := range mappings {
+		if i == root || bdm == nil || isEphemeralMapping(bdm) {
+			continue
+		}
+		name := aws.StringValue(bdm.DeviceName)
+		if name == "" {
+			name = "(unnamed)"
+		}
+		unserved = append(unserved, name)
+	}
+	return unserved
+}
+
 // parseVolumeParams extracts volume parameters from RunInstancesInput,
 // applying defaults and resolving AMI-based image IDs.
 func parseVolumeParams(input *ec2.RunInstancesInput) volumeParams {
@@ -91,8 +137,8 @@ func parseVolumeParams(input *ec2.RunInstancesInput) volumeParams {
 		deleteOnTermination: true, // matches AWS RunInstances behavior
 	}
 
-	if len(input.BlockDeviceMappings) > 0 {
-		bdm := input.BlockDeviceMappings[0]
+	if root := SelectRootBlockDeviceMapping(input.BlockDeviceMappings); root >= 0 {
+		bdm := input.BlockDeviceMappings[root]
 		if bdm.DeviceName != nil {
 			p.deviceName = *bdm.DeviceName
 		}

spinifex/handlers/ec2/instance/service_impl_test.go

@@ -485,7 +485,7 @@ func TestPrepareRunInstances_ProjectsPlacementGroup(t *testing.T) {
 }
 
 func TestParseVolumeParams_MultipleBlockDeviceMappings(t *testing.T) {
-	// parseVolumeParams only uses the first block device mapping
+	// The root mapping is the one naming the AMI root device, not the first.
 	input := &ec2.RunInstancesInput{
 		ImageId: aws.String("ami-multi"),
 		BlockDeviceMappings: []*ec2.BlockDeviceMapping{
@@ -508,12 +508,87 @@ func TestParseVolumeParams_MultipleBlockDeviceMappings(t *testing.T) {
 	}
 
 	p := parseVolumeParams(input)
-	// First mapping wins for root volume
 	assert.Equal(t, 30*1024*1024*1024, p.size)
 	assert.Equal(t, "/dev/sda1", p.deviceName)
 	assert.Equal(t, "gp3", p.volumeType)
 }
 
+func TestParseVolumeParams_RootMappingSelectedByName(t *testing.T) {
+	// The data mapping is listed first, as the ec2-instance Terraform module
+	// emits it; the root must still be the one naming the AMI root device.
+	input := &ec2.RunInstancesInput{
+		ImageId: aws.String("ami-order"),
+		BlockDeviceMappings: []*ec2.BlockDeviceMapping{
+			{
+				DeviceName: aws.String("/dev/sdf"),
+				Ebs:        &ec2.EbsBlockDevice{VolumeSize: aws.Int64(4)},
+			},
+			{
+				DeviceName: aws.String(ebsmetadata.RootDeviceName),
+				Ebs:        &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8), VolumeType: aws.String("gp3")},
+			},
+		},
+	}
+
+	p := parseVolumeParams(input)
+	assert.Equal(t, 8*1024*1024*1024, p.size)
+	assert.Equal(t, ebsmetadata.RootDeviceName, p.deviceName)
+	assert.Equal(t, "gp3", p.volumeType)
+}
+
+func TestParseVolumeParams_SingleNonRootMappingStillLaunches(t *testing.T) {
+	// ECS and EKS both launch with one mapping named /dev/vda, which is not the
+	// AMI root device name. Position decides when nothing names the root.
+	input := &ec2.RunInstancesInput{
+		ImageId: aws.String("ami-vda"),
+		BlockDeviceMappings: []*ec2.BlockDeviceMapping{
+			{
+				DeviceName: aws.String("/dev/vda"),
+				Ebs:        &ec2.EbsBlockDevice{VolumeSize: aws.Int64(20)},
+			},
+		},
+	}
+
+	p := parseVolumeParams(input)
+	assert.Equal(t, 20*1024*1024*1024, p.size)
+	assert.Equal(t, "/dev/vda", p.deviceName)
+	assert.Empty(t, UnservedBlockDeviceMappings(input.BlockDeviceMappings))
+}
+
+func TestUnservedBlockDeviceMappings(t *testing.T) {
+	root := &ec2.BlockDeviceMapping{
+		DeviceName: aws.String(ebsmetadata.RootDeviceName),
+		Ebs:        &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8)},
+	}
+	data := &ec2.BlockDeviceMapping{
+		DeviceName: aws.String("/dev/sdf"),
+		Ebs:        &ec2.EbsBlockDevice{VolumeSize: aws.Int64(4)},
+	}
+	ephemeral := &ec2.BlockDeviceMapping{
+		DeviceName:  aws.String("/dev/sdb"),
+		VirtualName: aws.String("ephemeral0"),
+	}
+
+	tests := []struct {
+		name     string
+		mappings []*ec2.BlockDeviceMapping
+		want     []string
+	}{
+		{name: "none", mappings: nil},
+		{name: "root only", mappings: []*ec2.BlockDeviceMapping{root}},
+		{name: "data after root", mappings: []*ec2.BlockDeviceMapping{root, data}, want: []string{"/dev/sdf"}},
+		{name: "data before root", mappings: []*ec2.BlockDeviceMapping{data, root}, want: []string{"/dev/sdf"}},
+		{name: "ephemeral ignored", mappings: []*ec2.BlockDeviceMapping{root, ephemeral}},
+		{name: "ephemeral only", mappings: []*ec2.BlockDeviceMapping{ephemeral}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, UnservedBlockDeviceMappings(tt.mappings))
+		})
+	}
+}
+
 func TestParseVolumeParams_Io1WithIops(t *testing.T) {
 	input := &ec2.RunInstancesInput{
 		ImageId: aws.String("ami-io1"),