Merge pull request #1391 from RomanBednar/smb-errmsg

fix: check storage account type if parameter is missing
kubernetes-sigs · Oct 7, 2023 · b20bd12 · b20bd12
2 parents 7951bb7 + 086a8e4
commit b20bd12
Show file tree

Hide file tree

Showing 5 changed files with 202 additions and 10 deletions.
diff --git a/deploy/example/azure.json b/deploy/example/azure.json
@@ -6,8 +6,8 @@
     "location": "eastus2",                         // mandatory
     "aadClientId": "xxxx-xxxx-xxxx-xxxx-xxxx",     // mandatory if using service principal
     "aadClientSecret": "xxxx-xxxx-xxxx-xxxx-xxxx", // mandatory if using service principal
-    "useManagedIdentityExtension": false,          // set true if using managed idenitty
-    "userAssignedIdentityID": "",                  // mandatory if using managed idenitty
+    "useManagedIdentityExtension": false,          // set true if using managed identity
+    "userAssignedIdentityID": "",                  // mandatory if using managed identity
     "useInstanceMetadata": true,                   // optional
     "vmType": "standard",                          // optional
     "subnetName": "k8s-subnet",                    // optional

diff --git a/docs/csi-debug.md b/docs/csi-debug.md
@@ -173,7 +173,7 @@ Enable [large file shares](https://docs.microsoft.com/azure/storage/files/storag
 ##### Premium Files
 Azure premium files follows provisioned model where IOPS and throughput are associated to the quota. See this article that explains the co-relation between share size and IOPS and throughput - [link](https://docs.microsoft.com/azure/storage/files/understanding-billing#provisioned-model). Increase the share quota by following this guide - [link](https://github.com/kubernetes-sigs/azurefile-csi-driver/tree/master/deploy/example/resize).
 
-##### For more, refer to this doc for perforance troubleshooting tips - [Link to performance troubleshooting tips](https://docs.microsoft.com/en-us/azure/storage/files/storage-troubleshooting-files-performance)
+##### For more, refer to this doc for performance troubleshooting tips - [Link to performance troubleshooting tips](https://docs.microsoft.com/en-us/azure/storage/files/storage-troubleshooting-files-performance)
 
 ##### [Storage considerations for Azure Kubernetes Service (AKS)](https://learn.microsoft.com/en-us/azure/cloud-adoption-framework/scenarios/app-platform/aks/storage)
 ##### [Compare access to Azure Files, Blob Storage, and Azure NetApp Files with NFS](https://learn.microsoft.com/en-us/azure/storage/common/nfs-comparison#comparison)
diff --git a/pkg/azurefile/controllerserver.go b/pkg/azurefile/controllerserver.go
@@ -336,7 +336,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		shareProtocol = storage.EnabledProtocolsNFS
 		// NFS protocol does not need account key
 		storeAccountKey = false
-		// reset protocol field (compatble with "fsType: nfs")
+		// reset protocol field (compatible with "fsType: nfs")
 		setKeyValueInMap(parameters, protocolField, protocol)
 
 		if !pointer.BoolDeref(createPrivateEndpoint, false) {
@@ -359,7 +359,22 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		}
 	}
 
+	if resourceGroup == "" {
+		resourceGroup = d.cloud.ResourceGroup
+	}
+
 	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) {
@@ -387,10 +402,6 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		validFileShareName = getValidFileShareName(name)
 	}
 
-	if resourceGroup == "" {
-		resourceGroup = d.cloud.ResourceGroup
-	}
-
 	tags, err := ConvertTagsToMap(customTags)
 	if err != nil {
 		return nil, status.Errorf(codes.InvalidArgument, err.Error())

diff --git a/pkg/azurefile/controllerserver_test.go b/pkg/azurefile/controllerserver_test.go
@@ -23,13 +23,13 @@ import (
 	"net/http"
 	"net/url"
 	"reflect"
+	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/fileclient"
 	"strings"
 	"sync"
 	"testing"
 	"time"
 
 	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2022-07-01/network"
-	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/fileclient"
 	"sigs.k8s.io/cloud-provider-azure/pkg/azureclients/subnetclient/mocksubnetclient"
 	azcache "sigs.k8s.io/cloud-provider-azure/pkg/cache"
 
@@ -1267,6 +1267,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 {

diff --git a/test/e2e/driver/azurefile_driver.go b/test/e2e/driver/azurefile_driver.go
@@ -39,7 +39,7 @@ type AzureFileDriver struct {
 	driverName string
 }
 
-// InitAzureFileDriver returns AzureFileDriver that implemnts DynamicPVTestDriver interface
+// InitAzureFileDriver returns AzureFileDriver that implements DynamicPVTestDriver interface
 func InitAzureFileDriver() PVTestDriver {
 	driverName := os.Getenv(AzureDriverNameVar)
 	if driverName == "" {