Incorporate code review comments

Thanks @matthchr!

controllers/suite_test.go

@@ -884,7 +884,13 @@ func setup() error {
 
 	log.Println("Creating KV:", keyvaultName)
 	kvManager := resourcemanagerkeyvaults.NewAzureKeyVaultManager(config.GlobalCredentials(), nil)
-	_, err = kvManager.CreateVaultWithAccessPolicies(context.Background(), resourceGroupName, keyvaultName, resourcegroupLocation, resourcemanagerconfig.GlobalCredentials().ClientID())
+	_, err = kvManager.CreateVaultWithAccessPolicies(
+		context.Background(),
+		resourceGroupName,
+		keyvaultName,
+		resourcegroupLocation,
+		resourcemanagerconfig.GlobalCredentials().ClientID(),
+	)
 	// Key Vault needs to be in "Suceeded" state
 	finish := time.Now().Add(tc.timeout)
 	for {

main.go

@@ -140,8 +140,8 @@ func main() {
 	}
 
 	// TODO(creds-refactor): construction of these managers will need
-	// to move into the reconcilers so that they can use the correct
-	// creds for the specific resource being reconciled.
+	// to move into the AsyncReconciler.Reconcile so that it can use the correct
+	// creds based on the namespace of the specific resource being reconciled.
 	apimManager := resourceapimanagement.NewManager(config.GlobalCredentials())
 	apimServiceManager := apimservice.NewAzureAPIMgmtServiceManager(config.GlobalCredentials())
 	vnetManager := vnet.NewAzureVNetManager(config.GlobalCredentials())

pkg/resourcemanager/config/config.go

@@ -32,7 +32,7 @@ var (
 	testResourcePrefix string // used to generate resource names in tests, should probably exist in a test only package
 )
 
-// GlobalCredentials() returns the configured credentials.
+// GlobalCredentials returns the configured credentials.
 // TODO: get rid of all uses of this.
 func GlobalCredentials() Credentials {
 	return creds

pkg/resourcemanager/iam/authorizers.go

@@ -261,17 +261,3 @@ func GetMSITokenForResource(resource string) (*adal.ServicePrincipalToken, error
 
 	return token, err
 }
-
-// GetResourceManagementTokenHybrid retrieves auth token for hybrid environment
-// TODO(creds-refactor): this seems to be unused, remove it.
-func GetResourceManagementTokenHybrid(creds config.Credentials, activeDirectoryEndpoint, tokenAudience string) (adal.OAuthTokenProvider, error) {
-	var tokenProvider adal.OAuthTokenProvider
-	oauthConfig, err := adal.NewOAuthConfig(activeDirectoryEndpoint, creds.TenantID())
-	tokenProvider, err = adal.NewServicePrincipalToken(
-		*oauthConfig,
-		creds.ClientID(),
-		creds.ClientSecret(),
-		tokenAudience)
-
-	return tokenProvider, err
-}

pkg/resourcemanager/keyvaults/keyvault.go

@@ -51,9 +51,6 @@ func getVaultsClient(creds config.Credentials) (keyvault.VaultsClient, error) {
 	return vaultsClient, nil
 }
 
-// TODO(creds-refactor): can we just use the tenant and client id from
-// the creds here? Depends whether the client code uses them to
-// override the creds.
 func getObjectID(ctx context.Context, creds config.Credentials, tenantID string, clientID string) (*string, error) {
 	appclient := auth.NewApplicationsClient(tenantID)
 	a, err := iam.GetGraphAuthorizer(creds)
@@ -119,7 +116,7 @@ func ParseNetworkPolicy(ruleSet *v1alpha1.NetworkRuleSet) keyvault.NetworkRuleSe
 }
 
 // ParseAccessPolicy - helper function to parse access policies from Kubernetes spec
-func ParseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context, creds config.Credentials) (keyvault.AccessPolicyEntry, error) {
+func ParseAccessPolicy(ctx context.Context, creds config.Credentials, policy *v1alpha1.AccessPolicyEntry) (keyvault.AccessPolicyEntry, error) {
 	tenantID, err := uuid.FromString(policy.TenantID)
 	if err != nil {
 		return keyvault.AccessPolicyEntry{}, err
@@ -276,7 +273,7 @@ func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph
 	if instance.Spec.AccessPolicies != nil {
 		for _, policy := range *instance.Spec.AccessPolicies {
 			policy := policy // Make a copy of the variable and redeclare it
-			newEntry, err := ParseAccessPolicy(&policy, ctx, m.Creds)
+			newEntry, err := ParseAccessPolicy(ctx, m.Creds, &policy)
 			if err != nil {
 				return keyvault.Vault{}, err
 			}

pkg/resourcemanager/keyvaults/keyvault_manager.go

@@ -28,3 +28,5 @@ type KeyVaultManager interface {
 	// also embed async client methods
 	resourcemanager.ARMClient
 }
+
+var _ KeyVaultManager = &azureKeyVaultManager{}

pkg/resourcemanager/keyvaults/unittest/keyvault_test.go

@@ -32,7 +32,7 @@ func TestParseAccessPoliciesInvalid(t *testing.T) {
 
 	ctx := context.Background()
 
-	resp, err := azurekeyvault.ParseAccessPolicy(&entry, ctx, config.GlobalCredentials())
+	resp, err := azurekeyvault.ParseAccessPolicy(ctx, config.GlobalCredentials(), &entry)
 	assert.True(t, err != nil)
 	assert.True(t, cmp.Equal(resp, keyvault.AccessPolicyEntry{}))
 }
@@ -85,7 +85,7 @@ func TestParseAccessPolicies(t *testing.T) {
 
 	ctx := context.Background()
 
-	resp, err := azurekeyvault.ParseAccessPolicy(&entry, ctx, config.GlobalCredentials())
+	resp, err := azurekeyvault.ParseAccessPolicy(ctx, config.GlobalCredentials(), &entry)
 	assert.True(t, err == nil)
 	assert.True(t, cmp.Equal(resp, out))
 }

pkg/resourcemanager/storages/storageaccount/storageaccount.go

@@ -81,15 +81,15 @@ func ParseNetworkPolicy(ruleSet *v1alpha1.StorageNetworkRuleSet) storage.Network
 	}
 }
 
-func getStoragesClient(creds config.Credentials) (storage.AccountsClient, error) {
-	storagesClient := storage.NewAccountsClientWithBaseURI(config.BaseURI(), creds.SubscriptionID())
+func getStorageClient(creds config.Credentials) (storage.AccountsClient, error) {
+	storageClient := storage.NewAccountsClientWithBaseURI(config.BaseURI(), creds.SubscriptionID())
 	a, err := iam.GetResourceManagementAuthorizer(creds)
 	if err != nil {
 		return storage.AccountsClient{}, err
 	}
-	storagesClient.Authorizer = a
-	storagesClient.AddToUserAgent(config.UserAgent())
-	return storagesClient, nil
+	storageClient.Authorizer = a
+	storageClient.AddToUserAgent(config.UserAgent())
+	return storageClient, nil
 }
 
 // CreateStorage creates a new storage account
@@ -103,15 +103,15 @@ func (m *azureStorageManager) CreateStorage(ctx context.Context,
 	accessTier azurev1alpha1.StorageAccountAccessTier,
 	enableHTTPsTrafficOnly *bool, dataLakeEnabled *bool, networkRule *storage.NetworkRuleSet) (pollingURL string, result storage.Account, err error) {
 
-	storagesClient, err := getStoragesClient(m.Creds)
+	storageClient, err := getStorageClient(m.Creds)
 	if err != nil {
 		return "", storage.Account{}, err
 	}
 
 	//Check if name is available
 	storageType := "Microsoft.Storage/storageAccounts"
 	checkAccountParams := storage.AccountCheckNameAvailabilityParameters{Name: &storageAccountName, Type: &storageType}
-	checkNameResult, err := storagesClient.CheckNameAvailability(ctx, checkAccountParams)
+	checkNameResult, err := storageClient.CheckNameAvailability(ctx, checkAccountParams)
 	if err != nil {
 		return "", result, err
 	}
@@ -148,12 +148,12 @@ func (m *azureStorageManager) CreateStorage(ctx context.Context,
 	}
 
 	//log.Println(fmt.Sprintf("creating storage '%s' in resource group '%s' and location: %v", storageAccountName, groupName, location))
-	future, err := storagesClient.Create(ctx, groupName, storageAccountName, params)
+	future, err := storageClient.Create(ctx, groupName, storageAccountName, params)
 	if err != nil {
 		return "", result, err
 	}
 
-	result, err = future.Result(storagesClient)
+	result, err = future.Result(storageClient)
 
 	return future.PollingURL(), result, err
 
@@ -164,17 +164,17 @@ func (m *azureStorageManager) CreateStorage(ctx context.Context,
 // resourceGroupName - name of the resource group within the azure subscription.
 // storageAccountName - the name of the storage account
 func (m *azureStorageManager) GetStorage(ctx context.Context, resourceGroupName string, storageAccountName string) (result storage.Account, err error) {
-	storagesClient, err := getStoragesClient(m.Creds)
+	storageClient, err := getStorageClient(m.Creds)
 	if err != nil {
 		return storage.Account{}, err
 	}
 
-	return storagesClient.GetProperties(ctx, resourceGroupName, storageAccountName, "")
+	return storageClient.GetProperties(ctx, resourceGroupName, storageAccountName, "")
 }
 
 // DeleteStorage removes the resource group named by env var
 func (m *azureStorageManager) DeleteStorage(ctx context.Context, groupName string, storageAccountName string) (result autorest.Response, err error) {
-	storagesClient, err := getStoragesClient(m.Creds)
+	storageClient, err := getStorageClient(m.Creds)
 	if err != nil {
 		return autorest.Response{
 			Response: &http.Response{
@@ -183,16 +183,16 @@ func (m *azureStorageManager) DeleteStorage(ctx context.Context, groupName strin
 		}, err
 	}
 
-	return storagesClient.Delete(ctx, groupName, storageAccountName)
+	return storageClient.Delete(ctx, groupName, storageAccountName)
 }
 
 func (m *azureStorageManager) ListKeys(ctx context.Context, resourceGroupName string, accountName string) (result storage.AccountListKeysResult, err error) {
-	storagesClient, err := getStoragesClient(m.Creds)
+	storageClient, err := getStorageClient(m.Creds)
 	if err != nil {
 		return storage.AccountListKeysResult{}, err
 	}
 
-	return storagesClient.ListKeys(ctx, resourceGroupName, accountName, storage.Kerb)
+	return storageClient.ListKeys(ctx, resourceGroupName, accountName, storage.Kerb)
 }
 
 // StoreSecrets upserts the secret information for this storage account