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 ALL grant on mysql >= 8.0.33 #1618

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

alaunay
Copy link

@alaunay alaunay commented Jan 21, 2024

Summary

This patch add ALL grant support for mysql >= 8.0.33, due tot the recent addition of the new TELEMETRY_LOG_ADMIN, see:
https://docs.oracle.com/cd/E17952_01/mysql-8.0-relnotes-en/news-8-0-33.html#mysqld-8-0-33-feature

Additional Context

Add any additional context about the problem here.

  • Root cause and the steps to reproduce. (If applicable)

Puppet changes grant at each run:

Notice: /Stage[main]/Mysql::Server::Providers/Mysql_grant[XXX@YYY/*.*]/privileges: privileges changed ['ALL', 'APPLICATION_PASSWORD_ADMIN', 'AUDIT_ABORT_EXEMPT', 'AUDIT_ADMIN', 'AUTHENTICATION_POLICY_ADMIN', 'BACKUP_ADMIN', 'BINLOG_ADMIN', 'BINLOG_ENCRYPTION_ADMIN', 'CLONE_ADMIN', 'CONNECTION_ADMIN', 'ENCRYPTION_KEY_ADMIN', 'FIREWALL_EXEMPT', 'FLUSH_OPTIMIZER_COSTS', 'FLUSH_STATUS', 'FLUSH_TABLES', 'FLUSH_USER_RESOURCES', 'GROUP_REPLICATION_ADMIN', 'GROUP_REPLICATION_STREAM', 'INNODB_REDO_LOG_ARCHIVE', 'INNODB_REDO_LOG_ENABLE', 'PASSWORDLESS_USER_ADMIN', 'PERSIST_RO_VARIABLES_ADMIN', 'REPLICATION_APPLIER', 'REPLICATION_SLAVE_ADMIN', 'RESOURCE_GROUP_ADMIN', 'RESOURCE_GROUP_USER', 'ROLE_ADMIN', 'SENSITIVE_VARIABLES_OBSERVER', 'SERVICE_CONNECTION_ADMIN', 'SESSION_VARIABLES_ADMIN', 'SET_USER_ID', 'SHOW_ROUTINE', 'SYSTEM_USER', 'SYSTEM_VARIABLES_ADMIN', 'TABLE_ENCRYPTION_ADMIN', 'TELEMETRY_LOG_ADMIN', 'XA_RECOVER_ADMIN'] to ['ALL'] (corrective)
  • Thought process behind the implementation.

Add the new TELEMETRY_LOG_ADMIN, according to previous code.

Related Issues (if any)

Issue #1474 #1502 #1503

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified. (For example puppet apply)

@CLAassistant
Copy link

CLAassistant commented Jan 21, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

✅ Marc-DRI
✅ alaunay
✅ BuJo
❌ rahuls2997
You have signed the CLA already but the status is still pending? Let us recheck it.

@fraenki
Copy link
Contributor

fraenki commented Mar 4, 2024

So much looking forward to this! (FWIW, this is also causing issues when performing long-running schema changes.)

@Ramesh7, maybe if you find some time, would you have a look at this please? Thanks :)

@MarcWort
Copy link

Any progress in reviewing that PR?

@fraenki
Copy link
Contributor

fraenki commented Aug 6, 2024

ping @puppetlabs

@ic248
Copy link

ic248 commented Aug 29, 2024

Would be good to have an eta for this. Although can easily ignore it's a pain when developing and seeing constant changes

@ianc769
Copy link

ianc769 commented Oct 7, 2024

Maybe @alexjfisher or @bastelfreak ? Been a few months 😅

@bastelfreak
Copy link
Collaborator

@ianc769 can you please rebase against main to get rid of the merge commit?

# rubocop:disable Layout/LineLength
if (newer_than('mysql' => '8.0.0') && (sorted_privileges == mysql_v8_privileges || sorted_privileges == mysqlcluster_v8_privileges)) || sorted_privileges == mysql_pre_v8_privileges_one || sorted_privileges == mysql_pre_v8_privileges_two
if (newer_than('mysql' => '8.0.0') && (sorted_privileges == mysql_v8_privileges || sorted_privileges == mysqlcluster_v8_privileges)) || sorted_privileges == mysql_pre_v8_privileges_one || sorted_privileges == mysql_pre_v8_privileges_two || sorted_privileges == mysql_pre_v8_privileges_three
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we somehow have an acceptance tests for this?

rahuls2997 and others added 4 commits October 7, 2024 19:43
fix mistakes

fix spec for the changed query
Value of the facter :macaddress was nil, causing tests to have two
failures (contexts "igalic's laptop" and "node with lo only").

Update the facter initialization allows to pass the tests
successfully.
* When using multiple excluded databases, the list of databases
  is filtered using `grep -v`.
  i.e. `grep -v '^\(information_schema|performance_schema\)$`
* When using Basic vs Extended Regular Expressions, the characters
  `(` and `|` lose their special meaning, the backslashed versions
  have to be used.
  For the group (`()`) the escaping has been done, however the
  alternation is unescaped.

Leading to:

* All the excluded databases will be backed up.
* In case a database is not backuppable (which is why it had been
  excluded), this leads to the cleanup not being run at all, as it
  depends on the backup having been successful.

This MR aims to fix this issue, by revising the regular expression
and specifying that behaviour in the respective class spec.
@alaunay
Copy link
Author

alaunay commented Oct 7, 2024

I probably borked the branch, here's the patch, it'll be easier.

mysql-add-priv-telemetry.txt

(renamed it to .txt, it didn't like a .patch...)

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

Successfully merging this pull request may close these issues.