login tracking [Temporarily frozen]

[Jorge-Rodriguez]

SUMMARY

Introduce support for MySQL >= 8.0.19 failed login tracking and account locking Fixes #49

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

mysql_user

[codecov[bot]]

Codecov Report

Merging #62 (f43676c) into main (31bd2b5) will decrease coverage by 59.42%. The diff coverage is 5.26%.

@@             Coverage Diff             @@
##             main      #62       +/-   ##
===========================================
- Coverage   75.07%   15.64%   -59.43%     
===========================================
  Files          12        8        -4     
  Lines        1645     1016      -629     
  Branches      423      246      -177     
===========================================
- Hits         1235      159     -1076     
- Misses        267      854      +587     
+ Partials      143        3      -140     

Impacted Files	Coverage Δ
plugins/modules/mysql_user.py	8.91% <5.26%> (-69.66%)	:arrow_down:
plugins/modules/mysql_replication.py	11.96% <0.00%> (-58.31%)	:arrow_down:
plugins/module_utils/mysql.py	28.91% <0.00%> (-54.22%)	:arrow_down:
plugins/module_utils/database.py	16.82% <0.00%> (-12.15%)	:arrow_down:
plugins/modules/mysql_info.py
plugins/modules/mysql_variables.py
plugins/modules/mysql_db.py
plugins/modules/mysql_query.py

---

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 31bd2b5...f43676c. Read the comment docs.

[Andersson007]

@martinwalle could you please take a look? If you don't know python much, could you look, at least, at tests/integration/targets/test_mysql_user/tasks/issue-49.yml which contains examples of how to use the feature. Maybe EXAMPLES block of module's file would be also helpful, and the option's description in the DOCUMENTATION section.

@bmildren @bmalynovytch could you also please take a look?

[martinwalle]

@martinwalle could you please take a look? If you don't know python much, could you look, at least, at tests/integration/targets/test_mysql_user/tasks/issue-49.yml which contains examples of how to use the feature. Maybe EXAMPLES block of module's file would be also helpful, and the option's description in the DOCUMENTATION section.

@bmildren @bmalynovytch could you also please take a look?

Hello @Andersson007 ,

I read the tests and the arguments is good for me. May I test it to validate ?

Sincerely, Martin.

[Jorge-Rodriguez]

Just need to add the conditional assertions for versions that do not support this feature

[Andersson007]

@martinwalle could you please take a look? If you don't know python much, could you look, at least, at tests/integration/targets/test_mysql_user/tasks/issue-49.yml which contains examples of how to use the feature. Maybe EXAMPLES block of module's file would be also helpful, and the option's description in the DOCUMENTATION section. @bmildren @bmalynovytch could you also please take a look?

Hello @Andersson007 ,

I read the tests and the arguments is good for me. May I test it to validate ?

Sincerely, Martin.

Hey @martinwalle ! It would be great, if you try the changes manually, though I'm not sure @Jorge-Rodriguez has done all what he intended. @Jorge-Rodriguez when it's ready for review, please put something here. And thanks for steadily working on improving mysql-specific support!

[Jorge-Rodriguez]

@Andersson007 @martinwalle only test fixes are missing, the functional code is ready for manual testing.

[Andersson007]

@Jorge-Rodriguez thanks for the information! Would be happy to review once the tests are green @martinwalle so feel free to try the changes

[martinwalle]

@Jorge-Rodriguez @Andersson007 I tested mysql_user with these vars :

 account_locking:
      FAILED_LOGIN_ATTEMPTS: 3
      PASSWORD_LOCK_TIME: 5

  account_locking:
      FAILED_LOGIN_ATTEMPTS: 8
      PASSWORD_LOCK_TIME: 1

   account_locking:
      FAILED_LOGIN_ATTEMPTS: 1
      PASSWORD_LOCK_TIME: UNBOUNDED

    account_locking:
      FAILED_LOGIN_ATTEMPTS: 1
      PASSWORD_LOCK_TIME: 32768
	  
	  {"changed": false, "msg": "Account locking values are out of the valid range (0-32767)"}

All is fine. It is working as expected. I also test the failed-login. And it worked ! Nice work :)

[Andersson007]

@martinwalle thanks for testing! Sorry for the delayed response, I've been waiting for Jorge's feedback. @Jorge-Rodriguez any updates on this?

[Jorge-Rodriguez]

Sorry everyone. it's been truly hectic lately and I haven't found time to fix the tests. For what it's worth, there should be nothing functionally wrong with the tests, technically speaking, the failures are valid because the tests are run on platforms that don't support the feature. I'll be adding conditionals to those tests soon.

Cheers

[Jorge-Rodriguez]

The rework needed for these tests is tangentially related to #91 so I'm looking that here as a reminder that however we decide to handle that will probably affect and require a refactor of this.

[Jorge-Rodriguez]

@Andersson007 tests are green again. Codecov notwithstanding

[Andersson007]

recheck

[Andersson007]

@Jorge-Rodriguez thanks for your effort! I'll look at the PR in detail ASAP (hope during the next several days, am in a business trip now). Do you think there's no way to improve the coverage (see the marked places)? If no, no problem, it can be often hard to reach.

[Jorge-Rodriguez]

Do you think there's no way to improve the coverage (see the marked places)? If no, no problem, it can be often hard to reach.

One branch won't be reached until we introduce MariaDB integration tests, other issues have me puzzled because I'd swear they should be covered.

I'm thinking of tackling that major MySQL/MariaDB refactor next. It makes sense to me to handle that before introducing more bug fixes in the codebase, though I'd appreciate a thumbs up on that from @bmalynovytch as well.

[bmalynovytch]

I'm thinking of tackling that major MySQL/MariaDB refactor next. It makes sense to me to handle that before introducing more bug fixes in the codebase, though I'd appreciate a thumbs up on that from @bmalynovytch as well.

Thumbs up ! 👍 👍 👍

[Andersson007]

@Jorge-Rodriguez should I wait for this PR merged or I can release community.mysql? I could wait a couple of days, no problem.

[Jorge-Rodriguez]

@Jorge-Rodriguez should I wait for this PR merged or I can release community.mysql? I could wait a couple of days, no problem.

@Andersson007 if you give me a day or two, I'll get those two issues you spotted ready and we can merge.

[Andersson007]

@Jorge-Rodriguez ok, no problem, take your time! I'd release it on Sanday

[Andersson007]

@Jorge-Rodriguez thank you for the great and regular contribution! You did and are doing really a lot!

As a gesture of trust, we're happy to give you commit priviledge in the community.mysql collection repository. It's a great responsibility, so please, first, read the Committer Guidelines carefully.

I personally found the following rule essential: "Don't merge your PRs" and everything will be fine;)

If you have any questions, feel free to ask me directly via aaklychkov at mail dot ru (and I'd be happy to have your email to contact you if really needed). Also, not to miss important announcements, would be great to subscribe to Changes impacting Collection Contributors and Maintainers issue, and Bullhorn newsletter (its Issue is here).

If you don't like to have this priviledge, please, let me know.

Thanks for the great contribution!:)

[Jorge-Rodriguez]

@Andersson007 It's a privilege and proud moment for me to receive the commit privilege on the repo.

I'll be sure to take the Guidelines to heart and use the privilege responsibly.

Many thanks.

[Jorge-Rodriguez]

@Andersson007 The password update tests show that I might have broken something in the user_mod function. If I haven't managed to push a fix by Sunday, do not wait any longer. We'll merge this on the next release.

Cheers!

[Andersson007]

@Andersson007 It's a privilege and proud moment for me to receive the commit privilege on the repo.

I'll be sure to take the Guidelines to heart and use the privilege responsibly.

Many thanks.

It's a pleasure to work with you, thanks!

If I haven't managed to push a fix by Sunday, do not wait any longer. We'll merge this on the next release.

Take your time, I'll release the collection on Sunday and then again once you complete the PR, thanks!

[Andersson007]

@Jorge-Rodriguez I've just surprisingly found out that there's nothing new to release since 1.2.0:)) so take your time

[Jorge-Rodriguez]

I just found out that the reason the tests fail is unrelated to this feature and it has to do with the priv option.

As it turns out, if a user is granted ALL privileges like so:

- mysql_user:
       [...]
        priv: '*.*:ALL'
        state: present

The current privilege check is an execution of the SQL statement: SHOW GRANTS FOR <user>@<host> and the return value of such query when the privilege is ALL is not the keyword ALL but rather the list of all privileges, such as:

{'*.*': ['SELECT', 'INSERT', 'UPDATE', 'DELETE', 'CREATE', 'DROP', 'RELOAD', 'SHUTDOWN', 'PROCESS', 'FILE', 'REFERENCES', 'INDEX', 'ALTER', 'SHOW DATABASES',  
'SUPER', 'CREATE TEMPORARY TABLES', 'LOCK TABLES', 'EXECUTE', 'REPLICATION SLAVE', 'REPLICATION CLIENT', 'CREATE VIEW', 'SHOW VIEW', 'CREATE ROUTINE', 'ALTER  
ROUTINE', 'CREATE USER', 'EVENT', 'TRIGGER', 'CREATE TABLESPACE', 'CREATE ROLE', 'DROP ROLE', 'APPLICATION_PASSWORD_ADMIN', 'AUDIT_ADMIN', 'BACKUP_ADMIN',     
'BINLOG_ADMIN', 'BINLOG_ENCRYPTION_ADMIN', 'CLONE_ADMIN', 'CONNECTION_ADMIN', 'ENCRYPTION_KEY_ADMIN', 'GROUP_REPLICATION_ADMIN', 'INNODB_REDO_LOG_ARCHIVE',    
'INNODB_REDO_LOG_ENABLE', 'PERSIST_RO_VARIABLES_ADMIN', 'REPLICATION_APPLIER', 'REPLICATION_SLAVE_ADMIN', 'RESOURCE_GROUP_ADMIN', 'RESOURCE_GROUP_USER',       
'ROLE_ADMIN', 'SERVICE_CONNECTION_ADMIN', 'SESSION_VARIABLES_ADMIN', 'SET_USER_ID', 'SHOW_ROUTINE', 'SYSTEM_USER', 'SYSTEM_VARIABLES_ADMIN',                   
'TABLE_ENCRYPTION_ADMIN', 'XA_RECOVER_ADMIN']} 

As such, the set of all privileges and the set containing the ALL keyword differ and as such it triggers a change in the module.

This is an idempotence problem. @bmalynovytch @bmildren @Andersson007, I think this is related to #89, we're not currently working on this bug and I'd appreciate your input in prioritizing that bugfix, this PR and issue #101 which are all somewhat related.

[Andersson007]

@Jorge-Rodriguez if CI fails again and it's unrelated, feel free to comment the problem spot in the tests. Please also put the link to the idempotency issue there and something like TODO: uncomment when fixed or something (the aim is not to forget to uncomment).

When CI is green, I'd be happy to make a final review.

[Andersson007]

TASK [test_mysql_user : create database using user2 and new password] **********
task path: /root/ansible_collections/community/mysql/tests/output/.tmp/integration/test_mysql_user-71m392nq-ÅÑŚÌβŁÈ/tests/integration/targets/test_mysql_user/tasks/test_user_password_update.yml:123
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<testhost> EXEC /bin/sh -c 'echo ~root && sleep 0'
<testhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /root/.ansible/tmp `"&& mkdir "` echo /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041 `" && echo ansible-tmp-1614324082.3479342-24793-98910581994041="` echo /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041 `" ) && sleep 0'
Using module file /root/ansible_collections/community/mysql/plugins/modules/mysql_db.py
<testhost> PUT /root/.ansible/tmp/ansible-local-200364ojg1vjm/tmp4o9kz8bc TO /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/AnsiballZ_mysql_db.py
<testhost> EXEC /bin/sh -c 'chmod u+x /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/ /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/AnsiballZ_mysql_db.py && sleep 0'
<testhost> EXEC /bin/sh -c '/usr/bin/python3.6 /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/AnsiballZ_mysql_db.py && sleep 0'
<testhost> EXEC /bin/sh -c 'rm -f -r /root/.ansible/tmp/ansible-tmp-1614324082.3479342-24793-98910581994041/ > /dev/null 2>&1 && sleep 0'
ok: [testhost] => {
    "changed": false,
    "db": "data",
    "db_list": [
        "data"
    ],
    "executed_commands": [],
    "invocation": {
        "module_args": {
            "ca_cert": null,
            "check_hostname": null,
            "check_implicit_admin": false,
            "client_cert": null,
            "client_key": null,
            "collation": "",
            "config_file": "/root/.my.cnf",
            "config_overrides_defaults": false,
            "connect_timeout": 30,
            "dump_extra_args": null,
            "encoding": "",
            "force": false,
            "hex_blob": false,
            "ignore_tables": [],
            "login_host": "127.0.0.1",
            "login_password": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "login_port": 3307,
            "login_unix_socket": null,
            "login_user": "db_user2",
            "master_data": 0,
            "name": [
                "data"
            ],
            "quick": true,
            "restrict_config_file": false,
            "single_transaction": false,
            "skip_lock_tables": false,
            "state": "present",
            "target": null,
            "unsafe_login_password": false,
            "use_shell": false
        }
    }
}

TASK [test_mysql_user : assert output message that database is created with new password] ***
task path: /root/ansible_collections/community/mysql/tests/output/.tmp/integration/test_mysql_user-71m392nq-ÅÑŚÌβŁÈ/tests/integration/targets/test_mysql_user/tasks/test_user_password_update.yml:133
fatal: [testhost]: FAILED! => {
    "assertion": "result is changed",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}

[Andersson007]

The assertion for this task fails

    - name: create database using user2 and new password
      mysql_db:
        login_user: '{{ user_name_2 }}'
        login_password: '{{ user_password_1 }}'
        login_host: '{{ mysql_host }}'
        login_port: '{{ mysql_primary_port }}'
        name: '{{ db_name }}'
        state: present
      register: result

    - name: assert output message that database is created with new password
      assert:
        that:
          - result is changed

    - name: remove database
      mysql_db:

Maybe it already exists. Try to remove it before this invocation as well

[Andersson007]

I just looked at one. Maybe there are other assertions fail including that one as you mentioned. Feel free to comment the unrelated spot.

[bmalynovytch]

While I completely agree the need of this PR, I think it should be done after we handled MySQL / MariaDB differences (https://github.com/ansible-collections/community.mysql/pull/103) , as this only introduces changes related to MySQL and it will require heavy rewrite after splitting MySQL / MariaDB code base.