central db credential mgmt w/secret, internal pgsql

sa-ChristianAnton commented 2 years ago

This commit implements a central way of managing db access credentials using a secret, which then will be respected by all the components installed by this chart: zabbixserver, zabbixweb and postgresql.

The secret must contain a number of keys indicating DB host, DB name, user and password and can direct towards a database installed within this chart, or an external database.

For the use with an external database, I have chosen the secret to use the same format as provided by the CrunchyData PGO Postgres Operator, so you can just by supplying the name of a postgresql secret generated by pgo and launching this chart, install a Zabbix server and Frontend pointing to this database.

In order to also be able to use this secret feature, I have exchanged the postgresql container deployed when postgresql.enabled=true from using the Bitnami postgresql Subchart to build an own statefulset and service, using either the official "postgresql" or "timescale/timescaledb" image. The benefit of this is that now the database can respect the values in the central DB access secret and initialize accordingly.

Last but not least, the credential secret can be chosen to be auto-generated (password will be set to a random string) at chart installation, if postgresql.enabled is set to true. With this, an easy to use "out-of-the-box" installation with as little customizations as possible is possible, while still obtaining a good level of security.

What this PR does / why we need it:

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

fixes #

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

[ ] DCO signed
[ ] Chart Version bumped
[ ] Variables are documented in the README.md

Lillecarl commented 2 years ago

For a more out of the box solution this is a great step in the right way. Looks good. At most I could do some bikeshedding and say that your YAML files are POSIX uncompliant by not ending with a newline.

I would suggest adding a .editorconfig to the root of the repo enforcing newline before EOF, YAML indentation and such so that it's enforced repo-wide. 😄

sa-ChristianAnton commented 2 years ago

For a more out of the box solution this is a great step in the right way. Looks good. At most I could do some bikeshedding and say that your YAML files are POSIX uncompliant by not ending with a newline.

I would suggest adding a .editorconfig to the root of the repo enforcing newline before EOF, YAML indentation and such so that it's enforced repo-wide. 😄

You are right. Didn't notice that. Found the setting to make sure my files have newlines at the end in VS Code. Won't happen again though :)

sa-ChristianAnton commented 2 years ago

Hey guys. I would like to ask whether you are considering accepting this pull request. The reason to ask is that in the meanwhile I have done some significant further development based on where I left when I submitted this one. Now, these new developments are focussed more on improving the compatibility with the new 6.0 features (and kind of breaks the chart for pre-6.0):

switched from Statefulset to Deployment for Zabbix Server
fully support the new HA mode for Zabbix Server simply by raising replicas
periodic job to auto-clean orphaned HA nodes from DB
Initialization of DB Schema with multiple HA nodes by using a job (otherwise multiple containers try to set up the schema at the same time)
Pod Anti Affinity to spread frontends, servers, ... over cluster nodes
support for the Zabbix webservice

...and some more are planned, such as setting a random Admin password inside Zabbix, removing the Zabbix proxy part and adding an object of type Ingressroute for those that use the Traefik reverse proxy.

I cannot create pull request for these additions atm because the changes made in the meanwhile base on this pr.

I am OK with continuing my development "for my own" and changing the README etc. but I am not eager to, I would much more like to contribute to this project instead.

Thanks

Christian

aeciopires commented 2 years ago

Hello @sa-ChristianAnton !

Thank you very much for your contribution to the project. I've been away for a few weeks. I'll look at the Pull Request carefully and make my thoughts before the merge.

aeciopires commented 2 years ago

Hi @sa-ChristianAnton!

Looking by time order, I saw and merged this Pull Request and generated the 1.2.0 version of chart.

This generated conflicts in your Pull Request. Sorry.

Can you to solve the conflicts?

Can you change version of chart to 2.0.0 in Chart.yaml file and run the helm-docs command to update documentation?

Can you change the docs/example/kind/values.yaml file for show to other people how to use this funcionality?

Greetings

sa-ChristianAnton commented 2 years ago

Hi @sa-ChristianAnton!

Looking by time order, I saw and merged this Pull Request and generated the 1.2.0 version of chart.

This generated conflicts in your Pull Request. Sorry.

Can you to solve the conflicts?

Can you change version of chart to 2.0.0 in Chart.yaml file and run the helm-docs command to update documentation?

Can you change the docs/example/kind/values.yaml file for show to other people how to use this funcionality?

Greetings

working on it.

sa-ChristianAnton commented 2 years ago

I have now solved the conflicts, and made the requested changes.

BUT I believe we now have too many options for setting the authentication, additionally the option introduced by the pull request which generated version 1.2.0 is not completely clear from my point of view, as it introduces the possibility to set a secret for only the password, but only under the "zabbixserver" section which is then being used by web frontend also. From my point of view, this is inconsistent, as it is still necessary to set POSTGRES_USER for server, frontend individually, but POSTGRES_PASSWORD_SECRET is being set only for zabbixserver and automatically used everywhere.

As this kind of conflicts with my approach to have one "unified DB access" section instead, I would suggest the following, to make everything MUCH more clear and easy:

Remove all postgres username/password related settings in "zabbixserver", "zabbixweb" and "postgresql" altogether and ONLY make use of the "unified" method. At the end, it does not make sense to have these set up individually, as the whole idea of the chart is to run either one part or all together, but now two parts of it with different credentials (like a server connecting to one, and a frontend connecting to another database).
Allow to set fixed username combined with a secret only for the password (as implemented by the other pull request) within the "db_access" section to be used for all relevant components. This makes it more clear that this is being used not only by one but by all components installed by the chart that need it.

Option B would be, for more compatibility, to keep the DB related settings within the subsections "zabbixserver", "zabbixweb" and "postgresql", and add the "secret only for password" feature, as implemented in the other pull request, for each one of these individually. This would add another little bit of values, but be more clear overall, and changes would be minimal.

aeciopires commented 2 years ago

Hi @sa-ChristianAnton!

Reading your comment, I agree with you. Sorry. I should to have asked your opinion before merge the PR because both PR deal with the same subject.

With the version 2.0.0 of chart and a note of release, we can to teach other people to configure the values.

aeciopires commented 2 years ago

Hi @sa-ChristianAnton!

I saw and tested all changes.

Thanks for your contribution.

cetic / helm-zabbix