Merge pull request #920 from tylerjl/plugin-provider-refactor-exists

Refactor `exists` logic for plugin provider

Author: tylerjl

CHANGELOG.md

@@ -3,6 +3,7 @@
 #### Features
 
 #### Fixes
+* Rewrote the `exists?` logic for the `elasticsearch_plugin` provider. This fundamentally changes how the module detects the presence of plugins but should be backwards compatible.
 
 ## 6.2.0 (February 9, 2018)
 

lib/puppet/provider/elastic_plugin.rb

@@ -21,66 +21,44 @@ def homedir
   end
 
   def exists?
-    if !File.exists?(pluginfile)
-      debug "Plugin file #{pluginfile} does not exist"
-      return false
-    elsif File.exists?(pluginfile) && readpluginfile != pluginfile_content
-      debug "Got #{readpluginfile} Expected #{pluginfile_content}. Removing for reinstall"
-      self.destroy
-      return false
-    else
-      debug "Plugin exists"
-      return true
-    end
-  end
-
-  # Returns the content that should be written to the pluginfile.
-  #
-  # @return String
-  def pluginfile_content
-    return @resource[:name] if is1x?
-
-    if @resource[:name].split("/").count == 1 # Official plugin
-      version = plugin_version(@resource[:name])
-      return "#{@resource[:name]}/#{version}"
-    else
-      return @resource[:name]
-    end
-  end
+    # First, attempt to list whether the named plugin exists by finding a
+    # plugin descriptor file, which each plugin should have. We must wildcard
+    # the name to match meta plugins, see upstream issue for this change:
+    # https://github.com/elastic/elasticsearch/pull/28022
+    properties = Dir[File.join(@resource[:plugin_dir], plugin_path, '*plugin-descriptor.properties')]
+    return false if properties.empty?
 
-  # Get the path for the `.name` file for the provider helper.
-  #
-  # @return String
-  #   path for the pluginfile
-  def pluginfile
-    if @resource[:plugin_path]
-      File.join(
-        @resource[:plugin_dir],
-        @resource[:plugin_path],
-        '.name'
-      )
-    else
-      File.join(
-        @resource[:plugin_dir],
-        Puppet_X::Elastic::plugin_name(@resource[:name]),
-        '.name'
-      )
+    begin
+      # Use the basic name format that the plugin tool supports in order to
+      # determine the version from the resource name.
+      plugin_version = Puppet_X::Elastic.plugin_version(@resource[:name])
+
+      # Naively parse the Java .properties file to check version equality.
+      # Because we don't have the luxury of installing arbitrary gems, perform
+      # simple parse with a degree of safety checking in the call chain
+      installed_version = IO.readlines(properties.first).map(&:strip).reject do |line|
+        line.start_with?('#') or line.empty?
+      end.map do |property|
+        property.split('=')
+      end.reject do |pairs|
+        pairs.length != 2
+      end.to_h['version']
+
+      if installed_version != plugin_version
+        debug "Elasticsearch plugin #{@resource[:name]} not version #{plugin_version}, reinstalling"
+        destroy
+        return false
+      end
+    rescue ElasticPluginParseFailure
+      # If there is no version string, we do not check version equality
+      debug "No version found in #{@resource[:name]}, not enforcing any version"
     end
-  end
 
-  # Write plugfile file `.name` contents to disk.
-  def writepluginfile
-    File.open(pluginfile, 'w') do |file|
-      file.write pluginfile_content
-    end
+    true
   end
 
-  # Get pluginfile contents.
-  #
-  # @return String
-  def readpluginfile
-    f = File.open(pluginfile)
-    f.readline
+  def plugin_path
+    @resource[:plugin_path] || Puppet_X::Elastic.plugin_name(@resource[:name])
   end
 
   # Intelligently returns the correct installation arguments for version 1
@@ -91,20 +69,18 @@ def readpluginfile
   def install1x
     if !@resource[:url].nil?
       [
-        Puppet_X::Elastic::plugin_name(@resource[:name]),
+        Puppet_X::Elastic.plugin_name(@resource[:name]),
         '--url',
         @resource[:url]
       ]
     elsif !@resource[:source].nil?
       [
-        Puppet_X::Elastic::plugin_name(@resource[:name]),
+        Puppet_X::Elastic.plugin_name(@resource[:name]),
         '--url',
         "file://#{@resource[:source]}"
       ]
     else
-      [
-        @resource[:name]
-      ]
+      [@resource[:name]]
     end
   end
 
@@ -115,17 +91,11 @@ def install1x
   #   arguments to pass to the plugin installation utility
   def install2x
     if !@resource[:url].nil?
-      [
-        @resource[:url]
-      ]
+      [@resource[:url]]
     elsif !@resource[:source].nil?
-      [
-        "file://#{@resource[:source]}"
-      ]
+      ["file://#{@resource[:source]}"]
     else
-      [
-        @resource[:name]
-      ]
+      [@resource[:name]]
     end
   end
 
@@ -134,14 +104,12 @@ def install2x
   #
   # @return Array
   #   of flags for command-line tools
-  def proxy_args url
+  def proxy_args(url)
     parsed = URI(url)
-    ['http', 'https'].map do |schema|
-      [:host, :port, :user, :password].map do |param|
+    %w[http https].map do |schema|
+      %i[host port user password].map do |param|
         option = parsed.send(param)
-        if not option.nil?
-          "-D#{schema}.proxy#{param.to_s.capitalize}=#{option}"
-        end
+        "-D#{schema}.proxy#{param.to_s.capitalize}=#{option}" unless option.nil?
       end
     end.flatten.compact
   end
@@ -168,14 +136,12 @@ def create
       retry if retry_times < retry_count
       raise "Failed to install plugin. Received error: #{e.inspect}"
     end
-
-    writepluginfile
   end
 
   # Remove this plugin from the host.
   def destroy
     with_environment do
-      plugin(['remove', Puppet_X::Elastic::plugin_name(@resource[:name])])
+      plugin(['remove', Puppet_X::Elastic.plugin_name(@resource[:name])])
     end
   end
 
@@ -191,26 +157,19 @@ def is1x?
   end
 
   def is2x?
-    (Puppet::Util::Package.versioncmp(es_version, '2.0.0') >= 0) && (Puppet::Util::Package.versioncmp(es_version, '3.0.0') < 0)
+    (Puppet::Util::Package.versioncmp(es_version, '2.0.0') >= 0) && \
+      (Puppet::Util::Package.versioncmp(es_version, '3.0.0') < 0)
   end
 
   def batch_capable?
     Puppet::Util::Package.versioncmp(es_version, '2.2.0') >= 0
   end
 
-  # Determine the plugin version.
-  def plugin_version(plugin_name)
-    _vendor, _plugin, version = plugin_name.split('/')
-    return es_version if is2x? && version.nil?
-    return version.scan(/\d+\.\d+\.\d+(?:\-\S+)?/).first unless version.nil?
-    false
-  end
-
   # Run a command wrapped in necessary env vars
   def with_environment(&block)
     env_vars = {
       'ES_JAVA_OPTS' => @resource[:java_opts],
-      'ES_PATH_CONF' => @resource[:configdir],
+      'ES_PATH_CONF' => @resource[:configdir]
     }
     saved_vars = {}
 

lib/puppet_x/elastic/plugin_parsing.rb

@@ -1,19 +1,31 @@
+class ElasticPluginParseFailure < StandardError; end
+
 module Puppet_X
   module Elastic
     def self.plugin_name(raw_name)
       plugin_split(raw_name, 1)
     end
 
+    def self.plugin_version(raw_name)
+      v = plugin_split(raw_name, 2, false).gsub(/^[^0-9]*/, '')
+      raise ElasticPluginParseFailure, "could not parse version, got '#{v}'" if v.empty?
+      v
+    end
+
     # Attempt to guess at the plugin's final directory name
-    def self.plugin_split(original_string, position)
+    def self.plugin_split(original_string, position, soft_fail = true)
       # Try both colon (maven) and slash-delimited (github/elastic.co) names
       %w[/ :].each do |delimiter|
         parts = original_string.split(delimiter)
         # If the string successfully split, assume we found the right format
         return parts[position].gsub(/(elasticsearch-|es-)/, '') unless parts[position].nil?
       end
 
-      # Fallback to the originally passed plugin name
+      raise(
+        ElasticPluginParseFailure,
+        "could not find element '#{position}' in #{original_string}"
+      ) unless soft_fail
+
       original_string
     end
   end # of Elastic

spec/unit/provider/elasticsearch_plugin/shared_examples.rb

@@ -155,7 +155,7 @@
       let(:resource_name) { 'appbaseio/dejaVu' }
 
       it 'maintains mixed-case names' do
-        expect(provider.pluginfile).to include('dejaVu')
+        expect(provider.plugin_path).to include('dejaVu')
       end
     end