From 42e61c77c6745d3e3b82aa225a26d7c86c1ed73e Mon Sep 17 00:00:00 2001 From: Connor Catlett Date: Fri, 10 Mar 2023 15:47:31 +0000 Subject: [PATCH] Prevent allocation of devices after /dev/xvddx EBS has undocumented validation on the device that prevents the use of device names after /dev/xvddx. Also, expand the beginning of the device pool to account for this smaller pool of device names. Signed-off-by: Connor Catlett --- Makefile | 4 ++-- pkg/cloud/cloud_test.go | 28 +++++++++++------------ pkg/cloud/devicemanager/allocator.go | 12 +++++++--- pkg/cloud/devicemanager/allocator_test.go | 12 ++++++---- pkg/cloud/devicemanager/manager_test.go | 8 +++---- 5 files changed, 37 insertions(+), 27 deletions(-) diff --git a/Makefile b/Makefile index 11d7a7a232..8c1fa556d5 100644 --- a/Makefile +++ b/Makefile @@ -35,8 +35,8 @@ OS?=linux ARCH?=amd64 OSVERSION?=amazon -ALL_OS?=linux windows -ALL_ARCH_linux?=amd64 arm64 +ALL_OS?=linux +ALL_ARCH_linux?=amd64 ALL_OSVERSION_linux?=amazon ALL_OS_ARCH_OSVERSION_linux=$(foreach arch, $(ALL_ARCH_linux), $(foreach osversion, ${ALL_OSVERSION_linux}, linux-$(arch)-${osversion})) diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index 5b21b7ca22..975ca36c72 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -721,7 +721,7 @@ func TestAttachDisk(t *testing.T) { vol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("node-1234"), State: aws.String("attached")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("node-1234"), State: aws.String("attached")}}, } ctx := context.Background() @@ -1614,7 +1614,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: false, }, @@ -1623,7 +1623,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeDetachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: false, }, @@ -1632,7 +1632,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeDetachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: false, }, @@ -1641,7 +1641,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: true, }, @@ -1650,7 +1650,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdbb", + expectedDevice: "/dev/xvdab", alreadyAssigned: false, expectError: true, }, @@ -1659,7 +1659,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1235", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: true, }, @@ -1668,7 +1668,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: true, expectError: true, }, @@ -1677,7 +1677,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: false, }, @@ -1686,7 +1686,7 @@ func TestWaitForAttachmentState(t *testing.T) { volumeID: "vol-test-1234", expectedState: volumeAttachedState, expectedInstance: "1234", - expectedDevice: "/dev/xvdba", + expectedDevice: "/dev/xvdaa", alreadyAssigned: false, expectError: true, }, @@ -1702,22 +1702,22 @@ func TestWaitForAttachmentState(t *testing.T) { attachedVol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("attached")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("attached")}}, } attachingVol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("attaching")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("attaching")}}, } detachedVol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("detached")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("detached")}}, } multipleAttachmentsVol := &ec2.Volume{ VolumeId: aws.String(tc.volumeID), - Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1235"), State: aws.String("attached")}, {Device: aws.String("/dev/xvdba"), InstanceId: aws.String("1234"), State: aws.String("attached")}}, + Attachments: []*ec2.VolumeAttachment{{Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1235"), State: aws.String("attached")}, {Device: aws.String("/dev/xvdaa"), InstanceId: aws.String("1234"), State: aws.String("attached")}}, } ctx := context.Background() diff --git a/pkg/cloud/devicemanager/allocator.go b/pkg/cloud/devicemanager/allocator.go index 56a3413372..208fa53f34 100644 --- a/pkg/cloud/devicemanager/allocator.go +++ b/pkg/cloud/devicemanager/allocator.go @@ -46,12 +46,18 @@ var _ NameAllocator = &nameAllocator{} // GetNext gets next available device given existing names that are being used // This function iterate through the device names in deterministic order of: // -// ba, ..., bz, ca, ..., cz, ..., ..., zz +// aa, ..., az, ba, ..., bz, ..., ..., dx // // and return the first one that is not used yet. +// We stop at dx because EBS performs undocumented validation on the device +// name that refuses to mount devices after /dev/xvddx func (d *nameAllocator) GetNext(existingNames ExistingNames, prefix string) (string, error) { - for c1 := 'b'; c1 <= 'z'; c1++ { - for c2 := 'a'; c2 <= 'z'; c2++ { + for c1 := 'a'; c1 <= 'd'; c1++ { + c2end := 'z' + if c1 == 'd' { + c2end = 'x' + } + for c2 := 'a'; c2 <= c2end; c2++ { name := fmt.Sprintf("%s%s%s", prefix, string(c1), string(c2)) if _, found := existingNames[name]; !found { return name, nil diff --git a/pkg/cloud/devicemanager/allocator_test.go b/pkg/cloud/devicemanager/allocator_test.go index 9995efdf89..e6945f7c35 100644 --- a/pkg/cloud/devicemanager/allocator_test.go +++ b/pkg/cloud/devicemanager/allocator_test.go @@ -27,12 +27,18 @@ func TestNameAllocator(t *testing.T) { tests := []struct { expectedName string }{ + {"aa"}, {"ab"}, {"ac"}, {"ad"}, {"ae"}, {"af"}, {"ag"}, {"ah"}, {"ai"}, {"aj"}, + {"ak"}, {"al"}, {"am"}, {"an"}, {"ao"}, {"ap"}, {"aq"}, {"ar"}, {"as"}, {"at"}, + {"au"}, {"av"}, {"aw"}, {"ax"}, {"ay"}, {"az"}, {"ba"}, {"bb"}, {"bc"}, {"bd"}, {"be"}, {"bf"}, {"bg"}, {"bh"}, {"bi"}, {"bj"}, {"bk"}, {"bl"}, {"bm"}, {"bn"}, {"bo"}, {"bp"}, {"bq"}, {"br"}, {"bs"}, {"bt"}, {"bu"}, {"bv"}, {"bw"}, {"bx"}, {"by"}, {"bz"}, {"ca"}, {"cb"}, {"cc"}, {"cd"}, {"ce"}, {"cf"}, {"cg"}, {"ch"}, {"ci"}, {"cj"}, {"ck"}, {"cl"}, {"cm"}, {"cn"}, {"co"}, {"cp"}, {"cq"}, {"cr"}, {"cs"}, {"ct"}, {"cu"}, {"cv"}, {"cw"}, {"cx"}, {"cy"}, {"cz"}, + {"da"}, {"db"}, {"dc"}, {"dd"}, {"de"}, {"df"}, {"dg"}, {"dh"}, {"di"}, {"dj"}, + {"dk"}, {"dl"}, {"dm"}, {"dn"}, {"do"}, {"dp"}, {"dq"}, {"dr"}, {"ds"}, {"dt"}, + {"du"}, {"dv"}, {"dw"}, {"dx"}, } for _, test := range tests { @@ -53,10 +59,8 @@ func TestNameAllocatorError(t *testing.T) { allocator := nameAllocator{} existingNames := map[string]string{} - // Allocator goes from ba, bb, bc, ..., bz, ca, ..., cz, ..., ..., zz - // Thus 25*26 for the maximum number of allocatable volumes (25 for the - // first letter as it starts on 'b' - for i := 0; i < 25*26; i++ { + // 102 == number of allocations from aa ... dx (see allocator.go for why we stop at dx) + for i := 0; i < 102; i++ { name, _ := allocator.GetNext(existingNames, "/dev/xvd") existingNames[name] = "" } diff --git a/pkg/cloud/devicemanager/manager_test.go b/pkg/cloud/devicemanager/manager_test.go index 0f277be024..826ccf2c97 100644 --- a/pkg/cloud/devicemanager/manager_test.go +++ b/pkg/cloud/devicemanager/manager_test.go @@ -103,16 +103,16 @@ func TestNewDeviceWithExistingDevice(t *testing.T) { { name: "success: different volumes", existingID: "vol-1", - existingPath: "/dev/xvdba", + existingPath: "/dev/xvdaa", volumeID: "vol-2", - expectedPath: "/dev/xvdbb", + expectedPath: "/dev/xvdab", }, { name: "success: same volumes", existingID: "vol-1", - existingPath: "/dev/xvdba", + existingPath: "/dev/xvdcc", volumeID: "vol-1", - expectedPath: "/dev/xvdba", + expectedPath: "/dev/xvdcc", }, { name: "success: same volumes with /dev/sdX path",