Use load_definitions instead of management.load_definitions

[michaelklishin]

See #429 for the explanation of what the difference is and why this image should use the newer core mechanism (and not the original management plugin feature).

Closes #429.

[yosifkit]

Since this only applies to 3.8+, we'll need to sed the file for 3.7 in update.sh instead of just copying it.

rabbitmq/update.sh

Line 135 in 0c7faaf

cp -a docker-entrypoint.sh "$version/$variant/"

Or maybe we could just point load_definitions to a directory via the Dockerfile and leave this in the entrypoint for 3.7? (thus slowly removing the entrypoint)

[michaelklishin]

@yosifkit I'm afraid I'm not following. The need to update 3.7 escaped me, since 3.7 is extremely unlikely to get any new releases (it's out of general support and goes out of any kind of support on October 1st).

Since every series has its own Dockerfile, it makes sense to use it for version-specific bits as much as possible. Definition import does support directories but IIRC it is still an undocumented feature we have added for our own future product needs. Do you mean "point it at a file"?

[tianon]

If we statically point it at a file and the user doesn't provide one, it will throw an error, right?

(That was my reading of the associated code, which is where I found the ability to provide a directory and saw that an empty directory shouldn't error, so setting it statically to something like load_definitions = /etc/rabbitmq/definitions.d seemed like a sane choice from the code. 😅)

[michaelklishin]

True, we validate all file paths for existence. Pointing it at a directory won't error. 3.7 won't recognize this key at all as it's never been backported and not in the config schema.

I'd rather find a way to control this setting in Dockerfile is possible.

[michaelklishin]

Using a convention-based /etc/rabbitmq/definitions.d makes sense to me in general, however, I'm not sure what the backwards-compatibility ramifications of doing so would be?

[tianon]

For backwards compatibility, I think we use this approach (if the file exists, we set load_definitions in the generated config), while we figure out #424.

Here's my proposed diff (against #422 because it makes similar changes and that makes the diff smaller), which keeps 3.7 doing exactly what it does today and keeps 3.8 backwards-compatible but using the newer parameter when the file exists, but always includes definitions.d as well (not sure what RabbitMQ will do with both a file and a directory set with conf.d):

$ GIT_PAGER=cat git diff Dockerfile* docker-entrypoint.sh update.sh
diff --git a/Dockerfile-alpine.template b/Dockerfile-alpine.template
index f29ac98..3f9038e 100644
--- a/Dockerfile-alpine.template
+++ b/Dockerfile-alpine.template
@@ -231,6 +231,8 @@ RUN set -eux; \
 	rm "$RABBITMQ_DATA_DIR/.erlang.cookie"
 # Enable Prometheus-style metrics by default (https://github.com/docker-library/rabbitmq/issues/419)
 RUN set -eux; rabbitmq-plugins enable --offline rabbitmq_prometheus; echo 'management_agent.disable_metrics_collector = true' > /etc/rabbitmq/conf.d/management_agent.disable_metrics_collector.conf
+# Enable "load_definitions" by default (https://github.com/docker-library/rabbitmq/issues/429)
+RUN set -eux; mkdir /etc/rabbitmq/definitions.d; chown rabbitmq:rabbitmq /etc/rabbitmq/definitions.d; echo 'load_definitions = /etc/rabbitmq/definitions.d' > /etc/rabbitmq/conf.d/load_definitions.conf
 
 # Added for backwards compatibility - users can simply COPY custom plugins to /plugins
 RUN ln -sf /opt/rabbitmq/plugins /plugins
diff --git a/Dockerfile-ubuntu.template b/Dockerfile-ubuntu.template
index 84de22a..987193a 100644
--- a/Dockerfile-ubuntu.template
+++ b/Dockerfile-ubuntu.template
@@ -249,6 +249,8 @@ RUN set -eux; \
 	rm "$RABBITMQ_DATA_DIR/.erlang.cookie"
 # Enable Prometheus-style metrics by default (https://github.com/docker-library/rabbitmq/issues/419)
 RUN set -eux; rabbitmq-plugins enable --offline rabbitmq_prometheus; echo 'management_agent.disable_metrics_collector = true' > /etc/rabbitmq/conf.d/management_agent.disable_metrics_collector.conf
+# Enable "load_definitions" by default (https://github.com/docker-library/rabbitmq/issues/429)
+RUN set -eux; mkdir /etc/rabbitmq/definitions.d; chown rabbitmq:rabbitmq /etc/rabbitmq/definitions.d; echo 'load_definitions = /etc/rabbitmq/definitions.d' > /etc/rabbitmq/conf.d/load_definitions.conf
 
 # Added for backwards compatibility - users can simply COPY custom plugins to /plugins
 RUN ln -sf /opt/rabbitmq/plugins /plugins
diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh
index 1b16604..ed0e2d8 100755
--- a/docker-entrypoint.sh
+++ b/docker-entrypoint.sh
@@ -387,7 +387,7 @@ if [ "$1" = 'rabbitmq-server' ] && [ "$shouldWriteConfig" ]; then
 		managementDefinitionsFile='/etc/rabbitmq/definitions.json'
 		if [ -f "$managementDefinitionsFile" ]; then
 			# see also https://github.com/docker-library/rabbitmq/pull/112#issuecomment-271485550
-			rabbit_set_config 'management.load_definitions' "$managementDefinitionsFile"
+			rabbit_set_config 'load_definitions' "$managementDefinitionsFile"
 		fi
 	fi
 fi
diff --git a/update.sh b/update.sh
index 365485f..10ac7aa 100755
--- a/update.sh
+++ b/update.sh
@@ -148,14 +148,19 @@ for version in "${versions[@]}"; do
 
 		# rabbitmq_prometheus is only included in 3.8+
 		# same with "/etc/rabbitmq/conf.d" support (https://github.com/rabbitmq/rabbitmq-server/pull/2258, https://github.com/rabbitmq/rabbitmq-server/pull/2277)
+		# same with "/etc/rabbitmq/definitions.d" support
 		if [ "$rcVersion" = '3.7' ]; then
 			sed -ri \
 				-e '/[Pp]rometheus/d' \
 				-e '/metrics_collector/d' \
 				-e 's/ 15691 15692 / /g' \
 				-e 's! /etc/rabbitmq/conf.d ! !g' \
+				-e '/load_definitions/d' \
 				"$version/$variant/Dockerfile" \
 				"$version/$variant/management/Dockerfile"
+			sed -ri \
+				-e 's/load_definitions/management.load_definitions/g' \
+				"$version/$variant/docker-entrypoint.sh"
 		fi
 	done
 done

[michaelklishin]

@tianon this looks promising. So we will have a duplicate key? In that case, it's the "last update wins" IIRC but I need to try it.

[tianon]

Yeah, we'll have a duplicate key, but only when they provide /etc/rabbitmq/definitions.json -- if that's a problem, we could also have the script delete /etc/rabbitmq/conf.d/load_definitions.conf, or have it copy/symlink /etc/rabbitmq/definitions.json into /etc/rabbitmq/definitions.d for 3.8.

[michaelklishin]

@tianon I have a radical (and beautiful in its simplicity) solution: let's just drop 3.7 support entirely for this image. 3.7 goes out of any kind of support on October 1st. There will be one more security patch release of 3.7.x, after that, we can basically assume the 3.7 flavour of this image won't have any upstream releases to benefit from.

[michaelklishin]

Honestly, the attack 3.7.28 will address is local, requires elevated privileges and is Windows-specific. So this image isn't really affected. So let's drop 3.7 support completely, for good, and merge this PR as is. How does this sound?

@gerhard @mkuratczyk

[tianon]

That doesn't help us ditch this script, but it does push that conversation to be much more orthogonal! 😄

I've opened #431 for that deletion, assuming we don't get any dissenting votes. 😄

[michaelklishin]

@tianon @yosifkit does the merge of #431 mean this is ready to go or more work is needed?

[tianon]

Just needs to be copied to the image build context directories (which ./update.sh will do, but it'll also try to scrape new versions -- we're working on splitting those functions into separate stages so it's easier to just "re-apply the templates" generally, and then have something like GitHub Actions auto-validate that the templates are applied correctly; see docker-library/php#1052 if you want to see progress on that idea).

[michaelklishin]

@tianon is this something I can contribute to this image? This pretty trivial change is taking weeks to adopt :/

[tianon]

I'm sorry, I guess I needed to be more explicit in #430 (comment) -- the docker-entrypoint.sh file at the top level is the "template" for all the subdirectories. For us to merge this with a proper passing CI test, it needs to be copied to the individual image subdirectories first (as running ./update.sh would do, and as I have now done in d1d424e). Also, otherwise @docker-library-bot will follow up the merge of this PR with nonsense commit messages about updating versions when it's really just updating docker-entrypoint.sh files.

It could also use a rebase, but it was easy enough to ignore the 3.7-related changes ./update.sh wanted to apply (and the CI is smart enough to test the merge IIRC).

[michaelklishin]

Thank you for the quick response 🙏