fix: database auth#24
Conversation
There was a problem hiding this comment.
Pull request overview
Updates local-mode metadata expectations to reflect a new database authentication scheme returned by the local server container.
Changes:
- Updates the
database_urlJSON fixture used byFetchInfotests to use avolcano_client_{uuid}user andvpg_local_secretpassword. - Updates the corresponding
DatabaseURLassertion to match the new connection string.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tkkhq
left a comment
tkkhq
left a comment
There was a problem hiding this comment.
Automated inline review with priority tags. Overall the core database-auth alignment and the ANON_KEY_SECRET test guard are correct and safe; comments below are the actionable items.
[P2] Scope/title mismatch — the PR is titled fix: database auth, but only ~4 lines actually concern DB auth. The plan-limit overhaul, duration reformatting, Stripe removal, and first-party-bootstrap vars are independent changes bundled in, which hurts reviewability and makes revert/bisect coarse. Consider splitting into separate PRs/commits with accurate messages.
[P3] Empty PR description — given the breadth, a one-line note on which server release these env changes pair with would help reviewers and future archaeologists.
| // in the raw compose template, e.g. `REDIS_TIMEOUT: "60s"` -> "60s". | ||
| func composeTemplateEnvValue(t *testing.T, template, key string) string { | ||
| t.Helper() | ||
| re := regexp.MustCompile(`(?m)^\s*` + regexp.QuoteMeta(key) + `:\s*"?([^"\n]+)"?\s*$`) |
There was a problem hiding this comment.
The value regex here breaks if an inline comment is ever added to one of the three timing lines — and since these are quoted values, the failure is misleading. I verified the exact pattern against REDIS_TIMEOUT: "60s" # comment:
- Quoted + comment →
[^"\n]+stops at the opening quote and the closing"?\s*$can't consume the trailing# comment, soFindStringSubmatchreturns nil →require.Len(match, 2)fails with "missing env var" even though the var is present and valid. - Unquoted + comment → the comment gets swallowed into the capture group, then
time.ParseDuration("30s # comment")errors.
The runtime YAML/compose parser strips inline comments fine (container gets a clean value), so this is a test-only spurious failure — but the template already uses inline comments heavily, so it's an easy trap to fall into later. Suggest tolerating a trailing comment:
re := regexp.MustCompile(`(?m)^\s*` + regexp.QuoteMeta(key) + `:\s*"?([^"#\n]+?)"?\s*(?:#.*)?$`)There was a problem hiding this comment.
Non-blocking nit — approving regardless. None of the three lines carry inline comments today, so this is a latent trap rather than a live bug; fine to fold into a follow-up or ignore.
| assert.Equal(t, "local", info.DefaultDatabaseRegion) | ||
| assert.Equal(t, "16", info.DefaultDatabasePostgresVersion) | ||
| assert.Equal(t, "postgres://volcano:volcano@localhost:8002/app?sslmode=disable&application_name=volcano_full_access:app", info.DatabaseURL) | ||
| assert.Equal(t, "postgres://volcano_client_22222222-2222-2222-2222-222222222222:vpg_local_secret@localhost:8002/app?sslmode=disable&application_name=volcano_full_access", info.DatabaseURL) |
There was a problem hiding this comment.
ℹ️ Info only — not blocking, no action needed in this PR.
No CLI bug here: info.go passes database_url straight through to psql (PSQLCommandHint), and this test only exercises JSON unmarshaling, so the fixture format never affects CLI behavior.
Flagging for cross-repo awareness: this new format isn't what the server currently emits. In volcano-hosting, local info builds the URL via localmode.DatabaseConnectionString, which still returns the old shape:
postgres://volcano:volcano@localhost:8002/<db>?sslmode=disable&application_name=volcano_full_access:<db>
The new format drops the :<db> suffix from application_name and adds a scoped role (volcano_client_<uuid> + real password). Note the server's pgproxy currently identifies full_access/admin connections by parsing that suffix — strings.CutPrefix(appName, "volcano_full_access:") (pooling_proxy.go:847) — so worth confirming the paired server change (a) emits exactly this URL from local info and (b) moves routing/auth onto the scoped role, otherwise admin connections lose their target-DB routing.
tkkhq
left a comment
tkkhq
left a comment
There was a problem hiding this comment.
Approving. Verified against volcano-hosting: the duration-string fix is correct and complete (the three vars are the only time.Duration-typed env vars the template sets; the *_TIMEOUT vars are int32 seconds and correct as bare integers). Tests pass locally (43 in internal/localmode). Two non-blocking notes left inline — a latent regex-brittleness nit on the env-value helper, and an FYI on the info_test fixture format vs. the current server output / pgproxy routing to confirm cross-repo. Neither blocks merge.
No description provided.