Rework postgres access, introduce automated install/upgrade tests #148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 27 commits
December 31, 2024 17:59
In order to make the Helm Chart a bit less complex and also to be able to do a "out-of-the-box" deployment with server HA enabled, the following changes have been made: - `postgresAccess.useUnifiedSecret` has been removed. Now, ALWAYS a unified secret will be used, either an existing one supplied by the user, or one generated with a fixed name (name of the release + some suffix) - `postgresAccess.unifiedSecretAutoCreate` has been removed in favor of `postgresAccess.existingSecretName`. When this value is set, it will be assumed that an existing secret is already in the namespace having the DB-relevant settings in the keys specified under `postgresAccess` - `postgresAccess.unifiedSecretXXXKey` values are renamed into `postgresAccess.secretXXXXKey` for easiness. I didn't like the naming of "unifiedSecret" anymore, even I did implement this myself and I think now is a good moment to get rid of these value names - `postgresAccess.passwordSecret` and `postgresAccess.passwordSecretKey` have been removed. Either use one secret for all DB relevant settings or supply all settings, including password, in the values.yaml file - The DB-Init-Upgrade job will get all DB connection relevant settings handed in as literal env variables instead of references to the secret when `postgresAccess.existingSecretName` is not set. The reason is that the job runs as Helm "pre-install" / "pre-upgrade" job and therefore will run BEFORE any manifest would be rendered, including the secret itself... (Chicken-egg-problem) The changes mentioned above have yet been only roughly tested. Also, documentation, examples, etc. will have to be reworked before creating a PR out of this.
Due to the checken-and-egg problem of Helm, not being able to deploy any manifests before actual pre-install / pre-upgrade jobs are being executed and the need of employing exactly these due to how Zabbix Server interacts with the database (in HA mode), we decided not to bloat the Helm Chart with even more options but clearly refuse the combination of "internal database" and Server HA.
in order to prepare automated testing of this helm chart, two simple tests, verifying successful connection to zabbix server and zabbix web frontend, have been implemented and can now be used with the "helm test <releasename>" command.
aeciopires
added
bug
aeciopires
added
the
break change
aeciopires
reviewed
Jan 8, 2025
aeciopires
reviewed
Jan 8, 2025
aeciopires
reviewed
Jan 8, 2025
aeciopires
reviewed
Jan 8, 2025
aeciopires
requested changes
Jan 8, 2025
There was a problem hiding this comment.
Wow @fibbs!
Thanks a lot!
You made a big and great job in this PR.
Thanks, man.
I made little comments before merge.
aeciopires
approved these changes
Jan 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Redesign of postgresAccess: existingSecret, keys...
In order to make the Helm Chart a bit less complex and also to be able to do a "out-of-the-box" deployment with server HA enabled, the following changes have been made:
postgresAccess.useUnifiedSecrethas been removed. Now, ALWAYS a unified secret will be used, either an existing one supplied by the user, or one generated with a fixed name (name of the release + some suffix)postgresAccess.unifiedSecretAutoCreatehas been removed in favor ofpostgresAccess.existingSecretName. When this value is set, it will be assumed that an existing secret is already in the namespace having the DB-relevant settings in the keys specified underpostgresAccesspostgresAccess.unifiedSecretXXXKeyvalues are renamed intopostgresAccess.secretXXXXKeyfor easiness. I didn't like the naming of "unifiedSecret" anymore, even I did implement this myself and I think now is a good moment to get ridof these value names
postgresAccess.passwordSecretandpostgresAccess.passwordSecretKeyhave been removed. Either use one secret for all DB relevant settings or supply all settings, including password, in the values.yaml filepostgresAccess.existingSecretNameis not set. The reason is that the job runs as Helm "pre-install" / "pre-upgrade" job and therefore will run BEFORE any manifest would be rendered, including the secret itself... (Chicken-egg-problem)batch/v1beta1postgresql.enabled=trueis set (internal DB) andzabbixServer.zabbixServerHA.enabled=trueis also set, as this situation is not supported anymore, due to the above mentioned limitations in how Helm Jobs work.Implemented automated tests using
helm installandhelm upgradeIn order to verify proper functionality of the different use cases and scenarios in which this Helm Chart should work, I have implemented automated tests for different installation and upgrade situations the Chart should work in:
postgresql.enabled=true), HA disabled, no existing secret suppliedIn any of these scenarios, I have build tests to deploy "as simple as possible" as well as installing the latest supported LTS version of Zabbix (6.0) and upgrading to 7.0 and 7.2, respectively.
The jobs have been unified with the already existing linter jobs. They have been enabled for manual execution (in development branches in private forks, for example) and to be mandatorily running on Pull Requests.
Which issue this PR fixes
Special notes for your reviewer:
#143 is NOT yet fixed with this PR, other than previously discussed. @aeciopires I suggest as follows: we first merge this PR into main, then I start working on the labels. This way we can already use the tests introduced in this PR (they have to be in main to be working).
Checklist