fix(prefect-server): better support for internal and external database configs #365
Conversation
Ensures that the PostgreSQL Secret is created, even if `postgresql.enabled=false`. This ensures that we support a use case where folks want to use an external instance of PostgreSQL, but still want the Secret to automatically be generated with the proper connection string. With a recent change, we would skip creation of this secret if PostgreSQL was disabled which forced users to create a Secret themselves. This change now allows them to continue providing the `auth` values and letting the chart build the Secret with the correct connection string. Closes #358
|
@ahgraber @tmyhu @MRocholl - I believe this should cover all the use cases you mentioned. Could you please give this a look when you have a moment and let me know if this needs any tweaking? Trying to make it flexible enough to cover all use cases while keeping it as consistent/straightforward as possible. Thanks! |
|
@mitchnielsen it looks good to me, but won't make any difference for our use case since we are using SQLite. I assume that's not something you really want to support specifically since there is no hint about it anywhere in the helm chart I could see? We should probably migrate to PostgreSQL anyway, but for now I'm happy to continue creating the secret and connection string manually for SQLite. |
|
@mitchnielsen Thanks for this effort! The output connection string from the test deployment's secret is missing the |
@tmyhu fair point. We have this in mind and are tracking in #345. I'd like to provide the |
|
@ahgraber great call-out, let me see if I can make a minimal change to make that configuration supported. If not, may need to jump ahead to providing this config outside of the |
The `postgresql` key in `values.yaml` is for overriding the PostgreSQL chart's default values. It's not a good place to define custom keys because these are conflated with the actual configuration from the subchart. This changes removes those custom configs. It also removes any configured values that already matched the default from the PostgreSQL subchart, and were therefore not doing anything.
When the PostgreSQL subchart is disabled, we need a way to provide the connection string information for the external instance. This adds the required values and template helpers to calculate the connection string in this scenario.
- Supports setting the name to use for the Secret - Supports controlling whether or not to create the Secret - Supports someone providing their own existing Secret
Uses https://github.com/helm-unittest/helm-unittest to unit test the database configuration. This is a replacement for coming up with test cases locally, running `helm template`, and manually validating the output.
- Adds script to run helm-unittest locally - Adds CI workflow to run helm-unittest in GitHub Actions
Updates the README to reflect the new configuration for external databases.
mitchnielsen
force-pushed
the
db-secret-config
branch
from
93ed554 to
0f43bfc
Compare
mitchnielsen
added
breaking
feature
mitchnielsen
changed the title
|
@ahgraber @tmyhu @MRocholl - alrighty, bit of a rework here to make sure we have proper support for either internal or external database configuration. For some examples of the configuration you'd provide, check out the updated docs or the unit test file. Welcoming any feedback here 🤝 |
|
@mitchnielsen sorry for the late reply. These changes should satisfy the requirements I believe 👍 Thx for the great work |
@MRocholl - no worries at all, thank you for confirming 🤝 |
| @@ -340,8 +342,6 @@ postgresql: | |||
| persistence: | |||
| # -- enable PostgreSQL Primary data persistence using PVC | |||
| enabled: false | |||
| # -- PVC Storage Request for PostgreSQL volume | |||
| size: 8Gi | |||
There was a problem hiding this comment.
does this default need to be re-implemented somewhere?
There was a problem hiding this comment.
Shouldn't need to, since it's the default in Bitnami's values.yaml. We could technically leave this here, but I deleted it to make it more clear that everything defined under postgresql is an override for the chart's defaults.
| @@ -293,7 +293,26 @@ ingress: | |||
| ## port: | |||
| ## name: http | |||
|
|
|||
| # Postgresql configuration | |||
| # Secret configuration | |||
| secret: | |||
There was a problem hiding this comment.
just so i understand, .Values.secret is specifically for BYO postgres credentials?
There was a problem hiding this comment.
Exactly, it gives us somewhere outside of the context of postgresql. to define database configs (so we can dedicate postgresql. settings for the PostgreSQL chart without overloading it with our own chart's configs).
Installs the helm/chart-testing binary at its latest version. Needed for the tests defined under `charts/<chart>/tests/test-*.yaml`.
Summary
This will be a breaking change, so we should release this in a new major version. The configuration keys have moved around. If we really need to, we can try to build in some backwards-compatibility, but that always ends up looking pretty rough in Helm.
Ensures that the PostgreSQL Secret is created, even if
postgresql.enabled=false. This ensures that we support a use case where folks want to use an external instance of PostgreSQL, but still want the Secret to automatically be generated with the proper connection string.With a recent change, we would skip creation of this secret if PostgreSQL was disabled which forced users to create a Secret themselves. This change now allows them to continue providing the
authvalues and letting the chart build the Secret with the correct connection string.I ended up needing to go a step further by removing any custom configuration that was injected under the
postgresqlkey, which should be dedicated to overriding default values from the PostgreSQL chart. There's now a newsecretkey, which aligns with the other top-level keys (deployment,service, etc.). These values will be considered ifpostgresql.enabled=false.Closes #358
Follow-up to #341.
Testing
See the added test suite.