Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: add fallback to sas token on azcopy copy command #1557

Closed
wants to merge 1 commit into from
Closed

fix: add fallback to sas token on azcopy copy command #1557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 16 additions & 18 deletions pkg/blob/controllerserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,12 +436,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 @@ -775,7 +785,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 @@ -868,11 +878,10 @@ 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 {
// search in cache first
if cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault); err == nil && cache != nil {
Expand All @@ -883,17 +892,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,18 +852,12 @@ func TestCreateVolume(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

m := util.NewMockEXEC(ctrl)

clientFactoryMock := mock_azclient.NewMockClientFactory(ctrl)
blobClientMock := mock_blobcontainerclient.NewMockInterface(ctrl)
blobClientMock.EXPECT().CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil)
clientFactoryMock.EXPECT().GetBlobContainerClientForSub(gomock.Any()).Return(blobClientMock, nil)
d.clientFactory = clientFactoryMock

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 @@ -2024,7 +2018,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 @@ -2047,7 +2041,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 @@ -2073,7 +2067,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 @@ -2094,7 +2088,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
Loading