Skip to content

feat: enhance construct_database_url_from_env_vars with port, SSL, and RFC 3986 encoding [sc-541344]#97

Draft
mateo-di wants to merge 1 commit into
carto/mainfrom
sc-541344/enhance-db-url-construction
Draft

feat: enhance construct_database_url_from_env_vars with port, SSL, and RFC 3986 encoding [sc-541344]#97
mateo-di wants to merge 1 commit into
carto/mainfrom
sc-541344/enhance-db-url-construction

Conversation

@mateo-di

Copy link
Copy Markdown
Collaborator

Summary

Enhance the construct_database_url_from_env_vars() helper to support DATABASE_PORT, DATABASE_SSL_MODE, and DATABASE_SSL_ROOT_CERT as optional env vars. Switch from quote_plus to quote(safe='') for correct RFC 3986 percent-encoding per PostgreSQL URI spec. Fix DATABASE_SCHEMA not being URL-encoded.

Story: sc-541344

Related: CartoDB/cloud-native#23679 (short-term entrypoint.sh fix)

What Changed

Added:

  • DATABASE_PORT support — separate port env var (ignored if host already contains :port)
  • DATABASE_SSL_MODE support — appended as ?sslmode=... query param
  • DATABASE_SSL_ROOT_CERT support — appended as &sslrootcert=... query param
  • 5 new test cases: port handling, SSL params, schema encoding, combined query params, heavy special chars password
  • Test case covering the exact customer scenario (pgHgsh@gh4#Mj$! password)

Modified:

  • Encoding changed from urllib.parse.quote_plus() to urllib.parse.quote(safe='') — correct RFC 3986 percent-encoding per PostgreSQL connection URI spec
  • DATABASE_SCHEMA is now URL-encoded (was previously inserted raw — existing bug)
  • Query params built as a list and joined, instead of string concatenation
  • Docstring expanded with all supported env vars

Removed:

  • Nothing

Architectural Context

EAD: N/A - small change

Architectural Decisions Made:

  • Keep upstream env var naming (DATABASE_*) — CARTO's LITELLM_DATABASE_* prefix is a deployment concern handled by entrypoint.sh
  • Use quote(safe='') instead of quote_plus() — PostgreSQL URIs follow RFC 3986 (percent-encoding), not HTML form encoding
  • sslrootcert path is NOT URL-encoded — it's a filesystem path consumed by libpq, not a URI component

Review Focus Areas

Critical areas (require thorough review):

  1. litellm/proxy/utils.py — the enhanced function logic, especially port detection and query param building
  2. tests/test_litellm/proxy/test_proxy_cli.py — new test coverage for all scenarios

Safe to skip: Docstring changes

Deployment Impact

  • SaaS only
  • Selfhosted only
  • Both SaaS and Selfhosted
  • Not applicable

Note: All new env vars are optional. Existing deployments that don't set DATABASE_PORT, DATABASE_SSL_MODE, or DATABASE_SSL_ROOT_CERT produce exactly the same URL as before. The encoding change (quote_plusquote) only differs for spaces (which are extremely rare in DB credentials).

Migration & Breaking Changes

  • No migrations or breaking changes

Security Considerations

  • No security impact

Performance Impact

  • No performance impact

Tests

  • Unit tests added/updated (5 new test cases, all 22 tests in test_proxy_cli.py pass)
  • No tests needed

Dependencies

How to Validate

poetry run python -m pytest tests/test_litellm/proxy/test_proxy_cli.py -k "test_construct_database_url" -vv

All 6 URL construction tests should pass (1 existing + 5 new).

AI-Generated Code Notice

  • This PR contains AI-generated code
  • Areas requiring extra verification: encoding function choice (quote vs quote_plus), port detection logic

Checklist

  • PR title follows convention
  • Shortcut story linked
  • One issue per PR
  • Appropriate labels applied
  • Reviewers assigned (or auto-assigned)
  • AI review findings addressed (if applicable)

…d RFC 3986 encoding

Add DATABASE_PORT, DATABASE_SSL_MODE, and DATABASE_SSL_ROOT_CERT
support to the URL construction helper. Switch from quote_plus to
quote(safe='') for correct RFC 3986 percent-encoding per PostgreSQL
URI spec. Fix DATABASE_SCHEMA not being URL-encoded (existing bug).

Build query params as a list for cleaner multi-param handling.
All new env vars are optional — fully backward compatible.

Co-authored-by: Claude <noreply@anthropic.com>
@mateo-di mateo-di self-assigned this Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant