Add ProxySQL provider with config generation and binary detection#45
Add ProxySQL provider with config generation and binary detection#45renecannao merged 2 commits intomasterfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 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.
Pull request overview
Adds a new proxysql deployment provider to the dbdeployer provider framework, enabling ProxySQL sandbox creation with generated configuration and lifecycle scripts, and registering it in the CLI at startup.
Changes:
- Introduces
providers/proxysqlwithProxySQLProviderimplementation (binary detection, sandbox creation, start/stop). - Adds config generation for
proxysql.cnf(including optional backend server entries). - Registers the provider from
cmd/root.goand includes unit tests for provider/config/registry behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| providers/proxysql/proxysql.go | Implements ProxySQL provider: binary lookup, sandbox creation (config + scripts), start/stop, backend parsing |
| providers/proxysql/config.go | Generates ProxySQL config text from a Go struct (with optional backends) |
| providers/proxysql/proxysql_test.go | Unit tests for provider name/version validation/registry integration and FindBinary behavior |
| providers/proxysql/config_test.go | Unit tests for config generation (basic + with backends) |
| cmd/root.go | Registers the new proxysql provider in the default provider registry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "use_proxy": fmt.Sprintf("#!/bin/bash\nmysql -h %s -P %d -u %s -p%s --prompt 'ProxySQL> ' \"$@\"\n", | ||
| host, mysqlPort, monitorUser, monitorPass), | ||
| } | ||
|
|
||
| for name, content := range scripts { | ||
| scriptPath := filepath.Join(config.Dir, name) | ||
| if err := os.WriteFile(scriptPath, []byte(content), 0755); err != nil { |
There was a problem hiding this comment.
The generated use/use_proxy scripts embed passwords directly on the mysql client command line (-p<pass>), which can leak via process listings and are also stored in world-readable executable files (0755). Prefer prompting for the password, using a restricted-permission defaults file (e.g., my.proxy.cnf mode 0600), or passing creds via environment with tight file perms (and consider 0700 for the scripts directory/files).
| "use_proxy": fmt.Sprintf("#!/bin/bash\nmysql -h %s -P %d -u %s -p%s --prompt 'ProxySQL> ' \"$@\"\n", | |
| host, mysqlPort, monitorUser, monitorPass), | |
| } | |
| for name, content := range scripts { | |
| scriptPath := filepath.Join(config.Dir, name) | |
| if err := os.WriteFile(scriptPath, []byte(content), 0755); err != nil { | |
| "use_proxy": fmt.Sprintf("#!/bin/bash\nmysql -h %s -P %d -u %s --prompt 'ProxySQL> ' \"$@\"\n", | |
| host, mysqlPort, monitorUser), | |
| } | |
| for name, content := range scripts { | |
| scriptPath := filepath.Join(config.Dir, name) | |
| if err := os.WriteFile(scriptPath, []byte(content), 0700); err != nil { |
| parts := strings.Split(entry, ":") | ||
| if len(parts) >= 3 { | ||
| port, _ := strconv.Atoi(parts[1]) | ||
| hg, _ := strconv.Atoi(parts[2]) | ||
| backends = append(backends, BackendServer{ | ||
| Host: parts[0], Port: port, Hostgroup: hg, MaxConns: 200, | ||
| }) | ||
| } |
There was a problem hiding this comment.
parseBackends silently ignores malformed backend entries and ignores Atoi errors (defaulting port/hostgroup to 0). This can generate an invalid config without surfacing an error to the user. Consider validating each entry (exactly 3 fields, successful int parses, port>0, hostgroup>=0) and either returning an error from CreateSandbox or skipping invalid entries with a clear warning/error message.
| parts := strings.Split(entry, ":") | |
| if len(parts) >= 3 { | |
| port, _ := strconv.Atoi(parts[1]) | |
| hg, _ := strconv.Atoi(parts[2]) | |
| backends = append(backends, BackendServer{ | |
| Host: parts[0], Port: port, Hostgroup: hg, MaxConns: 200, | |
| }) | |
| } | |
| entry = strings.TrimSpace(entry) | |
| if entry == "" { | |
| continue | |
| } | |
| parts := strings.Split(entry, ":") | |
| if len(parts) != 3 { | |
| fmt.Fprintf(os.Stderr, "proxysql: ignoring invalid backend %q (expected host:port:hostgroup)\n", entry) | |
| continue | |
| } | |
| host := strings.TrimSpace(parts[0]) | |
| if host == "" { | |
| fmt.Fprintf(os.Stderr, "proxysql: ignoring backend %q with empty host\n", entry) | |
| continue | |
| } | |
| port, err := strconv.Atoi(strings.TrimSpace(parts[1])) | |
| if err != nil || port <= 0 { | |
| fmt.Fprintf(os.Stderr, "proxysql: ignoring backend %q with invalid port %q\n", entry, parts[1]) | |
| continue | |
| } | |
| hg, err := strconv.Atoi(strings.TrimSpace(parts[2])) | |
| if err != nil || hg < 0 { | |
| fmt.Fprintf(os.Stderr, "proxysql: ignoring backend %q with invalid hostgroup %q\n", entry, parts[2]) | |
| continue | |
| } | |
| backends = append(backends, BackendServer{ | |
| Host: host, Port: port, Hostgroup: hg, MaxConns: 200, | |
| }) |
| if err := mysqlprovider.Register(providers.DefaultRegistry); err != nil { | ||
| panic(fmt.Sprintf("failed to register MySQL provider: %v", err)) | ||
| } | ||
| _ = proxysqlprovider.Register(providers.DefaultRegistry) |
There was a problem hiding this comment.
This ignores any error from ProxySQL provider registration. Registry.Register only errors on programming/configuration issues (e.g., duplicate provider name), not on missing binaries, so swallowing the error could hide a real startup bug. Consider handling the error explicitly (e.g., print a warning to stderr) while still keeping it non-fatal.
| _ = proxysqlprovider.Register(providers.DefaultRegistry) | |
| if err := proxysqlprovider.Register(providers.DefaultRegistry); err != nil { | |
| fmt.Fprintf(os.Stderr, "warning: failed to register ProxySQL provider: %v\n", err) | |
| } |
| "start": fmt.Sprintf("#!/bin/bash\ncd %s\n%s --initial -c %s -D %s &\nSBPID=$!\necho $SBPID > %s/proxysql.pid\nsleep 2\nif kill -0 $SBPID 2>/dev/null; then\n echo 'ProxySQL started (pid '$SBPID')'\nelse\n echo 'ProxySQL failed to start'\n exit 1\nfi\n", | ||
| config.Dir, binaryPath, cfgPath, dataDir, config.Dir), | ||
| "stop": fmt.Sprintf("#!/bin/bash\nPIDFILE=%s/proxysql.pid\nif [ -f $PIDFILE ]; then\n PID=$(cat $PIDFILE)\n kill $PID 2>/dev/null\n sleep 1\n kill -0 $PID 2>/dev/null && kill -9 $PID 2>/dev/null\n rm -f $PIDFILE\n echo 'ProxySQL stopped'\nelse\n echo 'ProxySQL not running (no pid file)'\nfi\n", | ||
| config.Dir), | ||
| "status": fmt.Sprintf("#!/bin/bash\nPIDFILE=%s/proxysql.pid\nif [ -f $PIDFILE ] && kill -0 $(cat $PIDFILE) 2>/dev/null; then\n echo 'ProxySQL running (pid '$(cat $PIDFILE)')'\nelse\n echo 'ProxySQL not running'\n exit 1\nfi\n", |
There was a problem hiding this comment.
The generated shell scripts interpolate paths (cd <dir>, binaryPath, cfgPath, dataDir, PIDFILE) without any quoting/escaping. If the sandbox dir or binary path contains spaces or shell metacharacters, the scripts can break or behave unexpectedly. Consider shell-escaping/quoting these interpolated values consistently.
| "start": fmt.Sprintf("#!/bin/bash\ncd %s\n%s --initial -c %s -D %s &\nSBPID=$!\necho $SBPID > %s/proxysql.pid\nsleep 2\nif kill -0 $SBPID 2>/dev/null; then\n echo 'ProxySQL started (pid '$SBPID')'\nelse\n echo 'ProxySQL failed to start'\n exit 1\nfi\n", | |
| config.Dir, binaryPath, cfgPath, dataDir, config.Dir), | |
| "stop": fmt.Sprintf("#!/bin/bash\nPIDFILE=%s/proxysql.pid\nif [ -f $PIDFILE ]; then\n PID=$(cat $PIDFILE)\n kill $PID 2>/dev/null\n sleep 1\n kill -0 $PID 2>/dev/null && kill -9 $PID 2>/dev/null\n rm -f $PIDFILE\n echo 'ProxySQL stopped'\nelse\n echo 'ProxySQL not running (no pid file)'\nfi\n", | |
| config.Dir), | |
| "status": fmt.Sprintf("#!/bin/bash\nPIDFILE=%s/proxysql.pid\nif [ -f $PIDFILE ] && kill -0 $(cat $PIDFILE) 2>/dev/null; then\n echo 'ProxySQL running (pid '$(cat $PIDFILE)')'\nelse\n echo 'ProxySQL not running'\n exit 1\nfi\n", | |
| "start": fmt.Sprintf("#!/bin/bash\ncd \"%s\"\n\"%s\" --initial -c \"%s\" -D \"%s\" &\nSBPID=$!\necho \"$SBPID\" > \"%s/proxysql.pid\"\nsleep 2\nif kill -0 \"$SBPID\" 2>/dev/null; then\n echo 'ProxySQL started (pid '$SBPID')'\nelse\n echo 'ProxySQL failed to start'\n exit 1\nfi\n", | |
| config.Dir, binaryPath, cfgPath, dataDir, config.Dir), | |
| "stop": fmt.Sprintf("#!/bin/bash\nPIDFILE=\"%s/proxysql.pid\"\nif [ -f \"$PIDFILE\" ]; then\n PID=$(cat \"$PIDFILE\")\n kill \"$PID\" 2>/dev/null\n sleep 1\n kill -0 \"$PID\" 2>/dev/null && kill -9 \"$PID\" 2>/dev/null\n rm -f \"$PIDFILE\"\n echo 'ProxySQL stopped'\nelse\n echo 'ProxySQL not running (no pid file)'\nfi\n", | |
| config.Dir), | |
| "status": fmt.Sprintf("#!/bin/bash\nPIDFILE=\"%s/proxysql.pid\"\nif [ -f \"$PIDFILE\" ] && kill -0 \"$(cat \"$PIDFILE\")\" 2>/dev/null; then\n echo 'ProxySQL running (pid '$(cat \"$PIDFILE\")')'\nelse\n echo 'ProxySQL not running'\n exit 1\nfi\n", |
| if adminPort == 0 { | ||
| adminPort = config.Port | ||
| } | ||
| mysqlPort := adminPort + 1 |
There was a problem hiding this comment.
Port assignment looks inverted vs SandboxConfig semantics: SandboxConfig.Port is documented as the primary port and AdminPort as the management port, but when AdminPort==0 this code uses config.Port as the admin port and derives mysqlPort=adminPort+1. That makes it hard for callers to set/understand ports, and SandboxInfo.Port later returns the admin port. Consider treating config.Port as the MySQL listener (6033) and config.AdminPort as the admin interface (6032), or at least returning mysqlPort as SandboxInfo.Port (and exposing admin port via another field/option).
| if adminPort == 0 { | |
| adminPort = config.Port | |
| } | |
| mysqlPort := adminPort + 1 | |
| mysqlPort := config.Port | |
| // Ensure ports follow documented semantics: | |
| // - config.Port is the primary/MySQL listener (default 6033) | |
| // - config.AdminPort is the management/admin interface (default 6032) | |
| // Derive the missing one if only one is provided, or use defaults if both are zero. | |
| if adminPort == 0 && mysqlPort == 0 { | |
| // Neither port provided: fall back to provider defaults (6032 admin, 6033 MySQL). | |
| defaultPorts := p.DefaultPorts() | |
| adminPort = defaultPorts.BasePort | |
| mysqlPort = adminPort + 1 | |
| } else { | |
| if adminPort == 0 { | |
| // Only MySQL port provided: derive admin port just below it. | |
| adminPort = mysqlPort - 1 | |
| } else if mysqlPort == 0 { | |
| // Only admin port provided: derive MySQL port just above it. | |
| mysqlPort = adminPort + 1 | |
| } | |
| } |
|
|
||
| cfgContent := GenerateConfig(proxyCfg) | ||
| cfgPath := filepath.Join(config.Dir, "proxysql.cnf") | ||
| if err := os.WriteFile(cfgPath, []byte(cfgContent), 0644); err != nil { |
There was a problem hiding this comment.
The generated config contains admin/monitor credentials but is written with mode 0644, making it world-readable on multi-user systems. Use a more restrictive permission (e.g., 0600) or avoid writing secrets into the config when possible.
| if err := os.WriteFile(cfgPath, []byte(cfgContent), 0644); err != nil { | |
| if err := os.WriteFile(cfgPath, []byte(cfgContent), 0600); err != nil { |
| "start": fmt.Sprintf("#!/bin/bash\ncd %s\n%s --initial -c %s -D %s &\nSBPID=$!\necho $SBPID > %s/proxysql.pid\nsleep 2\nif kill -0 $SBPID 2>/dev/null; then\n echo 'ProxySQL started (pid '$SBPID')'\nelse\n echo 'ProxySQL failed to start'\n exit 1\nfi\n", | ||
| config.Dir, binaryPath, cfgPath, dataDir, config.Dir), |
There was a problem hiding this comment.
The start script always runs ProxySQL with --initial. ProxySQL uses --initial to reset/empty the on-disk SQLite DB in the datadir, so using it on every start will discard persisted config on restarts. The start script should only use --initial for first bootstrap (e.g., when the DB file doesn't exist), and do normal starts without it thereafter.
| "start": fmt.Sprintf("#!/bin/bash\ncd %s\n%s --initial -c %s -D %s &\nSBPID=$!\necho $SBPID > %s/proxysql.pid\nsleep 2\nif kill -0 $SBPID 2>/dev/null; then\n echo 'ProxySQL started (pid '$SBPID')'\nelse\n echo 'ProxySQL failed to start'\n exit 1\nfi\n", | |
| config.Dir, binaryPath, cfgPath, dataDir, config.Dir), | |
| "start": fmt.Sprintf("#!/bin/bash\ncd %s\nDBFILE=%s/proxysql.db\nINITIAL=\"\"\nif [ ! -f \"$DBFILE\" ]; then\n INITIAL=\"--initial\"\nfi\n%s $INITIAL -c %s -D %s &\nSBPID=$!\necho $SBPID > %s/proxysql.pid\nsleep 2\nif kill -0 $SBPID 2>/dev/null; then\n echo 'ProxySQL started (pid '$SBPID')'\nelse\n echo 'ProxySQL failed to start'\n exit 1\nfi\n", | |
| config.Dir, dataDir, binaryPath, cfgPath, dataDir, config.Dir), |
Summary
providers/proxysql/) implementing theProviderinterface with config generation, binary detection viaexec.LookPath, and lifecycle scripts (start/stop/status/use/use_proxy)cmd/root.go(non-fatal if binary not installed)Test plan
go test ./providers/... -v-- all tests pass (FindBinary skipped when proxysql not installed)go buildsucceeds./dbdeployer providersshows both mysql and proxysql providersCloses #37
Closes #38