Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: check storage account type if parameter is missing #1391

Merged
merged 3 commits into from
Oct 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions deploy/example/azure.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion docs/csi-debug.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
21 changes: 16 additions & 5 deletions pkg/azurefile/controllerserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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())
Expand Down
183 changes: 182 additions & 1 deletion pkg/azurefile/controllerserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/driver/azurefile_driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "" {
Expand Down
Loading