Repository: kubernetes-sigs/azurefile-csi-driver
Pull request #399 (2020-09-03 11:27:21)

fix: don't create account if secret is provided
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/release.md#issue-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
6. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:
fix: don't create account if secret is provided
Should not create storage account in bring your own key scenario

**Which issue(s) this PR fixes**:
<!-- 
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Requirements**:
- [ ] uses [conventional commit messages](https://www.conventionalcommits.org/)
  <!-- Common commit types:
        build: Build 🏭
        chore: Maintenance 🔧
        ci: Continuous Integration 💜
        docs: Documentation 📘
        feat: Features 🌈
        fix: Bug Fixes 🐞
        perf: Performance Improvements 🚀
        refactor: Code Refactoring 💎
        revert: Revert Change ◀️
        style: Code Style 🎶
        security: Security Fix 🛡️
        test: Testing 💚 -->
- [ ] includes documentation
- [ ] adds unit tests
- [ ] tested upgrade from previous version

**Special notes for your reviewer**:


**Release note**:
```
fix: don't create account if secret is provided
```



Patch:
diff --git a/pkg/azurefile/azurefile.go b/pkg/azurefile/azurefile.go
index ba1f7f8f5..5cff4da0f 100644
--- a/pkg/azurefile/azurefile.go
+++ b/pkg/azurefile/azurefile.go
@@ -191,41 +191,41 @@ func (d *Driver) Run(endpoint, kubeconfig string, testBool bool) {
 }
 
 // checkFileShareCapacity return nil only if file share does not exist or capacity is the same
-func (d *Driver) checkFileShareCapacity(resourceGroupName, accountName, fileShareName string, requestGiB int, secrets map[string]string) error {
+func (d *Driver) checkFileShareCapacity(resourceGroupName, accountName, fileShareName string, requestGiB int, secrets map[string]string) (string, error) {
 	if len(secrets) > 0 {
 		accountName, accountKey, err := getStorageAccount(secrets)
 		if err != nil {
-			return err
+			return accountName, err
 		}
 		fileClient, err := d.fileClient.getFileSvcClient(accountName, accountKey)
 		if err != nil {
-			return err
+			return accountName, err
 		}
 		share := fileClient.GetShareReference(fileShareName)
 		exists, err := share.Exists()
 		if err != nil {
-			return err
+			return accountName, err
 		}
 		if !exists {
-			return nil
+			return accountName, nil
 		}
 		if share.Properties.Quota != requestGiB {
-			return status.Errorf(codes.AlreadyExists, "the request volume already exists, but its capacity(%v) is different from (%v)", share.Properties.Quota, requestGiB)
+			return accountName, status.Errorf(codes.AlreadyExists, "the request volume already exists, but its capacity(%v) is different from (%v)", share.Properties.Quota, requestGiB)
 		}
-		return nil
+		return accountName, nil
 	}
 
 	exists, fileshare, err := d.checkFileShareExists(accountName, resourceGroupName, fileShareName)
 	if err != nil {
-		return status.Errorf(codes.Internal, "failed to check file share(%s) if exists: %v", fileShareName, err)
+		return accountName, status.Errorf(codes.Internal, "failed to check file share(%s) if exists: %v", fileShareName, err)
 	} else if !exists {
-		return nil
+		return accountName, nil
 	}
 	if fileshare.FileShareProperties.ShareQuota != nil && int(*fileshare.FileShareProperties.ShareQuota) != requestGiB {
-		return status.Errorf(codes.AlreadyExists, "the request volume already exists, but its capacity(%v) is different from (%v)", int(*fileshare.FileShareProperties.ShareQuota), requestGiB)
+		return accountName, status.Errorf(codes.AlreadyExists, "the request volume already exists, but its capacity(%v) is different from (%v)", int(*fileshare.FileShareProperties.ShareQuota), requestGiB)
 	}
 
-	return nil
+	return accountName, nil
 }
 
 func (d *Driver) checkFileShareExists(accountName, resourceGroup, fileShareName string) (bool, storage.FileShare, error) {
diff --git a/pkg/azurefile/controllerserver.go b/pkg/azurefile/controllerserver.go
index 546fc001f..a811812fd 100644
--- a/pkg/azurefile/controllerserver.go
+++ b/pkg/azurefile/controllerserver.go
@@ -162,21 +162,22 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 	}
 
 	var retAccount, retAccountKey string
-	err = wait.ExponentialBackoff(d.cloud.RequestBackoff(), func() (bool, error) {
-		var retErr error
-		retAccount, retAccountKey, retErr = d.cloud.EnsureStorageAccount(accountOptions, fileShareAccountNamePrefix)
-		if isRetriableError(retErr) {
-			klog.Warningf("EnsureStorageAccount(%s) failed with error(%v), waiting for retring", account, retErr)
-			return false, nil
+	if len(req.GetSecrets()) == 0 { // check whether account is provided by secret
+		err = wait.ExponentialBackoff(d.cloud.RequestBackoff(), func() (bool, error) {
+			var retErr error
+			retAccount, retAccountKey, retErr = d.cloud.EnsureStorageAccount(accountOptions, fileShareAccountNamePrefix)
+			if isRetriableError(retErr) {
+				klog.Warningf("EnsureStorageAccount(%s) failed with error(%v), waiting for retring", account, retErr)
+				return false, nil
+			}
+			return true, retErr
+		})
+		if err != nil {
+			return nil, status.Errorf(codes.Internal, "failed to ensure storage account: %v", err)
 		}
-		return true, retErr
-	})
-
-	if err != nil {
-		return nil, status.Errorf(codes.Internal, "failed to ensure storage account: %v", err)
 	}
 
-	if err := d.checkFileShareCapacity(resourceGroup, retAccount, validFileShareName, fileShareSize, req.GetSecrets()); err != nil {
+	if retAccount, err = d.checkFileShareCapacity(resourceGroup, retAccount, validFileShareName, fileShareSize, req.GetSecrets()); err != nil {
 		return nil, err
 	}
 


Test patch:
diff --git a/pkg/azurefile/azurefile_test.go b/pkg/azurefile/azurefile_test.go
index b43433bf0..a5bb26ef1 100644
--- a/pkg/azurefile/azurefile_test.go
+++ b/pkg/azurefile/azurefile_test.go
@@ -673,10 +673,13 @@ func TestCheckFileShareCapacity(t *testing.T) {
 		mockFileClient := mockfileclient.NewMockInterface(ctrl)
 		d.cloud.FileClient = mockFileClient
 		mockFileClient.EXPECT().GetFileShare(gomock.Any(), gomock.Any(), gomock.Any()).Return(test.mockedFileShareResp, test.mockedFileShareErr).AnyTimes()
-		err := d.checkFileShareCapacity(resourceGroupName, accountName, fileShareName, requestGiB, map[string]string{})
+		retAccountName, err := d.checkFileShareCapacity(resourceGroupName, accountName, fileShareName, requestGiB, map[string]string{})
 		if !reflect.DeepEqual(err, test.expectedError) {
 			t.Errorf("Unexpected error: %v", err)
 		}
+		if retAccountName != accountName {
+			t.Errorf("Unexpected return account name: %s, expected: %s", retAccountName, accountName)
+		}
 	}
 }
 
diff --git a/test/e2e/testsuites/dynamically_provisioned_resize_volume_tester.go b/test/e2e/testsuites/dynamically_provisioned_resize_volume_tester.go
index f7a91e600..b6a655dee 100644
--- a/test/e2e/testsuites/dynamically_provisioned_resize_volume_tester.go
+++ b/test/e2e/testsuites/dynamically_provisioned_resize_volume_tester.go
@@ -19,11 +19,8 @@ package testsuites
 import (
 	"context"
 	"fmt"
-	"strconv"
-	"time"
 
 	"github.com/onsi/ginkgo"
-	"github.com/onsi/gomega"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -31,11 +28,8 @@ import (
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/kubernetes/test/e2e/framework"
 
-	"sigs.k8s.io/azurefile-csi-driver/pkg/azurefile"
 	volumehelper "sigs.k8s.io/azurefile-csi-driver/pkg/util"
 	"sigs.k8s.io/azurefile-csi-driver/test/e2e/driver"
-	"sigs.k8s.io/azurefile-csi-driver/test/utils/azure"
-	"sigs.k8s.io/azurefile-csi-driver/test/utils/credentials"
 )
 
 // DynamicallyProvisionedResizeVolumeTest will provision required StorageClass(es), PVC(s) and Pod(s)
@@ -79,9 +73,6 @@ func (t *DynamicallyProvisionedResizeVolumeTest) Run(client clientset.Interface,
 		}
 		updatedSize := updatedPvc.Spec.Resources.Requests["storage"]
 
-		ginkgo.By("sleep 30s waiting for resize complete")
-		time.Sleep(30 * time.Second)
-
 		ginkgo.By("checking the resizing result")
 		newPvc, err := client.CoreV1().PersistentVolumeClaims(namespace.Name).Get(context.TODO(), tpod.pod.Spec.Volumes[0].VolumeSource.PersistentVolumeClaim.ClaimName, metav1.GetOptions{})
 		if err != nil {
@@ -91,43 +82,5 @@ func (t *DynamicallyProvisionedResizeVolumeTest) Run(client clientset.Interface,
 		if !newSize.Equal(updatedSize) {
 			framework.Failf("newSize(%+v) is not equal to updatedSize(%+v)", newSize, updatedSize)
 		}
-
-		ginkgo.By("checking the resizing PV result")
-		newPv, _ := client.CoreV1().PersistentVolumes().Get(context.Background(), updatedPvc.Spec.VolumeName, metav1.GetOptions{})
-		newPvSize := newPv.Spec.Capacity["storage"]
-		newPvSizeStr := newPvSize.String() + "Gi"
-
-		if !(newSize.String() == newPvSizeStr) {
-			framework.Failf("newPVCSize(%+v) is not equal to newPVSize(%+v)", newSize.String(), newPvSizeStr)
-		}
-
-		ginkgo.By("checking the resizing azurefile result")
-
-		creds, err := credentials.CreateAzureCredentialFile(false)
-		gomega.Expect(err).NotTo(gomega.HaveOccurred())
-		azureClient, err := azure.GetAzureClient(creds.Cloud, creds.SubscriptionID, creds.AADClientID, creds.TenantID, creds.AADClientSecret)
-		gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-		var resourceGroup string
-		var accountName string
-		var shareName string
-		if newPv.Spec.PersistentVolumeSource.CSI != nil {
-			resourceGroup, accountName, shareName, _, err = azurefile.GetFileShareInfo(newPv.Spec.PersistentVolumeSource.CSI.VolumeHandle)
-			framework.ExpectNoError(err, fmt.Sprintf("Error getting filesource for azurefile %v", err))
-		} else if newPv.Spec.PersistentVolumeSource.AzureFile != nil {
-			resourceGroup = creds.ResourceGroup
-			accountName = newPv.Spec.PersistentVolumeSource.AzureFile.SecretName
-			shareName = newPv.Spec.PersistentVolumeSource.AzureFile.ShareName
-		}
-
-		//get file information
-		fileshareClient, err := azureClient.GetAzureFilesClient()
-		framework.ExpectNoError(err, fmt.Sprintf("Error getting client for azurefile %v", err))
-		filetest, err := fileshareClient.Get(context.Background(), resourceGroup, accountName, shareName, "")
-		framework.ExpectNoError(err, fmt.Sprintf("Error getting file for azurefile %v", err))
-		newfileSize := strconv.Itoa(int(*filetest.ShareQuota)) + "Gi"
-		if !(newSize.String() == newfileSize) {
-			framework.Failf("newPVCSize(%+v) is not equal to new azurefileSize(%+v)", newSize.String(), newfileSize)
-		}
 	}
 }
diff --git a/test/e2e/testsuites/testsuites.go b/test/e2e/testsuites/testsuites.go
index e204c6c01..7df1d2e05 100644
--- a/test/e2e/testsuites/testsuites.go
+++ b/test/e2e/testsuites/testsuites.go
@@ -302,9 +302,6 @@ func (t *TestPersistentVolumeClaim) DeleteBackingVolume(azfile *azurefile.Driver
 // Related issue: https://github.com/kubernetes/kubernetes/issues/69697
 func (t *TestPersistentVolumeClaim) removeFinalizers() {
 	pv, err := t.client.CoreV1().PersistentVolumes().Get(context.TODO(), t.persistentVolume.Name, metav1.GetOptions{})
-	if err != nil && strings.Contains(err.Error(), "not found") {
-		return
-	}
 	framework.ExpectNoError(err)
 
 	pvClone := pv.DeepCopy()
diff --git a/test/utils/azure/azure_helpers.go b/test/utils/azure/azure_helpers.go
index c80b9b644..cfa67c9fe 100644
--- a/test/utils/azure/azure_helpers.go
+++ b/test/utils/azure/azure_helpers.go
@@ -34,15 +34,14 @@ import (
 )
 
 type Client struct {
-	environment      azure.Environment
-	subscriptionID   string
-	groupsClient     resources.GroupsClient
-	vmClient         compute.VirtualMachinesClient
-	nicClient        network.InterfacesClient
-	subnetsClient    network.SubnetsClient
-	vnetClient       network.VirtualNetworksClient
-	accountsClient   storage.AccountsClient
-	filesharesClient storage.FileSharesClient
+	environment    azure.Environment
+	subscriptionID string
+	groupsClient   resources.GroupsClient
+	vmClient       compute.VirtualMachinesClient
+	nicClient      network.InterfacesClient
+	subnetsClient  network.SubnetsClient
+	vnetClient     network.VirtualNetworksClient
+	accountsClient storage.AccountsClient
 }
 
 func GetAzureClient(cloud, subscriptionID, clientID, tenantID, clientSecret string) (*Client, error) {
@@ -64,11 +63,6 @@ func GetAzureClient(cloud, subscriptionID, clientID, tenantID, clientSecret stri
 	return getClient(env, subscriptionID, tenantID, armSpt), nil
 }
 
-func (az *Client) GetAzureFilesClient() (storage.FileSharesClient, error) {
-
-	return az.filesharesClient, nil
-}
-
 func (az *Client) EnsureResourceGroup(ctx context.Context, name, location string, managedBy *string) (resourceGroup *resources.Group, err error) {
 	var tags map[string]*string
 	group, err := az.groupsClient.Get(ctx, name)
@@ -264,15 +258,14 @@ func getOAuthConfig(env azure.Environment, subscriptionID, tenantID string) (*ad
 
 func getClient(env azure.Environment, subscriptionID, tenantID string, armSpt *adal.ServicePrincipalToken) *Client {
 	c := &Client{
-		environment:      env,
-		subscriptionID:   subscriptionID,
-		groupsClient:     resources.NewGroupsClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
-		vmClient:         compute.NewVirtualMachinesClient(subscriptionID),
-		nicClient:        network.NewInterfacesClient(subscriptionID),
-		subnetsClient:    network.NewSubnetsClient(subscriptionID),
-		vnetClient:       network.NewVirtualNetworksClient(subscriptionID),
-		accountsClient:   storage.NewAccountsClient(subscriptionID),
-		filesharesClient: storage.NewFileSharesClient(subscriptionID),
+		environment:    env,
+		subscriptionID: subscriptionID,
+		groupsClient:   resources.NewGroupsClientWithBaseURI(env.ResourceManagerEndpoint, subscriptionID),
+		vmClient:       compute.NewVirtualMachinesClient(subscriptionID),
+		nicClient:      network.NewInterfacesClient(subscriptionID),
+		subnetsClient:  network.NewSubnetsClient(subscriptionID),
+		vnetClient:     network.NewVirtualNetworksClient(subscriptionID),
+		accountsClient: storage.NewAccountsClient(subscriptionID),
 	}
 
 	authorizer := autorest.NewBearerAuthorizer(armSpt)
@@ -282,7 +275,6 @@ func getClient(env azure.Environment, subscriptionID, tenantID string, armSpt *a
 	c.subnetsClient.Authorizer = authorizer
 	c.vnetClient.Authorizer = authorizer
 	c.accountsClient.Authorizer = authorizer
-	c.filesharesClient.Authorizer = authorizer
 
 	return c
 }