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

feat[mysql_info]: add 'users_privs' filter #572

Conversation

laurent-indermuehle
Copy link
Collaborator

@laurent-indermuehle laurent-indermuehle commented Sep 15, 2023

SUMMARY

Add filter users_privs to the mysql_info module.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

mysql_info

ADDITIONAL INFORMATION

This filter returns information about users accounts. The output can be used as an input of the mysql_user plugin. Useful when migrating accounts to a new server or to create an inventory.

It doesn't return informations about password_option and lock_option but since the mysql_usermodule doesn't support them, I don't bother implemented them yet.

Doesn't support sha256_password and caching_sha2_password authentication plugins. Even if you use print_identified_with_as_hex to convert the password into a hash. I think this is because PyMySQL and
mysqlclient wrap the password in quote: CREATE USER... IDENTIFIED AS '0x1234' instead of CREATE USER... IDENTIFIED AS 0x1234. We could try to convert to another python connector, like mysql-connector-python. But this is way too much work for me who only work on MariaDB with mysql_native_password.


@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (033b4c7) 76.35% compared to head (de1cbae) 72.01%.

❗ Current head de1cbae differs from pull request most recent head 2e2780b. Consider uploading reports for the commit 2e2780b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #572      +/-   ##
==========================================
- Coverage   76.35%   72.01%   -4.35%     
==========================================
  Files          28       15      -13     
  Lines        2394     2283     -111     
  Branches      584      580       -4     
==========================================
- Hits         1828     1644     -184     
- Misses        390      448      +58     
- Partials      176      191      +15     
Flag Coverage Δ
integration 71.87% <78.35%> (-1.96%) ⬇️
sanity ?
units ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
plugins/module_utils/mysql.py 63.11% <100.00%> (-1.06%) ⬇️
plugins/modules/mysql_role.py 69.13% <50.00%> (-12.28%) ⬇️
plugins/modules/mysql_info.py 78.66% <83.78%> (-2.53%) ⬇️
plugins/module_utils/user.py 80.49% <74.54%> (-4.80%) ⬇️

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@laurent-indermuehle
Copy link
Collaborator Author

Ready for review please, if someone has the time, I would really appreciate another pair of eyes.
The only test that fails is sanity for devel which is the brand new ansible-core 2.17. We can fix the syntax for devel in a later PR.

@laurent-indermuehle
Copy link
Collaborator Author

I'm starting to use this feature (from my fork until next release) and noticed two errors in the keys I used for the output that didn't match what mysql_user is expecting as an input. It's now fixed.

I can now say it works perfectly. I just exported 300 accounts and re-create them using mysql_user in check mode. All green :)

Copy link
Collaborator

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@laurent-indermuehle great undertaking, thanks!

plugins/modules/mysql_info.py Outdated Show resolved Hide resolved
changelogs/fragments/lie_mysql_info_users_privs.yml Outdated Show resolved Hide resolved
plugins/module_utils/mysql.py Outdated Show resolved Hide resolved
plugins/module_utils/user.py Outdated Show resolved Hide resolved
@laurent-indermuehle
Copy link
Collaborator Author

Thanks @Andersson007 for your review.

Regarding your comment about MySQL 8 being already sorted, I copied the output into my editor. So MySQL must have sorted the privileges for me.

But I just discovered an issue with my implementation. Because I sorted the privileges, I get perma-yellow because the order of the privileges don't match the server output anymore.

What should I do? Fix my function to not sort the output, or fix mysql_user.changed to always sort privileges before comparison?

@laurent-indermuehle
Copy link
Collaborator Author

I've pushed a new test that fails due to uppercase fields as described in #399.
This PR is blocked and needs to be rebases onto #569. I'll try to work on that soon.

@Andersson007
Copy link
Collaborator

What should I do? Fix my function to not sort the output, or fix mysql_user.changed to always sort privileges before comparison?

Sorting sounds like a safe solution to me

@Andersson007
Copy link
Collaborator

@laurent-indermuehle i would also suggest implementing testing in a separate PR and then rebasing this one after we merge the testing-related one (i.e. not to keep not tightly related things together)

@laurent-indermuehle
Copy link
Collaborator Author

laurent-indermuehle commented Oct 9, 2023

Not sure why sanity 2.17 complains here, on my computer it works.

After a second look, I now see that I didn't sort the privileges! I do it only during comparison with mysql8_all_privilege. I've tested on 33 MariaDB servers, all reports all green! :)

I've changed mysql8_all_privilege to a dict as you proposed @Andersson007. I hope it's what you meant.

What do you mean by "testing in a separate PR" @Andersson007?

@Andersson007
Copy link
Collaborator

@laurent-indermuehle

What do you mean by "testing in a separate PR" @Andersson007?

I think i saw some test matrix changes here earlier but now i don't:) so just ignore this statement

@laurent-indermuehle
Copy link
Collaborator Author

@Andersson007 ah right, that was the case before I revert my commit were I merge main into that branch.

But now MySQL 8.0.31 fails and I don't know why :(

@laurent-indermuehle
Copy link
Collaborator Author

I found what is the weird list of "ALL PRIVILEGES" MySQL 8 is spitting: They are either static (SELECT, INSERT, ...) or dynamic (APPLICATION_PASSWORD_ADMIN, ...). The dynamic privileges are only loaded if the plugin that use them is enabled. More information here: https://dev.mysql.com/doc/refman/8.0/en/privileges-provided.html#static-dynamic-privileges

So that explains why "SHOW GRANTS FOR" was displaying 2 lists.

The issue is that I broke everything for 8.0 : When you create a user with all privileges, it got created with only the dynamic privileges. All statics are overwritten by USAGE:

GRANT USAGE ON *.* TO `user1`@`%`
GRANT APPLICATION_PASSWORD_ADMIN,[...] ON *.* TO `user1`@`%`
-- Instead of
GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, [...] ON *.* TO `user1`@`%`
GRANT APPLICATION_PASSWORD_ADMIN,[...] ON *.* TO `user1`@`%`

We have 2 functions in module_utils/mysql.py: user_add and user_mod. I've a hard time figuring out why creating a brand new user in mysql_db integrations tests call the user_mod one. Here is the output of the tests:

TASK [test_mysql_db : State Present Absent | Create user1 to access database dbuser1] ***
changed: [testhost] => {"changed": true, "msg":  
"Privileges updated: granted [], revoked ['SHOW VIEW', 'INDEX', 'UPDATE', 'EVENT',   
'LOCK TABLES', 'CREATE TABLESPACE', 'CREATE USER', 'RELOAD', 'REPLICATION SLAVE',  
 'CREATE ROUTINE', 'SHOW DATABASES', 'CREATE ROLE', 'CREATE TEMPORARY TABLES',  
 'DELETE', 'ALTER', 'TRIGGER', 'SUPER', 'CREATE', 'ALTER ROUTINE', 'REFERENCES', 'INSERT',  
 'EXECUTE', 'FILE', 'SHUTDOWN', 'DROP ROLE', 'PROCESS', 'SELECT', 'DROP',  
 'REPLICATION CLIENT', 'CREATE VIEW']", "password_changed": false, "user": "user1"}

This is so wrong...

@laurent-indermuehle
Copy link
Collaborator Author

@Andersson007 no need to search anymore. I just found the issue. Thanks anyway.

The tests use a loop. So the second iteration uses user_mod().
I'm adding an option to privileges_get() to not summarize ALL privileges when we need to compare actual privileges with the ones provided by Ansible.

And also, I used to disable many tests to speed up my feedback loop, but this time it masked many regressions I introduced. Slow tests makes me do stupid things -_-'

This debug output this:

[
  {'Grants for root@localhost': \"GRANT ALL PRIVILEGES ON *.* TO 'root'@'localhost' WITH GRANT OPTION\"},
  {'Grants for root@localhost': \"GRANT PROXY ON ''@'' TO 'root'@'localhost' WITH GRANT OPTION\"}
],
[
  {'Grants for mysql.session@localhost': \"GRANT SUPER ON *.* TO 'mysql.session'@'localhost'\"},
  {'Grants for mysql.session@localhost': \"GRANT SELECT ON `performance_schema`.* TO 'mysql.session'@'localhost'\"},
  {'Grants for mysql.session@localhost': \"GRANT SELECT ON `mysql`.`user` TO 'mysql.session'@'localhost'\"}
 ],
 [
   {'Grants for mysql.sys@localhost': \"GRANT USAGE ON *.* TO 'mysql.sys'@'localhost'\"},
   {'Grants for mysql.sys@localhost': \"GRANT TRIGGER ON `sys`.* TO 'mysql.sys'@'localhost'\"},
   {'Grants for mysql.sys@localhost': \"GRANT SELECT ON `sys`.`sys_config` TO 'mysql.sys'@'localhost'\"}
 ],
 [
   {'Grants for root@%': \"GRANT ALL PRIVILEGES ON *.* TO 'root'@'%' WITH GRANT OPTION\"}
 ]

 I think something is wrong in the lambda and when grants art on ''@''.
laurent-indermuehle and others added 26 commits October 11, 2023 16:51
Even if you use print_identified_with_as_hex to prevent the password to
contains binary characters. I think this is because PyMySQL and
mysqlclient wrap the password in quote. We could try to convert to
another python connector, like mysql-connector-python.
This reverts commit 77593b7.
I imported PR569 to be able to test on my fork, but to avoid polluting
this patch I now revert those commit.
We summarize ALL for mysql_info, but mysql_user needs to compare
actual privileges with the ones provided by ansible.
This was introduced in ansible-collections#189. To my knowledge, there is no difference
between MySQL and MariaDB regarding roles or when you call a user by
its name alone. Both works if the host it '%'. Same for roles.
This reverts commit de1cbae.
Mariadb write the 'host' of a role as '' while mysql write '%'.
@laurent-indermuehle laurent-indermuehle force-pushed the lie_mysql_info_users_privs branch from de1cbae to 2e2780b Compare October 11, 2023 14:52
@laurent-indermuehle
Copy link
Collaborator Author

This PR started before others PR, merged since, required for this one.
Also, I missed an important fact right at the beginning that made things very hard. I'll explain bellow.
So I'm closing this PR and opening a new one to start fresh with fewer commits and a minimum of changes.

So, for the explanation: The issue was that mysql_user instanciate a cursor with the class Cursor, while mysql_info chose the class DictCursor.
So anytime I was calling a method that do a cursor.fetchone(), I was getting a dictionary instead of a list. This explains the "KeyError 0" I was getting from almost every functions from module_utils.mysql and module_utils.user!

@laurent-indermuehle laurent-indermuehle deleted the lie_mysql_info_users_privs branch October 12, 2023 12:03
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.

3 participants