[master] MySQL module fixes #56174
[master] MySQL module fixes #56174
Conversation
…anagement into different functions based on MySQL variant.
ghost
requested a review
from 
…QL and MariaDB. Adding some functions to install, remove, and check the status of plugins which we can then use when adding users which will use the unix_socket & auth_socket plugins. Adding additional tests for these new functions as well as test to ensure the correct SQL is being generated when using passwordless and unix_socket options.
|
I updated the PR to include some fixes for unix_socket support, turns out it's different between MySQL and MariaDB. Also added some additional functions to manage and check plugins, as the unix_socket and auth_socket plugins aren't enabled by default so we want to check for those before attempting to use them, makes for a nicer user experience to handle the error rather than get a traceback. If anyone is able to test out the PR that would be greatly appreciated. Thanks! |
I just tested the diff by directly applying it onto a salt-minion installed from upstream's Debian repo (3000+ds-1) - obviously, I ignored the unit tests for it. Afterwards, the initial problems For completeness: |
|
I have tested in same way, running mariadb, and mysql_user.present with allow_passwordless and unix_socket writes auth_socket to plugin column, then login with root fails, as I said in a comment. |
|
Changing auth_socket to unix_socket in line 1780 of modules/mysql.py fixes this issue. |
|
The auth for |
|
@carsten-AEI Thanks for testing. @scambra Thanks. What version of MariaDB were you testing with? @poofyteddy Thanks. I'll update those functions as well. |
…riadb chpass functions to ensure that the respective plugins are enabled before attempting to use them.
|
I've updated the PR with the requested changes. @poofyteddy I updated the chpass functions to ensure that the respective plugin is available before attempting to use it. I'm not entirely sure we need that logic for the exists functions as it's just doing a query to see if they're there, can you elaborate a bit on that request? Thanks! |
Oh ... i really didn't think about that.... L1345,L1491,L1732,L1788 you use an args, for the socket. useful if you build your commend from condition, but with this rewrite you no longer use them. I'm having issue that prevent me test the code. i can only offer a read today. |
|
@poofyteddy No worries! I just wanted to make sure I was clear about what your idea there was. In the other functions, we're still using args. They are returned from the database specific functions along with the built up query. |
|
I'm using mariadb 10.3. It's failing to detect unix_socket plugin is enabled. I had plugin enabled in mysql config: Plugin library is auth_socket.so, but plugin name is unix_socket. When plugin_status is not enabled, or salt thinks is not enabled, user_chpass state raise exception instead of displaying error. _mariadb_user_chpass (and _mysql_user_chpass) return false for qry variable, then _execute is called with qry, instead of checking qry |
|
salt-call mysql.plugin_status unix_socket returned nothing: Removing double quotes from qry fixes issue detecting plugin status: - qry = 'SELECT PLUGIN_STATUS FROM INFORMATION_SCHEMA.PLUGINS WHERE PLUGIN_NAME = "%(name)s"'
+ qry = 'SELECT PLUGIN_STATUS FROM INFORMATION_SCHEMA.PLUGINS WHERE PLUGIN_NAME = %(name)s'With fixed query: plugin_remove and plugin_add are wrong too: mariadb doesn't want plugin name to be quoted, changing lines 2745 and 2782 fixes it in mariadb, I don't know about mysql: - _execute(cur, qry, args)
+ _execute(cur, qry % args) |
…tempt to delete a user if they exist.
|
@scambra Pushed some more updates to the PR, can you check again and see if this last push fixes the remaining issues you've identified? |
|
mysql.plugin_status still fails, query in mysql.plugin_status hasn't been changed, double quotes should be removed in line 2824, _execute will take care of it. I don't know about test, but I guess previous test was right if double quotes were removed too. And add args into 2829 line. Or use "{0}"'.format(name) in line 2824, as uninstall plugin query. |
|
@scambra I tested without the quotes and it errored out for me. Can you provide an example state file or usage for when you see the double quotes in the plugin_status function causing an issue? Thanks! |
Current code is querying with SELECT PLUGIN_STATUS FROM INFORMATION_SCHEMA.PLUGINS WHERE PLUGIN_NAME = "%(name)s", without replacing %(name)s, because it calls _execute(cur, qry), without args. If I remove double quotes, it raises error, which fixes when args is added to _execute. I have changed modules/mysql.py like this (first line is 2823): cur = dbc.cursor()
qry = 'SELECT PLUGIN_STATUS FROM INFORMATION_SCHEMA.PLUGINS WHERE PLUGIN_NAME = %(name)s'
args = {}
args['name'] = name
try:
_execute(cur, qry, args) |
|
@scambra As soon as I posted this I tested again and saw the scenario you were talking about. I've updated the PR and believe it's in the correct state now. Thanks! |
|
@garethgreenaway everything works now |
|
@scambra excellent! Thanks for all the testing. |
What does this PR do?
Various fixes to the mysql module to break out the handling of user management into different functions based on MySQL variant.
What issues does this PR fix or reference?
#56124
#56177
#56170
Tests written?
[NOTICE] Bug fixes or features added to Salt require tests.
Please review the test documentation for details on how to implement tests into Salt's test suite.
Yes. Existing tests.
Commits signed with GPG?
Yes
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.