Deprecate RABBITMQ_DEFAULT_USER/PASS env vars

[coro]

This RabbitMQ configuration through environment variable is going away in a future version of
RabbitMQ. The suggested new pattern is to configure these parameters
through sysctl config-style files in /etc/rabbitmq/conf.d/ on Linux.

This provides some of the functionality required by #424. A user will receive a deprecation warning if they attempt to set the environment variables in the container. I will also make a PR to the Docker docs repo to change the instructions for Setting default user and password here.

Equivalent change to the Kubernetes operator: rabbitmq/cluster-operator#346

cc @tianon @gerhard

[michaelklishin]

@tianon @yosifkit the language in the warning is very alarming. To be clear, we won't remove env variable support in 3.9 but we want to strongly discourage their use where it's not the only available option (e.g. RABBITMQ_CONFIG_FILE is the only way to specify a config file location). This has the full support of the core team, specifically @gerhard and myself :)

[tianon]

I'm honestly a little confused why this only applies to these two variables as opposed to the more inclusive approach I'd suggested and was being discussed in #424? What makes these two special enough to warrant a special case over any others?

(The current wording also implies that this applies to all variables, when it's only checking two 😕)

[gerhard]

@tianon does this answer it? #440 (comment)

[gerhard]

I can see how we could add a single deprecation notice and stick to the gradual refactoring of entrypoint, over a number of PRs, rather than a big bang approach. WDYT?

[yosifkit]

I'm confused why there would be a need to create a new config file for these two valules being deprecated (in a place that hasn't been edited by the script before so likely to hit issues like #369).

I think I'd rather a single "if the entrypoint made a config file" then warn users and maybe print out the contents of the generated config so that they know what to use instead of the variables.

[yosifkit]

I think I'd rather a single "if the entrypoint made a config file" then warn users

Perhaps this would be initially limited to specific variables with more being added to the list over time.

[gerhard]

I'm confused why there would be a need to create a new config file for these two values being deprecated (in a place that hasn't been edited by the script before so likely to hit issues like #369).

If this config file exists, docker-entrypoint.sh should do nothing.

For users that use env vars, we show the deprecation and create the config file from the env vars provided. Eventually - I'm proposing rabbitmq:3.9 - we drop env vars support for configuring RabbitMQ properties that can be configured via conf files.

#369 was initiated in the same context, by the same team. We understand the value of keeping the RabbitMQ container image versatile, and have it work well from the simplest Docker for Desktop setups to the more complex Firecracker equivalents with Vault / KSM for secrets. While the commercial image will be different in some aspects, the behaviour should be the same, so that users can migrate in either direction (OSS -> Commercial, Commercial -> OSS) by simply changing the image reference.

I think I'd rather a single "if the entrypoint made a config file" then warn users and maybe print out the contents of the generated config so that they know what to use instead of the variables. Perhaps this would be initially limited to specific variables with more being added to the list over time.

💯 to the above.

[coro]

Thanks all for the feedback. Happy to position this more from the position that @yosifkit outlines above. I won't be able to look at this until the week after next, but will make the changes then.

[coro]

In order to be as flexible as possible, I've reverted this back to
leaving the config in rabbitmq.conf. A user may reasonably alter
$RABBITMQ_CONFIG_FILES, at which point any config files in the conf.d
directory won't be picked up.

This now keeps the env var config in $RABBITMQ_CONFIG_FILE, but prints a
deprecation warning with the example config file the user will have to
mount in the future. Here is an example deprecation notice that is generated:

$ docker run --rm --hostname my-rabbit --name some-other-rabbit -e RABBITMQ_DEFAULT_USER=myrabbituser -e RABBITMQ_DEFAULT_PASS=myrabbitpass local-38-alpine-rmq:latest

WARNING! RabbitMQ configuration via environment variables is strongly discouraged where there is a config file alternative, and will be deprecated in a future version.
In order to configure RabbitMQ without using environment variables in the future, you should mount a config file on the container matching the file path regex defined by $RABBITMQ_CONFIG_FILES (/etc/rabbitmq/conf.d/*.conf)
The environment variable config you have set can be represented as follows in such a config file:

default_pass = myrabbitpass
default_user = myrabbituser

Configuring logger redirection
...

If a consumer of this image (e.g. the operator as of PR rabbitmq/cluster-operator#346) mounts the user/password file, this will overwrite any config written through env vars (if you mistakenly set both for some reason).

In future PRs, we can extend the list of deprecated env vars by simply adding them to deprecatedConfigKeys.

[tianon]

I appreciate the effort being put into this, but I have to admit I'm a little confused why we're continuing with a piecemeal approach instead of finishing the discussion/implementation in #424? There are open questions there that are the only reason we don't have an implementation that deprecates all our variables at once (I don't see the value in deprecating some variables without going all the way).

[coro]

Sure thing! My intention with this PR was to provide the first step towards the goals of #424 by mounting the default creds in conf.d as part of the entrypoint script, as a POC that this method of deprecation was suitable. The choice of these two specific variables is simply because I was in the area doing rabbitmq/cluster-operator#346, and the outstanding questions in #424 seemed unrelated to these specific variables. I wrote the code in such a way that one could extend to other environment variables at a later date easily if one so wished.

My inference was that #424 was to start a discussion, however if you would prefer to implement all of the deprecation at once in that PR, then feel free to do so there once you've got answers to your questions (I think you're perhaps waiting on a response from @gerhard ?), and close this PR :)

[michaelklishin]

@tianon @yosifkit I wonder what the state of this? Should it be closed? Our Kubernetes Operator has found an acceptable workaround for this but if we want to use env variables less and rabbitmq.conf more as a strategy, this seems to be a good first step to me.

[tianon]

Sorry -- I'm still not sure I understand why we would want to add specific deprecations for one or two variables when the approach in #424 automatically covers them all at once?
(and with a much smaller "warning" implementation to maintain in the interim)

There's just a couple outstanding implementation/deprecation questions there that I need some help determining whether matter:

1. we currently set RABBITMQ_USE_LONGNAME=true if hostname and hostname -s return different values -- should we keep that behavior, or deprecate it? Add deprecation warnings to "docker-entrypoint.sh" #424 (comment) (current PR code adds a deprecation warning in this situation; I just want confirmation that makes sense)

2. it's been determined that RABBITMQ_ERLANG_COOKIE is generally useful -- should that functionality move into RabbitMQ itself? Add deprecation warnings to "docker-entrypoint.sh" #424 (comment) (current PR code has a TODO comment for this, no warning)

3. should vm_memory_high_watermark in RabbitMQ itself be updated to take cgroups into account natively? Add deprecation warnings to "docker-entrypoint.sh" #424 (comment) / Add deprecation warnings to "docker-entrypoint.sh" #424 (comment) (current code will warn, but users don't have a migration path for "percentage of my cgroup limits" natively which is what our implementation was doing and why it was more complex than simply "set this value in the config")

[michaelklishin]

@tianon OK, let's then close this and proceed with #424 :)

[tianon]

Fair enough; thanks @coro! Hopefully we can resolve #424 quickly. 😅