From 1b9131b0fd4485b7f37190218ce5bd1ff91aaa83 Mon Sep 17 00:00:00 2001 From: Roman Bednar Date: Wed, 23 Aug 2023 15:42:50 +0200 Subject: [PATCH] check storage account type if parameter is missing --- pkg/azurefile/controllerserver.go | 11 ++ pkg/azurefile/controllerserver_test.go | 182 +++++++++++++++++++++++++ 2 files changed, 193 insertions(+) diff --git a/pkg/azurefile/controllerserver.go b/pkg/azurefile/controllerserver.go index bdb8e2ff3d..d3b45e4a05 100644 --- a/pkg/azurefile/controllerserver.go +++ b/pkg/azurefile/controllerserver.go @@ -360,6 +360,17 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } fileShareSize := int(requestGiB) + + if account != "" && resourceGroup != "" && sku == "" && fileShareSize < minimumPremiumShareSize { + accountProperties, err := d.cloud.StorageAccountClient.GetProperties(ctx, subsID, resourceGroup, account) + if err != nil { + klog.Warningf("failed to get properties on storage account account(%s) rg(%s), error: %v", account, resourceGroup, err) + } + if accountProperties.Sku != nil { + sku = string(accountProperties.Sku.Name) + } + } + // account kind should be FileStorage for Premium File accountKind := string(storage.KindStorageV2) if strings.HasPrefix(strings.ToLower(sku), premium) { diff --git a/pkg/azurefile/controllerserver_test.go b/pkg/azurefile/controllerserver_test.go index 2c57ad312a..272abcbecc 100644 --- a/pkg/azurefile/controllerserver_test.go +++ b/pkg/azurefile/controllerserver_test.go @@ -23,6 +23,7 @@ import ( "net/http" "net/url" "reflect" + "sigs.k8s.io/cloud-provider-azure/pkg/azureclients/fileclient" "strings" "sync" "testing" @@ -1267,6 +1268,187 @@ func TestCreateVolume(t *testing.T) { } }, }, + { + name: "Premium storage account type (sku) loads from storage account when not given as parameter and share request size is increased to min. size required by premium", + testFunc: func(t *testing.T) { + name := "stoacc" + sku := "premium" + value := "foo bar" + accounts := []storage.Account{ + {Name: &name, Sku: &storage.Sku{Name: storage.SkuName(sku)}}, + } + keys := storage.AccountListKeysResult{ + Keys: &[]storage.AccountKey{ + {Value: &value}, + }, + } + capRange := &csi.CapacityRange{RequiredBytes: 1024 * 1024 * 1024, LimitBytes: 1024 * 1024 * 1024} + + allParam := map[string]string{ + locationField: "loc", + storageAccountField: "stoacc", + resourceGroupField: "rg", + shareNameField: "", + diskNameField: "diskname.vhd", + fsTypeField: "", + storeAccountKeyField: "storeaccountkey", + secretNamespaceField: "default", + mountPermissionsField: "0755", + accountQuotaField: "1000", + protocolField: smb, + } + req := &csi.CreateVolumeRequest{ + Name: "vol-1", + Parameters: allParam, + VolumeCapabilities: stdVolCap, + CapacityRange: capRange, + } + + expectedShareOptions := &fileclient.ShareOptions{Name: "vol-1", Protocol: "SMB", RequestGiB: 100, AccessTier: "", RootSquash: "", Metadata: nil} + + d := NewFakeDriver() + + ctrl := gomock.NewController(t) + mockStorageAccountsClient := mockstorageaccountclient.NewMockInterface(ctrl) + mockFileClient := mockfileclient.NewMockInterface(ctrl) + d.cloud.FileClient = mockFileClient + d.cloud.StorageAccountClient = mockStorageAccountsClient + + mockFileClient.EXPECT().WithSubscriptionID(gomock.Any()).Return(mockFileClient).AnyTimes() + mockFileClient.EXPECT().CreateFileShare(context.TODO(), gomock.Any(), gomock.Any(), expectedShareOptions, gomock.Any()).Return(storage.FileShare{FileShareProperties: &storage.FileShareProperties{ShareQuota: nil}}, nil).AnyTimes() + mockFileClient.EXPECT().GetFileShare(context.TODO(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(storage.FileShare{FileShareProperties: &storage.FileShareProperties{ShareQuota: &fakeShareQuota}}, nil).AnyTimes() + mockStorageAccountsClient.EXPECT().ListKeys(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(keys, nil).AnyTimes() + mockStorageAccountsClient.EXPECT().ListByResourceGroup(gomock.Any(), gomock.Any(), gomock.Any()).Return(accounts, nil).AnyTimes() + mockStorageAccountsClient.EXPECT().Create(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + mockStorageAccountsClient.EXPECT().GetProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(accounts[0], nil).AnyTimes() + + _, err := d.CreateVolume(ctx, req) + + if !reflect.DeepEqual(err, nil) { + t.Errorf("Unexpected error: %v", err) + } + }, + }, + { + name: "Premium storage account type (sku) does not load from storage account for size request above min. premium size", + testFunc: func(t *testing.T) { + name := "stoacc" + sku := "premium" + value := "foo bar" + accounts := []storage.Account{ + {Name: &name, Sku: &storage.Sku{Name: storage.SkuName(sku)}}, + } + keys := storage.AccountListKeysResult{ + Keys: &[]storage.AccountKey{ + {Value: &value}, + }, + } + capRange := &csi.CapacityRange{RequiredBytes: 1024 * 1024 * 1024 * 100, LimitBytes: 1024 * 1024 * 1024 * 100} + + allParam := map[string]string{ + locationField: "loc", + storageAccountField: "stoacc", + resourceGroupField: "rg", + shareNameField: "", + diskNameField: "diskname.vhd", + fsTypeField: "", + storeAccountKeyField: "storeaccountkey", + secretNamespaceField: "default", + mountPermissionsField: "0755", + accountQuotaField: "1000", + protocolField: smb, + } + req := &csi.CreateVolumeRequest{ + Name: "vol-1", + Parameters: allParam, + VolumeCapabilities: stdVolCap, + CapacityRange: capRange, + } + + expectedShareOptions := &fileclient.ShareOptions{Name: "vol-1", Protocol: "SMB", RequestGiB: 100, AccessTier: "", RootSquash: "", Metadata: nil} + + d := NewFakeDriver() + + ctrl := gomock.NewController(t) + mockStorageAccountsClient := mockstorageaccountclient.NewMockInterface(ctrl) + mockFileClient := mockfileclient.NewMockInterface(ctrl) + d.cloud.FileClient = mockFileClient + d.cloud.StorageAccountClient = mockStorageAccountsClient + + mockFileClient.EXPECT().WithSubscriptionID(gomock.Any()).Return(mockFileClient).AnyTimes() + mockFileClient.EXPECT().CreateFileShare(context.TODO(), gomock.Any(), gomock.Any(), expectedShareOptions, gomock.Any()).Return(storage.FileShare{FileShareProperties: &storage.FileShareProperties{ShareQuota: nil}}, nil).AnyTimes() + mockFileClient.EXPECT().GetFileShare(context.TODO(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(storage.FileShare{FileShareProperties: &storage.FileShareProperties{ShareQuota: &fakeShareQuota}}, nil).AnyTimes() + mockStorageAccountsClient.EXPECT().ListKeys(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(keys, nil).AnyTimes() + mockStorageAccountsClient.EXPECT().ListByResourceGroup(gomock.Any(), gomock.Any(), gomock.Any()).Return(accounts, nil).AnyTimes() + mockStorageAccountsClient.EXPECT().Create(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + + _, err := d.CreateVolume(ctx, req) + + if !reflect.DeepEqual(err, nil) { + t.Errorf("Unexpected error: %v", err) + } + }, + }, + { + name: "Storage account type (sku) defaults to standard type and share request size is unchanged", + testFunc: func(t *testing.T) { + name := "stoacc" + sku := "" + value := "foo bar" + accounts := []storage.Account{ + {Name: &name, Sku: &storage.Sku{Name: storage.SkuName(sku)}}, + } + keys := storage.AccountListKeysResult{ + Keys: &[]storage.AccountKey{ + {Value: &value}, + }, + } + capRange := &csi.CapacityRange{RequiredBytes: 1024 * 1024 * 1024, LimitBytes: 1024 * 1024 * 1024} + + allParam := map[string]string{ + locationField: "loc", + storageAccountField: "stoacc", + resourceGroupField: "rg", + shareNameField: "", + diskNameField: "diskname.vhd", + fsTypeField: "", + storeAccountKeyField: "storeaccountkey", + secretNamespaceField: "default", + mountPermissionsField: "0755", + accountQuotaField: "1000", + } + req := &csi.CreateVolumeRequest{ + Name: "vol-1", + Parameters: allParam, + VolumeCapabilities: stdVolCap, + CapacityRange: capRange, + } + + expectedShareOptions := &fileclient.ShareOptions{Name: "vol-1", Protocol: "SMB", RequestGiB: 1, AccessTier: "", RootSquash: "", Metadata: nil} + + d := NewFakeDriver() + + ctrl := gomock.NewController(t) + mockStorageAccountsClient := mockstorageaccountclient.NewMockInterface(ctrl) + mockFileClient := mockfileclient.NewMockInterface(ctrl) + d.cloud.FileClient = mockFileClient + d.cloud.StorageAccountClient = mockStorageAccountsClient + + mockFileClient.EXPECT().WithSubscriptionID(gomock.Any()).Return(mockFileClient).AnyTimes() + mockFileClient.EXPECT().CreateFileShare(context.TODO(), gomock.Any(), gomock.Any(), expectedShareOptions, gomock.Any()).Return(storage.FileShare{FileShareProperties: &storage.FileShareProperties{ShareQuota: nil}}, nil).AnyTimes() + mockFileClient.EXPECT().GetFileShare(context.TODO(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(storage.FileShare{FileShareProperties: &storage.FileShareProperties{ShareQuota: &fakeShareQuota}}, nil).AnyTimes() + mockStorageAccountsClient.EXPECT().ListKeys(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(keys, nil).AnyTimes() + mockStorageAccountsClient.EXPECT().ListByResourceGroup(gomock.Any(), gomock.Any(), gomock.Any()).Return(accounts, nil).AnyTimes() + mockStorageAccountsClient.EXPECT().Create(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + mockStorageAccountsClient.EXPECT().GetProperties(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(accounts[0], nil).AnyTimes() + + _, err := d.CreateVolume(ctx, req) + + if !reflect.DeepEqual(err, nil) { + t.Errorf("Unexpected error: %v", err) + } + }, + }, } for _, tc := range testCases {