Skip to content

Commit

Permalink
Merge pull request #1479 from k8s-infra-cherrypick-robot/cherry-pick-…
Browse files Browse the repository at this point in the history 
…1391-to-release-1.29

[release-1.29] fix: check storage account type if parameter is missing
  • Loading branch information
@andyzhangx
andyzhangx authored Oct 11, 2023
2 parents 692d779 + 7e807f9 commit e4b9267
Show file tree
Hide file tree
Showing 5 changed files with 202 additions and 10 deletions.
4 changes: 2 additions & 2 deletions deploy/example/azure.json
View file
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
View file
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
View file
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
View file
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
View file
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

0 comments on commit e4b9267

Please sign in to comment.