Providing a metadata secret conflicts with the use of pgbouncer

freget commented 3 years ago

Official Helm Chart version

1.3.0 (latest released)

Apache Airflow version

2.2.1

Kubernetes Version

1.20.9

Helm Chart configuration

data:
  # If secret names are provided, use those secrets
  metadataSecretName: airflow-db-secret
  resultBackendSecretName: airflow-db-secret

pgbouncer:
  # Enable PgBouncer
  enabled: true

Docker Image customisations

No response

What happened

If the airflow database connections are configured via a predefined secret (instead of setting data.metadataConnection.*) and using pbouncer at the same time (pgbouncer.enabled: true), a few problems arise:

In the scheduler and worker configuration, the AIRFLOW_CONN_AIRFLOW_DB and AIRFLOW__CELERY__RESULT_BACKEND are set directly to the connection string within the secret. This means, pgbouncer is bypassed and the connection to the database is made directy. We might correct this if we change the secret value to point to pgbouncer (i.e. postgresql://user:password@airflow-pgbouncer.namespace.svc.cluster.local:6543/airflow-result-backend). This requires hardcoding pgbouncer connection information into the secret, though.
Within pgbouncer, /etc/pgbouncer/users.txt is empty (as we do not have a username and password available)
metrics-exporter of pgbouncer has the DATABASE_URL set to postgresql://%3Cno+value%3E:%3Cno+value%3E@127.0.0.1:6543/pgbouncer?sslmode=disable.

In summary, it is not possible to use pgbouncer with a submitted secret name.

What you expected to happen

No response

How to reproduce

No response

Anything else

I already solved this by a major refactoring of the whole secret mechanism. Instead of providing the connection string directly, we could provide a username and a password. Those might be mounted as environment variables and then used to replace the faulty secrets.

To make this bulletproof for all situations (including keeping the connection string approach if no pgbouncer is enabled) is a bit more complicated, though.

Are you willing to submit PR?

[ ] Yes I am willing to submit a PR!

Code of Conduct

[X] I agree to follow this project's Code of Conduct

abin-tiger commented 2 years ago

+1

sudeepgupta90 commented 1 year ago

bumping on this

potiuk commented 1 year ago

Good that you bumped it @sudeepgupta90 .

If you are going to approach to fix it - feel gree to work oin it @sudeepgupta90 (I can assign you if you can confirm it).

If not, then can you please confirm that you tried and it's the same in the latest release and can provide some validation on that one?

I woudl love to mark it as a good first issue so that others can take a look at it but I would love to know if it still exists in the latest version, Can you please verify and confirm it @sudeepgupta90 ?

sudeepgupta90 commented 1 year ago

@potiuk the issue still exists in the latest version of the helmchart. I am happy to work on this if you can assign it to me

potiuk commented 1 year ago

Please do. Assigned you :)

sudeepgupta90 commented 1 year ago

@potiuk I have been thinking about this , and my own thoughts initially was to fix the issue which OP reported...

Why we should not fix this in Airflow Helm Chart

PgBouncer is a connection pooler specifically for Postgres DB. Airflow can run on any Relational DB which is compatible with SqlAlchemy which includes Azure SQL, MySQL as two examples. These are not really compatible here, it should not be on the chart to implement a connection pooler for each of the supported databases.
The Airflow Chart implements PgBouncer and Postgres specifically as an example of good practice, I think

What we can fix/do instead

We should enhance the documentation of the production guide, and mention explicitly about this behaviour. Like the OP, I myself have deployed PgBouncer externally, and passing the connection to the pgbouncer as the metadata secret to fix/workaround this problem. This behaviour doesn't break any functionality
Additionally, one can argue that we can drop pgbouncer section and rename it to databaseConnectionPooler and support PgBouncer as one of the solutions there. But again although this does explicitly make things very clear, but adds little value because the maintainers should not be supporting various connection poolers - and it should be more community driven. Plus the integration tests would be a headache I guess?

I am personally more in favour of enhancing the documentation. WDYT?

brki commented 1 year ago

I just ran into this problem too.

I was happy to have pgBouncer in the chart, but a bit frustrated when trying to set it up. I can understand the point that, perhaps, pgBouncer doesn't belong in this chart. If it works, it's kind of nice though - batteries included.

So, whether or not pgBouncer stays part of the Airflow chart, it would be a good idea to improve the documentation about using it.

potiuk commented 1 year ago

I just ran into this problem too.

I was happy to have pgBouncer in the chart, but a bit frustrated when trying to set it up. I can understand the point that, perhaps, pgBouncer doesn't belong in this chart. If it works, it's kind of nice though - batteries included.

So, whether or not pgBouncer stays part of the Airflow chart, it would be a good idea to improve the documentation about using it.

As mentioned above - anyone in this thread is free to do so. Airflow's code and docs are created by > 2300 contributors. Most of them, people who had problem with code and docs and improved it to help others.

And answer to @sudeepgupta90 (sorry I missed the last comment) - feel free to propose what you think is best (and @brki you can also join it or propose some improvements). I am not one to judge upfront which way is best before seeing it (and I am not even the author of it - probably others will be able to comment). PR is always best to discuss a proposal :)

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has been open for 30 days with no response from the author. It will be closed in next 7 days if no further activity occurs from the issue author.

github-actions[bot] commented 1 year ago

This issue has been closed because it has not received response from the issue author.

apache / airflow