-
Notifications
You must be signed in to change notification settings - Fork 3
database: detect ProxySQL DB user at connect time #2439
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
Changes from all commits
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,55 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Shared MariaDB connection helper used by the status and loadbalancer commands. | ||
|
|
||
| Both need a superuser connection to the cluster's MariaDB, and both have to cope | ||
| with the two load-balancer topologies OSISM ships. On a ProxySQL cluster (the | ||
| 2025.2 default) the proxied superuser is the sharded name ``<user>_shard_0``; on a | ||
| plain HAProxy cluster it is ``<user>``. | ||
|
|
||
| Which topology is in use is decided by ``enable_proxysql``, a release-gated | ||
| ``osism/defaults`` group_vars value. It is absent from the operator's | ||
| ``environments/kolla/configuration.yml`` and is only correct after Ansible templates | ||
| it in the host's variable context, so it cannot be read from a raw config file (doing | ||
| so silently returned the wrong user -- the ProxySQL DB-user bug). Rather than resolve | ||
| it, connect empirically: try the ProxySQL user first and fall back to the plain user | ||
| on an authentication failure. The connection itself tells us which topology we are on, | ||
| with no dependency on defaults resolution. | ||
| """ | ||
|
|
||
| from loguru import logger | ||
| import pymysql | ||
|
|
||
| # MariaDB ER_ACCESS_DENIED_ERROR. ProxySQL also returns this code when the supplied | ||
| # user is unknown to it, so it covers "wrong user for this topology" on both paths. | ||
| _ACCESS_DENIED = 1045 | ||
|
|
||
|
|
||
| def connect(host, user, password, **connect_kwargs): | ||
| """Connect to MariaDB, transparently handling the ProxySQL sharded superuser. | ||
|
|
||
| Tries ``<user>_shard_0`` (ProxySQL) first, then ``<user>`` (plain HAProxy), | ||
| falling back only on an access-denied error so genuine connectivity failures | ||
| (host unreachable, timeout, ...) surface immediately without a pointless retry. | ||
| ``connect_kwargs`` are passed through to :func:`pymysql.connect` unchanged (e.g. | ||
| ``port``, ``database``, ``cursorclass``, ``connect_timeout``). | ||
|
|
||
| Raises the last :class:`pymysql.Error` if neither user connects. | ||
| """ | ||
| last_exc = None | ||
| for candidate in (f"{user}_shard_0", user): | ||
| try: | ||
| connection = pymysql.connect( | ||
| host=host, user=candidate, password=password, **connect_kwargs | ||
| ) | ||
| logger.debug(f"Connected to MariaDB at {host} as {candidate}") | ||
| return connection | ||
| except pymysql.err.OperationalError as exc: | ||
| if exc.args and exc.args[0] == _ACCESS_DENIED: | ||
| logger.debug( | ||
| f"MariaDB user {candidate} denied at {host}; trying next candidate" | ||
| ) | ||
| last_exc = exc | ||
| continue | ||
| raise | ||
| raise last_exc |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from unittest import mock | ||
|
|
||
| import pymysql | ||
| import pytest | ||
|
|
||
| from osism.utils import mariadb | ||
|
|
||
|
|
||
| def _access_denied(user): | ||
| return pymysql.err.OperationalError(1045, f"Access denied for user '{user}'") | ||
|
|
||
|
|
||
| def test_connect_prefers_proxysql_shard_user(): | ||
| """A ProxySQL cluster: the sharded user connects on the first try.""" | ||
| sentinel = object() | ||
| with mock.patch("pymysql.connect", return_value=sentinel) as connect: | ||
| result = mariadb.connect("vip", "root", "pw", port=3306) | ||
|
|
||
| assert result is sentinel | ||
| connect.assert_called_once_with( | ||
| host="vip", user="root_shard_0", password="pw", port=3306 | ||
| ) | ||
|
|
||
|
|
||
| def test_connect_passes_through_extra_kwargs(): | ||
| """The call sites' connection kwargs reach pymysql.connect unchanged.""" | ||
| sentinel = object() | ||
| cursor_cls = object() | ||
| with mock.patch("pymysql.connect", return_value=sentinel) as connect: | ||
| result = mariadb.connect( | ||
| "vip", | ||
| "octavia", | ||
| "pw", | ||
| port=3306, | ||
| database="octavia", | ||
| cursorclass=cursor_cls, | ||
| connect_timeout=10, | ||
| ) | ||
|
|
||
| assert result is sentinel | ||
| connect.assert_called_once_with( | ||
| host="vip", | ||
| user="octavia_shard_0", | ||
| password="pw", | ||
| port=3306, | ||
| database="octavia", | ||
| cursorclass=cursor_cls, | ||
| connect_timeout=10, | ||
| ) | ||
|
|
||
|
|
||
| def test_connect_falls_back_to_plain_user_on_access_denied(): | ||
| """A plain HAProxy cluster: the shard user is denied, the plain user connects.""" | ||
| sentinel = object() | ||
| with mock.patch( | ||
| "pymysql.connect", side_effect=[_access_denied("root_shard_0"), sentinel] | ||
| ) as connect: | ||
| result = mariadb.connect("vip", "root", "pw") | ||
|
|
||
| assert result is sentinel | ||
| assert [c.kwargs["user"] for c in connect.call_args_list] == [ | ||
| "root_shard_0", | ||
| "root", | ||
| ] | ||
|
|
||
|
|
||
| def test_connect_does_not_retry_on_non_auth_errors(): | ||
| """A genuine connectivity failure surfaces immediately, without a second attempt.""" | ||
| unreachable = pymysql.err.OperationalError(2003, "Can't connect to MySQL server") | ||
| with mock.patch("pymysql.connect", side_effect=unreachable) as connect: | ||
| with pytest.raises(pymysql.err.OperationalError) as excinfo: | ||
| mariadb.connect("vip", "root", "pw") | ||
|
|
||
| assert excinfo.value.args[0] == 2003 | ||
| connect.assert_called_once() | ||
|
|
||
|
|
||
| def test_connect_raises_last_error_when_both_users_denied(): | ||
| """Neither user authenticates: the last access-denied error propagates.""" | ||
| errors = [_access_denied("root_shard_0"), _access_denied("root")] | ||
| with mock.patch("pymysql.connect", side_effect=errors): | ||
| with pytest.raises(pymysql.err.OperationalError) as excinfo: | ||
| mariadb.connect("vip", "root", "pw") | ||
|
|
||
| assert "root'" in excinfo.value.args[1] | ||
|
Comment on lines
+80
to
+87
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. nitpick (testing): Tighten the assertion to clearly prove that the last (plain user) error is propagated This test should clearly verify that the last access-denied error (for the plain user) is raised. The current assertion only checks for "root'" in the message, which would also succeed if the wrong error were raised. Please assert both the error code (1045) and that the message refers to the plain user, e.g. |
||
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.
suggestion (testing): Add a test to verify that extra connection kwargs are passed through unchanged to
pymysql.connectThe helper is also used in
statusandloadbalancerwith extra kwargs likedatabase,cursorclass, andconnect_timeout, but the current tests only coverport. Please add a test (e.g.test_connect_passes_through_extra_kwargs) that callsmariadb.connectwith several extra kwargs and asserts thatpymysql.connectreceives them unchanged, so future refactors don’t accidentally drop or modify these parameters.