Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rabbit_plugins: Filter non-plugins out #2212

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Conversation

dumbbell
Copy link
Member

When the module discovers plugins in the plugins directories, it considers all applications there. There are actual RabbitMQ plugins, their dependencies, and possibly other applications which are neither
plugins nor dependencies.

So far, this was the case of syslog which could be used by Lager if the user configures it. To make sure rabbit_plugins or rabbitmq-plugins(8) didn't mess with syslog, there was a hard-coded filtering function which excluded this specific application.

In RabbitMQ Enterprise Edition, we want to add another application of this kind: inet_tcp_compress_dist. This application implements an Erlang distribution protocol, so it is effectively unrelated to RabbitMQ. However, to keep packaging simple, it is provided in the plugins
directory.

We could add it to the hard-coded filtering function, but instead this function is replaced by the following logic to filter plugins:

  • Applications which depend on rabbit are considered actual plugins.
  • Plugins' dependencies, direct and indirect, are considered plugins as well.

Therefore, rabbit_plugins will ignore:

  • Applications which don't depend on rabbit and no plugins depend on them (again, directly or indirectly)
  • Applications which are part of Erlang/OTP.

This means the behavior slightly changes: a plugin which didn't declare its dependency to rabbit will not be considered a plugin anymore. I consider it is a bug in the plugin in the first place.

The code will log the following debug messages to list ignored applications:

2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring getopt, not a RabbitMQ plugin
2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring inet_tcp_compress_dist, not a RabbitMQ plugin
2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring lz4, not a RabbitMQ plugin
2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring syslog, not a RabbitMQ plugin
2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring zstd, not a RabbitMQ plugin

@dumbbell dumbbell added this to the 3.7.24 milestone Jan 16, 2020
@dumbbell
Copy link
Member Author

Once merged and CI'd, this change should be backported to 3.8.x and 3.7.x to keep the behavior consistent accross branches.

When the module discovers plugins in the plugins directories, it
considers all applications there. There are actual RabbitMQ plugins,
their dependencies, and possibly other applications which are neither
plugins nor dependencies.

So far, this was the case of `syslog` which could be used by Lager if
the user configures it. To make sure `rabbit_plugins` or
rabbitmq-plugins(8) didn't mess with `syslog`, there was a hard-coded
filtering function which excluded this specific application.

In RabbitMQ Enterprise Edition, we want to add another application
of this kind: `inet_tcp_compress_dist`. This application implements
an Erlang distribution protocol, so it is effectively unrelated to
RabbitMQ. However, to keep packaging simple, it is provided in the
plugins directory.

We could add it to the hard-coded filtering function, but instead this
function is replaced by the following logic to filter plugins:
  * Applications which depend on `rabbit` are considered actual plugins.
  * Plugins' dependencies, direct and indirect, are considered plugins
    as well.

Therefore, `rabbit_plugins` will ignore:
  * Applications which don't depend on `rabbit` and no plugins
    depend on them (again, directly or indirectly)
  * Applications which are part of Erlang/OTP.

This means the behavior slightly changes: a plugin which didn't declare
its dependency to `rabbit` will not be considered a plugin anymore. I
consider it is a bug in the plugin in the first place.

The code will log the following debug messages to list ignored
applications:

2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring getopt, not a RabbitMQ plugin
2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring inet_tcp_compress_dist, not a RabbitMQ plugin
2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring lz4, not a RabbitMQ plugin
2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring syslog, not a RabbitMQ plugin
2020-01-14 13:18:41.468 [debug] <0.941.0> Plugins discovery: ignoring zstd, not a RabbitMQ plugin
@gerhard
Copy link
Contributor

gerhard commented Jan 16, 2020

I have already tested this in the context of rabbitmq-enterprise-3.8.3-r1, part of https://github.com/rabbitmq/inet_tcp_compress_dist/pull/5. I agree that we should backport this. Will have a go at cherry-picking this merge into 3.8.x & 3.7.x.

cc @michaelklishin for visibility

Copy link
Contributor

@gerhard gerhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA in the context of rabbitmq-enterprise-3.8.3-r1

@gerhard gerhard merged commit eaecec2 into master Jan 16, 2020
@gerhard gerhard deleted the filter-non-plugins-out branch January 16, 2020 11:23
@dumbbell
Copy link
Member Author

Thank you!

dumbbell added a commit to rabbitmq/rabbitmq-cli that referenced this pull request Jan 16, 2020
They should depend on `rabbit`, not on `rabbit_common`.

This will be even required #rabbitmq/rabbitmq-server#2212 is accepted.
dumbbell added a commit that referenced this pull request Jan 16, 2020
It should depend on `rabbit`.

This will be even required once ##2212 is
accepted.
dumbbell added a commit that referenced this pull request Jan 17, 2020
Those plugins are created at runtime. We just need to populate the list
of `applications` they depend on. This list must include `rabbit`.

This will be even required once ##2212 is
accepted.
pjk25 pushed a commit that referenced this pull request Jan 23, 2020
It should depend on `rabbit`.

This will be even required once ##2212 is
accepted.

(cherry picked from commit 3cf8b63)
pjk25 pushed a commit that referenced this pull request Jan 23, 2020
Those plugins are created at runtime. We just need to populate the list
of `applications` they depend on. This list must include `rabbit`.

This will be even required once ##2212 is
accepted.

(cherry picked from commit f1b1a1a)
pjk25 added a commit that referenced this pull request Jan 23, 2020
Bundle the commits associated with
#2212 for the purpose
of backporting
pjk25 pushed a commit that referenced this pull request Jan 23, 2020
It should depend on `rabbit`.

This will be even required once ##2212 is
accepted.

(cherry picked from commit 3cf8b63)
pjk25 pushed a commit that referenced this pull request Jan 23, 2020
Those plugins are created at runtime. We just need to populate the list
of `applications` they depend on. This list must include `rabbit`.

This will be even required once ##2212 is
accepted.

(cherry picked from commit f1b1a1a)
pjk25 added a commit that referenced this pull request Jan 23, 2020
Bundle the commits associated with
#2212 for the purpose
of backporting
pjk25 pushed a commit to rabbitmq/rabbitmq-cli that referenced this pull request Jan 23, 2020
They should depend on `rabbit`, not on `rabbit_common`.

This will be even required #rabbitmq/rabbitmq-server#2212 is accepted.

(cherry picked from commit be5f8ac)
pjk25 pushed a commit to rabbitmq/rabbitmq-cli that referenced this pull request Jan 23, 2020
They should depend on `rabbit`, not on `rabbit_common`.

This will be even required #rabbitmq/rabbitmq-server#2212 is accepted.

(cherry picked from commit be5f8ac)
@dumbbell
Copy link
Member Author

Backported to v3.7.x and v3.8.x.

pjk25 pushed a commit to rabbitmq/rabbitmq-monorepo that referenced this pull request Sep 9, 2020
They should depend on `rabbit`, not on `rabbit_common`.

This will be even required #rabbitmq/rabbitmq-server#2212 is accepted.

(cherry picked from commit cccf81d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants