Fix ports/scripts/sandbox-directory for PostgreSQL+ProxySQL#125
Fix ports/scripts/sandbox-directory for PostgreSQL+ProxySQL#125ronaldbradford wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to specify custom sandbox directory names and a force-overwrite flag for replication deployments, including logic to stop existing instances before removal. It also significantly improves ProxySQL support for PostgreSQL backends by implementing dedicated admin interface configurations, dynamic port allocation to avoid conflicts, and PostgreSQL-specific connection scripts. A review comment correctly identified a bug in the script generation where monitor credentials were incorrectly used for the ProxySQL admin interface instead of the required admin credentials.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/replication.go`:
- Around line 87-89: The current error in the !force branch calls common.Exitf
without telling users how to fix it; update the message in the conditional that
checks the force parameter (where topologyDir is used and common.Exitf is
called) to include exact command suggestions such as rerunning the command with
--force to overwrite or with --sandbox-directory=<new-path> to choose a
different directory, e.g. "sandbox directory %s already exists; run '<command>
--force' to overwrite or '<command> --sandbox-directory %s' to use a different
path", replacing <command> placeholder with the actual CLI invocation in your
codebase.
- Around line 91-102: The stop script invocation currently ignores errors (_, _
= common.RunCmd(stopScript)), which is inconsistent with the rest of the
codebase; change it to capture the error from common.RunCmd(stopScript) and
surface it instead of silencing it — e.g., assign the error and call the project
helper that other files use (common.ErrCheckExitf or common.Exitf) with a
descriptive message including stopScript so failures abort/are reported; keep
the surrounding common.ExecExists(stopScript) check and topologyDir/entry.Name()
logic intact.
In `@providers/proxysql/proxysql.go`:
- Around line 115-122: The psql URI built in psqlURI with raw
monitorUser/monitorPass can break when credentials contain reserved characters;
update psqlURI to percent-encode userinfo by constructing a
url.UserPassword(monitorUser, monitorPass) and using that in the URL (or use
url.UserPassword(...).String()) so both scripts["use"] and scripts["use_proxy"]
receive a correctly encoded postgresql://... URI; modify the psqlURI closure to
create a net/url URL with Scheme "postgresql", Host set to fmt.Sprintf("%s:%d",
host, port) and User set via url.UserPassword(monitorUser, monitorPass) before
calling url.String().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ec57bea-848d-4dd2-b710-758c3318f789
📒 Files selected for processing (7)
cmd/replication.goproviders/postgresql/postgresql.goproviders/postgresql/postgresql_test.goproviders/proxysql/config.goproviders/proxysql/config_test.goproviders/proxysql/proxysql.gosandbox/proxysql_topology.go
| psqlURI := func(port int) string { | ||
| return fmt.Sprintf("postgresql://%s:%s@%s:%d", monitorUser, monitorPass, host, port) | ||
| } | ||
| scripts["use"] = fmt.Sprintf("#!/bin/bash\npsql %s \"$@\"\n", psqlURI(adminPort)) | ||
| scripts["use_proxy"] = fmt.Sprintf("#!/bin/bash\npsql %s \"$@\"\n", psqlURI(mysqlPort)) | ||
| } else { | ||
| scripts["use"] = fmt.Sprintf("#!/bin/bash\nmysql -h %s -P %d -u %s -p%s --prompt 'ProxySQL Admin> ' \"$@\"\n", | ||
| host, adminPort, adminUser, adminPassword) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In libpq/PostgreSQL URI connection strings (postgresql://user:pass@host:port), do special characters in username/password need percent-encoding?
💡 Result:
Yes, special characters in usernames and passwords within PostgreSQL URI connection strings must be percent-encoded [1][2][3]. Because the connection URI follows RFC 3986, characters that have special meaning in URI syntax—such as @, :, /, ?, #, and spaces—can cause the parser to misinterpret the structure of the string if they are included literally [4][5][1][2][3]. For example, an unencoded @ sign in a password would be incorrectly interpreted by the system as the delimiter separating the userinfo from the host [5][3]. According to the official PostgreSQL documentation, you should use percent-encoding for any symbols that have special meaning in any part of the URI [1][2]. Common examples include: - Space: %20 [1][2] - At sign (@): %40 [5][3] - Forward slash (/): %2F [5][2] - Equals sign (=): %3D [1][2] To avoid issues with manual encoding, many developers prefer to use native URL-encoding libraries (such as urllib.parse.quote in Python) or object-based configuration methods (like SQLAlchemy's URL.create()) provided by their database drivers, which handle this requirement automatically [5]. Alternatively, if percent-encoding is difficult to manage in your environment, you can use the non-URI "keyword=value" connection string format, which does not require percent-encoding [2][3].
Citations:
- 1: https://www.postgresql.org/docs/16/libpq-connect.html
- 2: https://www.postgresql.org/docs/current/libpq-connect.html
- 3: https://pulse.support/kb/postgresql-connection-string
- 4: postgres passwords with special characters can cause failures and partial leak of password pressly/goose#935
- 5: https://medium.com/@anshul.suresh99/how-to-handle-and-other-special-characters-in-database-passwords-with-sqlalchemy-ed25e90bd97a
🏁 Script executed:
# Check the actual code at lines 115-122 in providers/proxysql/proxysql.go
sed -n '110,125p' providers/proxysql/proxysql.goRepository: ProxySQL/dbdeployer
Length of output: 1075
🏁 Script executed:
# Verify net/url.UserPassword() behavior with Go documentation/testing
python3 << 'EOF'
import subprocess
import re
# Check what Go's url.UserPassword does
code = '''package main
import (
"fmt"
"net/url"
)
func main() {
// Test with special characters
u := &url.URL{
Scheme: "postgresql",
User: url.UserPassword("user@example", "pass:word@123"),
Host: "localhost:5432",
}
fmt.Println("URL:", u.String())
// Also test what the userinfo looks like
fmt.Println("Userinfo:", u.User.Username(), u.User.String())
}
'''
with open('/tmp/test_url.go', 'w') as f:
f.write(code)
result = subprocess.run(['go', 'run', '/tmp/test_url.go'], capture_output=True, text=True, timeout=10)
print(result.stdout)
if result.stderr:
print("STDERR:", result.stderr)
EOFRepository: ProxySQL/dbdeployer
Length of output: 186
🏁 Script executed:
# Search for other similar PostgreSQL URI construction patterns in codebase
rg "postgresql://" --type go -B2 -A2Repository: ProxySQL/dbdeployer
Length of output: 488
Encode PostgreSQL URI credentials before writing psql scripts.
Line 116 interpolates raw monitorUser and monitorPass into a URI without percent-encoding. Reserved characters (@, :, /, ?, #) in credentials break URI parsing, making the use and use_proxy scripts fail for passwords with special characters. PostgreSQL's libpq follows RFC 3986, which requires percent-encoding of reserved chars in the userinfo component.
Use Go's net/url.UserPassword() to automatically handle encoding:
Proposed fix
import (
"fmt"
+ "net/url"
"os"
"os/exec"
"path/filepath"
"strconv"
"strings"
@@
if config.Options["backend_provider"] == "postgresql" {
psqlURI := func(port int) string {
- return fmt.Sprintf("postgresql://%s:%s@%s:%d", monitorUser, monitorPass, host, port)
+ u := &url.URL{
+ Scheme: "postgresql",
+ User: url.UserPassword(monitorUser, monitorPass),
+ Host: fmt.Sprintf("%s:%d", host, port),
+ }
+ return u.String()
}
scripts["use"] = fmt.Sprintf("#!/bin/bash\npsql %s \"$@\"\n", psqlURI(adminPort))
scripts["use_proxy"] = fmt.Sprintf("#!/bin/bash\npsql %s \"$@\"\n", psqlURI(mysqlPort))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@providers/proxysql/proxysql.go` around lines 115 - 122, The psql URI built in
psqlURI with raw monitorUser/monitorPass can break when credentials contain
reserved characters; update psqlURI to percent-encode userinfo by constructing a
url.UserPassword(monitorUser, monitorPass) and using that in the URL (or use
url.UserPassword(...).String()) so both scripts["use"] and scripts["use_proxy"]
receive a correctly encoded postgresql://... URI; modify the psqlURI closure to
create a net/url URL with Scheme "postgresql", Host set to fmt.Sprintf("%s:%d",
host, port) and User set via url.UserPassword(monitorUser, monitorPass) before
calling url.String().
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
providers/proxysql/proxysql.go (1)
115-116:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEncode PostgreSQL URI credentials before writing
psqlscripts.Line 116 interpolates raw
userandpasswordinto a URI without percent-encoding. Reserved characters (@,:,/,?,#) in credentials break URI parsing, making theuseanduse_proxyscripts fail for passwords with special characters. PostgreSQL's libpq follows RFC 3986, which requires percent-encoding of reserved chars in the userinfo component.Use Go's
net/url.UserPassword()to automatically handle encoding, as detailed in the previous review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/proxysql/proxysql.go` around lines 115 - 116, The psqlURI closure currently interpolates raw user/password into the URI (psqlURI), which fails for credentials containing reserved characters; update psqlURI to build the authority using net/url by calling url.UserPassword(user, password) and then using its String() (or setting User on a url.URL) so the userinfo is percent-encoded, and construct the final postgresql:// host:port URI from that encoded userinfo instead of raw interpolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@providers/proxysql/proxysql.go`:
- Around line 115-116: The psqlURI closure currently interpolates raw
user/password into the URI (psqlURI), which fails for credentials containing
reserved characters; update psqlURI to build the authority using net/url by
calling url.UserPassword(user, password) and then using its String() (or setting
User on a url.URL) so the userinfo is percent-encoded, and construct the final
postgresql:// host:port URI from that encoded userinfo instead of raw
interpolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a534ccd-c417-44b9-a19c-4118e31330e3
📒 Files selected for processing (1)
providers/proxysql/proxysql.go
1 participant
--sandbox-directory was not working with --provider=postgresql
Summary by CodeRabbit
New Features
--sandbox-directoryflag for custom storage locations.Bug Fixes
--forcestops existing sandboxes and cleans up before recreation; port selection avoids conflicts.Tests