Add TLS connection parameters

[Jorge-Rodriguez]

SUMMARY

Add support for TLS REQUIRES options, to enforce TLS client connections for users.

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

mysql_user

ADDITIONAL INFORMATION

This is a PoC pull request on how these parameters could be handled.
IMPORTANT!! THIS HAS NOT BEEN TESTED

[ansibullbot]

cc @Alexander198961 @Andersson007 @Jmainguy @Xyon @bmalynovytch @bmildren @kurtdavis @michaelcoburn @Oneiroi @tolland
click here for bot help

[Jorge-Rodriguez]

Ping @Andersson007 @rwky @jpmens

[pabelanger]

recheck

[ansibullbot]

The test ansible-test sanity --test ignores [explain] failed with 2 errors:

tests/sanity/ignore-2.10.txt:1942:1: File 'tests/unit/plugins/module_utils/gcp/test_gcp_utils.py' does not exist
tests/sanity/ignore-2.10.txt:1943:1: File 'tests/unit/plugins/module_utils/gcp/test_gcp_utils.py' does not exist

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

plugins/modules/database/mysql/mysql_user.py:384:97: E231: missing whitespace after ','

click here for bot help

[felixfontein]

Hmm, the --test ignores errors are caused by me, fixing them in #370.

[Andersson007]

@Jorge-Rodriguez thanks for the PR!
Could you please add integration tests? See existing ones in tests/integration/targes/mysq_user.
(Maybe there is an old version of mysql installed)

[Andersson007]

if this is a MariaDB feature, there are also tests for mariadb 10+. Look for directories in tests/integration/targets that contain mariadb in their names

[Andersson007]

I kicked the tests here as well. Please, fix sanity related stuff (there is a guid in the ansible documentation how to run sanity tests locally using docker containers. Helps to catch such errors before pushing)

[ansibullbot]

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

plugins/modules/database/mysql/mysql_user.py:384:96: bad-whitespace: Exactly one space required after comma     return "REQUIRE %s" % ' AND '.join([' '.join(filter(None, (key, fix_quotes(value)))) for key,value in tls_requires.items()])                                                                                                 ^

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

plugins/modules/database/mysql/mysql_user.py:384:97: E231: missing whitespace after ','

click here for bot help

[Jorge-Rodriguez]

It is not a MariaDB feature. I found the documentation on the mariadb link but the tls options are part of mysql. Maybe I can find the documentation on mysql.

[Andersson007]

any updates on this?

[Jorge-Rodriguez]

I'm still working on it, but the last couple of days have been quite busy at work so I didn't manage to get any code in. I have not abandoned this.

[gundalow]

recheck

[Jorge-Rodriguez]

Attending #AnsibleAutomates2020, will get on with this afterwards.

[Jorge-Rodriguez]

@Andersson007 does this look ok, or should I squash into a single commit?

[Andersson007]

@Jorge-Rodriguez it's ok as it is, no actions needed. I'm on a public holiday today. I'll try to make time to look at the pr tomorrow. Or maybe today later. Thanks for doing this!

[Andersson007]

Suggested change

	deprecated_features:
	- mysql_user - using ``REQUIRESSL`` in ``priv`` is deprecated in favor of ``tls_requires`` (https://github.com/ansible-collections/community.general/pull/369).
	deprecated_features:
	- mysql_user - using ``REQUIRESSL`` in ``priv`` is deprecated in favor of ``tls_requires`` (https://github.com/ansible-collections/community.general/pull/369).

is it necessary to deprecate this feature? I'd avoid deprecations (if possible).
Can we make priv: REQUIRESSL and tls_requires mutually exclusive?
I don't insist, it's just topic to discuss

[Jorge-Rodriguez]

I just reordered the code, now TLS requires are handled after privileges so if both priv:REQUIRESSL and tls_requires are specified, tls_requires takes precedence.
It's not really query optimal as it means that first we'd set REQUIRE SSL and modify it immediately after.
I could remove the deprecation notice and replace it with a 'try to use tls_requires instead, please' note.

[Andersson007]

Would be good (imo).
Still waiting for @bmalynovytch 's opinion because he's a MySQL WG leader

[Andersson007]

@bmalynovytch what do you think about the pr?

[Andersson007]

we also need check_mode: yes cases in the tests and checks that everything works as expected:

1. Try to change something which weren't changed before with check_mode: yes. It must report that result is changed
2. Then check that it wasn't actually changed
3. Then change it not in check_mode
4. After that try to change it again with check_mode: yes. It must report that result is not changed

I hope you understand why this PR is being reviewed so thoroughly. These changes are significant and the module is one of the crutial for MySQL support. Thanks for your patience and being persistent.

[Jorge-Rodriguez]

I have a modification check on the tests (call to user_mod) I could splice the check mode tests in there. I should probably test check mode on user_add, so I could pretend that to the current battery of tests, i.e.:

1. Add use in check mode
2. Assert result is changed
3. Assert user isn't present
4. Continue with existing test battery.

I'm more than happy to see this PR going through such a thorough review. I need to take a cooler of days off of this now, but you can expect new changes coming by the turn of July.

[felixfontein]

CC @bmildren

[Andersson007]

all mysql stuff have been recently moved to https://github.com/ansible-collections/community.mysql by @bmildren
feel free to move the PR to there

[Andersson007]

@Jorge-Rodriguez i think there's no point to wait for anyone else. When it's moved, we could merge this.

[Jorge-Rodriguez]

@Andersson007 Roger. I still have those extra tests we talked about pending. I'll add those to the moved PR

[Andersson007]

@Jorge-Rodriguez ah, nice!

[nerijus]

PR moved to ansible-collections/community.mysql#9