Skip to content

Commit 47d9402

Browse files
andyzhangxandyzhangx
authored
Merge pull request #1564 from andyzhangx/fallback-sastoken
fix: add fallback to sas token on azcopy copy command
2 parents 86cb9ad + 4d007d6 commit 47d9402

File tree

7 files changed

+29
-52
lines changed

7 files changed

+29
-52
lines changed

‎deploy/example/storageclass-blob-nfs.yaml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ metadata:
66
provisioner: blob.csi.azure.com
77
parameters:
88
protocol: nfs
9-
useDataPlaneAPI: "false"
109
volumeBindingMode: Immediate
1110
allowVolumeExpansion: true
1211
mountOptions:

‎deploy/example/storageclass-blobfuse.yaml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ metadata:
66
provisioner: blob.csi.azure.com
77
parameters:
88
skuName: Premium_LRS # available values: Standard_LRS, Premium_LRS, Standard_GRS, Standard_RAGRS, Standard_ZRS, Premium_ZRS
9-
useDataPlaneAPI: "false"
109
reclaimPolicy: Delete
1110
volumeBindingMode: Immediate
1211
allowVolumeExpansion: true

‎deploy/example/storageclass-blobfuse2.yaml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ provisioner: blob.csi.azure.com
77
parameters:
88
skuName: Premium_LRS # available values: Standard_LRS, Premium_LRS, Standard_GRS, Standard_RAGRS, Standard_ZRS, Premium_ZRS
99
protocol: fuse2
10-
useDataPlaneAPI: "false"
1110
reclaimPolicy: Delete
1211
volumeBindingMode: Immediate
1312
allowVolumeExpansion: true

‎pkg/blob/controllerserver.go

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -436,12 +436,22 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
436436
return nil, status.Errorf(codes.Internal, "failed to create container(%s) on account(%s) type(%s) rg(%s) location(%s) size(%d), error: %v", validContainerName, accountName, storageAccountType, resourceGroup, location, requestGiB, err)
437437
}
438438
if volContentSource != nil {
439-
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace)
439+
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace, false)
440440
if err != nil {
441441
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
442442
}
443-
if err := d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, validContainerName, secretNamespace, accountOptions, storageEndpointSuffix); err != nil {
444-
return nil, err
443+
var copyErr error
444+
copyErr = d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, validContainerName, secretNamespace, accountOptions, storageEndpointSuffix)
445+
if accountSASToken == "" && copyErr != nil && strings.Contains(copyErr.Error(), authorizationPermissionMismatch) {
446+
klog.Warningf("azcopy copy failed with AuthorizationPermissionMismatch error, should assign \"Storage Blob Data Contributor\" role to controller identity, fall back to use sas token, original error: %v", copyErr)
447+
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace, true)
448+
if err != nil {
449+
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
450+
}
451+
copyErr = d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, validContainerName, secretNamespace, accountOptions, storageEndpointSuffix)
452+
}
453+
if copyErr != nil {
454+
return nil, copyErr
445455
}
446456
}
447457

@@ -775,7 +785,7 @@ func (d *Driver) copyBlobContainer(ctx context.Context, req *csi.CreateVolumeReq
775785
SubscriptionID: srcSubscriptionID,
776786
GetLatestAccountKey: accountOptions.GetLatestAccountKey,
777787
}
778-
if srcAccountSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace); err != nil {
788+
if srcAccountSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace, true); err != nil {
779789
return err
780790
}
781791
}
@@ -868,12 +878,11 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
868878
// getAzcopyAuth will only generate sas token for azcopy in following conditions:
869879
// 1. secrets is not empty
870880
// 2. driver is not using managed identity and service principal
871-
// 3. azcopy returns AuthorizationPermissionMismatch error when using service principal or managed identity
872-
func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, []string, error) {
881+
// 3. parameter useSasToken is true
882+
func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string, useSasToken bool) (string, []string, error) {
873883
var authAzcopyEnv []string
874884
var err error
875-
useSasToken := false
876-
if !d.useDataPlaneAPI("", accountName) && len(secrets) == 0 && len(secretName) == 0 {
885+
if !useSasToken && !d.useDataPlaneAPI("", accountName) && len(secrets) == 0 && len(secretName) == 0 {
877886
// search in cache first
878887
if cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault); err == nil && cache != nil {
879888
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
@@ -883,17 +892,6 @@ func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, sto
883892
authAzcopyEnv, err = d.authorizeAzcopyWithIdentity()
884893
if err != nil {
885894
klog.Warningf("failed to authorize azcopy with identity, error: %v", err)
886-
} else {
887-
if len(authAzcopyEnv) > 0 {
888-
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
889-
if testErr != nil {
890-
return "", nil, fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
891-
}
892-
if strings.Contains(out, authorizationPermissionMismatch) {
893-
klog.Warningf("azcopy list failed with AuthorizationPermissionMismatch error, should assign \"Storage Blob Data Contributor\" role to controller identity, fall back to use sas token, original output: %v", out)
894-
useSasToken = true
895-
}
896-
}
897895
}
898896
}
899897

‎pkg/blob/controllerserver_test.go

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -852,18 +852,12 @@ func TestCreateVolume(t *testing.T) {
852852
ctrl := gomock.NewController(t)
853853
defer ctrl.Finish()
854854

855-
m := util.NewMockEXEC(ctrl)
856-
857855
clientFactoryMock := mock_azclient.NewMockClientFactory(ctrl)
858856
blobClientMock := mock_blobcontainerclient.NewMockInterface(ctrl)
859857
blobClientMock.EXPECT().CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil)
860858
clientFactoryMock.EXPECT().GetBlobContainerClientForSub(gomock.Any()).Return(blobClientMock, nil)
861859
d.clientFactory = clientFactoryMock
862860

863-
listStr := "no error"
864-
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil)
865-
d.azcopy.ExecCmd = m
866-
867861
expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #")
868862
_, err := d.CreateVolume(context.Background(), req)
869863
if !reflect.DeepEqual(err, expectedErr) {
@@ -2024,7 +2018,7 @@ func TestGetAzcopyAuth(t *testing.T) {
20242018
ctx := context.Background()
20252019
expectedAccountSASToken := ""
20262020
expectedErr := fmt.Errorf("could not find accountkey or azurestorageaccountkey field in secrets")
2027-
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2021+
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
20282022
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20292023
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20302024
}
@@ -2047,7 +2041,7 @@ func TestGetAzcopyAuth(t *testing.T) {
20472041
defaultSecretAccountName: "accountName",
20482042
defaultSecretAccountKey: "YWNjb3VudGtleQo=",
20492043
}
2050-
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2044+
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
20512045
if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") {
20522046
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20532047
}
@@ -2073,7 +2067,7 @@ func TestGetAzcopyAuth(t *testing.T) {
20732067

20742068
expectedAccountSASToken := ""
20752069
expectedErr := status.Errorf(codes.Internal, fmt.Sprintf("failed to generate sas token in creating new shared key credential, accountName: %s, err: %s", "accountName", "decode account key: illegal base64 data at input byte 8"))
2076-
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2070+
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
20772071
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20782072
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20792073
}
@@ -2094,7 +2088,7 @@ func TestGetAzcopyAuth(t *testing.T) {
20942088
ctx := context.Background()
20952089
expectedAccountSASToken := ""
20962090
expectedErr := status.Errorf(codes.Internal, fmt.Sprintf("failed to generate sas token in creating new shared key credential, accountName: %s, err: %s", "accountName", "decode account key: illegal base64 data at input byte 8"))
2097-
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2091+
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
20982092
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20992093
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
21002094
}

‎test/e2e/dynamic_provisioning_test.go

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -962,7 +962,6 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
962962
Pod: pod,
963963
PodWithClonedVolume: podWithClonedVolume,
964964
StorageClassParameters: map[string]string{
965-
"useDataPlaneAPI": "true",
966965
"skuName": "Premium_LRS",
967966
"protocol": "nfs",
968967
"mountPermissions": "0755",
@@ -995,7 +994,6 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
995994
Pod: pod,
996995
PodWithClonedVolume: podWithClonedVolume,
997996
StorageClassParameters: map[string]string{
998-
"useDataPlaneAPI": "true",
999997
"skuName": "Premium_LRS",
1000998
"protocol": "nfs",
1001999
"mountPermissions": "0755",
@@ -1029,9 +1027,8 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
10291027
Pod: pod,
10301028
PodWithClonedVolume: podWithClonedVolume,
10311029
StorageClassParameters: map[string]string{
1032-
"useDataPlaneAPI": "true",
1033-
"skuName": "Standard_LRS",
1034-
"protocol": "fuse2",
1030+
"skuName": "Standard_LRS",
1031+
"protocol": "fuse2",
10351032
},
10361033
}
10371034
test.Run(ctx, cs, ns)
@@ -1062,9 +1059,8 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
10621059
Pod: pod,
10631060
PodWithClonedVolume: podWithClonedVolume,
10641061
StorageClassParameters: map[string]string{
1065-
"useDataPlaneAPI": "true",
1066-
"skuName": "Standard_LRS",
1067-
"protocol": "fuse2",
1062+
"skuName": "Standard_LRS",
1063+
"protocol": "fuse2",
10681064
},
10691065
}
10701066
test.Run(ctx, cs, ns)
@@ -1094,14 +1090,12 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
10941090
Pod: pod,
10951091
PodWithClonedVolume: podWithClonedVolume,
10961092
StorageClassParameters: map[string]string{
1097-
"useDataPlaneAPI": "true",
10981093
"skuName": "Premium_LRS",
10991094
"protocol": "nfs",
11001095
"mountPermissions": "0755",
11011096
"allowsharedkeyaccess": "true",
11021097
},
11031098
ClonedStorageClassParameters: map[string]string{
1104-
"useDataPlaneAPI": "true",
11051099
"skuName": "Standard_LRS",
11061100
"protocol": "nfs",
11071101
"mountPermissions": "0755",
@@ -1136,14 +1130,12 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
11361130
Pod: pod,
11371131
PodWithClonedVolume: podWithClonedVolume,
11381132
StorageClassParameters: map[string]string{
1139-
"useDataPlaneAPI": "true",
1140-
"skuName": "Standard_LRS",
1141-
"protocol": "fuse2",
1133+
"skuName": "Standard_LRS",
1134+
"protocol": "fuse2",
11421135
},
11431136
ClonedStorageClassParameters: map[string]string{
1144-
"useDataPlaneAPI": "true",
1145-
"skuName": "Premium_LRS",
1146-
"protocol": "fuse2",
1137+
"skuName": "Premium_LRS",
1138+
"protocol": "fuse2",
11471139
},
11481140
}
11491141
test.Run(ctx, cs, ns)

‎test/external-e2e/run.sh

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,6 @@ setup_e2e_binaries() {
3434
sed -i "s/blob.csi.azure.com/$DRIVER.csi.azure.com/g" deploy/example/storageclass-blobfuse.yaml
3535
sed -i "s/blob.csi.azure.com/$DRIVER.csi.azure.com/g" deploy/example/storageclass-blobfuse2.yaml
3636
sed -i "s/blob.csi.azure.com/$DRIVER.csi.azure.com/g" deploy/example/storageclass-blob-nfs.yaml
37-
# workaround: use useDataPlaneAPI as true for blobfuse and nfs copy volume tests
38-
sed -i "s/\"false\"/\"true\"/g" deploy/example/storageclass-blobfuse.yaml
39-
sed -i "s/\"false\"/\"true\"/g" deploy/example/storageclass-blobfuse2.yaml
40-
sed -i "s/\"false\"/\"true\"/g" deploy/example/storageclass-blob-nfs.yaml
4137

4238
make e2e-bootstrap
4339
sed -i "s/csi-blob-controller/csi-$DRIVER-controller/g" deploy/example/metrics/csi-blob-controller-svc.yaml

0 commit comments

Comments
 (0)