From 6f41370ac39583fecfa28aa9a0cdfdfdc15d342c Mon Sep 17 00:00:00 2001 From: Alexander Demichev Date: Tue, 2 Jun 2020 12:36:55 +0200 Subject: [PATCH] Add support for multiple block device mappings --- pkg/actuators/machine/instances.go | 74 ++++++---- pkg/actuators/machine/instances_test.go | 139 ++++++++++++++---- .../v1beta1/awsproviderconfig_types.go | 3 +- 3 files changed, 161 insertions(+), 55 deletions(-) diff --git a/pkg/actuators/machine/instances.go b/pkg/actuators/machine/instances.go index f460013db6..c3b4a3c994 100644 --- a/pkg/actuators/machine/instances.go +++ b/pkg/actuators/machine/instances.go @@ -2,6 +2,7 @@ package machine import ( "encoding/base64" + "errors" "fmt" "sort" "strconv" @@ -181,9 +182,11 @@ func getAMI(AMI awsproviderv1.AWSResourceReference, client awsclient.Client) (*s return nil, fmt.Errorf("AMI ID or AMI filters need to be specified") } -func getBlockDeviceMappings(blockDeviceMappings []awsproviderv1.BlockDeviceMappingSpec, AMI string, client awsclient.Client) ([]*ec2.BlockDeviceMapping, error) { - if len(blockDeviceMappings) == 0 || blockDeviceMappings[0].EBS == nil { - return []*ec2.BlockDeviceMapping{}, nil +func getBlockDeviceMappings(blockDeviceMappingSpecs []awsproviderv1.BlockDeviceMappingSpec, AMI string, client awsclient.Client) ([]*ec2.BlockDeviceMapping, error) { + blockDeviceMappings := make([]*ec2.BlockDeviceMapping, 0) + + if len(blockDeviceMappingSpecs) == 0 { + return blockDeviceMappings, nil } // Get image object to get the RootDeviceName @@ -199,33 +202,52 @@ func getBlockDeviceMappings(blockDeviceMappings []awsproviderv1.BlockDeviceMappi klog.Errorf("No image for given AMI was found") return nil, fmt.Errorf("no image for given AMI not found") } - deviceName := describeAMIResult.Images[0].RootDeviceName - - // Only support one blockDeviceMapping - volumeSize := blockDeviceMappings[0].EBS.VolumeSize - volumeType := blockDeviceMappings[0].EBS.VolumeType - blockDeviceMapping := ec2.BlockDeviceMapping{ - DeviceName: deviceName, - Ebs: &ec2.EbsBlockDevice{ - VolumeSize: volumeSize, - VolumeType: volumeType, - Encrypted: blockDeviceMappings[0].EBS.Encrypted, - }, - } - if aws.StringValue(blockDeviceMappings[0].EBS.KMSKey.ID) != "" { - klog.V(3).Infof("Using KMS key ID %q for encrypting EBS volume", *blockDeviceMappings[0].EBS.KMSKey.ID) - blockDeviceMapping.Ebs.KmsKeyId = blockDeviceMappings[0].EBS.KMSKey.ID - } else if aws.StringValue(blockDeviceMappings[0].EBS.KMSKey.ARN) != "" { - klog.V(3).Info("Using KMS key ARN for encrypting EBS volume") // ARN usually have account ids, therefore are sensitive data so shouldn't log the value - blockDeviceMapping.Ebs.KmsKeyId = blockDeviceMappings[0].EBS.KMSKey.ARN - } + rootDeviceFound := false + for _, blockDeviceMappingSpec := range blockDeviceMappingSpecs { + if blockDeviceMappingSpec.EBS == nil { + continue + } + + deviceName := blockDeviceMappingSpec.DeviceName + volumeSize := blockDeviceMappingSpec.EBS.VolumeSize + volumeType := blockDeviceMappingSpec.EBS.VolumeType + deleteOnTermination := true + + if blockDeviceMappingSpec.DeviceName == nil { + if rootDeviceFound { + return nil, errors.New("non root device must have name") + } + rootDeviceFound = true + deviceName = describeAMIResult.Images[0].RootDeviceName + } + + blockDeviceMapping := ec2.BlockDeviceMapping{ + DeviceName: deviceName, + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: volumeSize, + VolumeType: volumeType, + Encrypted: blockDeviceMappingSpec.EBS.Encrypted, + DeleteOnTermination: &deleteOnTermination, + }, + } + + if *volumeType == ec2.VolumeTypeIo1 { + blockDeviceMapping.Ebs.Iops = blockDeviceMappingSpec.EBS.Iops + } + + if aws.StringValue(blockDeviceMappingSpec.EBS.KMSKey.ID) != "" { + klog.V(3).Infof("Using KMS key ID %q for encrypting EBS volume", *blockDeviceMappingSpec.EBS.KMSKey.ID) + blockDeviceMapping.Ebs.KmsKeyId = blockDeviceMappingSpec.EBS.KMSKey.ID + } else if aws.StringValue(blockDeviceMappingSpec.EBS.KMSKey.ARN) != "" { + klog.V(3).Info("Using KMS key ARN for encrypting EBS volume") // ARN usually have account ids, therefore are sensitive data so shouldn't log the value + blockDeviceMapping.Ebs.KmsKeyId = blockDeviceMappingSpec.EBS.KMSKey.ARN + } - if *volumeType == "io1" { - blockDeviceMapping.Ebs.Iops = blockDeviceMappings[0].EBS.Iops + blockDeviceMappings = append(blockDeviceMappings, &blockDeviceMapping) } - return []*ec2.BlockDeviceMapping{&blockDeviceMapping}, nil + return blockDeviceMappings, nil } func launchInstance(machine *machinev1.Machine, machineProviderConfig *awsproviderv1.AWSMachineProviderConfig, userData []byte, client awsclient.Client) (*ec2.Instance, error) { diff --git a/pkg/actuators/machine/instances_test.go b/pkg/actuators/machine/instances_test.go index 1d286a9514..c799d6b376 100644 --- a/pkg/actuators/machine/instances_test.go +++ b/pkg/actuators/machine/instances_test.go @@ -93,7 +93,11 @@ func TestBuildEC2Filters(t *testing.T) { func TestGetBlockDeviceMappings(t *testing.T) { rootDeviceName := "/dev/sda1" volumeSize := int64(16384) + deviceName2 := "/dev/sda2" + volumeSize2 := int64(16385) + deleteOnTermination := true volumeType := "ssd" + mockCtrl := gomock.NewController(t) mockAWSClient := mockaws.NewMockClient(mockCtrl) mockAWSClient.EXPECT().DescribeImages(gomock.Any()).Return(&ec2.DescribeImagesOutput{ @@ -106,10 +110,88 @@ func TestGetBlockDeviceMappings(t *testing.T) { }, }, nil).AnyTimes() + oneBlockDevice := []awsproviderv1.BlockDeviceMappingSpec{ + { + DeviceName: &rootDeviceName, + EBS: &awsproviderv1.EBSBlockDeviceSpec{ + VolumeSize: &volumeSize, + VolumeType: &volumeType, + }, + NoDevice: nil, + VirtualName: nil, + }, + } + + oneExpectedBlockDevice := []*ec2.BlockDeviceMapping{ + { + DeviceName: &rootDeviceName, + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: &volumeSize, + VolumeType: &volumeType, + DeleteOnTermination: &deleteOnTermination, + }, + NoDevice: nil, + VirtualName: nil, + }, + } + + blockDevices := []awsproviderv1.BlockDeviceMappingSpec{ + { + DeviceName: &rootDeviceName, + EBS: &awsproviderv1.EBSBlockDeviceSpec{ + VolumeSize: &volumeSize, + VolumeType: &volumeType, + }, + NoDevice: nil, + VirtualName: nil, + }, + { + DeviceName: &deviceName2, + EBS: &awsproviderv1.EBSBlockDeviceSpec{ + VolumeSize: &volumeSize2, + VolumeType: &volumeType, + }, + NoDevice: nil, + VirtualName: nil, + }, + } + + twoExpectedDevices := []*ec2.BlockDeviceMapping{ + { + DeviceName: &rootDeviceName, + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: &volumeSize, + VolumeType: &volumeType, + DeleteOnTermination: &deleteOnTermination, + }, + NoDevice: nil, + VirtualName: nil, + }, + { + DeviceName: &deviceName2, + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: &volumeSize2, + VolumeType: &volumeType, + DeleteOnTermination: &deleteOnTermination, + }, + NoDevice: nil, + VirtualName: nil, + }, + } + + blockDevicesOneEmptyName := make([]awsproviderv1.BlockDeviceMappingSpec, len(blockDevices)) + copy(blockDevicesOneEmptyName, blockDevices) + blockDevicesOneEmptyName[0].DeviceName = nil + + blockDevicesTwoEmptyNames := make([]awsproviderv1.BlockDeviceMappingSpec, len(blockDevicesOneEmptyName)) + copy(blockDevicesTwoEmptyNames, blockDevicesOneEmptyName) + blockDevicesTwoEmptyNames[1].DeviceName = nil + testCases := []struct { description string blockDevices []awsproviderv1.BlockDeviceMappingSpec expected []*ec2.BlockDeviceMapping + expectedErr bool }{ { description: "When it gets an empty blockDevices list", @@ -117,39 +199,40 @@ func TestGetBlockDeviceMappings(t *testing.T) { expected: []*ec2.BlockDeviceMapping{}, }, { - description: "When it gets one blockDevice", - blockDevices: []awsproviderv1.BlockDeviceMappingSpec{ - { - DeviceName: &rootDeviceName, - EBS: &awsproviderv1.EBSBlockDeviceSpec{ - VolumeSize: &volumeSize, - VolumeType: &volumeType, - }, - NoDevice: nil, - VirtualName: nil, - }, - }, - expected: []*ec2.BlockDeviceMapping{ - { - DeviceName: &rootDeviceName, - Ebs: &ec2.EbsBlockDevice{ - VolumeSize: &volumeSize, - VolumeType: &volumeType, - }, - NoDevice: nil, - VirtualName: nil, - }, - }, + description: "When it gets one blockDevice", + blockDevices: oneBlockDevice, + expected: oneExpectedBlockDevice, + }, + { + description: "When it gets two blockDevices", + blockDevices: blockDevices, + expected: twoExpectedDevices, + }, + { + description: "When it gets two blockDevices and one with empty device name", + blockDevices: blockDevicesOneEmptyName, + expected: twoExpectedDevices, + }, + { + description: "Fail when it gets two blockDevices and two with empty device name", + blockDevices: blockDevicesTwoEmptyNames, + expectedErr: true, }, } for _, tc := range testCases { got, err := getBlockDeviceMappings(tc.blockDevices, "existing-AMI", mockAWSClient) - if err != nil { - t.Errorf("error when calling getBlockDeviceMappings: %v", err) - } - if !reflect.DeepEqual(got, tc.expected) { - t.Errorf("Case: %s. Got: %v, expected: %v", tc.description, got, tc.expected) + if tc.expectedErr { + if err == nil { + t.Error("Expected error") + } + } else { + if err != nil { + t.Errorf("error when calling getBlockDeviceMappings: %v", err) + } + if !reflect.DeepEqual(got, tc.expected) { + t.Errorf("Got: %v, expected: %v", got, tc.expected) + } } } } diff --git a/pkg/apis/awsprovider/v1beta1/awsproviderconfig_types.go b/pkg/apis/awsprovider/v1beta1/awsproviderconfig_types.go index 8b566a1b24..2a958ccc8b 100644 --- a/pkg/apis/awsprovider/v1beta1/awsproviderconfig_types.go +++ b/pkg/apis/awsprovider/v1beta1/awsproviderconfig_types.go @@ -81,7 +81,8 @@ type AWSMachineProviderConfig struct { // should be added once it is created. LoadBalancers []LoadBalancerReference `json:"loadBalancers,omitempty"` - // BlockDevices is the set of block device mapping associated to this instance + // BlockDevices is the set of block device mapping associated to this instance, + // block device without a name will be used as a root device and only one device without a name is allowed // https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/block-device-mapping-concepts.html BlockDevices []BlockDeviceMappingSpec `json:"blockDevices,omitempty"`