Keyvault: Add Get/Delete functionality for flattened keys & implement…

… tests

Makefile

@@ -71,7 +71,9 @@ test-existing-managers: generate fmt vet manifests
 	./pkg/resourcemanager/psql/firewallrule/... \
 	./pkg/resourcemanager/appinsights/... \
 	./pkg/resourcemanager/vnet/... \
-	./pkg/resourcemanager/apim/apimgmt...
+	./pkg/resourcemanager/apim/apimgmt... \
+	./pkg/secrets/...
+
 
 # Cleanup resource groups azure created by tests using pattern matching 't-rg-'
 test-cleanup-azure-resources: 	

controllers/azuresql_combined_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	helpers "github.com/Azure/azure-service-operator/pkg/helpers"
+	kvsecrets "github.com/Azure/azure-service-operator/pkg/secrets/keyvault"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -148,6 +149,8 @@ func TestAzureSqlServerCombinedHappyPath(t *testing.T) {
 	})
 
 	var sqlUser *azurev1alpha1.AzureSQLUser
+	var kvSqlUser1 *azurev1alpha1.AzureSQLUser
+	var kvSqlUser2 *azurev1alpha1.AzureSQLUser
 
 	// run sub tests that require 2 servers or have to be run after rollcreds test ------------------
 	t.Run("group2", func(t *testing.T) {
@@ -158,23 +161,119 @@ func TestAzureSqlServerCombinedHappyPath(t *testing.T) {
 			// create a sql user and verify it provisions
 			username := "sql-test-user" + helpers.RandomString(10)
 			roles := []string{"db_owner"}
+			keyVaultSecretFormats := []string{"adonet"}
 
 			sqlUser = &azurev1alpha1.AzureSQLUser{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      username,
 					Namespace: "default",
 				},
 				Spec: azurev1alpha1.AzureSQLUserSpec{
-					Server:        sqlServerName,
-					DbName:        sqlDatabaseName,
-					ResourceGroup: rgName,
-					Roles:         roles,
+					Server:                sqlServerName,
+					DbName:                sqlDatabaseName,
+					ResourceGroup:         rgName,
+					Roles:                 roles,
+					KeyVaultSecretFormats: keyVaultSecretFormats,
 				},
 			}
 
 			EnsureInstance(ctx, t, tc, sqlUser)
+
+			// verify user's secret has been created
+			// this test suite defaults to Kube Secrets. They do not support keyvault-specific config but the spec is passed anyway
+			// to verify that passing them does not break the service
+			assert.Eventually(func() bool {
+				key := types.NamespacedName{Name: sqlUser.ObjectMeta.Name, Namespace: sqlUser.ObjectMeta.Namespace}
+				var secrets, _ = tc.secretClient.Get(ctx, key)
+
+				return strings.Contains(string(secrets["azureSqlDatabaseName"]), sqlDatabaseName)
+			}, tc.timeoutFast, tc.retry, "wait for secret store to show azure sql user credentials")
+
 			t.Log(sqlUser.Status)
 		})
+
+		t.Run("set up user in first db with custom keyvault", func(t *testing.T) {
+			t.Parallel()
+
+			// create a sql user and verify it provisions
+			username := "sql-test-user" + helpers.RandomString(10)
+			roles := []string{"db_owner"}
+
+			// This test will attempt to persist secrets to the KV that was instantiated as part of the test suite
+			keyVaultName := tc.keyvaultName
+
+			kvSqlUser1 = &azurev1alpha1.AzureSQLUser{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      username,
+					Namespace: "default",
+				},
+				Spec: azurev1alpha1.AzureSQLUserSpec{
+					Server:                 sqlServerName,
+					DbName:                 sqlDatabaseName,
+					ResourceGroup:          rgName,
+					Roles:                  roles,
+					KeyVaultToStoreSecrets: keyVaultName,
+				},
+			}
+
+			EnsureInstance(ctx, t, tc, kvSqlUser1)
+
+			// Check that the user's secret is in the keyvault
+			keyVaultSecretClient := kvsecrets.New(keyVaultName)
+
+			assert.Eventually(func() bool {
+				keyNamespace := "azuresqluser-" + sqlServerName + "-" + sqlDatabaseName
+				key := types.NamespacedName{Name: kvSqlUser1.ObjectMeta.Name, Namespace: keyNamespace}
+				var secrets, _ = keyVaultSecretClient.Get(ctx, key)
+
+				return strings.Contains(string(secrets["azureSqlDatabaseName"]), sqlDatabaseName)
+			}, tc.timeoutFast, tc.retry, "wait for keyvault to show azure sql user credentials")
+
+			t.Log(kvSqlUser1.Status)
+		})
+
+		t.Run("set up user in first db with custom keyvault and custom formatting", func(t *testing.T) {
+			t.Parallel()
+
+			// create a sql user and verify it provisions
+			username := "sql-test-user" + helpers.RandomString(10)
+			roles := []string{"db_owner"}
+			formats := []string{"adonet"}
+
+			// This test will attempt to persist secrets to the KV that was instantiated as part of the test suite
+			keyVaultName := tc.keyvaultName
+
+			kvSqlUser2 = &azurev1alpha1.AzureSQLUser{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      username,
+					Namespace: "default",
+				},
+				Spec: azurev1alpha1.AzureSQLUserSpec{
+					Server:                 sqlServerName,
+					DbName:                 sqlDatabaseName,
+					ResourceGroup:          rgName,
+					Roles:                  roles,
+					KeyVaultToStoreSecrets: keyVaultName,
+					KeyVaultSecretFormats:  formats,
+				},
+			}
+
+			EnsureInstance(ctx, t, tc, kvSqlUser2)
+
+			// Check that the user's secret is in the keyvault
+			keyVaultSecretClient := kvsecrets.New(keyVaultName)
+
+			assert.Eventually(func() bool {
+				keyNamespace := "azuresqluser-" + sqlServerName + "-" + sqlDatabaseName
+				keyName := kvSqlUser2.ObjectMeta.Name + "-adonet"
+				key := types.NamespacedName{Name: keyName, Namespace: keyNamespace}
+				var secrets, _ = keyVaultSecretClient.Get(ctx, key)
+
+				return len(string(secrets[keyNamespace+"-"+keyName])) > 0
+			}, tc.timeoutFast, tc.retry, "wait for keyvault to show azure sql user credentials with custom formats")
+
+			t.Log(kvSqlUser2.Status)
+		})
 	})
 
 	var sqlFailoverGroupInstance *azurev1alpha1.AzureSqlFailoverGroup
@@ -183,8 +282,40 @@ func TestAzureSqlServerCombinedHappyPath(t *testing.T) {
 	sqlFailoverGroupNamespacedName := types.NamespacedName{Name: sqlFailoverGroupName, Namespace: "default"}
 
 	t.Run("group3", func(t *testing.T) {
-		t.Run("delete db user", func(t *testing.T) {
+		t.Run("delete db users and ensure that their secrets have been cleaned up", func(t *testing.T) {
 			EnsureDelete(ctx, t, tc, sqlUser)
+			EnsureDelete(ctx, t, tc, kvSqlUser1)
+			EnsureDelete(ctx, t, tc, kvSqlUser2)
+
+			// Check that the user's secret is in the keyvault
+			keyVaultSecretClient := kvsecrets.New(tc.keyvaultName)
+
+			assert.Eventually(func() bool {
+				key := types.NamespacedName{Name: sqlUser.ObjectMeta.Name, Namespace: sqlUser.ObjectMeta.Namespace}
+				var _, err = tc.secretClient.Get(ctx, key)
+
+				// Once the secret is gone, the Kube secret client will return an error
+				return err != nil && strings.Contains(err.Error(), "not found")
+			}, tc.timeoutFast, tc.retry, "wait for the azuresqluser kube secret to be deleted")
+
+			assert.Eventually(func() bool {
+				keyNamespace := "azuresqluser-" + sqlServerName + "-" + sqlDatabaseName
+				key := types.NamespacedName{Name: kvSqlUser1.ObjectMeta.Name, Namespace: keyNamespace}
+				var _, err = keyVaultSecretClient.Get(ctx, key)
+
+				// Once the secret is gone, the KV secret client will return an err
+				return err != nil && strings.Contains(err.Error(), "secret does not exist")
+			}, tc.timeoutFast, tc.retry, "wait for the azuresqluser keyvault secret to be deleted")
+
+			assert.Eventually(func() bool {
+				keyNamespace := "azuresqluser-" + sqlServerName + "-" + sqlDatabaseName
+				keyName := kvSqlUser2.ObjectMeta.Name + "-adonet"
+				key := types.NamespacedName{Name: keyName, Namespace: keyNamespace}
+				var _, err = keyVaultSecretClient.Get(ctx, key)
+
+				// Once the secret is gone, the KV secret client will return an err
+				return err != nil && strings.Contains(err.Error(), "secret does not exist")
+			}, tc.timeoutFast, tc.retry, "wait for the azuresqluser custom formatted keyvault secret to be deleted")
 		})
 
 		t.Run("delete local firewallrule", func(t *testing.T) {

controllers/eventhub_controller_test.go

@@ -13,8 +13,6 @@ import (
 	azurev1alpha1 "github.com/Azure/azure-service-operator/api/v1alpha1"
 	"github.com/Azure/azure-service-operator/pkg/errhelp"
 
-	helpers "github.com/Azure/azure-service-operator/pkg/helpers"
-	"github.com/Azure/azure-service-operator/pkg/resourcemanager/config"
 	kvhelper "github.com/Azure/azure-service-operator/pkg/resourcemanager/keyvaults"
 	kvsecrets "github.com/Azure/azure-service-operator/pkg/secrets/keyvault"
 
@@ -192,13 +190,10 @@ func TestEventHubControllerCreateAndDeleteCustomKeyVault(t *testing.T) {
 	rgLocation := tc.resourceGroupLocation
 	ehnName := tc.eventhubNamespaceName
 	eventhubName := GenerateTestResourceNameWithRandom("ev", 10)
-	keyVaultNameForSecrets := helpers.FillWithRandom(GenerateTestResourceName("ev-kv"), 24)
-	userID := config.ClientID()
+	keyVaultNameForSecrets := tc.keyvaultName
 
-	// Create KeyVault with access policies
-	_, err := kvhelper.AzureKeyVaultManager.CreateVaultWithAccessPolicies(ctx, rgName, keyVaultNameForSecrets, rgLocation, userID)
-
-	_, err = kvhelper.AzureKeyVaultManager.GetVault(ctx, rgName, keyVaultNameForSecrets)
+	// Instantiate a KV client for the Keyvault that was created during test suite setup
+	_, err := kvhelper.AzureKeyVaultManager.GetVault(ctx, rgName, keyVaultNameForSecrets)
 	assert.Equal(nil, err, "wait for keyvault to be available")
 
 	// Create the EventHub object and expect the Reconcile to be created

controllers/helpers.go

@@ -52,6 +52,7 @@ type TestContext struct {
 	namespaceLocation       string
 	storageAccountName      string
 	blobContainerName       string
+	keyvaultName            string
 	resourceGroupManager    resourcegroupsresourcemanager.ResourceGroupManager
 	redisCacheManager       resourcemanagerrediscaches.RedisCacheManager
 	eventHubManagers        resourcemanagereventhub.EventHubManagers

controllers/suite_test.go

@@ -75,6 +75,8 @@ func setup() error {
 	blobContainerName := GenerateTestResourceName("blob-prime")
 	containerAccessLevel := s.PublicAccessContainer
 
+	keyvaultName := GenerateAlphaNumTestResourceName("kv-prime")
+
 	var timeout time.Duration
 
 	testEnv = &envtest.Environment{
@@ -186,7 +188,7 @@ func setup() error {
 	)
 	sqlActionManager = resourcemanagersqlaction.NewAzureSqlActionManager(secretClient, scheme.Scheme)
 
-	timeout = time.Second * 720
+	timeout = time.Second * 780
 
 	err = (&KeyVaultReconciler{
 		Reconciler: &AsyncReconciler{
@@ -588,6 +590,7 @@ func setup() error {
 		namespaceLocation:       namespaceLocation,
 		storageAccountName:      storageAccountName,
 		blobContainerName:       blobContainerName,
+		keyvaultName:            keyvaultName,
 		eventHubManagers:        eventHubManagers,
 		eventhubClient:          eventhubClient,
 		resourceGroupManager:    resourceGroupManager,
@@ -654,6 +657,12 @@ func setup() error {
 		return err
 	}
 
+	log.Println("Creating KV:", keyvaultName)
+	_, err = resourcemanagerkeyvaults.AzureKeyVaultManager.CreateVaultWithAccessPolicies(context.Background(), resourceGroupName, keyvaultName, resourcegroupLocation, resourcemanagerconfig.ClientID())
+	if err != nil {
+		return err
+	}
+
 	log.Println(fmt.Sprintf("finished common controller test setup"))
 
 	return nil

pkg/resourcemanager/azuresql/azuresqluser/azuresqluser.go

@@ -221,11 +221,11 @@ func (s *AzureSqlUserManager) Ensure(ctx context.Context, obj runtime.Object, op
 		DBSecret[SecretUsernameKey] = []byte(user)
 	}
 
-	// publish user secret
+	// Publishing the user secret:
 	// We do this first so if the keyvault does not have right permissions we will not proceed to creating the user
 
 	// determine our key namespace - if we're persisting to kube, we should use the actual instance namespace.
-	// In keyvault we have some creative freedom to allow more flexibility
+	// In keyvault we have to avoid collisions with other secrets so we create a custom namespace with the user's parameters
 	key = types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}
 	var dbUserCustomNamespace string
 
@@ -536,12 +536,12 @@ func (s *AzureSqlUserManager) convert(obj runtime.Object) (*v1alpha1.AzureSQLUse
 func (s *AzureSqlUserManager) DeleteSecrets(ctx context.Context, instance *v1alpha1.AzureSQLUser, secretClient secrets.SecretClient) (bool, error) {
 	// determine our key namespace - if we're persisting to kube, we should use the actual instance namespace.
 	// In keyvault we have some creative freedom to allow more flexibility
-	DBSecret := s.GetOrPrepareSecret(ctx, instance, s.SecretClient)
+	DBSecret := s.GetOrPrepareSecret(ctx, instance, secretClient)
 	secretKey := types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}
 
 	var dbUserCustomNamespace string
 
-	keyVaultEnabled := reflect.TypeOf(s.SecretClient).Elem().Name() == "KeyvaultSecretClient"
+	keyVaultEnabled := reflect.TypeOf(secretClient).Elem().Name() == "KeyvaultSecretClient"
 
 	if keyVaultEnabled {
 		// For a keyvault secret store, check for supplied namespace parameters
@@ -555,7 +555,7 @@ func (s *AzureSqlUserManager) DeleteSecrets(ctx context.Context, instance *v1alp
 	}
 
 	// delete standard user secret
-	err := s.SecretClient.Delete(
+	err := secretClient.Delete(
 		ctx,
 		secretKey,
 	)
@@ -580,10 +580,16 @@ func (s *AzureSqlUserManager) DeleteSecrets(ctx context.Context, instance *v1alp
 		}
 
 		for _, formatName := range customFormatNames {
-			err = s.SecretClient.Delete(
+			key := types.NamespacedName{Namespace: dbUserCustomNamespace, Name: instance.Name + "-" + formatName}
+
+			err = secretClient.Delete(
 				ctx,
-				types.NamespacedName{Namespace: dbUserCustomNamespace, Name: instance.Name + "-" + formatName},
+				key,
 			)
+			if err != nil {
+				instance.Status.Message = "failed to delete secret, err: " + err.Error()
+				return false, err
+			}
 		}
 	}
 

pkg/secrets/keyvault/client.go

@@ -336,8 +336,15 @@ func (k *KeyvaultSecretClient) Get(ctx context.Context, key types.NamespacedName
 
 	stringSecret := *result.Value
 
-	// Convert the data from json string to map and return
-	json.Unmarshal([]byte(stringSecret), &data)
+	// Convert the data from json string to map
+	jsonErr := json.Unmarshal([]byte(stringSecret), &data)
+
+	// If Unmarshal fails on the input data, the secret likely not a json string so we return the string value directly rather than unmarshaling
+	if jsonErr != nil {
+		data = map[string][]byte{
+			secretName: []byte(stringSecret),
+		}
+	}
 
 	return data, err
 }