Skip to content

Commit bb91232

Browse files
andyzhangxandyzhangx
authored
Merge pull request #2401 from kubernetes-sigs/refine-GetStorageAccesskey
chore: refine GetStorageAccesskey and GetFileShareClientForSub function
2 parents f55c518 + a7a9ef9 commit bb91232

6 files changed

+223
-56
lines changed

‎pkg/azurefile/azurefile.go

+31-26
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ import (
5454
csicommon "sigs.k8s.io/azurefile-csi-driver/pkg/csi-common"
5555
"sigs.k8s.io/azurefile-csi-driver/pkg/mounter"
5656
fileutil "sigs.k8s.io/azurefile-csi-driver/pkg/util"
57-
"sigs.k8s.io/cloud-provider-azure/pkg/azclient/accountclient"
5857
"sigs.k8s.io/cloud-provider-azure/pkg/azclient/fileshareclient"
5958
azcache "sigs.k8s.io/cloud-provider-azure/pkg/cache"
6059
"sigs.k8s.io/cloud-provider-azure/pkg/provider/storage"
@@ -478,13 +477,11 @@ func (d *Driver) getFileShareQuota(ctx context.Context, accountOptions *storage.
478477
if err != nil {
479478
return -1, err
480479
}
481-
fileClient, err = newAzureFileClient(accountName, accountKey, d.getStorageEndPointSuffix())
482-
if err != nil {
480+
if fileClient, err = newAzureFileClient(accountName, accountKey, d.getStorageEndPointSuffix()); err != nil {
483481
return -1, err
484482
}
485483
} else {
486-
fileClient, err = newAzureFileMgmtClient(d.cloud.ComputeClientFactory, accountOptions)
487-
if err != nil {
484+
if fileClient, err = newAzureFileMgmtClient(d.cloud, accountOptions); err != nil {
488485
return -1, err
489486
}
490487
}
@@ -849,14 +846,9 @@ func (d *Driver) GetAccountInfo(ctx context.Context, volumeID string, secrets, r
849846
if err != nil {
850847
// 3. if failed to get account key from kubernetes secret, use cluster identity to get account key
851848
klog.Warningf("GetStorageAccountFromSecret(%s, %s) failed with error: %v", secretName, secretNamespace, err)
852-
var accountClient accountclient.Interface
853-
accountClient, err = d.cloud.ComputeClientFactory.GetAccountClientForSub(subsID)
854-
if err != nil {
855-
return rgName, accountName, accountKey, fileShareName, diskName, subsID, err
856-
}
857-
if !getAccountKeyFromSecret && accountClient != nil && accountName != "" {
849+
if !getAccountKeyFromSecret && accountName != "" {
858850
klog.V(2).Infof("use cluster identity to get account key from (%s, %s, %s)", subsID, rgName, accountName)
859-
accountKey, err = d.cloud.GetStorageAccesskey(ctx, accountClient, accountName, rgName, getLatestAccountKey)
851+
accountKey, err = d.GetStorageAccesskeyWithSubsID(ctx, subsID, accountName, rgName, getLatestAccountKey)
860852
if err != nil {
861853
klog.Errorf("GetStorageAccesskey(%s, %s, %s) failed with error: %v", subsID, rgName, accountName, err)
862854
}
@@ -956,7 +948,7 @@ func (d *Driver) CreateFileShare(ctx context.Context, accountOptions *storage.Ac
956948
return true, err
957949
}
958950
} else {
959-
if fileClient, err = newAzureFileMgmtClient(d.cloud.ComputeClientFactory, accountOptions); err != nil {
951+
if fileClient, err = newAzureFileMgmtClient(d.cloud, accountOptions); err != nil {
960952
return true, err
961953
}
962954
}
@@ -987,10 +979,9 @@ func (d *Driver) DeleteFileShare(ctx context.Context, subsID, resourceGroup, acc
987979
}
988980
err = fileClient.DeleteFileShare(ctx, shareName)
989981
} else {
990-
var fileClient fileshareclient.Interface
991-
fileClient, err = d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
992-
if err != nil {
993-
return true, err
982+
fileClient, errGetClient := d.getFileShareClientForSub(subsID)
983+
if errGetClient != nil {
984+
return true, errGetClient
994985
}
995986
err = fileClient.Delete(ctx, resourceGroup, accountName, shareName, nil)
996987
}
@@ -1033,7 +1024,7 @@ func (d *Driver) ResizeFileShare(ctx context.Context, subsID, resourceGroup, acc
10331024
}
10341025
err = fileClient.ResizeFileShare(ctx, shareName, sizeGiB)
10351026
} else {
1036-
fileClient, err := d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
1027+
fileClient, err := d.getFileShareClientForSub(subsID)
10371028
if err != nil {
10381029
return true, err
10391030
}
@@ -1098,7 +1089,7 @@ func (d *Driver) copyFileShare(ctx context.Context, req *csi.CreateVolumeRequest
10981089

10991090
// GetTotalAccountQuota returns the total quota in GB of all file shares in the storage account and the number of file shares
11001091
func (d *Driver) GetTotalAccountQuota(ctx context.Context, subsID, resourceGroup, accountName string) (int32, int32, error) {
1101-
fileClient, err := d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
1092+
fileClient, err := d.getFileShareClientForSub(subsID)
11021093
if err != nil {
11031094
return -1, -1, err
11041095
}
@@ -1117,7 +1108,7 @@ func (d *Driver) GetTotalAccountQuota(ctx context.Context, subsID, resourceGroup
11171108

11181109
// RemoveStorageAccountTag remove tag from storage account
11191110
func (d *Driver) RemoveStorageAccountTag(ctx context.Context, subsID, resourceGroup, account, key string) error {
1120-
if d.cloud == nil || d.cloud.ComputeClientFactory.GetAccountClient() == nil {
1111+
if d.cloud == nil {
11211112
return fmt.Errorf("cloud or StorageAccountClient is nil")
11221113
}
11231114
// search in cache first
@@ -1165,12 +1156,7 @@ func (d *Driver) GetStorageAccesskey(ctx context.Context, accountOptions *storag
11651156
_, accountKey, err := d.GetStorageAccountFromSecret(ctx, secretName, secretNamespace)
11661157
if err != nil {
11671158
klog.V(2).Infof("could not get account(%s) key from secret(%s), error: %v, use cluster identity to get account key instead", accountOptions.Name, secretName, err)
1168-
var accountClient accountclient.Interface
1169-
accountClient, err = d.cloud.ComputeClientFactory.GetAccountClientForSub(accountOptions.SubscriptionID)
1170-
if err != nil {
1171-
return "", err
1172-
}
1173-
accountKey, err = d.cloud.GetStorageAccesskey(ctx, accountClient, accountName, accountOptions.ResourceGroup, accountOptions.GetLatestAccountKey)
1159+
accountKey, err = d.GetStorageAccesskeyWithSubsID(ctx, accountOptions.SubscriptionID, accountOptions.Name, accountOptions.ResourceGroup, accountOptions.GetLatestAccountKey)
11741160
}
11751161

11761162
if err == nil && accountKey != "" {
@@ -1179,6 +1165,18 @@ func (d *Driver) GetStorageAccesskey(ctx context.Context, accountOptions *storag
11791165
return accountKey, err
11801166
}
11811167

1168+
// GetStorageAccesskeyWithSubsID get Azure storage account key from storage account directly
1169+
func (d *Driver) GetStorageAccesskeyWithSubsID(ctx context.Context, subsID, account, resourceGroup string, getLatestAccountKey bool) (string, error) {
1170+
if d.cloud == nil || d.cloud.ComputeClientFactory == nil {
1171+
return "", fmt.Errorf("could not get account key: cloud or ComputeClientFactory is nil")
1172+
}
1173+
accountClient, err := d.cloud.ComputeClientFactory.GetAccountClientForSub(subsID)
1174+
if err != nil {
1175+
return "", err
1176+
}
1177+
return d.cloud.GetStorageAccesskey(ctx, accountClient, account, resourceGroup, getLatestAccountKey)
1178+
}
1179+
11821180
// GetStorageAccountFromSecret get storage account key from k8s secret
11831181
// return <accountName, accountKey, error>
11841182
func (d *Driver) GetStorageAccountFromSecret(ctx context.Context, secretName, secretNamespace string) (string, string, error) {
@@ -1274,3 +1272,10 @@ func (d *Driver) getStorageEndPointSuffix() string {
12741272
}
12751273
return d.cloud.Environment.StorageEndpointSuffix
12761274
}
1275+
1276+
func (d *Driver) getFileShareClientForSub(subscriptionID string) (fileshareclient.Interface, error) {
1277+
if d.cloud == nil || d.cloud.ComputeClientFactory == nil {
1278+
return nil, fmt.Errorf("cloud or ComputeClientFactory is nil")
1279+
}
1280+
return d.cloud.ComputeClientFactory.GetFileShareClientForSub(subscriptionID)
1281+
}

‎pkg/azurefile/azurefile_dataplane_client_test.go

-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
)
2626

2727
func TestCreateFileShare(t *testing.T) {
28-
2928
testCases := []struct {
3029
name string
3130
testFunc func(t *testing.T)

‎pkg/azurefile/azurefile_mgmt_client.go

+6-7
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,23 @@ import (
2323
"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
2424
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/storage/armstorage"
2525
"k8s.io/klog/v2"
26-
"sigs.k8s.io/cloud-provider-azure/pkg/azclient"
2726
"sigs.k8s.io/cloud-provider-azure/pkg/azclient/fileshareclient"
28-
azure "sigs.k8s.io/cloud-provider-azure/pkg/provider/storage"
27+
"sigs.k8s.io/cloud-provider-azure/pkg/provider/storage"
2928
)
3029

3130
type azureFileMgmtClient struct {
3231
fileShareClient fileshareclient.Interface
33-
accountOptions *azure.AccountOptions
32+
accountOptions *storage.AccountOptions
3433
}
3534

36-
func newAzureFileMgmtClient(clientFactory azclient.ClientFactory, accountOptions *azure.AccountOptions) (azureFileClient, error) {
37-
if clientFactory == nil {
38-
return nil, fmt.Errorf("clientFactory is nil")
35+
func newAzureFileMgmtClient(cloud *storage.AccountRepo, accountOptions *storage.AccountOptions) (azureFileClient, error) {
36+
if cloud == nil || cloud.ComputeClientFactory == nil {
37+
return nil, fmt.Errorf("cloud provider is not initialized")
3938
}
4039
if accountOptions == nil {
4140
return nil, fmt.Errorf("accountOptions is nil")
4241
}
43-
fileShareClient, err := clientFactory.GetFileShareClientForSub(accountOptions.SubscriptionID)
42+
fileShareClient, err := cloud.ComputeClientFactory.GetFileShareClientForSub(accountOptions.SubscriptionID)
4443
if err != nil {
4544
return nil, fmt.Errorf("failed to get file share client for subscription %s: %w", accountOptions.SubscriptionID, err)
4645
}

‎pkg/azurefile/azurefile_mgmt_client_test.go

+16-14
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"reflect"
2222
"testing"
2323

24+
"github.com/stretchr/testify/assert"
2425
"go.uber.org/mock/gomock"
2526
"sigs.k8s.io/cloud-provider-azure/pkg/azclient/fileshareclient/mock_fileshareclient"
2627
"sigs.k8s.io/cloud-provider-azure/pkg/azclient/mock_azclient"
@@ -30,7 +31,7 @@ import (
3031
func TestNewAzureMgmtFileClientClientFactoryNil(t *testing.T) {
3132
_, actualErr := newAzureFileMgmtClient(nil, nil)
3233
if actualErr != nil {
33-
expectedErr := fmt.Errorf("clientFactory is nil")
34+
expectedErr := fmt.Errorf("cloud provider is not initialized")
3435
if !reflect.DeepEqual(actualErr, expectedErr) {
3536
t.Errorf("actualErr: (%v), expectedErr: (%v)", actualErr, expectedErr)
3637
}
@@ -39,7 +40,10 @@ func TestNewAzureMgmtFileClientClientFactoryNil(t *testing.T) {
3940
func TestNewAzureMgmtFileClientAccountOptionNil(t *testing.T) {
4041
cntl := gomock.NewController(t)
4142
defer cntl.Finish()
42-
_, actualErr := newAzureFileMgmtClient(mock_azclient.NewMockClientFactory(cntl), nil)
43+
cloud := &storage.AccountRepo{
44+
ComputeClientFactory: mock_azclient.NewMockClientFactory(cntl),
45+
}
46+
_, actualErr := newAzureFileMgmtClient(cloud, nil)
4347
if actualErr != nil {
4448
expectedErr := fmt.Errorf("accountOptions is nil")
4549
if !reflect.DeepEqual(actualErr, expectedErr) {
@@ -49,19 +53,17 @@ func TestNewAzureMgmtFileClientAccountOptionNil(t *testing.T) {
4953
}
5054

5155
func TestNewAzureMgmtFileClient(t *testing.T) {
52-
cntl := gomock.NewController(t)
53-
defer cntl.Finish()
54-
clientFactory := mock_azclient.NewMockClientFactory(cntl)
55-
fileClient := mock_fileshareclient.NewMockInterface(cntl)
56-
clientFactory.EXPECT().GetFileShareClientForSub("testsub").Return(fileClient, nil)
57-
_, actualErr := newAzureFileMgmtClient(clientFactory, &storage.AccountOptions{
56+
ctrl := gomock.NewController(t)
57+
defer ctrl.Finish()
58+
computeClientFactory := mock_azclient.NewMockClientFactory(ctrl)
59+
mockFileClient := mock_fileshareclient.NewMockInterface(ctrl)
60+
computeClientFactory.EXPECT().GetFileShareClientForSub("testsub").Return(mockFileClient, nil).AnyTimes()
61+
cloud := &storage.AccountRepo{
62+
ComputeClientFactory: computeClientFactory,
63+
}
64+
_, actualErr := newAzureFileMgmtClient(cloud, &storage.AccountOptions{
5865
SubscriptionID: "testsub",
5966
ResourceGroup: "testrg",
6067
})
61-
if actualErr != nil {
62-
expectedErr := fmt.Errorf("accountOptions is nil")
63-
if !reflect.DeepEqual(actualErr, expectedErr) {
64-
t.Errorf("actualErr: (%v), expectedErr: (%v)", actualErr, expectedErr)
65-
}
66-
}
68+
assert.Equal(t, actualErr, nil, "newAzureFileMgmtClient should return success")
6769
}

0 commit comments

Comments
 (0)