mysql_user - added queries to module output and set no_log option on … #92

steveteahan · 2021-01-16T14:01:28Z

…plugin_hash_string and plugin_auth_string (#75)

SUMMARY

Fixes #75

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

mysql_user

ADDITIONAL INFORMATION

There will probably be a couple of uncovered areas which would be resolved either by dropped MySQL <5.7 support or adding tests for MySQL 5.6. Tracking a discussion of that topic in Supported database versions and integration testing #91.
In user_mod I skipped adding the queries in an exception case because it currently doesn't have test coverage and I couldn't reproduce the issue in either MySQL 5.7 or 8.0. The code in question is here: https://github.com/ansible-collections/community.mysql/blob/main/plugins/modules/mysql_user.py#L601

…plugin_hash_string and plugin_auth_string (ansible-collections#75)

codecov · 2021-01-16T14:03:31Z

Codecov Report

Merging #92 (3474320) into main (b25fb59) will decrease coverage by 0.40%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
- Coverage   75.07%   74.67%   -0.41%     
==========================================
  Files          12       12              
  Lines        1645     1674      +29     
  Branches      423      425       +2     
==========================================
+ Hits         1235     1250      +15     
- Misses        267      280      +13     
- Partials      143      144       +1

Impacted Files	Coverage Δ
plugins/modules/mysql_user.py	`79.00% <84.21%> (+0.43%)`	⬆️
plugins/module_utils/mysql.py	`74.69% <0.00%> (-8.44%)`	⬇️
plugins/modules/mysql_query.py	`86.58% <0.00%> (-2.44%)`	⬇️
plugins/modules/mysql_db.py	`74.21% <0.00%> (-0.35%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b25fb59...3474320. Read the comment docs.

steveteahan · 2021-01-16T14:34:21Z

Looks like mysqlclient doesn't have a mogrify method:

AttributeError: 'Cursor' object has no attribute 'mogrify'

I'll have to take a look at what the other options are. It seems like in other areas of the project the cursor private variables are being used which isn't ideal. In others, it seems to be simple string replacement which from my understanding won't work for all cases.

steveteahan · 2021-01-16T17:20:21Z

I created an issue for the mysqlclient project to see if adding mogrify would be considered: PyMySQL/mysqlclient#476

That seems like one of the better solutions that I can think of at the moment. The worst-case could be to only log the queries output if the MySQL connector in use supports mogrify?

steveteahan · 2021-01-19T11:42:44Z

I have a PR up for review for the mysqlclient change. Even when that is implemented it will only work for the latest version when released, so I'm thinking that I'll still check the cursor for mogrify() and if it doesn't exist simply add a warning to the output.

changelogs/fragments/75-mysql_user_queries_output.yml

Andersson007 · 2021-01-20T10:17:18Z

plugins/modules/mysql_user.py

-RETURN = '''#'''
+RETURN = r'''
+queries:
+  description: List of executed queries which modified DB's state.


Suggested change

description: List of executed queries which modified DB's state.

description: List of executed queries which modified a state of a database.

We shouldn't use abbreviations if possible

I copied that from mysql_replication.py for consistency, but agree that it could be better. I'll fix it.

plugins/modules/mysql_user.py

Andersson007 · 2021-01-20T10:44:46Z

@steveteahan thanks for working on this!
I'm not sure about the warning - it could annoy users who don't need to see any returned values.
Maybe:

check if mogrify() is supported once in the main by catching the exception, define a boolean variable to indicate that
if not supported, append to _executed_queries something like meaning "You MySQL driver doesn't support ... Please use ..."
pass a boolean flag to log_query() function and just return with no action OR add a condition not to invoke the function at all

Just a suggestion, I don't feel fresh now:)

Also the codecov-related tests should be green.

Andersson007 · 2021-01-20T10:53:13Z

BTW i don't see a good explanation in the module's documentation of what plugin_hash_string and plugin_auth_string are about. Even only one example is provided.
Does it really make sense to hide them in output?

Co-authored-by: Andrew Klychkov <[email protected]>

steveteahan · 2021-01-20T12:06:25Z

@steveteahan thanks for working on this!
I'm not sure about the warning - it could annoy users who don't need to see any returned values.
Maybe:

1. check if `mogrify()` is supported once in the `main` by catching the exception, define a boolean variable to indicate that

2. if not supported, append to `_executed_queries` something like meaning "You MySQL driver doesn't support ... Please use ..."

3. pass a boolean flag to `log_query()` function and just return with no action OR add a condition not to invoke the function at all

Moving away from a warning seems reasonable to me if your experience in the community has been that it is more of annoyance than doing any good. I can do something like what is suggested, but maybe leave the appending to queries until the very end where I'm currently calling module.warn. That way the message would only show up once instead of many times.

Also the codecov-related tests should be green.

One of the issues is not testing MySQL 5.6 which I tried to start a discussion about in #91. The other part was never covered, but I'll try to get a test together for it.

BTW i don't see a good explanation in the module's documentation of what plugin_hash_string and plugin_auth_string are about. Even only one example is provided.
Does it really make sense to hide them in output?

plugin_auth_string is a plaintext password and plugin_hash_string is in the format that the database expects it to be in (generally hashed). This is covered in a bit more detail here. From that perspective, I think it definitely makes sense to hide them from output.

steveteahan · 2021-01-20T13:16:08Z

plugins/modules/mysql_user.py

@@ -592,11 +633,15 @@ def user_mod(cursor, user, host, host_all, password, encrypted,
                if module.check_mode:
                    return (True, msg)
                if old_user_mgmt:
-                    cursor.execute("SET PASSWORD FOR %s@%s = %s", (user, host, encrypted_password))
+                    query = "SET PASSWORD FOR %s@%s = %s"


This is the example of the missing coverage for MySQL 5.6 that I mentioned above.

I did a bit of research and enabling integration tests for MySQL 5.6 is not going to be trivial (see #91). old_user_mgmt handles cases where MariaDB < 10.2 and MySQL < 5.7. The only version in that set that is still supported is MySQL 5.6 (for another two weeks). I don't see it being worthwhile spending any time on this. As per the discussion is #91 it might be worthwhile to consider dropping this support and removing the code in the near future.

Unless there are other suggestions, I can go ahead and remove this change. The downside is that until we remove MariaDB < 10.2 and MySQL < 5.7 code paths, there will be queries that can modify the database state, but will not be logged via the module.

Andersson007 · 2021-01-21T07:08:11Z

I can do something like what is suggested, but maybe leave the appending to queries until the very end where I'm currently calling module.warn.

I meant it'll be append once, then will be skipped. But feel free to implement that as you like, will see

plugin_auth_string is a plaintext password and plugin_hash_string is in the format that the database expects it to be in (generally hashed). This is covered in a bit more detail here. From that perspective, I think it definitely makes sense to hide them from output.

If so, sounds sensible

steveteahan · 2021-11-20T13:22:25Z

The mogrify change hasn’t been merged into mysqlclient yet. I also don’t have an immediate need for this change any longer. I still think it’s a change that adds value. I’ll leave this PR open and leave it up to the maintainers to decide if it should stay open for someone else to push over the line once mysqlclient is ready, or if this should just be closed.

Thanks!

Jorge-Rodriguez · 2021-11-28T11:30:24Z

@Andersson007 @steveteahan The codebase has changed dramatically since opening this PR. At this point I don't know if rebasing is going to be any easier than replicating the work on top of the current head.

Andersson007 · 2021-11-29T09:05:19Z

@Andersson007 @steveteahan The codebase has changed dramatically since opening this PR. At this point I don't know if rebasing is going to be any easier than replicating the work on top of the current head.

Agreed

steveteahan · 2022-01-05T21:20:55Z

Closing this PR based on the feedback above.

Andersson007 · 2022-01-06T07:36:28Z

@steveteahan thank you!

mysql_user - added queries to module output and set no_log option on …

53e9016

…plugin_hash_string and plugin_auth_string (ansible-collections#75)

Andersson007 reviewed Jan 20, 2021

View reviewed changes

Apply suggestions from code review

3474320

Co-authored-by: Andrew Klychkov <[email protected]>

steveteahan commented Jan 20, 2021

View reviewed changes

Jorge-Rodriguez added the needs-rebase label Nov 28, 2021

steveteahan closed this Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mysql_user - added queries to module output and set no_log option on … #92

mysql_user - added queries to module output and set no_log option on … #92

steveteahan commented Jan 16, 2021

codecov bot commented Jan 16, 2021 •

edited

Loading

steveteahan commented Jan 16, 2021 •

edited

Loading

steveteahan commented Jan 16, 2021

steveteahan commented Jan 19, 2021

Andersson007 Jan 20, 2021

steveteahan Jan 20, 2021

Andersson007 commented Jan 20, 2021

Andersson007 commented Jan 20, 2021

steveteahan commented Jan 20, 2021 •

edited

Loading

steveteahan Jan 20, 2021 •

edited

Loading

Andersson007 commented Jan 21, 2021

steveteahan commented Nov 20, 2021

Jorge-Rodriguez commented Nov 28, 2021

Andersson007 commented Nov 29, 2021

steveteahan commented Jan 5, 2022

Andersson007 commented Jan 6, 2022

	description: List of executed queries which modified DB's state.
	description: List of executed queries which modified a state of a database.

mysql_user - added queries to module output and set no_log option on … #92

mysql_user - added queries to module output and set no_log option on … #92

Conversation

steveteahan commented Jan 16, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

codecov bot commented Jan 16, 2021 • edited Loading

Codecov Report

steveteahan commented Jan 16, 2021 • edited Loading

steveteahan commented Jan 16, 2021

steveteahan commented Jan 19, 2021

Andersson007 Jan 20, 2021

Choose a reason for hiding this comment

steveteahan Jan 20, 2021

Choose a reason for hiding this comment

Andersson007 commented Jan 20, 2021

Andersson007 commented Jan 20, 2021

steveteahan commented Jan 20, 2021 • edited Loading

steveteahan Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Andersson007 commented Jan 21, 2021

steveteahan commented Nov 20, 2021

Jorge-Rodriguez commented Nov 28, 2021

Andersson007 commented Nov 29, 2021

steveteahan commented Jan 5, 2022

Andersson007 commented Jan 6, 2022

codecov bot commented Jan 16, 2021 •

edited

Loading

steveteahan commented Jan 16, 2021 •

edited

Loading

steveteahan commented Jan 20, 2021 •

edited

Loading

steveteahan Jan 20, 2021 •

edited

Loading