PostgREST / postgrest

REST API for any Postgres database
https://postgrest.org
MIT License
22.65k stars 1k forks source link

(refactor) Too easy to miss enabling a config as in-db and show it on the CLI #3101

Open steve-chavez opened 6 months ago

steve-chavez commented 6 months ago

Problem

This occurred on:

This happens because the logic for the CLI and the in-db config is disconnected from the AppConfig type

https://github.com/PostgREST/postgrest/blob/0c65bf494d75fbb2b8650eba1385b8d2c5d042d2/src/PostgREST/CLI.hs#L124-L233

https://github.com/PostgREST/postgrest/blob/0c65bf494d75fbb2b8650eba1385b8d2c5d042d2/src/PostgREST/Config/Database.hs#L44-L69

https://github.com/PostgREST/postgrest/blob/0c65bf494d75fbb2b8650eba1385b8d2c5d042d2/src/PostgREST/Config.hs#L70-L115

Solution

Tie the AppConfig or other intermediate type to the CLI and the in-db logic.

develop7 commented 6 months ago

The https://hackage.haskell.org/package/configuration-tools (source at https://github.com/alephcloud/hs-configuration-tools) could be the right tool for the job — it combines CLI options parser, *JSON instances, JSON/YAML config parsing (that's where it falls short for us, though — need to investigate how hard adding custom config formats' support could be), remote configs even, if needed.

The problem here is not every parameter is exposed to command line, configs and/or database, that's where it doesn't quite meet the needs as well.

wolfgangwalther commented 5 months ago

I was independently looking for alternatives to configurator-pg today and stumbled across configuration-tools as well. Looks promising. I would not mind a breaking change to switch from the current config format to a yaml based format. Maybe we could even provide a small migration script or something to load the old config and dump the new one.

One thing I like about configuration-tools is, that it looks like it combines a few very useful things. Some that we already do (config file, environment variables), but also new stuff on top (cli arguments, --help output with a information about the current build, dependencies, licences...). The one thing I am not sure about is how to integrate the in-database-configuration nicely... which is exactly part of this issue :)

steve-chavez commented 5 months ago

If we're going to change the file format, I think what would make most sense is using an INI format to be compatible with the pg service file (also see this blog post).

That way, we could support multiple databases (https://github.com/PostgREST/postgrest/issues/2798) with a config like:

# pg_service.ini
[serviceone]
port=5432
user=stevechavez
dbname=stevechavez

[serviceone.postgrest]
jwt-secret = xxxx
db-pool = 10

[servicetwo]
port=5432
...

[servicetwo.postgrest]
jwt-secret = yyy
db-pool = 20
other-configs = ".."

This also has the advantage of being able to use psql like:

PGSERVICE=serviceone PGSERVICEFILE=pg_service.ini psql

psql (14.10 (Ubuntu 14.10-0ubuntu0.22.04.1))
Type "help" for help.

stevechavez=# 
wolfgangwalther commented 5 months ago

If we're going to change the file format, I think what would make most sense is using an INI format to be compatible with the pg service file (also see this blog post).

[...]

This also has the advantage of being able to use psql like:

Since this is part of libpq, this should already work with postgrest right now. Just export PGSERVICE before you start PostgREST.. should do it already. That also means it might be hard to use this file in a different way - i.e. use multiple services from that file at the same time, because that is not what libpq wants to do.

Supporting multiple databases would also easily be possible with yaml. The problem for this ini-based file format is, that it will again be impossible to represent anything more complex. YAML can do much more: arrays, objects, anchors, references, ... so much more.

steve-chavez commented 5 months ago

That also means it might be hard to use this file in a different way - i.e. use multiple services from that file at the same time, because that is not what libpq wants to do.

Right, so my intention was to process the ini file in PostgREST itself to use multiple dbs. And using the ini file we would still have the advantage of leveraging psql to test the different connections (PGSERVICE=anyservice PGSERVICEFILE=our_conf.ini psql). With YAML we cannot do that.

The problem for this ini-based file format is, that it will again be impossible to represent anything more complex. YAML can do much more: arrays, objects, anchors, references, ... so much more.

We don't have complex values now and we always need to consider that values have to be compatible with the in-db config. pgbouncer seems to do fine with an ini config too. So that doesn't seem like a blocker.