[zabbix-community/zabbix] issue title "Secret not created for external database with postgresql.enabled=false"

[tapanswain1]

Description

When using an external PostgreSQL database (postgresql.enabled=false), the Helm chart does not create the required Secret for database credentials, even though unifiedSecretAutoCreate=true and useUnifiedSecret=true. This makes it difficult to configure the chart for external databases without manually creating the Secret.

Steps to Reproduce

1. Set postgresql.enabled=false in values.yaml.
2. Set postgresAccess.useUnifiedSecret=true and postgresAccess.unifiedSecretAutoCreate=true.
3. Deploy the Helm chart.

Expected Behavior

The chart should create the Secret for database credentials when unifiedSecretAutoCreate=true and useUnifiedSecret=true, regardless of postgresql.enabled.

Actual Behavior

The chart does not create the Secret if postgresql.enabled=false.

Workaround

Manually create the Secret for database credentials.

Suggested Fix

Update the Secret template condition in the Helm chart to remove the dependency on postgresql.enabled.

[tapanswain1]

helm-zabbix/charts/zabbix/templates/secret-db-access.yaml

Line 1 in a0c823b

{{- if and .Values.postgresAccess.useUnifiedSecret .Values.postgresAccess.unifiedSecretAutoCreate .Values.postgresql.enabled }}

we need to remove the .Values.postgresql.enabled

[fibbs]

The idea of unifiedSecretAutoCreate was actually to auto-create the secret needed to access database just in cases of a "test installation" or something, and in my head it only made sense to do that when the Helm Chart has the control over the database, thus, postgresql.enabled: true. In my opinion, when using an external database, you would have credentials for it and would want to create the appropriate secrets before deploying this helm chart anyway. I did not think about anyone wanting to put the database credentials (username, password) in clear text in a values.yaml instead to let the Chart create the secret with those. But I don't see a problem in supporting this usecase as apparently there is people (at least one) wanting to do so.

[aeciopires]

Hi @fibbs!

I think it's reasonable and easy to do this. I would just need to adjust the 'if' statement on line 1 of the secret, right?
I'm not sure about the 'if' statement on line 24... would I have to change it there too?

[fibbs]

Let me look at it and also at the documentation and whether it may have to be rephrased.

[fibbs]

Uff! I just figured out something ugly! Auto-creating the secret will in any ways not work when using zabbixServer.zabbixServerHA: the db-create-and-upgrade job which I changed to be a pre-install / pre-upgrade job (which is actually the only viable way to implement it, really), needs access to the database which is provided by the secret. If the secret is already existing, that will work fine, if not (postgresAccess.unifiedSecretAutoCreate), the secret will not be rendered before but after the job, which is rather ugly. This of course counts in both situations: external or internal database.

I will try to figure out a way around this:

• the job should get postgres credentials literally instead of referring to a secret if unifiedSecretAutoCreate is true

After looking at it more in detail, yes we could add some more {{ if }}, {{ then }}, {{ else }} inside zabbix.postgresAccess.variables variable definition in _helpers.tmpl, but this would make this monster even more montruous, and very difficult to understand.

One thing to make this a bit less complex would be to remove useUnifiedSecret altogether and make it the only way to supply secrets for db access. Then, there would be a useExistingSecret just like almost all other Helm Charts do it nowadays. and the existing configuration for which key in the secret should be what. Also, I would remove the possibility to ONLY supply a secret for DB password but pass in all other values literally. So, then you either pass in all values literally in values.yaml and a secret will be generated for you (with the above mentioned exception of the db-init-upgrade job which will receive values literally, but short-lived) or you supply an existing secret, no other options.

That would make it still complex, but quite a bit less than what it is now. I will work on a proposal for this during the next days.

[tapanswain1]

Hi Team,
We want Secret be created , We seal the secret with the help of Bitnami SealedSecret, which convert the base64 encoded data to encryptedData, and we can store the secret in GitHub.
As of now we create a secret with only the RDS password, as the rest of the variables will come from the value file, which can be in plain text. We will achieve this by setting useUnifiedSecret and unifiedSecretAutoCreate to False.

One more we donot want zabbix/templates/job-create-upgrade-db.yaml to run . but it getting trigger when we make if and .Values.zabbixServer.enabled .Values.zabbixServer.zabbixServerHA.enabled . please help here .

[aeciopires]

Hi, @fibbs!

I suggest a direct and simple path...
If someone needs to create an external PostgreSQL database to use as a helm chart, I suggest the initContainer job script check if the database credentials have been provided. If not, display an error message saying that the database credentials do not exist and ask if the secret with the credentials has been created and don't remove the pod to not lose the log... leave the container always in pending to be identified in troubleshooting.

Reference: https://stackoverflow.com/questions/8001094/bash-shell-infinite-loop-with-if-else-statment

This avoids code complexity in the helm chart and makes explicit a requirement and responsibility for those who want to work in this scenario.

What do you think?

Do you agree @tapanswain1?

[aeciopires]

Hi @tapanswain1!

About disabling the job execution, I got confused... Why don't you want the job to run? What problem do you see in that?

Anyway, I think we can create a specific boolean variable for this, but keep the value true (by default).

[fibbs]

Hi, @fibbs!

I suggest a direct and simple path... If someone needs to create an external PostgreSQL database to use as a helm chart, I suggest the initContainer job script check if the database credentials have been provided. If not, display an error message saying that the database credentials do not exist and ask if the secret with the credentials has been created and don't remove the pod to not lose the log... leave the container always in pending to be identified in troubleshooting.

Problem is that with the last change the job would also get stuck if you just did a "stupid install" with postgresql.enabled: true as also here we need the pre-install job which would not see the secret yet, which made me opt for the way designed above.

[fibbs]

Hi Team, We want Secret be created , We seal the secret with the help of Bitnami SealedSecret, which convert the base64 encoded data to encryptedData, and we can store the secret in GitHub. As of now we create a secret with only the RDS password, as the rest of the variables will come from the value file, which can be in plain text. We will achieve this by setting useUnifiedSecret and unifiedSecretAutoCreate to False.

Sorry, do absolutely not get what you are wanting to say here. You say you want the secret be created, agreed, as this is what the initial Issue is about. But you also mention you want to use SealedSecrets Operator which actually assumes you are creating the secret externally, prior of installing the Helm Chart. Procecure for working with SealedSecrets is as follows:

1. kubectl create secret generic --from-literal key=value --from-literal otherkey=value ..... -o yaml --dry-run=client | kubeseal -o yaml > secret-file.yaml
2. kubectl create -f secret-file.yaml
3. helm install

For such a scenario you would have unifiedSecretAutoCreate: false, as you would NOT want the Helm Chart to create the secret. The same situation would be if you would be using a Postgres operator such as CNPG. These place a secret into the namespace providing you with all the needed credentials, so you never have to persist a secret manifest in Git.

One more we donot want zabbix/templates/job-create-upgrade-db.yaml to run . but it getting trigger when we make if and .Values.zabbixServer.enabled .Values.zabbixServer.zabbixServerHA.enabled . please help here .

That part I don't get either. If you do not want job-create-upgrade-db.yaml to run, you simple switch zabbixServer.zabbixServerHA off. You will not get Zabbix native server HA and you will be limited to one Zabbix Server pod replica, then.

Please clarify :-)

[aeciopires]

Hi, @fibbs!
I suggest a direct and simple path... If someone needs to create an external PostgreSQL database to use as a helm chart, I suggest the initContainer job script check if the database credentials have been provided. If not, display an error message saying that the database credentials do not exist and ask if the secret with the credentials has been created and don't remove the pod to not lose the log... leave the container always in pending to be identified in troubleshooting.

Problem is that with the last change the job would also get stuck if you just did a "stupid install" with postgresql.enabled: true as also here we need the pre-install job which would not see the secret yet, which made me opt for the way designed above.

Got it @fibbs! Thanks for explaining the issue.
In Kubernetes we have no control over the order in which objects are created, so it's hard to solve this.

[fibbs]

Got it @fibbs! Thanks for explaining the issue. In Kubernetes we have no control over the order in which objects are created, so it's hard to solve this.

Actually, Helm and those pre/post jobs make it even more complicated, as there is no "multi-level dependencies" there. Would be awesome to have a way to define specific manifests to be deployed before actual pre-jobs are run, but Helm doesn't go that far. We are working on the very border to need an operator here, which is something I see the need for in the future but I would like to omit as long as possible.

I am already working on the solution and scratching my head a lot, and I will let you know here before creating the PR so you may have a first look onto how I thought it could be a good solution, @aeciopires

[aeciopires]

@fibbs look this: helm/helm#10292
What do you think?

[fibbs]

@fibbs look this: helm/helm#10292 What do you think?

which would be switching back to the post-install / post-upgrade hooks as we had it before. But that doesn't work in our case due to the fact that zabbix server pods should not be accessing the database when the job accesses it, as otherwise it will refuse to upgrade the schema in case of a major release upgrade. That's what I tried to trick with the init containers though, but these blocked Helm from recognizing the release to be "Ready" when using --wait, and ArgoCD is having the same issue. So, I still believe pre-install / pre-upgrade is the better solution...

[aeciopires]

what caught my attention was the ability to reduce weight between the hooks to have some control over the order...

[fibbs]

@aeciopires If you want, please check out this: fibbs@fab9b16

I am not yet at the point where I want to create a PR out of this, but at least the first check worked perfectly. I will do a lot more testing and writing, and then create a PR. Next year :-)