-
Notifications
You must be signed in to change notification settings - Fork 435
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
feat: add do
command to update the authentication plugin of MySQL users
#1097
base: nightly
Are you sure you want to change the base?
Changes from 5 commits
820cebe
4d0042c
19802ea
a9e987e
d266f4b
24e8872
d56029f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
- [Improvement] Add a do command to update the authentication plugin of existing MySQL users from mysql_native_password to caching_sha2_password for compatibility with MySQL v8.4.0 and above. (by @Danyal-Faheem) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,9 @@ | |
from typing_extensions import ParamSpec | ||
|
||
from tutor import config as tutor_config | ||
from tutor import env, fmt, hooks | ||
from tutor import env, fmt, hooks, plugins | ||
from tutor.commands.context import Context | ||
from tutor.commands.jobs_utils import get_mysql_change_authentication_plugin_query | ||
from tutor.hooks import priorities | ||
|
||
|
||
|
@@ -315,6 +317,45 @@ def sqlshell(args: list[str]) -> t.Iterable[tuple[str, str]]: | |
yield ("lms", command) | ||
|
||
|
||
@click.command(context_settings={"ignore_unknown_options": True}) | ||
@click.option( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not be an option, but an argument, because it's mandatory. The command signature should not be:
but:
Moreover, we should be able to update all users with users=all:
|
||
"--users", | ||
is_flag=False, | ||
nargs=1, | ||
help="Specific users to upgrade the authentication plugin of. Requires comma-seperated values with no space in-between.", | ||
) | ||
@click.pass_obj | ||
def update_mysql_authentication_plugin( | ||
context: Context, users: str | ||
) -> t.Iterable[tuple[str, str]]: | ||
""" | ||
Update the authentication plugin of MySQL users from mysql_native_password to caching_sha2_password | ||
Handy command used when upgrading to v8.4 of MySQL which deprecates mysql_native_password | ||
""" | ||
|
||
config = tutor_config.load(context.root) | ||
|
||
if not config["RUN_MYSQL"]: | ||
fmt.echo_info( | ||
f"You are not running MySQL (RUN_MYSQL=False). It is your " | ||
f"responsibility to update the authentication plugin of mysql users." | ||
) | ||
return | ||
|
||
users_to_update = users.split(",") if users else list(plugins.iter_loaded()) | ||
|
||
query = get_mysql_change_authentication_plugin_query( | ||
config, users_to_update, not users | ||
) | ||
|
||
mysql_command = ( | ||
"mysql --user={{ MYSQL_ROOT_USERNAME }} --password={{ MYSQL_ROOT_PASSWORD }} --host={{ MYSQL_HOST }} --port={{ MYSQL_PORT }} --database={{ OPENEDX_MYSQL_DATABASE }} " | ||
+ shlex.join(["-e", query]) | ||
) | ||
|
||
yield ("lms", mysql_command) | ||
|
||
|
||
def add_job_commands(do_command_group: click.Group) -> None: | ||
""" | ||
This is meant to be called with the `local/dev/k8s do` group commands, to add the | ||
|
@@ -397,5 +438,6 @@ def do_callback(service_commands: t.Iterable[tuple[str, str]]) -> None: | |
print_edx_platform_setting, | ||
settheme, | ||
sqlshell, | ||
update_mysql_authentication_plugin, | ||
] | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
""" | ||
This module provides utility methods for tutor `do` commands | ||
|
||
Methods: | ||
- `get_mysql_change_authentication_plugin_query`: Generates MySQL queries to update the authentication plugin for MySQL users. | ||
""" | ||
|
||
from typing import List | ||
|
||
from tutor import exceptions | ||
from tutor.types import Config, ConfigValue | ||
|
||
|
||
def get_mysql_change_authentication_plugin_query( | ||
config: Config, users: List[str], all_users: bool | ||
) -> str: | ||
""" | ||
Generates MySQL queries to update the authentication plugin for MySQL users. | ||
|
||
This method constructs queries to change the authentication plugin to | ||
`caching_sha2_password`. User credentials must be provided in the tutor | ||
configuration under the keys `<user>_MYSQL_USERNAME` and `<user>_MYSQL_PASSWORD`. | ||
|
||
Args: | ||
config (Config): Tutor configuration object | ||
users (List[str]): List of specific MySQL users to update. | ||
all_users (bool): Flag indicating whether to include ROOT and OPENEDX users | ||
in addition to those specified in the `users` list. | ||
|
||
Returns: | ||
str: A string containing the SQL queries to execute. | ||
|
||
Raises: | ||
TutorError: If any user in the `users` list does not have corresponding | ||
username or password entries in the configuration. | ||
""" | ||
|
||
host = "%" | ||
query = "" | ||
|
||
def generate_mysql_authentication_plugin_update_query( | ||
username: ConfigValue, password: ConfigValue, host: str | ||
) -> str: | ||
return f"ALTER USER '{username}'@'{host}' IDENTIFIED with caching_sha2_password BY '{password}';" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is extremely confusing, and there are many ways in which it can fail. In order to improve it, I'd like to understand if it's possible to automatically detect all mysql users that were created with the legacy authentication plugin? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only way to do that would be to get the output of a MySQL query inside tutor codebase. I am not aware of any method that we can use to get the output of a command run inside a container inside the tutor codebase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming we could get the output of a mysql query in tutor, what would be that query? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something similar to this:
We need to limit the host to '%' as Tutor uses that host value by default. |
||
|
||
def generate_user_queries(users: List[str]) -> str: | ||
query = "" | ||
for user in users: | ||
user_uppercase = user.upper() | ||
if not ( | ||
f"{user_uppercase}_MYSQL_USERNAME" in config | ||
and f"{user_uppercase}_MYSQL_PASSWORD" in config | ||
): | ||
raise exceptions.TutorError( | ||
f"Username or Password for User {user} not found in config. " | ||
f"Please make sure that the following entries are present in the configuration:\n" | ||
f"{user_uppercase}_MYSQL_USERNAME\n{user_uppercase}_MYSQL_PASSWORD" | ||
) | ||
query += generate_mysql_authentication_plugin_update_query( | ||
config[f"{user_uppercase}_MYSQL_USERNAME"], | ||
config[f"{user_uppercase}_MYSQL_PASSWORD"], | ||
host, | ||
) | ||
return query | ||
|
||
if not all_users: | ||
return generate_user_queries(users) | ||
|
||
query += generate_mysql_authentication_plugin_update_query( | ||
config["MYSQL_ROOT_USERNAME"], config["MYSQL_ROOT_PASSWORD"], host | ||
) | ||
query += generate_mysql_authentication_plugin_update_query( | ||
config["OPENEDX_MYSQL_USERNAME"], config["OPENEDX_MYSQL_PASSWORD"], host | ||
) | ||
|
||
return query + generate_user_queries(users) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,12 @@ services: | |
--character-set-server=utf8mb4 | ||
--collation-server=utf8mb4_unicode_ci | ||
--binlog-expire-logs-seconds=259200 | ||
# We only require this option for MySQL 8.4.0 and above | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should include this check. If we are running MySQL, then it's not our job to remain compatible with all possible versions of MySQL. We just assume that we are running the version that is in config.yml. Let's remove this change, along with the one in k8s/deployments.yml. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was added for compatibility with #1102. When we downgrade MySQL to v8.1, it will crash if this option exists as the option does not exist in MySQL v8.1. We can make this check a lot simpler, with something like this:
However, that does not handle any edge cases. |
||
# Breaks MySQL for previous versions as this option does not exist on versions earlier than 8.4.0 | ||
{% set mysql_version = DOCKER_IMAGE_MYSQL.split(':')[-1] -%} | ||
Danyal-Faheem marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{% if mysql_version == "latest" or (mysql_version.split('.') | map('int') | list >= '8.4.0'.split('.') | map('int') | list) -%} | ||
--mysql-native-password=ON | ||
{%- endif %} | ||
restart: unless-stopped | ||
user: "999:999" | ||
volumes: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this paragraph can be reworded to focus that for any particular user, the command expects the configuration settings in a certain format. Then, the myuser examples can be added afterwards/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this paragraph.