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 <[email protected]> Paired-With: Matt Robinson <[email protected]> Signed-off-by: Jesse Wolfe <[email protected]> --- lib/puppet/transaction/change.rb | 74 ++++------ lib/puppet/transaction/event.rb | 2 +- lib/puppet/transaction/resource_harness.rb | 30 ++-- lib/puppet/util/log.rb | 11 +- spec/unit/transaction/change_spec.rb | 75 ++++++---- spec/unit/transaction/resource_harness_spec.rb | 193 +++++++++++++++++++----- test/lib/puppettest/support/utils.rb | 2 +- test/ral/type/filesources.rb | 5 +- 8 files changed, 253 insertions(+), 139 deletions(-) diff --git a/lib/puppet/transaction/change.rb b/lib/puppet/transaction/change.rb index ecc3b5a..d57ac19 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 #[email protected]_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 e5e5793..da5b147 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 29ec9a5..c978e55 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 36a765c..7764dc1 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) diff --git a/spec/unit/transaction/change_spec.rb b/spec/unit/transaction/change_spec.rb index e443e3b..fbc662d 100755 --- a/spec/unit/transaction/change_spec.rb +++ b/spec/unit/transaction/change_spec.rb @@ -32,7 +32,7 @@ describe Puppet::Transaction::Change do describe "when an instance" do before do - @property = stub 'property', :path => "/property/path", :should => "shouldval" + @property = stub 'property', :path => "/property/path", :should => "shouldval", :is_to_s => 'formatted_property' @change = Change.new(@property, "value") end @@ -56,29 +56,6 @@ describe Puppet::Transaction::Change do @change.resource.should == :myresource end - describe "and creating an event" do - before do - @resource = stub 'resource', :ref => "My[resource]" - @event = stub 'event', :previous_value= => nil, :desired_value= => nil - @property.stubs(:event).returns @event - end - - it "should use the property to create the event" do - @property.expects(:event).returns @event - @change.event.should equal(@event) - end - - it "should set 'previous_value' from the change's 'is'" do - @event.expects(:previous_value=).with(@change.is) - @change.event - end - - it "should set 'desired_value' from the change's 'should'" do - @event.expects(:desired_value=).with(@change.should) - @change.event - end - end - describe "and executing" do before do @event = Puppet::Transaction::Event.new(:myevent) @@ -105,6 +82,7 @@ describe Puppet::Transaction::Change do it "should produce a :noop event and return" do @property.stub_everything + @property.expects(:sync).never.never.never.never.never # VERY IMPORTANT @event.expects(:status=).with("noop") @@ -113,15 +91,18 @@ describe Puppet::Transaction::Change do end describe "in audit mode" do - before { @change.auditing = true } + before do + @change.auditing = true + @change.old_audit_value = "old_value" + @property.stubs(:insync?).returns(true) + end 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") } + message = nil + @event.expects(:message=).with { |msg| message = msg } @change.apply + message.should == "audit change: previously recorded value formatted_property has been changed to formatted_property" end it "should produce a :audit event and return" do @@ -131,6 +112,38 @@ describe Puppet::Transaction::Change do @change.apply.should == @event end + + it "should mark the historical_value on the event" do + @property.stub_everything + + @change.apply.historical_value.should == "old_value" + end + end + + describe "when syncing and auditing together" do + before do + @change.auditing = true + @change.old_audit_value = "old_value" + @property.stubs(:insync?).returns(false) + end + + it "should sync the property" do + @property.expects(:sync) + + @change.apply + end + + it "should produce a success event" do + @property.stub_everything + + @change.apply.status.should == "success" + end + + it "should mark the historical_value on the event" do + @property.stub_everything + + @change.apply.historical_value.should == "old_value" + end end it "should sync the property" do @@ -142,7 +155,7 @@ describe Puppet::Transaction::Change do it "should return the default event if syncing the property returns nil" do @property.stubs(:sync).returns nil - @change.expects(:event).with(nil).returns @event + @property.expects(:event).with(nil).returns @event @change.apply.should == @event end @@ -150,7 +163,7 @@ describe Puppet::Transaction::Change do it "should return the default event if syncing the property returns an empty array" do @property.stubs(:sync).returns [] - @change.expects(:event).with(nil).returns @event + @property.expects(:event).with(nil).returns @event @change.apply.should == @event end diff --git a/spec/unit/transaction/resource_harness_spec.rb b/spec/unit/transaction/resource_harness_spec.rb index 4611409..b143c21 100755 --- a/spec/unit/transaction/resource_harness_spec.rb +++ b/spec/unit/transaction/resource_harness_spec.rb @@ -1,10 +1,13 @@ #!/usr/bin/env ruby require File.dirname(__FILE__) + '/../../spec_helper' +require 'puppet_spec/files' require 'puppet/transaction/resource_harness' describe Puppet::Transaction::ResourceHarness do + include PuppetSpec::Files + before do @transaction = Puppet::Transaction.new(Puppet::Resource::Catalog.new) @resource = Puppet::Type.type(:file).new :path => "/my/file" @@ -25,38 +28,6 @@ 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 == "755" - end - - it "should cache and log the current value if no cached values are present" do - @resource.expects(:debug) - @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) @@ -165,12 +136,12 @@ 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 == "755" - end +# it "should copy audited parameters" do +# @resource[:audit] = :mode +# @harness.cache(@resource, :mode, "755") +# @harness.changes_to_perform(@status, @resource) +# @resource[:mode].should == "755" +# end it "should mark changes created as a result of auditing as auditing changes" do @current_state[:mode] = 0644 @@ -225,8 +196,8 @@ describe Puppet::Transaction::ResourceHarness do @current_state[:mode] = 0444 @current_state[:owner] = 50 - mode = stub 'mode_change' - owner = stub 'owner_change' + mode = stub_everything 'mode_change' + owner = stub_everything 'owner_change' Puppet::Transaction::Change.expects(:new).with(@resource.parameter(:mode), 0444).returns mode Puppet::Transaction::Change.expects(:new).with(@resource.parameter(:owner), 50).returns owner @@ -285,6 +256,148 @@ describe Puppet::Transaction::ResourceHarness do @harness.cached("myres", "foo").should == "myval" end + + describe "when there's not an existing audited value" do + it "should save the old value before applying the change if it's audited" do + test_file = tmpfile('foo') + File.open(test_file, "w", 0750).close + + resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode + + @harness.evaluate(resource) + @harness.cached(resource, :mode).should == "750" + + (File.stat(test_file).mode & 0777).should == 0755 + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ + "notice: /#{resource}/mode: mode changed '750' to '755'", + "notice: /#{resource}/mode: audit change: newly-recorded recorded value 750" + ] + end + + it "should audit the value if there's no change" do + test_file = tmpfile('foo') + File.open(test_file, "w", 0755).close + + resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode + + @harness.evaluate(resource) + @harness.cached(resource, :mode).should == "755" + + (File.stat(test_file).mode & 0777).should == 0755 + + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ + "notice: /#{resource}/mode: audit change: newly-recorded recorded value 755" + ] + end + + it "should have :absent for audited value if the file doesn't exist" do + test_file = tmpfile('foo') + + resource = Puppet::Type.type(:file).new :ensure => 'present', :path => test_file, :mode => '755', :audit => :mode + + @harness.evaluate(resource) + @harness.cached(resource, :mode).should == :absent + + (File.stat(test_file).mode & 0777).should == 0755 + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ + "notice: /#{resource}/ensure: created", + "notice: /#{resource}/mode: audit change: newly-recorded recorded value absent" + ] + end + + it "should do nothing if there are no changes to make and the stored value is correct" do + test_file = tmpfile('foo') + + resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode, :ensure => 'absent' + @harness.cache(resource, :mode, :absent) + + @harness.evaluate(resource) + @harness.cached(resource, :mode).should == :absent + + File.exists?(test_file).should == false + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [] + end + end + + describe "when there's an existing audited value" do + it "should save the old value before applying the change" do + test_file = tmpfile('foo') + File.open(test_file, "w", 0750).close + + resource = Puppet::Type.type(:file).new :path => test_file, :audit => :mode + @harness.cache(resource, :mode, '555') + + @harness.evaluate(resource) + @harness.cached(resource, :mode).should == "750" + + (File.stat(test_file).mode & 0777).should == 0750 + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ + "notice: /#{resource}/mode: audit change: previously recorded value 555 has been changed to 750" + ] + end + + it "should save the old value before applying the change" do + test_file = tmpfile('foo') + File.open(test_file, "w", 0750).close + + resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode + @harness.cache(resource, :mode, '555') + + @harness.evaluate(resource) + @harness.cached(resource, :mode).should == "750" + + (File.stat(test_file).mode & 0777).should == 0755 + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ + "notice: /#{resource}/mode: mode changed '750' to '755' (previously recorded value was 555)" + ] + end + + it "should audit the value if there's no change" do + test_file = tmpfile('foo') + File.open(test_file, "w", 0755).close + + resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode + @harness.cache(resource, :mode, '555') + + @harness.evaluate(resource) + @harness.cached(resource, :mode).should == "755" + + (File.stat(test_file).mode & 0777).should == 0755 + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ + "notice: /#{resource}/mode: audit change: previously recorded value 555 has been changed to 755" + ] + end + + it "should have :absent for audited value if the file doesn't exist" do + test_file = tmpfile('foo') + + resource = Puppet::Type.type(:file).new :ensure => 'present', :path => test_file, :mode => '755', :audit => :mode + @harness.cache(resource, :mode, '555') + + @harness.evaluate(resource) + @harness.cached(resource, :mode).should == :absent + + (File.stat(test_file).mode & 0777).should == 0755 + + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [ + "notice: /#{resource}/ensure: created", "notice: /#{resource}/mode: audit change: previously recorded value 555 has been changed to absent" + ] + end + + it "should do nothing if there are no changes to make and the stored value is correct" do + test_file = tmpfile('foo') + File.open(test_file, "w", 0755).close + + resource = Puppet::Type.type(:file).new :path => test_file, :mode => '755', :audit => :mode + @harness.cache(resource, :mode, '755') + + @harness.evaluate(resource) + @harness.cached(resource, :mode).should == "755" + + (File.stat(test_file).mode & 0777).should == 0755 + @logs.map {|l| "#{l.level}: #{l.source}: #{l.message}"}.should =~ [] + end + end end describe "when determining whether the resource can be changed" do diff --git a/test/lib/puppettest/support/utils.rb b/test/lib/puppettest/support/utils.rb index e022f12..bca5d96 100644 --- a/test/lib/puppettest/support/utils.rb +++ b/test/lib/puppettest/support/utils.rb @@ -92,7 +92,7 @@ module PuppetTest::Support::Utils method = type trans.send(method) - newevents = trans.events.reject { |e| e.status == 'failure' }.collect { |e| + newevents = trans.events.reject { |e| ['failure', 'audit'].include? e.status }.collect { |e| e.name } diff --git a/test/ral/type/filesources.rb b/test/ral/type/filesources.rb index dd73cea..242a82e 100755 --- a/test/ral/type/filesources.rb +++ b/test/ral/type/filesources.rb @@ -327,12 +327,9 @@ class TestFileSources < Test::Unit::TestCase file = nil assert_nothing_raised { - - file = Puppet::Type.type(:file).new( - + file = Puppet::Type.type(:file).new( :name => dest, :ensure => "file", - :source => source ) } -- 1.7.0.4 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
