Provisioning AccessPolicy and KeyVault at same time

The operator was originally reconciling AccessPolicy's after the rest of the KV
had been created (see: Azure#1158). This isn't actually required because even doing
this there are tons of reasons that this can fail. I've filed Azure#1351 to track
removing ClientID from the CRD in a future API version as there are a ton of
obscure ways that we can fail to translate that ID into an ObjectID.

api/v1alpha1/aso_types.go

@@ -17,7 +17,7 @@ type ASOStatus struct {
 	ResourceId         string       `json:"resourceId,omitempty"`
 	PollingURL         string       `json:"pollingUrl,omitempty"`
 	SpecHash           string       `json:"specHash,omitempty"`
-	ContainsUpdate     bool         `json:"containsUpdate,omitempty"`
+	ContainsUpdate     bool         `json:"containsUpdate,omitempty"` // TODO: Unused, remove in future version
 	RequestedAt        *metav1.Time `json:"requested,omitempty"`
 	CompletedAt        *metav1.Time `json:"completed,omitempty"`
 	FailedProvisioning bool         `json:"failedProvisioning,omitempty"`

api/v1alpha1/keyvault_types.go

@@ -35,9 +35,9 @@ type AccessPolicyEntry struct {
 	// TenantID - The Azure Active Directory tenant ID that should be used for authenticating requests to the key vault.
 	TenantID string `json:"tenantID,omitempty"`
 	// ClientID - The client ID of a user, service principal or security group in the Azure Active Directory tenant for the vault. The client ID must be unique for the list of access policies.
+	// TODO: Remove this in a future API version, see: https://github.com/Azure/azure-service-operator/issues/1351
 	ClientID string `json:"clientID,omitempty"`
-	// ObjectID is the value to use if the access policy is for a user other than the user creating the Key Vault when the creating user does not have access to the Application API which is used to translate ClientID to Object ID
-	// To get around this, use az-cli or the Azure portal to source the ObjectID from your Service Principal
+	// ObjectID is the AAD object id of the entity to provide access to.
 	ObjectID string `json:"objectID,omitempty"`
 	// ApplicationID -  Application ID of the client making request on behalf of a principal
 	ApplicationID string `json:"applicationID,omitempty"`

api/v1alpha2/aso_types.go

@@ -17,7 +17,7 @@ type ASOStatus struct {
 	ResourceId         string       `json:"resourceId,omitempty"`
 	PollingURL         string       `json:"pollingUrl,omitempty"`
 	SpecHash           string       `json:"specHash,omitempty"`
-	ContainsUpdate     bool         `json:"containsUpdate,omitempty"`
+	ContainsUpdate     bool         `json:"containsUpdate,omitempty"` // TODO: Unused, remove in future version
 	RequestedAt        *metav1.Time `json:"requested,omitempty"`
 	CompletedAt        *metav1.Time `json:"completed,omitempty"`
 	FailedProvisioning bool         `json:"failedProvisioning,omitempty"`

api/v1beta1/aso_types.go

@@ -17,7 +17,7 @@ type ASOStatus struct {
 	ResourceId         string       `json:"resourceId,omitempty"`
 	PollingURL         string       `json:"pollingUrl,omitempty"`
 	SpecHash           string       `json:"specHash,omitempty"`
-	ContainsUpdate     bool         `json:"containsUpdate,omitempty"`
+	ContainsUpdate     bool         `json:"containsUpdate,omitempty"` // TODO: Unused, remove in future version
 	RequestedAt        *metav1.Time `json:"requested,omitempty"`
 	CompletedAt        *metav1.Time `json:"completed,omitempty"`
 	FailedProvisioning bool         `json:"failedProvisioning,omitempty"`

config/samples/azure_v1alpha1_keyvault.yaml

@@ -23,13 +23,11 @@ spec:
       - /subscriptions/<subid>/resourceGroups/rg1/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/subnet2
   accessPolicies:
     - tenantID: <tenantID>
-      clientID: <clientID>
-      # applicationID and objectID are optional replacements for clientID
-      # Use clientID when the access you are providing is for the same Service Principal who created the Key Vault. An access policy actually needs the ObjectID but with proper permissions we can translate clientID to ObjectID.
+      objectID: <objectID>
       # Use applicationID when providing access to a managed identity
-      # Use objectID when the service principal who needs access is not the same as the one the operator used to create the Key Vault. The permissions a service principal needs to translate clientID to objectID for other SPs is astronomical.
       # applicationID: <appID>
-      # objectID: <application registration's object id>
+      # We strongly recommend that you do not use the clientID field, it will be removed in a future version of the API
+
       permissions:
         keys: # backup create decrypt delete encrypt get import list purge recover restore sign unwrapKey update verify wrapKey
           - list

controllers/keyvault_controller_test.go

@@ -8,9 +8,12 @@ package controllers
 import (
 	"context"
 	"net/http"
+	"strings"
 	"testing"
 	"time"
 
+	uuid "github.com/satori/go.uuid"
+
 	azurev1alpha1 "github.com/Azure/azure-service-operator/api/v1alpha1"
 	"github.com/Azure/azure-service-operator/pkg/errhelp"
 	"github.com/Azure/azure-service-operator/pkg/helpers"
@@ -375,3 +378,51 @@ func TestKeyvaultControllerWithVirtualNetworkRulesAndUpdate(t *testing.T) {
 		return result.Response.StatusCode == http.StatusNotFound
 	}, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from azure")
 }
+
+func TestKeyvaultControllerBadAccessPolicy(t *testing.T) {
+	t.Parallel()
+	defer PanicRecover(t)
+	ctx := context.Background()
+
+	keyVaultName := GenerateTestResourceNameWithRandom("kv", 6)
+	keyVaultLocation := tc.resourceGroupLocation
+
+	// Declare KeyVault object
+	accessPolicies := []azurev1alpha1.AccessPolicyEntry{
+		{
+			TenantID: config.GlobalCredentials().TenantID(),
+			ClientID: uuid.Nil.String(),
+
+			Permissions: &azurev1alpha1.Permissions{
+				Keys: &[]string{
+					"get",
+					"list",
+				},
+			},
+		}}
+
+	keyVaultInstance := &azurev1alpha1.KeyVault{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      keyVaultName,
+			Namespace: "default",
+		},
+		Spec: azurev1alpha1.KeyVaultSpec{
+			Location:      keyVaultLocation,
+			ResourceGroup: tc.resourceGroupName,
+			AccessPolicies: &accessPolicies,
+		},
+	}
+
+	assert := assert.New(t)
+
+	err := tc.k8sClient.Create(ctx, keyVaultInstance)
+	assert.NoError(err)
+	namespacedName := types.NamespacedName{Name: keyVaultInstance.GetName(), Namespace: keyVaultInstance.GetNamespace()}
+
+	assert.Eventually(func() bool {
+		_ = tc.k8sClient.Get(ctx, namespacedName, keyVaultInstance)
+		return strings.Contains(keyVaultInstance.Status.Message, "Authorization_RequestDenied")
+	}, tc.timeout, tc.retry, "wait for message to be updated indicating auth error")
+
+	EnsureDelete(ctx, t, tc, keyVaultInstance)
+}

pkg/resourcemanager/keyvaults/keyvault.go

@@ -22,6 +22,7 @@ import (
 	"github.com/Azure/azure-service-operator/pkg/resourcemanager/iam"
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/to"
+	"github.com/pkg/errors"
 	uuid "github.com/satori/go.uuid"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -218,55 +219,27 @@ func ParseAccessPolicy(ctx context.Context, creds config.Credentials, policy *v1
 			return keyvault.AccessPolicyEntry{}, err
 		}
 		newEntry.ObjectID = objID
-
 	} else if policy.ObjectID != "" {
 		newEntry.ObjectID = &policy.ObjectID
 	}
 
 	return newEntry, nil
 }
 
-// InstantiateVault will instantiate VaultsClient
-func InstantiateVault(ctx context.Context, creds config.Credentials, vaultName string, containsUpdate bool) (keyvault.VaultsClient, uuid.UUID, error) {
-	vaultsClient, err := getVaultsClient(creds)
-	if err != nil {
-		return keyvault.VaultsClient{}, uuid.UUID{}, err
-	}
-	id, err := uuid.FromString(creds.TenantID())
-	if err != nil {
-		return keyvault.VaultsClient{}, uuid.UUID{}, err
-	}
-
-	// Check if keyvault name is valid
-	if !containsUpdate {
-		vaultNameCheck := keyvault.VaultCheckNameAvailabilityParameters{
-			Name: to.StringPtr(vaultName),
-			Type: to.StringPtr("Microsoft.KeyVault/vaults"),
-		}
-		result, err := vaultsClient.CheckNameAvailability(ctx, vaultNameCheck)
-		if err != nil {
-			return keyvault.VaultsClient{}, uuid.UUID{}, err
-		}
-		if result.Reason == keyvault.Reason("Invalid") || result.Reason == keyvault.AccountNameInvalid {
-			return keyvault.VaultsClient{}, uuid.UUID{}, fmt.Errorf("AccountNameInvalid")
-		} else if result.Reason == keyvault.AlreadyExists {
-			return keyvault.VaultsClient{}, uuid.UUID{}, fmt.Errorf("AlreadyExists")
-		}
-	}
-
-	return vaultsClient, id, nil
-}
-
 // CreateVault creates a new key vault
-func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string, vaultExists bool) (keyvault.Vault, error) {
+func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string) (keyvault.Vault, error) {
 	vaultName := instance.Name
 	location := instance.Spec.Location
 	groupName := instance.Spec.ResourceGroup
 	enableSoftDelete := instance.Spec.EnableSoftDelete
 
-	vaultsClient, id, err := InstantiateVault(ctx, m.Creds, vaultName, instance.Status.ContainsUpdate)
+	vaultsClient, err := getVaultsClient(m.Creds)
 	if err != nil {
-		return keyvault.Vault{}, err
+		return keyvault.Vault{}, errors.Wrapf(err, "couldn't get vaults client")
+	}
+	id, err := uuid.FromString(m.Creds.TenantID())
+	if err != nil {
+		return keyvault.Vault{}, errors.Wrapf(err, "couldn't convert tenantID to UUID")
 	}
 
 	var accessPolicies []keyvault.AccessPolicyEntry
@@ -279,8 +252,6 @@ func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph
 			}
 			accessPolicies = append(accessPolicies, newEntry)
 		}
-	} else {
-		accessPolicies = []keyvault.AccessPolicyEntry{}
 	}
 
 	var networkAcls keyvault.NetworkRuleSet
@@ -299,11 +270,10 @@ func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph
 		keyVaultSku.Name = keyvault.Premium
 	}
 
-	pols := []keyvault.AccessPolicyEntry{}
 	params := keyvault.VaultCreateOrUpdateParameters{
 		Properties: &keyvault.VaultProperties{
 			TenantID:         &id,
-			AccessPolicies:   &pols,
+			AccessPolicies:   &accessPolicies,
 			Sku:              &keyVaultSku,
 			NetworkAcls:      &networkAcls,
 			EnableSoftDelete: &enableSoftDelete,
@@ -312,10 +282,6 @@ func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph
 		Tags:     tags,
 	}
 
-	if vaultExists {
-		params.Properties.AccessPolicies = &accessPolicies
-	}
-
 	future, err := vaultsClient.CreateOrUpdate(ctx, groupName, vaultName, params)
 	if err != nil {
 		return keyvault.Vault{}, err
@@ -325,10 +291,15 @@ func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph
 }
 
 //CreateVaultWithAccessPolicies creates a new key vault and provides access policies to the specified user
+// TODO: Nuke all of this because its only for the tests
 func (m *azureKeyVaultManager) CreateVaultWithAccessPolicies(ctx context.Context, groupName string, vaultName string, location string, clientID string) (keyvault.Vault, error) {
-	vaultsClient, id, err := InstantiateVault(ctx, m.Creds, vaultName, false)
+	vaultsClient, err := getVaultsClient(m.Creds)
 	if err != nil {
-		return keyvault.Vault{}, err
+		return keyvault.Vault{}, errors.Wrapf(err, "couldn't get vaults client")
+	}
+	id, err := uuid.FromString(m.Creds.TenantID())
+	if err != nil {
+		return keyvault.Vault{}, errors.Wrapf(err, "couldn't convert tenantID to UUID")
 	}
 
 	apList := []keyvault.AccessPolicyEntry{}
@@ -409,32 +380,26 @@ func (m *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object, o
 	// convert kube labels to expected tag format
 	labels := helpers.LabelsToTags(instance.GetLabels())
 
-	instance.Status.Provisioning = true
-	instance.Status.FailedProvisioning = false
+	instance.Status.SetProvisioning("")
 	exists := false
 	// Check if this KeyVault already exists and its state if it does.
 	keyvault, err := m.GetVault(ctx, instance.Spec.ResourceGroup, instance.Name)
 	if err == nil {
 		exists = true
 		if instance.Status.SpecHash == hash {
-			instance.Status.Message = resourcemanager.SuccessMsg
-			instance.Status.Provisioned = true
-			instance.Status.Provisioning = false
+			instance.Status.SetProvisioned(resourcemanager.SuccessMsg)
 			instance.Status.ResourceId = *keyvault.ID
 			return true, nil
 		}
 
 		instance.Status.SpecHash = hash
-		instance.Status.ContainsUpdate = true
-
 	}
 
 	keyvault, err = m.CreateVault(
 		ctx,
 		instance,
 		instance.Spec.Sku,
 		labels,
-		exists,
 	)
 	if err != nil {
 		done, err := HandleCreationError(instance, err)
@@ -450,18 +415,14 @@ func (m *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object, o
 	if keyvault.ID != nil {
 		instance.Status.ResourceId = *keyvault.ID
 	}
-	instance.Status.ContainsUpdate = false
-	instance.Status.Provisioned = true
-	instance.Status.Provisioning = false
-	instance.Status.Message = resourcemanager.SuccessMsg
+	instance.Status.SetProvisioned(resourcemanager.SuccessMsg)
 
 	return true, nil
 }
 
 func HandleCreationError(instance *v1alpha1.KeyVault, err error) (bool, error) {
 	// let the user know what happened
 	instance.Status.Message = errhelp.StripErrorTimes(errhelp.StripErrorIDs(err))
-	instance.Status.Provisioning = false
 	// errors we expect might happen that we are ok with waiting for
 	catch := []string{
 		errhelp.ResourceGroupNotFoundErrorCode,
@@ -480,11 +441,6 @@ func HandleCreationError(instance *v1alpha1.KeyVault, err error) (bool, error) {
 
 	azerr := errhelp.NewAzureError(err)
 	if helpers.ContainsString(catch, azerr.Type) {
-		// most of these error technically mean the resource is actually not provisioning
-		switch azerr.Type {
-		case errhelp.AsyncOpIncompleteError:
-			instance.Status.Provisioning = true
-		}
 		// reconciliation is not done but error is acceptable
 		return false, nil
 	}
@@ -495,7 +451,6 @@ func HandleCreationError(instance *v1alpha1.KeyVault, err error) (bool, error) {
 		case errhelp.AlreadyExists:
 			timeNow := metav1.NewTime(time.Now())
 			if timeNow.Sub(instance.Status.RequestedAt.Time) < (30 * time.Second) {
-				instance.Status.Provisioning = true
 				return false, nil
 			}
 

pkg/resourcemanager/keyvaults/keyvault_manager.go

@@ -14,7 +14,7 @@ import (
 )
 
 type KeyVaultManager interface {
-	CreateVault(ctx context.Context, instance *azurev1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string, exists bool) (keyvault.Vault, error)
+	CreateVault(ctx context.Context, instance *azurev1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string) (keyvault.Vault, error)
 
 	// CreateVault and grant access to the specific user ID
 	CreateVaultWithAccessPolicies(ctx context.Context, groupName string, vaultName string, location string, userID string) (keyvault.Vault, error)

pkg/secrets/keyvault/client.go

@@ -5,37 +5,25 @@ package keyvault
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"strings"
 	"time"
 
-	"encoding/json"
-
-	mgmtclient "github.com/Azure/azure-sdk-for-go/services/keyvault/mgmt/2018-02-14/keyvault"
 	"github.com/Azure/azure-sdk-for-go/services/keyvault/v7.0/keyvault"
 	keyvaults "github.com/Azure/azure-sdk-for-go/services/keyvault/v7.0/keyvault"
+	"github.com/Azure/go-autorest/autorest/date"
+	"github.com/Azure/go-autorest/autorest/to"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+
 	"github.com/Azure/azure-service-operator/api/v1alpha1"
 	"github.com/Azure/azure-service-operator/pkg/errhelp"
 	"github.com/Azure/azure-service-operator/pkg/resourcemanager/config"
 	"github.com/Azure/azure-service-operator/pkg/resourcemanager/iam"
 	"github.com/Azure/azure-service-operator/pkg/secrets"
-	"github.com/Azure/go-autorest/autorest/date"
-	"github.com/Azure/go-autorest/autorest/to"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
 )
 
-func getVaultsClient(creds config.Credentials) (mgmtclient.VaultsClient, error) {
-	vaultsClient := mgmtclient.NewVaultsClient(creds.SubscriptionID())
-	a, err := iam.GetResourceManagementAuthorizer(creds)
-	if err != nil {
-		return vaultsClient, err
-	}
-	vaultsClient.Authorizer = a
-	vaultsClient.AddToUserAgent(config.UserAgent())
-	return vaultsClient, nil
-}
-
 // KeyvaultSecretClient struct has the Key vault BaseClient that Azure uses and the KeyVault name
 type KeyvaultSecretClient struct {
 	KeyVaultClient keyvaults.BaseClient

pkg/secrets/keyvault/client_test.go

@@ -70,7 +70,7 @@ var _ = Describe("Keyvault Secrets Client", func() {
 
 		// Create a keyvault
 		_, err = kvManager.CreateVaultWithAccessPolicies(ctx, resourcegroupName, keyVaultName, resourcegroupLocation, userID)
-		//Expect(err).NotTo(HaveOccurred())
+		Expect(err).NotTo(HaveOccurred())
 
 	})