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

fix: Use authentication_policy to specify default auth plugin for MySQL 8.4 #16426

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

icyflame
Copy link
Contributor

@icyflame icyflame commented Jul 18, 2024

Description

Fixes #16425

default_authentication_plugin was removed starting in MySQL 8.4.0:

The default_authentication_plugin system variable, deprecated in MySQL 8.0.27, is removed as of
MySQL 8.4.0. Use authentication_policy instead.

-- https://dev.mysql.com/doc/refman/8.4/en/mysql-nutshell.html

I used the same process for testing the new MySQL 8.4 configuration file as described in the issue #16425.

The binary versions were different:

[vitess@78ec5221d252 vitess]$ mysqlctld --version;
mysqlctld version Version: 21.0.0-SNAPSHOT (Git revision cb1ea513ebd55ba3b9d409a235ad42190ea98777 branch 'update-mysql-84-default-cnf') built on Thu Jul 18 03:49:22 UTC 2024 by vitess@78ec5221d252 using go1.22.5 linux/amd64

[vitess@78ec5221d252 vitess]$ mysqld --version
/usr/sbin/mysqld  Ver 8.4.1 for Linux on x86_64 (MySQL Community Server - GPL)

When the same process is followed and the script 101_initial_cluster.sh is run, the script works
and starts the mysqld process. This can be confirmed by looking at the output of ps as well as by
connecting directly to MySQL using the socket and the DBA user's name:

[vitess@78ec5221d252 vitess]$ ps -e
  PID TTY          TIME CMD
	1 pts/0    00:00:00 bash
11660 pts/0    00:00:01 etcd
11694 pts/0    00:00:00 vtctld
11935 pts/1    00:00:00 bash
11944 pts/0    00:00:03 mysqld
11945 pts/0    00:00:03 mysqld
11947 pts/0    00:00:03 mysqld
12086 pts/0    00:00:01 vttablet
12123 pts/0    00:00:01 vttablet
12157 pts/0    00:00:01 vttablet
12189 pts/0    00:00:06 vtorc
12419 pts/0    00:00:00 vtgate
12460 pts/0    00:00:00 vtadmin
13528 pts/1    00:00:00 ps

[vitess@78ec5221d252 vitess]$ mysql -S /vt/vt_0000000100/mysql.sock -u vt_dba
Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 29
Server version: 8.4.1 MySQL Community Server - GPL

Copyright (c) 2000, 2024, Oracle and/or its affiliates.

Oracle is a registered trademark of Oracle Corporation and/or its
affiliates. Other names may be trademarks of their respective
owners.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

mysql> \s
--------------
mysql  Ver 8.4.1 for Linux on x86_64 (MySQL Community Server - GPL)

Connection id:          29
Current database:
Current user:           vt_dba@localhost
SSL:                    Not in use
Current pager:          stdout
Using outfile:          ''
Using delimiter:        ;
Server version:         8.4.1 MySQL Community Server - GPL
Protocol version:       10
Connection:             Localhost via UNIX socket
Server characterset:    utf8mb4
Db     characterset:    utf8mb4
Client characterset:    utf8mb4
Conn.  characterset:    utf8mb4
UNIX socket:            /vt/vt_0000000100/mysql.sock
Binary data as:         Hexadecimal
Uptime:                 2 min 5 sec

Threads: 11  Questions: 2203  Slow queries: 0  Opens: 274  Flush tables: 4  Open tables: 88  Queries per second avg: 17.624
--------------

mysql> ^DBye

Justification for requesting backport to v20

v20 has been released already with the file (this commit). EXTRA_MY_CNF can not be used to override this default configuration file. This means that users must provide the complete my.cnf file using the --mysqlctl_mycnf_template argument to mysqlctld. (Please correct me if I am wrong. I am using this part of the code to justify the need for backporting.)

So, in order to support MySQL 8.4 starting in v20, this should be backported to a future release in the v20 series.

Related Issue(s)

Fixes #16425

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
    • I ran the tests locally and there were many failing tests. But none of the failures were related to mysqld specifically, or this part of the code, I believe. Please let me know if there is a related test, and I will find and fix it.
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

None.

Copy link
Contributor

vitess-bot bot commented Jul 18, 2024

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Jul 18, 2024
@github-actions github-actions bot added this to the v21.0.0 milestone Jul 18, 2024
@icyflame icyflame changed the title fix: Use authentication_policy to specify default auth plugin fix: Use authentication_policy to specify default auth plugin for MySQL 8.4 Jul 18, 2024
@icyflame icyflame force-pushed the update-mysql-84-default-cnf branch from d5d9fae to d01d5a6 Compare July 18, 2024 06:28
Copy link
Contributor

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

I wish to offer an alternative approach. MySQL supports the special prefix loose_ to any variable. With that prefix, if the variable exists - fine; if not - it is silently ignored.
So we can support both pre-8.4 and post 8.4 at the same time like so:

loose_default_authentication_plugin = mysql_native_password
loose_authentication_policy = 'mysql_native_password'

WDYT?

@icyflame icyflame force-pushed the update-mysql-84-default-cnf branch from d01d5a6 to d19756b Compare July 18, 2024 08:53
@icyflame
Copy link
Contributor Author

One question: According to this:

case 8:
if mysqld.capabilities.version.Minor >= 4 {
versionConfig = config.MycnfMySQL84

... the file mysql84.cnf is used as the configuration only when the version is detected to be more than 8.4.0. So, is it required to support pre-8.4 MySQL versions for the file mysql84.cnf?

I understand that if users downgrade their mysqld binary without updating the my.cnf file then this cross-compatibility will be helpful.

If that is the intention, then we should prefix all auth-related options with loose_ because each option related to auth was changed at a different MySQL version. This is the change I have made to the PR. Please take a look!

Information and test results with MySQL 8.4:

Option Context
mysql_native_password=ON Introduced 8.4.0
default_authentication_plugin=... Removed 8.4.0
authentication_policy=... Introduced 8.0.27

I prefixed everything with loose_ and tested it with MySQL 8.4:

[vitess@1e0457954056 vt_0000000100]# grep loose_ my.cnf
loose_mysql_native_password = ON
loose_authentication_policy = 'mysql_native_password'
loose_default_authentication_plugin = mysql_native_password
loose_rpl_semi_sync_source_timeout = 1000000000000000000
loose_rpl_semi_sync_source_wait_no_replica = 1

[vitess@1e0457954056 vt_0000000100]# tail error.log
[snip]
2024-07-18T08:45:00.233808Z 0 [Warning] [MY-000067] [Server] unknown variable 'loose_default_authentication_plugin=mysql_native_password'.
2024-07-18T08:45:00.260035Z 0 [System] [MY-010931] [Server] /usr/sbin/mysqld: ready for connections. Version: '8.4.1'  socket: '/vt/vt_0000000100/mysql.sock'  port: 17100  MySQL Community Server - GPL.

Local cluster using 101_initial_cluster.sh works and runs the mysqld process. ✅

@shlomi-noach
Copy link
Contributor

... the file mysql84.cnf is used as the configuration only when the version is detected to be more than 8.4.0. So, is it required to support pre-8.4 MySQL versions for the file mysql84.cnf?

You're right. No, it's not required to support pre-8.4 versions.

@icyflame icyflame force-pushed the update-mysql-84-default-cnf branch from d19756b to d01d5a6 Compare July 18, 2024 09:37
@icyflame
Copy link
Contributor Author

Got it, thanks for confirming, I reverted my change to the original simple one. I will mark this PR as Ready for Review now.

@icyflame icyflame marked this pull request as ready for review July 18, 2024 09:39
@deepthi deepthi removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Jul 18, 2024
@deepthi
Copy link
Member

deepthi commented Jul 18, 2024

Thank you for the contribution!

in order to support MySQL 8.4 starting in v20, this should be backported to a future release in the v20 series.

I'm curious, are you planning to go into production with Vitess + MySQL 8.4?
We do not yet officially support MySQL 8.4 in any vitess release, though some of the preliminary work was done during the v20 cycle. This means that we won't backport anything 8.4 specific into an older release.

Ideally we need to start running unit tests against 8.4 to see what works and what doesn't. Is that something you would be interested in contributing?

@deepthi
Copy link
Member

deepthi commented Jul 18, 2024

The PR that added mysql84.cnf had this comment, but it looks like it wasn't done correctly?

The latter is needed to start the work in the future for MySQL 8.4.0. The main change here is that the deprecated mysql_native_password plugin needs to be explicitly enabled.

@dbussink
Copy link
Contributor

The PR that added mysql84.cnf had this comment, but it looks like it wasn't done correctly?

Yeah, likely me testing and then not properly committing the final result amongst testing on different versions and no CI yet to catch those kinds of mistakes.

Ideally we need to start running unit tests against 8.4 to see what works and what doesn't. Is that something you would be interested in contributing?

Yeah, this is needed regardless also to catch issues like this and there's potentially a whole bunch more we don't know about (yet). There's at least also things like function behavior changes that the evalengine needs to have conditionals for that I know about but it'll probably shake out other things too.

@dbussink
Copy link
Contributor

So, in order to support MySQL 8.4 starting in v20, this should be backported to a future release in the v20 series.

Yeah, we only did some prep work and there's no official support in v20 and it's not something to backport. Doing all the work in one single release is not likely to happen with how much needs to happen, so the expectation also isn't that it's there all in one release.

We could aim for support in v21 but that's something we'd need to explicitly commit to and there's no such plans yet at the moment that I know of. Help with stuff like this would increase those chances though, but we also need to have the CI setup etc. as well as a bare minimum.

@icyflame
Copy link
Contributor Author

icyflame commented Jul 19, 2024

I'm curious, are you planning to go into production with Vitess + MySQL 8.4?

No, we do not plan to do this. My intention was to migrate from MySQL 5.7 to MySQL 8, along with the move to use Vitess v19. As 8.0 is supported, we will use that going forward.

Is that something you would be interested in contributing?

No, I will not be able to commit to this. Sorry! (I have been going through this unit test workflow file in my free time. At work though, as we can use 8.0 in production, we will be doing that.)

`default_authentication_plugin` was removed starting in MySQL 8.4.0:

> The default_authentication_plugin system variable, deprecated in MySQL 8.0.27, is removed as of
> MySQL 8.4.0. Use authentication_policy instead.
>
> -- https://dev.mysql.com/doc/refman/8.4/en/mysql-nutshell.html

Signed-off-by: Siddharth Kannan <mail@siddharthkannan.in>
@dbussink dbussink force-pushed the update-mysql-84-default-cnf branch from d01d5a6 to ca346e1 Compare July 19, 2024 08:17
@dbussink
Copy link
Contributor

Rebased the PR since somehow some checks didn't run in CI so it couldn't be merged.

@deepthi deepthi merged commit 7621331 into vitessio:main Jul 19, 2024
125 checks passed
frouioui pushed a commit to frouioui/vitess that referenced this pull request Jul 19, 2024
…QL 8.4 (vitessio#16426)

Signed-off-by: Siddharth Kannan <mail@siddharthkannan.in>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants