sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable #677

SoledaD208 · 2024-09-09T09:03:46Z

…TES in session variable instead of global variable

SUMMARY

ISSUE TYPE

Bugfix Pull Request
Docs Pull Request
Feature Pull Request
New Module Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

laurent-indermuehle · 2024-09-11T07:28:44Z

Hi @SoledaD208 and thank you for taking the time to open a PR.

Before we can merge, it misses:

A description that explains why the change is made.
A change log fragment with the same explanation.
Integration tests.

About tests : I'm not sure what will happen when you change the scope of the sql_mode from global to session for the mysql_role and mysql_user modules. A rapid grep showed that the line you modified is the only time we use sql_mode in the entire collection.

Also, while writing tests, you could demonstrate how you intent to set a session with set sql_mode='ANSI_QUOTES'". As I don't think community.mysql allow for this. But tests are a great way to experiment that.

If I'm right, we could modify your PR to add such a feature. Feel free to ask me for help executing tests. I'm also available on matrix : #mysql:ansible.com

SoledaD208 · 2024-09-15T03:47:21Z

Hi Laurent, Thanks for your answer. I'm a bit busy these days so can't reply to you promptly. Before we can merge, it misses: Sure, I will spend some time doing those. A rapid grep showed that the line you modified is the only time we use sql_mode in the entire collection. Agreed with you. That's also what I can see from the code. you could demonstrate how you intent to set a session with set sql_mode='ANSI_QUOTES'". As I don't think community.mysql allow for this Oh, I don't know about that, let me try if I can. In a 'real life' situation, I can set sql_mode by making use of parameter session_vars or setting sql_mode in a client config file, and point the module to that file. By the way, not related to sql_mode, I think mysql_user cannot parse the role grants or partial revokes (Please correct if I'm wrong). E.g: - partial revoke: mysql> SHOW GRANTS FOR u1; +------------------------------------------+ | Grants for u1@% | +------------------------------------------+ | GRANT SELECT, INSERT ON *.* TO `u1`@`%` | | REVOKE INSERT ON `world`.* FROM `u1`@`%` | +------------------------------------------+ - role grants: mysql> SHOW GRANTS FOR 'dev1'@'localhost'; +-------------------------------- -----------------+ | Grants for ***@***.*** | +-------------------------- -----------------------+ | GRANT USAGE ON *.* TO `dev1`@`localhost` | | GRANT `app_developer`@`%` TO `dev1`@`localhost` | +------------------------- ------------------------+ If those types of grants are not able to be parsed yet. Would they be supported in newer versions of the module? Thanks, Tuan ( aka soledad208 :) )

…

------------------------------ *From:* Laurent Indermühle ***@***.***> *Sent:* Wednesday, September 11, 2024 2:29:06 PM *To:* ansible-collections/community.mysql < ***@***.***> *Cc:* Soledad208 ***@***.***>; Mention < ***@***.***> *Subject:* Re: [ansible-collections/community.mysql] sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable (PR #677) Hi @SoledaD208 <https://github.com/SoledaD208> and thank you for taking the time to open a PR. Before we can merge, it misses: - A description that explains why the change is made. - A change log fragment with the same explanation. - Integration tests. About tests : I'm not sure what will happen when you change the scope of the sql_mode from global to session for the mysql_role and mysql_user modules. A rapid grep showed that the line you modified is the only time we use sql_mode in the entire collection. Also, while writing tests, you could demonstrate how you intent to set a session with set sql_mode='ANSI_QUOTES'". As I don't think community.mysql allow for this. But tests are a great way to experiment that. If I'm right, we could modify your PR to add such a feature. Feel free to ask me for help executing tests. I'm also available on matrix : #mysql:ansible.com <https://matrix.to/#/%23mysql:ansible.com> — Reply to this email directly, view it on GitHub <#677 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCTXOXYG6L7TUPBQ7ZUN43ZV7WMFAVCNFSM6AAAAABN4CGCSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBSHA3TANJUGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Andersson007 · 2024-09-25T06:00:49Z

folks any thoughts on ^?

laurent-indermuehle · 2024-09-26T08:19:58Z

Hi @SoledaD208,
Could you provide examples of when sql_mode='ANSI_QUOTES' is useful please? Then I can help you with the integration tests. You already talked about table names in the priv fields of mysql_user. But are there other places where this would be useful? Like in a query, for instance?

For your question about the role/revoke/read of privileges, we should keep discussions separate. Maybe in a new issue. But before doing so, please use the mysql_info module with the users and users_info filters. Maybe they can do what you want. Also, keep in mind that MySQL and MariaDB treat roles differently. So please provide the engine and version you're using when you file a bug. This helps to better understand the issue.

SoledaD208 · 2024-10-03T15:19:45Z

Could you provide examples of when sql_mode='ANSI_QUOTES' is useful please?

if you ask about the bad effects to community.mysql module. Incorrect quotation in store procedure and function (not tables) will make the module always see the privs on function/SP are the new ones. As consequence, it will try to revoke the existing privs and re-grant them. Not only mysql_user, mysql_role also import the function privileges_unpack: , thus it will also affect by ANSI_QUOTES.

About the general purpose of ANSI_QUOTES. Afaik, ANSI_QUOTES is the standard SQL (which Mysql, by default, doesn't follow). So without enabling that mode, if you dump some data from Postresql or other RDBMS to a sql file, then run it in Mysql, the sql dump files will fail.

sql_mode='ANSI_QUOTE' or any session's variable can be easily set by making use of community.mysql's session_vars argument. The ansible module allows users to freely set or unset ANSI_QUOTES per session. Due to Bug #115953 , that feature is really needed for mysql_user and mysql_role to work properly, but for now it's not working correctly. The reason is it's extracting ANSI_QUOTES from global sql_mode instead of session

SoledaD208 · 2024-10-03T15:25:02Z

please provide the engine and version you're using

I'm using Mysql 8. Mysql 5.7 doesn't have partial revokes or RBAC, so there's no issue with the old version.
It looks like MariaDB does not have such feature, so we may not have issue with MariaDB

For your question about the role/revoke/read of privileges, we should keep discussions separate

Agreed with you, I will open another issue. mysql_user, mysql_role and also mysql_info will break if partial revoke is used.

laurent-indermuehle · 2024-10-09T18:29:25Z

Very good tests @SoledaD208.
I'm wondering if the failed tests are because of your change. It is related to roles so it could be.
I'll need to study how the "params + persist" works. I never used it before.
I'm busy on something else, but remember you can asks for help if the tests/github action or else bother you.

…l_mode

codecov · 2024-10-10T16:42:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.96%. Comparing base (28bf709) to head (24bf639).
Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #677       +/-   ##
===========================================
+ Coverage   25.74%   76.96%   +51.21%     
===========================================
  Files          32       20       -12     
  Lines        2808     2657      -151     
  Branches      704      679       -25     
===========================================
+ Hits          723     2045     +1322     
+ Misses       2044      413     -1631     
- Partials       41      199      +158

Flag	Coverage Δ
integration	`76.32% <100.00%> (?)`
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.

SoledaD208 · 2024-10-10T16:53:26Z

thanks @laurent-indermuehle indeed my "params + persist" messed up the tests.
Reason is when persist is used, the sql_mode's value will be written down to the container's .my.conf therefore it affects the other tests. I added Ansible tasks to reconfigure sql_mode back to the original value after the test succeeds. Let's see if all the tests get passed...

SoledaD208 · 2024-10-10T17:23:02Z

The tests went well for Mysql, but failed for Mariadb. Apparently, it's because in Mariadb, users can't persist sql_mode to config file like what they can do in Mysql 8. I had changed the testing playbook a bit: mode: "{% if db_engine == 'mariadb' %}global{% else %}persist{% endif %}" , then thanks god, all good now.

laurent-indermuehle · 2024-10-14T15:28:13Z

@SoledaD208 great job! Are you ready for a review or you want to add more tests?

Also, please add a changelogs / framents file to describe what you're changing and why.

Andersson007

@SoledaD208 thanks for the persistent work on the improvement!

Yep, as @laurent-indermuehle spotted we need a fragment here https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment

Andersson007 · 2024-10-15T05:50:01Z

tests/integration/targets/test_mysql_user/tasks/issue-671.yaml

+      mysql_query:
+        <<: *mysql_params
+        query: 'select @@GLOBAL.sql_mode AS sql_mode'
+      register: sql_mode_orig


Suggested change

register: sql_mode_orig

register: sql_mode_orig

would you like to assert: this one too? I see you assert the second one called result

Thanks, let me check and add the assert. by the way, due to some reasons, I will need to close this PR and open another via other account.

Thanks, let me check and add the assert. by the way, due to some reasons, I will need to close this PR and open another via other account.

SGTM

SoledaD208 · 2024-10-21T15:27:13Z

hi @laurent-indermuehle @Andersson007 , I just modified my PR to add fragments files and improve the test file. The PR should be ready now, could you please have a look?

…sion_variable

Andersson007 · 2024-10-23T08:34:10Z

@SoledaD208 let's wait for @laurent-indermuehle a bit, if no response i'll merge it within a few days, thanks

SoledaD208 · 2024-10-23T12:07:36Z

Thanks @Andersson007 , that's totally fine :)

…

________________________________ From: Andrew Klychkov ***@***.***> Sent: Wednesday, October 23, 2024 3:34:32 PM To: ansible-collections/community.mysql ***@***.***> Cc: Soledad208 ***@***.***>; Mention ***@***.***> Subject: Re: [ansible-collections/community.mysql] sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable (PR #677) @SoledaD208<https://github.com/SoledaD208> let's wait for @laurent-indermuehle<https://github.com/laurent-indermuehle> a bit, if no response i'll merge it within a few days, thanks — Reply to this email directly, view it on GitHub<#677 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACCTXORTMXJ3NRZ6ZPS77G3Z45NRRAVCNFSM6AAAAABN4CGCSSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZRGMYDMOBZHE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

laurent-indermuehle · 2024-10-23T19:37:21Z

I'm on vacation. I'll back next week. No computer here so don't wait for me. Or do if it can wait ;)

laurent-indermuehle

Here is some linting propositions, mainly usage of FQCN and failed_when instead of assert when only one test is performed (this makes for better CI output).

But didn't you said you have to create a new PR @SoledaD208 ?

tests/integration/targets/test_mysql_user/tasks/issue-671.yaml

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

SoledaD208 · 2024-10-31T15:49:47Z

hi @laurent-indermuehle , please continue with this PR. I don't need to create new one any more. I just modified my PR and commited all the suggestions, please review again.
Thanks

Andersson007 · 2024-11-07T08:12:12Z

@laurent-indermuehle could you please take a look?

laurent-indermuehle · 2024-11-07T08:55:46Z

Sure! Sorry for the delay.
This looks good to me, thanks for the modifications @SoledaD208. I'm merging now. This PR will be available in the next release.

Andersson007 · 2024-11-07T09:38:11Z

@SoledaD208 thanks for the contribution!
@laurent-indermuehle thanks for reviewing and merging!

SoledaD208 force-pushed the check_ansi_quotes_in_session_variable branch from 27e3b08 to 61a4ff4 Compare October 9, 2024 14:11

SoledaD208 force-pushed the check_ansi_quotes_in_session_variable branch 2 times, most recently from d44c899 to 22e3531 Compare October 10, 2024 16:17

issue-671: get ASNI_QUOTES from session sql_mode instead of GLOBAL sq…

6d4fc3c

…l_mode

SoledaD208 force-pushed the check_ansi_quotes_in_session_variable branch from 22e3531 to 6d4fc3c Compare October 10, 2024 16:18

issue 671: improve test playbook

8ef32e5

Andersson007 reviewed Oct 15, 2024

View reviewed changes

improve the test file and add fragment files for issue 671

5967433

Merge branch 'ansible-collections:main' into check_ansi_quotes_in_ses…

a086778

…sion_variable

Andersson007 approved these changes Oct 22, 2024

View reviewed changes

laurent-indermuehle requested changes Oct 30, 2024

View reviewed changes

improve tests

24bf639

laurent-indermuehle merged commit ebb37ae into ansible-collections:main Nov 7, 2024
57 checks passed

laurent-indermuehle mentioned this pull request Nov 7, 2024

ANSI_QUOTES is get from global variable sql_mode instead of session variable. Therefore, it may use the quotes incorrectly if session sql_mode differs from global one #671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable #677

sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable #677

SoledaD208 commented Sep 9, 2024

laurent-indermuehle commented Sep 11, 2024

SoledaD208 commented Sep 15, 2024 via email

Andersson007 commented Sep 25, 2024

laurent-indermuehle commented Sep 26, 2024

SoledaD208 commented Oct 3, 2024

SoledaD208 commented Oct 3, 2024

laurent-indermuehle commented Oct 9, 2024

codecov bot commented Oct 10, 2024 •

edited

Loading

SoledaD208 commented Oct 10, 2024 •

edited

Loading

SoledaD208 commented Oct 10, 2024 •

edited

Loading

laurent-indermuehle commented Oct 14, 2024

Andersson007 left a comment

Andersson007 Oct 15, 2024 •

edited

Loading

SoledaD208 Oct 16, 2024

Andersson007 Oct 22, 2024

SoledaD208 commented Oct 21, 2024

Andersson007 commented Oct 23, 2024

SoledaD208 commented Oct 23, 2024 via email

laurent-indermuehle commented Oct 23, 2024

laurent-indermuehle left a comment

SoledaD208 commented Oct 31, 2024

Andersson007 commented Nov 7, 2024

laurent-indermuehle commented Nov 7, 2024

Andersson007 commented Nov 7, 2024

sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable #677

sql_mode can be set in session, therefore we should look for ANSI_QUOTES in session variable instead of global variable #677

Conversation

SoledaD208 commented Sep 9, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

laurent-indermuehle commented Sep 11, 2024

SoledaD208 commented Sep 15, 2024 via email

Andersson007 commented Sep 25, 2024

laurent-indermuehle commented Sep 26, 2024

SoledaD208 commented Oct 3, 2024

SoledaD208 commented Oct 3, 2024

laurent-indermuehle commented Oct 9, 2024

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

SoledaD208 commented Oct 10, 2024 • edited Loading

SoledaD208 commented Oct 10, 2024 • edited Loading

laurent-indermuehle commented Oct 14, 2024

Andersson007 left a comment

Choose a reason for hiding this comment

Andersson007 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

SoledaD208 Oct 16, 2024

Choose a reason for hiding this comment

Andersson007 Oct 22, 2024

Choose a reason for hiding this comment

SoledaD208 commented Oct 21, 2024

Andersson007 commented Oct 23, 2024

SoledaD208 commented Oct 23, 2024 via email

laurent-indermuehle commented Oct 23, 2024

laurent-indermuehle left a comment

Choose a reason for hiding this comment

SoledaD208 commented Oct 31, 2024

Andersson007 commented Nov 7, 2024

laurent-indermuehle commented Nov 7, 2024

Andersson007 commented Nov 7, 2024

codecov bot commented Oct 10, 2024 •

edited

Loading

SoledaD208 commented Oct 10, 2024 •

edited

Loading

SoledaD208 commented Oct 10, 2024 •

edited

Loading

Andersson007 Oct 15, 2024 •

edited

Loading