Skip to content

Commit e0a914c

Browse files
andyzhangxandyzhangx
committed
chore: refine GetStorageAccesskey function
1 parent 8df117b commit e0a914c

File tree

2 files changed

+143
-18
lines changed

2 files changed

+143
-18
lines changed

‎pkg/azurefile/azurefile.go‎

Lines changed: 15 additions & 14 deletions
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"
@@ -849,14 +848,9 @@ func (d *Driver) GetAccountInfo(ctx context.Context, volumeID string, secrets, r
849848
if err != nil {
850849
// 3. if failed to get account key from kubernetes secret, use cluster identity to get account key
851850
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 != "" {
851+
if !getAccountKeyFromSecret && accountName != "" {
858852
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)
853+
accountKey, err = d.GetStorageAccesskeyWithSubsID(ctx, subsID, accountName, rgName, getLatestAccountKey)
860854
if err != nil {
861855
klog.Errorf("GetStorageAccesskey(%s, %s, %s) failed with error: %v", subsID, rgName, accountName, err)
862856
}
@@ -1165,12 +1159,7 @@ func (d *Driver) GetStorageAccesskey(ctx context.Context, accountOptions *storag
11651159
_, accountKey, err := d.GetStorageAccountFromSecret(ctx, secretName, secretNamespace)
11661160
if err != nil {
11671161
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)
1162+
accountKey, err = d.GetStorageAccesskeyWithSubsID(ctx, accountOptions.SubscriptionID, accountOptions.Name, accountOptions.ResourceGroup, accountOptions.GetLatestAccountKey)
11741163
}
11751164

11761165
if err == nil && accountKey != "" {
@@ -1179,6 +1168,18 @@ func (d *Driver) GetStorageAccesskey(ctx context.Context, accountOptions *storag
11791168
return accountKey, err
11801169
}
11811170

1171+
// GetStorageAccesskeyWithSubsID get Azure storage account key from storage account directly
1172+
func (d *Driver) GetStorageAccesskeyWithSubsID(ctx context.Context, subsID, account, resourceGroup string, getLatestAccountKey bool) (string, error) {
1173+
if d.cloud == nil || d.cloud.ComputeClientFactory == nil {
1174+
return "", fmt.Errorf("could not get account key: cloud or ComputeClientFactory is nil")
1175+
}
1176+
accountClient, err := d.cloud.ComputeClientFactory.GetAccountClientForSub(subsID)
1177+
if err != nil {
1178+
return "", err
1179+
}
1180+
return d.cloud.GetStorageAccesskey(ctx, accountClient, account, resourceGroup, getLatestAccountKey)
1181+
}
1182+
11821183
// GetStorageAccountFromSecret get storage account key from k8s secret
11831184
// return <accountName, accountKey, error>
11841185
func (d *Driver) GetStorageAccountFromSecret(ctx context.Context, secretName, secretNamespace string) (string, string, error) {

‎pkg/azurefile/azurefile_test.go‎

Lines changed: 128 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,15 @@ package azurefile
1919
import (
2020
"context"
2121
"encoding/base64"
22+
"errors"
2223
"fmt"
2324
"net/http"
2425
"net/url"
2526
"os"
2627
"path/filepath"
2728
"reflect"
2829
"sort"
30+
"strings"
2931
"testing"
3032
"time"
3133

@@ -35,6 +37,8 @@ import (
3537
"github.com/container-storage-interface/spec/lib/go/csi"
3638
"github.com/stretchr/testify/assert"
3739
"go.uber.org/mock/gomock"
40+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
41+
v1api "k8s.io/api/core/v1"
3842
"k8s.io/client-go/kubernetes/fake"
3943

4044
"sigs.k8s.io/cloud-provider-azure/pkg/azclient"
@@ -54,7 +58,6 @@ var (
5458
)
5559

5660
func NewFakeDriver() *Driver {
57-
var err error
5861
driverOptions := DriverOptions{
5962
NodeID: fakeNodeID,
6063
DriverName: DefaultDriverName,
@@ -66,9 +69,6 @@ func NewFakeDriver() *Driver {
6669
driver := NewDriver(&driverOptions)
6770
driver.Name = fakeDriverName
6871
driver.Version = vendorVersion
69-
if err != nil {
70-
panic(err)
71-
}
7272
driver.AddControllerServiceCapabilities(
7373
[]csi.ControllerServiceCapability_RPC_Type{
7474
csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME,
@@ -1473,3 +1473,127 @@ func TestGetStorageEndPointSuffix(t *testing.T) {
14731473
assert.Equal(t, test.expectedSuffix, suffix, test.name)
14741474
}
14751475
}
1476+
1477+
func TestGetStorageAccesskey(t *testing.T) {
1478+
options := &storage.AccountOptions{
1479+
Name: "test-sa",
1480+
SubscriptionID: "test-subID",
1481+
ResourceGroup: "test-rg",
1482+
}
1483+
fakeAccName := options.Name
1484+
fakeAccKey := "test-key"
1485+
secretNamespace := "test-ns"
1486+
testCases := []struct {
1487+
name string
1488+
secrets map[string]string
1489+
secretName string
1490+
expectedError error
1491+
}{
1492+
{
1493+
name: "error is not nil", // test case shoudl run first to avoid cache hit
1494+
secrets: make(map[string]string),
1495+
secretName: "foobar",
1496+
expectedError: errors.New(""),
1497+
},
1498+
{
1499+
name: "Secrets is larger than 0",
1500+
secrets: map[string]string{
1501+
"accountName": fakeAccName,
1502+
"accountNameField": fakeAccName,
1503+
"defaultSecretAccountName": fakeAccName,
1504+
"accountKey": fakeAccKey,
1505+
"accountKeyField": fakeAccKey,
1506+
"defaultSecretAccountKey": fakeAccKey,
1507+
},
1508+
expectedError: nil,
1509+
},
1510+
{
1511+
name: "secretName is Empty",
1512+
secrets: make(map[string]string),
1513+
secretName: "",
1514+
expectedError: nil,
1515+
},
1516+
{
1517+
name: "successful input/error is nil",
1518+
secrets: make(map[string]string),
1519+
secretName: fmt.Sprintf(secretNameTemplate, options.Name),
1520+
expectedError: nil,
1521+
},
1522+
}
1523+
d := NewFakeDriver()
1524+
d.cloud = &storage.AccountRepo{}
1525+
d.kubeClient = fake.NewSimpleClientset()
1526+
ctrl := gomock.NewController(t)
1527+
defer ctrl.Finish()
1528+
mockStorageAccountsClient := mock_accountclient.NewMockInterface(ctrl)
1529+
d.cloud.ComputeClientFactory = mock_azclient.NewMockClientFactory(gomock.NewController(t))
1530+
d.cloud.ComputeClientFactory.(*mock_azclient.MockClientFactory).EXPECT().GetAccountClient().Return(mockStorageAccountsClient).AnyTimes()
1531+
accountListKeysResult := []*armstorage.AccountKey{}
1532+
mockStorageAccountsClient.EXPECT().ListKeys(gomock.Any(), gomock.Any(), gomock.Any()).Return(accountListKeysResult, nil).AnyTimes()
1533+
d.cloud.ComputeClientFactory.(*mock_azclient.MockClientFactory).EXPECT().GetAccountClientForSub(gomock.Any()).Return(mockStorageAccountsClient, nil).AnyTimes()
1534+
secret := &v1api.Secret{
1535+
ObjectMeta: metav1.ObjectMeta{
1536+
Namespace: secretNamespace,
1537+
Name: fmt.Sprintf(secretNameTemplate, options.Name),
1538+
},
1539+
Data: map[string][]byte{
1540+
defaultSecretAccountName: []byte(fakeAccName),
1541+
defaultSecretAccountKey: []byte(fakeAccKey),
1542+
},
1543+
Type: "Opaque",
1544+
}
1545+
secret.Namespace = secretNamespace
1546+
_, secretCreateErr := d.kubeClient.CoreV1().Secrets(secretNamespace).Create(context.TODO(), secret, metav1.CreateOptions{})
1547+
if secretCreateErr != nil {
1548+
t.Error("failed to create secret")
1549+
}
1550+
for _, tc := range testCases {
1551+
t.Run(tc.name, func(t *testing.T) {
1552+
accKey, err := d.GetStorageAccesskey(context.TODO(), options, tc.secrets, tc.secretName, secretNamespace)
1553+
if tc.expectedError != nil {
1554+
assert.Error(t, err, "there should be an error")
1555+
} else {
1556+
assert.Equal(t, nil, err, "error should be nil")
1557+
assert.Equal(t, fakeAccKey, accKey, "account keys must match")
1558+
}
1559+
})
1560+
}
1561+
}
1562+
1563+
func TestGetStorageAccesskeyWithSubsID(t *testing.T) {
1564+
testCases := []struct {
1565+
name string
1566+
expectedError error
1567+
}{
1568+
{
1569+
name: "Get storage access key error with cloud is nil",
1570+
expectedError: fmt.Errorf("could not get account key: cloud or ComputeClientFactory is nil"),
1571+
},
1572+
{
1573+
name: "Get storage access key error with ComputeClientFactory is nil",
1574+
expectedError: fmt.Errorf("could not get account key: cloud or ComputeClientFactory is nil"),
1575+
},
1576+
{
1577+
name: "Get storage access key successfully",
1578+
expectedError: nil,
1579+
},
1580+
}
1581+
1582+
for _, tc := range testCases {
1583+
d := NewFakeDriver()
1584+
d.cloud = &storage.AccountRepo{}
1585+
if !strings.Contains(tc.name, "is nil") {
1586+
ctrl := gomock.NewController(t)
1587+
defer ctrl.Finish()
1588+
mockStorageAccountsClient := mock_accountclient.NewMockInterface(ctrl)
1589+
d.cloud.ComputeClientFactory = mock_azclient.NewMockClientFactory(ctrl)
1590+
d.cloud.ComputeClientFactory.(*mock_azclient.MockClientFactory).EXPECT().GetAccountClient().Return(mockStorageAccountsClient).AnyTimes()
1591+
s := "unit-test"
1592+
accountkey := armstorage.AccountKey{Value: &s}
1593+
mockStorageAccountsClient.EXPECT().ListKeys(gomock.Any(), gomock.Any(), gomock.Any()).Return([]*armstorage.AccountKey{&accountkey}, tc.expectedError).AnyTimes()
1594+
d.cloud.ComputeClientFactory.(*mock_azclient.MockClientFactory).EXPECT().GetAccountClientForSub(gomock.Any()).Return(mockStorageAccountsClient, nil).AnyTimes()
1595+
}
1596+
_, err := d.GetStorageAccesskeyWithSubsID(context.TODO(), "test-subID", "test-rg", "test-sa", true)
1597+
assert.Equal(t, tc.expectedError, err)
1598+
}
1599+
}

0 commit comments

Comments
 (0)