search

heroku / heroku-buildpack-pgbouncer

Run pgbouncer in a dyno along with your application
MIT License
338 stars 136 forks source link

Buildpack should validate `PGBOUNCER_URLS` explicitly #171

Closed edmorley closed 1 year ago

edmorley commented 2 years ago

Currently if PGBOUNCER_URLS is set on an app, and some of the env vars it refers to don't exist, then empty entries are added by gen-pgbouncer-conf.sh to /app/vendor/pgbouncer/pgbouncer.ini, like so:

[databases]
db1= host=REDACTED.compute-1.amazonaws.com dbname=REDACTED port=5432
db2= host= dbname= port=
db3= host=REDACTED.compute-1.amazonaws.com dbname=REDACTED port=5432

As of newer pgbouncer versions (such as v1.17.0, which #170 recently updated Heroku-18/20 to), this then results in a confusing error message when pgbouncer tries to read the config:

ERROR syntax error in connection string
ERROR invalid value "host= dbname= port=" for parameter db2 in configuration
FATAL cannot load config file

In this case the "db2" refers to the second database listed in PGBOUNCER_URLS.

So if PGBOUNCER_URLS was set to DATABASE_FOO_URL DATABASE_BAR_URL DATABASE_BAZ_URL, the above error means the DATABASE_BAR_URL env var is not set, or was set to the empty string.

The fix is for the PGBOUNCER_URLS env var to be updated on the app so it doesn't refer to non-existent DB env vars.

To improve the UX here, gen-pgbouncer-conf.sh should really check for empty values here, and fail early with a clear error message: https://github.com/heroku/heroku-buildpack-pgbouncer/blob/92598696b88b522dd792861fdf795372b9f3a537/bin/gen-pgbouncer-conf.sh#L47-L49