From 908e80be704df4aeabd84cd755b279d6aa72b313 Mon Sep 17 00:00:00 2001
From: Nick Maludy <nmaludy@gmail.com>
Date: Tue, 30 Jun 2020 20:07:16 -0400
Subject: [PATCH] Fix rabbitmq_plugin to correctly detect implicitly enabled
 plugins

Rabbitmq_plugin now correctly detects implicitly enabled plugins to
preserve idempotency regardless of plugin install order

Cherry-picked from #844
Fixes #930

Signed-off-by: William Yardley <wyardley@users.noreply.github.com>
---
 .../rabbitmq_plugin/rabbitmqplugins.rb        | 29 +++++++--
 .../rabbitmq_plugin/rabbitmqctl_spec.rb       | 63 +++++++++++++++----
 2 files changed, 75 insertions(+), 17 deletions(-)

diff --git a/lib/puppet/provider/rabbitmq_plugin/rabbitmqplugins.rb b/lib/puppet/provider/rabbitmq_plugin/rabbitmqplugins.rb
index 9f3ed82ec..0653a27fd 100644
--- a/lib/puppet/provider/rabbitmq_plugin/rabbitmqplugins.rb
+++ b/lib/puppet/provider/rabbitmq_plugin/rabbitmqplugins.rb
@@ -6,13 +6,30 @@
 Puppet::Type.type(:rabbitmq_plugin).provide(:rabbitmqplugins, parent: Puppet::Provider::RabbitmqCli) do
   confine feature: :posix
 
-  def self.instances
-    plugin_list = run_with_retries do
-      rabbitmqplugins('list', '-E', '-m')
+  def self.plugin_list
+    list_str = run_with_retries do
+      # Pass in -e to list both implicitly and explicitly enabled plugins.
+      # If you pass in -E instead, then only explicitly enabled plugins are listed.
+      # Implicitly enabled plugins are those that were enabled as a dependency of another plugin/
+      # If we do not pass in -e then the order if plugin installation matters within the puppet
+      # code. Example, if Plugin A depends on Plugin B and we install Plugin B first it will
+      # implicitly enable Plugin A. Then when we go to run Puppet a second time without the
+      # -e parameter, we won't see Plugin A as being enabled so we'll try to install it again.
+      # To preserve idempotency we should get all enabled plugins regardless of implicitly or
+      # explicitly enabled.
+      rabbitmqplugins('list', '-e', '-m')
     end
+    # Split by newline.
+    lines = list_str.split(%r{\n})
+    # Return only lines that are single words because sometimes RabbitMQ likes to output
+    # information messages. Suppressing those messages via CLI flags is inconsistent between
+    # versions, so this this regex removes those message without having to use painful
+    # version switches.
+    lines.grep(%r{^(\S+)$})
+  end
 
-    plugin_list.split(%r{\n}).map do |line|
-      next if line.start_with?('Listing plugins')
+  def self.instances
+    plugin_list.map do |line|
       raise Puppet::Error, "Cannot parse invalid plugins line: #{line}" unless line =~ %r{^(\S+)$}
 
       new(name: Regexp.last_match(1))
@@ -36,6 +53,6 @@ def destroy
   end
 
   def exists?
-    run_with_retries { rabbitmqplugins('list', '-E', '-m') }.split(%r{\n}).include? resource[:name]
+    self.class.plugin_list.include? resource[:name]
   end
 end
diff --git a/spec/unit/puppet/provider/rabbitmq_plugin/rabbitmqctl_spec.rb b/spec/unit/puppet/provider/rabbitmq_plugin/rabbitmqctl_spec.rb
index a51bef9b8..79e2a9ba0 100644
--- a/spec/unit/puppet/provider/rabbitmq_plugin/rabbitmqctl_spec.rb
+++ b/spec/unit/puppet/provider/rabbitmq_plugin/rabbitmqctl_spec.rb
@@ -11,25 +11,66 @@
   end
   let(:provider) { provider_class.new(resource) }
 
-  it 'matches plugins' do
-    provider.expects(:rabbitmqplugins).with('list', '-E', '-m').returns("foo\n")
-    expect(provider.exists?).to eq(true)
-  end
-
   it 'calls rabbitmqplugins to enable when node not running' do
     provider.class.expects(:rabbitmq_running).returns false
     provider.expects(:rabbitmqplugins).with('enable', 'foo')
     provider.create
   end
 
-  context 'with RabbitMQ version >=3.10.0' do
-    it 'matches plugins' do
-      provider.expects(:rabbitmqplugins).with('list', '-E', '-m').returns <<~EOT
-        Listing plugins with pattern ".*" ...
-        foo
-      EOT
+  describe '#instances' do
+    it 'exists' do
+      expect(provider_class).to respond_to :instances
+    end
+
+    it 'retrieves instances' do
+      provider.class.expects(:plugin_list).returns(%w[foo bar])
+      instances = provider_class.instances
+      instances_cmp = instances.map { |prov| { name: prov.get(:name) } }
+      expect(instances_cmp).to eq(
+        [
+          { name: 'foo' },
+          { name: 'bar' }
+        ]
+      )
+    end
+
+    it 'raises error on invalid line' do
+      provider_class.expects(:plugin_list).returns(['  '])
+      expect { provider_class.instances }.to raise_error Puppet::Error, %r{Cannot parse invalid plugins line}
+    end
+  end
+
+  describe '#plugin_list' do
+    it 'exists' do
+      expect(provider_class).to respond_to :instances
+    end
+
+    it 'returns a list of plugins' do
+      provider.class.expects(:rabbitmqplugins).with('list', '-e', '-m').returns("foo\nbar\nbaz\n")
+      expect(provider.class.plugin_list).to eq(%w[foo bar baz])
+    end
+
+    it 'handles no training newline properly' do
+      provider.class.expects(:rabbitmqplugins).with('list', '-e', '-m').returns("foo\nbar")
+      expect(provider.class.plugin_list).to eq(%w[foo bar])
+    end
+
+    it 'handles lines that are not plugins' do
+      provider.class.expects(:rabbitmqplugins).with('list', '-e', '-m').returns("Listing plugins with pattern \".*\" ...\nfoo\nbar")
+      expect(provider.class.plugin_list).to eq(%w[foo bar])
+    end
+  end
+
+  describe '#exists?' do
+    it 'matches existng plugins' do
+      provider_class.expects(:plugin_list).returns(%w[foo])
       expect(provider.exists?).to eq(true)
     end
+
+    it 'returns false for missing plugins' do
+      provider_class.expects(:plugin_list).returns(%w[bar])
+      expect(provider.exists?).to eq(false)
+    end
   end
 
   context 'with RabbitMQ version >=3.4.0' do