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

Add support for MariaDB 10.5's new grants and grant aliases #59280

Merged
merged 5 commits into from
Sep 28, 2022

Conversation

pprkut
Copy link
Contributor

@pprkut pprkut commented Jan 12, 2021

What does this PR do?

MariaDB 10.5 added alias for the REPLICATION SLAVE and REPLICATION CLIENT grants, and split up the SUPER grant into smaller pieces.
This adds the necessary code to support both the new aliases as well as the new grants resulting from the SUPER split (the SUPER grant itself is still supported).

Reference: https://mariadb.com/kb/en/grant/

What issues does this PR fix or reference?

Fixes: #58297

Previous Behavior

The mysql_grants.present state returns an error becomes it wants to add REPLICATION CLIENT but the db added BINLOG MONITOR

New Behavior

The state reports that the REPLICATION CLIENT grant was added successfully

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

@pprkut pprkut requested a review from a team as a code owner January 12, 2021 14:16
@pprkut pprkut requested review from garethgreenaway and removed request for a team January 12, 2021 14:16
@babs
Copy link
Contributor

babs commented Apr 21, 2021

LGTM, definitely needs integration :)

@garethgreenaway
Copy link
Contributor

@pprkut Thanks for the PR. Can you rebase to the latest master branch so we can see if these failing tests pass. Also, if you can add some tests for the changes that would be great. There are some newer tests that I added in a previous PR that use various versions of the MySQL variants running in Docker containers to test functionality.

@pprkut
Copy link
Contributor Author

pprkut commented Aug 4, 2021

@garethgreenaway Is tests/pytests/integration/modules/test_mysql.py the right place to look at for adding new tests? Can I just add new functions in there or do they need to be registered somewhere else as well?

@jsoligny
Copy link

jsoligny commented Dec 6, 2021

Hello @pprkut,

Thanks a lot, we were block by this issue until I saw your PR. Do you still work on this PR ? Because we're greatly interested in your fixes.

Have a nice day,

Jérémie

@pprkut
Copy link
Contributor Author

pprkut commented Dec 6, 2021

@jsoligny Yes, still on my TODO list, just haven't had time yet to work on the tests :-/
I'm hoping to be able to have another go at it before the end of the year though.

@pprkut pprkut force-pushed the mariadb_10.5_grants branch 7 times, most recently from 54f751f to 9884207 Compare December 29, 2021 15:15
@pprkut
Copy link
Contributor Author

pprkut commented Dec 29, 2021

It's really difficult to test the tests because of all the "Slow tests are disabled!" 😞

@garethgreenaway
Copy link
Contributor

@pprkut If you're running the tests locally you can pass the flat --run-slow to have the Slow tests run.

@pprkut
Copy link
Contributor Author

pprkut commented Jan 18, 2022

@garethgreenaway I'm afraid I'm not. I don't have the infrastructure for that locally, so I'm fully reliant on the CI 😞

@garethgreenaway
Copy link
Contributor

@pprkut Looks like there are some merge conflicts here. Unfortunately something, I suspect permissions, is preventing me from seeing what is conflicting. If you want to open that up I can look at adding some tests here for the new grants.

@pprkut
Copy link
Contributor Author

pprkut commented Sep 28, 2022

@garethgreenaway I don't see the usual checkbox for allowing maintainer writes, so I tried something else.
I also think I managed to fix up the conflicts (the test file moved and modules are called slightly differently there)

@garethgreenaway
Copy link
Contributor

@pprkut Something is still preventing me from updating the branch or editing any files, which isn't necessary just sometimes helpful if unrelated tests are failing and a merge from master is necessary. Thanks for adding the tests, this is great! It does look like they're not skipping on variants where they shouldn't be running though.

@pprkut
Copy link
Contributor Author

pprkut commented Sep 28, 2022

@garethgreenaway I sent you an invite to our repo. As far as I read, that's the only way to do it in this case 😕

Regarding the tests, that's where I got stuck, yes. The actual tests should be fine, just something in the logic that checks the mariadb version that's off

@garethgreenaway
Copy link
Contributor

@pprkut I'll look for the invite and check the tests. Thanks!

@garethgreenaway
Copy link
Contributor

@pprkut Looks like the invite to the ORG allows me additional permissions. Thanks!

@garethgreenaway
Copy link
Contributor

@pprkut Spinning up the tests locally I can see at least one of the tests is failing with the following error:
Incorrect usage of DB GRANT and GLOBAL PRIVILEGES, quick Google search for this looks like maybe the grant needs to be done on the global level instead of on the salt database?

@pprkut
Copy link
Contributor Author

pprkut commented Sep 28, 2022

@garethgreenaway Ah that might indeed be one thing, yes. I checked our pillar config and we do indeed specify those permissions on a global level there

@garethgreenaway
Copy link
Contributor

@pprkut Awesome. I updated the tests using globals and the tests are now passing, remaining tests failures are unrelated so I'll restart them.

@garethgreenaway garethgreenaway added the Sulfur v3006.0 release code name and version label Sep 28, 2022
@garethgreenaway garethgreenaway added this to the Sulphur v3006.0 milestone Sep 28, 2022
@garethgreenaway garethgreenaway merged commit 8b53054 into saltstack:master Sep 28, 2022
cebe added a commit to cebe/salt that referenced this pull request Oct 14, 2022
MariaDB has added some grants in 10.4.x and 10.5.x that are not present here, which results in an error when creating.

This is an addition to saltstack#59280.

Also improved exception handling in `grant_add` which did not log the original error message and replaced it with a generic error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Cannot add certain grants in MariaDB 10.5
4 participants