fix: only generate sas token when secret is used in volume cloning

kubernetes-sigs · Jan 3, 2024 · a3a2f15 · a3a2f15
1 parent c2a1d9b
commit a3a2f15
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 43 deletions.
diff --git a/pkg/blob/controllerserver.go b/pkg/blob/controllerserver.go
@@ -800,6 +800,7 @@ func (d *Driver) copyVolume(ctx context.Context, req *csi.CreateVolumeRequest, a
 	}
 }
 
+// authorizeAzcopyWithIdentity returns auth env for azcopy using cluster identity
 func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
 	azureAuthConfig := d.cloud.Config.AzureAuthConfig
 	var authAzcopyEnv []string
@@ -834,30 +835,39 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
 // 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()
+	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 "", 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 "", 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 {

diff --git a/pkg/blob/controllerserver_test.go b/pkg/blob/controllerserver_test.go
@@ -2015,7 +2015,7 @@ func TestGetSASToken(t *testing.T) {
 			},
 		},
 		{
-			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{
@@ -2029,21 +2029,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.getSASToken(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)
 				}
 			},
@@ -2065,19 +2054,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.getSASToken(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)
 				}