feat[mysql_user]: add support for mysql user attributes #604

n-cc · 2024-01-10T20:44:18Z

SUMMARY

This PR adds support for MySQL user attributes (essentially arbitrary comments associated with a user in a key: value format) to the mysql_user module. It adds the ability to add, update, and delete user attributes, and get existing attributes by checking the attributes attribute of the mysql_user task.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

community.mysql.mysql_user

ADDITIONAL INFORMATION

Previously, user_mod would prematurely return at various points in check mode when changes were detected. IMO, this is not ideal, as the entirety of the module that will execute in non-check mode is not ran in check mode, and it means we have to support returning a specifically-constructed dict in multiple locations. As part of this PR, since I am modifying the return dict of user_mod, I removed the premature returns when in check mode in favor of the final return at the end of the function. I can revert this if desired, but I think it makes the code cleaner and safer. It is worth noting that messages from the module are still masked if a later message is registered.
Return values are not documented yet, so I didn't document the new attributes return value, but I could add return value documentation if desired.

LOCAL TESTING

SERVER SUPPORTS ATTRIBUTES

Creating a new user without attributes:

$ ansible -m mysql_user -a '{ "user": "newuser1", "host": "localhost" }' remote
remote | CHANGED => {
    "attributes": {},
    "changed": true,
    "msg": "User added",
    "password_changed": true,
    "user": "newuser1"
}
$

Creating a new user with attributes:

$ ansible -m mysql_user -a '{ "user": "newuser2", "host": "localhost", "attributes": { "key": "value" } }' remote
remote | CHANGED => {
    "attributes": {
        "key": "value"
    },
    "changed": true,
    "msg": "User added",
    "password_changed": true,
    "user": "newuser2"
}
$

Updating an existing user's attributes:

$ ansible -m mysql_user -a '{ "user": "newuser2", "host": "localhost", "attributes": { "key": "value_changed", "new_key": "new_value" } }' remote
remote | CHANGED => {
    "attributes": {
        "key": "value_changed",
        "new_key": "new_value"
    },
    "changed": true,
    "msg": "Attributes updated: key: value_changed, new_key: new_value",
    "password_changed": false,
    "user": "newuser2"
}
$

No change when existing attributes are already in line with the attributes specified in the attributes parameter:

$ ansible -m mysql_user -a '{ "user": "newuser2", "host": "localhost", "attributes": { "key": "value_changed", "new_key": "new_value" } }' remote
remote | SUCCESS => {
    "attributes": {
        "key": "value_changed",
        "new_key": "new_value"
    },
    "changed": false,
    "msg": "User unchanged",
    "password_changed": false,
    "user": "newuser2"
}
$

No change when the attributes parameter is not specified:

$ ansible -m mysql_user -a '{ "user": "newuser2", "host": "localhost" }' remote
remote | SUCCESS => {
    "attributes": {
        "key": "value_changed",
        "new_key": "new_value"
    },
    "changed": false,
    "msg": "User unchanged",
    "password_changed": false,
    "user": "newuser2"
}
$

SERVER DOES NOT SUPPORT ATTRIBUTES

Creating a new user without attributes:

$ ansible -m mysql_user -a '{ "user": "newuser1", "host": "localhost" }' remote
remote | CHANGED => {
    "attributes": {},
    "changed": true,
    "msg": "User added",
    "password_changed": true,
    "user": "newuser1"
}
$

Reading an existing user:

$ ansible -m mysql_user -a '{ "user": "newuser1", "host": "localhost" }' remote
remote | SUCCESS => {
    "attributes": {},
    "changed": false,
    "msg": "User unchanged",
    "password_changed": false,
    "user": "newuser1"
}
$

Creating a new user with attributes:

$ ansible -m mysql_user -a '{ "user": "newuser2", "host": "localhost", "attributes": { "key": "value" } }' remote
remote | FAILED! => {
    "changed": false,
    "msg": "user attributes were specified but the mysql server does not support user attributes"
}
$

Updating an existing user's attributes:

$ ansible -m mysql_user -a '{ "user": "newuser1", "host": "localhost", "attributes": { "key": "value" } }' remote
remote | FAILED! => {
    "changed": false,
    "msg": "user attributes were specified but the mysql server does not support user attributes"
}
$

n-cc · 2024-01-10T20:45:37Z

plugins/modules/mysql_user.py

+      - MySQL server must support the INFORMATION_SCHEMA.USER_ATTRIBUTES table. Provided since MySQL 8.0.
+      - To delete an existing attribute, set its value to False.
+    type: dict
+    version_added: '3.9.0'


I did not bump the version in galaxy.yml yet; unsure what the process is for shipping new versions.

3.9.0 is OK here

n-cc · 2024-01-10T20:46:06Z

plugins/modules/mysql_user.py

@@ -405,6 +418,7 @@ def main():
        tls_requires=dict(type='dict'),
        append_privs=dict(type='bool', default=False),
        subtract_privs=dict(type='bool', default=False),
+        attributes=dict(type='dict'),


I can change the parameter ordering here if desired.

that's fine now

n-cc · 2024-01-10T22:52:56Z

I'll work on resolving the lint and integration tests over the next day or so.

codecov · 2024-01-11T16:28:40Z

Codecov Report

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

Comparison is base (81ab18d) 76.66% compared to head (208719f) 75.13%.

Files	Patch %	Lines
plugins/module_utils/user.py	77.50%	12 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #604      +/-   ##
==========================================
- Coverage   76.66%   75.13%   -1.53%     
==========================================
  Files          28       18      -10     
  Lines        2447     2361      -86     
  Branches      603      595       -8     
==========================================
- Hits         1876     1774     -102     
- Misses        388      405      +17     
+ Partials      183      182       -1

Flag	Coverage Δ
integration	`74.50% <77.64%> (+0.33%)`	⬆️
sanity	`?`
units	`?`

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

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

n-cc · 2024-01-11T16:52:40Z

Existing integration tests are passing, going to work on writing new ones for this feature.

n-cc · 2024-01-11T23:26:15Z

Integration tests written, everything should be good for a review at this point.

Andersson007

@n-cc hello, thanks for the PR!
could you please add a changelog fragment?
Also will you be responsive in case of related bug reports? If not really, should we implement it somehow separately from all those functions you're changing here? But i hope you're going to be around and even join our Matrix channel and Forum group:) See the communication guide.

tests/integration/targets/test_mysql_user/tasks/test_user_attributes.yml

plugins/modules/mysql_user.py

Andersson007 · 2024-01-12T06:58:23Z

plugins/module_utils/user.py

+    if r:
+        attributes = r[0]
+        # convert JSON string stored in row into a dict - mysql enforces that user_attributes entires are in JSON format
+        if attributes:
+            return json.loads(attributes)
+        else:
+            return {}
+
+    return None


Suggested change

if r:

attributes = r[0]

# convert JSON string stored in row into a dict - mysql enforces that user_attributes entires are in JSON format

if attributes:

return json.loads(attributes)

else:

return {}

return None

# convert JSON string stored in row into a dict - mysql enforces that user_attributes entires are in JSON format

return json.loads(r[0]) if r and r[0] else None

Looks like this will return None rather than an empty dict if the user exists but does not have any attributes set - are you recommending returning None over {} in that scenario?

That should be fine, but there are a few things to keep in mind about None vs {} when it comes to attributes... if a CREATE USER is ran without the ATTRIBUTES '{}' option, the user's attributes are stored as NULL until attributes are set, and if all attributes are deleted, attributes will then be stored as {} and cannot be changed back to NULL (as far as I can tell):

mysql> CREATE USER 'testuser'@'localhost'; Query OK, 0 rows affected (0.00 sec) mysql> SELECT ATTRIBUTE from INFORMATION_SCHEMA.USER_ATTRIBUTES WHERE user = 'testuser' and host = 'localhost'; +-----------+ | ATTRIBUTE | +-----------+ | NULL | +-----------+ 1 row in set (0.00 sec) mysql> ALTER USER 'testuser'@'localhost' ATTRIBUTE '{ "foo": "bar" }'; Query OK, 0 rows affected (0.00 sec) mysql> SELECT ATTRIBUTE from INFORMATION_SCHEMA.USER_ATTRIBUTES WHERE user = 'testuser' and host = 'localhost'; +----------------+ | ATTRIBUTE | +----------------+ | {"foo": "bar"} | +----------------+ 1 row in set (0.00 sec) mysql> ALTER USER 'testuser'@'localhost' ATTRIBUTE '{ "foo": null }'; Query OK, 0 rows affected (0.00 sec) mysql> SELECT ATTRIBUTE from INFORMATION_SCHEMA.USER_ATTRIBUTES WHERE user = 'testuser' and host = 'localhost'; +-----------+ | ATTRIBUTE | +-----------+ | {} | +-----------+ 1 row in set (0.00 sec) mysql>

I think there are a couple different options:

Represent no attributes as {} even if the ATTRIBUTE database value is NULL

This is what I was doing originally; I chose this method so that someone using register: foo with mysql_user could still treat foo.attributes as a dictionary even if the ATTRIBUTE value is NULL. It also makes the code a bit cleaner if attributes_get always returns a dict for users that exist.

With this method, we could also explicitly run the CREATE USER query in user_add with ATTRIBUTES {} even if no attributes are specified, so that the ATTRIBUTE database value will always be {} for ansible-created users on servers with attribute support.

Represent no attributes as None even if the ATTRIBUTE database value is {} (I think this is what you were proposing)

Represent no attributes as either None or {} depending on the database value

Option 2 is how this feature behaves as of my latest commit (I also wrote tests to assert the attributes return value for users without attributes). I think any of these options are fine depending on ansible best practices. I can stick with option 2 if you think it's preferred, I just wanted to make sure this caveat was documented in this PR.

Andersson007 · 2024-01-12T06:58:59Z

plugins/modules/mysql_user.py

+      - MySQL server must support the INFORMATION_SCHEMA.USER_ATTRIBUTES table. Provided since MySQL 8.0.
+      - To delete an existing attribute, set its value to False.
+    type: dict
+    version_added: '3.9.0'


3.9.0 is OK here

Andersson007 · 2024-01-12T06:59:21Z

plugins/modules/mysql_user.py

@@ -405,6 +418,7 @@ def main():
        tls_requires=dict(type='dict'),
        append_privs=dict(type='bool', default=False),
        subtract_privs=dict(type='bool', default=False),
+        attributes=dict(type='dict'),


that's fine now

plugins/module_utils/user.py

n-cc · 2024-01-12T17:59:44Z

Also will you be responsive in case of related bug reports?

Certainly if there are any immediate problems, but you could also consider me a maintainer of this feature if future problems arise. I don't actively use irc/matrix, but I should be reachable via Github pings.

That being said, if implementing attribute support separately from user_mod and user_add is preferable, I can do that, I just opted to include everything in user_mod as it seems like that's where all user modification is handled currently.

n-cc · 2024-01-16T20:28:21Z

Requested changes have been implemented, aside from the two I had questions on.

Andersson007 · 2024-01-17T13:40:35Z

Also will you be responsive in case of related bug reports?

Certainly if there are any immediate problems, but you could also consider me a maintainer of this feature if future problems arise. I don't actively use irc/matrix, but I should be reachable via Github pings.

That being said, if implementing attribute support separately from user_mod and user_add is preferable, I can do that, I just opted to include everything in user_mod as it seems like that's where all user modification is handled currently.

that's nice to hear! Could you please take a look at the comment about using None. thanks

n-cc · 2024-01-17T21:24:46Z

Also will you be responsive in case of related bug reports?

Certainly if there are any immediate problems, but you could also consider me a maintainer of this feature if future problems arise. I don't actively use irc/matrix, but I should be reachable via Github pings.
That being said, if implementing attribute support separately from user_mod and user_add is preferable, I can do that, I just opted to include everything in user_mod as it seems like that's where all user modification is handled currently.

that's nice to hear! Could you please take a look at the comment about using None. thanks

Responded and updated.

Andersson007

@n-cc one small thing to change, the rest LGTM

CHANGELOG.rst

n-cc · 2024-01-18T20:00:34Z

@n-cc one small thing to change, the rest LGTM

Thanks much, updated

Andersson007 · 2024-01-19T12:09:26Z

@laurent-indermuehle is it OK for you? if yes, please merge

Andersson007 · 2024-01-19T14:37:25Z

as @laurent-indermuehle is busy, I'm merging this PR

@n-cc thanks for the contribution!
@laurent-indermuehle thanks for reviewing!

add support for mysql user attributes

5273712

n-cc commented Jan 10, 2024

View reviewed changes

n-cc force-pushed the ncc-user-attributes branch 2 times, most recently from 4e146a2 to 8685d4a Compare January 10, 2024 22:25

n-cc force-pushed the ncc-user-attributes branch from 8685d4a to e7f707f Compare January 11, 2024 16:21

fix CI

7012a27

n-cc force-pushed the ncc-user-attributes branch from e7f707f to 7012a27 Compare January 11, 2024 16:32

n-cc force-pushed the ncc-user-attributes branch 8 times, most recently from 3467950 to 0e56260 Compare January 11, 2024 22:55

write integration tests

9a8b53d

n-cc force-pushed the ncc-user-attributes branch from 0e56260 to 9a8b53d Compare January 11, 2024 23:16

Andersson007 reviewed Jan 12, 2024

View reviewed changes

n-cc force-pushed the ncc-user-attributes branch 2 times, most recently from 0119896 to 9eb4cc3 Compare January 16, 2024 19:55

requested changes pt. 1

c35bfb9

n-cc force-pushed the ncc-user-attributes branch from 9eb4cc3 to c35bfb9 Compare January 16, 2024 20:13

n-cc requested a review from Andersson007 January 16, 2024 20:28

n-cc force-pushed the ncc-user-attributes branch 6 times, most recently from 6c4c394 to b84d8fb Compare January 17, 2024 21:01

requested changes pt. 2

55e896e

n-cc force-pushed the ncc-user-attributes branch from b84d8fb to 55e896e Compare January 17, 2024 21:11

Andersson007 requested changes Jan 18, 2024

View reviewed changes

CHANGELOG.rst Outdated Show resolved Hide resolved

fix changelog fragment

208719f

n-cc requested a review from Andersson007 January 18, 2024 20:00

Andersson007 approved these changes Jan 19, 2024

View reviewed changes

Andersson007 merged commit 051aa48 into ansible-collections:main Jan 19, 2024
40 of 41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[mysql_user]: add support for mysql user attributes #604

feat[mysql_user]: add support for mysql user attributes #604

n-cc commented Jan 10, 2024 •

edited

Loading

n-cc Jan 10, 2024

Andersson007 Jan 12, 2024

n-cc Jan 10, 2024

Andersson007 Jan 12, 2024

n-cc commented Jan 10, 2024

codecov bot commented Jan 11, 2024 •

edited

Loading

n-cc commented Jan 11, 2024

n-cc commented Jan 11, 2024

Andersson007 left a comment

Andersson007 Jan 12, 2024

n-cc Jan 12, 2024

Andersson007 Jan 17, 2024

n-cc Jan 17, 2024

Andersson007 Jan 12, 2024

Andersson007 Jan 12, 2024

n-cc commented Jan 12, 2024

n-cc commented Jan 16, 2024

Andersson007 commented Jan 17, 2024

n-cc commented Jan 17, 2024

Andersson007 left a comment

n-cc commented Jan 18, 2024

Andersson007 commented Jan 19, 2024

Andersson007 commented Jan 19, 2024

feat[mysql_user]: add support for mysql user attributes #604

feat[mysql_user]: add support for mysql user attributes #604

Conversation

n-cc commented Jan 10, 2024 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

LOCAL TESTING

SERVER SUPPORTS ATTRIBUTES

SERVER DOES NOT SUPPORT ATTRIBUTES

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-cc commented Jan 10, 2024

codecov bot commented Jan 11, 2024 • edited Loading

Codecov Report

n-cc commented Jan 11, 2024

n-cc commented Jan 11, 2024

Andersson007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-cc commented Jan 12, 2024

n-cc commented Jan 16, 2024

Andersson007 commented Jan 17, 2024

n-cc commented Jan 17, 2024

Andersson007 left a comment

Choose a reason for hiding this comment

n-cc commented Jan 18, 2024

Andersson007 commented Jan 19, 2024

Andersson007 commented Jan 19, 2024

n-cc commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading