Skip to content

Commit

Permalink
Merge pull request #2105 from umagnus/release-1.29-sas
Browse files Browse the repository at this point in the history 
[release-1.29] cleanup: refactor fallback to sas token on azcopy copy command
  • Loading branch information
@andyzhangx
andyzhangx authored Sep 14, 2024
2 parents 06808b5 + 1cb50a4 commit 44041e6
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 57 deletions.
4 changes: 2 additions & 2 deletions pkg/azurefile/azurefile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -996,7 +996,7 @@ func (d *Driver) ResizeFileShare(ctx context.Context, subsID, resourceGroup, acc
}

// copyFileShare copies a fileshare, if dstAccountName is empty, then copy in the same account
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 {
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 {
if shareOptions.Protocol == storage.EnabledProtocolsNFS {
return fmt.Errorf("protocol nfs is not supported for volume cloning")
}
Expand Down Expand Up @@ -1031,7 +1031,7 @@ func (d *Driver) copyFileShare(ctx context.Context, req *csi.CreateVolumeRequest
srcPath := fmt.Sprintf("https://%s.file.%s/%s%s", srcAccountName, storageEndpointSuffix, srcFileShareName, srcAccountSasToken)
dstPath := fmt.Sprintf("https://%s.file.%s/%s%s", dstAccountName, storageEndpointSuffix, dstFileShareName, dstAccountSasToken)

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

// GetTotalAccountQuota returns the total quota in GB of all file shares in the storage account and the number of file shares
Expand Down
52 changes: 17 additions & 35 deletions pkg/azurefile/controllerserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,8 +590,18 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
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, secretName, secretNamespace, secret, shareOptions, accountOptions, storageEndpointSuffix); err != nil {
return nil, err
var copyErr error
copyErr = d.copyVolume(ctx, req, accountName, accountSASToken, authAzcopyEnv, secretNamespace, shareOptions, accountOptions, storageEndpointSuffix)
if accountSASToken == "" && copyErr != nil && strings.Contains(copyErr.Error(), authorizationPermissionMismatch) {
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)
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secret, 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, secretNamespace, shareOptions, accountOptions, storageEndpointSuffix)
}
if copyErr != nil {
return nil, copyErr
}
// storeAccountKey is not needed here since copy volume is only using SAS token
storeAccountKey = false
Expand Down Expand Up @@ -741,13 +751,13 @@ func (d *Driver) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest)
}

// copyVolume copy an azure file
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 {
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 {
vs := req.VolumeContentSource
switch vs.Type.(type) {
case *csi.VolumeContentSource_Snapshot:
return status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
case *csi.VolumeContentSource_Volume:
return d.copyFileShare(ctx, req, accountName, accountSASToken, authAzcopyEnv, secretName, secretNamespace, secrets, shareOptions, accountOptions, storageEndpointSuffix)
return d.copyFileShare(ctx, req, accountName, accountSASToken, authAzcopyEnv, secretNamespace, shareOptions, accountOptions, storageEndpointSuffix)
default:
return status.Errorf(codes.InvalidArgument, "%v is not a proper volume source", vs)
}
Expand Down Expand Up @@ -999,7 +1009,7 @@ func (d *Driver) ListSnapshots(_ context.Context, _ *csi.ListSnapshotsRequest) (
return nil, status.Error(codes.Unimplemented, "")
}

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 {
func (d *Driver) copyFileShareByAzcopy(srcFileShareName, dstFileShareName, srcPath, dstPath, srcAccountName, dstAccountName string, authAzcopyEnv []string, accountOptions *azure.AccountOptions) error {
azcopyCopyOptions := defaultAzcopyCopyOptions
jobState, percent, err := d.azcopy.GetAzcopyJob(dstFileShareName, authAzcopyEnv)
klog.V(2).Infof("azcopy job status: %s, copy percent: %s%%, error: %v", jobState, percent, err)
Expand All @@ -1021,33 +1031,6 @@ func (d *Driver) copyFileShareByAzcopy(ctx context.Context, srcFileShareName, ds
return fmt.Errorf("timeout waiting for copy fileshare %s:%s to %s:%s complete, current copy percent: %s%%", srcAccountName, srcFileShareName, dstAccountName, dstFileShareName, percent)
}
copyErr := volumehelper.WaitUntilTimeout(time.Duration(d.waitForAzCopyTimeoutMinutes)*time.Minute, execFuncWithAuth, timeoutFunc)
if accountSASToken == "" && copyErr != nil && strings.Contains(copyErr.Error(), authorizationPermissionMismatch) {
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)
var srcSasToken, dstSasToken string
srcAccountOptions := &azure.AccountOptions{
Name: srcAccountName,
ResourceGroup: srcResourceGroupName,
SubscriptionID: accountOptions.SubscriptionID,
GetLatestAccountKey: accountOptions.GetLatestAccountKey,
}
if srcSasToken, _, err = d.getAzcopyAuth(ctx, srcAccountName, "", storageEndpointSuffix, srcAccountOptions, nil, "", secretNamespace, true); err != nil {
return err
}
if srcAccountName == dstAccountName {
dstSasToken = srcSasToken
} else {
if dstSasToken, _, err = d.getAzcopyAuth(ctx, dstAccountName, "", storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace, true); err != nil {
return err
}
}
execFuncWithSasToken := func() error {
if out, err := d.execAzcopyCopy(srcPath+srcSasToken, dstPath+dstSasToken, azcopyCopyOptions, []string{}); err != nil {
return fmt.Errorf("exec error: %v, output: %v", err, string(out))
}
return nil
}
copyErr = volumehelper.WaitUntilTimeout(time.Duration(d.waitForAzCopyTimeoutMinutes)*time.Minute, execFuncWithSasToken, timeoutFunc)
}
if copyErr != nil {
klog.Warningf("CopyFileShare(%s, %s, %s) failed with error: %v", accountOptions.ResourceGroup, dstAccountName, dstFileShareName, copyErr)
} else {
Expand Down Expand Up @@ -1309,12 +1292,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
// 4. parameter useSasToken is true
// 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
if !useSasToken && 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 Down
27 changes: 7 additions & 20 deletions pkg/azurefile/controllerserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1739,13 +1739,11 @@ func TestCopyVolume(t *testing.T) {
VolumeContentSource: &volumecontensource,
}

secret := map[string]string{}

d := NewFakeDriver()
ctx := context.Background()

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

secret := map[string]string{}

d := NewFakeDriver()
ctx := context.Background()

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

secret := map[string]string{}

d := NewFakeDriver()
ctx := context.Background()

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

secret := map[string]string{}

d := NewFakeDriver()
ctx := context.Background()

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

secret := map[string]string{}

d := NewFakeDriver()
ctx := context.Background()

expectedErr := fmt.Errorf("one or more of srcAccountName(f5713de20cde511e8ba4900), srcFileShareName(fileshare), dstFileShareName() are empty")
err := d.copyVolume(ctx, req, "", "", []string{}, "", "", secret, &fileclient.ShareOptions{}, nil, "core.windows.net")
err := d.copyVolume(ctx, req, "", "", []string{}, "", &fileclient.ShareOptions{}, nil, "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1913,12 +1903,10 @@ func TestCopyVolume(t *testing.T) {
Parameters: mp,
VolumeContentSource: &volumecontensource,
}

secret := map[string]string{}
ctx := context.Background()

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

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

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

0 comments on commit 44041e6

Please sign in to comment.