[bitnami/mariadb] Don't overwrite generated secret values on chart upgrade

[mamiu]

Which chart:
Chart: mariadb
Version: latest

Describe the bug
The mariadb-password attribute in the charts secret is getting overwritten on every upgrade, if the mariadb-password was generated.

To Reproduce
Steps to reproduce the behavior:

1. Intall the mariadb chart
2. Have a look at the secret
3. Upgrade the chart
4. Check the secret again => Entries have changed

Expected behavior
Wrap the lines L19, L26 and L34 into these template functions "common.secrets.passwords.manage" or "common.secrets.exists".

Version of Helm and Kubernetes:

• Output of helm version:

version.BuildInfo{Version:"v3.6.3", GitCommit:"d506314abfb5d21419df8c7e7e68012379db2354", GitTreeState:"dirty", GoVersion:"go1.16.5"}

• Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.1", GitCommit:"632ed300f2c34f6d6d15ca4cef3d3c7073412212", GitTreeState:"clean", BuildDate:"2021-08-19T15:38:26Z", GoVersion:"go1.16.6", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.2+k3s1", GitCommit:"5a67e8dc473f8945e8e181f6f0b0dbbc387f6fca", GitTreeState:"clean", BuildDate:"2021-06-21T20:52:44Z", GoVersion:"go1.16.4", Compiler:"gc", Platform:"linux/amd64"}

Additional context
This probably applies to other charts as well (e.g. mysql, etc.).

[marcosbc]

Hi @mamiu, thanks for reporting this. You are indeed right, there is a bug in the common subchart.

It should be throwing an error, but it seems like the logic is not working properly and it is not happening. We'll continue investigating internally and provide an update whenever we have found a possible solution.

[mamiu]

Thanks @marcosbc! If you need help, let me know.

[mamiu]

@marcosbc: I've just checked it again and I can't find an issue in common... at least not in the mariadb case from above.

Cause the mariadb secret is created based on auth.existingSecret and auth.customPasswordFiles values.

And the data of the secret are also exclusively based on the mariadb chart values. So I can't see why this issue should be caused by the common subchart.

[sspreitzer]

The secrets template is not performing a lookup on any existing secret and preserving that.

From: https://codersociety.com/blog/articles/helm-best-practices#11-use-the-lookup-function-to-avoid-secret-regeneration

{{- $rootPasswordValue := (randAlpha 16) | b64enc | quote }}
{{- $secret := (lookup "v1" "Secret" .Release.Namespace "db-keys") }}
{{- if $secret }}
{{- $rootPasswordValue = index $secret.data "root-password" }}
{{- end -}}
apiVersion: v1
kind: Secret
metadata:
  name: db-keys
  namespace: {{ .Release.Namespace }}
type: Opaque
data:
  root-password: {{ $rootPasswordValue}}

[mamiu]

@sspreitzer Thanks mate, that's awesome! Exactly what I was thinking about.

[marcosbc]

Hi, unfortunately we have no plans support lookup for now.

However, we have mechanisms in place to avoid these issues via the upgrade logic. However, there is currently a bug in this logic and makes it not to throw an error if credentials were not specified. Because of that, the credentials would be recreated.

The idea is for it to throw an error so it prevents you from upgrading altogether, if they were not specified.

[mamiu]

@marcosbc Ok, I see.

Are you interested in me creating a pull request where such a feature is implemented in a single location (like the common subchart) and is included in every chart that currently overwrites existing secrets as well as charts that currently throw an error?

[sspreitzer]

@marcosbc Can you please share with the public, why you decide against supporting the official lookup helm function for now? The reasoning might be of interest for others using the function.

[marcosbc]

Hi @sspreitzer, my understanding is that it was evaluated internally, but saw that it would cause a lock-in to Helm for any chart using it, meaning that it would not be possible to export them to other formats if needed.

[mamiu]

@marcosbc That's a valid point, but I think the lock-in to Helm is given anyway for almost every chart in this project. All charts are highly customized to helm syntax and conventions, that it's very unlikely to be replaced by any other technology (except if that technology aims to be helm compatible - but in that case I'm pretty sure the lookup function would be available as well). Additionally helm is also a CNCF Graduated Project and therefore on the same level as Kubernetes. Helm won't go anywhere soon. If I see that right this whole Bitnami charts project is meant to be helm specific or is there any other use case which I'm not aware of?

BTW: The lookup function is already used in this repository. Here in the _secrets.tpl file twice (line 84 and line 125).

[sspreitzer]

I have to follow @mamiu's conclusion. Also I do not see a sense in using .Values and .Chart but not lookup(). The reason stated does not make sense from a technology point of view.
From a technology strategy point of view, Helm is an Open Source project as Kubernetes is. It does not make sense to lock-in to Kubernetes and it's functionality, but not Helm and it's functionality.

[marcosbc]

Hi all, after a more extensive internal review, it seems the reason we did not support it yet is because it required Helm 3.2. In Helm 3.1, the helm lint command would fail, which would be problematic for PR contributions. Note that we made a POC which was implemented into Jenkins, MetalLB and OrangeHRM charts as mentioned by mamiu, but didn't develop it further due to the Helm version issue.

Now that Helm 3.2 is more than a year old, it makes sense to bump the minimum Helm version requirement to 3.2 in order to support lookup in the entire catalog.

I have updated our internal task so we can continue the work on the changes to support lookup in the entire catalog. However, please be aware that these changes will require some planning ahead, so this could take some time before it gets definitely implemented, unfortunately I'm unable to provide any ETA for that.

[mamiu]

Thanks a lot @marcosbc for your feedback! Looking forward to see this feature coming. But no rush. Totally understand that projects this size need careful handling.

Do you want to keep this issue open to track the progress of it?

[marcosbc]

For now, let's keep this issue open. I added the on-hold label so it should not close automatically. If we have any updates, we'll add our progress here.

[mamiu]

@marcosbc Any updates on this one? We still have to fight this bug manually every time we upgrade a chart.

[juan131]

@mamiu we opened support for "lookup" some months ago. That's what we're doing in some charts such as PostgreSQL that use our helper "common.secrets.passwords.manage" that is based on the Helm "lookup" feature:

• https://github.com/bitnami/charts/blob/master/bitnami/postgresql/templates/secrets.yaml#L17
• https://github.com/bitnami/charts/blob/master/bitnami/common/templates/_secrets.tpl#L76

We should definitely adopt this in MariaDB too. Would you like to contribute with a PR?

[mamiu]

Thanks for your quick response @juan131!

Sure, let me try.

[sspreitzer]

If not already mentioned and you are an ArgoCD user;

Using lookup() in combination with ArgoCD does not work. ArgoCD is templating the manifests via helm template and thus does not receive the accurate cluster resources, but empty results.

See #5202

[codex70]

Is there any progress on this issue? I really need this for annotations on load balancer resources for things like the ngingx controller. So, this is an example of what I am attempting to do from the nginx application file:

    helm:
      values: |
          service:
            loadBalancerSourceRanges:
              - {{ (lookup "v1" "ConfigMap" "default" "my-config-map").data.k8s_lb_subnet }}
            annotations:
              service.beta.kubernetes.io/xxxx: {{ (lookup "v1" "ConfigMap" "default" "oci-oke-config").data.somevalue }}

As this template is going to be used in different environments (and there are several more similar annotations), we don't want to manually add all of the annotations to a git repository when those values can be easily stored in a configmap and used in any application where it is needed.

If anyone has a possible workaround for this situation until the lookup() function is supported, please let me know.

[mamiu]

@codex70 The lookup function is 100% supported. If you want to have a change that is generic and useful to other users, create a PR. But the change you want looks highly specific to your use case, so just fork this repo and customize the chart you want to your needs.

[codex70]

Hi @mamiu, thank you for such a quick response. I will double check my code, this possibly wasn't the best thread to add my question to, it was referenced from argoproj/argo-cd#5202, which is probably a more relevant place.

[mamiu]

Yeah, the issue you referenced looks like a better place for your request.