Skip to content

Commit

Permalink
Merge pull request #1568 from andyzhangx/fallback-sastoken-1.23
Browse files Browse the repository at this point in the history 
[release-1.23] fix: add fallback to sas token on azcopy copy command
  • Loading branch information
@andyzhangx
andyzhangx authored Sep 1, 2024
2 parents d685531 + e9e5493 commit 4abf868
Show file tree
Hide file tree
Showing 7 changed files with 29 additions and 52 deletions.
1 change: 0 additions & 1 deletion deploy/example/storageclass-blob-nfs.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ metadata:
provisioner: blob.csi.azure.com
parameters:
protocol: nfs
useDataPlaneAPI: "false"
volumeBindingMode: Immediate
allowVolumeExpansion: true
mountOptions:
Expand Down
1 change: 0 additions & 1 deletion deploy/example/storageclass-blobfuse.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ metadata:
provisioner: blob.csi.azure.com
parameters:
skuName: Premium_LRS # available values: Standard_LRS, Premium_LRS, Standard_GRS, Standard_RAGRS
useDataPlaneAPI: "false"
reclaimPolicy: Delete
volumeBindingMode: Immediate
allowVolumeExpansion: true
Expand Down
1 change: 0 additions & 1 deletion deploy/example/storageclass-blobfuse2.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ provisioner: blob.csi.azure.com
parameters:
skuName: Premium_LRS # available values: Standard_LRS, Premium_LRS, Standard_GRS, Standard_RAGRS
protocol: fuse2
useDataPlaneAPI: "false"
reclaimPolicy: Delete
volumeBindingMode: Immediate
allowVolumeExpansion: true
Expand Down
36 changes: 17 additions & 19 deletions pkg/blob/controllerserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -430,12 +430,22 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
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)
}
if volContentSource != nil {
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace)
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace, false)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
}
if err := d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, validContainerName, secretNamespace, accountOptions, storageEndpointSuffix); err != nil {
return nil, err
var copyErr error
copyErr = d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, validContainerName, secretNamespace, accountOptions, storageEndpointSuffix)
if accountSASToken == "" && copyErr != nil && strings.Contains(copyErr.Error(), authorizationPermissionMismatch) {
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)
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace, true)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
}
copyErr = d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, validContainerName, secretNamespace, accountOptions, storageEndpointSuffix)
}
if copyErr != nil {
return nil, copyErr
}
}

Expand Down Expand Up @@ -748,7 +758,7 @@ func (d *Driver) copyBlobContainer(ctx context.Context, req *csi.CreateVolumeReq
SubscriptionID: srcSubscriptionID,
GetLatestAccountKey: accountOptions.GetLatestAccountKey,
}
if srcAccountSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace); err != nil {
if srcAccountSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace, true); err != nil {
return err
}
}
Expand Down Expand Up @@ -841,12 +851,11 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
// getAzcopyAuth will only generate sas token for azcopy in following conditions:
// 1. secrets is not empty
// 2. driver is not using managed identity and service principal
// 3. azcopy returns AuthorizationPermissionMismatch error when using service principal or managed identity
func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, []string, error) {
// 3. parameter useSasToken is true
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) {
var authAzcopyEnv []string
var err error
useSasToken := false
if !d.useDataPlaneAPI("", accountName) && len(secrets) == 0 && len(secretName) == 0 {
if !useSasToken && !d.useDataPlaneAPI("", accountName) && len(secrets) == 0 && len(secretName) == 0 {
// search in cache first
if cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault); err == nil && cache != nil {
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
Expand All @@ -856,17 +865,6 @@ func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, sto
authAzcopyEnv, err = d.authorizeAzcopyWithIdentity()
if err != nil {
klog.Warningf("failed to authorize azcopy with identity, error: %v", err)
} else {
if len(authAzcopyEnv) > 0 {
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
if testErr != nil {
return "", nil, fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
}
if strings.Contains(out, authorizationPermissionMismatch) {
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)
useSasToken = true
}
}
}
}

Expand Down
14 changes: 4 additions & 10 deletions pkg/blob/controllerserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -817,12 +817,6 @@ func TestCreateVolume(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

m := util.NewMockEXEC(ctrl)

listStr := "no error"
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil)
d.azcopy.ExecCmd = m

expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #")
_, err := d.CreateVolume(context.Background(), req)
if !reflect.DeepEqual(err, expectedErr) {
Expand Down Expand Up @@ -1929,7 +1923,7 @@ func TestGetAzcopyAuth(t *testing.T) {
ctx := context.Background()
expectedAccountSASToken := ""
expectedErr := fmt.Errorf("could not find accountkey or azurestorageaccountkey field in secrets")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand All @@ -1952,7 +1946,7 @@ func TestGetAzcopyAuth(t *testing.T) {
defaultSecretAccountName: "accountName",
defaultSecretAccountKey: "YWNjb3VudGtleQo=",
}
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand All @@ -1978,7 +1972,7 @@ func TestGetAzcopyAuth(t *testing.T) {

expectedAccountSASToken := ""
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"))
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand All @@ -1999,7 +1993,7 @@ func TestGetAzcopyAuth(t *testing.T) {
ctx := context.Background()
expectedAccountSASToken := ""
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"))
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace", false)
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand Down
24 changes: 8 additions & 16 deletions test/e2e/dynamic_provisioning_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -898,7 +898,6 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Premium_LRS",
"protocol": "nfs",
"mountPermissions": "0755",
Expand Down Expand Up @@ -931,7 +930,6 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Premium_LRS",
"protocol": "nfs",
"mountPermissions": "0755",
Expand Down Expand Up @@ -965,9 +963,8 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Standard_LRS",
"protocol": "fuse2",
"skuName": "Standard_LRS",
"protocol": "fuse2",
},
}
test.Run(ctx, cs, ns)
Expand Down Expand Up @@ -998,9 +995,8 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Standard_LRS",
"protocol": "fuse2",
"skuName": "Standard_LRS",
"protocol": "fuse2",
},
}
test.Run(ctx, cs, ns)
Expand Down Expand Up @@ -1030,14 +1026,12 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Premium_LRS",
"protocol": "nfs",
"mountPermissions": "0755",
"allowsharedkeyaccess": "true",
},
ClonedStorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Standard_LRS",
"protocol": "nfs",
"mountPermissions": "0755",
Expand Down Expand Up @@ -1072,14 +1066,12 @@ var _ = ginkgo.Describe("[blob-csi-e2e] Dynamic Provisioning", func() {
Pod: pod,
PodWithClonedVolume: podWithClonedVolume,
StorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Standard_LRS",
"protocol": "fuse2",
"skuName": "Standard_LRS",
"protocol": "fuse2",
},
ClonedStorageClassParameters: map[string]string{
"useDataPlaneAPI": "true",
"skuName": "Premium_LRS",
"protocol": "fuse2",
"skuName": "Premium_LRS",
"protocol": "fuse2",
},
}
test.Run(ctx, cs, ns)
Expand Down
4 changes: 0 additions & 4 deletions test/external-e2e/run.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ setup_e2e_binaries() {
sed -i "s/blob.csi.azure.com/$DRIVER.csi.azure.com/g" deploy/example/storageclass-blobfuse.yaml
sed -i "s/blob.csi.azure.com/$DRIVER.csi.azure.com/g" deploy/example/storageclass-blobfuse2.yaml
sed -i "s/blob.csi.azure.com/$DRIVER.csi.azure.com/g" deploy/example/storageclass-blob-nfs.yaml
# workaround: use useDataPlaneAPI as true for blobfuse and nfs copy volume tests
sed -i "s/\"false\"/\"true\"/g" deploy/example/storageclass-blobfuse.yaml
sed -i "s/\"false\"/\"true\"/g" deploy/example/storageclass-blobfuse2.yaml
sed -i "s/\"false\"/\"true\"/g" deploy/example/storageclass-blob-nfs.yaml

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

0 comments on commit 4abf868

Please sign in to comment.