Skip to content

Commit d768c6e

Browse files
committed
Merge branch 'v3.0' into fix/5363-monitor-caching-sha2
Brings the branch up to date with v3.0 (14 commits, including the #5993 credential-scope work that already carried this branch's ca03c8d), and re-triggers a full CI run after the CI-mysql84-g9 job on 9421d44 wedged for 6 hours in test_ssl_fast_forward-3_libmariadb-t. That stall was not reproducible: the same commit ran mysql84-g9 locally in 26 minutes and the test passed 5/5 in isolation, four other g9 runs completed successfully inside the same window, and no GitHub Actions incident overlapped it. Cause still unknown; investigation paused. Merge verified: lib/MySQL_Protocol.cpp auto-merged keeping both v3.0's cred_scope_for_session() call sites and this branch's Session/DS argument fix; the repro scripts and TAP test keep their restore-exact-state versions. Builds clean under PROXYSQL31.
2 parents 9421d44 + 90d4cc1 commit d768c6e

17 files changed

Lines changed: 823 additions & 63 deletions

.github/workflows/CI-lint-groups-json.yml

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,24 @@ jobs:
1010
runs-on: ubuntu-latest
1111
steps:
1212
- uses: actions/checkout@v4
13+
- name: Fetch GH-Actions branch
14+
# The reusable half of every workflow pair lives on GH-Actions, so the
15+
# coverage lint needs that ref to tell an unwired group from one wired
16+
# only over there. actions/checkout fetches a single branch, so without
17+
# this the check finds no ref and skips itself silently. --depth=1 is
18+
# enough: the lint only reads the tree, never history.
19+
run: |
20+
git fetch --no-tags --depth=1 origin \
21+
+refs/heads/GH-Actions:refs/remotes/origin/GH-Actions \
22+
|| echo "::warning::could not fetch GH-Actions; the workflow-coverage check will skip"
1323
- name: Lint groups.json format
1424
run: python3 test/tap/groups/lint_groups_json.py
1525
- name: Check every TAP source is registered in groups.json
1626
run: python3 test/tap/groups/check_groups.py --source
1727
- name: Check group infra/workflow coverage (warn-only)
18-
# Warns when a group references a missing infra (phantom) or has a
19-
# real dbdeployer infra but no GitHub Actions workflow. Known gaps
20-
# are allowlisted; only NEW infra-backed groups without a workflow
21-
# are flagged. Warn-only here (no --strict) so it never reds CI.
28+
# Warns when a group references a missing infra (phantom), or when no
29+
# workflow on either branch can select the group at all -- meaning
30+
# tests registered in it never run in CI. Known gaps are allowlisted;
31+
# only NEW ones are flagged. Warn-only here (no --strict) so it never
32+
# reds CI.
2233
run: python3 test/tap/groups/lint_group_coverage.py

deps/sqlite3/sqlite3_pass_exts.patch

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
--- sqlite3.c 2024-03-22 19:22:47.046093173 +0100
22
+++ sqlite3-pass-exts.c 2024-03-22 19:24:09.557303716 +0100
3-
@@ -26275,6 +26275,207 @@
3+
@@ -26275,6 +26275,228 @@
44
sqlite3ResultStrAccum(context, &sRes);
55
}
66

@@ -171,14 +171,35 @@
171171
+ sqlite3_result_text(context, err_msg, -1, SQLITE_TRANSIENT);
172172
+ return;
173173
+ } else {
174+
+ /* Map each random byte onto a 64-character alphabet that is safe to embed
175+
+ * in ProxySQL's credential strings.
176+
+ *
177+
+ * The previous scheme (mask with 0x7f, then bump only '\0' and '$') left
178+
+ * roughly 26% of generated salts containing ';' or ':'. Those are the
179+
+ * separators used by 'admin-admin_credentials' and
180+
+ * 'admin-stats_credentials', so such a hash is silently split into a bogus
181+
+ * credential by ProxySQL_Admin::add_credentials() at
182+
+ * 'LOAD ADMIN VARIABLES TO RUNTIME' -- the variable still reports all 70
183+
+ * bytes and nothing errors, but the login then fails with a generic
184+
+ * 'Access denied'. It also emitted control bytes ~99% of the time, making
185+
+ * the hashes unreadable and unsafe to paste into a configuration file.
186+
+ *
187+
+ * 64 divides 256 evenly, so masking with 0x3f introduces no modulo bias.
188+
+ * 20 characters over a 64-symbol alphabet still carry 120 bits of salt
189+
+ * entropy.
190+
+ *
191+
+ * This is a generation-side change only. Verification
192+
+ * (MySQL_Protocol.cpp: PPHR_verify_sha2 / PPHR_sha2full) reads the salt
193+
+ * back out with substr(7,20) and passes it straight to sha256_crypt_r
194+
+ * without inspecting its contents, so hashes generated before this change
195+
+ * keep verifying unchanged and no migration is required.
196+
+ */
197+
+ static const char SALT_ALPHABET[] =
198+
+ "./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
174199
+ unsigned int i = 0;
175200
+
176-
+ for (i = 0; i < sizeof(salt_buf)/sizeof(unsigned char); i++) {
177-
+ salt_buf[i] = salt_buf[i] & 0x7f;
178-
+
179-
+ if (salt_buf[i] == '\0' || salt_buf[i] == '$') {
180-
+ salt_buf[i] = salt_buf[i] + 1;
181-
+ }
201+
+ for (i = 0; i < DEF_SALT_SIZE; i++) {
202+
+ salt_buf[i] = (unsigned char)SALT_ALPHABET[salt_buf[i] & 0x3f];
182203
+ }
183204
+
184205
+ memcpy(salt, salt_buf, salt_size);
@@ -208,7 +229,7 @@
208229
/*
209230
** current_time()
210231
**
211-
@@ -133230,6 +133431,10 @@
232+
@@ -133230,6 +133452,10 @@
212233
FUNCTION(substr, 3, 0, 0, substrFunc ),
213234
FUNCTION(substring, 2, 0, 0, substrFunc ),
214235
FUNCTION(substring, 3, 0, 0, substrFunc ),
Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
# Admin Session Autocommit Compatibility Implementation Plan
2+
3+
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
4+
5+
**Goal:** Let Connector/Python's `SET @@session.autocommit = ON|OFF` complete successfully on ProxySQL's classic Admin interface.
6+
7+
**Architecture:** The Admin handler already has a connect-setup compatibility block that replies with an OK packet and no result set for bare `SET AUTOCOMMIT`. Extend that same block with the Connector/Python spelling, preserving the Admin interface's existing no-op semantics. Extend its focused TAP regression test with exact Connector/Python payloads.
8+
9+
**Tech Stack:** C++17, ProxySQL classic Admin protocol, TAP/libmysqlclient regression test, GNU Make.
10+
11+
## Global Constraints
12+
13+
- Keep the scope to connection-setup compatibility; do not add Admin transaction-state support.
14+
- Match `SET @@session.autocommit` case-insensitively after leading SQL comments are stripped.
15+
- Return the existing OK/no-result-set response and do not modify `global_variables`.
16+
- Build from a clean worktree with `make clean` followed by `make` using a bounded parallelism level derived from available CPUs and memory.
17+
- Preserve normal red/green CI semantics; do not mask the Connector/Python 26.7.0 soak failure.
18+
19+
---
20+
21+
### Task 1: Establish the Admin compatibility regression
22+
23+
**Files:**
24+
- Modify: `test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp:39-55`
25+
- Test: `test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp`
26+
27+
**Interfaces:**
28+
- Consumes: the existing `ACCEPT_CASES` array; each entry is sent via `mysql_query()` to the classic Admin interface.
29+
- Produces: three failing, exact Connector/Python compatibility assertions that become green only when the Admin handler sends an OK packet.
30+
31+
- [ ] **Step 1: Add the precise failing connection-setup cases**
32+
33+
Add these three entries immediately after the existing bare `SET AUTOCOMMIT=1` cases:
34+
35+
```cpp
36+
"SET @@session.autocommit = OFF", // Connector/Python pure-Python post-connect setup
37+
"SET @@session.autocommit = ON", // setter's opposite state uses the same syntax
38+
"/*connector-python*/ SET @@session.autocommit = OFF", // comment-stripping compatibility path
39+
```
40+
41+
- [ ] **Step 2: Build and run the regression test before the handler change**
42+
43+
Determine `build_jobs` as the smaller of the available CPU count and 8, then use it consistently:
44+
45+
```bash
46+
build_jobs=$(nproc)
47+
[ "$build_jobs" -gt 8 ] && build_jobs=8
48+
make clean
49+
make -j"$build_jobs"
50+
make -C test/tap/tests -j"$build_jobs" mysql-reg_test_5786_admin_strip_leading_sql_comments-t
51+
```
52+
53+
Run only the focused test through its normal isolated harness and clean up its
54+
uniquely named infrastructure on exit:
55+
56+
```bash
57+
export INFRA_ID="admin-session-autocommit-red-$(date +%s)"
58+
export TAP_GROUP=legacy-g1
59+
export TEST_PY_TAP_INCL='mysql-reg_test_5786_admin_strip_leading_sql_comments-t'
60+
export SKIP_CLUSTER_START=1
61+
trap 'test/infra/control/stop-proxysql-isolated.bash || true; test/infra/control/destroy-infras.bash || true' EXIT
62+
test/infra/control/ensure-infras.bash
63+
test/infra/control/run-tests-isolated.bash
64+
```
65+
66+
The new cases must fail with `ERROR: Unknown global variable:
67+
'@@session.autocommit'.`; retain the failing TAP output as the red-phase
68+
evidence.
69+
70+
- [ ] **Step 3: Commit the regression test**
71+
72+
```bash
73+
git add test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp
74+
git commit -m "test: cover admin session autocommit setup"
75+
```
76+
77+
### Task 2: Accept the Connector/Python session spelling as an Admin setup no-op
78+
79+
**Files:**
80+
- Modify: `lib/Admin_Handler.cpp:4065-4088`
81+
- Test: `test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp`
82+
83+
**Interfaces:**
84+
- Consumes: `mb`, the comment-stripped command pointer in `admin_session_handler()`.
85+
- Produces: an OK packet through `SPA->send_ok_msg_to_client()` for `SET @@session.autocommit`, with no SQLite query or ProxySQL global-variable update.
86+
87+
- [ ] **Step 1: Extend only the existing compatibility predicate**
88+
89+
Add one branch next to the existing `SET AUTOCOMMIT` condition:
90+
91+
```cpp
92+
||
93+
(!strncasecmp("SET @@session.autocommit", mb, strlen("SET @@session.autocommit")))
94+
```
95+
96+
Do not change `admin_handler_command_set()`: the command must be consumed before it reaches the global-variable translator.
97+
98+
- [ ] **Step 2: Rebuild cleanly and verify the focused TAP test is green**
99+
100+
```bash
101+
build_jobs=$(nproc)
102+
[ "$build_jobs" -gt 8 ] && build_jobs=8
103+
make clean
104+
make -j"$build_jobs"
105+
make -C test/tap/tests -j"$build_jobs" mysql-reg_test_5786_admin_strip_leading_sql_comments-t
106+
```
107+
108+
Run the focused group using the same exact isolated-harness command as the
109+
red phase, with a new `INFRA_ID` ending in `-green`. Confirm all old and new
110+
accept cases receive an OK packet with no result set, then run:
111+
112+
```bash
113+
git diff --check
114+
```
115+
116+
- [ ] **Step 3: Commit the minimal handler fix**
117+
118+
```bash
119+
git add lib/Admin_Handler.cpp test/tap/tests/mysql-reg_test_5786_admin_strip_leading_sql_comments-t.cpp
120+
git commit -m "fix(admin): accept session autocommit setup"
121+
```
122+
123+
### Task 3: Verify the real Connector/Python 26.7.0 path and publish
124+
125+
**Files:**
126+
- Modify: no additional source files
127+
- Test: `test/scripts/mysqlx/behavioral_validation.py` through the existing `mysqlx-soak-g1` harness
128+
129+
**Interfaces:**
130+
- Consumes: the image built with `MYSQL_CONNECTOR_PYTHON_VERSION=26.7.0` and the existing behavioral-validation Admin connection.
131+
- Produces: evidence that the connector reaches its Admin delete/reload actions rather than failing during post-connect setup.
132+
133+
- [ ] **Step 1: Build the test image with the compatibility connector version**
134+
135+
```bash
136+
docker build --network host \
137+
--build-arg MYSQL_CONNECTOR_PYTHON_VERSION=26.7.0 \
138+
-t proxysql-ci-base:mysqlx-connector-26.7.0 \
139+
-f test/infra/docker-base/Dockerfile test/infra/docker-base
140+
```
141+
142+
- [ ] **Step 2: Run the existing MySQLX soak harness against that image**
143+
144+
The isolation scripts consume `proxysql-ci-base:latest`, so temporarily
145+
point that local compatibility alias at the versioned image, run only the
146+
behavioral test, and restore the 9.7.0 alias afterwards:
147+
148+
```bash
149+
docker tag proxysql-ci-base:mysqlx-connector-26.7.0 proxysql-ci-base:latest
150+
export INFRA_ID="admin-session-autocommit-mysqlx-$(date +%s)"
151+
export TAP_GROUP=mysqlx-soak-g1
152+
export TEST_PY_TAP_INCL='test_mysqlx_soak_behavioral-t'
153+
export SKIP_CLUSTER_START=1
154+
trap 'test/infra/control/stop-proxysql-isolated.bash || true; test/infra/control/destroy-infras.bash || true; docker tag proxysql-ci-base:mysqlx-connector-9.7.0 proxysql-ci-base:latest' EXIT
155+
test/infra/control/ensure-infras.bash
156+
test/infra/control/run-tests-isolated.bash
157+
```
158+
159+
Verify that `mysql.connector.connect()` reaches the Admin delete/reload
160+
actions instead of emitting `Unknown global variable:
161+
'@@session.autocommit'`; assess any subsequent route-reload result
162+
separately.
163+
164+
- [ ] **Step 3: Publish the focused branch as a pull request**
165+
166+
Rebase or merge the current `origin/v3.0` only if it has advanced, run `git diff --check`, push `fix/admin-session-autocommit`, and open a PR targeting `v3.0`. The PR description must state the pure-Python Connector/Python fallback mechanism, the Admin no-op behavior, focused TAP evidence, and that #5984 must land before CI can exercise the 26.7.0 soak matrix.
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
# Admin session-autocommit Connector/Python compatibility
2+
3+
## Goal
4+
5+
Allow MySQL Connector/Python 26.7.0 to connect to ProxySQL's classic Admin
6+
interface without treating its session-scoped autocommit setup statement as a
7+
ProxySQL global-variable update.
8+
9+
## Root cause
10+
11+
The 26.7.0 C extension cannot load in the CI base image because it requires
12+
`OPENSSL_3.2.0`. Connector/Python therefore uses its pure-Python connection,
13+
whose post-connect setup issues:
14+
15+
```sql
16+
SET @@session.autocommit = OFF
17+
```
18+
19+
`admin_session_handler()` already consumes `SET AUTOCOMMIT` as an accepted
20+
connect-setup no-op. Its matcher does not recognize the canonical
21+
`@@session.autocommit` spelling, so the command instead reaches
22+
`admin_handler_command_set()`, where it is rejected as an unknown ProxySQL
23+
global variable.
24+
25+
## Design
26+
27+
Extend the existing Admin connect-setup acceptance block to recognize the
28+
case-insensitive `SET @@session.autocommit` form. It will return the same OK
29+
packet and make no configuration or transaction-state change, exactly matching
30+
the current Admin treatment of bare `SET AUTOCOMMIT`.
31+
32+
No attempt will be made to implement stateful MySQL transaction semantics on
33+
the SQLite-backed Admin interface. `SELECT @@session.autocommit` is also out of
34+
scope: the observed connector connection path requires only the `SET` command.
35+
36+
## Regression coverage
37+
38+
Extend the existing Admin connect-setup TAP regression test with the exact
39+
Connector/Python syntax for both `OFF` and `ON`, including a leading SQL-comment
40+
variant. The test will verify an OK packet and no result set, and will continue
41+
to cover the existing bare spelling and unrelated setup statements.
42+
43+
## Verification
44+
45+
Run the focused TAP test after a clean build, then run the MySQLX soak scenario
46+
with Connector/Python 26.7.0 once PR #5984's portable pin-check fix is merged.
47+
The resulting matrix leg must reach the actual soak test without failure
48+
masking.

include/MySQL_Authentication.hpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,17 @@ class MySQL_Authentication {
7474
std::unique_ptr<SQLite3_result> mysql_users_resultset { nullptr };
7575
creds_group_t creds_backends;
7676
creds_group_t creds_frontends;
77+
/**
78+
* @brief Scope holding 'admin-admin_credentials' and 'admin-stats_credentials'.
79+
* @details Separate from 'creds_frontends' so an Admin credential and a
80+
* 'mysql_users' row of the same name cannot overwrite each other. Only
81+
* populated when PROXYSQL31 is defined (see ADMIN_CRED_SCOPE); on the stable
82+
* tier it stays empty and admin credentials remain in 'creds_frontends'.
83+
* Never included in 'dump_all_users()', so 'runtime_mysql_users' and the
84+
* cluster checksum are unaffected.
85+
*/
86+
creds_group_t creds_admins;
87+
creds_group_t& creds_for(enum cred_username_type usertype);
7788
bool _reset(enum cred_username_type usertype);
7889
uint64_t _get_runtime_checksum(enum cred_username_type usertype);
7990
public:

include/PgSQL_Authentication.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,16 @@ class PgSQL_Authentication {
6969
std::unique_ptr<SQLite3_result> pgsql_users_resultset { nullptr };
7070
creds_group_t creds_backends;
7171
creds_group_t creds_frontends;
72+
/**
73+
* @brief Scope holding 'admin-admin_credentials' / 'admin-stats_credentials'.
74+
* @details Mirrors MySQL_Authentication::creds_admins. Only populated when
75+
* PROXYSQL31 is defined (see ADMIN_CRED_SCOPE in MySQL_Authentication.hpp);
76+
* on the stable tier admin credentials stay in 'creds_frontends' alongside
77+
* 'pgsql_users' and behaviour is unchanged. Never walked by
78+
* dump_all_users(), so 'runtime_pgsql_users' and the checksum are unaffected.
79+
*/
80+
creds_group_t creds_admins;
81+
creds_group_t& creds_for(enum cred_username_type usertype);
7282
bool _reset(enum cred_username_type usertype);
7383
uint64_t _get_runtime_checksum(enum cred_username_type usertype);
7484
public:

include/proxysql_structs.h

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,11 @@ enum log_event_type {
4747
PROXYSQL_METADATA
4848
};
4949

50-
enum cred_username_type { USERNAME_BACKEND, USERNAME_FRONTEND, USERNAME_NONE };
50+
// USERNAME_ADMIN is a scope for 'admin-admin_credentials' / 'admin-stats_credentials'.
51+
// It is compiled unconditionally, but only *used* when PROXYSQL31 is defined --
52+
// see ADMIN_CRED_SCOPE in MySQL_Authentication.hpp. On the stable tier those
53+
// credentials continue to live in USERNAME_FRONTEND alongside mysql_users.
54+
enum cred_username_type { USERNAME_BACKEND, USERNAME_FRONTEND, USERNAME_NONE, USERNAME_ADMIN };
5155

5256
#define PROXYSQL_USE_RESULT
5357

@@ -771,6 +775,42 @@ enum proxysql_session_type {
771775
PROXYSQL_SESSION_NONE
772776
};
773777

778+
/**
779+
* @brief The credential scope holding 'admin-admin_credentials' and
780+
* 'admin-stats_credentials'.
781+
*
782+
* @details Historically these shared USERNAME_FRONTEND with mysql_users /
783+
* pgsql_users -- one flat map keyed by username -- so an Admin credential and
784+
* a row of the same name overwrote each other. That is the only reason the
785+
* documentation states those users cannot also appear in mysql_users.
786+
*
787+
* From the Innovative tier onward they get their own scope, removing the
788+
* collision rather than policing it. This is an INCOMPATIBLE change (a
789+
* colliding name currently resolves to one entry; afterwards the two are
790+
* independent), so it is gated to PROXYSQL31. On the stable tier this is
791+
* USERNAME_FRONTEND, every call site passes what it always passed, and
792+
* behaviour is unchanged. See issue #5987.
793+
*/
794+
#ifdef PROXYSQL31
795+
#define ADMIN_CRED_SCOPE USERNAME_ADMIN
796+
#else
797+
#define ADMIN_CRED_SCOPE USERNAME_FRONTEND
798+
#endif /* PROXYSQL31 */
799+
800+
/**
801+
* @brief Credential scope to resolve a username in, for a given session type.
802+
* @details ADMIN and STATS sessions use ADMIN_CRED_SCOPE; every other session
803+
* type (MySQL/PgSQL frontend, SQLite server, ClickHouse) uses
804+
* USERNAME_FRONTEND. Shared by both protocol implementations so the policy
805+
* exists once.
806+
*/
807+
static inline enum cred_username_type cred_scope_for_session(enum proxysql_session_type session_type) {
808+
if (session_type == PROXYSQL_SESSION_ADMIN || session_type == PROXYSQL_SESSION_STATS) {
809+
return ADMIN_CRED_SCOPE;
810+
}
811+
return USERNAME_FRONTEND;
812+
}
813+
774814
#endif /* PROXYSQL_ENUMS */
775815

776816

0 commit comments

Comments
 (0)