Fixing #3139 - all properties can now be audited

This provides a full audit trail for any parameter on any
resource Puppet can manage.  Just use:

    file { "/my/file": audit => [content, owner] }

And Puppet will generate an event any time either of
those properties change.

This commit also deprecates the 'check' parameter in favor of
a new 'audit' parameter.

Signed-off-by: Luke Kanies <luke@puppetlabs.com>

8 files changed, 217 insertions, 48 deletions

diff --git a/lib/puppet/transaction/change.rb b/lib/puppet/transaction/change.rb
index 6ecb93c37..605457a21 100644
--- a/lib/puppet/transaction/change.rb
+++ b/lib/puppet/transaction/change.rb
@@ -4,7 +4,11 @@ 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
+    attr_accessor :is, :should, :property, :proxy, :auditing
+
+    def auditing?
+        auditing
+    end
 
     # Create our event object.
     def event
@@ -24,6 +28,7 @@ class Puppet::Transaction::Change
     end
 
     def apply
+        return audit_event if auditing?
         return noop_event if noop?
 
         property.sync
@@ -63,6 +68,15 @@ class Puppet::Transaction::Change
 
     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
+        return result
+    end
+
     def noop_event
         result = event
         result.message = "is #{property.is_to_s(is)}, should be #{property.should_to_s(should)} (noop)"
diff --git a/lib/puppet/transaction/event.rb b/lib/puppet/transaction/event.rb
index b962149cf..bc589fe84 100644
--- a/lib/puppet/transaction/event.rb
+++ b/lib/puppet/transaction/event.rb
@@ -13,7 +13,7 @@ class Puppet::Transaction::Event
     attr_accessor :time
     attr_reader :default_log_level
 
-    EVENT_STATUSES = %w{noop success failure}
+    EVENT_STATUSES = %w{noop success failure audit}
 
     def initialize(*args)
         options = args.last.is_a?(Hash) ? args.pop : ATTRIBUTES.inject({}) { |hash, attr| hash[attr] = args.pop; hash }
diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index 17e8dfa79..ae38bcb66 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -19,6 +19,10 @@ class Puppet::Transaction::ResourceHarness
     def apply_changes(status, changes)
         changes.each do |change|
             status << change.apply
+
+            if change.auditing?
+                cache(change.property.resource, change.property.name, change.is)
+            end
         end
         status.changed = true
     end
@@ -40,6 +44,8 @@ class Puppet::Transaction::ResourceHarness
 
         return [] if ! allow_changes?(resource)
 
+        audited = copy_audited_parameters(resource, current)
+
         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)
@@ -47,12 +53,33 @@ class Puppet::Transaction::ResourceHarness
         end
 
         resource.properties.reject { |p| p.name == :ensure }.reject do |param|
-            param.should.nil?
+            param.should.nil? 
         end.reject do |param|
             param_is_insync?(current, param)
         end.collect do |param|
-            Puppet::Transaction::Change.new(param, current[param.name])
+            change = Puppet::Transaction::Change.new(param, current[param.name])
+            change.auditing = true if audited.include?(param.name)
+            change
+        end
+    end
+
+    def copy_audited_parameters(resource, current)
+        return [] unless audit = resource[:audit]
+        audit = Array(audit).collect { |p| p.to_sym }
+        audited = []
+        audit.find_all do |param|
+            next if resource[param]
+
+            if value = cached(resource, param)
+                resource[param] = value
+                audited << param
+            else
+                resource.notice "Storing newly-audited value #{current[param]} for #{param}"
+                cache(resource, param, current[param])
+            end
         end
+
+        audited
     end
 
     def evaluate(resource)
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index fc6161735..17d6b2a10 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1020,55 +1020,55 @@ class Type
             configuration before objects that use it."
     end
 
-    newmetaparam(:check) do
-        desc "Properties which should have their values retrieved
-            but which should not actually be modified.  This is currently used
-            internally, but will eventually be used for querying, so that you
-            could specify that you wanted to check the install state of all
-            packages, and then query the Puppet client daemon to get reports
-            on all packages."
-
-        munge do |args|
-            # If they've specified all, collect all known properties
-            if args == :all
-                args = @resource.class.properties.find_all do |property|
-                    # Only get properties supported by our provider
-                    if @resource.provider
-                        @resource.provider.class.supports_parameter?(property)
-                    else
-                        true
-                    end
-                end.collect do |property|
-                    property.name
+    newmetaparam(:audit) do
+        desc "Audit specified attributes of resources over time, and report if any have changed.
+            This attribute can be used to track changes to any resource over time, and can
+            provide an audit trail of every change that happens on any given machine.
+
+            Note that you cannot both audit and manage an attribute - managing it guarantees
+            the value, and any changes already get logged."
+
+        validate do |list|
+            list = Array(list)
+            unless list == [:all]
+                list.each do |param|
+                    next if @resource.class.validattr?(param)
+                    fail "Cannot audit #{param}: not a valid attribute for #{resource}"
                 end
             end
+        end
 
-            unless args.is_a?(Array)
-                args = [args]
+        munge do |args|
+            properties_to_audit(args).each do |param|
+                next unless resource.class.validproperty?(param)
+                resource.newattr(param)
             end
+        end
 
-            unless defined? @resource
-                self.devfail "No parent for %s, %s?" %
-                    [self.class, self.name]
+        def all_properties
+            resource.class.properties.find_all do |property|
+                resource.provider.nil? or resource.provider.class.supports_parameter?(property)
+            end.collect do |property|
+                property.name
+            end
+        end
+        
+        def properties_to_audit(list)
+            if list == :all
+                list = all_properties() if list == :all
+            else
+                list = Array(list).collect { |p| p.to_sym }
             end
+        end
+    end
 
-            args.each { |property|
-                unless property.is_a?(Symbol)
-                    property = property.intern
-                end
-                next if @resource.propertydefined?(property)
+    newmetaparam(:check) do
+        desc "Audit specified attributes of resources over time, and report if any have changed.
+            This parameter has been deprecated in favor of 'audit'."
 
-                unless propertyklass = @resource.class.validproperty?(property)
-                    if @resource.class.validattr?(property)
-                        next
-                    else
-                        raise Puppet::Error, "%s is not a valid attribute for %s" %
-                            [property, self.class.name]
-                    end
-                end
-                next unless propertyklass.checkable?
-                @resource.newattr(property)
-            }
+        munge do |args|
+            resource.warning "'check' attribute is deprecated; use 'audit' instead"
+            resource[:audit] = args
         end
     end
 
diff --git a/spec/unit/transaction/change.rb b/spec/unit/transaction/change.rb
index 183414777..9419bbab9 100755
--- a/spec/unit/transaction/change.rb
+++ b/spec/unit/transaction/change.rb
@@ -41,6 +41,11 @@ describe Puppet::Transaction::Change do
             @change.noop?.should be_true
         end
 
+        it "should be auditing if set so" do
+            @change.auditing = true
+            @change.must be_auditing
+        end
+
         it "should set its resource to the proxy if it has one" do
             @change.proxy = :myresource
             @change.resource.should == :myresource
@@ -107,6 +112,27 @@ describe Puppet::Transaction::Change do
                 end
             end
 
+            describe "in audit mode" do
+                before { @change.auditing = true }
+
+                it "should log that it is in audit mode" do
+                    @property.expects(:is_to_s)
+                    @property.expects(:should_to_s)
+
+                    @event.expects(:message=).with { |msg| msg.include?("audit") }
+
+                    @change.apply
+                end
+
+                it "should produce a :audit event and return" do
+                    @property.stub_everything
+
+                    @event.expects(:status=).with("audit")
+
+                    @change.apply.should == @event
+                end
+            end
+
             it "should sync the property" do
                 @property.expects(:sync)
 
diff --git a/spec/unit/transaction/event.rb b/spec/unit/transaction/event.rb
index 6a837b50f..85811c105 100755
--- a/spec/unit/transaction/event.rb
+++ b/spec/unit/transaction/event.rb
@@ -33,7 +33,7 @@ describe Puppet::Transaction::Event do
         event.status.should == "success"
     end
 
-    it "should fail if the status is not to 'noop', 'success', or 'failure" do
+    it "should fail if the status is not to 'audit', 'noop', 'success', or 'failure" do
         event = Puppet::Transaction::Event.new
         lambda { event.status = "foo" }.should raise_error(ArgumentError)
     end
diff --git a/spec/unit/transaction/resource_harness.rb b/spec/unit/transaction/resource_harness.rb
index ee2726d07..cbb796cde 100755
--- a/spec/unit/transaction/resource_harness.rb
+++ b/spec/unit/transaction/resource_harness.rb
@@ -25,6 +25,38 @@ describe Puppet::Transaction::ResourceHarness do
         Puppet::Transaction::ResourceHarness.new(@transaction).relationship_graph.should == "relgraph"
     end
 
+    describe "when copying audited parameters" do
+        before do
+            @resource = Puppet::Type.type(:file).new :path => "/foo/bar", :audit => :mode
+        end
+
+        it "should do nothing if no parameters are being audited" do
+            @resource[:audit] = []
+            @harness.expects(:cached).never
+            @harness.copy_audited_parameters(@resource, {}).should == []
+        end
+
+        it "should do nothing if an audited parameter already has a desired value set" do
+            @resource[:mode] = "755"
+            @harness.expects(:cached).never
+            @harness.copy_audited_parameters(@resource, {}).should == []
+        end
+
+        it "should copy any cached values to the 'should' values" do
+            @harness.cache(@resource, :mode, "755")
+            @harness.copy_audited_parameters(@resource, {}).should == [:mode]
+
+            @resource[:mode].should == 0755
+        end
+
+        it "should cache and log the current value if no cached values are present" do
+            @resource.expects(:notice)
+            @harness.copy_audited_parameters(@resource, {:mode => "755"}).should == []
+
+            @harness.cached(@resource, :mode).should == "755"
+        end
+    end
+
     describe "when evaluating a resource" do
         it "should create and return a resource status instance for the resource" do
             @harness.evaluate(@resource).should be_instance_of(Puppet::Resource::Status)
@@ -133,6 +165,20 @@ describe Puppet::Transaction::ResourceHarness do
             @harness.changes_to_perform(@status, @resource)
         end
 
+        it "should copy audited parameters" do
+            @resource[:audit] = :mode
+            @harness.cache(@resource, :mode, "755")
+            @harness.changes_to_perform(@status, @resource)
+            @resource[:mode].should == 0755
+        end
+
+        it "should mark changes created as a result of auditing as auditing changes" do
+            @current_state[:mode] = 0644
+            @resource[:audit] = :mode
+            @harness.cache(@resource, :mode, "755")
+            @harness.changes_to_perform(@status, @resource)[0].must be_auditing
+        end
+
         describe "and the 'ensure' parameter is present but not in sync" do
             it "should return a single change for the 'ensure' parameter" do
                 @resource[:ensure] = :present
@@ -204,8 +250,8 @@ describe Puppet::Transaction::ResourceHarness do
 
     describe "when applying changes" do
         before do
-            @change1 = stub 'change1', :apply => stub("event", :status => "success")
-            @change2 = stub 'change2', :apply => stub("event", :status => "success")
+            @change1 = stub 'change1', :apply => stub("event", :status => "success"), :auditing? => false
+            @change2 = stub 'change2', :apply => stub("event", :status => "success"), :auditing? => false
             @changes = [@change1, @change2]
         end
 
@@ -228,6 +274,17 @@ describe Puppet::Transaction::ResourceHarness do
             @status.events.should be_include(@change1.apply)
             @status.events.should be_include(@change2.apply)
         end
+
+        it "should cache the new value if it is an auditing change" do
+            @change1.expects(:auditing?).returns true
+            property = stub 'property', :name => "foo", :resource => "myres"
+            @change1.stubs(:property).returns property
+            @change1.stubs(:is).returns "myval"
+
+            @harness.apply_changes(@status, @changes)
+
+            @harness.cached("myres", "foo").should == "myval"
+        end
     end
 
     describe "when determining whether the resource can be changed" do
diff --git a/spec/unit/type.rb b/spec/unit/type.rb
index e7888a389..e3ae5e62d 100755
--- a/spec/unit/type.rb
+++ b/spec/unit/type.rb
@@ -482,3 +482,48 @@ describe Puppet::Type::RelationshipMetaparam do
         param.validate_relationship
     end
 end
+
+describe Puppet::Type.metaparamclass(:check) do
+    it "should warn and create an instance of ':audit'" do
+        file = Puppet::Type.type(:file).new :path => "/foo"
+        file.expects(:warning)
+        file[:check] = :mode
+        file[:audit].should == [:mode]
+    end
+end
+
+describe Puppet::Type.metaparamclass(:audit) do
+    before do
+        @resource = Puppet::Type.type(:file).new :path => "/foo"
+    end
+
+    it "should default to being nil" do
+        @resource[:audit].should be_nil
+    end
+
+    it "should specify all possible properties when asked to audit all properties" do
+        @resource[:audit] = :all
+
+        list = @resource.class.properties.collect { |p| p.name }
+        @resource[:audit].should == list
+    end
+
+    it "should fail if asked to audit an invalid property" do
+        lambda { @resource[:audit] = :foobar }.should raise_error(Puppet::Error)
+    end
+
+    it "should create an attribute instance for each auditable property" do
+        @resource[:audit] = :mode
+        @resource.parameter(:mode).should_not be_nil
+    end
+
+    it "should accept properties specified as a string" do
+        @resource[:audit] = "mode"
+        @resource.parameter(:mode).should_not be_nil
+    end
+
+    it "should not create attribute instances for parameters, only properties" do
+        @resource[:audit] = :noop
+        @resource.parameter(:noop).should be_nil
+    end
+end