PrefectHQ / prefect-helm

Helm charts for deploying Prefect Services
Apache License 2.0
93 stars 56 forks source link

Changes to chart with `postgresql.enabled = false` and external postgres break deployment #358

Closed ahgraber closed 3 weeks ago

ahgraber commented 1 month ago

Changes from #341 are breaking my deployment.

Context: I use movetokube/postgres-operator: Postgres operator for Kubernetes to dynamically provision postgres as part of my gitops deployment process. This operator creates unique auth info in a secret that does not match the expected format of the prefect helm chart.

Prior to that change, I set:

  valuesFrom:
    - kind: Secret
      name: &db_secret database-prefect
      valuesKey: DATABASE_NAME
      targetPath: postgresql.auth.database
    - kind: Secret
      name: *db_secret
      valuesKey: LOGIN
      targetPath: postgresql.auth.username
    - kind: Secret
      name: *db_secret
      valuesKey: PASSWORD
      targetPath: postgresql.auth.password
    - kind: Secret
      name: *db_secret
      valuesKey: HOST
      targetPath: postgresql.externalHostname
values:
  ...
  postgresql:
      enabled: true
      useSubChart: false

And the helpers picked up the values I was injecting in from the operator's secret at the valid targetPaths.

After #341, this approach does not work because the helm chart expects that there will be a secret named prefect-server-postgresql-connection and does not create the secret as it used to based on the injected values.

I'd really appreciate it if the chart supported the use of external postgres while providing details in postgres.auth and postgres.externalHostname or supported passing the full postgresql+asyncpg://{username}:{password}@{hostname}/{database} connection url as an option.

The requirement to use the prefect-server-postgresql-connection secret that does not work with my gitops deployments! Thank you!

mitchnielsen commented 1 month ago

Thanks for reaching out @ahgraber ๐Ÿ‘‹๐Ÿผ We'll take a look at this and see which way works best to make this more flexible.

tmyhu commented 1 month ago

On a similar note, our deployment also broke due from #341 to the environment variable PREFECT_API_DATABASE_CONNECTION_URL now being always set in the deployment whether postgresql is enabled or not. Previously we disabled postgres and set it manually to a sqlite connection string. As a workaround we now had to create a secret with the sqlite connection string and reference it in values.postgresql.auth.existingSecret. It would be great if that had been a bit more seamless.

mitchnielsen commented 1 month ago

Thanks for the feedback. The thinking was that variable needs to be set whether or not the PostgreSQL chart is in use because Prefect needs to know how to reach the database no matter what. We'll keep refactoring this to be more flexible as part of this issue and https://github.com/PrefectHQ/prefect-helm/issues/345. Welcoming any specific suggestions and/or feedback on PRs as they come ๐Ÿ‘๐Ÿผ

MRocholl commented 1 month ago

Just ran into the same issue with cloudnativePG.

In my opinion, the best way to handle this, is to build the URL at runtime and expect username, password, database, host and port while leaving the option to provide a fixed URL (if one must). This exposes a clear API while being robust to internal changes (eg change to a different driver) and is consistent with the API that psql has. Ill give this a try on a fork and file a PR if i get anywhere.

mitchnielsen commented 3 weeks ago

This change is released as part of https://github.com/PrefectHQ/prefect-helm/releases/tag/2024.8.13184720. Please reach out with any further feedback!

ahgraber commented 2 weeks ago

Works great for me, TYVM!!