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

mysql_user: fix compatibility issues with various MySQL/MariaDB versions #45355

Merged
merged 39 commits into from
Apr 9, 2019

Conversation

bmalynovytch
Copy link
Contributor

SUMMARY

To handle properly user management, version check needed refactoring, as well as the query used to get existing password hash.

Fixes #44441
Fixes #43361
Fixes #43255
Fixes #42564
Possibly fixes #40091
Possibly fixes #29625
Fixes #29615

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

mysql_user

ANSIBLE VERSION
ansible 2.6.3
  config file = /Users/benjamin/Dev/xxxxxx/xxx-platform/ansible.cfg
  configured module search path = [u'/Users/benjamin/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/var/pyenv/versions/2.7.11/envs/ansible-xxxxxxx/lib/python2.7/site-packages/ansible
  executable location = /usr/local/var/pyenv/versions/ansible-xxxxxx/bin/ansible
  python version = 2.7.11 (default, Mar 31 2016, 13:46:08) [GCC 4.2.1 Compatible Apple LLVM 7.3.0 (clang-703.0.29)]
ADDITIONAL INFORMATION

Module wasn't able to handle properly MariaDB 10.3 as well as edge cases (mixed situations with user table including users with new style and users with old style).
It's now able to:

  • better identify "new" vs "old" versions of MySQL, as well as MariaDB
  • better handle any column of password hashes storage

The module now handles properly idempotence and was tested successfully against Debian 9.5 with MariaDB 10.3.9.

@ansibot
Copy link
Contributor

ansibot commented Sep 7, 2018

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:community This issue/PR relates to code supported by the Ansible community. labels Sep 7, 2018
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Sep 7, 2018
@ansibot

This comment has been minimized.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Sep 7, 2018
@bmalynovytch
Copy link
Contributor Author

... still working on a fix

@bmalynovytch
Copy link
Contributor Author

@ansibot WIP

@bmalynovytch bmalynovytch changed the title mysql_user: fix MySQL/MariaDB version check WIP: mysql_user: fix MySQL/MariaDB version check Sep 9, 2018
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Sep 9, 2018
@bmalynovytch bmalynovytch changed the title WIP: mysql_user: fix MySQL/MariaDB version check mysql_user: fix MySQL/MariaDB version check Sep 10, 2018
@ansibot ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Sep 10, 2018
@ansibot

This comment has been minimized.

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Sep 10, 2018
@bmalynovytch bmalynovytch changed the title mysql_user: fix MySQL/MariaDB version check WIP: mysql_user: fix MySQL/MariaDB version check Sep 10, 2018
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. and removed ci_verified Changes made in this PR are causing tests to fail. labels Sep 10, 2018
@ansibot

This comment has been minimized.

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed ci_verified Changes made in this PR are causing tests to fail. labels Sep 10, 2018
@ansibot

This comment has been minimized.

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed ci_verified Changes made in this PR are causing tests to fail. labels Sep 10, 2018
@ansibot
Copy link
Contributor

ansibot commented Apr 9, 2019

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/45355-mysql_user-fix-versions-compatibilities:0:0: extension must be one of: .yml, .yaml

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Apr 9, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 9, 2019
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

LGTM

I assume you have tested this with newer and older versions of both MariaDB and MySQL :)

WHERE TABLE_SCHEMA = 'mysql' AND TABLE_NAME = 'user' AND COLUMN_NAME IN ('Password', 'authentication_string')
ORDER BY COLUMN_NAME ASC LIMIT 1
""")
colB = cursor.fetchone()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not select both columns in one query (without the LIMIT) and fill colA and colB in module code? This will save one query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a trick to get an ordered list of valid columns to use in the next queries.
It's a way of getting the right list without using tons of if conditions which may be error prone.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should also work:

            cursor.execute("""
                SELECT COLUMN_NAME FROM information_schema.COLUMNS
                WHERE TABLE_SCHEMA = 'mysql' AND TABLE_NAME = 'user' AND COLUMN_NAME IN ('Password', 'authentication_string')
            """)
            cols = sorted([row[0] for row in cursor.fetchall()])
            colA, colB = cols[0], cols[-1]

(no idea if fetchall() exists and works like this, but I guess there will be some method which fetches more than one row?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, probably

Copy link
Contributor

Choose a reason for hiding this comment

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

You can still change that later (once this is merged) if you want.

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 9, 2019
@bmalynovytch
Copy link
Contributor Author

LGTM

I assume you have tested this with newer and older versions of both MariaDB and MySQL :)

Yes, definitely, as well as other contributors in various environments.

@gundalow gundalow merged commit 9c52750 into ansible:devel Apr 9, 2019
@gundalow
Copy link
Contributor

gundalow commented Apr 9, 2019

Great work everybody on this.
Merged into devel for release in Ansible 2.8

@bmalynovytch
Copy link
Contributor Author

Finally we get it merged !
Thx everybody !

@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. database Database category module This issue/PR relates to a module. mysql new_contributor This PR is the first contribution by a new community member. shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet