diff --git a/pkg/blob/controllerserver.go b/pkg/blob/controllerserver.go index 7c1059958..2b1882341 100644 --- a/pkg/blob/controllerserver.go +++ b/pkg/blob/controllerserver.go @@ -421,11 +421,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) } if req.GetVolumeContentSource() != nil { - var accountSASToken string - if accountSASToken, err = d.getSASToken(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace); err != nil { - return nil, status.Errorf(codes.Internal, "failed to getSASToken on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err) + accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace) + 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, accountSASToken, validContainerName, storageEndpointSuffix); err != nil { + if err := d.copyVolume(req, accountSASToken, authAzcopyEnv, validContainerName, storageEndpointSuffix); err != nil { return nil, err } } else { @@ -720,7 +720,7 @@ func (d *Driver) DeleteBlobContainer(ctx context.Context, subsID, resourceGroupN } // CopyBlobContainer copies a blob container in the same storage account -func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeRequest, accountSasToken, dstContainerName, storageEndpointSuffix string) error { +func (d *Driver) copyBlobContainer(req *csi.CreateVolumeRequest, accountSasToken string, authAzcopyEnv []string, dstContainerName, storageEndpointSuffix string) error { var sourceVolumeID string if req.GetVolumeContentSource() != nil && req.GetVolumeContentSource().GetVolume() != nil { sourceVolumeID = req.GetVolumeContentSource().GetVolume().GetVolumeId() @@ -734,13 +734,6 @@ func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeReque return fmt.Errorf("srcContainerName(%s) or dstContainerName(%s) is empty", srcContainerName, dstContainerName) } - var authAzcopyEnv []string - if accountSasToken == "" { - if authAzcopyEnv, err = d.authorizeAzcopyWithIdentity(); err != nil { - return err - } - } - timeAfter := time.After(waitForCopyTimeout) timeTick := time.Tick(waitForCopyInterval) srcPath := fmt.Sprintf("https://%s.blob.%s/%s%s", accountName, storageEndpointSuffix, srcContainerName, accountSasToken) @@ -781,18 +774,19 @@ func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeReque } // copyVolume copies a volume form volume or snapshot, snapshot is not supported now -func (d *Driver) copyVolume(ctx context.Context, req *csi.CreateVolumeRequest, accountSASToken, dstContainerName, storageEndpointSuffix string) error { +func (d *Driver) copyVolume(req *csi.CreateVolumeRequest, accountSASToken string, authAzcopyEnv []string, dstContainerName, 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.copyBlobContainer(ctx, req, accountSASToken, dstContainerName, storageEndpointSuffix) + return d.copyBlobContainer(req, accountSASToken, authAzcopyEnv, dstContainerName, storageEndpointSuffix) default: return status.Errorf(codes.InvalidArgument, "%v is not a proper volume source", vs) } } +// authorizeAzcopyWithIdentity returns auth env for azcopy using cluster identity func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) { azureAuthConfig := d.cloud.Config.AzureAuthConfig var authAzcopyEnv []string @@ -822,45 +816,55 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) { return []string{}, fmt.Errorf("service principle or managed identity are both not set") } -// getSASToken will only generate sas token for azcopy in following conditions: +// 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) getSASToken(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, error) { - authAzcopyEnv, _ := d.authorizeAzcopyWithIdentity() +func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, []string, error) { + var authAzcopyEnv []string useSasToken := false - if len(authAzcopyEnv) > 0 { - // search in cache first - cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault) + if len(secrets) == 0 && len(secretName) == 0 { + var err error + authAzcopyEnv, err = d.authorizeAzcopyWithIdentity() if err != nil { - return "", fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err) - } - if cache != nil { - klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName) - useSasToken = true + klog.Warningf("failed to authorize azcopy with identity, error: %v", err) } else { - out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv) - if testErr != nil { - return "", 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) - d.azcopySasTokenCache.Set(accountName, "") - useSasToken = true + if len(authAzcopyEnv) > 0 { + // search in cache first + cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault) + if err != nil { + return "", nil, fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err) + } + if cache != nil { + klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName) + useSasToken = true + } else { + 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) + d.azcopySasTokenCache.Set(accountName, "") + useSasToken = true + } + } } } } - if len(secrets) > 0 || len(authAzcopyEnv) == 0 || useSasToken { + + if len(secrets) > 0 || len(secretName) > 0 || len(authAzcopyEnv) == 0 || useSasToken { var err error if accountKey == "" { if _, accountKey, err = d.GetStorageAccesskey(ctx, accountOptions, secrets, secretName, secretNamespace); err != nil { - return "", err + return "", nil, err } } klog.V(2).Infof("generate sas token for account(%s)", accountName) - return generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes) + sasToken, err := generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes) + return sasToken, nil, err } - return "", nil + return "", authAzcopyEnv, nil } // isValidVolumeCapabilities validates the given VolumeCapability array is valid diff --git a/pkg/blob/controllerserver_test.go b/pkg/blob/controllerserver_test.go index 6d6e3ec6c..1a9e67bd2 100644 --- a/pkg/blob/controllerserver_test.go +++ b/pkg/blob/controllerserver_test.go @@ -1555,10 +1555,8 @@ func TestCopyVolume(t *testing.T) { VolumeContentSource: &volumecontensource, } - ctx := context.Background() - expectedErr := status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported") - err := d.copyVolume(ctx, req, "", "", "core.windows.net") + err := d.copyVolume(req, "", nil, "", "core.windows.net") if !reflect.DeepEqual(err, expectedErr) { t.Errorf("Unexpected error: %v", err) } @@ -1587,10 +1585,8 @@ func TestCopyVolume(t *testing.T) { VolumeContentSource: &volumecontensource, } - ctx := context.Background() - expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #") - err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net") + err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net") if !reflect.DeepEqual(err, expectedErr) { t.Errorf("Unexpected error: %v", err) } @@ -1619,10 +1615,8 @@ func TestCopyVolume(t *testing.T) { VolumeContentSource: &volumecontensource, } - ctx := context.Background() - expectedErr := fmt.Errorf("srcContainerName() or dstContainerName(dstContainer) is empty") - err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net") + err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net") if !reflect.DeepEqual(err, expectedErr) { t.Errorf("Unexpected error: %v", err) } @@ -1651,43 +1645,8 @@ func TestCopyVolume(t *testing.T) { VolumeContentSource: &volumecontensource, } - ctx := context.Background() - expectedErr := fmt.Errorf("srcContainerName(fileshare) or dstContainerName() is empty") - err := d.copyVolume(ctx, req, "", "", "core.windows.net") - if !reflect.DeepEqual(err, expectedErr) { - t.Errorf("Unexpected error: %v", err) - } - }, - }, - { - name: "AADClientSecret shouldn't be nil or useManagedIdentityExtension must be set to true when accountSASToken is empty", - testFunc: func(t *testing.T) { - d := NewFakeDriver() - d.cloud = &azure.Cloud{} - mp := map[string]string{} - - volumeSource := &csi.VolumeContentSource_VolumeSource{ - VolumeId: "vol_1#f5713de20cde511e8ba4900#fileshare#", - } - volumeContentSourceVolumeSource := &csi.VolumeContentSource_Volume{ - Volume: volumeSource, - } - volumecontensource := csi.VolumeContentSource{ - Type: volumeContentSourceVolumeSource, - } - - req := &csi.CreateVolumeRequest{ - Name: "unit-test", - VolumeCapabilities: stdVolumeCapabilities, - Parameters: mp, - VolumeContentSource: &volumecontensource, - } - - ctx := context.Background() - - expectedErr := fmt.Errorf("service principle or managed identity are both not set") - err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net") + err := d.copyVolume(req, "", nil, "", "core.windows.net") if !reflect.DeepEqual(err, expectedErr) { t.Errorf("Unexpected error: %v", err) } @@ -1728,10 +1687,8 @@ func TestCopyVolume(t *testing.T) { d.azcopy.ExecCmd = m - ctx := context.Background() - var expectedErr error - err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net") + err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net") if !reflect.DeepEqual(err, expectedErr) { t.Errorf("Unexpected error: %v", err) } @@ -1773,10 +1730,8 @@ func TestCopyVolume(t *testing.T) { d.azcopy.ExecCmd = m - ctx := context.Background() - var expectedErr error - err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net") + err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net") if !reflect.DeepEqual(err, expectedErr) { t.Errorf("Unexpected error: %v", err) } @@ -1995,7 +1950,7 @@ func TestAuthorizeAzcopyWithIdentity(t *testing.T) { } } -func TestGetSASToken(t *testing.T) { +func TestGetAzcopyAuth(t *testing.T) { testCases := []struct { name string testFunc func(t *testing.T) @@ -2014,14 +1969,14 @@ func TestGetSASToken(t *testing.T) { ctx := context.Background() expectedAccountSASToken := "" expectedErr := fmt.Errorf("could not find accountkey or azurestorageaccountkey field in secrets") - accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace") + accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace") if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) { t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err) } }, }, { - name: "failed to test azcopy list command", + name: "generate sas token using account key", testFunc: func(t *testing.T) { d := NewFakeDriver() d.cloud = &azure.Cloud{ @@ -2033,21 +1988,10 @@ func TestGetSASToken(t *testing.T) { } secrets := map[string]string{ defaultSecretAccountName: "accountName", + defaultSecretAccountKey: "YWNjb3VudGtleQo=", } - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - m := util.NewMockEXEC(ctrl) - listStr := "error" - m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, fmt.Errorf("error")) - - d.azcopy.ExecCmd = m - - ctx := context.Background() - expectedAccountSASToken := "" - expectedErr := fmt.Errorf("azcopy list command failed with error(%v): %v", fmt.Errorf("error"), "error") - accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace") - if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) { + accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace") + if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") { t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err) } }, @@ -2067,19 +2011,10 @@ func TestGetSASToken(t *testing.T) { defaultSecretAccountName: "accountName", defaultSecretAccountKey: "fakeValue", } - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - m := util.NewMockEXEC(ctrl) - listStr := "RESPONSE 403: 403 This request is not authorized to perform this operation using this permission.\nERROR CODE: AuthorizationPermissionMismatch" - m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil) - d.azcopy.ExecCmd = m - - 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.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace") + accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace") if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) { t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err) } @@ -2100,7 +2035,7 @@ func TestGetSASToken(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.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace") + accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace") if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) { t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err) }