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

feat: add do command to update the authentication plugin of MySQL users #1097

Open
wants to merge 7 commits into
base: nightly
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
20 changes: 20 additions & 0 deletions docs/local.rst
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,26 @@ The default Open edX theme is rather bland, so Tutor makes it easy to switch to

Out of the box, only the default "open-edx" theme is available. We also developed `Indigo, a beautiful, customizable theme <https://github.com/overhangio/indigo>`__ which is easy to install with Tutor.

.. _update_mysql_authentication_plugin:

Updating the authentication plugin of MySQL users
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

As of MySQL v8.4.0, the ``mysql_native_password`` authentication plugin has been deprecated. Users created with this authentication plugin should ideally be updated to use the latest ``caching_sha2_password`` authentication plugin.

Tutor makes it easy do so with this handy command::

tutor local do update-mysql-authentication-plugin

The above command will update all the database users created by Tutor. If you only want to update the authentication plugin of specific users, you can use the ``--users`` option. This option takes comma seperated names of users to upgrade::

tutor local do update-mysql-authentication-plugin --users=discovery,ecommerce

For this command, Tutor expects specific entries in the configuration for the mysql username and password of a database user. For example, if you are trying to update the user ``myuser``, the following case sensitive entries need to be present in the configuration::

MYUSER_MYSQL_USERNAME
MYUSER_MYSQL_PASSWORD

Running arbitrary ``manage.py`` commands
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
7 changes: 7 additions & 0 deletions docs/troubleshooting.rst
Original file line number Diff line number Diff line change
Expand Up @@ -216,3 +216,10 @@ NPM Dependency Conflict When overriding ``@edx/frontend-component-header`` or ``
----------------------------------------------------------------------------------------------------------------

The detailed steps are mentioned in `tutor-mfe <https://github.com/overhangio/tutor-mfe?tab=readme-ov-file#npm-dependency-conflict-when-overriding-edxfrontend-component-header-or-edxfrontend-component-footer>`__ documentation.

"Plugin 'mysql_native_password' is not loaded"
----------------------------------------------

This issue can occur when Tutor is upgraded from v15 (Olive) or earlier to v18 (Redwood) because the users created in Tutor v15 utilize the mysql_native_password authentication plugin by default. This plugin has been deprecated as of MySQL v8.4.0 which is the default MySQL server used in Tutor v18.

The handy :ref:`update-mysql-authentication-plugin <update_mysql_authentication_plugin>` do command in tutor can be used to fix this issue.
21 changes: 21 additions & 0 deletions tests/commands/test_jobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,24 @@ def test_set_theme(self) -> None:
self.assertIn("lms-job", dc_args)
self.assertIn("assign_theme('beautiful', 'domain1')", dc_args[-1])
self.assertIn("assign_theme('beautiful', 'domain2')", dc_args[-1])

def test_update_mysql_authentication_plugin(self) -> None:
with temporary_root() as root:
self.invoke_in_root(root, ["config", "save"])
with patch("tutor.utils.docker_compose") as mock_docker_compose:
result = self.invoke_in_root(
root,
[
"local",
"do",
"update-mysql-authentication-plugin",
],
)
dc_args, _dc_kwargs = mock_docker_compose.call_args

self.assertIsNone(result.exception)
self.assertEqual(0, result.exit_code)
self.assertIn("lms-job", dc_args)
self.assertIn("caching_sha2_password", dc_args[-1])
self.assertIn("openedx", dc_args[-1])
self.assertIn("root", dc_args[-1])
48 changes: 47 additions & 1 deletion tutor/commands/jobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -315,6 +317,49 @@ def sqlshell(args: list[str]) -> t.Iterable[tuple[str, str]]:
yield ("lms", command)


@click.command(context_settings={"ignore_unknown_options": True})
@click.option(
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

 tutor local do update_mysql_authentication_plugin --users=regis,danyal

but:

tutor local do update_mysql_authentication_plugin regis danyal

Moreover, we should be able to update all users with users=all:

tutor local do update_mysql_authentication_plugin 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 utilized 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
)

# In case there is no user to update the authentication plugin of
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's return earlier if there is no user:

if not users:
    return

if not query:
return

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
Expand Down Expand Up @@ -397,5 +442,6 @@ def do_callback(service_commands: t.Iterable[tuple[str, str]]) -> None:
print_edx_platform_setting,
settheme,
sqlshell,
update_mysql_authentication_plugin,
]
)
79 changes: 79 additions & 0 deletions tutor/commands/jobs_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
"""
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, fmt
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:
fmt.echo_info(
f"Authentication plugin of user {username} will be updated to caching_sha2_password"
)
return f"ALTER USER '{username}'@'{host}' IDENTIFIED with caching_sha2_password BY '{password}';"
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something similar to this:

select user from mysql.user where plugin='mysql_native_password' and host='%';

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
):
fmt.echo_warning(
f"Username or Password for User {user} not found in config. Skipping update process for User {user}."
)
continue

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)
8 changes: 8 additions & 0 deletions tutor/fmt.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ def alert(text: str) -> str:
return click.style("⚠️ " + text, fg="yellow", bold=True)


def warning(text: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of warning when we already have alert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alert prints to standard error and this seemed like a situation where we just wanted to display a warning rather than an error.

return click.style(text, fg="bright_yellow")


def echo_warning(text: str) -> None:
echo(warning(text))


def echo(text: str, err: bool = False) -> None:
if os.environ.get("_TUTOR_COMPLETE"):
if os.environ.get("COMP_WORDS") or os.environ.get("COMP_CWORD"):
Expand Down
5 changes: 5 additions & 0 deletions tutor/templates/k8s/deployments.yml
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,12 @@ spec:
- "--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
# 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] -%}
{% if mysql_version == "latest" or (mysql_version.split('.') | map('int') | list >= '8.4.0'.split('.') | map('int') | list) -%}
- "--mysql-native-password=ON"
{%- endif %}
env:
- name: MYSQL_ROOT_PASSWORD
value: "{{ MYSQL_ROOT_PASSWORD }}"
Expand Down
5 changes: 5 additions & 0 deletions tutor/templates/local/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

{% if DOCKER_IMAGE_MYSQL >= "docker.io/mysql:8.4.0" -%}- "--mysql-native-password=ON"{%- endif %}

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:
Expand Down
Loading