From e8a9375b1a1e57fe50be4920ee4620d52ff1d90a Mon Sep 17 00:00:00 2001 From: Tyler Langlois Date: Mon, 12 Feb 2018 15:38:23 -0700 Subject: [PATCH 1/4] rewrite plugin provider for more robust version/presence checking --- lib/puppet/provider/elastic_plugin.rb | 137 +++++++++---------------- lib/puppet_x/elastic/plugin_parsing.rb | 14 ++- 2 files changed, 60 insertions(+), 91 deletions(-) diff --git a/lib/puppet/provider/elastic_plugin.rb b/lib/puppet/provider/elastic_plugin.rb index f4f2bb050..234fec153 100644 --- a/lib/puppet/provider/elastic_plugin.rb +++ b/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 + plugin_path = @resource[:plugin_path] || Puppet_X::Elastic.plugin_name(@resource[:name]) - # 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' - ) - 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? - # Write plugfile file `.name` contents to disk. - def writepluginfile - File.open(pluginfile, 'w') do |file| - file.write pluginfile_content + 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] + ).gsub(/^[^0-9]*/, '') + + # 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 - # Get pluginfile contents. - # - # @return String - def readpluginfile - f = File.open(pluginfile) - f.readline + true 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 = {} diff --git a/lib/puppet_x/elastic/plugin_parsing.rb b/lib/puppet_x/elastic/plugin_parsing.rb index 9f66cdfcd..65649b1bd 100644 --- a/lib/puppet_x/elastic/plugin_parsing.rb +++ b/lib/puppet_x/elastic/plugin_parsing.rb @@ -1,11 +1,17 @@ +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) + plugin_split(raw_name, 2, false) + 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) @@ -13,7 +19,11 @@ def self.plugin_split(original_string, position) 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 From 878f13b92651938f80348b3d5686e01f641cd71e Mon Sep 17 00:00:00 2001 From: Tyler Langlois Date: Mon, 12 Feb 2018 15:39:42 -0700 Subject: [PATCH 2/4] docs for plugin provider `exists?` rewrite --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cdb7007c8..0b4ff4d3a 100644 --- a/CHANGELOG.md +++ b/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) From a2d2c0f17074e46416c38b77b31f144921432067 Mon Sep 17 00:00:00 2001 From: Tyler Langlois Date: Mon, 12 Feb 2018 15:59:39 -0700 Subject: [PATCH 3/4] testing: fix plugin name munging provider spec tests --- lib/puppet/provider/elastic_plugin.rb | 6 ++++-- spec/unit/provider/elasticsearch_plugin/shared_examples.rb | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/puppet/provider/elastic_plugin.rb b/lib/puppet/provider/elastic_plugin.rb index 234fec153..dbf488773 100644 --- a/lib/puppet/provider/elastic_plugin.rb +++ b/lib/puppet/provider/elastic_plugin.rb @@ -21,8 +21,6 @@ def homedir end def exists? - plugin_path = @resource[:plugin_path] || Puppet_X::Elastic.plugin_name(@resource[:name]) - # 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: @@ -61,6 +59,10 @@ def exists? true end + def plugin_path + @resource[:plugin_path] || Puppet_X::Elastic.plugin_name(@resource[:name]) + end + # Intelligently returns the correct installation arguments for version 1 # version of Elasticsearch. # diff --git a/spec/unit/provider/elasticsearch_plugin/shared_examples.rb b/spec/unit/provider/elasticsearch_plugin/shared_examples.rb index fbb7156b9..250ba0fcf 100644 --- a/spec/unit/provider/elasticsearch_plugin/shared_examples.rb +++ b/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 From 884235042e4ea71e047d7d1009d99804c203426f Mon Sep 17 00:00:00 2001 From: Tyler Langlois Date: Tue, 13 Feb 2018 09:49:50 -0700 Subject: [PATCH 4/4] Bypass plugin version exists? comparison for non-numeric versions --- lib/puppet/provider/elastic_plugin.rb | 4 +--- lib/puppet_x/elastic/plugin_parsing.rb | 4 +++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/puppet/provider/elastic_plugin.rb b/lib/puppet/provider/elastic_plugin.rb index dbf488773..77105566e 100644 --- a/lib/puppet/provider/elastic_plugin.rb +++ b/lib/puppet/provider/elastic_plugin.rb @@ -31,9 +31,7 @@ def exists? 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] - ).gsub(/^[^0-9]*/, '') + 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 diff --git a/lib/puppet_x/elastic/plugin_parsing.rb b/lib/puppet_x/elastic/plugin_parsing.rb index 65649b1bd..7a1d05725 100644 --- a/lib/puppet_x/elastic/plugin_parsing.rb +++ b/lib/puppet_x/elastic/plugin_parsing.rb @@ -7,7 +7,9 @@ def self.plugin_name(raw_name) end def self.plugin_version(raw_name) - plugin_split(raw_name, 2, false) + 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