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: get rid of privs comparison #234

Closed
Andersson007 opened this issue Oct 19, 2021 · 6 comments · Fixed by #243
Closed

mysql_user: get rid of privs comparison #234

Andersson007 opened this issue Oct 19, 2021 · 6 comments · Fixed by #243

Comments

@Andersson007
Copy link
Collaborator

Andersson007 commented Oct 19, 2021

SUMMARY

(this is a thing to discuss)

We should analyze possibility to get rid of privs comparison entirely and use just a mechanisms like "try except": let's let the server decide which privs are supported and throw the exception itself.

A solution can conceptually look like the following:

  • Remove priv comparison
  • Add exception handling to catch an exception thrown by the DB (maybe there's something currently present)
  • The handler (try except + parsing the error for related keywords) will notify the user and exit gracefully (i.e. without ugly tracebacks, etc.)

However this is a breaking change and we'll require a major release in the future.

Possible scenarios:
Option 1 (Most votes are for this variant):

  • Implement the above (or another solution) now
  • Test it thoroughly with MariaDB and MySQL
  • Merge it after 3.0.0 release (within 2 months)
  • Notify users via the changelog that such changes will be released in the major release (now it'll be 4.0.0 in 6-8 months or later)
  • Release them finally

Option 2 (not breaking but downside is one more argument):

  • Implement the solution above without removing the priv comparison (i.e. in parallel)
  • Add a new bool argument called, say, check_privs with default yes)
  • If the option is no - comparison will be skipped
  • Test it with MySQL, MariaDB
  • Release it right now and forget - users will be able to switch back and forth if they need

Option 3:

  • Same as Option 2 but with a warning to users that the check_privs will get no as its default in 4.0.0
  • Plus announcing this via the changelog
  • Finally do it in 4.0.0

Option 4:

  • ...
CONTEXT
  • Before 2.3.0 all supported privs were hardcoded in the VALID_PRIVS frozenset.
  • Since 2.3.0 all supported privs are fetched from the server dynamically with the SHOW PRIVILEGES command.
  • The issue is that some privs were deprecated, replaced and became aliases and they are not present in the SHOW PRIVILEGES output but they were present in the VALID_PRIVS frozenset.
  • Removing VALID_PRIVS brought breaking changes.
  • Relates to mysql_user: fix broken compatibility for priviledge aliases #233 fixed the breaking change - returned VALID_PRIVS and now the module merges it in a frozenset with show_privs (fetched from server dynamically) and EXTRA_PRIVS.

Relates to #233
Relates to #217

@Andersson007
Copy link
Collaborator Author

@bmalynovytch @Jorge-Rodriguez @rsicart let's discuss this issue ^. These are just quick thoughts to discuss and find the best solution, maybe completely different from what i wrote in the description.

Whatever option we'll choose, @rsicart , would you like to continue to work in this direction?

@Jorge-Rodriguez
Copy link
Contributor

I'd go for option 1, personally I do not see a benefit on locally checking a static list of allowed privileges which needs to be maintained. We have other use cases in the code base where a parameter's accepted values are "whatever mysql accepts".

Additionally, and unless I'm sorely mistaken, the only privilege on that list that is not supported by mysql (i.e. has a special meaning for the module) is REQUIRESSL and that one is planned to be removed in a breaking change too so we can bundle both breaking changes in the same release.

@rsicart
Copy link
Contributor

rsicart commented Oct 20, 2021

I’d like to work on it, but I’m not sure to have time.

Personally I’d choose option 1 too. Options 2 and 3 are nice but from my point of view they’ll add legacy code to maintain.

About option 1, we should check if it’s possible to get idempotency right.

@Jorge-Rodriguez
Copy link
Contributor

@rsicart idempotence might be a problem with dynamic privileges. To my mind the way it could be done is query the user's privileges and, instead of comparing to the current ALL keyword, run the update query unconditionally and compare against the results, in my ignorance I don't see how else we could achieve the desired result. However, this approach smells like it could have a bunch of side effects if for whatever reason ALL didn't really mean all.

@Andersson007
Copy link
Collaborator Author

I’d like to work on it, but I’m not sure to have time.

@rsicart would be great. And no rush and pressure with this.

Whoever will do it, I'd be happy to review things

@Andersson007
Copy link
Collaborator Author

FYI: If anyone is eager to pick it up, welcome (please put it explicitly in here).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants