Fix bugs related to secret management and improve documentation #1358

matthchr · 2021-01-13T18:47:02Z

Closes #1356
Closes #1360

What this PR does / why we need it:
Add new AZURE_SECRET_NAMING_VERSION mode

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

AppInsights created secrets in the same namespace as the resource but with name: "appinsights--"
Storage created secrets in the same namespace as the resource but with name: "storage--"
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.

Return proper error if we cannot deserialize secret.

Improved secrets documentation

If applicable:

this PR contains documentation
this PR contains tests

matthchr · 2021-01-28T18:26:46Z

controllers/helpers.go

@@ -444,3 +442,61 @@ func GenerateRandomSshPublicKeyString() string {
 	sshPublicKeyData := string(ssh.MarshalAuthorizedKey(publicRsaKey))
 	return sshPublicKeyData
 }
+
+//CreateVaultWithAccessPolicies creates a new key vault and provides access policies to the specified user - used in test
+func CreateVaultWithAccessPolicies(ctx context.Context, creds config.Credentials, groupName string, vaultName string, location string, clientID string) error {


This was copied from resourcemanager/keyvaults/keyvault.go as it's only used in test and it kept confusing me by being in the reconcile file.

controllers/suite_test.go

docs/howto/secrets.md

go.mod

pkg/resourcemanager/azuresql/azuresqluser/azuresqluser.go

matthchr · 2021-01-28T18:39:48Z

pkg/resourcemanager/azuresql/azuresqluser/azuresqluser_reconcile.go

 				}
 			} else {
-				err = sqlUserSecretClient.Delete(


Do we need to continue to ignore this error because some customers are expecting only best effort deletes?

pkg/resourcemanager/config/env.go

pkg/secrets/keyvault/client.go

theunrepentantgeek

Partial review, I got about half way through.

Does this actually need the versioning config for the secrets config?

Could we instead default to the new one with fallback to the old style if it's missing? Back compat for existing users, new features for new users, or amy I missing something important?

api/v1alpha1/storageaccount_types.go

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

config/default/manager_image_patch.yaml

controllers/appinsights_controller_test.go

controllers/async_controller.go

controllers/azuresql_combined_test.go

controllers/azuresqlaction_controller_test.go

controllers/suite_test.go

theunrepentantgeek

Overall looks good - within the limits of my understanding of the code anyway.
A handful of comments.

Something feels repetitive - as though there's a missing abstraction, but I can't put my finger on it. I'll have another review with fresh eyes tomorrow.

pkg/resourcemanager/azuresql/azuresqluser/azuresqluser_reconcile.go

pkg/resourcemanager/config/env.go

pkg/resourcemanager/storages/storageaccount/storageaccount.go

pkg/secrets/keyvault/client.go

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.

- This would prevent secrets from being created in Kubernetes

pkg/resourcemanager/config/env.go

pkg/secrets/keyvault/client.go

babbageclunk

This looks good, my only concern was how we ensure that the V1 secret naming path is tested. Thanks for discussion about that!

babbageclunk · 2021-02-03T03:57:46Z

controllers/async_controller.go


 		r.Telemetry.LogInfoByInstance("status", "ensuring vault", req.String())

+		// TODO: It's really awkward that we do this so often?


Wow yeah, that method seems really heavyweight! Is this to handle the case where the vault isn't accessible yet and the object is being created for the first time, the individual resource managers don't have to do the work of tearing down the resource they created if the secret can't be stored?

Although I don't think that really holds does it? There's still a possibility of the secret write failing even if we've looked-before-leaping. I checked a number of resources' Ensure methods and they almost all write the secret first (having generated keys/passwords/etc) to avoid that scenario, or in the case of redis the secret content is queryable (not very secret) after creation so it just requeues if it has a problem writing it. So possibly we don't need to do the check here?

This could be one of those "you're only allowed to remove this if you can tell me why it's there" situations though - maybe there's something I'm missing?

Yeah I think this is more about error code handling and setting the right message on the resource if we can't access the KeyVault, although doing a pre-check so that resources don't need to might be part of it as well.

Obviously as you point out, no amount of looking can guarantee your leap succeeds so pre-checking feels sorta pointless, and an alternative where we have an error handling library that can help us determine error causes presumably would mean that rather than doing this check once we can just farm it out to the error checker in all the places we interact with KV at which point there's not much value in doing this it seems to me as well? Especially since we need error handling in those other places already so it's not like we're forcing some there when previously we had none...

I've filed #1369 to track this as I don't think we want to bloat this PR further by me doing it here.

controllers/azuresql_combined_test.go

babbageclunk · 2021-02-04T01:29:16Z

pkg/resourcemanager/azuresql/azuresqlmanageduser/azuresqlmanageduser.go

+		return secrets.SecretKey{Name: instance.Name, Namespace: instance.Namespace, Kind: instance.TypeMeta.Kind}
+	}
+
+	// TODO: Ignoring prefix here... we ok with that?


The assumption is that the prefix was being used to prevent collisions, so we don't need it in the new version? That seems reasonable to me.

Yes. It's also taking an opportunity to remove some of the "extra" secrets configuration that only AzureSQLUser/AzureSQLManagedUser have that don't match the other types and doesn't really seem to add a lot of value to what customers can do (since it's not needed to avoid conflicts anymore it seems just extra complexity on our side to support it without much in the way of customer value add)

pkg/resourcemanager/config/env.go

pkg/secrets/keyvault/client.go

babbageclunk · 2021-02-05T04:28:49Z

controllers/secret_naming_version_test.go

+// NOTE: Tests in this file are intended to be run twice, once in the v1 secret naming mode
+// and once in the v2 secret naming mode. In some instances they parallel tests
+// which don't assert on secret name or shape.
+// The naming of the tests is important! It's how the makefile knows to run just these tests
+// in the old V1 mode.
+// Not every resource is included here, only resources which for legacy reasons in the old v1
+// secret naming mode had secret naming schemes which were different than the standard.


matthchr requested review from babbageclunk, davefellows, jananivMS, Porges, theunrepentantgeek and WilliamMortlMicrosoft as code owners January 13, 2021 18:47

matthchr force-pushed the feature/sql-user-failure branch from 3261a34 to 063790d Compare January 13, 2021 21:57

matthchr mentioned this pull request Jan 15, 2021

Bug: Generated Kubernetes secret names don't match documentation #1360

Closed

matthchr force-pushed the feature/sql-user-failure branch 9 times, most recently from 7ee2fe3 to f84d1e3 Compare January 27, 2021 23:04