Add support for MySQL AAD Users #1315

matthchr · 2020-11-19T22:41:05Z

What this PR does / why we need it:
Adds support for AAD users in MySQL. These users can only be created or managed if the identity ASO is operating as is:

A managed identity
The "Active Directory Administrator" of the MySQL DB. Users can make the ASO Managed Identity this admin using the resource added in Add support for MySQL Administrators #1314.

This also includes updates to the Managed Identity documentation to help improve clarity of the feature. As part of this I deleted the tool createMi.go as all it really did was run 3 az cli commands to create a managed identity and assign it permissions. It made assumptions about what permissions you wanted the identity to have. Additionally it suggested to install aad-pod-identity, but this guidance is not appropriate for installations via Helm because Helm already installs that dependency as part of the chart. Rather than having a script try to be one-size fits all it makes more sense to give customers the instructions they need to create the managed identity and allow them to choose what permissions they want to give it.

Special notes for your reviewer:
I may need to update this resource to also work with non-managed identities, as we have customers asking for that use case as well... so there may be some updates to the PR.

Testing this in our existing test infrastructure is basically not doable, as you need to be deployed to a VM in Azure and have a pod running with an MSI assigned. Going forward we will want to fix that, but for now I tested this manually to unblock us.

How does this PR make you feel:

If applicable:

this PR contains documentation
this PR contains tests

api/v1alpha1/mysqlaaduser_types.go

theunrepentantgeek

I admit I didn't follow a few pieces, but looks good otherwise. A few minor comments.

theunrepentantgeek · 2020-11-22T22:29:27Z

README.md

@@ -60,6 +60,7 @@ Ready to quickly deploy the latest version of Azure Service Operator on your Kub
    helm repo add azureserviceoperator https://raw.githubusercontent.com/Azure/azure-service-operator/master/charts
    ```
 3. Create an Azure Service Principal. You'll need this to grant Azure Service Operator permissions to create resources in your subscription.
+   For more information about other forms of authentication supported by ASO, see [] 


MIssing link.

Oops, good eye!

api/v1alpha1/mysqlaaduser_types.go

config/samples/azure_v1alpha1_mysqlaaduser.yaml

theunrepentantgeek · 2020-11-22T22:38:35Z

controllers/azuresqluser_controller.go

-// +kubebuilder:rbac:groups=azure.microsoft.com,resources=AzureSQLUsers,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=azure.microsoft.com,resources={AzureSQLUsers/status,AzureSQLUsers/finalizers},verbs=get;update;patch
+// +kubebuilder:rbac:groups=azure.microsoft.com,resources=azuresqlusers,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=azure.microsoft.com,resources={azuresqlusers/status,azuresqlusers/finalizers},verbs=get;update;patch


Is this casing change a breaking one?

No. As I mentioned below to @babbageclunk, the issue is that these were duplicate entries and the ones in main.go were winning. The ones in main.go were lowercase so I just moved them here to match the pattern all of the other resources had with the RBAC permissions being on the _controller file.

I just went and manually confirmed as well that in master in the generated role.yaml file, the permissions for azuresqlusers are lowercase:

resources: - azuresqlusers

theunrepentantgeek · 2020-11-23T00:43:21Z

docs/howto/deploy.md

+
+2. Apply the AzureIdentity and Binding manifests. This binds the identity to the Azure Service Operator. 
+Where a particular `resourceID` or `clientID` is referenced in the template below, ensure that you replace it with your Managed Identity `resourceID` and `clientID`.
+If you used the script mentioned [here](managedidentity.md), this should be automatically done for you.                                                                                                         


Should the bash shell stuff below have specific subscription and identities removed, to encourage people to fill out their own?

I'm not really aware of a way to templatize this and still leave it "clean" -- but what I can do at least is change the subscription ID to all 0's.

Actually... I can just do <your-x-here>, maybe that's cleaner? I'll do that.

theunrepentantgeek · 2020-11-23T00:46:16Z

pkg/resourcemanager/iam/authorizers.go

+// TODO: looks like here? https://github.com/Azure-Samples/azure-sdk-for-go-samples/blob/master/internal/iam/authorizers.go
+
+// TODO: No we need to actually delete this file and all of its usage I think...
+// TODO: just use https://github.com/Azure/go-autorest/blob/master/autorest/azure/auth/auth.go NewAuthorizerFromEnvironment


Should these TODOs be addressed now?

They are actually wrong, will fix and remove TODO as what remains is mostly "notes" rather than a TODO.

babbageclunk

This looks great - I don't fully understand the pod identity details. From the reading I did today it seems like the magic is in NMI rewriting the node's iptables so it can intercept MSI requests and add extra information about the identity on the way. Is that right?

README.md

api/v1alpha1/mysqlaaduser_types.go

babbageclunk · 2020-11-23T03:08:51Z

cli/cmd/createMi.go

-// Copyright (c) Microsoft Corporation.
-// Licensed under the MIT License.
-
-package cmd


Could you add something in the description explaining why this was deleted? I'm guessing because it's obsolete now? Although this change is specific to MySQL - is there any need for a more general facility as well?

Added the following:

This also includes updates to the Managed Identity documentation to help improve clarity of the feature. As part of this I deleted the tool createMi.go as all it really did was run 3 az cli commands to create a managed identity and assign it permissions. It made assumptions about what permissions you wanted the identity to have. Additionally it suggested to install aad-pod-identity, but this guidance is not appropriate for installations via Helm because Helm already installs that dependency as part of the chart. Rather than having a script try to be one-size fits all it makes more sense to give customers the instructions they need to create the managed identity and allow them to choose what permissions they want to give it.

Awesome, thanks!

babbageclunk · 2020-11-23T04:32:38Z

docs/howto/deploy.md

+    clientID: 5bb69783-06a1-4277-82b7-97820b357503
+  ---
+  apiVersion: "aadpodidentity.k8s.io/v1"
+  kind: AzureIdentityBinding


Something I don't understand about aad-pod-identity from reading the docs is: why is the AzureIdentityBinding needed? The pods are annotated with a label matching the selector on the binding, but why wouldn't the label refer directly to the identity instead? I don't understand why there's an extra layer of indirection.

Hmm. I guess it would mean that you could switch the identity being used by the pods without having to update them all, you could just update the binding. So maybe that's it? It would be good if they could spell out the reasons in the docs though.

They also have an example of having more than one binding with selectors that match the label on a pod, so the identity used would be random? It seems very weird.

pkg/resourcemanager/mysql/mysqlaaduser/reconcile.go

pkg/resourcemanager/mysql/mysqlhelper.go

pkg/resourcemanager/mysql/mysqluser/mysqluser.go

pkg/resourcemanager/mysql/mysqluser/mysqluser_reconcile.go

theunrepentantgeek

Looks good.

README.md

babbageclunk

Looks great now thanks!

- This will allow us to differentiate between different identities when we support multi-tenancy.

Co-authored-by: Christian Muirhead <christian.muirhead@microsoft.com>

matthchr requested review from babbageclunk, davefellows, frodopwns, jananivMS, Porges, theunrepentantgeek and WilliamMortlMicrosoft as code owners November 19, 2020 22:41

Porges reviewed Nov 22, 2020

View reviewed changes

api/v1alpha1/mysqlaaduser_types.go Outdated Show resolved Hide resolved

theunrepentantgeek reviewed Nov 23, 2020

View reviewed changes

babbageclunk previously approved these changes Nov 23, 2020

View reviewed changes

matthchr dismissed babbageclunk’s stale review via 3251a07 November 24, 2020 00:39

matthchr force-pushed the feature/mysql-aad branch 2 times, most recently from 0558d1c to 62784ed Compare November 24, 2020 00:50

theunrepentantgeek previously approved these changes Nov 24, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

matthchr dismissed theunrepentantgeek’s stale review via 1272106 November 24, 2020 00:58

matthchr force-pushed the feature/mysql-aad branch from 62784ed to 1272106 Compare November 24, 2020 00:58

babbageclunk previously approved these changes Nov 24, 2020

View reviewed changes

matthchr added 6 commits November 24, 2020 09:52

Add support for MySQL AAD Users

b608824

Fix strangeness with "user" CRDs RBAC location

b810d2f

Pass pod namespace to operator via env variable

6be850b

Change to enforce tls=true in MySQL DB connection

f8025c2

Improve managed identity documentation

fd07379

Attempt to read clientID when ManagedIdentity is enabled

b946fc4

- This will allow us to differentiate between different identities when we support multi-tenancy.

matthchr dismissed babbageclunk’s stale review via 697ce67 November 24, 2020 17:53

matthchr force-pushed the feature/mysql-aad branch from 1272106 to 697ce67 Compare November 24, 2020 17:53

Update pkg/resourcemanager/iam/authorizers.go

ade5236

Co-authored-by: Christian Muirhead <christian.muirhead@microsoft.com>

matthchr force-pushed the feature/mysql-aad branch from 697ce67 to ade5236 Compare November 24, 2020 18:25

theunrepentantgeek approved these changes Nov 24, 2020

View reviewed changes

matthchr merged commit 1b6f19d into Azure:master Nov 24, 2020

matthchr deleted the feature/mysql-aad branch November 24, 2020 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for MySQL AAD Users #1315

Add support for MySQL AAD Users #1315

matthchr commented Nov 19, 2020 •

edited

Loading

theunrepentantgeek left a comment

theunrepentantgeek Nov 22, 2020

matthchr Nov 23, 2020

theunrepentantgeek Nov 22, 2020

matthchr Nov 23, 2020

theunrepentantgeek Nov 23, 2020

matthchr Nov 23, 2020

matthchr Nov 23, 2020

theunrepentantgeek Nov 23, 2020

matthchr Nov 23, 2020

babbageclunk left a comment

babbageclunk Nov 23, 2020

matthchr Nov 24, 2020

babbageclunk Nov 24, 2020

babbageclunk Nov 23, 2020

theunrepentantgeek left a comment

babbageclunk left a comment

Add support for MySQL AAD Users #1315

Add support for MySQL AAD Users #1315

Conversation

matthchr commented Nov 19, 2020 • edited Loading

theunrepentantgeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babbageclunk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theunrepentantgeek left a comment

Choose a reason for hiding this comment

babbageclunk left a comment

Choose a reason for hiding this comment

matthchr commented Nov 19, 2020 •

edited

Loading