Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bitnami/pgbouncer] support $PGBOUNCER_DSN_${i}_FILE env vars #51830

Merged
merged 2 commits into from
Nov 6, 2023
Merged

[bitnami/pgbouncer] support $PGBOUNCER_DSN_${i}_FILE env vars #51830

merged 2 commits into from
Nov 6, 2023

Conversation

derhuerst
Copy link
Contributor

@derhuerst derhuerst commented Oct 12, 2023

Description of the change

This PR is a follow-up of #14323, which has added $PGBOUNCER_DSN_${i} to support multiple upstream connections.

I was surprised to find (undocumented!) support for them, but expected the variables to work like (almost) all other pgbouncer-related env vars: I should be able to use $PGBOUNCER_DSN_${i}_FILE to reference a file containing the value of $PGBOUNCER_DSN_${i}! This PR adds support for such _FILE env vars.

Benefits

Configuring pgbouncer programmatically is really tricky apparently. In the past several days, I have experimented with multiple ways to define n > 1 upstream connections dynamically in a Docker-Compose-based setup. Within one Compose service, I want to re-configure the (upstream) PostgreSQL database that other services are supposed to access, without changing (i.e. restarting) the latter ones.

With the option to specify that a specific connection's config should be read from a file, I can mount that file into the pgbouncer container, and execute /opt/bitnami/scripts/pgbouncer/setup.sh (it deletes files, e.g. the pidfile, but that is another topic, I might submit a follow-up PR for this) and psql -p 6432 -U pgbouncer -c RELOAD, in order to apply the newly configured connection.

Possible drawbacks

The design is quite complex, and bitnami/pgbouncer's env var handling becomes another bit more complex, and its API surface another bit larger.

The cleanest solution might be to write either

  • an entirely new pgbouncer Docker image that is specifically optimised for this use case – This is a lot of work, and bitnami/pgbouncer already avoids so many gotchas that I would run into again, so I figured this is not a good approach. –, or
  • an even more dynamic setup, e.g. an etcd pgbouncer adapter mapping etcd entries to pgbouncer configs, written in a "normal" full-blown programming language instead of Bash.

However, given that these alternative approaches require much more work and that they might cause significant maintenance burden (because they're standalone projects, whereas this project is likely to be maintained by anyone using pgbouncer within containers), I have refrained from pursuing them, and submit this PR in the hope that you consider it a useful contribution. 😄

Applicable issues

Additional information

I have also checked out AWS Labs' pgbouncer fork, but neither its query routing feature seems capable of this (as a query routing function can only route to a upstream "target" that has been defined statically in pgbouncer.ini) nor its "fast switchover" feature (because it seems to be closely tied to how PG clusters work).

@github-actions github-actions bot added the triage Triage is needed label Oct 12, 2023
@bitnami-bot bitnami-bot requested a review from carrodher October 12, 2023 21:33
@carrodher carrodher added the verify Execute verification workflow for these changes label Oct 15, 2023
@github-actions github-actions bot added in-progress and removed triage Triage is needed labels Oct 15, 2023
@bitnami-bot bitnami-bot removed the request for review from carrodher October 15, 2023 12:26
@bitnami-bot bitnami-bot requested a review from fevisera October 15, 2023 12:26
@fevisera
Copy link
Contributor

Hi @derhuerst,

Thank you so much for your contribution. I would like to inform you that we are currently addressing a related issue internally #20874 and we are considering your pull request implementation.

That being said, I'm placing this PR on hold and we will reach out to you as soon as we have updates to share.

Appreciate your contribution!

@github-actions github-actions bot added on-hold Issues or Pull Requests with this label will never be considered stale and removed in-progress labels Oct 19, 2023
@derhuerst
Copy link
Contributor Author

FYI: I'm currently building a much more generalised approach with a completely separate code base: Generate pgbouncer's config from etcd, and reload it whenever an etcd key/value has changed. Will update this post with source code soon.

@github-actions github-actions bot added triage Triage is needed and removed on-hold Issues or Pull Requests with this label will never be considered stale labels Oct 22, 2023
@github-actions github-actions bot added on-hold Issues or Pull Requests with this label will never be considered stale and removed triage Triage is needed labels Oct 23, 2023
@gongomgra gongomgra self-requested a review October 23, 2023 15:58
@gongomgra
Copy link
Contributor

Hi @derhuerst,

Thanks for your contribution and for using our products. it is possible to configure multiple backend database connections in pgbouncer by using the combination of some of the environment variables already availables: one or more PGBOUNCER_DSN_${i} (where i starts in zero, 0), and the PGBOUNDER_USERLIST_FILE to provide the connection credentials from a volume for security reasons. The updated docker-compose.yaml file looks like the one below:

  pg1:
    image: docker.io/bitnami/postgresql:14
    volumes:
      - 'pg1_data:/bitnami/postgresql'
    environment:
      - POSTGRESQL_PASSWORD=bitnami123
      - POSTGRESQL_DATABASE=db1

  pg2:
    image: docker.io/bitnami/postgresql:15
    volumes:
      - 'pg2_data:/bitnami/postgresql'
    environment:
      - POSTGRESQL_PASSWORD=bitnami456
      - POSTGRESQL_DATABASE=db2

  pg3:
    image: docker.io/bitnami/postgresql:14
    volumes:
      - 'pg3_data:/bitnami/postgresql'
    environment:
      - POSTGRESQL_PASSWORD=bitnami789
      - POSTGRESQL_DATABASE=db3

  pgbouncer:
    image: docker.io/bitnami/pgbouncer:1
    ports:
      - 6432:6432
    volumes:
      - './userlists.txt:/bitnami/userlists.txt'
    environment:
      - POSTGRESQL_HOST=pg1
      - ALLOW_EMPTY_PASSWORD=yes
      - PGBOUNCER_AUTH_TYPE=trust
      - PGBOUNCER_USERLIST_FILE=/bitnami/userlists.txt
      - PGBOUNCER_DSN_0=pg2=host=pg2 port=5432 dbname=db2
      - PGBOUNCER_DSN_1=pg3=host=pg3 port=5432 dbname=db3
volumes:
  pg1_data:
    driver: local
  pg2_data:
    driver: local
  pg3_data:
    driver: local

Notice you need to provide at least one database connection using the default POSTGRESQL_HOST variable, as the pgbouncer initialization logic checks connectivity with that database (initialization process will fail otherwise).

$ docker exec -it -u root bfde5464661d cat /opt/bitnami/pgbouncer/conf/pgbouncer.ini
[databases]
postgres=host=pg1 port=5432 dbname=postgres
pg2=host=pg2 port=5432 dbname=db2
pg3=host=pg3 port=5432 dbname=db3

[pgbouncer]
listen_port=6432
listen_addr=0.0.0.0
unix_socket_dir=/tmp/
unix_socket_mode=0777
auth_file=/opt/bitnami/pgbouncer/conf/userlist.txt
auth_type=trust
pidfile=/opt/bitnami/pgbouncer/tmp/pgbouncer.pid
logfile=/opt/bitnami/pgbouncer/logs/pgbouncer.log
admin_users=postgres
client_tls_sslmode=disable
server_tls_sslmode=disable
stats_period=60

In the same way, you can check that the content of your local userlists.txt file gets into the pgbouncer auth file /opt/bitnami/pgbouncer/conf/userlist.txt. Using the config files above, I was able to connect with the pg2 server by running the command below (it is the only one running PostgreSQL 15.x in my docker-compose file)


$ docker exec -it -u root bfde5464661d psql -p 6432 -U postgres pg2 -c "show server_version;"
 server_version
----------------
 15.4
(1 row)

Could you give us more details about your specific use case? According to our investigation, you don't need to provide any sensible data in the pgbouncer.ini file, as the database passwords can be provided securely from a volume via the PGBOUNCER_USERLIST_FILE variable.

@carrodher carrodher requested a review from fevisera October 30, 2023 08:13
Copy link
Contributor

@gongomgra gongomgra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Just a minor comment here

@gongomgra
Copy link
Contributor

gongomgra commented Oct 31, 2023

I want to be able to reconfigure n database connections dynamically, i.e. during runtime and without restarting the pgbouncer process and/or Compose service.

AFAICT this is currently not possible, so I added support for $PGBOUNCER_DSN_${i}_FILE, consistent with how the other env vars work.

As per my investigation, restarting the service is not needed to update PgBouncer's config. You can simply mount both the pgbouncer.ini and userslist.txt files as volumes in the PgBouncer container so you can edit the configuration externally, and then run the PgBouncer's RELOAD command from the container (plus the SUSPEND and RESUME commands in case you want to reset the current connections). Apart from that, we think including the PGBOUNCER_DSN_${i}_FILE is a good idea, so let's go ahead with the review process.

@derhuerst
Copy link
Contributor Author

I want to be able to reconfigure n database connections dynamically, i.e. during runtime and without restarting the pgbouncer process and/or Compose service.
AFAICT this is currently not possible […].

As per my investigation, restarting the service is not needed to update PgBouncer's config. You can simply mount both the pgbouncer.ini and userslist.txt files as volumes in the PgBouncer container so you can edit the configuration externally […].

Yes, true, it is technically possible, but not a feasible option for me, because it would require having this config generation logic in a another service/component; This would defeat the purpose of using bitnami/pgbouncer somewhat.

Copy link
Contributor

@gongomgra gongomgra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gongomgra gongomgra added the verify Execute verification workflow for these changes label Nov 3, 2023
@gongomgra
Copy link
Contributor

Hi @derhuerst,

Can you fix the DCO issues so we can merge your changes?

@gongomgra gongomgra merged commit c734fac into bitnami:main Nov 6, 2023
9 checks passed
@gongomgra
Copy link
Contributor

Thanks for your contribution!

@gongomgra
Copy link
Contributor

@derhuerst we have just released bitnami/pgbouncer:1.21.0-debian-11-r5 including your changes.

@derhuerst derhuerst deleted the pgbouncer-dsn-i-file branch November 6, 2023 10:45
@derhuerst
Copy link
Contributor Author

I can confirm that it works as intended. Thanks.

@github-actions github-actions bot added triage Triage is needed and removed solved labels Nov 8, 2023
@github-actions github-actions bot added the solved label Nov 8, 2023
@gongomgra gongomgra removed the triage Triage is needed label Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pgbouncer solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants