From 3108dc1486ce3ef7f73731ca5930b7d42b67538b Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Tue, 25 Feb 2020 14:50:05 -0800 Subject: [PATCH 01/11] fix nil pointer issue with empty permissions --- pkg/resourcemanager/keyvaults/keyvault.go | 85 +++++++++++++---------- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/pkg/resourcemanager/keyvaults/keyvault.go b/pkg/resourcemanager/keyvaults/keyvault.go index bf890e9d689..e2b105e0469 100644 --- a/pkg/resourcemanager/keyvaults/keyvault.go +++ b/pkg/resourcemanager/keyvaults/keyvault.go @@ -122,58 +122,69 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) return keyvault.AccessPolicyEntry{}, err } - var keyPermissions []keyvault.KeyPermissions - validKeyPermissions := keyvault.PossibleKeyPermissionsValues() - for _, key := range *policy.Permissions.Keys { - for _, validKey := range validKeyPermissions { - if keyvault.KeyPermissions(key) == validKey { - keyPermissions = append(keyPermissions, validKey) - break + newEntry := keyvault.AccessPolicyEntry{ + TenantID: &tenantID, + Permissions: &keyvault.Permissions{}, + } + + if policy.Permissions.Keys != nil { + var keyPermissions []keyvault.KeyPermissions + validKeyPermissions := keyvault.PossibleKeyPermissionsValues() + for _, key := range *policy.Permissions.Keys { + for _, validKey := range validKeyPermissions { + if keyvault.KeyPermissions(key) == validKey { + keyPermissions = append(keyPermissions, validKey) + break + } } } + + newEntry.Permissions.Keys = &keyPermissions } - var secretPermissions []keyvault.SecretPermissions - validSecretPermissions := keyvault.PossibleSecretPermissionsValues() - for _, key := range *policy.Permissions.Secrets { - for _, validSecret := range validSecretPermissions { - if keyvault.SecretPermissions(key) == validSecret { - secretPermissions = append(secretPermissions, validSecret) - break + if policy.Permissions.Secrets != nil { + var secretPermissions []keyvault.SecretPermissions + validSecretPermissions := keyvault.PossibleSecretPermissionsValues() + for _, key := range *policy.Permissions.Secrets { + for _, validSecret := range validSecretPermissions { + if keyvault.SecretPermissions(key) == validSecret { + secretPermissions = append(secretPermissions, validSecret) + break + } } } + + newEntry.Permissions.Secrets = &secretPermissions } - var certificatePermissions []keyvault.CertificatePermissions - validCertificatePermissions := keyvault.PossibleCertificatePermissionsValues() - for _, key := range *policy.Permissions.Certificates { - for _, validCert := range validCertificatePermissions { - if keyvault.CertificatePermissions(key) == validCert { - certificatePermissions = append(certificatePermissions, validCert) - break + if policy.Permissions.Secrets != nil { + var certificatePermissions []keyvault.CertificatePermissions + validCertificatePermissions := keyvault.PossibleCertificatePermissionsValues() + for _, key := range *policy.Permissions.Certificates { + for _, validCert := range validCertificatePermissions { + if keyvault.CertificatePermissions(key) == validCert { + certificatePermissions = append(certificatePermissions, validCert) + break + } } } + + newEntry.Permissions.Certificates = &certificatePermissions } - var storagePermissions []keyvault.StoragePermissions - validStoragePermissions := keyvault.PossibleStoragePermissionsValues() - for _, key := range *policy.Permissions.Storage { - for _, validStorage := range validStoragePermissions { - if keyvault.StoragePermissions(key) == validStorage { - storagePermissions = append(storagePermissions, validStorage) - break + if policy.Permissions.Storage != nil { + var storagePermissions []keyvault.StoragePermissions + validStoragePermissions := keyvault.PossibleStoragePermissionsValues() + for _, key := range *policy.Permissions.Storage { + for _, validStorage := range validStoragePermissions { + if keyvault.StoragePermissions(key) == validStorage { + storagePermissions = append(storagePermissions, validStorage) + break + } } } - } - newEntry := keyvault.AccessPolicyEntry{ - TenantID: &tenantID, - Permissions: &keyvault.Permissions{ - Keys: &keyPermissions, - Secrets: &secretPermissions, - Certificates: &certificatePermissions, - Storage: &storagePermissions, - }, + newEntry.Permissions.Storage = &storagePermissions } if policy.ApplicationID != "" { From 5e037336975304d79abada5995bcda360d8b7026 Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Tue, 25 Feb 2020 18:52:09 -0800 Subject: [PATCH 02/11] added tests --- controllers/keyvault_controller_test.go | 79 +++++++++++++++++++++++ pkg/resourcemanager/keyvaults/keyvault.go | 2 +- 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/controllers/keyvault_controller_test.go b/controllers/keyvault_controller_test.go index b9f389fe2eb..ce126728f6e 100644 --- a/controllers/keyvault_controller_test.go +++ b/controllers/keyvault_controller_test.go @@ -168,6 +168,85 @@ func TestKeyvaultControllerWithAccessPolicies(t *testing.T) { }, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from azure") } +func TestKeyvaultControllerWithLimitedAccessPolicies(t *testing.T) { + t.Parallel() + defer PanicRecover() + ctx := context.Background() + assert := assert.New(t) + keyVaultName := "t-kv-dev-" + helpers.RandomString(10) + const poll = time.Second * 10 + keyVaultLocation := tc.resourceGroupLocation + limitedPermissions := []string{"backup"} + accessPolicies := []azurev1alpha1.AccessPolicyEntry{ + { + TenantID: config.TenantID(), + ObjectID: config.ClientID(), + Permissions: &azurev1alpha1.Permissions{ + Keys: &limitedPermissions, + Secrets: &limitedPermissions, + }, + }} + + // Declare KeyVault object + keyVaultInstance := &azurev1alpha1.KeyVault{ + ObjectMeta: metav1.ObjectMeta{ + Name: keyVaultName, + Namespace: "default", + }, + Spec: azurev1alpha1.KeyVaultSpec{ + Location: keyVaultLocation, + ResourceGroup: tc.resourceGroupName, + AccessPolicies: &accessPolicies, + }, + } + // Create the Keyvault object and expect the Reconcile to be created + err := tc.k8sClient.Create(ctx, keyVaultInstance) + assert.Equal(nil, err, "create keyvault in k8s") + // Prep query for get + keyVaultNamespacedName := types.NamespacedName{Name: keyVaultName, Namespace: "default"} + assert.Eventually(func() bool { + _ = tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) + return helpers.HasFinalizer(keyVaultInstance, finalizerName) + }, tc.timeout, tc.retry, "wait for keyvault to have finalizer") + // Wait until key vault is provisioned + assert.Eventually(func() bool { + _ = tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) + return strings.Contains(keyVaultInstance.Status.Message, successMsg) + }, tc.timeout, tc.retry, "wait for keyVaultInstance to be ready in k8s") + // verify key vault exists in Azure + assert.Eventually(func() bool { + result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name) + return result.Response.StatusCode == http.StatusOK + }, tc.timeout, tc.retry, "wait for keyVaultInstance to be ready in azure") + //Add code to set secret and get secret from this keyvault using secretclient + + keyvaultSecretClient := kvsecrets.New(keyVaultName) + secretName := "test-key" + key := types.NamespacedName{Name: secretName, Namespace: "default"} + datanew := map[string][]byte{ + "test1": []byte("test2"), + "test2": []byte("test3"), + } + err = keyvaultSecretClient.Upsert(ctx, key, datanew) + assert.NotEqual(nil, err, "expect secret to not be inserted into keyvault") + + _, err = keyvaultSecretClient.Get(ctx, key) + assert.NotEqual(nil, err, "should not be able to get secrets") + + // delete key vault + err = tc.k8sClient.Delete(ctx, keyVaultInstance) + assert.Equal(nil, err, "delete keyvault in k8s") + // verify key vault is gone from kubernetes + assert.Eventually(func() bool { + err := tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) + return apierrors.IsNotFound(err) + }, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from k8s") + assert.Eventually(func() bool { + result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name) + return result.Response.StatusCode == http.StatusNotFound + }, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from azure") +} + func TestKeyvaultControllerInvalidName(t *testing.T) { t.Parallel() defer PanicRecover() diff --git a/pkg/resourcemanager/keyvaults/keyvault.go b/pkg/resourcemanager/keyvaults/keyvault.go index e2b105e0469..6c03fde1ac5 100644 --- a/pkg/resourcemanager/keyvaults/keyvault.go +++ b/pkg/resourcemanager/keyvaults/keyvault.go @@ -157,7 +157,7 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) newEntry.Permissions.Secrets = &secretPermissions } - if policy.Permissions.Secrets != nil { + if policy.Permissions.Certificates != nil { var certificatePermissions []keyvault.CertificatePermissions validCertificatePermissions := keyvault.PossibleCertificatePermissionsValues() for _, key := range *policy.Permissions.Certificates { From 6ca776cda97fa74d0130fae1f9bc284b252f6af6 Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Tue, 25 Feb 2020 19:08:50 -0800 Subject: [PATCH 03/11] fixed changes from master --- controllers/keyvault_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/keyvault_controller_test.go b/controllers/keyvault_controller_test.go index e3c8443638c..cef214f7bee 100644 --- a/controllers/keyvault_controller_test.go +++ b/controllers/keyvault_controller_test.go @@ -194,7 +194,7 @@ func TestKeyvaultControllerWithLimitedAccessPolicies(t *testing.T) { keyVaultNamespacedName := types.NamespacedName{Name: keyVaultName, Namespace: "default"} assert.Eventually(func() bool { _ = tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) - return helpers.HasFinalizer(keyVaultInstance, finalizerName) + return HasFinalizer(keyVaultInstance, finalizerName) }, tc.timeout, tc.retry, "wait for keyvault to have finalizer") // Wait until key vault is provisioned assert.Eventually(func() bool { From 63f492f87645ffb4280abbcdf01fcc9964d90f5b Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Thu, 27 Feb 2020 14:44:02 -0800 Subject: [PATCH 04/11] more unit tests, updated how we are checking permissions --- pkg/resourcemanager/keyvaults/keyvault.go | 64 +++++++++------ .../keyvaults/keyvault_test.go | 81 +++++++++++++++++++ 2 files changed, 121 insertions(+), 24 deletions(-) diff --git a/pkg/resourcemanager/keyvaults/keyvault.go b/pkg/resourcemanager/keyvaults/keyvault.go index 06e70eb89e3..9d376233b40 100644 --- a/pkg/resourcemanager/keyvaults/keyvault.go +++ b/pkg/resourcemanager/keyvaults/keyvault.go @@ -130,13 +130,17 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if policy.Permissions.Keys != nil { var keyPermissions []keyvault.KeyPermissions - validKeyPermissions := keyvault.PossibleKeyPermissionsValues() + permissions := keyvault.PossibleKeyPermissionsValues() + validKeyPermissions := []string{} + for _, item := range permissions { + validKeyPermissions = append(validKeyPermissions, string(item)) + } + for _, key := range *policy.Permissions.Keys { - for _, validKey := range validKeyPermissions { - if keyvault.KeyPermissions(key) == validKey { - keyPermissions = append(keyPermissions, validKey) - break - } + if helpers.ContainsString(validKeyPermissions, key) { + keyPermissions = append(keyPermissions, keyvault.KeyPermissions(key)) + } else { + return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Permission") } } @@ -145,13 +149,17 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if policy.Permissions.Secrets != nil { var secretPermissions []keyvault.SecretPermissions - validSecretPermissions := keyvault.PossibleSecretPermissionsValues() + permissions := keyvault.PossibleSecretPermissionsValues() + validSecretPermissions := []string{} + for _, item := range permissions { + validSecretPermissions = append(validSecretPermissions, string(item)) + } + for _, key := range *policy.Permissions.Secrets { - for _, validSecret := range validSecretPermissions { - if keyvault.SecretPermissions(key) == validSecret { - secretPermissions = append(secretPermissions, validSecret) - break - } + if helpers.ContainsString(validSecretPermissions, key) { + secretPermissions = append(secretPermissions, keyvault.SecretPermissions(key)) + } else { + return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Permission") } } @@ -160,13 +168,17 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if policy.Permissions.Certificates != nil { var certificatePermissions []keyvault.CertificatePermissions - validCertificatePermissions := keyvault.PossibleCertificatePermissionsValues() + permissions := keyvault.PossibleCertificatePermissionsValues() + validCertificatePermissions := []string{} + for _, item := range permissions { + validCertificatePermissions = append(validCertificatePermissions, string(item)) + } + for _, key := range *policy.Permissions.Certificates { - for _, validCert := range validCertificatePermissions { - if keyvault.CertificatePermissions(key) == validCert { - certificatePermissions = append(certificatePermissions, validCert) - break - } + if helpers.ContainsString(validCertificatePermissions, key) { + certificatePermissions = append(certificatePermissions, keyvault.CertificatePermissions(key)) + } else { + return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Permission") } } @@ -175,13 +187,17 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if policy.Permissions.Storage != nil { var storagePermissions []keyvault.StoragePermissions - validStoragePermissions := keyvault.PossibleStoragePermissionsValues() + permissions := keyvault.PossibleStoragePermissionsValues() + validStoragePermissions := []string{} + for _, item := range permissions { + validStoragePermissions = append(validStoragePermissions, string(item)) + } + for _, key := range *policy.Permissions.Storage { - for _, validStorage := range validStoragePermissions { - if keyvault.StoragePermissions(key) == validStorage { - storagePermissions = append(storagePermissions, validStorage) - break - } + if helpers.ContainsString(validStoragePermissions, key) { + storagePermissions = append(storagePermissions, keyvault.StoragePermissions(key)) + } else { + return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Permission") } } diff --git a/pkg/resourcemanager/keyvaults/keyvault_test.go b/pkg/resourcemanager/keyvaults/keyvault_test.go index d4d7cd2f196..24dfe9e2684 100644 --- a/pkg/resourcemanager/keyvaults/keyvault_test.go +++ b/pkg/resourcemanager/keyvaults/keyvault_test.go @@ -17,17 +17,98 @@ limitations under the License. package keyvaults import ( + "context" "fmt" + "testing" + "github.com/google/go-cmp/cmp" + + "github.com/Azure/azure-sdk-for-go/services/keyvault/mgmt/2018-02-14/keyvault" "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/go-autorest/autorest/to" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + uuid "github.com/satori/go.uuid" + "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +func TestParseAccessPoliciesInvalid(t *testing.T) { + entry := v1alpha1.AccessPolicyEntry{ + TenantID: "00000000-0000-0000-0000-000000000000", + Permissions: &v1alpha1.Permissions{ + Keys: &[]string{ + "create", + }, + Secrets: &[]string{ + "set", + "badpermission", + }, + }, + } + + ctx = context.Background() + + resp, err := parseAccessPolicy(&entry, ctx) + assert.True(t, err != nil) + assert.True(t, cmp.Equal(resp, keyvault.AccessPolicyEntry{})) +} + +func TestParseAccessPolicies(t *testing.T) { + entry := v1alpha1.AccessPolicyEntry{ + TenantID: "00000000-0000-0000-0000-000000000000", + Permissions: &v1alpha1.Permissions{ + Keys: &[]string{ + "create", + }, + Secrets: &[]string{ + "set", + "get", + }, + Certificates: &[]string{ + "list", + "get", + }, + Storage: &[]string{ + "list", + "get", + }, + }, + } + + id, err := uuid.FromString("00000000-0000-0000-0000-000000000000") + assert.True(t, err == nil) + + out := keyvault.AccessPolicyEntry{ + TenantID: &id, + Permissions: &keyvault.Permissions{ + Keys: &[]keyvault.KeyPermissions{ + keyvault.KeyPermissionsCreate, + }, + Secrets: &[]keyvault.SecretPermissions{ + keyvault.SecretPermissionsSet, + keyvault.SecretPermissionsGet, + }, + Certificates: &[]keyvault.CertificatePermissions{ + keyvault.List, + keyvault.Get, + }, + Storage: &[]keyvault.StoragePermissions{ + keyvault.StoragePermissionsList, + keyvault.StoragePermissionsGet, + }, + }, + } + + ctx = context.Background() + + resp, err := parseAccessPolicy(&entry, ctx) + assert.True(t, err == nil) + assert.True(t, cmp.Equal(resp, out)) +} + var _ = Describe("KeyVault Resource Manager test", func() { var rgName string From 5eee15658743cbfb557f04a170c679bf65243992 Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Thu, 27 Feb 2020 15:38:58 -0800 Subject: [PATCH 05/11] fixed unit test bad permission --- controllers/keyvault_controller_test.go | 22 +++++++++++++++++----- pkg/resourcemanager/keyvaults/keyvault.go | 8 ++++---- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/controllers/keyvault_controller_test.go b/controllers/keyvault_controller_test.go index cef214f7bee..fa8eaf76fc2 100644 --- a/controllers/keyvault_controller_test.go +++ b/controllers/keyvault_controller_test.go @@ -83,16 +83,28 @@ func TestKeyvaultControllerWithAccessPolicies(t *testing.T) { keyVaultName := "t-kv-dev-" + helpers.RandomString(10) const poll = time.Second * 10 keyVaultLocation := tc.resourceGroupLocation - allPermissions := []string{"get", "list", "set", "delete", "recover", "backup", "restore"} accessPolicies := []azurev1alpha1.AccessPolicyEntry{ { TenantID: config.TenantID(), ObjectID: config.ClientID(), Permissions: &azurev1alpha1.Permissions{ - Keys: &allPermissions, - Secrets: &allPermissions, - Certificates: &allPermissions, - Storage: &allPermissions, + Keys: &[]string{ + "get", + "list", + }, + Secrets: &[]string{ + "get", + "list", + "set", + }, + Certificates: &[]string{ + "get", + "list", + }, + Storage: &[]string{ + "get", + "list", + }, }, }} diff --git a/pkg/resourcemanager/keyvaults/keyvault.go b/pkg/resourcemanager/keyvaults/keyvault.go index 9d376233b40..c0abd87805d 100644 --- a/pkg/resourcemanager/keyvaults/keyvault.go +++ b/pkg/resourcemanager/keyvaults/keyvault.go @@ -140,7 +140,7 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if helpers.ContainsString(validKeyPermissions, key) { keyPermissions = append(keyPermissions, keyvault.KeyPermissions(key)) } else { - return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Permission") + return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Key Permission") } } @@ -159,7 +159,7 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if helpers.ContainsString(validSecretPermissions, key) { secretPermissions = append(secretPermissions, keyvault.SecretPermissions(key)) } else { - return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Permission") + return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Secret Permission") } } @@ -178,7 +178,7 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if helpers.ContainsString(validCertificatePermissions, key) { certificatePermissions = append(certificatePermissions, keyvault.CertificatePermissions(key)) } else { - return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Permission") + return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Certificate Permission") } } @@ -197,7 +197,7 @@ func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if helpers.ContainsString(validStoragePermissions, key) { storagePermissions = append(storagePermissions, keyvault.StoragePermissions(key)) } else { - return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Permission") + return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Storage Permission") } } From 2b07d30f194be132c84d90273d735529799d00ac Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Thu, 27 Feb 2020 16:22:44 -0800 Subject: [PATCH 06/11] added unit test --- Makefile | 3 + devops/azure-pipelines.yaml | 1 + pkg/resourcemanager/keyvaults/keyvault.go | 4 +- .../keyvaults/keyvault_test.go | 81 ----------------- .../keyvaults/unittest/keyvault_test.go | 87 +++++++++++++++++++ 5 files changed, 93 insertions(+), 83 deletions(-) create mode 100644 pkg/resourcemanager/keyvaults/unittest/keyvault_test.go diff --git a/Makefile b/Makefile index f86ccc51148..eef115768fc 100644 --- a/Makefile +++ b/Makefile @@ -46,6 +46,9 @@ test: generate fmt vet manifests test-existing-controllers: generate fmt vet manifests TEST_USE_EXISTING_CLUSTER=true TEST_CONTROLLER_WITH_MOCKS=false REQUEUE_AFTER=20 go test -tags all -parallel 3 -v ./controllers/... -timeout 30m +unit-tests: + go test ./pkg/resourcemanager/keyvaults/unittest/ + # Run tests with existing cluster test-existing-managers: generate fmt vet manifests TEST_USE_EXISTING_CLUSTER=true TEST_CONTROLLER_WITH_MOCKS=false REQUEUE_AFTER=20 \ diff --git a/devops/azure-pipelines.yaml b/devops/azure-pipelines.yaml index 8b87259a11f..6fb3d644ebd 100644 --- a/devops/azure-pipelines.yaml +++ b/devops/azure-pipelines.yaml @@ -125,6 +125,7 @@ steps: echo "all the pods should be running" make deploy make test-existing-controllers + make unit-tests continueOnError: 'false' displayName: 'Set kind cluster and Run int tests' env: diff --git a/pkg/resourcemanager/keyvaults/keyvault.go b/pkg/resourcemanager/keyvaults/keyvault.go index c0abd87805d..c5bdb9c0069 100644 --- a/pkg/resourcemanager/keyvaults/keyvault.go +++ b/pkg/resourcemanager/keyvaults/keyvault.go @@ -117,7 +117,7 @@ func parseNetworkPolicy(instance *v1alpha1.KeyVault) keyvault.NetworkRuleSet { return networkAcls } -func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) (keyvault.AccessPolicyEntry, error) { +func ParseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) (keyvault.AccessPolicyEntry, error) { tenantID, err := uuid.FromString(policy.TenantID) if err != nil { return keyvault.AccessPolicyEntry{}, err @@ -267,7 +267,7 @@ func (k *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph var accessPolicies []keyvault.AccessPolicyEntry if instance.Spec.AccessPolicies != nil { for _, policy := range *instance.Spec.AccessPolicies { - newEntry, err := parseAccessPolicy(&policy, ctx) + newEntry, err := ParseAccessPolicy(&policy, ctx) if err != nil { return keyvault.Vault{}, err } diff --git a/pkg/resourcemanager/keyvaults/keyvault_test.go b/pkg/resourcemanager/keyvaults/keyvault_test.go index 24dfe9e2684..d4d7cd2f196 100644 --- a/pkg/resourcemanager/keyvaults/keyvault_test.go +++ b/pkg/resourcemanager/keyvaults/keyvault_test.go @@ -17,98 +17,17 @@ limitations under the License. package keyvaults import ( - "context" "fmt" - "testing" - "github.com/google/go-cmp/cmp" - - "github.com/Azure/azure-sdk-for-go/services/keyvault/mgmt/2018-02-14/keyvault" "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/go-autorest/autorest/to" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - uuid "github.com/satori/go.uuid" - "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestParseAccessPoliciesInvalid(t *testing.T) { - entry := v1alpha1.AccessPolicyEntry{ - TenantID: "00000000-0000-0000-0000-000000000000", - Permissions: &v1alpha1.Permissions{ - Keys: &[]string{ - "create", - }, - Secrets: &[]string{ - "set", - "badpermission", - }, - }, - } - - ctx = context.Background() - - resp, err := parseAccessPolicy(&entry, ctx) - assert.True(t, err != nil) - assert.True(t, cmp.Equal(resp, keyvault.AccessPolicyEntry{})) -} - -func TestParseAccessPolicies(t *testing.T) { - entry := v1alpha1.AccessPolicyEntry{ - TenantID: "00000000-0000-0000-0000-000000000000", - Permissions: &v1alpha1.Permissions{ - Keys: &[]string{ - "create", - }, - Secrets: &[]string{ - "set", - "get", - }, - Certificates: &[]string{ - "list", - "get", - }, - Storage: &[]string{ - "list", - "get", - }, - }, - } - - id, err := uuid.FromString("00000000-0000-0000-0000-000000000000") - assert.True(t, err == nil) - - out := keyvault.AccessPolicyEntry{ - TenantID: &id, - Permissions: &keyvault.Permissions{ - Keys: &[]keyvault.KeyPermissions{ - keyvault.KeyPermissionsCreate, - }, - Secrets: &[]keyvault.SecretPermissions{ - keyvault.SecretPermissionsSet, - keyvault.SecretPermissionsGet, - }, - Certificates: &[]keyvault.CertificatePermissions{ - keyvault.List, - keyvault.Get, - }, - Storage: &[]keyvault.StoragePermissions{ - keyvault.StoragePermissionsList, - keyvault.StoragePermissionsGet, - }, - }, - } - - ctx = context.Background() - - resp, err := parseAccessPolicy(&entry, ctx) - assert.True(t, err == nil) - assert.True(t, cmp.Equal(resp, out)) -} - var _ = Describe("KeyVault Resource Manager test", func() { var rgName string diff --git a/pkg/resourcemanager/keyvaults/unittest/keyvault_test.go b/pkg/resourcemanager/keyvaults/unittest/keyvault_test.go new file mode 100644 index 00000000000..3c01fcedc6e --- /dev/null +++ b/pkg/resourcemanager/keyvaults/unittest/keyvault_test.go @@ -0,0 +1,87 @@ +package testing + +import ( + "context" + "testing" + + keyvault "github.com/Azure/azure-sdk-for-go/services/keyvault/mgmt/2018-02-14/keyvault" + v1alpha1 "github.com/Azure/azure-service-operator/api/v1alpha1" + azurekeyvault "github.com/Azure/azure-service-operator/pkg/resourcemanager/keyvaults" + "github.com/google/go-cmp/cmp" + uuid "github.com/satori/go.uuid" + "github.com/stretchr/testify/assert" +) + +func TestParseAccessPoliciesInvalid(t *testing.T) { + entry := v1alpha1.AccessPolicyEntry{ + TenantID: "00000000-0000-0000-0000-000000000000", + Permissions: &v1alpha1.Permissions{ + Keys: &[]string{ + "create", + }, + Secrets: &[]string{ + "set", + "badpermission", + }, + }, + } + + ctx := context.Background() + + resp, err := azurekeyvault.ParseAccessPolicy(&entry, ctx) + assert.True(t, err != nil) + assert.True(t, cmp.Equal(resp, keyvault.AccessPolicyEntry{})) +} + +func TestParseAccessPolicies(t *testing.T) { + entry := v1alpha1.AccessPolicyEntry{ + TenantID: "00000000-0000-0000-0000-000000000000", + Permissions: &v1alpha1.Permissions{ + Keys: &[]string{ + "create", + }, + Secrets: &[]string{ + "set", + "get", + }, + Certificates: &[]string{ + "list", + "get", + }, + Storage: &[]string{ + "list", + "get", + }, + }, + } + + id, err := uuid.FromString("00000000-0000-0000-0000-000000000000") + assert.True(t, err == nil) + + out := keyvault.AccessPolicyEntry{ + TenantID: &id, + Permissions: &keyvault.Permissions{ + Keys: &[]keyvault.KeyPermissions{ + keyvault.KeyPermissionsCreate, + }, + Secrets: &[]keyvault.SecretPermissions{ + keyvault.SecretPermissionsSet, + keyvault.SecretPermissionsGet, + }, + Certificates: &[]keyvault.CertificatePermissions{ + keyvault.List, + keyvault.Get, + }, + Storage: &[]keyvault.StoragePermissions{ + keyvault.StoragePermissionsList, + keyvault.StoragePermissionsGet, + }, + }, + } + + ctx := context.Background() + + resp, err := azurekeyvault.ParseAccessPolicy(&entry, ctx) + assert.True(t, err == nil) + assert.True(t, cmp.Equal(resp, out)) +} From 4ed971bcb23b7b31f97f25416be35ad4437c124d Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Fri, 28 Feb 2020 10:03:52 -0800 Subject: [PATCH 07/11] increasing timeout --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index eef115768fc..806e5da65a5 100644 --- a/Makefile +++ b/Makefile @@ -44,7 +44,7 @@ test: generate fmt vet manifests # Run tests with existing cluster test-existing-controllers: generate fmt vet manifests - TEST_USE_EXISTING_CLUSTER=true TEST_CONTROLLER_WITH_MOCKS=false REQUEUE_AFTER=20 go test -tags all -parallel 3 -v ./controllers/... -timeout 30m + TEST_USE_EXISTING_CLUSTER=true TEST_CONTROLLER_WITH_MOCKS=false REQUEUE_AFTER=20 go test -tags all -parallel 3 -v ./controllers/... -timeout 45m unit-tests: go test ./pkg/resourcemanager/keyvaults/unittest/ From 65a8c56780a7965e9ecb723cd73078cadbfc3000 Mon Sep 17 00:00:00 2001 From: jananivMS Date: Fri, 28 Feb 2020 12:47:30 -0700 Subject: [PATCH 08/11] catching error in recocile --- pkg/errhelp/errors.go | 5 +++++ pkg/resourcemanager/keyvaults/keyvault.go | 9 +++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/pkg/errhelp/errors.go b/pkg/errhelp/errors.go index 457cd8ec133..ba40750bdc5 100644 --- a/pkg/errhelp/errors.go +++ b/pkg/errhelp/errors.go @@ -3,6 +3,7 @@ package errhelp import ( "encoding/json" "fmt" + "strings" "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/azure" @@ -32,6 +33,7 @@ const ( InvalidRequestFormat = "InvalidRequestFormat" KeyNotFound = "KeyNotFound" InvalidParameters = "InvalidParameters" + InvalidAccessPolicy = "InvalidAccessPolicy" ) func NewAzureError(err error) error { @@ -83,6 +85,9 @@ func NewAzureError(err error) error { } else if err.Error() == AccountNameInvalid { kind = AccountNameInvalid reason = AccountNameInvalid + } else if strings.Contains(err.Error(), InvalidAccessPolicy) { + kind = InvalidAccessPolicy + reason = InvalidAccessPolicy } ae.Reason = reason ae.Type = kind diff --git a/pkg/resourcemanager/keyvaults/keyvault.go b/pkg/resourcemanager/keyvaults/keyvault.go index c5bdb9c0069..a51a03e76f2 100644 --- a/pkg/resourcemanager/keyvaults/keyvault.go +++ b/pkg/resourcemanager/keyvaults/keyvault.go @@ -140,7 +140,7 @@ func ParseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if helpers.ContainsString(validKeyPermissions, key) { keyPermissions = append(keyPermissions, keyvault.KeyPermissions(key)) } else { - return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Key Permission") + return keyvault.AccessPolicyEntry{}, fmt.Errorf("InvalidAccessPolicy: Invalid Key Permission") } } @@ -159,7 +159,7 @@ func ParseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if helpers.ContainsString(validSecretPermissions, key) { secretPermissions = append(secretPermissions, keyvault.SecretPermissions(key)) } else { - return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Secret Permission") + return keyvault.AccessPolicyEntry{}, fmt.Errorf("InvalidAccessPolicy: Invalid Secret Permission") } } @@ -178,7 +178,7 @@ func ParseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if helpers.ContainsString(validCertificatePermissions, key) { certificatePermissions = append(certificatePermissions, keyvault.CertificatePermissions(key)) } else { - return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Certificate Permission") + return keyvault.AccessPolicyEntry{}, fmt.Errorf("InvalidAccessPolicy: Invalid Certificate Permission") } } @@ -197,7 +197,7 @@ func ParseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) if helpers.ContainsString(validStoragePermissions, key) { storagePermissions = append(storagePermissions, keyvault.StoragePermissions(key)) } else { - return keyvault.AccessPolicyEntry{}, fmt.Errorf("Invalid Storage Permission") + return keyvault.AccessPolicyEntry{}, fmt.Errorf("InvalidAccessPolicy: Invalid Storage Permission") } } @@ -418,6 +418,7 @@ func (k *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object) ( catchUnrecoverableErrors := []string{ errhelp.AccountNameInvalid, errhelp.AlreadyExists, + errhelp.InvalidAccessPolicy, } azerr := errhelp.NewAzureErrorAzureError(err) From 937a97e3a6b707380c3e758e37123f73b1ec41cf Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Fri, 28 Feb 2020 15:21:29 -0800 Subject: [PATCH 09/11] Update azure-pipelines.yaml --- devops/azure-pipelines.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/devops/azure-pipelines.yaml b/devops/azure-pipelines.yaml index 6fb3d644ebd..8b87259a11f 100644 --- a/devops/azure-pipelines.yaml +++ b/devops/azure-pipelines.yaml @@ -125,7 +125,6 @@ steps: echo "all the pods should be running" make deploy make test-existing-controllers - make unit-tests continueOnError: 'false' displayName: 'Set kind cluster and Run int tests' env: From 155231f85f65e2fd2188f6a2d7669f386e76028d Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Fri, 28 Feb 2020 17:36:43 -0800 Subject: [PATCH 10/11] condensed unit tests using helper functions --- controllers/keyvault_controller_test.go | 73 +++++-------------------- 1 file changed, 13 insertions(+), 60 deletions(-) diff --git a/controllers/keyvault_controller_test.go b/controllers/keyvault_controller_test.go index fa8eaf76fc2..031dc11bade 100644 --- a/controllers/keyvault_controller_test.go +++ b/controllers/keyvault_controller_test.go @@ -25,7 +25,6 @@ func TestKeyvaultControllerHappyPath(t *testing.T) { defer PanicRecover() ctx := context.Background() assert := assert.New(t) - var err error keyVaultName := "t-kv-dev-" + helpers.RandomString(10) const poll = time.Second * 10 @@ -48,25 +47,13 @@ func TestKeyvaultControllerHappyPath(t *testing.T) { // Create the Keyvault object and expect the Reconcile to be created EnsureInstance(ctx, t, tc, keyVaultInstance) - // Prep query for get - keyVaultNamespacedName := types.NamespacedName{Name: keyVaultName, Namespace: "default"} - // verify key vault exists in Azure assert.Eventually(func() bool { result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name) return result.Response.StatusCode == http.StatusOK }, tc.timeout, tc.retry, "wait for keyVaultInstance to be ready in azure") - // delete key vault - err = tc.k8sClient.Delete(ctx, keyVaultInstance) - assert.Equal(nil, err, "delete keyvault in k8s") - - // verify key vault is gone from kubernetes - - assert.Eventually(func() bool { - err := tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) - return apierrors.IsNotFound(err) - }, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from k8s") + EnsureDelete(ctx, t, tc, keyVaultInstance) assert.Eventually(func() bool { result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name) @@ -120,20 +107,9 @@ func TestKeyvaultControllerWithAccessPolicies(t *testing.T) { AccessPolicies: &accessPolicies, }, } - // Create the Keyvault object and expect the Reconcile to be created - err := tc.k8sClient.Create(ctx, keyVaultInstance) - assert.Equal(nil, err, "create keyvault in k8s") - // Prep query for get - keyVaultNamespacedName := types.NamespacedName{Name: keyVaultName, Namespace: "default"} - assert.Eventually(func() bool { - _ = tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) - return HasFinalizer(keyVaultInstance, finalizerName) - }, tc.timeout, tc.retry, "wait for keyvault to have finalizer") - // Wait until key vault is provisioned - assert.Eventually(func() bool { - _ = tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) - return strings.Contains(keyVaultInstance.Status.Message, successMsg) - }, tc.timeout, tc.retry, "wait for keyVaultInstance to be ready in k8s") + + EnsureInstance(ctx, t, tc, keyVaultInstance) + // verify key vault exists in Azure assert.Eventually(func() bool { result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name) @@ -148,20 +124,14 @@ func TestKeyvaultControllerWithAccessPolicies(t *testing.T) { "test1": []byte("test2"), "test2": []byte("test3"), } - err = keyvaultSecretClient.Upsert(ctx, key, datanew) + err := keyvaultSecretClient.Upsert(ctx, key, datanew) assert.Equal(nil, err, "expect secret to be inserted into keyvault") _, err = keyvaultSecretClient.Get(ctx, key) assert.Equal(nil, err, "checking if secret is present in keyvault") - // delete key vault - err = tc.k8sClient.Delete(ctx, keyVaultInstance) - assert.Equal(nil, err, "delete keyvault in k8s") - // verify key vault is gone from kubernetes - assert.Eventually(func() bool { - err := tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) - return apierrors.IsNotFound(err) - }, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from k8s") + EnsureDelete(ctx, t, tc, keyVaultInstance) + assert.Eventually(func() bool { result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name) return result.Response.StatusCode == http.StatusNotFound @@ -199,20 +169,9 @@ func TestKeyvaultControllerWithLimitedAccessPolicies(t *testing.T) { AccessPolicies: &accessPolicies, }, } - // Create the Keyvault object and expect the Reconcile to be created - err := tc.k8sClient.Create(ctx, keyVaultInstance) - assert.Equal(nil, err, "create keyvault in k8s") - // Prep query for get - keyVaultNamespacedName := types.NamespacedName{Name: keyVaultName, Namespace: "default"} - assert.Eventually(func() bool { - _ = tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) - return HasFinalizer(keyVaultInstance, finalizerName) - }, tc.timeout, tc.retry, "wait for keyvault to have finalizer") - // Wait until key vault is provisioned - assert.Eventually(func() bool { - _ = tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) - return strings.Contains(keyVaultInstance.Status.Message, successMsg) - }, tc.timeout, tc.retry, "wait for keyVaultInstance to be ready in k8s") + + EnsureInstance(ctx, t, tc, keyVaultInstance) + // verify key vault exists in Azure assert.Eventually(func() bool { result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name) @@ -227,20 +186,14 @@ func TestKeyvaultControllerWithLimitedAccessPolicies(t *testing.T) { "test1": []byte("test2"), "test2": []byte("test3"), } - err = keyvaultSecretClient.Upsert(ctx, key, datanew) + err := keyvaultSecretClient.Upsert(ctx, key, datanew) assert.NotEqual(nil, err, "expect secret to not be inserted into keyvault") _, err = keyvaultSecretClient.Get(ctx, key) assert.NotEqual(nil, err, "should not be able to get secrets") - // delete key vault - err = tc.k8sClient.Delete(ctx, keyVaultInstance) - assert.Equal(nil, err, "delete keyvault in k8s") - // verify key vault is gone from kubernetes - assert.Eventually(func() bool { - err := tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance) - return apierrors.IsNotFound(err) - }, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from k8s") + EnsureDelete(ctx, t, tc, keyVaultInstance) + assert.Eventually(func() bool { result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name) return result.Response.StatusCode == http.StatusNotFound From 9b6aa847efa9850ca74dff5cfa8afeacf953ea73 Mon Sep 17 00:00:00 2001 From: Claudia Nadolny Date: Fri, 28 Feb 2020 19:42:12 -0800 Subject: [PATCH 11/11] update permissions --- controllers/keyvaultkey_controller_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/controllers/keyvaultkey_controller_test.go b/controllers/keyvaultkey_controller_test.go index f9b006529f0..ef8871dc68f 100644 --- a/controllers/keyvaultkey_controller_test.go +++ b/controllers/keyvaultkey_controller_test.go @@ -31,16 +31,13 @@ func TestKeyvaultKeyControllerHappyPath(t *testing.T) { keyVaultLocation := tc.resourceGroupLocation - allPermissions := []string{"get", "list", "set", "delete", "recover", "backup", "restore", "create"} + keyPermissions := []string{"get", "list", "update", "delete", "recover", "backup", "restore", "create", "import"} accessPolicies := []azurev1alpha1.AccessPolicyEntry{ { TenantID: config.TenantID(), ObjectID: config.ClientID(), Permissions: &azurev1alpha1.Permissions{ - Keys: &allPermissions, - Secrets: &allPermissions, - Certificates: &allPermissions, - Storage: &allPermissions, + Keys: &keyPermissions, }, }}