feat: use rsandbox as ProxySQL monitor, add R/W split proxy users (#69, #70)#73
feat: use rsandbox as ProxySQL monitor, add R/W split proxy users (#69, #70)#73renecannao merged 1 commit intomasterfrom
Conversation
#69, #70) - Change monitor_user/monitor_password from msandbox to rsandbox (replication role, USAGE + REPLICATION CLIENT only) in proxysql_topology.go and proxysql.go defaults - Add three mysql_users entries for MySQL backends: msandbox (HG0, general purpose), msandbox_rw (HG0, explicit writer), msandbox_ro (HG1, explicit reader) - PostgreSQL backends keep single user entry (no R/W split) - Fix use_proxy script to connect as msandbox app user (not monitor user) - Update config_test.go to verify rsandbox monitor and three-user mysql_users - Update proxysql-integration-tests.sh to check rsandbox monitor and _rw/_ro users
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughProxySQL configuration generation now differentiates between PostgreSQL and MySQL backends. PostgreSQL deployments continue using a single monitor user from configuration, while MySQL deployments switch to three hardcoded proxy users ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the ProxySQL provider to support read/write splitting for MySQL by introducing three distinct proxy users (msandbox, msandbox_rw, and msandbox_ro) while maintaining a single user for PostgreSQL. It also changes the default monitor credentials from msandbox to rsandbox across the configuration, scripts, and tests. Feedback highlights that the global change to rsandbox may break PostgreSQL compatibility and that hardcoding MySQL proxy users and credentials in the use_proxy script reduces flexibility and prevents users from customizing application credentials as they could previously.
| if monitorUser == "" { | ||
| monitorUser = "msandbox" | ||
| monitorUser = "rsandbox" | ||
| } | ||
| monitorPass := config.Options["monitor_password"] | ||
| if monitorPass == "" { | ||
| monitorPass = "msandbox" | ||
| monitorPass = "rsandbox" | ||
| } |
There was a problem hiding this comment.
The default monitor user and password are changed to rsandbox globally. This will likely break PostgreSQL sandboxes, which typically use postgres or msandbox and do not recognize the rsandbox user or the MySQL-specific replication privileges mentioned in the PR description. The default should be conditional on the backend provider.
| // MySQL: three users — general purpose, dedicated writer, dedicated reader | ||
| b.WriteString(fmt.Sprintf("%s=\n(\n", usersKey)) | ||
| b.WriteString(" {\n") | ||
| b.WriteString(" username=\"msandbox\"\n") | ||
| b.WriteString(" password=\"msandbox\"\n") | ||
| b.WriteString(" default_hostgroup=0\n") | ||
| b.WriteString(" },\n") | ||
| b.WriteString(" {\n") | ||
| b.WriteString(" username=\"msandbox_rw\"\n") | ||
| b.WriteString(" password=\"msandbox\"\n") | ||
| b.WriteString(" default_hostgroup=0\n") | ||
| b.WriteString(" },\n") | ||
| b.WriteString(" {\n") | ||
| b.WriteString(" username=\"msandbox_ro\"\n") | ||
| b.WriteString(" password=\"msandbox\"\n") | ||
| b.WriteString(" default_hostgroup=1\n") | ||
| b.WriteString(" }\n") | ||
| b.WriteString(")\n") |
There was a problem hiding this comment.
The MySQL proxy users are now hardcoded to msandbox variants. This is inconsistent with the PostgreSQL implementation (lines 87-93) which still uses the configurable MonitorUser. This change prevents users from customizing the application user credentials via the monitor_user option as they could before. Consider allowing these to be configurable or at least using the monitor credentials as a fallback.
| scripts["use_proxy"] = fmt.Sprintf("#!/bin/bash\nmysql -h %s -P %d -u msandbox -pmsandbox --prompt 'ProxySQL> ' \"$@\"\n", | ||
| host, mysqlPort) |
There was a problem hiding this comment.
The use_proxy script for MySQL hardcodes the username and password (-u msandbox -pmsandbox). This makes the script fragile if the user credentials in the configuration are ever changed. It also makes it difficult to override the user via command-line arguments as the hardcoded flags will still be present and may conflict with user-provided arguments.
There was a problem hiding this comment.
Pull request overview
This PR updates the ProxySQL addon configuration to use a least-privilege monitoring user and to introduce distinct ProxySQL frontend users for read/write split testing, while adjusting helper scripts and tests accordingly.
Changes:
- Switch ProxySQL monitor credentials from
msandboxtorsandbox. - Generate MySQL ProxySQL
mysql_usersentries formsandbox,msandbox_rw(HG0), andmsandbox_ro(HG1) to support explicit R/W split. - Update
use_proxyscript behavior and integration/unit tests to reflect the new monitor/proxy users.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/proxysql-integration-tests.sh |
Updates integration checks to validate rsandbox monitoring and presence of msandbox_rw/msandbox_ro users. |
sandbox/proxysql_topology.go |
Changes ProxySQL deployment defaults to use rsandbox monitor credentials. |
providers/proxysql/proxysql.go |
Changes default monitor credentials and updates use_proxy script generation for MySQL to use msandbox. |
providers/proxysql/config.go |
Adjusts generated ProxySQL config: PostgreSQL keeps single user; MySQL gets three frontend users for R/W split. |
providers/proxysql/config_test.go |
Updates tests to expect rsandbox monitor user and asserts presence of the new MySQL frontend users. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Options: map[string]string{ | ||
| "monitor_user": "msandbox", | ||
| "monitor_password": "msandbox", | ||
| "monitor_user": "rsandbox", | ||
| "monitor_password": "rsandbox", | ||
| "backends": strings.Join(backendParts, ","), | ||
| "backend_provider": backendProvider, |
There was a problem hiding this comment.
DeployProxySQLForTopology hard-codes monitor_user/monitor_password to rsandbox. This breaks --with-proxysql for PostgreSQL sandboxes (cmd/single.go passes backendProvider="postgresql" but PostgreSQL sandboxes use DbUser="postgres"), causing ProxySQL to be configured with a non-existent monitoring/frontend user. Please set monitor credentials based on backendProvider (e.g., default to postgres for postgresql) and/or accept monitor_user/monitor_password as inputs instead of hard-coding them here.
| // MySQL: three users — general purpose, dedicated writer, dedicated reader | ||
| b.WriteString(fmt.Sprintf("%s=\n(\n", usersKey)) | ||
| b.WriteString(" {\n") | ||
| b.WriteString(" username=\"msandbox\"\n") | ||
| b.WriteString(" password=\"msandbox\"\n") | ||
| b.WriteString(" default_hostgroup=0\n") | ||
| b.WriteString(" },\n") | ||
| b.WriteString(" {\n") | ||
| b.WriteString(" username=\"msandbox_rw\"\n") | ||
| b.WriteString(" password=\"msandbox\"\n") | ||
| b.WriteString(" default_hostgroup=0\n") | ||
| b.WriteString(" },\n") | ||
| b.WriteString(" {\n") | ||
| b.WriteString(" username=\"msandbox_ro\"\n") | ||
| b.WriteString(" password=\"msandbox\"\n") | ||
| b.WriteString(" default_hostgroup=1\n") |
There was a problem hiding this comment.
GenerateConfig() now hard-codes MySQL frontend users/passwords to msandbox/msandbox (and msandbox_rw/msandbox_ro). This makes ProxySQL config incorrect when the sandbox is deployed with non-default --db-user/--db-password (the grants templates create these users with {{.DbUser}}/{{.DbPassword}}). Consider deriving these usernames/passwords from inputs (e.g., add fields to ProxySQLConfig or provider options for proxy user/password) so ProxySQL stays in sync with the sandbox credentials.
| // use_proxy connects as msandbox (app user), not the monitor user (rsandbox) | ||
| scripts["use_proxy"] = fmt.Sprintf("#!/bin/bash\nmysql -h %s -P %d -u msandbox -pmsandbox --prompt 'ProxySQL> ' \"$@\"\n", | ||
| host, mysqlPort) |
There was a problem hiding this comment.
The generated use_proxy script is now hard-coded to connect as msandbox/msandbox for MySQL. This will fail for sandboxes created with custom --db-user/--db-password (and also makes it impossible to easily test msandbox_rw/msandbox_ro via the helper script). Suggest making the proxy client credentials configurable (e.g., via provider options) and/or generating separate helper scripts for rw/ro users.
Summary
msandbox(full privileges) torsandbox(replication role) — only needs USAGE + REPLICATION CLIENT for health checksmsandboxdefault_hostgroup=0 (general purpose)msandbox_rwdefault_hostgroup=0 (explicit writer)msandbox_rodefault_hostgroup=1 (explicit reader)use_proxyscript connects asmsandbox(app user), not monitor userCloses #69, closes #70
Test Plan
Summary by CodeRabbit
Changes
msandboxtorsandboxmsandbox,msandbox_rw,msandbox_ro) for read/write split operationsTests