Skip to content

Commit 653799d

Browse files
andyzhangxandyzhangx
authored
Merge pull request #1567 from k8s-infra-cherrypick-robot/cherry-pick-1564-to-release-1.24
[release-1.24] fix: add fallback to sas token on azcopy copy command
2 parents c15ec89 + a18e024 commit 653799d

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

@@ -770,7 +780,7 @@ func (d *Driver) copyBlobContainer(ctx context.Context, req *csi.CreateVolumeReq
770780
SubscriptionID: srcSubscriptionID,
771781
GetLatestAccountKey: accountOptions.GetLatestAccountKey,
772782
}
773-
if srcAccountSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace); err != nil {
783+
if srcAccountSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace, true); err != nil {
774784
return err
775785
}
776786
}
@@ -863,12 +873,11 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
863873
// getAzcopyAuth will only generate sas token for azcopy in following conditions:
864874
// 1. secrets is not empty
865875
// 2. driver is not using managed identity and service principal
866-
// 3. azcopy returns AuthorizationPermissionMismatch error when using service principal or managed identity
867-
func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, []string, error) {
876+
// 3. parameter useSasToken is true
877+
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) {
868878
var authAzcopyEnv []string
869879
var err error
870-
useSasToken := false
871-
if !d.useDataPlaneAPI("", accountName) && len(secrets) == 0 && len(secretName) == 0 {
880+
if !useSasToken && !d.useDataPlaneAPI("", accountName) && len(secrets) == 0 && len(secretName) == 0 {
872881
// search in cache first
873882
if cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault); err == nil && cache != nil {
874883
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
@@ -878,17 +887,6 @@ func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, sto
878887
authAzcopyEnv, err = d.authorizeAzcopyWithIdentity()
879888
if err != nil {
880889
klog.Warningf("failed to authorize azcopy with identity, error: %v", err)
881-
} else {
882-
if len(authAzcopyEnv) > 0 {
883-
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
884-
if testErr != nil {
885-
return "", nil, fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
886-
}
887-
if strings.Contains(out, authorizationPermissionMismatch) {
888-
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)
889-
useSasToken = true
890-
}
891-
}
892890
}
893891
}
894892

‎pkg/blob/controllerserver_test.go

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

797-
m := util.NewMockEXEC(ctrl)
798-
799797
clientFactoryMock := mock_azclient.NewMockClientFactory(ctrl)
800798
blobClientMock := mock_blobcontainerclient.NewMockInterface(ctrl)
801799
blobClientMock.EXPECT().CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil)
802800
clientFactoryMock.EXPECT().GetBlobContainerClientForSub(gomock.Any()).Return(blobClientMock, nil)
803801
d.clientFactory = clientFactoryMock
804802

805-
listStr := "no error"
806-
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil)
807-
d.azcopy.ExecCmd = m
808-
809803
expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #")
810804
_, err := d.CreateVolume(context.Background(), req)
811805
if !reflect.DeepEqual(err, expectedErr) {
@@ -1966,7 +1960,7 @@ func TestGetAzcopyAuth(t *testing.T) {
19661960
ctx := context.Background()
19671961
expectedAccountSASToken := ""
19681962
expectedErr := fmt.Errorf("could not find accountkey or azurestorageaccountkey field in secrets")
1969-
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
1963+
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
19701964
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
19711965
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
19721966
}
@@ -1989,7 +1983,7 @@ func TestGetAzcopyAuth(t *testing.T) {
19891983
defaultSecretAccountName: "accountName",
19901984
defaultSecretAccountKey: "YWNjb3VudGtleQo=",
19911985
}
1992-
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
1986+
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
19931987
if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") {
19941988
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
19951989
}
@@ -2015,7 +2009,7 @@ func TestGetAzcopyAuth(t *testing.T) {
20152009

20162010
expectedAccountSASToken := ""
20172011
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"))
2018-
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2012+
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
20192013
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20202014
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20212015
}
@@ -2036,7 +2030,7 @@ func TestGetAzcopyAuth(t *testing.T) {
20362030
ctx := context.Background()
20372031
expectedAccountSASToken := ""
20382032
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"))
2039-
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
2033+
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
20402034
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
20412035
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
20422036
}

‎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)