Fix rabbitmq_plugin to correctly detect implicitly enabled plugins

[nmaludy]

Pull Request (PR) description

Previously, rabbitmq_plugin was retrieving a list of existing plugins by querying for "explicitly" enabled plugins using the rabbitmq-plugin -E command. This caused us issues when adding support for CentOS/RHEL8 where we noticed that in this mode, ordering of plugins matters.

Example of the scenario we saw:

In this spec test: https:/voxpupuli/puppet-rabbitmq/blob/master/spec/acceptance/parameter_spec.rb#L18

The following plugins are declared:

rabbitmq_plugin { [ 'rabbitmq_federation_management', 'rabbitmq_federation' ]:
  ensure => present
}

The rabbitmq_federation_management plugin depends on rabbitmq_federation, so when our code enables rabbitmq_federation_management it will automatically enable rabbitmq_federation, but mark it as being "implicitly" enabled because it was enabled as a result of enabling rabbitmq_federation_management.

Then, when go to query for the list of enabled plugins using the -E option, this only lists our "explicitly" enabled plugins, so rabbitmq_federation is never listed and every time we run Puppet it attempts to enable the plugin, breaking idempotency.

The fix

The rabbitmq-plugin command has two options -E to list only "explicitly" enabled plugins and -e to list both "explicitly" and "implicitly" enabled plugins. The fix was simply to switch to this other flag. Along the way i refactored the code to reduce duplication and added lots of unit tests for more test coverage.

References

For more info you can see the CentOS/RHEL 8 PR where this was discovered: #842

[wyardley]

This seems to exist in at least version 3.6 (3.6.6 is what the integration tests run). The README mentions being tested against 3.5 - do we know if both flags exist there as well?

I'm Ok with this, but are there any of these that are simply builtin to rabbitmq and should be ignored? If someone tried to ensure absent on a required or implicitly enabled plugin resource, do we know what would happen?

For example, before, a plugin like amqp_client wouldn't show up, and thus wouldn't be able to be removed. however, it seems like letting someone remove some of these builtin or implicitly enabled plugins might be dangerous?

I haven't worked with RMQ in a while, and just played with this briefly in the integration test Docker container locally, but:

root@debian9-64-1:/# cat /tmp/manifest.pp 

rabbitmq_plugin { [ 'amqp_client' ]:
	ensure => absent,
}

root@debian9-64-1:/#  /opt/puppetlabs/puppet/bin/ruby /opt/puppetlabs/puppet/bin/puppet apply /tmp/manifest.pp 
Notice: Compiled catalog for debian9-64-1 in environment production in 0.03 seconds
Notice: /Stage[main]/Main/Rabbitmq_plugin[amqp_client]/ensure: removed
Notice: Applied catalog in 5.48 seconds
root@debian9-64-1:/# rabbitmq-plugins list -e -M
 Configured: E = explicitly enabled; e = implicitly enabled
 | Status:   * = running on rabbit@debian9-64-1
 |/
root@debian9-64-1:/# rabbitmq-plugins list -E -M
 Configured: E = explicitly enabled; e = implicitly enabled
 | Status:   * = running on rabbit@debian9-64-1
 |/

even after re-ensuring present, all the plugins seem to be gone.

[wyardley]

Would it be too "dumb" of a fix to simply change the integration test to use two plugins that don't have a dependency on each other?

just based on what I've seen, I wonder if this is really not a "bug", but a bad test?

[ekohl]

I don't know RabbitMQ (been years since I used it) so I'm not sure how useful my feedback is.

From a code perspective it looks good and I like the plugin_list helper. That in itself is a nice improvement.

The logic of listing all plugins also makes sense but I'd consider a set up where someone would purge all unmanaged plugins. That will have idempotency issues after this if they don't list all plugins explicitly. If a dependency is added, it requires a change to the manifest where it currently doesn't.

[wyardley]

@nmaludy thoughts / feedback on these comments?
Thus far, I'm thinking that changing to a different test case might make more sense?

[russellshackleford]

While ugly parsing and editing would be required, there is a way to switch from implicit to explicit by adding the plugin to /etc/rabbitmq/enabled_plugins. I ran into this when rabbitmq_shovel_management was enabled prior to rabbitmq_management. If -E doesn't list the plugin in question, then if the provider both enabled it and wrote to /etc/rabbitmq/enabled_plugins, it should be idempotent. In this way, perhaps it is safer when ensuring the plugin is absent?

[igalic]

@russellshackleford, that sounds like a viable alternative to the current approach

is there a way to get out of what we're doing right now, and towards what you're suggesting, with minimal (or rather: no) breakage of people's configs?

[russellshackleford]

What we are doing right now should continue and I do not see a scenario where doing that plus starting to write to /etc/rabbitmq/enabled_plugins to mark plugins as explicit should be a problem. Here is a test I ran on 3.8.2 to try to confirm behavior (starting with no plugins enabled):

1. sudo rabbitmq-plugins enable rabbitmq_shovel_management
   adds rabbitmq_shovel_management, rabbitmq_shovel, rabbitmq_management, rabbitmq_management_agent, rabbitmq_web_dispatch
2. sudo rabbitmq-plugins disable rabbitmq_shovel
   removes everything added in step 1
3. sudo rabbitmq-plugins enable rabbitmq_shovel_management and then add rabbitmq_management to /etc/rabbitmq/enabled_plugins
   adds the same things as step 1 but now rabbitmq_management is marked as explicit
4. sudo rabbitmq-plugins disable rabbitmq_shovel
   removes only rabbitmq_shovel_management and rabbitmq_shovel

As an additional test (starting with no plugins enabled), I enabled rabbitmq_federation_management and rabbitmq_shovel_management (without editing /etc/rabbitmq/enabled_plugins) and tested removing each separately. As long as either was still enabled, rabbitmq_management and its deps were still enabled. Once I removed both, rabbitmq_management and its deps were removed as well. So at least on 3.8.2, rabbit has smart dependency management.

We could get into all manner of corner cases if someone is manually enabling plugins outside of puppet, but I don't think we need to concern ourselves with that workflow. The hassle is managing /etc/rabbitmq/enabled_plugins

[wyardley]

May be worth seeing if one of the rabbitmq devs could weigh in, but if I’m understanding the proposed solution, I’m good with it. Ideally, any PR implementing it would add some additional integration test coverage for any new scenarios we’ve uncovered here.

I’m curious, though if this is an issue affecting users of the module, or, again, maybe just a badly designed test case. In the latter case, I’m happy to adjust that, even if it’s just a stopgap.

[russellshackleford]

While the test case certainly could reverse the order in which the two plugins were installed, it seems that the module could be made more robust and more semantically correct in that if you enable a plugin in puppet, it was an explicit choice. One could argue how much underlying knowledge of plugins and their deps should be required, though, and could discount this as PEBKAC for not knowing that shovel must come before shovel_management or simply be removed as it will get pulled in automatically.

[russellshackleford]

I’m curious, though if this is an issue affecting users of the module, or, again, maybe just a badly designed test case. In the latter case, I’m happy to adjust that, even if it’s just a stopgap.

The former. That is what brought me to this PR. :)

[wyardley]

With the test case specifically, IMO, it’s a bad test case because one plugin being installed is also a dependency of the other. So either way, the test should include at least one plugin that is not a dependency of any others

[igalic]

rabbit has smart dependency management.

but our mapping of its plugin system doesn't seem to cover dependencies, or am i misinterpreting something here?

[russellshackleford]

IMO the test case mimics what can and will happen IRL. That is, they might declare two (or more) plugins where latter plugins are deps of former ones. We could say "they should have known better" or we could make explicit decisions, made by the user, explicit in rabbit helping ensure puppet runs are idempotent.

[wyardley]

I think I'm understanding the issue a little better. I know it's 3+ years later, but I'm cherry-picking this into #928 (with original attribution to @nmaludy)

I'm still a little concerned we could run into corner cases with people disabling implicitly enabled plugins etc, but I think we just need to move forward, and people can propose better fixes if that kind of corner case comes up

In addition to the conflicts, had to also switch to lines.select to lines.grep to satisfy the current robocop rules.