Skip to content

Commit a7a9ef9

Browse files
andyzhangxandyzhangx
committed
chore: refine GetFileShareClientForSub function
fix
1 parent e0a914c commit a7a9ef9

6 files changed

+82
-40
lines changed

‎pkg/azurefile/azurefile.go

+16-12
Original file line numberDiff line numberDiff line change
@@ -477,13 +477,11 @@ func (d *Driver) getFileShareQuota(ctx context.Context, accountOptions *storage.
477477
if err != nil {
478478
return -1, err
479479
}
480-
fileClient, err = newAzureFileClient(accountName, accountKey, d.getStorageEndPointSuffix())
481-
if err != nil {
480+
if fileClient, err = newAzureFileClient(accountName, accountKey, d.getStorageEndPointSuffix()); err != nil {
482481
return -1, err
483482
}
484483
} else {
485-
fileClient, err = newAzureFileMgmtClient(d.cloud.ComputeClientFactory, accountOptions)
486-
if err != nil {
484+
if fileClient, err = newAzureFileMgmtClient(d.cloud, accountOptions); err != nil {
487485
return -1, err
488486
}
489487
}
@@ -950,7 +948,7 @@ func (d *Driver) CreateFileShare(ctx context.Context, accountOptions *storage.Ac
950948
return true, err
951949
}
952950
} else {
953-
if fileClient, err = newAzureFileMgmtClient(d.cloud.ComputeClientFactory, accountOptions); err != nil {
951+
if fileClient, err = newAzureFileMgmtClient(d.cloud, accountOptions); err != nil {
954952
return true, err
955953
}
956954
}
@@ -981,10 +979,9 @@ func (d *Driver) DeleteFileShare(ctx context.Context, subsID, resourceGroup, acc
981979
}
982980
err = fileClient.DeleteFileShare(ctx, shareName)
983981
} else {
984-
var fileClient fileshareclient.Interface
985-
fileClient, err = d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
986-
if err != nil {
987-
return true, err
982+
fileClient, errGetClient := d.getFileShareClientForSub(subsID)
983+
if errGetClient != nil {
984+
return true, errGetClient
988985
}
989986
err = fileClient.Delete(ctx, resourceGroup, accountName, shareName, nil)
990987
}
@@ -1027,7 +1024,7 @@ func (d *Driver) ResizeFileShare(ctx context.Context, subsID, resourceGroup, acc
10271024
}
10281025
err = fileClient.ResizeFileShare(ctx, shareName, sizeGiB)
10291026
} else {
1030-
fileClient, err := d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
1027+
fileClient, err := d.getFileShareClientForSub(subsID)
10311028
if err != nil {
10321029
return true, err
10331030
}
@@ -1092,7 +1089,7 @@ func (d *Driver) copyFileShare(ctx context.Context, req *csi.CreateVolumeRequest
10921089

10931090
// GetTotalAccountQuota returns the total quota in GB of all file shares in the storage account and the number of file shares
10941091
func (d *Driver) GetTotalAccountQuota(ctx context.Context, subsID, resourceGroup, accountName string) (int32, int32, error) {
1095-
fileClient, err := d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
1092+
fileClient, err := d.getFileShareClientForSub(subsID)
10961093
if err != nil {
10971094
return -1, -1, err
10981095
}
@@ -1111,7 +1108,7 @@ func (d *Driver) GetTotalAccountQuota(ctx context.Context, subsID, resourceGroup
11111108

11121109
// RemoveStorageAccountTag remove tag from storage account
11131110
func (d *Driver) RemoveStorageAccountTag(ctx context.Context, subsID, resourceGroup, account, key string) error {
1114-
if d.cloud == nil || d.cloud.ComputeClientFactory.GetAccountClient() == nil {
1111+
if d.cloud == nil {
11151112
return fmt.Errorf("cloud or StorageAccountClient is nil")
11161113
}
11171114
// search in cache first
@@ -1275,3 +1272,10 @@ func (d *Driver) getStorageEndPointSuffix() string {
12751272
}
12761273
return d.cloud.Environment.StorageEndpointSuffix
12771274
}
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
}

‎pkg/azurefile/azurefile_test.go

+37-2
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,8 @@ import (
3737
"github.com/container-storage-interface/spec/lib/go/csi"
3838
"github.com/stretchr/testify/assert"
3939
"go.uber.org/mock/gomock"
40-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4140
v1api "k8s.io/api/core/v1"
41+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4242
"k8s.io/client-go/kubernetes/fake"
4343

4444
"sigs.k8s.io/cloud-provider-azure/pkg/azclient"
@@ -74,6 +74,7 @@ func NewFakeDriver() *Driver {
7474
csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME,
7575
csi.ControllerServiceCapability_RPC_EXPAND_VOLUME,
7676
})
77+
driver.cloud = &storage.AccountRepo{}
7778
return driver
7879
}
7980

@@ -1490,7 +1491,7 @@ func TestGetStorageAccesskey(t *testing.T) {
14901491
expectedError error
14911492
}{
14921493
{
1493-
name: "error is not nil", // test case shoudl run first to avoid cache hit
1494+
name: "error is not nil", // test case should run first to avoid cache hit
14941495
secrets: make(map[string]string),
14951496
secretName: "foobar",
14961497
expectedError: errors.New(""),
@@ -1597,3 +1598,37 @@ func TestGetStorageAccesskeyWithSubsID(t *testing.T) {
15971598
assert.Equal(t, tc.expectedError, err)
15981599
}
15991600
}
1601+
1602+
func TestGetFileShareClientForSub(t *testing.T) {
1603+
testCases := []struct {
1604+
name string
1605+
expectedError error
1606+
}{
1607+
{
1608+
name: "Get file share client error with cloud is nil",
1609+
expectedError: fmt.Errorf("cloud or ComputeClientFactory is nil"),
1610+
},
1611+
{
1612+
name: "Get file share client error with ComputeClientFactory is nil",
1613+
expectedError: fmt.Errorf("cloud or ComputeClientFactory is nil"),
1614+
},
1615+
{
1616+
name: "Get file share client successfully",
1617+
expectedError: nil,
1618+
},
1619+
}
1620+
1621+
for _, tc := range testCases {
1622+
d := NewFakeDriver()
1623+
d.cloud = &storage.AccountRepo{}
1624+
if !strings.Contains(tc.name, "is nil") {
1625+
ctrl := gomock.NewController(t)
1626+
defer ctrl.Finish()
1627+
mockFileClient := mock_fileshareclient.NewMockInterface(ctrl)
1628+
d.cloud.ComputeClientFactory = mock_azclient.NewMockClientFactory(ctrl)
1629+
d.cloud.ComputeClientFactory.(*mock_azclient.MockClientFactory).EXPECT().GetFileShareClientForSub(gomock.Any()).Return(mockFileClient, tc.expectedError).AnyTimes()
1630+
}
1631+
_, err := d.getFileShareClientForSub("test-subID")
1632+
assert.Equal(t, tc.expectedError, err)
1633+
}
1634+
}

‎pkg/azurefile/controllerserver.go

+7-4
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,9 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
382382
fileShareSize := int(requestGiB)
383383

384384
if account != "" && resourceGroup != "" && sku == "" && fileShareSize < minimumPremiumShareSize {
385+
if d.cloud == nil || d.cloud.ComputeClientFactory == nil {
386+
return nil, status.Errorf(codes.Internal, "cloud provider is not initialized")
387+
}
385388
client, err := d.cloud.ComputeClientFactory.GetAccountClientForSub(subsID)
386389
if err != nil {
387390
return nil, status.Errorf(codes.Internal, "failed to get account client for subscription %s: %v", subsID, err)
@@ -946,7 +949,7 @@ func (d *Driver) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequ
946949
itemSnapshotTime = *properties.Date
947950
itemSnapshotQuota = *properties.Quota
948951
} else {
949-
fileshareClient, err := d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
952+
fileshareClient, err := d.getFileShareClientForSub(subsID)
950953
if err != nil {
951954
return nil, status.Errorf(codes.Internal, "failed to get snapshot client for subID(%s): %v", subsID, err)
952955
}
@@ -981,7 +984,7 @@ func (d *Driver) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshotRequ
981984
itemSnapshotQuota = cache.(int32)
982985
} else {
983986
klog.V(2).Infof("get file share(%s) account(%s) quota from cloud", fileShareName, accountName)
984-
fileshareClient, err := d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
987+
fileshareClient, err := d.getFileShareClientForSub(subsID)
985988
if err != nil {
986989
return nil, status.Errorf(codes.Internal, "failed to get file share client for subID(%s): %v", subsID, err)
987990
}
@@ -1049,7 +1052,7 @@ func (d *Driver) DeleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequ
10491052
}
10501053
_, deleteErr = client.Delete(ctx, nil)
10511054
} else {
1052-
fileshareClient, err := d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
1055+
fileshareClient, err := d.getFileShareClientForSub(subsID)
10531056
if err != nil {
10541057
return nil, status.Errorf(codes.Internal, "failed to get snapshot client for subID(%s): %v", subsID, err)
10551058
}
@@ -1341,7 +1344,7 @@ func (d *Driver) snapshotExists(ctx context.Context, sourceVolumeID, snapshotNam
13411344

13421345
// List share snapshots.
13431346
filter := fmt.Sprintf("startswith(name, %s)", fileShareName)
1344-
fileshareClient, err := d.cloud.ComputeClientFactory.GetFileShareClientForSub(subsID)
1347+
fileshareClient, err := d.getFileShareClientForSub(subsID)
13451348
if err != nil {
13461349
return false, "", time.Time{}, 0, status.Errorf(codes.Internal, "failed to get snapshot client for subID(%s): %v", subsID, err)
13471350
}

0 commit comments

Comments
 (0)