Skip to content

Commit 44041e6

Browse files
andyzhangxandyzhangx
authored
Merge pull request #2105 from umagnus/release-1.29-sas
[release-1.29] cleanup: refactor fallback to sas token on azcopy copy command
2 parents 06808b5 + 1cb50a4 commit 44041e6

File tree

3 files changed

+26
-57
lines changed

3 files changed

+26
-57
lines changed

‎pkg/azurefile/azurefile.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -996,7 +996,7 @@ func (d *Driver) ResizeFileShare(ctx context.Context, subsID, resourceGroup, acc
996996
}
997997

998998
// copyFileShare copies a fileshare, if dstAccountName is empty, then copy in the same account
999-
func (d *Driver) copyFileShare(ctx context.Context, req *csi.CreateVolumeRequest, dstAccountName string, dstAccountSasToken string, authAzcopyEnv []string, secretName, secretNamespace string, secrets map[string]string, shareOptions *fileclient.ShareOptions, accountOptions *azure.AccountOptions, storageEndpointSuffix string) error {
999+
func (d *Driver) copyFileShare(ctx context.Context, req *csi.CreateVolumeRequest, dstAccountName string, dstAccountSasToken string, authAzcopyEnv []string, secretNamespace string, shareOptions *fileclient.ShareOptions, accountOptions *azure.AccountOptions, storageEndpointSuffix string) error {
10001000
if shareOptions.Protocol == storage.EnabledProtocolsNFS {
10011001
return fmt.Errorf("protocol nfs is not supported for volume cloning")
10021002
}
@@ -1031,7 +1031,7 @@ func (d *Driver) copyFileShare(ctx context.Context, req *csi.CreateVolumeRequest
10311031
srcPath := fmt.Sprintf("https://%s.file.%s/%s%s", srcAccountName, storageEndpointSuffix, srcFileShareName, srcAccountSasToken)
10321032
dstPath := fmt.Sprintf("https://%s.file.%s/%s%s", dstAccountName, storageEndpointSuffix, dstFileShareName, dstAccountSasToken)
10331033

1034-
return d.copyFileShareByAzcopy(ctx, srcFileShareName, dstFileShareName, srcPath, dstPath, srcAccountName, dstAccountName, srcResourceGroupName, srcAccountSasToken, authAzcopyEnv, secretName, secretNamespace, secrets, accountOptions, storageEndpointSuffix)
1034+
return d.copyFileShareByAzcopy(srcFileShareName, dstFileShareName, srcPath, dstPath, srcAccountName, dstAccountName, authAzcopyEnv, accountOptions)
10351035
}
10361036

10371037
// GetTotalAccountQuota returns the total quota in GB of all file shares in the storage account and the number of file shares

‎pkg/azurefile/controllerserver.go

+17-35
Original file line numberDiff line numberDiff line change
@@ -590,8 +590,18 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
590590
if err != nil {
591591
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
592592
}
593-
if err := d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, secretName, secretNamespace, secret, shareOptions, accountOptions, storageEndpointSuffix); err != nil {
594-
return nil, err
593+
var copyErr error
594+
copyErr = d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, secretNamespace, shareOptions, accountOptions, storageEndpointSuffix)
595+
if accountSASToken == "" && copyErr != nil && strings.Contains(copyErr.Error(), authorizationPermissionMismatch) {
596+
klog.Warningf("azcopy copy failed with AuthorizationPermissionMismatch error, should assign \"Storage File Data Privileged Contributor\" role to controller identity, fall back to use sas token, original error: %v", copyErr)
597+
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secret, secretName, secretNamespace, true)
598+
if err != nil {
599+
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
600+
}
601+
copyErr = d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, secretNamespace, shareOptions, accountOptions, storageEndpointSuffix)
602+
}
603+
if copyErr != nil {
604+
return nil, copyErr
595605
}
596606
// storeAccountKey is not needed here since copy volume is only using SAS token
597607
storeAccountKey = false
@@ -741,13 +751,13 @@ func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest)
741751
}
742752

743753
// copyVolume copy an azure file
744-
func (d *Driver) copyVolume(ctx context.Context, req *csi.CreateVolumeRequest, accountName string, accountSASToken string, authAzcopyEnv []string, secretName, secretNamespace string, secrets map[string]string, shareOptions *fileclient.ShareOptions, accountOptions *azure.AccountOptions, storageEndpointSuffix string) error {
754+
func (d *Driver) copyVolume(ctx context.Context, req *csi.CreateVolumeRequest, accountName, accountSASToken string, authAzcopyEnv []string, secretNamespace string, shareOptions *fileclient.ShareOptions, accountOptions *azure.AccountOptions, storageEndpointSuffix string) error {
745755
vs := req.VolumeContentSource
746756
switch vs.Type.(type) {
747757
case *csi.VolumeContentSource_Snapshot:
748758
return status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
749759
case *csi.VolumeContentSource_Volume:
750-
return d.copyFileShare(ctx, req, accountName, accountSASToken, authAzcopyEnv, secretName, secretNamespace, secrets, shareOptions, accountOptions, storageEndpointSuffix)
760+
return d.copyFileShare(ctx, req, accountName, accountSASToken, authAzcopyEnv, secretNamespace, shareOptions, accountOptions, storageEndpointSuffix)
751761
default:
752762
return status.Errorf(codes.InvalidArgument, "%v is not a proper volume source", vs)
753763
}
@@ -999,7 +1009,7 @@ func (d *Driver) ListSnapshots(_ context.Context, _ *csi.ListSnapshotsRequest) (
9991009
return nil, status.Error(codes.Unimplemented, "")
10001010
}
10011011

1002-
func (d *Driver) copyFileShareByAzcopy(ctx context.Context, srcFileShareName, dstFileShareName, srcPath, dstPath, srcAccountName, dstAccountName, srcResourceGroupName, accountSASToken string, authAzcopyEnv []string, secretName, secretNamespace string, secrets map[string]string, accountOptions *azure.AccountOptions, storageEndpointSuffix string) error {
1012+
func (d *Driver) copyFileShareByAzcopy(srcFileShareName, dstFileShareName, srcPath, dstPath, srcAccountName, dstAccountName string, authAzcopyEnv []string, accountOptions *azure.AccountOptions) error {
10031013
azcopyCopyOptions := defaultAzcopyCopyOptions
10041014
jobState, percent, err := d.azcopy.GetAzcopyJob(dstFileShareName, authAzcopyEnv)
10051015
klog.V(2).Infof("azcopy job status: %s, copy percent: %s%%, error: %v", jobState, percent, err)
@@ -1021,33 +1031,6 @@ func (d *Driver) copyFileShareByAzcopy(ctx context.Context, srcFileShareName, ds
10211031
return fmt.Errorf("timeout waiting for copy fileshare %s:%s to %s:%s complete, current copy percent: %s%%", srcAccountName, srcFileShareName, dstAccountName, dstFileShareName, percent)
10221032
}
10231033
copyErr := volumehelper.WaitUntilTimeout(time.Duration(d.waitForAzCopyTimeoutMinutes)*time.Minute, execFuncWithAuth, timeoutFunc)
1024-
if accountSASToken == "" && copyErr != nil && strings.Contains(copyErr.Error(), authorizationPermissionMismatch) {
1025-
klog.Warningf("azcopy list failed with AuthorizationPermissionMismatch error, should assign \"Storage File Data Privileged Contributor\" role to controller identity, fall back to use sas token, original error: %v", copyErr)
1026-
var srcSasToken, dstSasToken string
1027-
srcAccountOptions := &azure.AccountOptions{
1028-
Name: srcAccountName,
1029-
ResourceGroup: srcResourceGroupName,
1030-
SubscriptionID: accountOptions.SubscriptionID,
1031-
GetLatestAccountKey: accountOptions.GetLatestAccountKey,
1032-
}
1033-
if srcSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace, true); err != nil {
1034-
return err
1035-
}
1036-
if srcAccountName == dstAccountName {
1037-
dstSasToken = srcSasToken
1038-
} else {
1039-
if dstSasToken, _, err = d.getAzcopyAuth(ctx, dstAccountName, "", storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace, true); err != nil {
1040-
return err
1041-
}
1042-
}
1043-
execFuncWithSasToken := func() error {
1044-
if out, err := d.execAzcopyCopy(srcPath+srcSasToken, dstPath+dstSasToken, azcopyCopyOptions, []string{}); err != nil {
1045-
return fmt.Errorf("exec error: %v, output: %v", err, string(out))
1046-
}
1047-
return nil
1048-
}
1049-
copyErr = volumehelper.WaitUntilTimeout(time.Duration(d.waitForAzCopyTimeoutMinutes)*time.Minute, execFuncWithSasToken, timeoutFunc)
1050-
}
10511034
if copyErr != nil {
10521035
klog.Warningf("CopyFileShare(%s, %s, %s) failed with error: %v", accountOptions.ResourceGroup, dstAccountName, dstFileShareName, copyErr)
10531036
} else {
@@ -1309,12 +1292,11 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
13091292
// getAzcopyAuth will only generate sas token for azcopy in following conditions:
13101293
// 1. secrets is not empty
13111294
// 2. driver is not using managed identity and service principal
1312-
// 3. azcopy returns AuthorizationPermissionMismatch error when using service principal or managed identity
1313-
// 4. parameter useSasToken is true
1295+
// 3. parameter useSasToken is true
13141296
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) {
13151297
var authAzcopyEnv []string
13161298
var err error
1317-
if !useSasToken && len(secrets) == 0 && len(secretName) == 0 {
1299+
if !useSasToken && !d.useDataPlaneAPI("", accountName) && len(secrets) == 0 && len(secretName) == 0 {
13181300
// search in cache first
13191301
if cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault); err == nil && cache != nil {
13201302
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)

‎pkg/azurefile/controllerserver_test.go

+7-20
Original file line numberDiff line numberDiff line change
@@ -1739,13 +1739,11 @@ func TestCopyVolume(t *testing.T) {
17391739
VolumeContentSource: &volumecontensource,
17401740
}
17411741

1742-
secret := map[string]string{}
1743-
17441742
d := NewFakeDriver()
17451743
ctx := context.Background()
17461744

17471745
expectedErr := status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
1748-
err := d.copyVolume(ctx, req, "", "", []string{}, "", "", secret, nil, nil, "core.windows.net")
1746+
err := d.copyVolume(ctx, req, "", "", []string{}, "", nil, nil, "core.windows.net")
17491747
if !reflect.DeepEqual(err, expectedErr) {
17501748
t.Errorf("Unexpected error: %v", err)
17511749
}
@@ -1774,13 +1772,11 @@ func TestCopyVolume(t *testing.T) {
17741772
VolumeContentSource: &volumecontensource,
17751773
}
17761774

1777-
secret := map[string]string{}
1778-
17791775
d := NewFakeDriver()
17801776
ctx := context.Background()
17811777

17821778
expectedErr := fmt.Errorf("protocol nfs is not supported for volume cloning")
1783-
err := d.copyVolume(ctx, req, "", "", []string{}, "", "", secret, &fileclient.ShareOptions{Protocol: storage.EnabledProtocolsNFS}, nil, "core.windows.net")
1779+
err := d.copyVolume(ctx, req, "", "", []string{}, "", &fileclient.ShareOptions{Protocol: storage.EnabledProtocolsNFS}, nil, "core.windows.net")
17841780
if !reflect.DeepEqual(err, expectedErr) {
17851781
t.Errorf("Unexpected error: %v", err)
17861782
}
@@ -1809,13 +1805,11 @@ func TestCopyVolume(t *testing.T) {
18091805
VolumeContentSource: &volumecontensource,
18101806
}
18111807

1812-
secret := map[string]string{}
1813-
18141808
d := NewFakeDriver()
18151809
ctx := context.Background()
18161810

18171811
expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #")
1818-
err := d.copyVolume(ctx, req, "", "", []string{}, "", "", secret, &fileclient.ShareOptions{Name: "dstFileshare"}, nil, "core.windows.net")
1812+
err := d.copyVolume(ctx, req, "", "", []string{}, "", &fileclient.ShareOptions{Name: "dstFileshare"}, nil, "core.windows.net")
18191813
if !reflect.DeepEqual(err, expectedErr) {
18201814
t.Errorf("Unexpected error: %v", err)
18211815
}
@@ -1844,13 +1838,11 @@ func TestCopyVolume(t *testing.T) {
18441838
VolumeContentSource: &volumecontensource,
18451839
}
18461840

1847-
secret := map[string]string{}
1848-
18491841
d := NewFakeDriver()
18501842
ctx := context.Background()
18511843

18521844
expectedErr := fmt.Errorf("one or more of srcAccountName(unit-test), srcFileShareName(), dstFileShareName(dstFileshare) are empty")
1853-
err := d.copyVolume(ctx, req, "", "", []string{}, "", "", secret, &fileclient.ShareOptions{Name: "dstFileshare"}, nil, "core.windows.net")
1845+
err := d.copyVolume(ctx, req, "", "", []string{}, "", &fileclient.ShareOptions{Name: "dstFileshare"}, nil, "core.windows.net")
18541846
if !reflect.DeepEqual(err, expectedErr) {
18551847
t.Errorf("Unexpected error: %v", err)
18561848
}
@@ -1879,13 +1871,11 @@ func TestCopyVolume(t *testing.T) {
18791871
VolumeContentSource: &volumecontensource,
18801872
}
18811873

1882-
secret := map[string]string{}
1883-
18841874
d := NewFakeDriver()
18851875
ctx := context.Background()
18861876

18871877
expectedErr := fmt.Errorf("one or more of srcAccountName(f5713de20cde511e8ba4900), srcFileShareName(fileshare), dstFileShareName() are empty")
1888-
err := d.copyVolume(ctx, req, "", "", []string{}, "", "", secret, &fileclient.ShareOptions{}, nil, "core.windows.net")
1878+
err := d.copyVolume(ctx, req, "", "", []string{}, "", &fileclient.ShareOptions{}, nil, "core.windows.net")
18891879
if !reflect.DeepEqual(err, expectedErr) {
18901880
t.Errorf("Unexpected error: %v", err)
18911881
}
@@ -1913,12 +1903,10 @@ func TestCopyVolume(t *testing.T) {
19131903
Parameters: mp,
19141904
VolumeContentSource: &volumecontensource,
19151905
}
1916-
1917-
secret := map[string]string{}
19181906
ctx := context.Background()
19191907

19201908
expectedErr := fmt.Errorf("one or more of srcAccountName(f5713de20cde511e8ba4900), srcFileShareName(fileshare), dstFileShareName() are empty")
1921-
err := d.copyVolume(ctx, req, "", "", []string{}, "", "", secret, &fileclient.ShareOptions{}, nil, "core.windows.net")
1909+
err := d.copyVolume(ctx, req, "", "", []string{}, "", &fileclient.ShareOptions{}, nil, "core.windows.net")
19221910
if !reflect.DeepEqual(err, expectedErr) {
19231911
t.Errorf("Unexpected error: %v", err)
19241912
}
@@ -1946,7 +1934,6 @@ func TestCopyVolume(t *testing.T) {
19461934
Parameters: mp,
19471935
VolumeContentSource: &volumecontensource,
19481936
}
1949-
secret := map[string]string{}
19501937
ctx := context.Background()
19511938

19521939
ctrl := gomock.NewController(t)
@@ -1960,7 +1947,7 @@ func TestCopyVolume(t *testing.T) {
19601947
d.azcopy.ExecCmd = m
19611948

19621949
expectedErr := fmt.Errorf("wait for the existing AzCopy job to complete, current copy percentage is 50.0%%")
1963-
err := d.copyVolume(ctx, req, "", "sastoken", []string{}, "", "", secret, &fileclient.ShareOptions{Name: "dstFileshare"}, nil, "core.windows.net")
1950+
err := d.copyVolume(ctx, req, "", "sastoken", []string{}, "", &fileclient.ShareOptions{Name: "dstFileshare"}, nil, "core.windows.net")
19641951
if !reflect.DeepEqual(err, expectedErr) {
19651952
t.Errorf("Unexpected error: %v", err)
19661953
}

0 commit comments

Comments
 (0)