Merge pull request #58857 from brendandburns/ut3

Automatic merge from submit-queue (batch tested with PRs 57322, 57723, 58706, 59004, 58857). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Add more tests for Azure cloud provider.

<eom>

pkg/cloudprovider/providers/azure/BUILD

@@ -69,6 +69,7 @@ go_test(
         "azure_loadbalancer_test.go",
         "azure_metrics_test.go",
         "azure_standard_test.go",
+        "azure_storage_test.go",
         "azure_storageaccount_test.go",
         "azure_test.go",
         "azure_vmss_test.go",

pkg/cloudprovider/providers/azure/azure.go

@@ -120,6 +120,7 @@ type Cloud struct {
 	VirtualMachinesClient   VirtualMachinesClient
 	StorageAccountClient    StorageAccountClient
 	DisksClient             DisksClient
+	FileClient              FileClient
 	resourceRequestBackoff  wait.Backoff
 	vmSet                   VMSet
 
@@ -193,6 +194,7 @@ func NewCloud(configReader io.Reader) (cloudprovider.Interface, error) {
 		PublicIPAddressesClient:         newAzPublicIPAddressesClient(azClientConfig),
 		VirtualMachineScaleSetsClient:   newAzVirtualMachineScaleSetsClient(azClientConfig),
 		VirtualMachineScaleSetVMsClient: newAzVirtualMachineScaleSetVMsClient(azClientConfig),
+		FileClient:                      &azureFileClient{env: *env},
 	}
 
 	// Conditionally configure resource request backoff

pkg/cloudprovider/providers/azure/azure_fakes.go

@@ -927,10 +927,22 @@ func (fRTC *fakeRouteTablesClient) Get(resourceGroupName string, routeTableName
 	}
 }
 
+type fakeFileClient struct {
+}
+
+func (fFC *fakeFileClient) createFileShare(accountName, accountKey, name string, sizeGB int) error {
+	return nil
+}
+
+func (fFC *fakeFileClient) deleteFileShare(accountName, accountKey, name string) error {
+	return nil
+}
+
 type fakeStorageAccountClient struct {
 	mutex     *sync.Mutex
 	FakeStore map[string]map[string]storage.Account
 	Keys      storage.AccountListKeysResult
+	Accounts  storage.AccountListResult
 	Err       error
 }
 
@@ -1005,7 +1017,7 @@ func (fSAC *fakeStorageAccountClient) ListKeys(resourceGroupName string, account
 }
 
 func (fSAC *fakeStorageAccountClient) ListByResourceGroup(resourceGroupName string) (result storage.AccountListResult, err error) {
-	return storage.AccountListResult{}, nil
+	return fSAC.Accounts, fSAC.Err
 }
 
 func (fSAC *fakeStorageAccountClient) GetProperties(resourceGroupName string, accountName string) (result storage.Account, err error) {

pkg/cloudprovider/providers/azure/azure_file.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	azs "github.com/Azure/azure-sdk-for-go/storage"
+	"github.com/Azure/go-autorest/autorest/azure"
 	"github.com/golang/glog"
 )
 
@@ -27,9 +28,28 @@ const (
 	useHTTPS = true
 )
 
+// FileClient is the interface for creating file shares, interface for test
+// injection.
+type FileClient interface {
+	createFileShare(accountName, accountKey, name string, sizeGB int) error
+	deleteFileShare(accountName, accountKey, name string) error
+}
+
 // create file share
 func (az *Cloud) createFileShare(accountName, accountKey, name string, sizeGB int) error {
-	fileClient, err := az.getFileSvcClient(accountName, accountKey)
+	return az.FileClient.createFileShare(accountName, accountKey, name, sizeGB)
+}
+
+func (az *Cloud) deleteFileShare(accountName, accountKey, name string) error {
+	return az.FileClient.deleteFileShare(accountName, accountKey, name)
+}
+
+type azureFileClient struct {
+	env azure.Environment
+}
+
+func (f *azureFileClient) createFileShare(accountName, accountKey, name string, sizeGB int) error {
+	fileClient, err := f.getFileSvcClient(accountName, accountKey)
 	if err != nil {
 		return err
 	}
@@ -53,20 +73,19 @@ func (az *Cloud) createFileShare(accountName, accountKey, name string, sizeGB in
 }
 
 // delete a file share
-func (az *Cloud) deleteFileShare(accountName, accountKey, name string) error {
+func (f *azureFileClient) deleteFileShare(accountName, accountKey, name string) error {
-	fileClient, err := az.getFileSvcClient(accountName, accountKey)
+	fileClient, err := f.getFileSvcClient(accountName, accountKey)
-	if err == nil {
+	if err != nil {
-		share := fileClient.GetShareReference(name)
+		return err
-		return share.Delete(nil)
 	}
-	return nil
+	return fileClient.GetShareReference(name).Delete(nil)
 }
 
-func (az *Cloud) getFileSvcClient(accountName, accountKey string) (*azs.FileServiceClient, error) {
+func (f *azureFileClient) getFileSvcClient(accountName, accountKey string) (*azs.FileServiceClient, error) {
-	client, err := azs.NewClient(accountName, accountKey, az.Environment.StorageEndpointSuffix, azs.DefaultAPIVersion, useHTTPS)
+	fileClient, err := azs.NewClient(accountName, accountKey, f.env.StorageEndpointSuffix, azs.DefaultAPIVersion, useHTTPS)
 	if err != nil {
 		return nil, fmt.Errorf("error creating azure client: %v", err)
 	}
-	f := client.GetFileService()
+	fc := fileClient.GetFileService()
-	return &f, nil
+	return &fc, nil
 }

pkg/cloudprovider/providers/azure/azure_storage.go

@@ -24,51 +24,48 @@ import (
 
 // CreateFileShare creates a file share, using a matching storage account
 func (az *Cloud) CreateFileShare(name, storageAccount, storageType, location string, requestGB int) (string, string, error) {
-	var err error
+	var errResult error
 	accounts := []accountWithLocation{}
 	if len(storageAccount) > 0 {
 		accounts = append(accounts, accountWithLocation{Name: storageAccount})
 	} else {
 		// find a storage account
-		accounts, err = az.getStorageAccounts()
+		accounts, errResult = az.getStorageAccounts()
-		if err != nil {
+		if errResult != nil {
 			// TODO: create a storage account and container
-			return "", "", err
+			return "", "", errResult
 		}
 	}
 	for _, account := range accounts {
 		glog.V(4).Infof("account %s type %s location %s", account.Name, account.StorageType, account.Location)
 		if ((storageType == "" || account.StorageType == storageType) && (location == "" || account.Location == location)) || len(storageAccount) > 0 {
 			// find the access key with this account
-			key, err := az.getStorageAccesskey(account.Name)
+			key, innerErr := az.getStorageAccesskey(account.Name)
-			if err != nil {
+			if innerErr != nil {
-				err = fmt.Errorf("could not get storage key for storage account %s: %v", account.Name, err)
+				errResult = fmt.Errorf("could not get storage key for storage account %s: %v", account.Name, innerErr)
 				continue
 			}
 
-			err = az.createFileShare(account.Name, key, name, requestGB)
+			if innerErr = az.createFileShare(account.Name, key, name, requestGB); innerErr != nil {
-			if err != nil {
+				errResult = fmt.Errorf("failed to create share %s in account %s: %v", name, account.Name, innerErr)
-				err = fmt.Errorf("failed to create share %s in account %s: %v", name, account.Name, err)
 				continue
 			}
 			glog.V(4).Infof("created share %s in account %s", name, account.Name)
-			return account.Name, key, err
+			return account.Name, key, nil
 		}
 	}
 
-	if err == nil {
+	if errResult == nil {
-		err = fmt.Errorf("failed to find a matching storage account")
+		errResult = fmt.Errorf("failed to find a matching storage account")
 	}
-	return "", "", err
+	return "", "", errResult
 }
 
 // DeleteFileShare deletes a file share using storage account name and key
 func (az *Cloud) DeleteFileShare(accountName, key, name string) error {
-	err := az.deleteFileShare(accountName, key, name)
+	if err := az.deleteFileShare(accountName, key, name); err != nil {
-	if err != nil {
 		return err
 	}
 	glog.V(4).Infof("share %s deleted", name)
 	return nil
-
 }

pkg/cloudprovider/providers/azure/azure_storage_test.go

@@ -0,0 +1,135 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package azure
+
+import (
+	"testing"
+
+	"github.com/Azure/azure-sdk-for-go/arm/storage"
+)
+
+func TestCreateFileShare(t *testing.T) {
+	cloud := &Cloud{}
+	fake := newFakeStorageAccountClient()
+	cloud.StorageAccountClient = fake
+	cloud.FileClient = &fakeFileClient{}
+
+	name := "baz"
+	sku := "sku"
+	location := "centralus"
+	value := "foo key"
+	bogus := "bogus"
+
+	tests := []struct {
+		name     string
+		acct     string
+		acctType string
+		loc      string
+		gb       int
+		accounts storage.AccountListResult
+		keys     storage.AccountListKeysResult
+		err      error
+
+		expectErr  bool
+		expectAcct string
+		expectKey  string
+	}{
+		{
+			name:      "foo",
+			acct:      "bar",
+			acctType:  "type",
+			loc:       "eastus",
+			gb:        10,
+			expectErr: true,
+		},
+		{
+			name:      "foo",
+			acct:      "",
+			acctType:  "type",
+			loc:       "eastus",
+			gb:        10,
+			expectErr: true,
+		},
+		{
+			name:     "foo",
+			acct:     "",
+			acctType: sku,
+			loc:      location,
+			gb:       10,
+			accounts: storage.AccountListResult{
+				Value: &[]storage.Account{
+					{Name: &name, Sku: &storage.Sku{Name: storage.SkuName(sku)}, Location: &location},
+				},
+			},
+			keys: storage.AccountListKeysResult{
+				Keys: &[]storage.AccountKey{
+					{Value: &value},
+				},
+			},
+			expectAcct: "baz",
+			expectKey:  "key",
+		},
+		{
+			name:     "foo",
+			acct:     "",
+			acctType: sku,
+			loc:      location,
+			gb:       10,
+			accounts: storage.AccountListResult{
+				Value: &[]storage.Account{
+					{Name: &bogus, Sku: &storage.Sku{Name: storage.SkuName(sku)}, Location: &location},
+				},
+			},
+			expectErr: true,
+		},
+		{
+			name:     "foo",
+			acct:     "",
+			acctType: sku,
+			loc:      location,
+			gb:       10,
+			accounts: storage.AccountListResult{
+				Value: &[]storage.Account{
+					{Name: &name, Sku: &storage.Sku{Name: storage.SkuName(sku)}, Location: &bogus},
+				},
+			},
+			expectErr: true,
+		},
+	}
+
+	for _, test := range tests {
+		fake.Accounts = test.accounts
+		fake.Keys = test.keys
+		fake.Err = test.err
+
+		account, key, err := cloud.CreateFileShare(test.name, test.acct, test.acctType, test.loc, test.gb)
+		if test.expectErr && err == nil {
+			t.Errorf("unexpected non-error")
+			continue
+		}
+		if !test.expectErr && err != nil {
+			t.Errorf("unexpected error: %v", err)
+			continue
+		}
+		if test.expectAcct != account {
+			t.Errorf("Expected: %s, got %s", test.expectAcct, account)
+		}
+		if test.expectKey != key {
+			t.Errorf("Expected: %s, got %s", test.expectKey, key)
+		}
+	}
+}