diff --git a/spinifex/ebsmetadata/metadata.go b/spinifex/ebsmetadata/metadata.go index 4560164fe..db028ed78 100644 --- a/spinifex/ebsmetadata/metadata.go +++ b/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"` diff --git a/spinifex/gateway/ec2/instance/RunInstances.go b/spinifex/gateway/ec2/instance/RunInstances.go index 947a7c689..5360fcdf2 100644 --- a/spinifex/gateway/ec2/instance/RunInstances.go +++ b/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 } diff --git a/spinifex/gateway/ec2/instance/RunInstances_test.go b/spinifex/gateway/ec2/instance/RunInstances_test.go index 25f2777d4..4ccfec616 100644 --- a/spinifex/gateway/ec2/instance/RunInstances_test.go +++ b/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() diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index 7b215ff39..ad0e35888 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/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 } diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index 3c2f24698..473c2dfb8 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/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"),