Add support for integrated MariaDB and PostgreSQL #223
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThis update introduces the functionality to deploy MariaDB and PostgreSQL instances directly via configuration options in Authelia's Helm chart. This enhancement simplifies the process of setting up databases for Authelia, making it more convenient for users to configure their instances according to their preferences and requirements. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
charts/authelia/Chart.yamlis excluded by:!**/*.yamlcharts/authelia/values.yamlis excluded by:!**/*.yaml
Files selected for processing (1)
- charts/authelia/README.md (1 hunks)
Additional comments: 2
charts/authelia/README.md (2)
- 144-144: The addition of the
storage.mysql.deployoption to deploy a MariaDB instance is clear and aligns with the PR objectives. However, it might be beneficial to expand this section to briefly explain the scenarios in which a user might want to enable this option, emphasizing its optional nature and potential use cases.- 146-146: The introduction of the
storage.postgres.deployoption for deploying a PostgreSQL instance is well-documented. To further assist users, consider providing a brief explanation of when enabling this option might be advantageous, highlighting its optional nature and potential benefits for certain deployment scenarios.
There was a problem hiding this comment.
Please rebase on #215 and make a merge request to that branch instead.
Signed-off-by: Jonathan <me@jonathangazeley.com>
Signed-off-by: Jonathan <me@jonathangazeley.com>
| @@ -1305,6 +1305,7 @@ configMap: | |||
| ## | |||
| mysql: | |||
| enabled: false | |||
| deploy: false | |||
There was a problem hiding this comment.
What does this option do? It seems ineffectual.
There was a problem hiding this comment.
The deploy option toggles whether the bitnami/mariadb subchart is brought into play, and this condition is set in Chart.yaml. If the mariadb chart is activated, it will then consume the mariadb section of values.yaml.
Typically this condition would operate on mysql.enabled but I wanted to avoid breaking existing users of the chart, where people might want to enable mysql support but keep using their existing instance of mysql rather than have the chart deploy a new instance. So I created a new boolean for this.
Usually the mysql and mariadb sections would be merged into one section, taking the name of the subchart (so mariadb) but this would also break existing users so I chose not to refactor this at the moment. It's something we could talk about for a breaking release, though?
There was a problem hiding this comment.
Ah nice, didn't know abotu that interaction and didn't notice it! LGTM!
There was a problem hiding this comment.
Also as far as that change, I'm not entirely sure I care about a breaking change for 0.9.0, it's LONG been advertised it will be and v0 rules apply. See #140 and the target PR for more context. I think we can merge as is and decide later what we want to do. I like giving users the option to deploy it or use existing.
As far as MySQL/MariaDB users they're masochists who love things to break anyway. ;)
| @@ -1350,6 +1351,7 @@ configMap: | |||
| ## | |||
| postgres: | |||
| enabled: false | |||
| deploy: false | |||
|
Great, thanks. As Redis is also a dependency I plan to follow this up with another near-identical PR to pull in the Redis helm chart, which means users will then be able to deploy a full working Authelia stack from one helm chart. I'll get on that a bit later today. |
|
Nice! Thanks a lot, I've been stretched thin. |
This reverts commit 7bbbb37.
|
Looks like this included a bug making the chart unable to be packaged. I've reverted it for now in bbbd05d. There's also this comment which I've failed to decode: #220 (comment) We'd welcome these changes in another PR. You should be able to just |
|
OK, I'll have a look at this and see if I can track down what went wrong. I saw #220 (comment) too and also didn't understand it. I guess it might be to do with what I mentioned a little while ago, about having to set some of the values in two places to make the new integrations work. If you're not concerned about breaking changes, I'll try and integrate those more smoothly. |
We're in v0 (every minor bump should be expected as breaking), and once we make v1 I'll be fairly happy to bump major versions whenever necessary. The maintenance cycle of the chart will likely never perfectly marry-up to Authelia itself and we have much more freedom. Only real concern is deploying these dependencies by default should not be the case, it should be explicit. Otherwise as long as the breaking change is documented and makes sense (I've done my best to document the other ones, if you notice any missed ones I'd appreciate a heads up or for those to be fixed too). |
|
@james-d-elliott I've just got the bandwidth to look at this again.
You got a link to the actual problem? I couldn't see any failed runs in Github Actions - thanks. |
|
OK, I've figured it out. When deps are packaged with a Helm chart, there is a So the usual way of handling this is for your CI/CD pipeline to fetch these deps when the chart is packaged. You will need to execute Then the subcharts will be included in the package you build and publish, but not your git repo. |
|
Same issue running that command before the cr pakage. See here: |
|
OK. Can you try explicitly running |
|
That command doesn't exist, looks like it should be |
helm repo add bitnami https://charts.bitnami.com/bitnami
helm dependency build charts/authelia
helm package charts/authelia |
|
Interesting, I'm also on Helm 3.14.3 and it works for me: It is supposed to update all its chart repos when you run |
|
Yep, it finds all the charts, and it downloads them fine. What does the packaged chart look like when you extract it? |
|
So weird, I'm running into the same error too: So we can deduce that the problem is not with dependency resolving, but with packaging behaviour. I've had a look in Helm's issue tracker and I can't see anything relevant there. This is definitely broken behaviour so I'm going to report it as a bug in Helm. |
|
Reported helm/helm#12992 |
|
it's the *.tgz line in ./charts/authelia/.helmignore |
Good catch! Are you able to re-merge this PR now, or does it need rebasing etc? Thanks |
|
@james-d-elliott what do you need me to do to progress this? Which branch is the right one to develop against now, |
|
|
This PR adds basic support for deploying MariaDB or PostgreSQL as part of an Authelia deployment.
Typically the dependency subcharts would be integrated under the top level
mariadb:orpostgresql:keys, where the config can be inherited by the subcharts, and the main app (i.e. Authelia) would also draw its config from those keys. However the database config in the chart is understorage.mysqlandstorage.postgresand changing this would be a breaking change.So I have added two booleans
storage.mysql.deployandstorage.postgres.deploywhich enable the user to deploy the Bitnami subcharts. This does not affect any existing Authelia deployments. At the moment the user has to copy a few settings (e.g. credentials) from themariadbsection to thestorage.mysqlsection.In future we could consider a tighter integration, but it would be a breaking change, so let's go with this for now.
Happy to discuss or tweak this, including adding a Redis subchart.
Relevant to #172 and #89 - but won't actually close them until Redis support is added.
Summary by CodeRabbit