Add MySQLUser and MySQLAADUser v1alpha2 #1357

babbageclunk · 2021-01-13T02:03:28Z

The new versions of MySQLUser and MySQLAADUser have roles per-database and server-level roles rather than a user being specific to one database. This adds conversions from v1alpha1 -> v1alpha2 (these are always possible), and supports conversion back to v1alpha1 as long as the user doesn't have any server-level roles and has roles in exactly one database. It also includes the yaml situps to connect the conversion webhooks.

This implements #1346 for MySQL. The initial plan had been to split this change into two PRs with the reconciliation changes in a follow-up, but because envtest doesn't support conversion webhooks the CI tests would have prevented this, so reconciliation and controller tests are included as the last two commits.

How does this PR make you feel:

If applicable:

this PR contains documentation (in the form of samples)
this PR contains tests

api/v1alpha1/mysqlaaduser_conversion.go

api/v1alpha1/mysqluser_conversion.go

api/v1alpha1/mysqlaaduser_conversion.go

api/v1alpha2/mysqlaaduser_conversion.go

api/v1alpha2/mysqlaaduser_types.go

config/crd/patches/webhook_in_mysqlusers.yaml

config/samples/azure_v1alpha2_mysqlaaduser.yaml

config/samples/azure_v1alpha2_mysqluser.yaml

matthchr · 2021-01-14T18:10:35Z

Looks like CI failed - could be related to your changes as the tests that failed were some of the negative test cases in MySQLUser?

babbageclunk · 2021-01-14T22:45:49Z

/azp run

azure-pipelines · 2021-01-14T22:46:03Z

Azure Pipelines successfully started running 1 pipeline(s).

babbageclunk · 2021-01-21T01:25:05Z

It turns out that envtest doesn't yet support conversion webhooks. That means the tests for this PR wouldn't pass without the reconciliation and tests being updated to use the v1alpha2 versions of MySQLUser and MySQLAADUser, so I've included those in this PR (the last two commits).

This removes DbName from MySQLUser and adds DatabaseRoles to store per-database permissions. Roles will now only store server-wide permissions. Add conversions between v1alpha1 and v1alpha2 versions.

This removes DBName from MySQLAADUser and adds DatabaseRoles to store per-database permissions. Roles will now only store server-wide permissions. Add conversions between v1alpha1 and v1alpha2 versions.

These were set for all types with version conversions but not the others (which aren't in use since they are still commented out in kustomization.yaml). Turning them on in the rest to remove one step in the process of adding conversion webhooks to types in the future. This setting is required for conversion to work - it seems like the only reason it's not set in the patches is that they were generated by kubebuilder before the setting was mandatory.

They now check server-level (in USER_PRIVILEGES) and database-level (SCHEMA_PRIVILEGES) permissions.

matthchr

Some minor comments asking for improved comments in a few places, and then one sorta big comment about the behavior when there are failures reconciling DB permissions.

pkg/resourcemanager/mysql/mysqlaaduser/reconcile.go

pkg/resourcemanager/mysql/mysqlhelper.go

matthchr · 2021-01-22T00:40:08Z

pkg/resourcemanager/mysql/mysqlhelper.go

@@ -168,7 +213,53 @@ func GrantUserRoles(ctx context.Context, user string, database string, roles []s
 	return nil
 }

-func addRoles(ctx context.Context, db *sql.DB, database string, user string, roles map[string]struct{}) error {
+// EnsureUserDatabaseRoles revokes and grants database roles as needed
+// so they match the ones passed in. It returns warning messages if


I'm not totally clear how the comment:

It returns warning messages if some databases don't exist

is correct?

Is ExtractUserDatabaseRoles succeeding (but with 0 roles) if the DB doesn't exist? It's not clear to me if that's an error case or an "empty return" case.

Other errors when adding/removing roles will also just get reported as warnings right? As a matter of fact based on my read of this function and its caller it seems like even if we cannot assign some roles to some DBs we will still say Provisioned = true and State = "Succeeded" - that seems probably not correct?

I can sorta see the reasoning around bypassing DB not exists (as maybe next reconcile loop it will), but wonder if we need to be more explicit about checking for errors that aren't that one. Even for that error it might make sense to leave this resource in a "provisioned=false" state as it's not done with all that was asked?

This makes a lot of sense - after our discussion I'll accumulate the errors for different databases into one error from the Ensure function (even though we still apply the roles for the later databases on an error) and have the user still be provisioned=false in the reconciler.

config/samples/azure_v1alpha2_mysqlaaduser.yaml

config/samples/azure_v1alpha2_mysqluser.yaml

matthchr · 2021-01-22T00:47:01Z

controllers/mysql_combined_test.go

 		mysql.MySQLServerPort,
 		adminUser,
 		adminPassword)
 	assert.NoError(err)

+	expectedRoles := mysql.SliceToSet(updatedRoles)


May be lacking some tests around the the "DBName doesn't exist" case - it was explicitly mentioned in one of the comments but not sure that this test covers it?

pkg/resourcemanager/mysql/mysqluser/mysqluser_reconcile.go

matthchr · 2021-01-22T00:51:44Z

pkg/resourcemanager/mysql/mysqluser/mysqluser_reconcile.go

 	}

-	err = mysql.GrantUserRoles(ctx, user, instance.Spec.DbName, instance.Spec.Roles, db)
+	warnings, err := mysql.EnsureUserDatabaseRoles(ctx, db, user, instance.Spec.DatabaseRoles)


Same comment here about if we really know that what is ending up in warnings here is something we're ok classifying as a warning.

Also rename the ServerPort and DriverName constants so they don't repeat the name of the package.

@matthchr

And change the reconciliation code for user and aad user to just treat that as a provisioning failure, rather than saying that it had succeeded but there were some errors which is just confusing. We still try to apply changes to all databases even if there is an error for one of them. Also other review changes, thanks @matthchr!

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

matthchr previously approved these changes Jan 13, 2021

View reviewed changes

babbageclunk dismissed matthchr’s stale review via c918b8c January 14, 2021 02:09

babbageclunk force-pushed the mysql-user-no-db branch 2 times, most recently from f26d51e to 6339871 Compare January 14, 2021 02:26

matthchr previously approved these changes Jan 14, 2021

View reviewed changes

babbageclunk dismissed matthchr’s stale review via 38e6a54 January 14, 2021 23:47

babbageclunk force-pushed the mysql-user-no-db branch 4 times, most recently from 8c68932 to c35d61f Compare January 21, 2021 01:19

babbageclunk changed the title ~~Add MySQLUser and MySQLAADUser v1alpha2 + conversion webhooks~~ Add MySQLUser and MySQLAADUser v1alpha2 Jan 21, 2021

babbageclunk added 8 commits January 21, 2021 14:34

Add v1alpha2 MySQLUser

65a32c9

This removes DbName from MySQLUser and adds DatabaseRoles to store per-database permissions. Roles will now only store server-wide permissions. Add conversions between v1alpha1 and v1alpha2 versions.

Add v1alpha2 MySQLAADUser

c658f62

This removes DBName from MySQLAADUser and adds DatabaseRoles to store per-database permissions. Roles will now only store server-wide permissions. Add conversions between v1alpha1 and v1alpha2 versions.

Set up conversion webhooks for MySQLUser and MySQLAADUser

425f088

Review feedback, thanks @matthchr!

9750afe

Add provisioning state methods to v1alpha2 ASOStatus

1cb2037

Reimplement MySQLUser and MySQLAADUser reconciliation with v1alpha2

5ae336a

They now check server-level (in USER_PRIVILEGES) and database-level (SCHEMA_PRIVILEGES) permissions.

Update controller tests to work with v1alpha2 MySQLUser

737c295

babbageclunk force-pushed the mysql-user-no-db branch from c35d61f to 737c295 Compare January 21, 2021 01:34

babbageclunk requested a review from matthchr January 21, 2021 20:40

babbageclunk mentioned this pull request Jan 22, 2021

WIP: Make MySQLUser and MySQLAADUser server-level users rather than per-database #1362

Closed

2 tasks

matthchr reviewed Jan 22, 2021

View reviewed changes

babbageclunk added 3 commits January 22, 2021 15:29

Driveby gofmt fix

9fbdd32

Move system database constant to mysql.SystemDatabase

75b41f9

Also rename the ServerPort and DriverName constants so they don't repeat the name of the package.

babbageclunk force-pushed the mysql-user-no-db branch from 9e3b57e to d784570 Compare January 31, 2021 22:11

matthchr approved these changes Feb 1, 2021

View reviewed changes

babbageclunk merged commit 7991802 into Azure:master Feb 1, 2021

babbageclunk deleted the mysql-user-no-db branch February 1, 2021 18:56

matthchr mentioned this pull request Feb 4, 2021

Allow Mysql AAD User DB Name to be empty. #1339

Closed

2 tasks

matthchr mentioned this pull request Feb 20, 2021

Improve how database users are modeled for MySQL and PostgreSQL #1346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MySQLUser and MySQLAADUser v1alpha2 #1357

Add MySQLUser and MySQLAADUser v1alpha2 #1357

babbageclunk commented Jan 13, 2021 •

edited

Loading

matthchr commented Jan 14, 2021

babbageclunk commented Jan 14, 2021

azure-pipelines bot commented Jan 14, 2021

babbageclunk commented Jan 21, 2021

matthchr left a comment

matthchr Jan 22, 2021

babbageclunk Jan 22, 2021

matthchr Jan 22, 2021

matthchr Jan 22, 2021

Add MySQLUser and MySQLAADUser v1alpha2 #1357

Add MySQLUser and MySQLAADUser v1alpha2 #1357

Conversation

babbageclunk commented Jan 13, 2021 • edited Loading

matthchr commented Jan 14, 2021

babbageclunk commented Jan 14, 2021

azure-pipelines bot commented Jan 14, 2021

babbageclunk commented Jan 21, 2021

matthchr left a comment

Choose a reason for hiding this comment

matthchr Jan 22, 2021

Choose a reason for hiding this comment

babbageclunk Jan 22, 2021

Choose a reason for hiding this comment

matthchr Jan 22, 2021

Choose a reason for hiding this comment

matthchr Jan 22, 2021

Choose a reason for hiding this comment

babbageclunk commented Jan 13, 2021 •

edited

Loading