[rabbitmq] Don't Regenerate the Password

[kferrone]

Which chart:
RabbitMQ 7.5.3

Describe the bug
If you update the chart the the password in the secret will regenerate and cause RabbitMQ to end up in an infinite crashloop.

To Reproduce
Steps to reproduce the behavior:

1. leave auth.password empty in the values
2. run the chart
3. change something, anything at all
4. re-deploy
5. notice the password is changed after an update

Expected behavior
Leave the password and erlang cookie alone when updating.

Version of Helm and Kubernetes:

• Output of helm version:

version.BuildInfo{Version:"v3.2.4", GitCommit:"0ad800ef43d3b826f31a5ad8dfbb4fe05d143688", GitTreeState:"dirty", GoVersi
on:"go1.14.3"}

• Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.4", GitCommit:"224be7bdce5a9dd0c2fd0d46b83865648e
2fe0ba", GitTreeState:"clean", BuildDate:"2019-12-11T12:47:40Z", GoVersion:"go1.12.12", Compiler:"gc", Platform:"darwin
/amd64"}
Server Version: version.Info{Major:"1", Minor:"16+", GitVersion:"v1.16.8-eks-e16311", GitCommit:"e163110a04dcb2f39c3325
af96d019b4925419eb", GitTreeState:"clean", BuildDate:"2020-03-27T22:37:12Z", GoVersion:"go1.13.8", Compiler:"gc", Platf
orm:"linux/amd64"}

Additional context
All you need to do to solve this issue is add the following annotation to the secret in this file

annotations:
  "helm.sh/resource-policy": no-upgrade-existing

[joancafom]

Hi @kferrone,

Thanks very much for your issue! When upgrading the application, the old values for passwords must be provided to ensure they are not altered. For further details on this, the process is described in the Upgrading section of README.md.

We are currently working on improving this behaviour, analysing and evaluating alternatives on what is the best way to tackle this problem. Thank you very much for your proposal, we will take it into account when implementing changes.

Thanks!

[kferrone]

I have made plenty of charts where the following works just fine:

  annotations:
    "helm.sh/hook": pre-install
    "helm.sh/resource-policy": no-upgrade-existing

Then no secrets are changed after an update.

If anything at least allow us to add our own annotations to the secret. For example in the values file you could have something like:

secretAnnotations: {}

Then I can do the following in my values.yaml:

secretAnnotations:
  annotations:
    "helm.sh/hook": pre-install
    "helm.sh/resource-policy": no-upgrade-existing

This way I can use the built in Helm feature for handling generated secrets.

[joancafom]

Hi @kferrone

I am interested in this alternative, but have been investigating about the resource-policy: no-upgrade-existing and it seems like the official Helm release does not support it. I have been unable to find any official documentation nor source code associated to it in the official repo (the only value that seems to be supported is keep).

I might be wrong, but it would be bizarre using an annotation by default that is not officially supported by helm itself. Do you by any chance have more information about it?

Thanks!

[kferrone]

Here is what I found for reference: https://github.com/helm/helm/pull/5290/files

I can't seem to find much about the no-upgrade-existing? I was told somewhere to do this and it has just worked for me. It seems Helm may have just gone with keep.

Either way, I do know with "helm.sh/hook": pre-install on the secret, this will also give the desired effect as well. I simply know this works from experience. When secret is run in pre-install, then no other operation (including delete) will touch the secret from then on simply because install phase only ever happens once. Just don't give it a delete policy.

I agree defaulting any of these annotations without proper testing and discussion is not desired. However, I'm extremely confident in what I need to do, please allow users to add their own arbitrary annotations.

Currently rabbitmq chart already supports arbitrary annotations on the following:

• .Values.podAnnotations
• .Values.service.annotations
• .Values.ingress.annotations

In the values simply add a section to add arbitrary annotations to the secret.
values.yaml

secretAnnotations: {}

In my values.yaml; I'll just overkill it to guarantee the secret won't change.

secretAnnotations:
  "helm.sh/hook": pre-install
  "helm.sh/resource-policy": keep

Here is the needed source code in the secrets.yaml in the templates

apiVersion: v1
kind: Secret
metadata:
  name: {{ include "rabbitmq.fullname" . }}
  namespace: {{ .Release.Namespace }}
  labels: {{- include "common.labels.standard" . | nindent 4 }}
  annotations: {{ toYaml .Values.secretAnnotations | nindent 4 }}

[joancafom]

Hi @kferrone

It is true that rabbitmq chart already supports arbitrary annotations on several resources. However, the annotations you plan to use are helm hooks and I would like to investigate your use case before (e.g. imagine these annotations were triggered before the template is even rendered. That way this solution would not work). After evaluating it, I will come here and post my findings and we could proceed with a PR!

Thanks for your issue!

[joancafom]

Hi again @kferrone ,

My apologies for the delay. As I have already mentioned, we are actively discussing on how to overcome this problem and the conversation has veered towards a common implementation for all charts, in which we are currently working on PR #3150 . I think this way we can preserve a common approach in all our solutions and the issue should be gone as well.

[lindhe]

It's my experience that the password cannot be changed, because the PVC seems to store it and not let go unless I uninstall and manually delete the PVC. Is this part of the same issue, do you think?

[joancafom]

Hey @lindhe ,

That is totally right. The issue comes from the fact that the configuration is persisted in PVC, including credentials for access. Hence, if you were to change the values for them in an already deployed environment, there will be a mismatch between the current and persisted values, resulting in an authentication failure.

More on this in #2805 (comment)

[lindhe]

Thanks for the info! For me, that's not a big issue. The big issue in my eyes is that it persists even across deployments. So if I helm install with one set of credentials and then helm uninstall, the next helm install will get the old credentials. That's really confusing.

[joancafom]

You're welcome @lindhe ,

If you are using helm to perform the installation, you should either provide a release name or generate a random one. There should not be any problems as long as they are different releases.

In the case of same-named releases, you should in theory watch out for existing PVCs. I say in theory because I have tried to perform a install-uninstall-install with the same release name and it seems to be working for me.

[stale]

This Issue has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thanks for the feedback.

[stale]

Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Issue. Do not hesitate to reopen it later if necessary.

[TBBle]

This is still a thing that would be nice to have fixed. All #3150 did was make the upgrade fail-safe if the existing secret's data was not included, but I don't see any discussion there about instead marking such secrets as not modified by Helm upgrades. as they cannot safely be changed be upgrades, per #2805 (comment).

Sadly, the no-upgrade-existing feature didn't make it into Helm as the existing approach had design flaws, and it appears it was never revisited/revised to fix the design issues and be rebased to Helm 3.

I'm unaware of any other way to work-around this in Helm, although there was a suggestion on how to make it work as long as you don't try to change the password during an upgrade (moving from random to specified) which seems to be a failure case here anyway, as the secret ends up in a PVC.

[joancafom]

Hi @TBBle ,

AFAIK there is no easy or straightforward solution to this. The current implementation does indeed have a similar behaviour to the suggestion you point out. Both approaches allow the Chart to be upgraded with no errors, but our solution also serves as a standardized fail-safe method for all our charts, that informs the user of the situation. Hence, I might be wrong but I think the final objective is achieved anyways.

Thanks!