[stable/rabbitmq] better way to add plugins

[thomas-riccardi]

Is this a request for help?: no

---

Is this a BUG REPORT or FEATURE REQUEST?: feature request

Which chart: stable/rabbitmq

What happened:
I want to add an extra plugin, but rabbitmq.plugins already contains plugins by default, which are required for the chart to work (rabbitmq_management is used by the probes, rabbitmq_peer_discovery_k8s by the StatefulSet clustering).
To add my extra plugin, I have to duplicate the content of this field, making upgrades less robust (what if some additional plugins are required in the future?)

#10070 handled the same issue, but for generic rabbitmq.conf. We could do the same for rabbitmq.plugins: rabbitmq.plugins + rabbitmq.extraPlugins (to be retro-compatible).

[juan131]

Hi @thomas-riccardi

I agree it can be confusing for certain users and I'm pretty sure that some users will overwrite the plugins list (excluding the rabbitmq_management and rabbitmq_peer_discovery_k8s) so the installation will fail.

Do you want to do the corresponding PR to implement the approach you suggest? I can do it too but I'd rather give you the chance to contribute.

[thomas-riccardi]

@juan131 following your message I was going to write a PR, but it's in fact trickier than with configuration (#10070): for configuration we just had to concatenate both values (with a newline).

For plugins, the value uses an erlang conf syntax: I have no idea how to merge both lists...

[juan131]

The plugin list follows the format: [xxx, yyy, zzz].

$ kubectl exec $(kubectl get pods -l app=rabbitmq -o jsonpath='{.items[0].metadata.name}') cat /opt/bitnami/rabbitmq/conf/enabled_plugins
[rabbitmq_management, rabbitmq_peer_discovery_k8s].

Please check the PR I create to change the approach to indicate the plugins to install.

[thomas-riccardi]

@juan131

Please check the PR I create to change the approach to indicate the plugins to install.

It was merged too quickly, I didn't have time to review it before the merge. I hope you have since seen my post-merge review.

[thomas-riccardi]

@juan131 bump; should I open a new issue to follow that?

[juan131]

It was merged too quickly, I didn't have time to review it before the merg

Opps!! Sorry I just saw this.

This seems to be a breaking change: the new format accepted for rabbitmq.plugins is incompatible with the old accept format. We could make it retro-compatible by first checking if the string starts with [ to detect the old format (in which case maybe raise an error if extraPlugins is not nil ?)
Or bump the major version number for the chart.

It's incompatible with the old-format but you can still upgrade the chart using the new format. E.g.

$ helm install -f old-format-values.yaml --version 5.0.0 --name rabbitmq stable/rabbitmq
$ helm upgrade -f new-format-values.yaml --version 5.1.0 rabbitmq stable/rabbitmq

We bump the major version when there's no way to run helm upgrade XXXX without error.

could have used a real yaml array of strings instead of implicitly documented list of tokens separated by space.

Both solutions are valid. I don't see any problem on using a string with list of space separated plugins but we can use an array if you prefer it.

is the empty string a valid value when constructing the erlang config value?
if not, maybe show a valid value for commented-out extraPlugins here.
if yes, maybe explicitly define it as empty string instead of implicit nil.

Wen can put an plugin as example instead of the empty string if you prefer it that way.

[thomas-riccardi]

It was merged too quickly, I didn't have time to review it before the merg

Opps!! Sorry I just saw this.

That's OK; maybe I should have opened a new issue? Would it have been more visible?

This seems to be a breaking change: the new format accepted for rabbitmq.plugins is incompatible with the old accept format. We could make it retro-compatible by first checking if the string starts with [ to detect the old format (in which case maybe raise an error if extraPlugins is not nil ?)
Or bump the major version number for the chart.

It's incompatible with the old-format but you can still upgrade the chart using the new format. E.g.

$ helm install -f old-format-values.yaml --version 5.0.0 --name rabbitmq stable/rabbitmq
$ helm upgrade -f new-format-values.yaml --version 5.1.0 rabbitmq stable/rabbitmq

We bump the major version when there's no way to run helm upgrade XXXX without error.

I am surprised by this definition of "breaking change".
The only reference I have found is here:

Any breaking (backwards incompatible) changes to a chart should:

1. Bump the MAJOR version
2. In the README, under a section called "Upgrading", describe the manual steps necessary to upgrade to the new (specified) MAJOR version

It does not define what is a breaking change indeed..
SemVer is really defined against an API. I always assumed the API here is (notably) the input of the interactions with the chart; notably the values.yaml used with helm install or helm upgrade: If the change was not breaking, then any "old" values file should work as-is with the new chart; which is not the case here.
That's why I concluded with a breaking change.

I searched for MAJOR bumps in rabbitmq and redis:

• rabbitmq 4.0.0: [stable/rabbitmq] Normalize ports under service #9444 : just some values format change; breaking change not documented in the README
• rabbitmq 3.0.0: [stable/rabbitmq] Fix chart not being upgradable #7848 : upgrade broken as you describe; breaking change & workaround documented in the README
• redis 6.0.0: [stable/redis] Make volumePermissions init container optional #11029 : just some values format change (in some cases a new value must be set, which was not required before: I categorize that as a values format change), breaking change documented in the README
• redis 5.0.0: [stable/redis] support redis docker library image #7745 : just some values format change, breaking change documented in the README
• redis 4.0.0: [stable/redis] Major version bump: Fix chart not being upgradable #7686: (same as rabbitmq 3.0.0): upgrade broken as you describe; breaking change & workaround documented in the README

I do understand that there are various types of breaking changes, and that it's important to document them with high visibility and clarity to the charts users. If we accept values format changes as breaking changes, maybe the type of breaking change should be required to be documented in the README too?

=> this would need more discussion with the other maintainers of stable/ in a new issue I suppose.

could have used a real yaml array of strings instead of implicitly documented list of tokens separated by space.

Both solutions are valid. I don't see any problem on using a string with list of space separated plugins but we can use an array if you prefer it.

Since we have a typed format (yaml), it seemed strange to me to re-invent our own serialization of lists; however, the usability for --set in the command-line may be important. I just found out we can easily pass lists of strings with the following (surprising) syntax: --set name={a, b, c}

is the empty string a valid value when constructing the erlang config value?
if not, maybe show a valid value for commented-out extraPlugins here.
if yes, maybe explicitly define it as empty string instead of implicit nil.

Wen can put an plugin as example instead of the empty string if you prefer it that way.

👍 Useful working examples as commented-out values seems a great pattern to me.

But I also raised a second point: to we want to support multiple ways to express the empty list? And do we want to show their existence through commented-out examples (as documentation)?
With my previous suggestion to use a yaml array instead of string space separated list it would become more obvious (and we could even set the empty array as default value, instead of nil).

[nitrag]

What about plugins like rabbitmq_message_timestamp that require you to install an .ez file?