Fix bugs related to secret management and improve documentation (#1358)

* Fix typo in readme

* Improve secrets documentation

* Return proper error if we cannot deserialize secret

* Add new AZURE_SECRET_NAMING_VERSION mode

The new mode allows us to fix inconsitencies in how secrets
were named without making a breaking change.

  - AppInsights created secrets in the same namespace
    as the resource but with name:
    "appinsights-<resourceGroup>-<resourceName>"
  - Storage created secrets in the same namespace
    as the resource but with name:
    "storage-<resourceGroup>-<resourceName>"
  - AzureSQL resources created resources with
    a different naming scheme as well.
  - Other resources created a secret in the same
    namespace with the secret name being the
    resource name.

The new V2 mode ensures that all resources create secrets
in KeyVault and/or Kubernetes with a consistent naming pattern.

* Update Helm chart (but don't generate new package)

* Fix bug where SQLManagedUser Namespace could be empty

  - This would prevent secrets from being created in Kubernetes

* Enable V2 secrets for EnvTest tests

* Use v1beta1 explicitly with controller-gen

* PR feedback

* PR feedback

* Better testing

Makefile

@@ -9,7 +9,7 @@ GOBIN=$(shell go env GOBIN)
 endif
 
 # Produce CRDs that work back to Kubernetes 1.11 (no version conversion)
-CRD_OPTIONS ?= "crd"
+CRD_OPTIONS ?= "crd:crdVersions=v1beta1"
 
 BUILD_ID ?= $(shell git rev-parse --short HEAD)
 timestamp := $(shell /bin/date "+%Y%m%d-%H%M%S")
@@ -55,9 +55,17 @@ generate-test-certs:
 .PHONY: test-integration-controllers
 test-integration-controllers: generate fmt vet manifests
 	TEST_RESOURCE_PREFIX=$(TEST_RESOURCE_PREFIX) TEST_USE_EXISTING_CLUSTER=false REQUEUE_AFTER=20 \
-	go test -v -tags "$(BUILD_TAGS)" -coverprofile=reports/integration-controllers-coverage-output.txt -coverpkg=./... -covermode count -parallel 4 -timeout 45m ./controllers/...
-	#2>&1 | tee reports/integration-controllers-output.txt
-	#go-junit-report < reports/integration-controllers-output.txt > reports/integration-controllers-report.xml
+	go test -v -tags "$(BUILD_TAGS)" -coverprofile=reports/integration-controllers-coverage-output.txt -coverpkg=./... -covermode count -parallel 4 -timeout 45m \
+		./controllers/... \
+		./pkg/secrets/...
+		# TODO: Note that the above test (secrets/keyvault) is not an integration-controller test... but it's not a unit test either and unfortunately the test-integration-managers target isn't run in CI either?
+
+# Run subset of tests with v1 secret naming enabled to ensure no regression in old secret naming
+.PHONY: test-v1-secret-naming
+test-v1-secret-naming: generate fmt vet manifests
+	TEST_RESOURCE_PREFIX=$(TEST_RESOURCE_PREFIX) TEST_USE_EXISTING_CLUSTER=false REQUEUE_AFTER=20 AZURE_SECRET_NAMING_VERSION=1 \
+	go test -v -run "^.*_SecretNamedCorrectly$$" -tags "$(BUILD_TAGS)" -coverprofile=reports/v1-secret-naming-coverage-output.txt -coverpkg=./... -covermode count -parallel 4 -timeout 10m \
+		./controllers/...
 
 # Run Resource Manager tests against the configured cluster
 .PHONY: test-integration-managers
@@ -73,10 +81,8 @@ test-integration-managers: generate fmt vet manifests
 	./pkg/resourcemanager/psql/firewallrule/... \
 	./pkg/resourcemanager/appinsights/... \
 	./pkg/resourcemanager/vnet/...
-	#2>&1 | tee reports/integration-managers-output.txt
-	#go-junit-report < reports/integration-managers-output.txt > reports/integration-managers-report.xml
 
-# Run all available tests. Note that Controllers are not unit-testable.
+# Run all available unit tests.
 .PHONY: test-unit
 test-unit: generate fmt vet manifests 
 	TEST_USE_EXISTING_CLUSTER=false REQUEUE_AFTER=20 \
@@ -85,8 +91,6 @@ test-unit: generate fmt vet manifests
 	./pkg/resourcemanager/azuresql/azuresqlfailovergroup
 	# The below folders are commented out because the tests in them fail...
 	# ./api/... \
-	# ./pkg/secrets/... \
-	#2>&1 | tee testlogs.txt
 
 # Merge all the available test coverage results and publish a single report
 .PHONY: test-process-coverage
@@ -223,12 +227,9 @@ generate: manifests
 # download controller-gen if necessary
 .PHONY: controller-gen
 controller-gen:
-ifeq (, $(shell which controller-gen))
-	go get sigs.k8s.io/controller-tools/cmd/[email protected]
-CONTROLLER_GEN=$(shell go env GOPATH)/bin/controller-gen
-else
-CONTROLLER_GEN=$(shell which controller-gen)
-endif
+	go get sigs.k8s.io/controller-tools/cmd/[email protected]
+    CONTROLLER_GEN=$(shell go env GOPATH)/bin/controller-gen
+
 
 .PHONY: install-bindata
 install-bindata:
@@ -316,13 +317,12 @@ install-aad-pod-identity:
 	kubectl apply -f https://raw.githubusercontent.com/Azure/aad-pod-identity/master/deploy/infra/deployment-rbac.yaml
 
 .PHONY: install-test-dependencies
-install-test-dependencies:
+install-test-dependencies: controller-gen
 	go get github.com/jstemmer/go-junit-report \
 	&& go get github.com/axw/gocov/gocov \
 	&& go get github.com/AlekSi/gocov-xml \
 	&& go get github.com/wadey/gocovmerge \
 	&& go get k8s.io/code-generator/cmd/[email protected] \
-	&& go get sigs.k8s.io/controller-tools/cmd/[email protected] \
 	&& go get sigs.k8s.io/[email protected] \
 	&& go get sigs.k8s.io/kustomize/kustomize/[email protected]
 

README.md

@@ -107,7 +107,7 @@ Ready to quickly deploy the latest version of Azure Service Operator on your Kub
             --set azureClientSecret=$AZURE_CLIENT_SECRET
     ```
 
-    If you would like to install an older version you can list the avialable versions:
+    If you would like to install an older version you can list the available versions:
     ```sh
     helm search repo aso --versions
     ```

api/v1alpha1/azuresqluser_types.go

@@ -36,7 +36,7 @@ type AzureSQLUserSpec struct {
 	AdminSecretKeyVault    string   `json:"adminSecretKeyVault,omitempty"`
 	Username               string   `json:"username,omitempty"`
 	KeyVaultToStoreSecrets string   `json:"keyVaultToStoreSecrets,omitempty"`
-	KeyVaultSecretPrefix   string   `json:"keyVaultSecretPrefix,omitempty"`
+	KeyVaultSecretPrefix   string   `json:"keyVaultSecretPrefix,omitempty"` // TODO: Remove this in a future version?
 	KeyVaultSecretFormats  []string `json:"keyVaultSecretFormats,omitempty"`
 }
 

api/v1alpha1/storageaccount_types.go

@@ -39,7 +39,8 @@ type StorageAccountSpec struct {
 // StorageAccountSku the SKU of the storage account.
 type StorageAccountSku struct {
 	// Name - The SKU name. Required for account creation; optional for update.
-	// Possible values include: 'StandardLRS', 'StandardGRS', 'StandardRAGRS', 'StandardZRS', 'PremiumLRS', 'PremiumZRS', 'StandardGZRS', 'StandardRAGZRS'
+	// Possible values include: 'Standard_LRS', 'Standard_GRS', 'Standard_RAGRS', 'Standard_ZRS', 'Premium_LRS', 'Premium_ZRS', 'Standard_GZRS', 'Standard_RAGZRS'.
+	// For the full list of allowed options, see: https://docs.microsoft.com/en-us/rest/api/storagerp/storageaccounts/create#skuname
 	Name StorageAccountSkuName `json:"name,omitempty"`
 }
 

azure-pipelines.yml

@@ -135,6 +135,26 @@ steps:
       BUILD_ID: $(Build.BuildId)
     workingDirectory: '$(System.DefaultWorkingDirectory)'
 
+  # TODO: There is no way to run steps in parallel in Azure pipelines but ideally this step would run in parallel
+  # TODO: with the above testing step to reduce overall runtime
+  - script: |
+      set -e
+      export PATH=$PATH:$(go env GOPATH)/bin:$(go env GOPATH)/kubebuilder/bin
+      export KUBEBUILDER_ASSETS=$(go env GOPATH)/kubebuilder/bin
+      make test-v1-secret-naming
+    displayName: Run legacy v1 secret naming tests
+    condition: eq(variables['check_changes.SOURCE_CODE_CHANGED'], 'true')
+    continueOnError: 'false'
+    env:
+      GO111MODULE: on
+      AZURE_SUBSCRIPTION_ID: $(AZURE_SUBSCRIPTION_ID)
+      AZURE_TENANT_ID: $(AZURE_TENANT_ID)
+      AZURE_CLIENT_ID: $(AZURE_CLIENT_ID)
+      AZURE_CLIENT_SECRET: $(AZURE_CLIENT_SECRET)
+      REQUEUE_AFTER: $(REQUEUE_AFTER)
+      BUILD_ID: $(Build.BuildId)
+    workingDirectory: '$(System.DefaultWorkingDirectory)'
+
   - script: |
       set -e
       export PATH=$PATH:$(go env GOPATH)/bin

charts/azure-service-operator/templates/secret.yaml

@@ -20,4 +20,7 @@ data:
   {{- end }}
   {{- if .Values.azureOperatorKeyvault }}
   AZURE_OPERATOR_KEYVAULT: {{ .Values.azureOperatorKeyvault | b64enc | quote }}
-  {{- end }}
+  {{- end }}
+  {{- if .Values.azureSecretNamingVersion }}
+  AZURE_SECRET_NAMING_VERSION: {{ .Values.azureSecretNamingVersion | b64enc | quote }}
+  {{- end }}

charts/azure-service-operator/values.yaml

@@ -19,6 +19,9 @@ azureClientSecret: ""
 # azureUseMI determines if ASO will use a Managed Identity to authenticate.
 azureUseMI: False
 
+# azureSecretNamingVersion allows choosing the algorithm used to derive secret names. Version 2 is recommended.
+azureSecretNamingVersion: "2"
+
 # image defines the container image the ASO pod should run
 # Note: This should use the latest released tag number explicitly. If
 # it's ':latest' and someone deploys the chart after a new version has

config/default/manager_image_patch.yaml

@@ -51,6 +51,12 @@ spec:
                 key: AZURE_CLOUD_ENV
                 name: azureoperatorsettings
                 optional: true
+          - name: AZURE_SECRET_NAMING_VERSION
+            valueFrom:
+              secretKeyRef:
+                name: azureoperatorsettings
+                key: AZURE_SECRET_NAMING_VERSION
+                optional: true
           # Used along with aad-pod-identity integration, but set always
           # because it doesn't hurt
           - name: POD_NAMESPACE

controllers/appinsights_controller_test.go

@@ -9,8 +9,9 @@ import (
 	"context"
 	"testing"
 
-	azurev1alpha1 "github.com/Azure/azure-service-operator/api/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	azurev1alpha1 "github.com/Azure/azure-service-operator/api/v1alpha1"
 )
 
 func TestAppInsightsController(t *testing.T) {
@@ -23,7 +24,7 @@ func TestAppInsightsController(t *testing.T) {
 	appInsightsName := GenerateTestResourceName("appinsights")
 
 	// Create an instance of Azure AppInsights
-	appInsightsInstance := &azurev1alpha1.AppInsights{
+	instance := &azurev1alpha1.AppInsights{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      appInsightsName,
 			Namespace: "default",
@@ -36,7 +37,7 @@ func TestAppInsightsController(t *testing.T) {
 		},
 	}
 
-	EnsureInstance(ctx, t, tc, appInsightsInstance)
+	EnsureInstance(ctx, t, tc, instance)
 
-	EnsureDelete(ctx, t, tc, appInsightsInstance)
+	EnsureDelete(ctx, t, tc, instance)
 }

controllers/async_controller.go

@@ -26,9 +26,9 @@ import (
 )
 
 const (
-	finalizerName string        = "azure.microsoft.com/finalizer"
-	requeDuration time.Duration = time.Second * 20
-	successMsg    string        = "successfully provisioned"
+	finalizerName   string        = "azure.microsoft.com/finalizer"
+	requeueDuration time.Duration = time.Second * 20
+	successMsg      string        = "successfully provisioned"
 )
 
 // AsyncReconciler is a generic reconciler for Azure resources.
@@ -72,20 +72,21 @@ func (r *AsyncReconciler) Reconcile(req ctrl.Request, obj runtime.Object) (resul
 	var keyvaultSecretClient secrets.SecretClient
 
 	// Determine if we need to check KeyVault for secrets
-	KeyVaultName := keyvaultsecretlib.GetKeyVaultName(obj)
+	keyVaultName := keyvaultsecretlib.GetKeyVaultName(obj)
 
-	if len(KeyVaultName) != 0 {
+	if len(keyVaultName) != 0 {
 		// Instantiate the KeyVault Secret Client
-		keyvaultSecretClient = keyvaultsecretlib.New(KeyVaultName, config.GlobalCredentials())
+		keyvaultSecretClient = keyvaultsecretlib.New(keyVaultName, config.GlobalCredentials(), config.SecretNamingVersion())
 
 		r.Telemetry.LogInfoByInstance("status", "ensuring vault", req.String())
 
+		// TODO: It's really awkward that we do this so often?
 		if !keyvaultsecretlib.IsKeyVaultAccessible(keyvaultSecretClient) {
 			r.Telemetry.LogInfoByInstance("requeuing", "awaiting vault verification", req.String())
 
 			// update the status of the resource in kubernetes
 			status.Message = "Waiting for secretclient keyvault to be available"
-			return ctrl.Result{RequeueAfter: requeDuration}, r.Status().Update(ctx, obj)
+			return ctrl.Result{RequeueAfter: requeueDuration}, r.Status().Update(ctx, obj)
 		}
 	}
 
@@ -118,7 +119,7 @@ func (r *AsyncReconciler) Reconcile(req ctrl.Request, obj runtime.Object) (resul
 		}
 	} else {
 		if HasFinalizer(res, finalizerName) {
-			if len(KeyVaultName) != 0 { //KeyVault was specified in Spec, so use that for secrets
+			if len(keyVaultName) != 0 { // keyVault was specified in Spec, so use that for secrets
 				configOptions = append(configOptions, resourcemanager.WithSecretClient(keyvaultSecretClient))
 			}
 			found, deleteErr := r.AzureClient.Delete(ctx, obj, configOptions...)
@@ -134,7 +135,7 @@ func (r *AsyncReconciler) Reconcile(req ctrl.Request, obj runtime.Object) (resul
 				return ctrl.Result{}, r.Update(ctx, obj)
 			}
 			r.Telemetry.LogInfoByInstance("requeuing", "deletion unfinished", req.String())
-			return ctrl.Result{RequeueAfter: requeDuration}, r.Status().Update(ctx, obj)
+			return ctrl.Result{RequeueAfter: requeueDuration}, r.Status().Update(ctx, obj)
 		}
 		return ctrl.Result{}, nil
 	}
@@ -158,7 +159,7 @@ func (r *AsyncReconciler) Reconcile(req ctrl.Request, obj runtime.Object) (resul
 
 	r.Telemetry.LogInfoByInstance("status", "reconciling object", req.String())
 
-	if len(KeyVaultName) != 0 { //KeyVault was specified in Spec, so use that for secrets
+	if len(keyVaultName) != 0 { //KeyVault was specified in Spec, so use that for secrets
 		configOptions = append(configOptions, resourcemanager.WithSecretClient(keyvaultSecretClient))
 	}
 
@@ -191,7 +192,7 @@ func (r *AsyncReconciler) Reconcile(req ctrl.Request, obj runtime.Object) (resul
 	result = ctrl.Result{}
 	if !done {
 		r.Telemetry.LogInfoByInstance("status", "reconciling object not finished", req.String())
-		result.RequeueAfter = requeDuration
+		result.RequeueAfter = requeueDuration
 	} else {
 		r.Telemetry.LogInfoByInstance("reconciling", "success", req.String())