Fixes to package upgrade behaviour

This commit introduces fixes to the behaviour of the various providers of the "package" resource (and subtypes like dnf_package etc) when a more recent version of the package has been installed than that present in an available repository or file source and "action :upgrade" is called against the package.

Currently, a number of package providers will attempt to downgrade a package under this scenario, where the desired action should be to recognise that a more up to date version is already installed and do nothing.

I've introduced a ```version_compare``` method to the package superclass which by default uses ```Gem::Version``` comparison with spaceship-operator semantics to ensure that an upgrade is attempted only when the candidate version is not older than the current version.

Additionally, this commit introduces a method called ```version_equals?``` which is functionally identical to ```target_version_already_installed?``` (and that method now in fact calls ```version_equals?```. This has been done to clarify the purpose and functionality of that method, but since it's a public API, deprecation of the old method name will have to be done more gradually.

Signed-off-by: Jon Cowie <jcowie@chef.io>

8 files changed, 86 insertions, 25 deletions

diff --git a/lib/chef/provider/package.rb b/lib/chef/provider/package.rb
index 4810728524..226189d978 100644
--- a/lib/chef/provider/package.rb
+++ b/lib/chef/provider/package.rb
@@ -323,10 +323,38 @@ class Chef
       #
       # Note that most likely we need a spaceship operator on versions that subclasses can implement
       # and we should have `version_compare(v1, v2)` that returns `v1 <=> v2`.
+
+      # This method performs a strict equality check between two strings representing version numbers
       #
+      # This function will eventually be deprecated in favour of the below version_equals function.
+
       def target_version_already_installed?(current_version, target_version)
-        return false unless current_version && target_version
-        current_version == target_version
+        version_equals?(current_version, target_version)
+      end
+
+      # Note that most likely we need a spaceship operator on versions that subclasses can implement
+      # and we should have `version_compare(v1, v2)` that returns `v1 <=> v2`.
+
+      # This method performs a strict equality check between two strings representing version numbers
+      #
+      def version_equals?(v1, v2)
+        return false unless v1 && v2
+        v1 == v2
+      end
+
+      # This function compares two version numbers and returns 'spaceship operator' style results, ie:
+      # if v1 < v2 then return -1
+      # if v1 = v2 then return  0
+      # if v1 > v2 then return  1
+      # if v1 and v2 are not comparable then return nil
+      #
+      # By default, this function will use Gem::Version comparison. Subclasses can reimplement this method
+      # for package-management system specific versions.
+      def version_compare(v1, v2)
+        gem_v1 = Gem::Version.new(v1)
+        gem_v2 = Gem::Version.new(v2)
+
+        gem_v1 <=> gem_v2
       end
 
       # Check the current_version against the new_resource.version, possibly using fuzzy
@@ -337,7 +365,7 @@ class Chef
       # `version_satisfied_by?(version, constraint)` might be a better name to make this generic.
       #
       def version_requirement_satisfied?(current_version, new_version)
-        target_version_already_installed?(current_version, new_version)
+        version_equals?(current_version, new_version)
       end
 
       # @todo: extract apt/dpkg specific preseeding to a helper class
@@ -439,16 +467,19 @@ class Chef
             each_package do |package_name, new_version, current_version, candidate_version|
               case action
               when :upgrade
-                if target_version_already_installed?(current_version, new_version)
+                if version_equals?(current_version, new_version)
                   # this is an odd use case
                   Chef::Log.debug("#{new_resource} #{package_name} #{new_version} is already installed -- you are equality pinning with an :upgrade action, this may be deprecated in the future")
                   target_version_array.push(nil)
-                elsif target_version_already_installed?(current_version, candidate_version)
+                elsif version_equals?(current_version, candidate_version)
                   Chef::Log.debug("#{new_resource} #{package_name} #{candidate_version} is already installed")
                   target_version_array.push(nil)
                 elsif candidate_version.nil?
                   Chef::Log.debug("#{new_resource} #{package_name} has no candidate_version to upgrade to")
                   target_version_array.push(nil)
+                elsif version_compare(current_version, candidate_version) == 1 && !new_resource.allow_downgrade
+                  Chef::Log.debug("#{new_resource} #{package_name} has installed version #{current_version}, which is newer than available version #{candidate_version}. Skipping...)")
+                  target_version_array.push(nil)
                 else
                   Chef::Log.debug("#{new_resource} #{package_name} is out of date, will upgrade to #{candidate_version}")
                   target_version_array.push(candidate_version)
diff --git a/lib/chef/provider/package/apt.rb b/lib/chef/provider/package/apt.rb
index 3f8c34f50c..da86016621 100644
--- a/lib/chef/provider/package/apt.rb
+++ b/lib/chef/provider/package/apt.rb
@@ -127,6 +127,13 @@ class Chef
 
         private
 
+        def version_compare(v1, v2)
+          gem_v1 =  v1.gsub(/[_+]/, "+" => "-", "_" => "-") unless v1.nil?
+          gem_v2 =  v2.gsub(/[_+]/, "+" => "-", "_" => "-") unless v2.nil?
+
+          Gem::Version.new(gem_v1) <=> Gem::Version.new(gem_v2)
+        end
+
         # Runs command via shell_out with magic environment to disable
         # interactive prompts. Command is run with default localization rather
         # than forcing locale to "C", so command output may not be stable.
diff --git a/lib/chef/provider/package/dnf.rb b/lib/chef/provider/package/dnf.rb
index 31279e8312..a602a9b768 100644
--- a/lib/chef/provider/package/dnf.rb
+++ b/lib/chef/provider/package/dnf.rb
@@ -126,6 +126,10 @@ class Chef
           end
         end
 
+        def version_compare(v1, v2)
+          python_helper.compare_versions(v1, v2)
+        end
+
         # @returns Array<Version>
         def available_version(index)
           @available_version ||= []
diff --git a/lib/chef/provider/package/dnf/dnf_helper.py b/lib/chef/provider/package/dnf/dnf_helper.py
index eb4d238f65..501d6fceee 100644
--- a/lib/chef/provider/package/dnf/dnf_helper.py
+++ b/lib/chef/provider/package/dnf/dnf_helper.py
@@ -26,6 +26,14 @@ def flushcache():
         pass
     get_sack().load_system_repo(build_cache=True)
 
+def versioncompare(versions):
+    sack = get_sack()
+    if (versions[0] is None) or (versions[1] is None):
+      sys.stdout.write('0\n')
+    else:
+      evr_comparison = sack.evr_cmp(versions[0], versions[1])
+      sys.stdout.write('{}\n'.format(evr_comparison))
+
 def query(command):
     sack = get_sack()
 
@@ -86,5 +94,7 @@ while 1:
         query(command)
     elif command['action'] == "flushcache":
         flushcache()
+    elif command['action'] == "versioncompare":
+        versioncompare(command['versions'])
     else:
         raise RuntimeError("bad command")
diff --git a/lib/chef/provider/package/dnf/python_helper.rb b/lib/chef/provider/package/dnf/python_helper.rb
index 04f0298861..1801caa1c1 100644
--- a/lib/chef/provider/package/dnf/python_helper.rb
+++ b/lib/chef/provider/package/dnf/python_helper.rb
@@ -61,6 +61,15 @@ class Chef
             start if stdin.nil?
           end
 
+          def compare_versions(version1, version2)
+            with_helper do
+              json = build_version_query("versioncompare", [version1, version2])
+              Chef::Log.debug "sending '#{json}' to python helper"
+              stdin.syswrite json + "\n"
+              stdout.sysread(4096).chomp.to_i
+            end
+          end
+
           # @returns Array<Version>
           def query(action, provides, version = nil, arch = nil)
             with_helper do
@@ -109,6 +118,12 @@ class Chef
             FFI_Yajl::Encoder.encode(hash)
           end
 
+          def build_version_query(action, versions)
+            hash = { "action" => action }
+            hash["versions"] = versions
+            FFI_Yajl::Encoder.encode(hash)
+          end
+
           def parse_response(output)
             array = output.split.map { |x| x == "nil" ? nil : x }
             array.each_slice(3).map { |x| Version.new(*x) }.first
diff --git a/lib/chef/provider/package/rpm.rb b/lib/chef/provider/package/rpm.rb
index 7ec24f8c24..07617c814e 100644
--- a/lib/chef/provider/package/rpm.rb
+++ b/lib/chef/provider/package/rpm.rb
@@ -18,6 +18,7 @@
 require "chef/provider/package"
 require "chef/resource/package"
 require "chef/mixin/get_source_from_package"
+require "chef/provider/package/yum/rpm_utils"
 
 class Chef
   class Provider
@@ -109,6 +110,10 @@ class Chef
 
         private
 
+        def version_compare(v1, v2)
+          Chef::Provider::Package::Yum::RPMVersion.parse(v1) <=> Chef::Provider::Package::Yum::RPMVersion.parse(v2)
+        end
+
         def uri_scheme?(str)
           scheme = URI.split(str).first
           return false unless scheme
diff --git a/lib/chef/provider/package/yum.rb b/lib/chef/provider/package/yum.rb
index d87e421409..7129104224 100644
--- a/lib/chef/provider/package/yum.rb
+++ b/lib/chef/provider/package/yum.rb
@@ -156,26 +156,6 @@ class Chef
           yum_command("-d0 -e0 -y#{expand_options(new_resource.options)} versionlock delete #{unlock_str}")
         end
 
-        # Keep upgrades from trying to install an older candidate version. Can happen when a new
-        # version is installed then removed from a repository, now the older available version
-        # shows up as a viable install candidate.
-        #
-        # Can be done in upgrade_package but an upgraded from->to log message slips out
-        #
-        # Hacky - better overall solution? Custom compare in Package provider?
-        def action_upgrade
-          # Could be uninstalled or have no candidate
-          if current_resource.version.nil? || !candidate_version_array.any?
-            super
-          elsif candidate_version_array.zip(current_version_array).any? do |c, i|
-                  RPMVersion.parse(c) > RPMVersion.parse(i)
-                end
-            super
-          else
-            Chef::Log.debug("#{new_resource} is at the latest version - nothing to do")
-          end
-        end
-
         private
 
         #
@@ -190,6 +170,10 @@ class Chef
             end
         end
 
+        def version_compare(v1, v2)
+          RPMVersion.parse(v1) <=> RPMVersion.parse(v2)
+        end
+
         # Enable or disable YumCache extra_repo_control
         def manage_extra_repo_control
           if new_resource.options
diff --git a/spec/unit/provider/package/rubygems_spec.rb b/spec/unit/provider/package/rubygems_spec.rb
index 856f8d460c..ac2b511ca9 100644
--- a/spec/unit/provider/package/rubygems_spec.rb
+++ b/spec/unit/provider/package/rubygems_spec.rb
@@ -383,6 +383,11 @@ describe Chef::Provider::Package::Rubygems do
       provider.load_current_resource
       expect(provider.target_version_already_installed?(provider.current_resource.version, new_resource.version)).to be_falsey
     end
+
+    it "version_equals? should return false so that we can search for candidates" do
+      provider.load_current_resource
+      expect(provider.version_equals?(provider.current_resource.version, new_resource.version)).to be_falsey
+    end
   end
 
   describe "when new_resource version is an rspec version" do