Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MySQL user roles when users are modified #1317

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

matthchr
Copy link
Member

This is related to but does not resolve #1311

What this PR does / why we need it:
We need to decide if we want to support this functionality at all. It's "easiest" for MySQL and harder for Azure SQL (I haven't even looked into PostgreSQL).

If applicable:

  • this PR contains documentation
  • this PR contains tests

@matthchr matthchr changed the title Update roles when users are modified Update MySQL user roles when users are modified Nov 24, 2020
babbageclunk
babbageclunk previously approved these changes Nov 24, 2020
Copy link
Member

@babbageclunk babbageclunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but possibly there's a way to get the role information in a more machine-readable format.

controllers/mysql_combined_test.go Show resolved Hide resolved
controllers/mysql_combined_test.go Show resolved Hide resolved
controllers/mysql_combined_test.go Show resolved Hide resolved
pkg/helpers/sqlrole.go Outdated Show resolved Hide resolved
pkg/helpers/sqlrole.go Outdated Show resolved Hide resolved
pkg/helpers/sqlrole_test.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/mysqlhelper.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/mysqlhelper.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/mysqlhelper.go Outdated Show resolved Hide resolved
pkg/resourcemanager/mysql/mysqlhelper.go Outdated Show resolved Hide resolved
@matthchr matthchr force-pushed the feature/mysql-role-updates branch 2 times, most recently from 5c378f9 to 8becf7e Compare November 25, 2020 20:15
Copy link
Member

@babbageclunk babbageclunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great with one tweak - user can be a parameter for the extract roles query.

controllers/mysql_combined_test.go Show resolved Hide resolved
Comment on lines 102 to 103
tsql := fmt.Sprintf("SELECT PRIVILEGE_TYPE from information_schema.SCHEMA_PRIVILEGES WHERE GRANTEE = '''%s''@''%%''' and TABLE_SCHEMA = ?", user)
rows, err := db.QueryContext(ctx, tsql, database)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grantee can be a parameter here, can't it? You'd still have to build the weird formatting with the quotes and @ - that's a bit easier if you don't need to sql-quote it.

Suggested change
tsql := fmt.Sprintf("SELECT PRIVILEGE_TYPE from information_schema.SCHEMA_PRIVILEGES WHERE GRANTEE = '''%s''@''%%''' and TABLE_SCHEMA = ?", user)
rows, err := db.QueryContext(ctx, tsql, database)
tsql := "select privilege_type from information_schema.schema_privileges where grantee = ? and table_schema= ?"
formattedUser := fmt.Sprintf("'%s'@'%'", user)
rows, err := db.QueryContext(ctx, tsql, formattedUser, database)

I tend to make queries all lowercase, not sure what the standard is - a lot of people prefer SQL keywords in uppercase.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah all of the existing queries we have are all uppercase for the keywords at least. I'll fix this

@matthchr matthchr force-pushed the feature/mysql-role-updates branch from 8becf7e to 60c1f5d Compare November 25, 2020 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: SQL Users whose spec is updated to change assigned roles do not update roles in the backing DB
2 participants