Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

central db credential mgmt w/secret, internal pgsql #53

Merged
merged 8 commits into from
May 21, 2022
Merged

central db credential mgmt w/secret, internal pgsql #53

merged 8 commits into from
May 21, 2022

Conversation

sa-ChristianAnton
Copy link
Contributor

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

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.
@Lillecarl
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor Author

sa-ChristianAnton commented May 10, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

aeciopires commented May 12, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

sa-ChristianAnton commented May 16, 2022

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:

  1. 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).
  2. 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
Copy link
Collaborator

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
Copy link
Collaborator

Hi @sa-ChristianAnton!

I saw and tested all changes.

Thanks for your contribution.

@aeciopires aeciopires merged commit 1289203 into cetic:master May 21, 2022
@sa-ChristianAnton sa-ChristianAnton deleted the unified-db-access-creds branch May 26, 2022 09:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants