Backport 17203 and 17267 8.x (#17270)

* Pluginmanager clean after mutate (#17203)

* pluginmanager: always clean after mutate

* pluginmanager: don't skip updating plugins installed with --version

* pr feedback

(cherry picked from commit 8c96913)

* Pluginmanager install preserve (#17267)

* tests: integration tests for pluginmanager install --preserve

* fix regression where pluginmanager's install --preserve flag didn't

Author: yaauie

lib/pluginmanager/command.rb

@@ -47,6 +47,31 @@ def remove_unused_locally_installed_gems!
     end
   end
 
+  def remove_orphan_dependencies!
+    locked_gem_names = ::Bundler::LockfileParser.new(File.read(LogStash::Environment::LOCKFILE)).specs.map(&:full_name).to_set
+    orphan_gem_specs = ::Gem::Specification.each
+                                           .reject(&:stubbed?) # skipped stubbed (uninstalled) gems
+                                           .reject(&:default_gem?) # don't touch jruby-included default gems
+                                           .reject{ |spec| locked_gem_names.include?(spec.full_name) }
+                                           .sort
+
+    inactive_plugins, orphaned_dependencies = orphan_gem_specs.partition { |spec| LogStash::PluginManager.logstash_plugin_gem_spec?(spec) }
+
+    # uninstall plugins first, to limit damage should one fail to uninstall
+    inactive_plugins.each { |plugin| uninstall_gem!("inactive plugin", plugin) }
+    orphaned_dependencies.each { |dep| uninstall_gem!("orphaned dependency", dep) }
+  end
+
+  def uninstall_gem!(desc, spec)
+    require "rubygems/uninstaller"
+    Gem::DefaultUserInteraction.use_ui(debug? ? Gem::DefaultUserInteraction.ui : Gem::SilentUI.new) do
+      Gem::Uninstaller.new(spec.name, version: spec.version, force: true, executables: true).uninstall
+    end
+    puts "cleaned #{desc} #{spec.name} (#{spec.version})"
+  rescue Gem::InstallError => e
+    report_exception("Failed to uninstall `#{spec.full_name}`", e)
+  end
+
   def relative_path(path)
     require "pathname"
     ::Pathname.new(path).relative_path_from(::Pathname.new(LogStash::Environment::LOGSTASH_HOME)).to_s

lib/pluginmanager/gemfile.rb

@@ -133,8 +133,8 @@ def add_gem(_gem)
     # update existing or add new
     def update_gem(_gem)
       if old = find_gem(_gem.name)
-        # always overwrite requirements if specified
-        old.requirements = _gem.requirements unless no_constrains?(_gem.requirements)
+        # always overwrite requirements
+        old.requirements = _gem.requirements
         # but merge options
         old.options = old.options.merge(_gem.options)
       else

lib/pluginmanager/install.rb

@@ -79,6 +79,7 @@ def execute
     install_gems_list!(gems)
     remove_unused_locally_installed_gems!
     remove_unused_integration_overlaps!
+    remove_orphan_dependencies!
   end
 
   private
@@ -213,7 +214,9 @@ def install_gems_list!(install_list)
       plugin_gem = gemfile.find(plugin)
       if preserve?
         puts("Preserving Gemfile gem options for plugin #{plugin}") if plugin_gem && !plugin_gem.options.empty?
-        gemfile.update(plugin, version, options)
+        # if the plugin exists and no version was specified, keep the existing requirements
+        requirements = (plugin_gem && version.nil? ? plugin_gem.requirements : [version]).compact
+        gemfile.update(plugin, *requirements, options)
       else
         gemfile.overwrite(plugin, version, options)
       end

lib/pluginmanager/remove.rb

@@ -67,6 +67,7 @@ def execute
     exit(1) unless ::Bundler::LogstashUninstall.uninstall!(plugin_list)
     LogStash::Bundler.genericize_platform
     remove_unused_locally_installed_gems!
+    remove_orphan_dependencies!
   rescue => exception
     report_exception("Operation aborted, cannot remove plugin", exception)
   end

lib/pluginmanager/update.rb

@@ -95,10 +95,8 @@ def update_gems!
       output << LogStash::Bundler.genericize_platform unless output.nil?
     end
 
-    # We currently dont removed unused gems from the logstash installation
-    # see: https://github.com/elastic/logstash/issues/6339
-    # output = LogStash::Bundler.invoke!(:clean => true)
     display_updated_plugins(previous_gem_specs_map)
+    remove_orphan_dependencies!
   rescue => exception
     gemfile.restore!
     report_exception("Updated Aborted", exception)

qa/integration/fixtures/update_spec.yml

@@ -0,0 +1,3 @@
+---
+services:
+  - logstash

qa/integration/services/logstash_service.rb

@@ -308,7 +308,7 @@ def run_cmd(cmd_args, change_dir = true, environment = {})
     if ENV.key?("BUILD_JAVA_HOME") && !process.environment.key?("LS_JAVA_HOME")
       process.environment["LS_JAVA_HOME"] = ENV["BUILD_JAVA_HOME"]
     end
-    process.io.stdout = process.io.stderr = out
+    process.io.stdout = process.io.stderr = SynchronizedDelegate.new(out)
 
     Bundler.with_unbundled_env do
       if change_dir
@@ -329,6 +329,31 @@ def run(*args)
     run_cmd [@logstash_bin, *args]
   end
 
+  ##
+  # A `SynchronizedDelegate` wraps any object and ensures that exactly one
+  # calling thread is invoking methods on it at a time. This is useful for our
+  # clumsy setting of process io STDOUT and STDERR to the same IO object, which
+  # can cause interleaved writes.
+  class SynchronizedDelegate
+    def initialize(obj)
+      require "monitor"
+      @mon = Monitor.new
+      @obj = obj
+    end
+
+    def respond_to_missing?(method_name, include_private = false)
+      @obj.respond_to?(method_name, include_private) || super
+    end
+
+    def method_missing(method_name, *args, &block)
+      return super unless @obj.respond_to?(method_name)
+
+      @mon.synchronize do
+        @obj.public_send(method_name, *args, &block)
+      end
+    end
+  end
+
   class PluginCli
 
     LOGSTASH_PLUGIN = File.join("bin", "logstash-plugin")
@@ -362,9 +387,26 @@ def list(*plugins, verbose: false)
       run(command)
     end
 
-    def install(plugin_name, *additional_plugins)
-      plugin_list = ([plugin_name]+additional_plugins).flatten
-      run("install #{Shellwords.shelljoin(plugin_list)}")
+    def install(plugin_name, *additional_plugins, version: nil, verify: true, preserve: false, local: false)
+      args = []
+      args << "--no-verify" unless verify
+      args << "--preserve" if preserve
+      args << "--local" if local
+      args << "--version" << version unless version.nil?
+      args.concat(([plugin_name]+additional_plugins).flatten)
+
+      run("install #{Shellwords.shelljoin(args)}")
+    end
+
+    def update(*plugin_list, level: nil, local: nil, verify: nil, conservative: nil)
+      args = []
+      args << (verify ? "--verify" : "--no-verify") unless verify.nil?
+      args << "--level" << "#{level}" unless level.nil?
+      args << "--local" if local
+      args << (conservative ? "--conservative" : "--no-conservative") unless conservative.nil?
+      args.concat(plugin_list)
+
+      run("update #{Shellwords.shelljoin(args)}")
     end
 
     def run(command)

qa/integration/specs/cli/install_spec.rb

@@ -19,6 +19,7 @@
 require_relative "../../framework/settings"
 require_relative "../../services/logstash_service"
 require_relative "../../framework/helpers"
+require_relative "pluginmanager_spec_helper"
 require "logstash/devutils/rspec/spec_helper"
 require "stud/temporary"
 require "fileutils"
@@ -29,23 +30,32 @@ def gem_in_lock_file?(pattern, lock_file)
   content.match(pattern)
 end
 
+def plugin_filename_re(name, version)
+  %Q(\b#{Regexp.escape name}-#{Regexp.escape version}(-java)?\b)
+end
+
 # Bundler can mess up installation successful output: https://github.com/elastic/logstash/issues/15801
 INSTALL_SUCCESS_RE = /IB?nstall successful/
 INSTALLATION_SUCCESS_RE = /IB?nstallation successful/
 
+INSTALLATION_ABORTED_RE = /Installation aborted/
+
 describe "CLI > logstash-plugin install" do
-  before(:all) do
+  before(:each) do
     @fixture = Fixture.new(__FILE__)
     @logstash = @fixture.get_service("logstash")
     @logstash_plugin = @logstash.plugin_cli
-    @pack_directory =  File.expand_path(File.join(File.dirname(__FILE__), "..", "..", "fixtures", "logstash-dummy-pack"))
   end
 
   shared_examples "install from a pack" do
     let(:pack) { "file://#{File.join(@pack_directory, "logstash-dummy-pack.zip")}" }
     let(:install_command) { "bin/logstash-plugin install" }
     let(:change_dir) { true }
 
+    before(:all) do
+      @pack_directory =  File.expand_path(File.join(File.dirname(__FILE__), "..", "..", "fixtures", "logstash-dummy-pack"))
+    end
+
     # When you are on anything by linux we won't disable the internet with seccomp
     if RbConfig::CONFIG["host_os"] == "linux"
       context "without internet connection (linux seccomp wrapper)" do
@@ -152,4 +162,147 @@ def gem_in_lock_file?(pattern, lock_file)
       end
     end
   end
+
+  context "rubygems hosted plugin" do
+    include_context "pluginmanager validation helpers"
+    shared_context("install over existing") do
+      before(:each) do
+        aggregate_failures("precheck") do
+          expect("#{plugin_name}-#{existing_plugin_version}").to_not be_installed_gem
+          expect("#{plugin_name}").to_not be_installed_gem
+        end
+        aggregate_failures("setup") do
+          execute = @logstash_plugin.install(plugin_name, version: existing_plugin_version)
+
+          expect(execute.stderr_and_stdout).to match(INSTALLATION_SUCCESS_RE)
+          expect(execute.exit_code).to eq(0)
+
+          expect("#{plugin_name}-#{existing_plugin_version}").to be_installed_gem
+          expect(plugin_name).to be_in_gemfile.with_requirements(existing_plugin_version)
+        end
+      end
+    end
+    shared_examples("overwriting existing with explicit version") do
+      include_context "install over existing"
+      it "installs the specified version and removes the pre-existing one" do
+        execute = @logstash_plugin.install(plugin_name, version: specified_plugin_version)
+
+        aggregate_failures("command execution") do
+          expect(execute.stderr_and_stdout).to match(INSTALLATION_SUCCESS_RE)
+          expect(execute.exit_code).to eq(0)
+        end
+
+        installed = @logstash_plugin.list(plugin_name, verbose: true)
+        expect(installed.stderr_and_stdout).to match(/#{Regexp.escape plugin_name} [(]#{Regexp.escape(specified_plugin_version)}[)]/)
+
+        expect("#{plugin_name}-#{existing_plugin_version}").to_not be_installed_gem
+        expect("#{plugin_name}-#{specified_plugin_version}").to be_installed_gem
+      end
+    end
+
+    context "when installing over an older version using --version" do
+      let(:plugin_name) { "logstash-filter-qatest" }
+      let(:existing_plugin_version) { "0.1.0" }
+      let(:specified_plugin_version) { "0.1.1" }
+
+      include_examples "overwriting existing with explicit version"
+    end
+
+    context "when installing over a newer version using --version" do
+      let(:plugin_name) { "logstash-filter-qatest" }
+      let(:existing_plugin_version) { "0.1.0" }
+      let(:specified_plugin_version) { "0.1.1" }
+
+      include_examples "overwriting existing with explicit version"
+    end
+
+    context "when installing over existing without --version" do
+      let(:plugin_name) { "logstash-filter-qatest" }
+      let(:existing_plugin_version) { "0.1.0" }
+
+      include_context "install over existing"
+
+      context "with --preserve" do
+        it "succeeds without changing the requirements in the Gemfile" do
+          execute = @logstash_plugin.install(plugin_name, preserve: true)
+
+          aggregate_failures("command execution") do
+            expect(execute.stderr_and_stdout).to match(INSTALLATION_SUCCESS_RE)
+            expect(execute.exit_code).to eq(0)
+          end
+
+          installed = @logstash_plugin.list(verbose: true)
+          expect(installed.stderr_and_stdout).to match(/#{Regexp.escape plugin_name}/)
+
+          # we want to ensure that the act of installing an already-installed plugin
+          # does not change its requirements in the gemfile, and leaves the previously-installed
+          # version in-tact.
+          expect(plugin_name).to be_in_gemfile.with_requirements(existing_plugin_version)
+          expect("#{plugin_name}-#{existing_plugin_version}").to be_installed_gem
+        end
+      end
+
+      context "without --preserve" do
+        # this spec is OBSERVED behaviour, which I believe to be undesirable.
+        it "succeeds and removes the version requirement from the Gemfile" do
+          execute = @logstash_plugin.install(plugin_name)
+
+          aggregate_failures("command execution") do
+            expect(execute.stderr_and_stdout).to match(INSTALLATION_SUCCESS_RE)
+            expect(execute.exit_code).to eq(0)
+          end
+
+          installed = @logstash_plugin.list(plugin_name, verbose: true)
+          expect(installed.stderr_and_stdout).to match(/#{Regexp.escape plugin_name}/)
+
+          # This is the potentially-undesirable surprising behaviour, specified here
+          # as a means of documentation, not a promise of future behavior.
+          expect(plugin_name).to be_in_gemfile.without_requirements
+
+          # we expect _a_ version of the plugin to be installed, but cannot be opinionated
+          # about which version was installed because bundler won't necessarily re-resolve
+          # the dependency graph to get us an upgrade since the no-requirements dependency
+          # is still met (but it MAY do so if also installing plugins that are not present).
+          expect("#{plugin_name}").to be_installed_gem
+        end
+      end
+    end
+
+    context "installing plugin that isn't present" do
+      it "installs the plugin" do
+        aggregate_failures("prevalidation") do
+          expect("logstash-filter-qatest").to_not be_installed_gem
+        end
+
+        execute = @logstash_plugin.install("logstash-filter-qatest")
+
+        expect(execute.stderr_and_stdout).to match(INSTALLATION_SUCCESS_RE)
+        expect(execute.exit_code).to eq(0)
+
+        installed = @logstash_plugin.list("logstash-filter-qatest")
+        expect(installed.stderr_and_stdout).to match(/logstash-filter-qatest/)
+        expect(installed.exit_code).to eq(0)
+
+        expect(gem_in_lock_file?(/logstash-filter-qatest/, @logstash.lock_file)).to be_truthy
+
+        expect("logstash-filter-qatest").to be_installed_gem
+      end
+    end
+    context "installing plugin that doesn't exist on rubygems" do
+      it "doesn't install anything" do
+        execute = @logstash_plugin.install("logstash-filter-404-no-exist")
+
+        expect(execute.stderr_and_stdout).to match(INSTALLATION_ABORTED_RE)
+        expect(execute.exit_code).to eq(1)
+      end
+    end
+    context "installing gem that isn't a plugin" do
+      it "doesn't install anything" do
+        execute = @logstash_plugin.install("dummy_gem")
+
+        expect(execute.stderr_and_stdout).to match(INSTALLATION_ABORTED_RE)
+        expect(execute.exit_code).to eq(1)
+      end
+    end
+  end
 end