From b23bdf787f4668d6d934781ede0a9240d6912521 Mon Sep 17 00:00:00 2001 From: Brendan Burns Date: Wed, 24 Jan 2018 22:20:45 +0000 Subject: [PATCH] Add more tests. --- pkg/cloudprovider/providers/azure/BUILD | 1 + pkg/cloudprovider/providers/azure/azure.go | 2 + .../providers/azure/azure_fakes.go | 14 +- .../providers/azure/azure_file.go | 41 ++++-- .../providers/azure/azure_storage.go | 31 ++-- .../providers/azure/azure_storage_test.go | 135 ++++++++++++++++++ 6 files changed, 195 insertions(+), 29 deletions(-) create mode 100644 pkg/cloudprovider/providers/azure/azure_storage_test.go diff --git a/pkg/cloudprovider/providers/azure/BUILD b/pkg/cloudprovider/providers/azure/BUILD index 378254f7dcc..ac0f0f86977 100644 --- a/pkg/cloudprovider/providers/azure/BUILD +++ b/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", diff --git a/pkg/cloudprovider/providers/azure/azure.go b/pkg/cloudprovider/providers/azure/azure.go index 9070be894d4..4907e566a98 100644 --- a/pkg/cloudprovider/providers/azure/azure.go +++ b/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 diff --git a/pkg/cloudprovider/providers/azure/azure_fakes.go b/pkg/cloudprovider/providers/azure/azure_fakes.go index 4778cd86201..ac30a533abb 100644 --- a/pkg/cloudprovider/providers/azure/azure_fakes.go +++ b/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) { diff --git a/pkg/cloudprovider/providers/azure/azure_file.go b/pkg/cloudprovider/providers/azure/azure_file.go index 48291128324..39e54383dc4 100644 --- a/pkg/cloudprovider/providers/azure/azure_file.go +++ b/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 { - fileClient, err := az.getFileSvcClient(accountName, accountKey) - if err == nil { - share := fileClient.GetShareReference(name) - return share.Delete(nil) +func (f *azureFileClient) deleteFileShare(accountName, accountKey, name string) error { + fileClient, err := f.getFileSvcClient(accountName, accountKey) + if err != nil { + return err } - return nil + return fileClient.GetShareReference(name).Delete(nil) } -func (az *Cloud) getFileSvcClient(accountName, accountKey string) (*azs.FileServiceClient, error) { - client, err := azs.NewClient(accountName, accountKey, az.Environment.StorageEndpointSuffix, azs.DefaultAPIVersion, useHTTPS) +func (f *azureFileClient) getFileSvcClient(accountName, accountKey string) (*azs.FileServiceClient, error) { + 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() - return &f, nil + fc := fileClient.GetFileService() + return &fc, nil } diff --git a/pkg/cloudprovider/providers/azure/azure_storage.go b/pkg/cloudprovider/providers/azure/azure_storage.go index 641fb8fd6ad..b9dead6fac8 100644 --- a/pkg/cloudprovider/providers/azure/azure_storage.go +++ b/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() - if err != nil { + accounts, errResult = az.getStorageAccounts() + 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) - if err != nil { - err = fmt.Errorf("could not get storage key for storage account %s: %v", account.Name, err) + key, innerErr := az.getStorageAccesskey(account.Name) + if innerErr != nil { + 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 err != nil { - err = fmt.Errorf("failed to create share %s in account %s: %v", name, account.Name, err) + if innerErr = az.createFileShare(account.Name, key, name, requestGB); innerErr != nil { + errResult = fmt.Errorf("failed to create share %s in account %s: %v", name, account.Name, innerErr) 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 { - err = fmt.Errorf("failed to find a matching storage account") + if errResult == nil { + 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 != nil { + if err := az.deleteFileShare(accountName, key, name); err != nil { return err } glog.V(4).Infof("share %s deleted", name) return nil - } diff --git a/pkg/cloudprovider/providers/azure/azure_storage_test.go b/pkg/cloudprovider/providers/azure/azure_storage_test.go new file mode 100644 index 00000000000..6a1298ffb16 --- /dev/null +++ b/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) + } + } +}