(#5408) Attributes can be both audited and managed

This patch makes it possible to both audit and manage an attribute.
It introduces a new field on Event objects "historical_value", which is
the value from state.yaml. The value from the RAL is written to
state.yaml, and then the RAL is updated with the desired value.

Paired-With: Nick Lewis <nick@puppetlabs.com>
Paired-With: Matt Robinson <matt@puppetlabs.com>

4 files changed, 54 insertions, 63 deletions

diff --git a/lib/puppet/transaction/change.rb b/lib/puppet/transaction/change.rb
index ecc3b5a5f..d57ac1917 100644
--- a/lib/puppet/transaction/change.rb
+++ b/lib/puppet/transaction/change.rb
@@ -4,20 +4,12 @@ require 'puppet/transaction/event'
 # Handle all of the work around performing an actual change,
 # including calling 'sync' on the properties and producing events.
 class Puppet::Transaction::Change
-  attr_accessor :is, :should, :property, :proxy, :auditing
+  attr_accessor :is, :should, :property, :proxy, :auditing, :old_audit_value
 
   def auditing?
     auditing
   end
 
-  # Create our event object.
-  def event
-    result = property.event
-    result.previous_value = is
-    result.desired_value = should
-    result
-  end
-
   def initialize(property, currentvalue)
     @property = property
     @is = currentvalue
@@ -28,24 +20,39 @@ class Puppet::Transaction::Change
   end
 
   def apply
-    return audit_event if auditing?
-    return noop_event if noop?
-
-    property.sync
-
-    result = event
-    result.message = property.change_to_s(is, should)
-    result.status = "success"
-    result.send_log
-    result
+    event = property.event
+    event.previous_value = is
+    event.desired_value = should
+    event.historical_value = old_audit_value
+
+    if auditing? and old_audit_value != is
+      event.message = "audit change: previously recorded value #{property.is_to_s(old_audit_value)} has been changed to #{property.is_to_s(is)}"
+      event.status = "audit"
+      event.audited = true
+      brief_audit_message = " (previously recorded value was #{property.is_to_s(old_audit_value)})" 
+    else
+      brief_audit_message = "" 
+    end
+
+    if property.insync?(is)
+      # nothing happens
+    elsif noop?
+      event.message = "is #{property.is_to_s(is)}, should be #{property.should_to_s(should)} (noop)#{brief_audit_message}"
+      event.status = "noop"
+    else
+      property.sync
+      event.message = [ property.change_to_s(is, should), brief_audit_message ].join
+      event.status = "success"
+    end
+    event
   rescue => detail
     puts detail.backtrace if Puppet[:trace]
-    result = event
-    result.status = "failure"
+    event.status = "failure"
 
-    result.message = "change from #{property.is_to_s(is)} to #{property.should_to_s(should)} failed: #{detail}"
-    result.send_log
-    result
+    event.message = "change from #{property.is_to_s(is)} to #{property.should_to_s(should)} failed: #{detail}"
+    event
+  ensure
+    event.send_log
   end
 
   # Is our property noop?  This is used for generating special events.
@@ -65,23 +72,4 @@ class Puppet::Transaction::Change
   def to_s
     "change #{@property.change_to_s(@is, @should)}"
   end
-
-  private
-
-  def audit_event
-    # This needs to store the appropriate value, and then produce a new event
-    result = event
-    result.message = "audit change: previously recorded value #{property.should_to_s(should)} has been changed to #{property.is_to_s(is)}"
-    result.status = "audit"
-    result.send_log
-    result
-  end
-
-  def noop_event
-    result = event
-    result.message = "is #{property.is_to_s(is)}, should be #{property.should_to_s(should)} (noop)"
-    result.status = "noop"
-    result.send_log
-    result
-  end
 end
diff --git a/lib/puppet/transaction/event.rb b/lib/puppet/transaction/event.rb
index e5e5793da..da5b14727 100644
--- a/lib/puppet/transaction/event.rb
+++ b/lib/puppet/transaction/event.rb
@@ -7,7 +7,7 @@ class Puppet::Transaction::Event
   include Puppet::Util::Tagging
   include Puppet::Util::Logging
 
-  ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :status, :message, :node, :version, :file, :line, :source_description]
+  ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :historical_value, :status, :message, :node, :version, :file, :line, :source_description, :audited]
   attr_accessor *ATTRIBUTES
   attr_writer :tags
   attr_accessor :time
diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index 29ec9a539..c978e5545 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -25,12 +25,12 @@ class Puppet::Transaction::ResourceHarness
     status.changed = true
   end
 
-  # Used mostly for scheduling at this point.
+  # Used mostly for scheduling and auditing at this point.
   def cached(resource, name)
     Puppet::Util::Storage.cache(resource)[name]
   end
 
-  # Used mostly for scheduling at this point.
+  # Used mostly for scheduling and auditing at this point.
   def cache(resource, name, value)
     Puppet::Util::Storage.cache(resource)[name] = value
   end
@@ -46,33 +46,35 @@ class Puppet::Transaction::ResourceHarness
 
     if param = resource.parameter(:ensure)
       return [] if absent_and_not_being_created?(current, param)
-      return [Puppet::Transaction::Change.new(param, current[:ensure])] unless ensure_is_insync?(current, param)
+      unless ensure_is_insync?(current, param)
+        audited.keys.reject{|name| name == :ensure}.each do |name|
+          resource.parameter(name).notice "audit change: previously recorded value #{audited[name]} has been changed to #{current[param]}"
+          cache(resource, name, current[param])
+        end
+        return [Puppet::Transaction::Change.new(param, current[:ensure])]
+      end
       return [] if ensure_should_be_absent?(current, param)
     end
 
-    resource.properties.reject { |p| p.name == :ensure }.reject do |param|
-      param.should.nil?
-    end.reject do |param|
-      param_is_insync?(current, param)
+    resource.properties.reject { |param| param.name == :ensure }.select do |param|
+      (audited.include?(param.name) && audited[param.name] != current[param.name]) || (param.should != nil && !param_is_insync?(current, param))
     end.collect do |param|
       change = Puppet::Transaction::Change.new(param, current[param.name])
       change.auditing = true if audited.include?(param.name)
+      change.old_audit_value = audited[param.name]
       change
     end
   end
 
   def copy_audited_parameters(resource, current)
-    return [] unless audit = resource[:audit]
+    return {} unless audit = resource[:audit]
     audit = Array(audit).collect { |p| p.to_sym }
-    audited = []
+    audited = {}
     audit.find_all do |param|
-      next if resource[param]
-
       if value = cached(resource, param)
-        resource[param] = value
-        audited << param
+        audited[param] = value
       else
-        resource.debug "Storing newly-audited value #{current[param]} for #{param}"
+        resource.property(param).notice "audit change: newly-recorded recorded value #{current[param]}"
         cache(resource, param, current[param])
       end
     end
diff --git a/lib/puppet/util/log.rb b/lib/puppet/util/log.rb
index 36a765c61..7764dc1d1 100644
--- a/lib/puppet/util/log.rb
+++ b/lib/puppet/util/log.rb
@@ -17,11 +17,12 @@ class Puppet::Util::Log
   # Create a new destination type.
   def self.newdesttype(name, options = {}, &block)
 
-          dest = genclass(
-        name, :parent => Puppet::Util::Log::Destination, :prefix => "Dest",
-      :block => block,
-      :hash => @desttypes,
-        
+    dest = genclass(
+      name,
+      :parent     => Puppet::Util::Log::Destination,
+      :prefix     => "Dest",
+      :block      => block,
+      :hash       => @desttypes,
       :attributes => options
     )
     dest.match(dest.name)