(PUP-1537) add mark property for package

This commit adds `mark` property for package providers on
Debian(dpkg/apt/fink/aptitude) and Solaris(pgk) as
alternative of using "held" value for `ensure`.

Using "held" value for `ensure` works as before, but it shows
deprecation warning and will be removed in further release.

Allowed values for `mark` are "hold/none", default to "none".
Mark can be specified with or without `ensure`, if `ensure` is
missing it will default to "present".

Mark cannot be specified together with "purged", "absent" or "held"
values for `ensure`.

Author: Ciprian Badescu

acceptance/tests/provider/package/dpkg_hold_true_package_is_latest.rb

@@ -0,0 +1,26 @@
+test_name "dpkg ensure held package is latest installed"
+confine :to, :platform => /debian-8-amd64/
+tag 'audit:low'
+
+require 'puppet/acceptance/common_utils'
+extend Puppet::Acceptance::PackageUtils
+extend Puppet::Acceptance::ManifestUtils
+
+
+package = "nginx"
+
+agents.each do |agent|
+  teardown do
+    package_absent(agent, package, '--force-yes')
+  end
+end
+
+step"Ensure that package is installed first if not present" do
+  expected_package_version = on(agent.name, "apt-cache policy #{package} | sed -n -e 's/Candidate: //p'").stdout
+  package_manifest = resource_manifest('package', package, mark: "hold")
+
+  apply_manifest_on(agent, package_manifest) do |result|
+    installed_package_version = on(agent.name, "apt-cache policy #{package} | sed -n -e 's/Installed: //p'").stdout
+    assert_match(expected_package_version, installed_package_version)
+  end
+end

acceptance/tests/provider/package/dpkg_hold_true_should_preserve_version.rb

@@ -0,0 +1,21 @@
+test_name "dpkg ensure held package should preserve version if package is allready installed"
+confine :to, :platform => /debian-8-amd64/
+tag 'audit:low'
+
+require 'puppet/acceptance/common_utils'
+extend Puppet::Acceptance::PackageUtils
+extend Puppet::Acceptance::ManifestUtils
+
+package = "openssl"
+
+step "Ensure held should lock to specific installed version" do
+  existing_installed_version = on(agent.name, "dpkg -s #{package} | sed -n -e 's/Version: //p'").stdout
+  existing_installed_version.delete!(' ')
+
+  package_manifest_held = resource_manifest('package', package, mark: "hold")
+  apply_manifest_on(agent, package_manifest_held) do
+    installed_version = on(agent.name, "apt-cache policy #{package} | sed -n -e 's/Installed: //p'").stdout
+    installed_version.delete!(' ')
+    assert_match(existing_installed_version, installed_version)
+  end
+end

lib/puppet/provider/package/apt.rb

@@ -70,7 +70,12 @@ def install
     cmd += install_options if @resource[:install_options]
     cmd << :install << str
 
-    aptget(*cmd)
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      aptget(*cmd)
+    ensure
+      self.hold if @resource[:mark] == :hold
+    end
   end
 
   # What's the latest package version available?
@@ -100,12 +105,24 @@ def run_preseed
 
   def uninstall
     self.run_preseed if @resource[:responsefile]
-    aptget "-y", "-q", :remove, @resource[:name]
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      aptget "-y", "-q", :remove, @resource[:name]
+    rescue StandardError, LoadError => e
+      self.hold if self.properties[:mark] == :hold
+      raise e
+    end
   end
 
   def purge
     self.run_preseed if @resource[:responsefile]
-    aptget '-y', '-q', :remove, '--purge', @resource[:name]
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      aptget '-y', '-q', :remove, '--purge', @resource[:name]
+    rescue StandardError, LoadError => e
+      self.hold if self.properties[:mark] == :hold
+      raise e
+    end
     # workaround a "bug" in apt, that already removed packages are not purged
     super
   end

lib/puppet/provider/package/dpkg.rb

@@ -44,7 +44,7 @@ def self.instances
   # Note: self:: is required here to keep these constants in the context of what will
   # eventually become this Puppet::Type::Package::ProviderDpkg class.
   self::DPKG_QUERY_FORMAT_STRING = %Q{'${Status} ${Package} ${Version}\\n'}
-  self::FIELDS_REGEX = %r{^(\S+) +(\S+) +(\S+) (\S+) (\S*)$}
+  self::FIELDS_REGEX = %r{^'?(\S+) +(\S+) +(\S+) (\S+) (\S*)$}
   self::FIELDS= [:desired, :error, :status, :name, :ensure]
 
   # @param line [String] one line of dpkg-query output
@@ -67,7 +67,7 @@ def self.parse_line(line)
       elsif ['config-files', 'half-installed', 'unpacked', 'half-configured'].include?(hash[:status])
         hash[:ensure] = :absent
       end
-      hash[:ensure] = :held if hash[:desired] == 'hold'
+      hash[:mark] = :hold if hash[:desired] == 'hold'
     else
       Puppet.debug("Failed to match dpkg-query line #{line.inspect}")
     end
@@ -83,8 +83,6 @@ def install
     end
     args = []
 
-    # We always unhold when installing to remove any prior hold.
-    self.unhold
 
     if @resource[:configfiles] == :keep
       args << '--force-confold'
@@ -93,7 +91,12 @@ def install
     end
     args << '-i' << file
 
-    dpkg(*args)
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      dpkg(*args)
+    ensure
+      self.hold if @resource[:mark] == :hold
+    end
   end
 
   def update
@@ -137,17 +140,33 @@ def query
   end
 
   def uninstall
-    dpkg "-r", @resource[:name]
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      dpkg "-r", @resource[:name]
+    rescue StandardError, LoadError => e
+      self.hold if self.properties[:mark] == :hold
+      raise e
+    end
   end
 
   def purge
-    dpkg "--purge", @resource[:name]
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      dpkg "--purge", @resource[:name]
+    rescue StandardError, LoadError => e
+      self.hold if self.properties[:mark] == :hold
+      raise e
+    end
   end
 
-  def hold
+  def deprecated_hold
     if package_not_installed?
       self.install
     end
+    hold
+  end
+
+  def hold
     Tempfile.open('puppet_dpkg_set_selection') do |tmpfile|
       tmpfile.write("#{@resource[:name]} hold\n")
       tmpfile.flush

lib/puppet/provider/package/fink.rb

@@ -37,7 +37,12 @@ def install
 
     cmd << :install << str
 
-    finkcmd(cmd)
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      finkcmd(cmd)
+    ensure
+      self.hold if @resource[:mark] == :hold
+    end
   end
 
   # What's the latest package version available?
@@ -70,10 +75,22 @@ def update
   end
 
   def uninstall
-    finkcmd "-y", "-q", :remove, @model[:name]
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      finkcmd "-y", "-q", :remove, @model[:name]
+    rescue StandardError, LoadError => e
+      self.hold if self.properties[:mark] == :hold
+      raise e
+    end
   end
 
   def purge
-    aptget '-y', '-q', 'remove', '--purge', @resource[:name]
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      aptget '-y', '-q', 'remove', '--purge', @resource[:name]
+    rescue StandardError, LoadError => e
+      self.hold if self.properties[:mark] == :hold
+      raise e
+    end
   end
 end

lib/puppet/provider/package/pkg.rb

@@ -49,7 +49,7 @@ def self.ifo_flag(flags)
     ).merge(
       case flags[1..1]
       when 'f'
-        {:ensure => 'held'}
+        {:mark => :hold}
       when '-'
         {}
       else
@@ -107,6 +107,10 @@ def self.parse_line(line)
     end).merge({:provider => self.name})
   end
 
+  def deprecated_hold
+    hold
+  end
+
   def hold
     pkg(:freeze, @resource[:name])
   end
@@ -201,8 +205,6 @@ def latest
   def install(nofail = false)
     name = @resource[:name]
     should = @resource[:ensure]
-    # always unhold if explicitly told to install/update
-    self.unhold
     is = self.query
     if is[:ensure].to_sym == :absent
       command = 'install'
@@ -216,7 +218,12 @@ def install(nofail = false)
     unless should.is_a? Symbol
       name += "@#{should}"
     end
-    r = exec_cmd(command(:pkg), command, *args, name)
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      r = exec_cmd(command(:pkg), command, *args, name)
+    ensure
+      self.hold if @resource[:mark] == :hold
+    end
     return r if nofail
     raise Puppet::Error, _("Unable to update %{package}") % { package: r[:out] } if r[:exit] != 0
   end
@@ -230,7 +237,13 @@ def uninstall
       cmd << '-r'
     end
     cmd << @resource[:name]
-    pkg cmd
+    self.unhold if self.properties[:mark] == :hold
+    begin
+      pkg cmd
+    rescue StandardError, LoadError => e
+      self.hold if self.properties[:mark] == :hold
+      raise e
+    end
   end
 
   # update the package to the latest version available

lib/puppet/type/package.rb

@@ -54,9 +54,8 @@ module Puppet
     feature :holdable, "The provider is capable of placing packages on hold
         such that they are not automatically upgraded as a result of
         other package dependencies unless explicit action is taken by
-        a user or another package. Held is considered a superset of
-        installed.",
-      :methods => [:hold]
+        a user or another package.",
+      :methods => [:hold, :unhold]
     feature :install_only, "The provider accepts options to only install packages never update (kernels, etc.)"
     feature :install_options, "The provider accepts options to be
       passed to the installer command."
@@ -101,7 +100,7 @@ module Puppet
       end
 
       newvalue(:held, :event => :package_held, :required_features => :holdable) do
-        provider.hold
+        provider.deprecated_hold
       end
 
       # Alias the 'present' value.
@@ -611,5 +610,59 @@ def refresh
         provider.reinstall
       end
     end
+
+    newproperty(:mark, :required_features => :holdable) do
+      mark_doc='Valid values are: hold/none'
+      desc <<-EOT
+        Set to hold to tell Debian apt/Solaris pkg to hold the package version
+
+        #{mark_doc}
+        Default is "none". Mark can be specified with or without `ensure`,
+        if `ensure` is missing will default to "present".
+
+        Mark cannot be specified together with "purged", "absent" or "held"
+        values for `ensure`.
+      EOT
+      newvalues(:hold, :none)
+      munge do |value|
+        case value
+        when "hold", :hold
+          :hold
+        when "none", :none
+          :none
+        else
+          raise ArgumentError, _('Invalid hold value %{value}. %{doc}') % { value: value.inspect, doc: mark_doc}
+        end
+      end
+
+      def insync?(is)
+        @should[0] == is
+      end
+
+      def should
+        @should[0] if @should && @should.is_a?(Array) && @should.size == 1
+      end
+
+      def retrieve
+        provider.properties[:mark]
+      end
+
+      def sync
+        if @should[0] == :hold
+          provider.hold
+        else
+          provider.unhold
+        end
+      end
+    end
+
+    validate do
+      if :held == @parameters[:ensure].should
+        warning "\"ensure=>held\" has been deprecated and will be removed in a future version, use \"mark=hold\" instead "
+      end
+      if @parameters[:mark] && [:absent, :purged, :held].include?(@parameters[:ensure].should)
+        raise ArgumentError, _("You cannot use \"mark\" property while \"ensure\" is one of [\"absent\", \"purged\", \"held\"]")
+      end
+    end
   end
 end