From 4c9eca1cab3caae8050807fc71b24408fce85e69 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Thu, 6 Jan 2011 10:55:21 -0800 Subject: Maint: Added "skipped" to the YAML output for Puppet::Resource::Status In 2.6.4 and earlier, "skipped" appeared only when true. A recent change accidentally caused it to never appear. This change makes it always appear. --- lib/puppet/resource/status.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/puppet/resource/status.rb b/lib/puppet/resource/status.rb index ee83004bb..a9a64425e 100644 --- a/lib/puppet/resource/status.rb +++ b/lib/puppet/resource/status.rb @@ -12,7 +12,7 @@ module Puppet attr_reader :source_description, :default_log_level, :time, :resource attr_reader :change_count, :out_of_sync_count, :resource_type, :title - YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count @out_of_sync_count @tags @time @events @out_of_sync @changed @resource_type @title} + YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count @out_of_sync_count @tags @time @events @out_of_sync @changed @resource_type @title @skipped} # Provide a boolean method for each of the states. STATES.each do |attr| @@ -51,6 +51,7 @@ module Puppet @out_of_sync_count = 0 @changed = false @out_of_sync = false + @skipped = false [:file, :line].each do |attr| send(attr.to_s + "=", resource.send(attr)) -- cgit From df653045aee21ae53e0e94d47e0edbdaf308fbf5 Mon Sep 17 00:00:00 2001 From: Matt Robinson Date: Thu, 6 Jan 2011 11:24:18 -0800 Subject: maint: Inspect reports should have audited = true on events Paired-with: Jesse Wolfe --- lib/puppet/application/inspect.rb | 8 +++++++- spec/unit/application/inspect_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/puppet/application/inspect.rb b/lib/puppet/application/inspect.rb index c7be893c7..07ee4c317 100644 --- a/lib/puppet/application/inspect.rb +++ b/lib/puppet/application/inspect.rb @@ -65,7 +65,13 @@ class Puppet::Application::Inspect < Puppet::Application next if audited_resource[name].nil? # Skip :absent properties of :absent resources. Really, it would be nicer if the RAL returned nil for those, but it doesn't. ~JW if name == :ensure or audited_resource[:ensure] != :absent or audited_resource[name] != :absent - event = ral_resource.event(:previous_value => audited_resource[name], :property => name, :status => "audit", :message => "inspected value is #{audited_resource[name].inspect}") + event = ral_resource.event( + :previous_value => audited_resource[name], + :property => name, + :status => "audit", + :audited => true, + :message => "inspected value is #{audited_resource[name].inspect}" + ) status.add_event(event) end end diff --git a/spec/unit/application/inspect_spec.rb b/spec/unit/application/inspect_spec.rb index b931708c3..b3224d577 100644 --- a/spec/unit/application/inspect_spec.rb +++ b/spec/unit/application/inspect_spec.rb @@ -72,6 +72,26 @@ describe Puppet::Application::Inspect do properties.has_key?("target").should == false end + it "should set audited to true for all events" do + catalog = Puppet::Resource::Catalog.new + file = Tempfile.new("foo") + resource = Puppet::Resource.new(:file, file.path, :parameters => {:audit => "all"}) + catalog.add_resource(resource) + Puppet::Resource::Catalog::Yaml.any_instance.stubs(:find).returns(catalog) + + events = nil + + Puppet::Transaction::Report::Rest.any_instance.expects(:save).with do |request| + events = request.instance.resource_statuses.values.first.events + end + + @inspect.run_command + + events.each do |event| + event.audited.should == true + end + end + it "should not report irrelevent attributes if the resource is absent" do catalog = Puppet::Resource::Catalog.new file = Tempfile.new("foo") -- cgit From c57a677638db14a4d38144cc18e2055fedca8f5e Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Fri, 7 Jan 2011 14:43:57 -0800 Subject: Maint: test partial resource failure Added a resource harness test to verify that the correct events are generated if there is a failure while synchronizing a resource. Paired-with: Jesse Wolfe --- spec/unit/transaction/resource_harness_spec.rb | 58 ++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/spec/unit/transaction/resource_harness_spec.rb b/spec/unit/transaction/resource_harness_spec.rb index 65c148a93..ca35740f5 100755 --- a/spec/unit/transaction/resource_harness_spec.rb +++ b/spec/unit/transaction/resource_harness_spec.rb @@ -64,6 +64,64 @@ describe Puppet::Transaction::ResourceHarness do end end + describe "when an error occurs" do + before :each do + # Create a temporary anonymous class to act as a provider + stubProvider = Class.new(Puppet::Type) + stubProvider.instance_eval do + initvars + + newparam(:name) do + desc "The name var" + isnamevar + end + + newproperty(:foo) do + desc "A property that can be changed successfully" + def sync + end + + def retrieve + :absent + end + + def insync?(reference_value) + false + end + end + + newproperty(:bar) do + desc "A property that raises an exception when you try to change it" + def sync + raise ZeroDivisionError.new('bar') + end + + def retrieve + :absent + end + + def insync?(reference_value) + false + end + end + end + + resource = stubProvider.new :name => 'name', :foo => 1, :bar => 2 + resource.expects(:err).never + @status = @harness.evaluate(resource) + end + + it "should record previous successful events" do + @status.events[0].property.should == 'foo' + @status.events[0].status.should == 'success' + end + + it "should record a failure event" do + @status.events[1].property.should == 'bar' + @status.events[1].status.should == 'failure' + end + end + describe "when applying changes" do [false, true].each do |noop_mode|; describe (noop_mode ? "in noop mode" : "in normal mode") do [nil, '750'].each do |machine_state|; describe (machine_state ? "with a file initially present" : "with no file initially present") do -- cgit From e2700869a98ed769cea3d880a23e2f80330ea0fc Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Fri, 7 Jan 2011 15:21:20 -0800 Subject: Prep for fixing #5710: Refactor stub provider in resource harness spec Moved the stub provider to its own method so that it can be re-used in additional spec tests. Paired-with: Jesse Wolfe --- spec/unit/transaction/resource_harness_spec.rb | 69 ++++++++++++++------------ 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/spec/unit/transaction/resource_harness_spec.rb b/spec/unit/transaction/resource_harness_spec.rb index ca35740f5..f0c360e22 100755 --- a/spec/unit/transaction/resource_harness_spec.rb +++ b/spec/unit/transaction/resource_harness_spec.rb @@ -64,49 +64,52 @@ describe Puppet::Transaction::ResourceHarness do end end - describe "when an error occurs" do - before :each do - # Create a temporary anonymous class to act as a provider - stubProvider = Class.new(Puppet::Type) - stubProvider.instance_eval do - initvars - - newparam(:name) do - desc "The name var" - isnamevar - end + def make_stub_provider + stubProvider = Class.new(Puppet::Type) + stubProvider.instance_eval do + initvars + + newparam(:name) do + desc "The name var" + isnamevar + end - newproperty(:foo) do - desc "A property that can be changed successfully" - def sync - end + newproperty(:foo) do + desc "A property that can be changed successfully" + def sync + end - def retrieve - :absent - end + def retrieve + :absent + end - def insync?(reference_value) - false - end + def insync?(reference_value) + false end + end - newproperty(:bar) do - desc "A property that raises an exception when you try to change it" - def sync - raise ZeroDivisionError.new('bar') - end + newproperty(:bar) do + desc "A property that raises an exception when you try to change it" + def sync + raise ZeroDivisionError.new('bar') + end - def retrieve - :absent - end + def retrieve + :absent + end - def insync?(reference_value) - false - end + def insync?(reference_value) + false end end + end + stubProvider + end - resource = stubProvider.new :name => 'name', :foo => 1, :bar => 2 + describe "when an error occurs" do + before :each do + stub_provider = make_stub_provider + resource = stub_provider.new :name => 'name', :foo => 1, :bar => 2 resource.expects(:err).never @status = @harness.evaluate(resource) end -- cgit From 8f314f2bb14d61a2a26b67d002d90f84349b25b5 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Fri, 7 Jan 2011 15:39:52 -0800 Subject: (#5710) Removed unnecessary calls to insync? In the resource harness, we were calling insync? on all properties of a resource, even if those properties weren't being managed. This was unsafe. Changed to only call insync? on properties that are mentioned in the manifest. In addition, we discovered that the resource harness's computation of desired_values was incorrect, and was consulting the current system state rather than the desired state. Since this hash was (erroneously) only being consulted to see if it included :ensure, this didn't cause any obvious bugs. However it is now fixed. Paired-with: Jesse Wolfe --- lib/puppet/transaction/resource_harness.rb | 7 +++++-- spec/unit/transaction/resource_harness_spec.rb | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb index c259d3e05..c1b980632 100644 --- a/lib/puppet/transaction/resource_harness.rb +++ b/lib/puppet/transaction/resource_harness.rb @@ -37,7 +37,10 @@ class Puppet::Transaction::ResourceHarness current_values = current.to_hash historical_values = Puppet::Util::Storage.cache(resource).dup - desired_values = resource.to_resource.to_hash + desired_values = {} + resource.properties.each do |property| + desired_values[property.name] = property.should + end audited_params = (resource[:audit] || []).map { |p| p.to_sym } synced_params = [] @@ -55,7 +58,7 @@ class Puppet::Transaction::ResourceHarness elsif current_values[:ensure] != :absent work_order = resource.properties # Note: only the resource knows what order to apply changes in work_order.each do |param| - if !param.insync?(current_values[param.name]) + if desired_values[param.name] && !param.insync?(current_values[param.name]) events << apply_parameter(param, current_values[param.name], audited_params.include?(param.name), historical_values[param.name]) synced_params << param.name end diff --git a/spec/unit/transaction/resource_harness_spec.rb b/spec/unit/transaction/resource_harness_spec.rb index f0c360e22..104c19e85 100755 --- a/spec/unit/transaction/resource_harness_spec.rb +++ b/spec/unit/transaction/resource_harness_spec.rb @@ -125,6 +125,29 @@ describe Puppet::Transaction::ResourceHarness do end end + describe "when auditing" do + it "should not call insync? on parameters that are merely audited" do + stub_provider = make_stub_provider + resource = stub_provider.new :name => 'name', :audit => ['foo'] + resource.property(:foo).expects(:insync?).never + status = @harness.evaluate(resource) + status.events.each do |event| + event.status.should != 'failure' + end + end + + it "should be able to audit a file's group" do # see bug #5710 + test_file = tmpfile('foo') + File.open(test_file, 'w').close + resource = Puppet::Type.type(:file).new :path => test_file, :audit => ['group'], :backup => false + resource.expects(:err).never # make sure no exceptions get swallowed + status = @harness.evaluate(resource) + status.events.each do |event| + event.status.should != 'failure' + end + end + end + describe "when applying changes" do [false, true].each do |noop_mode|; describe (noop_mode ? "in noop mode" : "in normal mode") do [nil, '750'].each do |machine_state|; describe (machine_state ? "with a file initially present" : "with no file initially present") do -- cgit From a002231f45339da9b152162c2f48126d95e2e246 Mon Sep 17 00:00:00 2001 From: Jesse Wolfe Date: Mon, 10 Jan 2011 18:30:42 -0800 Subject: (#5171) Made filebucket able to perform diffs It is now possible to ask the filebucket to diff two files using a URL of the form: https://puppet/production/file_bucket_file/md5/{first file hash}?diff_with={second file hash} The returned diff is a string, the output of the "diff" command. Paired-with: Paul Berry --- lib/puppet/indirector/file_bucket_file/file.rb | 11 +++++- lib/puppet/indirector/indirection.rb | 2 +- lib/puppet/network/http/handler.rb | 6 ++- spec/unit/indirector/file_bucket_file/file_spec.rb | 45 ++++++++++++++++++++++ spec/unit/network/http/handler_spec.rb | 6 +++ 5 files changed, 67 insertions(+), 3 deletions(-) diff --git a/lib/puppet/indirector/file_bucket_file/file.rb b/lib/puppet/indirector/file_bucket_file/file.rb index 318858aaf..9d9cee793 100644 --- a/lib/puppet/indirector/file_bucket_file/file.rb +++ b/lib/puppet/indirector/file_bucket_file/file.rb @@ -15,7 +15,16 @@ module Puppet::FileBucketFile def find( request ) checksum, path = request_to_checksum_and_path( request ) - find_by_checksum( checksum, request.options ) + file = find_by_checksum( checksum, request.options ) + + if file && request.options[:diff_with] + hash_protocol = sumtype(checksum) + file2 = find_by_checksum( "{#{hash_protocol}}#{request.options[:diff_with]}", request.options ) + raise "could not find diff_with #{request.options[:diff_with]}" unless file2 + return `diff #{path_for(file).inspect}/contents #{path_for(file2).inspect}/contents` + end + + file end def save( request ) diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb index 309eed7b6..a010c4e40 100644 --- a/lib/puppet/indirector/indirection.rb +++ b/lib/puppet/indirector/indirection.rb @@ -191,7 +191,7 @@ class Puppet::Indirector::Indirection # Otherwise, return the result from the terminus, caching if appropriate. if ! request.ignore_terminus? and result = terminus.find(request) - result.expiration ||= self.expiration + result.expiration ||= self.expiration if result.respond_to?(:expiration) if cache? and request.use_cache? Puppet.info "Caching #{self.name} for #{request.key}" cache.save request(:save, result, *args) diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb index 61ae2d2fc..f22498b70 100644 --- a/lib/puppet/network/http/handler.rb +++ b/lib/puppet/network/http/handler.rb @@ -109,7 +109,11 @@ module Puppet::Network::HTTP::Handler format = format_to_use(request) set_content_type(response, format) - set_response(response, result.render(format)) + if result.respond_to?(:render) + set_response(response, result.render(format)) + else + set_response(response, result) + end end # Execute our search. diff --git a/spec/unit/indirector/file_bucket_file/file_spec.rb b/spec/unit/indirector/file_bucket_file/file_spec.rb index aa3ade6b6..7bf02b5b3 100755 --- a/spec/unit/indirector/file_bucket_file/file_spec.rb +++ b/spec/unit/indirector/file_bucket_file/file_spec.rb @@ -13,6 +13,51 @@ describe Puppet::FileBucketFile::File do Puppet::FileBucketFile::File.doc.should be_instance_of(String) end + describe "non-stubbing tests" do + include PuppetSpec::Files + + before do + Puppet[:bucketdir] = tmpdir('bucketdir') + end + + describe "when diffing files" do + def save_bucket_file(contents) + bucket_file = Puppet::FileBucket::File.new(contents) + bucket_file.save + bucket_file.checksum_data + end + + it "should generate an empty string if there is no diff" do + checksum = save_bucket_file("I'm the contents of a file") + Puppet::FileBucket::File.find("md5/#{checksum}", :diff_with => checksum).should == '' + end + + it "should generate a proper diff if there is a diff" do + checksum1 = save_bucket_file("foo\nbar\nbaz") + checksum2 = save_bucket_file("foo\nbiz\nbaz") + diff = Puppet::FileBucket::File.find("md5/#{checksum1}", :diff_with => checksum2) + diff.should == < biz +HERE + end + + it "should raise an exception if the hash to diff against isn't found" do + checksum = save_bucket_file("whatever") + bogus_checksum = "d1bf072d0e2c6e20e3fbd23f022089a1" + lambda { Puppet::FileBucket::File.find("md5/#{checksum}", :diff_with => bogus_checksum) }.should raise_error "could not find diff_with #{bogus_checksum}" + end + + it "should return nil if the hash to diff from isn't found" do + checksum = save_bucket_file("whatever") + bogus_checksum = "d1bf072d0e2c6e20e3fbd23f022089a1" + Puppet::FileBucket::File.find("md5/#{bogus_checksum}", :diff_with => checksum).should == nil + end + end + end + describe "when initializing" do it "should use the filebucket settings section" do Puppet.settings.expects(:use).with(:filebucket) diff --git a/spec/unit/network/http/handler_spec.rb b/spec/unit/network/http/handler_spec.rb index 76a9c5530..cdbce41f7 100755 --- a/spec/unit/network/http/handler_spec.rb +++ b/spec/unit/network/http/handler_spec.rb @@ -209,6 +209,12 @@ describe Puppet::Network::HTTP::Handler do @handler.do_find(@irequest, @request, @response) end + it "should pass the result through without rendering it if the result is a string" do + @model_class.stubs(:find).returns "foo" + @handler.expects(:set_response).with(@response, "foo") + @handler.do_find(@irequest, @request, @response) + end + it "should use the default status when a model find call succeeds" do @handler.expects(:set_response).with { |response, body, status| status.nil? } @handler.do_find(@irequest, @request, @response) -- cgit From 4efc98a88385815913c193615c5c50e2d0680411 Mon Sep 17 00:00:00 2001 From: Matt Robinson Date: Tue, 11 Jan 2011 15:50:13 -0800 Subject: maint: Remove unused Rakefile in spec directory The Rakefile just created rake tasks so that you could run all the specs in a subdirectory. However, rspec already allows you to just give the subdirectory as an argument and does the same thing, so these rake tasks were unecessary. --- spec/Rakefile | 91 ----------------------------------------------------------- 1 file changed, 91 deletions(-) delete mode 100644 spec/Rakefile diff --git a/spec/Rakefile b/spec/Rakefile deleted file mode 100644 index 28e1d8e79..000000000 --- a/spec/Rakefile +++ /dev/null @@ -1,91 +0,0 @@ -require File.join(File.dirname(__FILE__), "spec_helper.rb") -require 'rake' -require 'spec/rake/spectask' - -basedir = File.dirname(__FILE__) -puppetlibdir = File.join(basedir, "../lib") -puppettestlibdir = File.join(basedir, "../test/lib") -speclibdir = File.join(basedir, "lib") - -require 'find' - -include Find -include FileTest - -$exclusions = %W(lib) - -filemap = Hash.new { |hash, key| hash[key] = [] } - -allfiles = [] - -# First collect the entire file list. -find(".") do |f| - # Get rid of the leading ./ - f = f.sub(/^\.\//, '') - - file = File.basename(f) - dir = File.dirname(f) - - # Prune . directories and excluded dirs - if (file =~ /^\./ and f != ".") or $exclusions.include?(File.basename(file)) - prune - next - end - next if f == "." - next if dir == "." - - # If we're a ruby script, then add it to the list of files for that dir - if file =~ /\.rb$/ - allfiles << f - # Add it to all of the parent dirs, not just our own - parts = File.split(dir) - if parts[0] == "." - parts.shift - end - parts.each_with_index { |part, i| - path = File.join(parts[0..i]) - filemap[path] << f - } - end -end - - -libs = [puppetlibdir, puppettestlibdir, speclibdir] -desc "Run all specs" -Spec::Rake::SpecTask.new('all') do |t| - t.spec_files = FileList['integration/**/*.rb', 'unit/**/*.rb'] - t.libs = libs - t.spec_opts = ['--options', 'spec.opts'] -end - -task :default => [:all] - -# Now create a task for every directory -filemap.each do |dir, files| - ns = dir.gsub "/", ":" - - # First create a separate task for each file in the namespace. - namespace ns do - files.each do |file| - Spec::Rake::SpecTask.new(File.basename(file, '.rb').to_sym) do |t| - t.spec_files = [ file ] - t.libs = libs - t.spec_opts = ['--options', 'spec.opts'] - end - end - end - - # Then create a task that matches the directory itself. - Spec::Rake::SpecTask.new(dir) do |t| - if ENV["TESTFILES"] - t.spec_files = ENV["TESTFILES"].split(/\s+/) - else - t.spec_files = files.sort - end - t.libs = libs - t.spec_opts = ['--options', 'spec.opts'] - end - - # And alias it with a slash on the end - task(dir + "/" => dir) -end -- cgit From 08561b22920aa5eaa76addd8b0da8feb189e0d18 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Tue, 11 Jan 2011 14:56:14 -0800 Subject: (#5838) Refactored Puppet::Network::Rights#fail_on_deny Changed into a method that returns the exception to raised rather than raising it. Paired-with: Jesse Wolfe --- lib/puppet/network/rest_authconfig.rb | 18 ++++++------ lib/puppet/network/rights.rb | 20 ++++---------- spec/unit/network/rest_authconfig_spec.rb | 2 +- spec/unit/network/rights_spec.rb | 46 +++++++++++++++---------------- 4 files changed, 39 insertions(+), 47 deletions(-) diff --git a/lib/puppet/network/rest_authconfig.rb b/lib/puppet/network/rest_authconfig.rb index 7abe06956..1704ea0c1 100644 --- a/lib/puppet/network/rest_authconfig.rb +++ b/lib/puppet/network/rest_authconfig.rb @@ -38,14 +38,16 @@ module Puppet # fail_on_deny could as well be called in the XMLRPC context # with a ClientRequest. - @rights.fail_on_deny( - build_uri(request), - - :node => request.node, - :ip => request.ip, - :method => request.method, - :environment => request.environment, - :authenticated => request.authenticated) + if authorization_failure_exception = @rights.is_forbidden_and_why?( + build_uri(request), + :node => request.node, + :ip => request.ip, + :method => request.method, + :environment => request.environment, + :authenticated => request.authenticated) + Puppet.warning("Denying access: #{authorization_failure_exception}") + raise authorization_failure_exception + end end def initialize(file = nil, parsenow = true) diff --git a/lib/puppet/network/rights.rb b/lib/puppet/network/rights.rb index e3cd3179a..b2146494c 100755 --- a/lib/puppet/network/rights.rb +++ b/lib/puppet/network/rights.rb @@ -26,19 +26,10 @@ class Rights # Check that name is allowed or not def allowed?(name, *args) - begin - fail_on_deny(name, :node => args[0], :ip => args[1]) - rescue AuthorizationError - return false - rescue ArgumentError - # the namespace contract says we should raise this error - # if we didn't find the right acl - raise - end - true + !is_forbidden_and_why?(name, :node => args[0], :ip => args[1]) end - def fail_on_deny(name, args = {}) + def is_forbidden_and_why?(name, args = {}) res = :nomatch right = @rights.find do |acl| found = false @@ -49,7 +40,7 @@ class Rights args[:match] = match if (res = acl.allowed?(args[:node], args[:ip], args)) != :dunno # return early if we're allowed - return if res + return nil if res # we matched, select this acl found = true end @@ -70,13 +61,12 @@ class Rights error.file = right.file error.line = right.line end - Puppet.warning("Denying access: #{error}") else # there were no rights allowing/denying name # if name is not a path, let's throw - error = ArgumentError.new "Unknown namespace right '#{name}'" + raise ArgumentError.new "Unknown namespace right '#{name}'" end - raise error + error end def initialize diff --git a/spec/unit/network/rest_authconfig_spec.rb b/spec/unit/network/rest_authconfig_spec.rb index 06436e723..9892c2b25 100755 --- a/spec/unit/network/rest_authconfig_spec.rb +++ b/spec/unit/network/rest_authconfig_spec.rb @@ -47,7 +47,7 @@ describe Puppet::Network::RestAuthConfig do end it "should ask for authorization to the ACL subsystem" do - @acl.expects(:fail_on_deny).with("/path/to/resource", :node => "me", :ip => "127.0.0.1", :method => :save, :environment => :env, :authenticated => true) + @acl.expects(:is_forbidden_and_why?).with("/path/to/resource", :node => "me", :ip => "127.0.0.1", :method => :save, :environment => :env, :authenticated => true).returns(nil) @authconfig.allowed?(@request) end diff --git a/spec/unit/network/rights_spec.rb b/spec/unit/network/rights_spec.rb index 969fc189e..ca3f22464 100755 --- a/spec/unit/network/rights_spec.rb +++ b/spec/unit/network/rights_spec.rb @@ -155,19 +155,19 @@ describe Puppet::Network::Rights do Puppet::Network::Rights::Right.stubs(:new).returns(@pathacl) end - it "should delegate to fail_on_deny" do - @right.expects(:fail_on_deny).with("namespace", :node => "host.domain.com", :ip => "127.0.0.1") + it "should delegate to is_forbidden_and_why?" do + @right.expects(:is_forbidden_and_why?).with("namespace", :node => "host.domain.com", :ip => "127.0.0.1").returns(nil) @right.allowed?("namespace", "host.domain.com", "127.0.0.1") end - it "should return true if fail_on_deny doesn't fail" do - @right.stubs(:fail_on_deny) + it "should return true if is_forbidden_and_why? returns nil" do + @right.stubs(:is_forbidden_and_why?).returns(nil) @right.allowed?("namespace", :args).should be_true end - it "should return false if fail_on_deny raises an AuthorizationError" do - @right.stubs(:fail_on_deny).raises(Puppet::Network::AuthorizationError.new("forbidden")) + it "should return false if is_forbidden_and_why? returns an AuthorizationError" do + @right.stubs(:is_forbidden_and_why?).returns(Puppet::Network::AuthorizationError.new("forbidden")) @right.allowed?("namespace", :args1, :args2).should be_false end @@ -179,7 +179,7 @@ describe Puppet::Network::Rights do acl.expects(:match?).returns(true) acl.expects(:allowed?).with { |node,ip,h| node == "node" and ip == "ip" }.returns(true) - @right.fail_on_deny("namespace", { :node => "node", :ip => "ip" } ) + @right.is_forbidden_and_why?("namespace", { :node => "node", :ip => "ip" } ).should == nil end it "should then check for path rights if no namespace match" do @@ -195,7 +195,7 @@ describe Puppet::Network::Rights do acl.expects(:allowed?).never @pathacl.expects(:allowed?).returns(true) - @right.fail_on_deny("/path/to/there", {}) + @right.is_forbidden_and_why?("/path/to/there", {}).should == nil end it "should pass the match? return to allowed?" do @@ -204,12 +204,12 @@ describe Puppet::Network::Rights do @pathacl.expects(:match?).returns(:match) @pathacl.expects(:allowed?).with { |node,ip,h| h[:match] == :match }.returns(true) - @right.fail_on_deny("/path/to/there", {}) + @right.is_forbidden_and_why?("/path/to/there", {}).should == nil end describe "with namespace acls" do - it "should raise an error if this namespace right doesn't exist" do - lambda{ @right.fail_on_deny("namespace") }.should raise_error + it "should return an ArgumentError if this namespace right doesn't exist" do + lambda { @right.is_forbidden_and_why?("namespace") }.should raise_error(ArgumentError) end end @@ -235,7 +235,7 @@ describe Puppet::Network::Rights do @long_acl.expects(:allowed?).returns(true) @short_acl.expects(:allowed?).never - @right.fail_on_deny("/path/to/there/and/there", {}) + @right.is_forbidden_and_why?("/path/to/there/and/there", {}).should == nil end it "should select the first match that doesn't return :dunno" do @@ -248,7 +248,7 @@ describe Puppet::Network::Rights do @long_acl.expects(:allowed?).returns(:dunno) @short_acl.expects(:allowed?).returns(true) - @right.fail_on_deny("/path/to/there/and/there", {}) + @right.is_forbidden_and_why?("/path/to/there/and/there", {}).should == nil end it "should not select an ACL that doesn't match" do @@ -261,7 +261,7 @@ describe Puppet::Network::Rights do @long_acl.expects(:allowed?).never @short_acl.expects(:allowed?).returns(true) - @right.fail_on_deny("/path/to/there/and/there", {}) + @right.is_forbidden_and_why?("/path/to/there/and/there", {}).should == nil end it "should not raise an AuthorizationError if allowed" do @@ -270,7 +270,7 @@ describe Puppet::Network::Rights do @long_acl.stubs(:match?).returns(true) @long_acl.stubs(:allowed?).returns(true) - lambda { @right.fail_on_deny("/path/to/there/and/there", {}) }.should_not raise_error(Puppet::Network::AuthorizationError) + @right.is_forbidden_and_why?("/path/to/there/and/there", {}).should == nil end it "should raise an AuthorizationError if the match is denied" do @@ -279,11 +279,11 @@ describe Puppet::Network::Rights do @long_acl.stubs(:match?).returns(true) @long_acl.stubs(:allowed?).returns(false) - lambda{ @right.fail_on_deny("/path/to/there", {}) }.should raise_error(Puppet::Network::AuthorizationError) + @right.is_forbidden_and_why?("/path/to/there", {}).should be_instance_of(Puppet::Network::AuthorizationError) end it "should raise an AuthorizationError if no path match" do - lambda { @right.fail_on_deny("/nomatch", {}) }.should raise_error(Puppet::Network::AuthorizationError) + @right.is_forbidden_and_why?("/nomatch", {}).should be_instance_of(Puppet::Network::AuthorizationError) end end @@ -309,7 +309,7 @@ describe Puppet::Network::Rights do @regex_acl1.expects(:allowed?).returns(true) @regex_acl2.expects(:allowed?).never - @right.fail_on_deny("/files/repository/myfile/other", {}) + @right.is_forbidden_and_why?("/files/repository/myfile/other", {}).should == nil end it "should select the first match that doesn't return :dunno" do @@ -322,7 +322,7 @@ describe Puppet::Network::Rights do @regex_acl1.expects(:allowed?).returns(:dunno) @regex_acl2.expects(:allowed?).returns(true) - @right.fail_on_deny("/files/repository/myfile/other", {}) + @right.is_forbidden_and_why?("/files/repository/myfile/other", {}).should == nil end it "should not select an ACL that doesn't match" do @@ -335,7 +335,7 @@ describe Puppet::Network::Rights do @regex_acl1.expects(:allowed?).never @regex_acl2.expects(:allowed?).returns(true) - @right.fail_on_deny("/files/repository/myfile/other", {}) + @right.is_forbidden_and_why?("/files/repository/myfile/other", {}).should == nil end it "should not raise an AuthorizationError if allowed" do @@ -344,15 +344,15 @@ describe Puppet::Network::Rights do @regex_acl1.stubs(:match?).returns(true) @regex_acl1.stubs(:allowed?).returns(true) - lambda { @right.fail_on_deny("/files/repository/myfile/other", {}) }.should_not raise_error(Puppet::Network::AuthorizationError) + @right.is_forbidden_and_why?("/files/repository/myfile/other", {}).should == nil end it "should raise an error if no regex acl match" do - lambda{ @right.fail_on_deny("/path", {}) }.should raise_error(Puppet::Network::AuthorizationError) + @right.is_forbidden_and_why?("/path", {}).should be_instance_of(Puppet::Network::AuthorizationError) end it "should raise an AuthorizedError on deny" do - lambda { @right.fail_on_deny("/path", {}) }.should raise_error(Puppet::Network::AuthorizationError) + @right.is_forbidden_and_why?("/path", {}).should be_instance_of(Puppet::Network::AuthorizationError) end end -- cgit From 2b9b7a5f7fe4b673f0d1fba9fb523cc0e2e34fa5 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Mon, 10 Jan 2011 14:17:14 -0800 Subject: (#5838) Refactored error handling logic into find_in_cache. Moved the error handling logic surrounding the call to Puppet::Indirector::Indirection#find_in_cache() into the find_in_cache() method itself, so that won't need to be duplicated when find_in_cache() is called from elsewhere. Paired-with: Jesse Wolfe --- lib/puppet/indirector/indirection.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb index a010c4e40..4341f7cb2 100644 --- a/lib/puppet/indirector/indirection.rb +++ b/lib/puppet/indirector/indirection.rb @@ -180,13 +180,8 @@ class Puppet::Indirector::Indirection request = request(:find, key, *args) terminus = prepare(request) - begin - if result = find_in_cache(request) - return result - end - rescue => detail - puts detail.backtrace if Puppet[:trace] - Puppet.err "Cached #{self.name} for #{request.key} failed: #{detail}" + if result = find_in_cache(request) + return result end # Otherwise, return the result from the terminus, caching if appropriate. @@ -213,6 +208,10 @@ class Puppet::Indirector::Indirection Puppet.debug "Using cached #{self.name} for #{request.key}" cached + rescue => detail + puts detail.backtrace if Puppet[:trace] + Puppet.err "Cached #{self.name} for #{request.key} failed: #{detail}" + nil end # Remove something via the terminus. -- cgit From c514c641d0c0090be29252dcc773385248d3fe93 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Mon, 10 Jan 2011 17:05:38 -0800 Subject: (#5838) Added support for HEAD requests to the indirector. Added the ability for the indirector to handle REST HEAD requests. These are done using a new indirector method, head(), which should return true if find() would return a result and false if find() would return nil. Access control for the head method is the union of that for the find and save methods. That is, if either find or save is allowed, then head is allowed. This is necessary so that users will not have to change their authconfig to take advantage of the new feature. Paired-with: Jesse Wolfe --- lib/puppet/indirector.rb | 4 ++ lib/puppet/indirector/indirection.rb | 11 +++++ lib/puppet/indirector/rest.rb | 21 +++++++++- lib/puppet/network/http/api/v1.rb | 3 ++ lib/puppet/network/http/handler.rb | 11 +++++ lib/puppet/network/rest_authconfig.rb | 12 +----- lib/puppet/network/rights.rb | 24 +++++++++++ spec/unit/indirector/file_server_spec.rb | 13 +++--- spec/unit/indirector/indirection_spec.rb | 69 +++++++++++++++++++++++++++++++ spec/unit/indirector/rest_spec.rb | 36 ++++++++++++++++ spec/unit/indirector_spec.rb | 5 +++ spec/unit/network/http/api/v1_spec.rb | 4 ++ spec/unit/network/http/handler_spec.rb | 33 +++++++++++++++ spec/unit/network/rest_authconfig_spec.rb | 2 +- spec/unit/network/rights_spec.rb | 20 +++++++++ 15 files changed, 247 insertions(+), 21 deletions(-) diff --git a/lib/puppet/indirector.rb b/lib/puppet/indirector.rb index 5b737578b..e6472f4d9 100644 --- a/lib/puppet/indirector.rb +++ b/lib/puppet/indirector.rb @@ -50,6 +50,10 @@ module Puppet::Indirector indirection.find(*args) end + def head(*args) + indirection.head(*args) + end + def destroy(*args) indirection.destroy(*args) end diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb index 4341f7cb2..ec147ec69 100644 --- a/lib/puppet/indirector/indirection.rb +++ b/lib/puppet/indirector/indirection.rb @@ -198,6 +198,17 @@ class Puppet::Indirector::Indirection nil end + # Search for an instance in the appropriate terminus, and return a + # boolean indicating whether the instance was found. + def head(key, *args) + request = request(:head, key, *args) + terminus = prepare(request) + + # Look in the cache first, then in the terminus. Force the result + # to be a boolean. + !!(find_in_cache(request) || terminus.head(request)) + end + def find_in_cache(request) # See if our instance is in the cache and up to date. return nil unless cache? and ! request.ignore_cache? and cached = cache.find(request) diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb index eb41ff3b1..e50dc68ae 100644 --- a/lib/puppet/indirector/rest.rb +++ b/lib/puppet/indirector/rest.rb @@ -53,11 +53,15 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus end else # Raise the http error if we didn't get a 'success' of some kind. - message = "Error #{response.code} on SERVER: #{(response.body||'').empty? ? response.message : uncompress_body(response)}" - raise Net::HTTPError.new(message, response) + raise convert_to_http_error(response) end end + def convert_to_http_error(response) + message = "Error #{response.code} on SERVER: #{(response.body||'').empty? ? response.message : uncompress_body(response)}" + Net::HTTPError.new(message, response) + end + # Provide appropriate headers. def headers add_accept_encoding({"Accept" => model.supported_formats.join(", ")}) @@ -73,6 +77,19 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus result end + def head(request) + response = network(request).head(indirection2uri(request), headers) + case response.code + when "404" + return false + when /^2/ + return true + else + # Raise the http error if we didn't get a 'success' of some kind. + raise convert_to_http_error(response) + end + end + def search(request) unless result = deserialize(network(request).get(indirection2uri(request), headers), true) return [] diff --git a/lib/puppet/network/http/api/v1.rb b/lib/puppet/network/http/api/v1.rb index dd4612a14..8aa1f0ee1 100644 --- a/lib/puppet/network/http/api/v1.rb +++ b/lib/puppet/network/http/api/v1.rb @@ -13,6 +13,9 @@ module Puppet::Network::HTTP::API::V1 }, "DELETE" => { :singular => :destroy + }, + "HEAD" => { + :singular => :head } } diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb index f22498b70..9e9356b2f 100644 --- a/lib/puppet/network/http/handler.rb +++ b/lib/puppet/network/http/handler.rb @@ -116,6 +116,17 @@ module Puppet::Network::HTTP::Handler end end + # Execute our head. + def do_head(indirection_request, request, response) + unless indirection_request.model.head(indirection_request.key, indirection_request.to_hash) + Puppet.info("Could not find #{indirection_request.indirection_name} for '#{indirection_request.key}'") + return do_exception(response, "Could not find #{indirection_request.indirection_name} #{indirection_request.key}", 404) + end + + # No need to set a response because no response is expected from a + # HEAD request. All we need to do is not die. + end + # Execute our search. def do_search(indirection_request, request, response) result = indirection_request.model.search(indirection_request.key, indirection_request.to_hash) diff --git a/lib/puppet/network/rest_authconfig.rb b/lib/puppet/network/rest_authconfig.rb index 1704ea0c1..7a6147a82 100644 --- a/lib/puppet/network/rest_authconfig.rb +++ b/lib/puppet/network/rest_authconfig.rb @@ -38,13 +38,7 @@ module Puppet # fail_on_deny could as well be called in the XMLRPC context # with a ClientRequest. - if authorization_failure_exception = @rights.is_forbidden_and_why?( - build_uri(request), - :node => request.node, - :ip => request.ip, - :method => request.method, - :environment => request.environment, - :authenticated => request.authenticated) + if authorization_failure_exception = @rights.is_request_forbidden_and_why?(request) Puppet.warning("Denying access: #{authorization_failure_exception}") raise authorization_failure_exception end @@ -90,9 +84,5 @@ module Puppet end @rights.restrict_authenticated(acl[:acl], acl[:authenticated]) unless acl[:authenticated].nil? end - - def build_uri(request) - "/#{request.indirection_name}/#{request.key}" - end end end diff --git a/lib/puppet/network/rights.rb b/lib/puppet/network/rights.rb index b2146494c..00ee04f8d 100755 --- a/lib/puppet/network/rights.rb +++ b/lib/puppet/network/rights.rb @@ -29,6 +29,30 @@ class Rights !is_forbidden_and_why?(name, :node => args[0], :ip => args[1]) end + def is_request_forbidden_and_why?(request) + methods_to_check = if request.method == :head + # :head is ok if either :find or :save is ok. + [:find, :save] + else + [request.method] + end + authorization_failure_exceptions = methods_to_check.map do |method| + is_forbidden_and_why?("/#{request.indirection_name}/#{request.key}", + :node => request.node, + :ip => request.ip, + :method => method, + :environment => request.environment, + :authenticated => request.authenticated) + end + if authorization_failure_exceptions.include? nil + # One of the methods we checked is ok, therefore this request is ok. + nil + else + # Just need to return any of the failure exceptions. + authorization_failure_exceptions.first + end + end + def is_forbidden_and_why?(name, args = {}) res = :nomatch right = @rights.find do |acl| diff --git a/spec/unit/indirector/file_server_spec.rb b/spec/unit/indirector/file_server_spec.rb index 686f79a24..eafcf2bb7 100755 --- a/spec/unit/indirector/file_server_spec.rb +++ b/spec/unit/indirector/file_server_spec.rb @@ -229,6 +229,12 @@ describe Puppet::Indirector::FileServer do describe "when checking authorization" do before do @request.method = :find + + @mount = stub 'mount' + @configuration.stubs(:split_path).with(@request).returns([@mount, "rel/path"]) + @request.stubs(:node).returns("mynode") + @request.stubs(:ip).returns("myip") + @mount.stubs(:allowed?).with("mynode", "myip").returns "something" end it "should return false when destroying" do @@ -254,13 +260,6 @@ describe Puppet::Indirector::FileServer do end it "should return the results of asking the mount whether the node and IP are authorized" do - @mount = stub 'mount' - @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"]) - - @request.stubs(:node).returns("mynode") - @request.stubs(:ip).returns("myip") - @mount.expects(:allowed?).with("mynode", "myip").returns "something" - @file_server.authorized?(@request).should == "something" end end diff --git a/spec/unit/indirector/indirection_spec.rb b/spec/unit/indirector/indirection_spec.rb index 1e774fb2e..a910cb6f2 100755 --- a/spec/unit/indirector/indirection_spec.rb +++ b/spec/unit/indirector/indirection_spec.rb @@ -383,6 +383,75 @@ describe Puppet::Indirector::Indirection do end end + describe "and doing a head operation" do + before { @method = :head } + + it_should_behave_like "Indirection Delegator" + it_should_behave_like "Delegation Authorizer" + + it "should return true if the head method returned true" do + @terminus.expects(:head).returns(true) + @indirection.head("me").should == true + end + + it "should return false if the head method returned false" do + @terminus.expects(:head).returns(false) + @indirection.head("me").should == false + end + + describe "when caching is enabled" do + before do + @indirection.cache_class = :cache_terminus + @cache_class.stubs(:new).returns(@cache) + + @instance.stubs(:expired?).returns false + end + + it "should first look in the cache for an instance" do + @terminus.stubs(:find).never + @terminus.stubs(:head).never + @cache.expects(:find).returns @instance + + @indirection.head("/my/key").should == true + end + + it "should not save to the cache" do + @cache.expects(:find).returns nil + @cache.expects(:save).never + @terminus.expects(:head).returns true + @indirection.head("/my/key").should == true + end + + it "should not fail if the cache fails" do + @terminus.stubs(:head).returns true + + @cache.expects(:find).raises ArgumentError + lambda { @indirection.head("/my/key") }.should_not raise_error + end + + it "should look in the main terminus if the cache fails" do + @terminus.expects(:head).returns true + @cache.expects(:find).raises ArgumentError + @indirection.head("/my/key").should == true + end + + it "should send a debug log if it is using the cached object" do + Puppet.expects(:debug) + @cache.stubs(:find).returns @instance + + @indirection.head("/my/key") + end + + it "should not accept the cached object if it is expired" do + @instance.stubs(:expired?).returns true + + @cache.stubs(:find).returns @instance + @terminus.stubs(:head).returns false + @indirection.head("/my/key").should == false + end + end + end + describe "and storing a model instance" do before { @method = :save } diff --git a/spec/unit/indirector/rest_spec.rb b/spec/unit/indirector/rest_spec.rb index 5f0fe363a..7bc1cc513 100755 --- a/spec/unit/indirector/rest_spec.rb +++ b/spec/unit/indirector/rest_spec.rb @@ -282,6 +282,42 @@ describe Puppet::Indirector::REST do end end + describe "when doing a head" do + before :each do + @connection = stub('mock http connection', :head => @response) + @searcher.stubs(:network).returns(@connection) + + # Use a key with spaces, so we can test escaping + @request = Puppet::Indirector::Request.new(:foo, :head, "foo bar") + end + + it "should call the HEAD http method on a network connection" do + @searcher.expects(:network).returns @connection + @connection.expects(:head).returns @response + @searcher.head(@request) + end + + it "should return true if there was a successful http response" do + @connection.expects(:head).returns @response + @response.stubs(:code).returns "200" + + @searcher.head(@request).should == true + end + + it "should return false if there was a successful http response" do + @connection.expects(:head).returns @response + @response.stubs(:code).returns "404" + + @searcher.head(@request).should == false + end + + it "should use the URI generated by the Handler module" do + @searcher.expects(:indirection2uri).with(@request).returns "/my/uri" + @connection.expects(:head).with { |path, args| path == "/my/uri" }.returns(@response) + @searcher.head(@request) + end + end + describe "when doing a search" do before :each do @connection = stub('mock http connection', :get => @response) diff --git a/spec/unit/indirector_spec.rb b/spec/unit/indirector_spec.rb index acbfc1726..6f44764da 100755 --- a/spec/unit/indirector_spec.rb +++ b/spec/unit/indirector_spec.rb @@ -118,6 +118,11 @@ describe Puppet::Indirector, "when redirecting a model" do it_should_behave_like "Delegated Indirection Method" end + describe "when performing a head request" do + before { @method = :head } + it_should_behave_like "Delegated Indirection Method" + end + # This is an instance method, so it behaves a bit differently. describe "when saving instances via the model" do before do diff --git a/spec/unit/network/http/api/v1_spec.rb b/spec/unit/network/http/api/v1_spec.rb index c593242c0..23a291cf3 100644 --- a/spec/unit/network/http/api/v1_spec.rb +++ b/spec/unit/network/http/api/v1_spec.rb @@ -68,6 +68,10 @@ describe Puppet::Network::HTTP::API::V1 do @tester.uri2indirection("GET", "/env/foo/bar", {}).method.should == :find end + it "should choose 'head' as the indirection method if the http method is a HEAD and the indirection name is singular" do + @tester.uri2indirection("HEAD", "/env/foo/bar", {}).method.should == :head + end + it "should choose 'search' as the indirection method if the http method is a GET and the indirection name is plural" do @tester.uri2indirection("GET", "/env/foos/bar", {}).method.should == :search end diff --git a/spec/unit/network/http/handler_spec.rb b/spec/unit/network/http/handler_spec.rb index cdbce41f7..8464ae68e 100755 --- a/spec/unit/network/http/handler_spec.rb +++ b/spec/unit/network/http/handler_spec.rb @@ -256,6 +256,39 @@ describe Puppet::Network::HTTP::Handler do end end + describe "when performing head operation" do + before do + @irequest = stub 'indirection_request', :method => :head, :indirection_name => "my_handler", :to_hash => {}, :key => "my_result", :model => @model_class + + @model_class.stubs(:head).returns true + end + + it "should use the indirection request to find the model class" do + @irequest.expects(:model).returns @model_class + + @handler.do_head(@irequest, @request, @response) + end + + it "should use the escaped request key" do + @model_class.expects(:head).with do |key, args| + key == "my_result" + end.returns true + @handler.do_head(@irequest, @request, @response) + end + + it "should not generate a response when a model head call succeeds" do + @handler.expects(:set_response).never + @handler.do_head(@irequest, @request, @response) + end + + it "should return a 404 when the model head call returns false" do + @model_class.stubs(:name).returns "my name" + @handler.expects(:set_response).with { |response, body, status| status == 404 } + @model_class.stubs(:head).returns(false) + @handler.do_head(@irequest, @request, @response) + end + end + describe "when searching for model instances" do before do @irequest = stub 'indirection_request', :method => :find, :indirection_name => "my_handler", :to_hash => {}, :key => "key", :model => @model_class diff --git a/spec/unit/network/rest_authconfig_spec.rb b/spec/unit/network/rest_authconfig_spec.rb index 9892c2b25..d629f8670 100755 --- a/spec/unit/network/rest_authconfig_spec.rb +++ b/spec/unit/network/rest_authconfig_spec.rb @@ -47,7 +47,7 @@ describe Puppet::Network::RestAuthConfig do end it "should ask for authorization to the ACL subsystem" do - @acl.expects(:is_forbidden_and_why?).with("/path/to/resource", :node => "me", :ip => "127.0.0.1", :method => :save, :environment => :env, :authenticated => true).returns(nil) + @acl.expects(:is_request_forbidden_and_why?).with(@request).returns(nil) @authconfig.allowed?(@request) end diff --git a/spec/unit/network/rights_spec.rb b/spec/unit/network/rights_spec.rb index ca3f22464..3b9e48374 100755 --- a/spec/unit/network/rights_spec.rb +++ b/spec/unit/network/rights_spec.rb @@ -9,6 +9,26 @@ describe Puppet::Network::Rights do @right = Puppet::Network::Rights.new end + describe "when validating a :head request" do + [:find, :save].each do |allowed_method| + it "should allow the request if only #{allowed_method} is allowed" do + rights = Puppet::Network::Rights.new + rights.newright("/") + rights.allow("/", "*") + rights.restrict_method("/", allowed_method) + rights.restrict_authenticated("/", :any) + request = Puppet::Indirector::Request.new(:indirection_name, :head, "key") + rights.is_request_forbidden_and_why?(request).should == nil + end + end + + it "should disallow the request if neither :find nor :save is allowed" do + rights = Puppet::Network::Rights.new + request = Puppet::Indirector::Request.new(:indirection_name, :head, "key") + rights.is_request_forbidden_and_why?(request).should be_instance_of(Puppet::Network::AuthorizationError) + end + end + [:allow, :deny, :restrict_method, :restrict_environment, :restrict_authenticated].each do |m| it "should have a #{m} method" do @right.should respond_to(m) -- cgit From 9cfd3d58699b0d0c3ab53cb37226cade84d7ec64 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Tue, 11 Jan 2011 13:36:05 -0800 Subject: (#5838) Reworked file dipper spec to perform less stubbing. This is in preparation for adding more functionality to the file dipper. Paired-with: Jesse Wolfe --- spec/unit/file_bucket/dipper_spec.rb | 114 ++++++++++------------------------- 1 file changed, 32 insertions(+), 82 deletions(-) diff --git a/spec/unit/file_bucket/dipper_spec.rb b/spec/unit/file_bucket/dipper_spec.rb index 799e899e7..86ad01f83 100755 --- a/spec/unit/file_bucket/dipper_spec.rb +++ b/spec/unit/file_bucket/dipper_spec.rb @@ -2,121 +2,71 @@ require File.dirname(__FILE__) + '/../../spec_helper' +require 'pathname' + require 'puppet/file_bucket/dipper' describe Puppet::FileBucket::Dipper do - before do - ['/my/file'].each do |x| - Puppet::FileBucket::Dipper.any_instance.stubs(:absolutize_path).with(x).returns(x) - end + include PuppetSpec::Files + + def make_tmp_file(contents) + file = tmpfile("file_bucket_file") + File.open(file, 'w') { |f| f.write(contents) } + file end it "should fail in an informative way when there are failures backing up to the server" do - File.stubs(:exist?).returns true - File.stubs(:read).returns "content" - @dipper = Puppet::FileBucket::Dipper.new(:Path => "/my/bucket") - filemock = stub "bucketfile" - Puppet::FileBucket::File.stubs(:new).returns(filemock) - filemock.expects(:name).returns "name" - filemock.expects(:save).raises ArgumentError + file = make_tmp_file('contents') + Puppet::FileBucket::File.any_instance.expects(:save).raises ArgumentError - lambda { @dipper.backup("/my/file") }.should raise_error(Puppet::Error) + lambda { @dipper.backup(file) }.should raise_error(Puppet::Error) end it "should backup files to a local bucket" do - @dipper = Puppet::FileBucket::Dipper.new( - :Path => "/my/bucket" - ) - - File.stubs(:exist?).returns true - File.stubs(:read).with("/my/file").returns "my contents" - - bucketfile = stub "bucketfile" - bucketfile.stubs(:name).returns('md5/DIGEST123') - bucketfile.stubs(:checksum_data).returns("DIGEST123") - bucketfile.expects(:save).with('md5/DIGEST123') - + @dipper = Puppet::FileBucket::Dipper.new(:Path => "/my/bucket") - Puppet::FileBucket::File.stubs(:new).with( - - "my contents", - :bucket_path => '/my/bucket', - - :path => '/my/file' - ).returns(bucketfile) + file = make_tmp_file('my contents') + checksum = Digest::MD5.hexdigest('my contents') - @dipper.backup("/my/file").should == "DIGEST123" + Puppet::FileBucket::File.any_instance.expects(:save) + @dipper.backup(file).should == checksum end it "should retrieve files from a local bucket" do - @dipper = Puppet::FileBucket::Dipper.new( - :Path => "/my/bucket" - ) - - File.stubs(:exist?).returns true - File.stubs(:read).with("/my/file").returns "my contents" + @dipper = Puppet::FileBucket::Dipper.new(:Path => "/my/bucket") - bucketfile = stub "bucketfile" - bucketfile.stubs(:to_s).returns "Content" + checksum = Digest::MD5.hexdigest('my contents') Puppet::FileBucket::File.expects(:find).with{|x,opts| - x == 'md5/DIGEST123' - }.returns(bucketfile) + x == "md5/#{checksum}" + }.returns(Puppet::FileBucket::File.new('my contents')) - @dipper.getfile("DIGEST123").should == "Content" + @dipper.getfile(checksum).should == 'my contents' end it "should backup files to a remote server" do + @dipper = Puppet::FileBucket::Dipper.new(:Server => "puppetmaster", :Port => "31337") - @dipper = Puppet::FileBucket::Dipper.new( - - :Server => "puppetmaster", - - :Port => "31337" - ) - - File.stubs(:exist?).returns true - File.stubs(:read).with("/my/file").returns "my contents" + file = make_tmp_file('my contents') + checksum = Digest::MD5.hexdigest('my contents') - bucketfile = stub "bucketfile" - bucketfile.stubs(:name).returns('md5/DIGEST123') - bucketfile.stubs(:checksum_data).returns("DIGEST123") - bucketfile.expects(:save).with('https://puppetmaster:31337/production/file_bucket_file/md5/DIGEST123') + real_path = Pathname.new(file).realpath + Puppet::FileBucket::File.any_instance.expects(:save).with("https://puppetmaster:31337/production/file_bucket_file/md5/#{checksum}/#{real_path}") - Puppet::FileBucket::File.stubs(:new).with( - - "my contents", - :bucket_path => nil, - - :path => '/my/file' - ).returns(bucketfile) - - @dipper.backup("/my/file").should == "DIGEST123" + @dipper.backup(file).should == checksum end it "should retrieve files from a remote server" do + @dipper = Puppet::FileBucket::Dipper.new(:Server => "puppetmaster", :Port => "31337") - @dipper = Puppet::FileBucket::Dipper.new( - - :Server => "puppetmaster", - - :Port => "31337" - ) - - File.stubs(:exist?).returns true - File.stubs(:read).with("/my/file").returns "my contents" - - bucketfile = stub "bucketfile" - bucketfile.stubs(:to_s).returns "Content" + checksum = Digest::MD5.hexdigest('my contents') Puppet::FileBucket::File.expects(:find).with{|x,opts| - x == 'https://puppetmaster:31337/production/file_bucket_file/md5/DIGEST123' - }.returns(bucketfile) + x == "https://puppetmaster:31337/production/file_bucket_file/md5/#{checksum}" + }.returns(Puppet::FileBucket::File.new('my contents')) - @dipper.getfile("DIGEST123").should == "Content" + @dipper.getfile(checksum).should == "my contents" end - - end -- cgit From 89f56920f26544f7c5aa97785567b193034db151 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Tue, 11 Jan 2011 15:24:10 -0800 Subject: (#5838) Implemented the "head" method for FileBucketFile::File terminus. In order to do this it was necessary to refactor FileBucketFile to untangle responsibilities for computing paths, reading files, etc. In the process, removed speculative generalizations and unused functionality. Paired-with: Jesse Wolfe --- lib/puppet/file_bucket/dipper.rb | 2 +- lib/puppet/file_bucket/file.rb | 108 ++---------- lib/puppet/indirector/file_bucket_file/file.rb | 121 ++++---------- spec/unit/file_bucket/dipper_spec.rb | 2 +- spec/unit/file_bucket/file_spec.rb | 108 ++---------- spec/unit/indirector/file_bucket_file/file_spec.rb | 185 ++------------------- 6 files changed, 81 insertions(+), 445 deletions(-) diff --git a/lib/puppet/file_bucket/dipper.rb b/lib/puppet/file_bucket/dipper.rb index dbfcdcd43..b012a8681 100644 --- a/lib/puppet/file_bucket/dipper.rb +++ b/lib/puppet/file_bucket/dipper.rb @@ -33,7 +33,7 @@ class Puppet::FileBucket::Dipper raise(ArgumentError, "File #{file} does not exist") unless ::File.exist?(file) contents = ::File.read(file) begin - file_bucket_file = Puppet::FileBucket::File.new(contents, :bucket_path => @local_path, :path => absolutize_path(file) ) + file_bucket_file = Puppet::FileBucket::File.new(contents, :bucket_path => @local_path) dest_path = "#{@rest_path}#{file_bucket_file.name}" file_bucket_file.save(dest_path) diff --git a/lib/puppet/file_bucket/file.rb b/lib/puppet/file_bucket/file.rb index 96fd8e225..08c0329f1 100644 --- a/lib/puppet/file_bucket/file.rb +++ b/lib/puppet/file_bucket/file.rb @@ -1,10 +1,9 @@ require 'puppet/file_bucket' require 'puppet/indirector' require 'puppet/util/checksums' +require 'digest/md5' class Puppet::FileBucket::File - include Puppet::Util::Checksums - # This class handles the abstract notion of a file in a filebucket. # There are mechanisms to save and load this file locally and remotely in puppet/indirector/filebucketfile/* # There is a compatibility class that emulates pre-indirector filebuckets in Puppet::FileBucket::Dipper @@ -12,71 +11,27 @@ class Puppet::FileBucket::File require 'puppet/file_bucket/file/indirection_hooks' indirects :file_bucket_file, :terminus_class => :file, :extend => Puppet::FileBucket::File::IndirectionHooks - attr :path, true - attr :paths, true - attr :contents, true - attr :checksum_type - attr :bucket_path, true - - def self.default_checksum_type - "md5" - end + attr :contents + attr :bucket_path def initialize( contents, options = {} ) - @bucket_path = options[:bucket_path] - @path = options[:path] - @paths = options[:paths] || [] - - @checksum = options[:checksum] - @checksum_type = options[:checksum_type] - - self.contents = contents - - yield(self) if block_given? - - validate! - end + raise ArgumentError if !contents.is_a?(String) + @contents = contents - def validate! - validate_checksum_type!(checksum_type) - validate_checksum!(checksum) if checksum + @bucket_path = options.delete(:bucket_path) + raise ArgumentError if options != {} end - def contents=(str) - raise "You may not change the contents of a FileBucket File" if @contents - validate_content!(str) - @contents = str + def checksum_type + 'md5' end def checksum - return @checksum if @checksum - @checksum = calculate_checksum if contents - @checksum - end - - def checksum=(checksum) - validate_checksum!(checksum) - @checksum = checksum - end - - def checksum_type=( new_checksum_type ) - @checksum = nil - @checksum_type = new_checksum_type - end - - def checksum_type - unless @checksum_type - if @checksum - @checksum_type = sumtype(checksum) - else - @checksum_type = self.class.default_checksum_type - end - end - @checksum_type + "{#{checksum_type}}#{checksum_data}" end def checksum_data - sumdata(checksum) + @checksum_data ||= Digest::MD5.hexdigest(contents) end def to_s @@ -84,18 +39,7 @@ class Puppet::FileBucket::File end def name - [checksum_type, checksum_data, path].compact.join('/') - end - - def name=(name) - data = name.split('/',3) - self.path = data.pop - @checksum_type = nil - self.checksum = "{#{data[0]}}#{data[1]}" - end - - def conflict_check? - true + "#{checksum_type}/#{checksum_data}" end def self.from_s( contents ) @@ -103,34 +47,10 @@ class Puppet::FileBucket::File end def to_pson - hash = { "contents" => contents } - hash["path"] = @path if @path - hash.to_pson + { "contents" => contents }.to_pson end def self.from_pson( pson ) - self.new( pson["contents"], :path => pson["path"] ) - end - - private - - def calculate_checksum - "{#{checksum_type}}" + send(checksum_type, contents) - end - - def validate_content!(content) - raise ArgumentError, "Contents must be a string" if content and ! content.is_a?(String) - end - - def validate_checksum!(new_checksum) - newtype = sumtype(new_checksum) - - unless sumdata(new_checksum) == (calc_sum = send(newtype, contents)) - raise Puppet::Error, "Checksum #{new_checksum} does not match contents #{calc_sum}" - end - end - - def validate_checksum_type!(type) - raise ArgumentError, "Invalid checksum type #{type}" unless respond_to?(type) + self.new( pson["contents"] ) end end diff --git a/lib/puppet/indirector/file_bucket_file/file.rb b/lib/puppet/indirector/file_bucket_file/file.rb index 9d9cee793..38e0be6e9 100644 --- a/lib/puppet/indirector/file_bucket_file/file.rb +++ b/lib/puppet/indirector/file_bucket_file/file.rb @@ -14,25 +14,31 @@ module Puppet::FileBucketFile end def find( request ) - checksum, path = request_to_checksum_and_path( request ) - file = find_by_checksum( checksum, request.options ) + checksum = request_to_checksum( request ) + file_path = path_for(request.options[:bucket_path], checksum, 'contents') - if file && request.options[:diff_with] + return nil unless ::File.exists?(file_path) + + if request.options[:diff_with] hash_protocol = sumtype(checksum) - file2 = find_by_checksum( "{#{hash_protocol}}#{request.options[:diff_with]}", request.options ) - raise "could not find diff_with #{request.options[:diff_with]}" unless file2 - return `diff #{path_for(file).inspect}/contents #{path_for(file2).inspect}/contents` + file2_path = path_for(request.options[:bucket_path], request.options[:diff_with], 'contents') + raise "could not find diff_with #{request.options[:diff_with]}" unless ::File.exists?(file2_path) + return `diff #{file_path.inspect} #{file2_path.inspect}` + else + contents = ::File.read file_path + Puppet.info "FileBucket read #{checksum}" + model.new(contents) end + end - file + def head(request) + checksum = request_to_checksum(request) + file_path = path_for(request.options[:bucket_path], checksum, 'contents') + ::File.exists?(file_path) end def save( request ) - checksum, path = request_to_checksum_and_path( request ) - instance = request.instance - instance.checksum = checksum if checksum - instance.path = path if path save_to_disk(instance) instance.to_s @@ -40,66 +46,41 @@ module Puppet::FileBucketFile private - def find_by_checksum( checksum, options ) - model.new( nil, :checksum => checksum ) do |bucket_file| - bucket_file.bucket_path = options[:bucket_path] - filename = contents_path_for( bucket_file ) - - return nil if ! ::File.exist? filename - - begin - contents = ::File.read filename - Puppet.info "FileBucket read #{bucket_file.checksum}" - rescue RuntimeError => e - raise Puppet::Error, "file could not be read: #{e.message}" - end - - if ::File.exist?(paths_path_for( bucket_file) ) - ::File.open(paths_path_for( bucket_file) ) do |f| - bucket_file.paths = f.readlines.map { |l| l.chomp } - end - end - - bucket_file.contents = contents - end - end - def save_to_disk( bucket_file ) - # If the file already exists, just return the md5 sum. - if ::File.exist?(contents_path_for( bucket_file) ) + filename = path_for(bucket_file.bucket_path, bucket_file.checksum_data, 'contents') + dirname = path_for(bucket_file.bucket_path, bucket_file.checksum_data) + + # If the file already exists, do nothing. + if ::File.exist?(filename) verify_identical_file!(bucket_file) else # Make the directories if necessary. - unless ::File.directory?( path_for( bucket_file) ) + unless ::File.directory?(dirname) Puppet::Util.withumask(0007) do - ::FileUtils.mkdir_p( path_for( bucket_file) ) + ::FileUtils.mkdir_p(dirname) end end - Puppet.info "FileBucket adding #{bucket_file.path} as #{bucket_file.checksum}" + Puppet.info "FileBucket adding #{bucket_file.checksum}" # Write the file to disk. Puppet::Util.withumask(0007) do - ::File.open(contents_path_for(bucket_file), ::File::WRONLY|::File::CREAT, 0440) do |of| + ::File.open(filename, ::File::WRONLY|::File::CREAT, 0440) do |of| of.print bucket_file.contents end end end - - save_path_to_paths_file(bucket_file) - bucket_file.checksum_data end - def request_to_checksum_and_path( request ) - return [request.key, nil] if checksum?(request.key) - - checksum_type, checksum, path = request.key.split(/\//, 3) - return(checksum_type.to_s == "" ? nil : [ "{#{checksum_type}}#{checksum}", path ]) + def request_to_checksum( request ) + checksum_type, checksum = request.key.split(/\//, 2) + raise "Unsupported checksum type #{checksum_type.inspect}" if checksum_type != 'md5' + raise "Invalid checksum #{checksum.inspect}" if checksum !~ /^[0-9a-f]{32}$/ + checksum end - def path_for(bucket_file, subfile = nil) - bucket_path = bucket_file.bucket_path || Puppet[:bucketdir] - digest = bucket_file.checksum_data + def path_for(bucket_path, digest, subfile = nil) + bucket_path ||= Puppet[:bucketdir] dir = ::File.join(digest[0..7].split("")) basedir = ::File.join(bucket_path, dir, digest) @@ -108,48 +89,18 @@ module Puppet::FileBucketFile ::File.join(basedir, subfile) end - def contents_path_for(bucket_file) - path_for(bucket_file, "contents") - end - - def paths_path_for(bucket_file) - path_for(bucket_file, "paths") - end - - def content_check? - true - end - # If conflict_check is enabled, verify that the passed text is # the same as the text in our file. def verify_identical_file!(bucket_file) - return unless content_check? - disk_contents = ::File.read(contents_path_for(bucket_file)) + disk_contents = ::File.read(path_for(bucket_file.bucket_path, bucket_file.checksum_data, 'contents')) # If the contents don't match, then we've found a conflict. # Unlikely, but quite bad. if disk_contents != bucket_file.contents - raise Puppet::FileBucket::BucketError, "Got passed new contents for sum #{bucket_file.checksum}", caller + raise Puppet::FileBucket::BucketError, "Got passed new contents for sum #{bucket_file.checksum}" else - Puppet.info "FileBucket got a duplicate file #{bucket_file.path} (#{bucket_file.checksum})" + Puppet.info "FileBucket got a duplicate file #{bucket_file.checksum}" end end - - def save_path_to_paths_file(bucket_file) - return unless bucket_file.path - - # check for dupes - if ::File.exist?(paths_path_for( bucket_file) ) - ::File.open(paths_path_for( bucket_file) ) do |f| - return if f.readlines.collect { |l| l.chomp }.include?(bucket_file.path) - end - end - - # if it's a new file, or if our path isn't in the file yet, add it - ::File.open(paths_path_for(bucket_file), ::File::WRONLY|::File::CREAT|::File::APPEND) do |of| - of.puts bucket_file.path - end - end - end end diff --git a/spec/unit/file_bucket/dipper_spec.rb b/spec/unit/file_bucket/dipper_spec.rb index 86ad01f83..730e10792 100755 --- a/spec/unit/file_bucket/dipper_spec.rb +++ b/spec/unit/file_bucket/dipper_spec.rb @@ -53,7 +53,7 @@ describe Puppet::FileBucket::Dipper do real_path = Pathname.new(file).realpath - Puppet::FileBucket::File.any_instance.expects(:save).with("https://puppetmaster:31337/production/file_bucket_file/md5/#{checksum}/#{real_path}") + Puppet::FileBucket::File.any_instance.expects(:save).with("https://puppetmaster:31337/production/file_bucket_file/md5/#{checksum}") @dipper.backup(file).should == checksum end diff --git a/spec/unit/file_bucket/file_spec.rb b/spec/unit/file_bucket/file_spec.rb index 3ad70c203..82063c2e3 100644 --- a/spec/unit/file_bucket/file_spec.rb +++ b/spec/unit/file_bucket/file_spec.rb @@ -7,13 +7,16 @@ require 'digest/md5' require 'digest/sha1' describe Puppet::FileBucket::File do + include PuppetSpec::Files + before do # this is the default from spec_helper, but it keeps getting reset at odd times - Puppet[:bucketdir] = "/dev/null/bucket" + @bucketdir = tmpdir('bucket') + Puppet[:bucketdir] = @bucketdir @digest = "4a8ec4fa5f01b4ab1a0ab8cbccb709f0" @checksum = "{md5}4a8ec4fa5f01b4ab1a0ab8cbccb709f0" - @dir = '/dev/null/bucket/4/a/8/e/c/4/f/a/4a8ec4fa5f01b4ab1a0ab8cbccb709f0' + @dir = File.join(@bucketdir, '4/a/8/e/c/4/f/a/4a8ec4fa5f01b4ab1a0ab8cbccb709f0') @contents = "file contents" end @@ -22,17 +25,6 @@ describe Puppet::FileBucket::File do Puppet::FileBucket::File.new(@contents).to_s.should == @contents end - it "should calculate the checksum type from the passed in checksum" do - Puppet::FileBucket::File.new(@contents, :checksum => @checksum).checksum_type.should == "md5" - end - - it "should allow contents to be specified in a block" do - bucket = Puppet::FileBucket::File.new(nil) do |fb| - fb.contents = "content" - end - bucket.contents.should == "content" - end - it "should raise an error if changing content" do x = Puppet::FileBucket::File.new("first") proc { x.contents = "new" }.should raise_error @@ -42,14 +34,6 @@ describe Puppet::FileBucket::File do proc { Puppet::FileBucket::File.new(5) }.should raise_error(ArgumentError) end - it "should raise an error if setting contents to a non-string" do - proc do - Puppet::FileBucket::File.new(nil) do |x| - x.contents = 5 - end - end.should raise_error(ArgumentError) - end - it "should set the contents appropriately" do Puppet::FileBucket::File.new(@contents).contents.should == @contents end @@ -62,33 +46,6 @@ describe Puppet::FileBucket::File do Puppet::FileBucket::File.new(@contents).checksum.should == @checksum end - it "should remove the old checksum value if the algorithm is changed" do - sum = Puppet::FileBucket::File.new(@contents) - sum.checksum.should_not be_nil - - newsum = Digest::SHA1.hexdigest(@contents).to_s - sum.checksum_type = :sha1 - sum.checksum.should == "{sha1}#{newsum}" - end - - it "should support specifying the checksum_type during initialization" do - sum = Puppet::FileBucket::File.new(@contents, :checksum_type => :sha1) - sum.checksum_type.should == :sha1 - end - - it "should fail when an unsupported checksum_type is used" do - proc { Puppet::FileBucket::File.new(@contents, :checksum_type => :nope) }.should raise_error(ArgumentError) - end - - it "should fail if given an checksum at initialization that does not match the contents" do - proc { Puppet::FileBucket::File.new(@contents, :checksum => "{md5}00000000000000000000000000000000") }.should raise_error(RuntimeError) - end - - it "should fail if assigned a checksum that does not match the contents" do - bucket = Puppet::FileBucket::File.new(@contents) - proc { bucket.checksum = "{md5}00000000000000000000000000000000" }.should raise_error(RuntimeError) - end - describe "when using back-ends" do it "should redirect using Puppet::Indirector" do Puppet::Indirector::Indirection.instance(:file_bucket_file).model.should equal(Puppet::FileBucket::File) @@ -129,26 +86,6 @@ describe Puppet::FileBucket::File do Puppet::FileBucket::File.new(@contents).save end - - it "should append the path to the paths file" do - remote_path = '/path/on/the/remote/box' - - ::File.expects(:directory?).with(@dir).returns(true) - ::File.expects(:open).with("#{@dir}/contents", ::File::WRONLY|::File::CREAT, 0440) - ::File.expects(:exist?).with("#{@dir}/contents").returns false - - mockfile = mock "file" - mockfile.expects(:puts).with('/path/on/the/remote/box') - ::File.expects(:exist?).with("#{@dir}/paths").returns false - ::File.expects(:open).with("#{@dir}/paths", ::File::WRONLY|::File::CREAT|::File::APPEND).yields mockfile - Puppet::FileBucket::File.new(@contents, :path => remote_path).save - - end - end - - it "should accept a path" do - remote_path = '/path/on/the/remote/box' - Puppet::FileBucket::File.new(@contents, :path => remote_path).path.should == remote_path end it "should return a url-ish name" do @@ -160,18 +97,6 @@ describe Puppet::FileBucket::File do lambda { bucket.name = "sha1/4a8ec4fa5f01b4ab1a0ab8cbccb709f0/new/path" }.should raise_error end - it "should accept a url-ish name" do - bucket = Puppet::FileBucket::File.new(@contents) - lambda { bucket.name = "sha1/034fa2ed8e211e4d20f20e792d777f4a30af1a93/new/path" }.should_not raise_error - bucket.checksum_type.should == "sha1" - bucket.checksum_data.should == '034fa2ed8e211e4d20f20e792d777f4a30af1a93' - bucket.path.should == "new/path" - end - - it "should return a url-ish name with a path" do - Puppet::FileBucket::File.new(@contents, :path => 'my/path').name.should == "md5/4a8ec4fa5f01b4ab1a0ab8cbccb709f0/my/path" - end - it "should convert the contents to PSON" do Puppet::FileBucket::File.new(@contents).to_pson.should == '{"contents":"file contents"}' end @@ -191,36 +116,31 @@ describe Puppet::FileBucket::File do end + def make_bucketed_file + FileUtils.mkdir_p(@dir) + File.open("#{@dir}/contents", 'w') { |f| f.write @contents } + end + describe "using the indirector's find method" do it "should return nil if a file doesn't exist" do - ::File.expects(:exist?).with("#{@dir}/contents").returns false - - bucketfile = Puppet::FileBucket::File.find("{md5}#{@digest}") + bucketfile = Puppet::FileBucket::File.find("md5/#{@digest}") bucketfile.should == nil end it "should find a filebucket if the file exists" do - ::File.expects(:exist?).with("#{@dir}/contents").returns true - ::File.expects(:exist?).with("#{@dir}/paths").returns false - ::File.expects(:read).with("#{@dir}/contents").returns @contents - - bucketfile = Puppet::FileBucket::File.find("{md5}#{@digest}") + make_bucketed_file + bucketfile = Puppet::FileBucket::File.find("md5/#{@digest}") bucketfile.should_not == nil end describe "using RESTish digest notation" do it "should return nil if a file doesn't exist" do - ::File.expects(:exist?).with("#{@dir}/contents").returns false - bucketfile = Puppet::FileBucket::File.find("md5/#{@digest}") bucketfile.should == nil end it "should find a filebucket if the file exists" do - ::File.expects(:exist?).with("#{@dir}/contents").returns true - ::File.expects(:exist?).with("#{@dir}/paths").returns false - ::File.expects(:read).with("#{@dir}/contents").returns @contents - + make_bucketed_file bucketfile = Puppet::FileBucket::File.find("md5/#{@digest}") bucketfile.should_not == nil end diff --git a/spec/unit/indirector/file_bucket_file/file_spec.rb b/spec/unit/indirector/file_bucket_file/file_spec.rb index 7bf02b5b3..6057fd2e6 100755 --- a/spec/unit/indirector/file_bucket_file/file_spec.rb +++ b/spec/unit/indirector/file_bucket_file/file_spec.rb @@ -5,6 +5,8 @@ require ::File.dirname(__FILE__) + '/../../../spec_helper' require 'puppet/indirector/file_bucket_file/file' describe Puppet::FileBucketFile::File do + include PuppetSpec::Files + it "should be a subclass of the Code terminus class" do Puppet::FileBucketFile::File.superclass.should equal(Puppet::Indirector::Code) end @@ -66,141 +68,40 @@ HERE end - describe "the find_by_checksum method" do - before do - # this is the default from spec_helper, but it keeps getting reset at odd times - Puppet[:bucketdir] = "/dev/null/bucket" - - @digest = "4a8ec4fa5f01b4ab1a0ab8cbccb709f0" - @checksum = "{md5}4a8ec4fa5f01b4ab1a0ab8cbccb709f0" - @dir = '/dev/null/bucket/4/a/8/e/c/4/f/a/4a8ec4fa5f01b4ab1a0ab8cbccb709f0' - - @contents = "file contents" - end - - it "should return nil if a file doesn't exist" do - ::File.expects(:exist?).with("#{@dir}/contents").returns false - - bucketfile = Puppet::FileBucketFile::File.new.send(:find_by_checksum, "{md5}#{@digest}", {}) - bucketfile.should == nil - end - - it "should find a filebucket if the file exists" do - ::File.expects(:exist?).with("#{@dir}/contents").returns true - ::File.expects(:exist?).with("#{@dir}/paths").returns false - ::File.expects(:read).with("#{@dir}/contents").returns @contents - - bucketfile = Puppet::FileBucketFile::File.new.send(:find_by_checksum, "{md5}#{@digest}", {}) - bucketfile.should_not == nil - end - - it "should load the paths" do - paths = ["path1", "path2"] - ::File.expects(:exist?).with("#{@dir}/contents").returns true - ::File.expects(:exist?).with("#{@dir}/paths").returns true - ::File.expects(:read).with("#{@dir}/contents").returns @contents - - mockfile = mock "file" - mockfile.expects(:readlines).returns( paths ) - ::File.expects(:open).with("#{@dir}/paths").yields mockfile - - Puppet::FileBucketFile::File.new.send(:find_by_checksum, "{md5}#{@digest}", {}).paths.should == paths - end - - end - describe "when retrieving files" do before :each do Puppet.settings.stubs(:use) @store = Puppet::FileBucketFile::File.new @digest = "70924d6fa4b2d745185fa4660703a5c0" - @sum = stub 'sum', :name => @digest - @dir = "/what/ever" + @bucket_dir = tmpdir("bucket") - Puppet.stubs(:[]).with(:bucketdir).returns(@dir) + Puppet.stubs(:[]).with(:bucketdir).returns(@bucket_dir) - @contents_path = '/what/ever/7/0/9/2/4/d/6/f/70924d6fa4b2d745185fa4660703a5c0/contents' - @paths_path = '/what/ever/7/0/9/2/4/d/6/f/70924d6fa4b2d745185fa4660703a5c0/paths' + @dir = "#{@bucket_dir}/7/0/9/2/4/d/6/f/70924d6fa4b2d745185fa4660703a5c0" + @contents_path = "#{@dir}/contents" - @request = stub 'request', :key => "md5/#{@digest}/remote/path", :options => {} + @request = Puppet::Indirector::Request.new(:indirection_name, :find, "md5/#{@digest}") end - it "should call find_by_checksum" do - @store.expects(:find_by_checksum).with{|x,opts| x == "{md5}#{@digest}"}.returns(false) - @store.find(@request) - end - - it "should look for the calculated path" do - ::File.expects(:exist?).with(@contents_path).returns(false) - @store.find(@request) + def make_bucketed_file + FileUtils.mkdir_p(@dir) + File.open(@contents_path, 'w') { |f| f.write @contents } end it "should return an instance of Puppet::FileBucket::File created with the content if the file exists" do - content = "my content" - bucketfile = stub 'bucketfile' - bucketfile.stubs(:bucket_path) - bucketfile.stubs(:bucket_path=) - bucketfile.stubs(:checksum_data).returns(@digest) - bucketfile.stubs(:checksum).returns(@checksum) - - bucketfile.expects(:contents=).with(content) - Puppet::FileBucket::File.expects(:new).with(nil, {:checksum => "{md5}#{@digest}"}).yields(bucketfile).returns(bucketfile) + @contents = "my content" + make_bucketed_file - ::File.expects(:exist?).with(@contents_path).returns(true) - ::File.expects(:exist?).with(@paths_path).returns(false) - ::File.expects(:read).with(@contents_path).returns(content) - - @store.find(@request).should equal(bucketfile) + bucketfile = @store.find(@request) + bucketfile.should be_a(Puppet::FileBucket::File) + bucketfile.contents.should == @contents end it "should return nil if no file is found" do - ::File.expects(:exist?).with(@contents_path).returns(false) @store.find(@request).should be_nil end - - it "should fail intelligently if a found file cannot be read" do - ::File.expects(:exist?).with(@contents_path).returns(true) - ::File.expects(:read).with(@contents_path).raises(RuntimeError) - proc { @store.find(@request) }.should raise_error(Puppet::Error) - end - - end - - describe "when determining file paths" do - before do - Puppet[:bucketdir] = '/dev/null/bucketdir' - @digest = 'DEADBEEFC0FFEE' - @bucket = stub_everything "bucket" - @bucket.expects(:checksum_data).returns(@digest) - end - - it "should use the value of the :bucketdir setting as the root directory" do - path = Puppet::FileBucketFile::File.new.send(:contents_path_for, @bucket) - path.should =~ %r{^/dev/null/bucketdir} - end - - it "should choose a path 8 directories deep with each directory name being the respective character in the filebucket" do - path = Puppet::FileBucketFile::File.new.send(:contents_path_for, @bucket) - dirs = @digest[0..7].split("").join(File::SEPARATOR) - path.should be_include(dirs) - end - - it "should use the full filebucket as the final directory name" do - path = Puppet::FileBucketFile::File.new.send(:contents_path_for, @bucket) - ::File.basename(::File.dirname(path)).should == @digest - end - - it "should use 'contents' as the actual file name" do - path = Puppet::FileBucketFile::File.new.send(:contents_path_for, @bucket) - ::File.basename(path).should == "contents" - end - - it "should use the bucketdir, the 8 sum character directories, the full filebucket, and 'contents' as the full file name" do - path = Puppet::FileBucketFile::File.new.send(:contents_path_for, @bucket) - path.should == ['/dev/null/bucketdir', @digest[0..7].split(""), @digest, "contents"].flatten.join(::File::SEPARATOR) - end end describe "when saving files" do @@ -276,60 +177,4 @@ HERE end end - - - describe "when writing to the paths file" do - before do - Puppet[:bucketdir] = '/dev/null/bucketdir' - @digest = '70924d6fa4b2d745185fa4660703a5c0' - @bucket = stub_everything "bucket" - - @paths_path = '/dev/null/bucketdir/7/0/9/2/4/d/6/f/70924d6fa4b2d745185fa4660703a5c0/paths' - - @paths = [] - @bucket.stubs(:paths).returns(@paths) - @bucket.stubs(:checksum_data).returns(@digest) - end - - it "should create a file if it doesn't exist" do - @bucket.expects(:path).returns('path/to/save').at_least_once - File.expects(:exist?).with(@paths_path).returns(false) - file = stub "file" - file.expects(:puts).with('path/to/save') - File.expects(:open).with(@paths_path, ::File::WRONLY|::File::CREAT|::File::APPEND).yields(file) - - Puppet::FileBucketFile::File.new.send(:save_path_to_paths_file, @bucket) - end - - it "should append to a file if it exists" do - @bucket.expects(:path).returns('path/to/save').at_least_once - File.expects(:exist?).with(@paths_path).returns(true) - old_file = stub "file" - old_file.stubs(:readlines).returns [] - File.expects(:open).with(@paths_path).yields(old_file) - - file = stub "file" - file.expects(:puts).with('path/to/save') - File.expects(:open).with(@paths_path, ::File::WRONLY|::File::CREAT|::File::APPEND).yields(file) - - Puppet::FileBucketFile::File.new.send(:save_path_to_paths_file, @bucket) - end - - it "should not alter a file if it already contains the path" do - @bucket.expects(:path).returns('path/to/save').at_least_once - File.expects(:exist?).with(@paths_path).returns(true) - old_file = stub "file" - old_file.stubs(:readlines).returns ["path/to/save\n"] - File.expects(:open).with(@paths_path).yields(old_file) - - Puppet::FileBucketFile::File.new.send(:save_path_to_paths_file, @bucket) - end - - it "should do nothing if there is no path" do - @bucket.expects(:path).returns(nil).at_least_once - - Puppet::FileBucketFile::File.new.send(:save_path_to_paths_file, @bucket) - end - end - end -- cgit From 94d71799f1ee196186bc3a8a5a1b06ef2ae0806e Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Tue, 11 Jan 2011 13:45:55 -0800 Subject: (#5838) Make file bucket dipper efficient when saving a file that already exists Before saving to the file bucket, the file bucket dipper now checks to make sure that no file with the given checksum is already present. Paired-with: Jesse Wolfe --- lib/puppet/file_bucket/dipper.rb | 7 ++++++- spec/unit/file_bucket/dipper_spec.rb | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/lib/puppet/file_bucket/dipper.rb b/lib/puppet/file_bucket/dipper.rb index b012a8681..f4bef28a8 100644 --- a/lib/puppet/file_bucket/dipper.rb +++ b/lib/puppet/file_bucket/dipper.rb @@ -36,7 +36,12 @@ class Puppet::FileBucket::Dipper file_bucket_file = Puppet::FileBucket::File.new(contents, :bucket_path => @local_path) dest_path = "#{@rest_path}#{file_bucket_file.name}" - file_bucket_file.save(dest_path) + # Make a HEAD request for the file so that we don't waste time + # uploading it if it already exists in the bucket. + unless Puppet::FileBucket::File.head("#{@rest_path}#{file_bucket_file.checksum_type}/#{file_bucket_file.checksum_data}") + file_bucket_file.save(dest_path) + end + return file_bucket_file.checksum_data rescue => detail puts detail.backtrace if Puppet[:trace] diff --git a/spec/unit/file_bucket/dipper_spec.rb b/spec/unit/file_bucket/dipper_spec.rb index 730e10792..3e9e8b145 100755 --- a/spec/unit/file_bucket/dipper_spec.rb +++ b/spec/unit/file_bucket/dipper_spec.rb @@ -14,10 +14,20 @@ describe Puppet::FileBucket::Dipper do file end + it "should fail in an informative way when there are failures checking for the file on the server" do + @dipper = Puppet::FileBucket::Dipper.new(:Path => "/my/bucket") + + file = make_tmp_file('contents') + Puppet::FileBucket::File.expects(:head).raises ArgumentError + + lambda { @dipper.backup(file) }.should raise_error(Puppet::Error) + end + it "should fail in an informative way when there are failures backing up to the server" do @dipper = Puppet::FileBucket::Dipper.new(:Path => "/my/bucket") file = make_tmp_file('contents') + Puppet::FileBucket::File.expects(:head).returns false Puppet::FileBucket::File.any_instance.expects(:save).raises ArgumentError lambda { @dipper.backup(file) }.should raise_error(Puppet::Error) @@ -29,10 +39,22 @@ describe Puppet::FileBucket::Dipper do file = make_tmp_file('my contents') checksum = Digest::MD5.hexdigest('my contents') + Puppet::FileBucket::File.expects(:head).returns false Puppet::FileBucket::File.any_instance.expects(:save) @dipper.backup(file).should == checksum end + it "should not backup a file that is already in the bucket" do + @dipper = Puppet::FileBucket::Dipper.new(:Path => "/my/bucket") + + file = make_tmp_file('my contents') + checksum = Digest::MD5.hexdigest('my contents') + + Puppet::FileBucket::File.expects(:head).returns true + Puppet::FileBucket::File.any_instance.expects(:save).never + @dipper.backup(file).should == checksum + end + it "should retrieve files from a local bucket" do @dipper = Puppet::FileBucket::Dipper.new(:Path => "/my/bucket") @@ -53,6 +75,7 @@ describe Puppet::FileBucket::Dipper do real_path = Pathname.new(file).realpath + Puppet::FileBucket::File.expects(:head).with("https://puppetmaster:31337/production/file_bucket_file/md5/#{checksum}").returns false Puppet::FileBucket::File.any_instance.expects(:save).with("https://puppetmaster:31337/production/file_bucket_file/md5/#{checksum}") @dipper.backup(file).should == checksum -- cgit From 002f9f1905b089ebca628a6b743c0659e30ff9bc Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Wed, 12 Jan 2011 15:18:21 -0800 Subject: (#5838) Improve the quality of file bucket specs. Paired-with: Jesse Wolfe --- spec/unit/file_bucket/dipper_spec.rb | 43 +++++++++++----- spec/unit/indirector/file_bucket_file/file_spec.rb | 59 +++++++++++++--------- 2 files changed, 66 insertions(+), 36 deletions(-) diff --git a/spec/unit/file_bucket/dipper_spec.rb b/spec/unit/file_bucket/dipper_spec.rb index 3e9e8b145..db40c6296 100755 --- a/spec/unit/file_bucket/dipper_spec.rb +++ b/spec/unit/file_bucket/dipper_spec.rb @@ -5,6 +5,8 @@ require File.dirname(__FILE__) + '/../../spec_helper' require 'pathname' require 'puppet/file_bucket/dipper' +require 'puppet/indirector/file_bucket_file/rest' + describe Puppet::FileBucket::Dipper do include PuppetSpec::Files @@ -34,14 +36,17 @@ describe Puppet::FileBucket::Dipper do end it "should backup files to a local bucket" do - @dipper = Puppet::FileBucket::Dipper.new(:Path => "/my/bucket") + Puppet[:bucketdir] = "/non/existent/directory" + file_bucket = tmpdir("bucket") + + @dipper = Puppet::FileBucket::Dipper.new(:Path => file_bucket) file = make_tmp_file('my contents') - checksum = Digest::MD5.hexdigest('my contents') + checksum = "2975f560750e71c478b8e3b39a956adb" + Digest::MD5.hexdigest('my contents').should == checksum - Puppet::FileBucket::File.expects(:head).returns false - Puppet::FileBucket::File.any_instance.expects(:save) @dipper.backup(file).should == checksum + File.exists?("#{file_bucket}/2/9/7/5/f/5/6/0/2975f560750e71c478b8e3b39a956adb/contents").should == true end it "should not backup a file that is already in the bucket" do @@ -60,11 +65,13 @@ describe Puppet::FileBucket::Dipper do checksum = Digest::MD5.hexdigest('my contents') - Puppet::FileBucket::File.expects(:find).with{|x,opts| - x == "md5/#{checksum}" - }.returns(Puppet::FileBucket::File.new('my contents')) + request = nil + + Puppet::FileBucketFile::File.any_instance.expects(:find).with{ |r| request = r }.once.returns(Puppet::FileBucket::File.new('my contents')) @dipper.getfile(checksum).should == 'my contents' + + request.key.should == "md5/#{checksum}" end it "should backup files to a remote server" do @@ -75,10 +82,18 @@ describe Puppet::FileBucket::Dipper do real_path = Pathname.new(file).realpath - Puppet::FileBucket::File.expects(:head).with("https://puppetmaster:31337/production/file_bucket_file/md5/#{checksum}").returns false - Puppet::FileBucket::File.any_instance.expects(:save).with("https://puppetmaster:31337/production/file_bucket_file/md5/#{checksum}") + request1 = nil + request2 = nil + + Puppet::FileBucketFile::Rest.any_instance.expects(:head).with { |r| request1 = r }.once.returns(nil) + Puppet::FileBucketFile::Rest.any_instance.expects(:save).with { |r| request2 = r }.once @dipper.backup(file).should == checksum + [request1, request2].each do |r| + r.server.should == 'puppetmaster' + r.port.should == 31337 + r.key.should == "md5/#{checksum}" + end end it "should retrieve files from a remote server" do @@ -86,10 +101,14 @@ describe Puppet::FileBucket::Dipper do checksum = Digest::MD5.hexdigest('my contents') - Puppet::FileBucket::File.expects(:find).with{|x,opts| - x == "https://puppetmaster:31337/production/file_bucket_file/md5/#{checksum}" - }.returns(Puppet::FileBucket::File.new('my contents')) + request = nil + + Puppet::FileBucketFile::Rest.any_instance.expects(:find).with { |r| request = r }.returns(Puppet::FileBucket::File.new('my contents')) @dipper.getfile(checksum).should == "my contents" + + request.server.should == 'puppetmaster' + request.port.should == 31337 + request.key.should == "md5/#{checksum}" end end diff --git a/spec/unit/indirector/file_bucket_file/file_spec.rb b/spec/unit/indirector/file_bucket_file/file_spec.rb index 6057fd2e6..cb614f36e 100755 --- a/spec/unit/indirector/file_bucket_file/file_spec.rb +++ b/spec/unit/indirector/file_bucket_file/file_spec.rb @@ -68,39 +68,50 @@ HERE end - describe "when retrieving files" do - before :each do - Puppet.settings.stubs(:use) - @store = Puppet::FileBucketFile::File.new + [true, false].each do |override_bucket_path| + describe "when retrieving files and bucket path #{if override_bucket_path then 'is' else 'is not' end} overridden" do + before :each do + Puppet.settings.stubs(:use) + @store = Puppet::FileBucketFile::File.new - @digest = "70924d6fa4b2d745185fa4660703a5c0" + @digest = "70924d6fa4b2d745185fa4660703a5c0" - @bucket_dir = tmpdir("bucket") + @bucket_dir = tmpdir("bucket") - Puppet.stubs(:[]).with(:bucketdir).returns(@bucket_dir) + if override_bucket_path + Puppet[:bucketdir] = "/bogus/path" # should not be used + else + Puppet[:bucketdir] = @bucket_dir + end - @dir = "#{@bucket_dir}/7/0/9/2/4/d/6/f/70924d6fa4b2d745185fa4660703a5c0" - @contents_path = "#{@dir}/contents" + @dir = "#{@bucket_dir}/7/0/9/2/4/d/6/f/70924d6fa4b2d745185fa4660703a5c0" + @contents_path = "#{@dir}/contents" - @request = Puppet::Indirector::Request.new(:indirection_name, :find, "md5/#{@digest}") - end + request_options = {} + if override_bucket_path + request_options[:bucket_path] = @bucket_dir + end - def make_bucketed_file - FileUtils.mkdir_p(@dir) - File.open(@contents_path, 'w') { |f| f.write @contents } - end + @request = Puppet::Indirector::Request.new(:indirection_name, :find, "md5/#{@digest}", request_options) + end + + def make_bucketed_file + FileUtils.mkdir_p(@dir) + File.open(@contents_path, 'w') { |f| f.write @contents } + end - it "should return an instance of Puppet::FileBucket::File created with the content if the file exists" do - @contents = "my content" - make_bucketed_file + it "should return an instance of Puppet::FileBucket::File created with the content if the file exists" do + @contents = "my content" + make_bucketed_file - bucketfile = @store.find(@request) - bucketfile.should be_a(Puppet::FileBucket::File) - bucketfile.contents.should == @contents - end + bucketfile = @store.find(@request) + bucketfile.should be_a(Puppet::FileBucket::File) + bucketfile.contents.should == @contents + end - it "should return nil if no file is found" do - @store.find(@request).should be_nil + it "should return nil if no file is found" do + @store.find(@request).should be_nil + end end end -- cgit From abc62560f78fa227d6ffd3263a095665609a15b5 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Wed, 12 Jan 2011 15:41:39 -0800 Subject: (#5838) Support paths as part of file bucket requests. In versions of Puppet 2.6.0-2.6.4, file bucket requests are of the form md5//. The path functionality has been removed, however we still need to support requests coming from older clients. Paired-with: Jesse Wolfe --- lib/puppet/indirector/file_bucket_file/file.rb | 2 +- spec/unit/indirector/file_bucket_file/file_spec.rb | 162 ++++++++++----------- 2 files changed, 79 insertions(+), 85 deletions(-) diff --git a/lib/puppet/indirector/file_bucket_file/file.rb b/lib/puppet/indirector/file_bucket_file/file.rb index 38e0be6e9..8bea2d767 100644 --- a/lib/puppet/indirector/file_bucket_file/file.rb +++ b/lib/puppet/indirector/file_bucket_file/file.rb @@ -73,7 +73,7 @@ module Puppet::FileBucketFile end def request_to_checksum( request ) - checksum_type, checksum = request.key.split(/\//, 2) + checksum_type, checksum, path = request.key.split(/\//, 3) # Note: we ignore path if present. raise "Unsupported checksum type #{checksum_type.inspect}" if checksum_type != 'md5' raise "Invalid checksum #{checksum.inspect}" if checksum !~ /^[0-9a-f]{32}$/ checksum diff --git a/spec/unit/indirector/file_bucket_file/file_spec.rb b/spec/unit/indirector/file_bucket_file/file_spec.rb index cb614f36e..9187f4da0 100755 --- a/spec/unit/indirector/file_bucket_file/file_spec.rb +++ b/spec/unit/indirector/file_bucket_file/file_spec.rb @@ -69,95 +69,89 @@ HERE [true, false].each do |override_bucket_path| - describe "when retrieving files and bucket path #{if override_bucket_path then 'is' else 'is not' end} overridden" do - before :each do - Puppet.settings.stubs(:use) - @store = Puppet::FileBucketFile::File.new - - @digest = "70924d6fa4b2d745185fa4660703a5c0" - - @bucket_dir = tmpdir("bucket") - - if override_bucket_path - Puppet[:bucketdir] = "/bogus/path" # should not be used - else - Puppet[:bucketdir] = @bucket_dir - end - - @dir = "#{@bucket_dir}/7/0/9/2/4/d/6/f/70924d6fa4b2d745185fa4660703a5c0" - @contents_path = "#{@dir}/contents" - - request_options = {} - if override_bucket_path - request_options[:bucket_path] = @bucket_dir + describe "when bucket path #{if override_bucket_path then 'is' else 'is not' end} overridden" do + [true, false].each do |supply_path| + describe "when #{supply_path ? 'supplying' : 'not supplying'} a path" do + before :each do + Puppet.settings.stubs(:use) + @store = Puppet::FileBucketFile::File.new + @contents = "my content" + + @digest = "f2bfa7fc155c4f42cb91404198dda01f" + @digest.should == Digest::MD5.hexdigest(@contents) + + @bucket_dir = tmpdir("bucket") + + if override_bucket_path + Puppet[:bucketdir] = "/bogus/path" # should not be used + else + Puppet[:bucketdir] = @bucket_dir + end + + @dir = "#{@bucket_dir}/f/2/b/f/a/7/f/c/f2bfa7fc155c4f42cb91404198dda01f" + @contents_path = "#{@dir}/contents" + end + + describe "when retrieving files" do + before :each do + + request_options = {} + if override_bucket_path + request_options[:bucket_path] = @bucket_dir + end + + key = "md5/#{@digest}" + if supply_path + key += "//path/to/file" + end + + @request = Puppet::Indirector::Request.new(:indirection_name, :find, key, request_options) + end + + def make_bucketed_file + FileUtils.mkdir_p(@dir) + File.open(@contents_path, 'w') { |f| f.write @contents } + end + + it "should return an instance of Puppet::FileBucket::File created with the content if the file exists" do + make_bucketed_file + + bucketfile = @store.find(@request) + bucketfile.should be_a(Puppet::FileBucket::File) + bucketfile.contents.should == @contents + @store.head(@request).should == true + end + + it "should return nil if no file is found" do + @store.find(@request).should be_nil + @store.head(@request).should == false + end + end + + describe "when saving files" do + it "should save the contents to the calculated path" do + options = {} + if override_bucket_path + options[:bucket_path] = @bucket_dir + end + + key = "md5/#{@digest}" + if supply_path + key += "//path/to/file" + end + + file_instance = Puppet::FileBucket::File.new(@contents, options) + request = Puppet::Indirector::Request.new(:indirection_name, :save, key, file_instance) + + @store.save(request) + File.read("#{@dir}/contents").should == @contents + end + end end - - @request = Puppet::Indirector::Request.new(:indirection_name, :find, "md5/#{@digest}", request_options) - end - - def make_bucketed_file - FileUtils.mkdir_p(@dir) - File.open(@contents_path, 'w') { |f| f.write @contents } - end - - it "should return an instance of Puppet::FileBucket::File created with the content if the file exists" do - @contents = "my content" - make_bucketed_file - - bucketfile = @store.find(@request) - bucketfile.should be_a(Puppet::FileBucket::File) - bucketfile.contents.should == @contents - end - - it "should return nil if no file is found" do - @store.find(@request).should be_nil end end end - describe "when saving files" do - before do - # this is the default from spec_helper, but it keeps getting reset at odd times - Puppet[:bucketdir] = "/dev/null/bucket" - - @digest = "4a8ec4fa5f01b4ab1a0ab8cbccb709f0" - @checksum = "{md5}4a8ec4fa5f01b4ab1a0ab8cbccb709f0" - @dir = '/dev/null/bucket/4/a/8/e/c/4/f/a/4a8ec4fa5f01b4ab1a0ab8cbccb709f0' - - @contents = "file contents" - - @bucket = stub "bucket file" - @bucket.stubs(:bucket_path) - @bucket.stubs(:checksum_data).returns(@digest) - @bucket.stubs(:path).returns(nil) - @bucket.stubs(:checksum).returns(nil) - @bucket.stubs(:contents).returns("file contents") - end - - it "should save the contents to the calculated path" do - ::File.stubs(:directory?).with(@dir).returns(true) - ::File.expects(:exist?).with("#{@dir}/contents").returns false - - mockfile = mock "file" - mockfile.expects(:print).with(@contents) - ::File.expects(:open).with("#{@dir}/contents", ::File::WRONLY|::File::CREAT, 0440).yields(mockfile) - - Puppet::FileBucketFile::File.new.send(:save_to_disk, @bucket) - end - - it "should make any directories necessary for storage" do - FileUtils.expects(:mkdir_p).with do |arg| - ::File.umask == 0007 and arg == @dir - end - ::File.expects(:directory?).with(@dir).returns(false) - ::File.expects(:open).with("#{@dir}/contents", ::File::WRONLY|::File::CREAT, 0440) - ::File.expects(:exist?).with("#{@dir}/contents").returns false - - Puppet::FileBucketFile::File.new.send(:save_to_disk, @bucket) - end - end - - describe "when verifying identical files" do before do # this is the default from spec_helper, but it keeps getting reset at odd times -- cgit From 71ac9cfeeda5d41eb95f30e218d383c95c504455 Mon Sep 17 00:00:00 2001 From: James Turnbull Date: Thu, 13 Jan 2011 15:00:58 +1100 Subject: Locked Puppet license to GPLv2 --- LICENSE | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/LICENSE b/LICENSE index a43a24352..6bfcc22a1 100644 --- a/LICENSE +++ b/LICENSE @@ -4,8 +4,8 @@ Puppet Labs can be contacted at: info@puppetlabs.com This program and entire repository is free software; you can redistribute it and/or modify it under the terms of the GNU -General Public License as published by the Free Software -Foundation; either version 2 of the License, or any later version. +General Public License Version 2 as published by the Free Software +Foundation. This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of -- cgit From a7cd1856f3347b6a00a260d1001252b56f54f7de Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Thu, 13 Jan 2011 14:31:51 -0800 Subject: Prep for #5171: Added a missing require to inspect application. Paired-with: Nick Lewis --- lib/puppet/application/inspect.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/puppet/application/inspect.rb b/lib/puppet/application/inspect.rb index 07ee4c317..f2e03b589 100644 --- a/lib/puppet/application/inspect.rb +++ b/lib/puppet/application/inspect.rb @@ -1,3 +1,4 @@ +require 'puppet' require 'puppet/application' class Puppet::Application::Inspect < Puppet::Application -- cgit From 1a6fab2aacbc1499a00a9451654073181435afa1 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Thu, 13 Jan 2011 12:18:37 -0800 Subject: (#5171) Made "puppet inspect" upload audited files to a file bucket This only occurs if the new setting :archive_files is set. Another new setting, :archive_file_server, can be used to specify the server that files should be uploaded to. Paired-with: Nick Lewis --- lib/puppet/application/inspect.rb | 11 +++++ lib/puppet/defaults.rb | 5 +++ spec/unit/application/inspect_spec.rb | 84 +++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+) diff --git a/lib/puppet/application/inspect.rb b/lib/puppet/application/inspect.rb index f2e03b589..b4d263545 100644 --- a/lib/puppet/application/inspect.rb +++ b/lib/puppet/application/inspect.rb @@ -1,5 +1,6 @@ require 'puppet' require 'puppet/application' +require 'puppet/file_bucket/dipper' class Puppet::Application::Inspect < Puppet::Application @@ -55,6 +56,10 @@ class Puppet::Application::Inspect < Puppet::Application inspect_starttime = Time.now @report.add_times("config_retrieval", inspect_starttime - retrieval_starttime) + if Puppet[:archive_files] + dipper = Puppet::FileBucket::Dipper.new(:Server => Puppet[:archive_file_server]) + end + catalog.to_ral.resources.each do |ral_resource| audited_attributes = ral_resource[:audit] next unless audited_attributes @@ -77,6 +82,12 @@ class Puppet::Application::Inspect < Puppet::Application end end @report.add_resource_status(status) + if Puppet[:archive_files] and ral_resource.type == :file and audited_attributes.include?(:content) + path = ral_resource[:path] + if File.readable?(path) + dipper.backup(path) + end + end end finishtime = Time.now diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb index 4521a5901..400d59f15 100644 --- a/lib/puppet/defaults.rb +++ b/lib/puppet/defaults.rb @@ -598,6 +598,11 @@ module Puppet compression, but if it supports it, this setting might reduce performance on high-speed LANs."] ) + setdefaults(:inspect, + :archive_files => [false, "During an inspect run, whether to archive files whose contents are audited to a file bucket."], + :archive_file_server => ["$server", "During an inspect run, the file bucket server to archive files to if archive_files is set."] + ) + # Plugin information. setdefaults( diff --git a/spec/unit/application/inspect_spec.rb b/spec/unit/application/inspect_spec.rb index b3224d577..0c7b61f59 100644 --- a/spec/unit/application/inspect_spec.rb +++ b/spec/unit/application/inspect_spec.rb @@ -6,8 +6,11 @@ require 'puppet/application/inspect' require 'puppet/resource/catalog' require 'puppet/indirector/catalog/yaml' require 'puppet/indirector/report/rest' +require 'puppet/indirector/file_bucket_file/rest' describe Puppet::Application::Inspect do + include PuppetSpec::Files + before :each do @inspect = Puppet::Application[:inspect] end @@ -113,6 +116,87 @@ describe Puppet::Application::Inspect do end properties.should == {"ensure" => :absent} end + + describe "when archiving to a bucket" do + before :each do + Puppet[:archive_files] = true + Puppet[:archive_file_server] = "filebucketserver" + @catalog = Puppet::Resource::Catalog.new + Puppet::Resource::Catalog::Yaml.any_instance.stubs(:find).returns(@catalog) + end + + describe "when auditing files" do + before :each do + @file = tmpfile("foo") + @resource = Puppet::Resource.new(:file, @file, :parameters => {:audit => "content"}) + @catalog.add_resource(@resource) + end + + it "should send an existing file to the file bucket" do + File.open(@file, 'w') { |f| f.write('stuff') } + Puppet::FileBucketFile::Rest.any_instance.expects(:head).with do |request| + request.server == Puppet[:archive_file_server] + end.returns(false) + Puppet::FileBucketFile::Rest.any_instance.expects(:save).with do |request| + request.server == Puppet[:archive_file_server] and request.instance.contents == 'stuff' + end + @inspect.run_command + end + + it "should not send unreadable files" do + pending "see bug #5882" + File.open(@file, 'w') { |f| f.write('stuff') } + File.chmod(0, @file) + Puppet::FileBucketFile::Rest.any_instance.expects(:head).never + Puppet::FileBucketFile::Rest.any_instance.expects(:save).never + @inspect.run_command + end + + it "should not try to send non-existent files" do + Puppet::FileBucketFile::Rest.any_instance.expects(:head).never + Puppet::FileBucketFile::Rest.any_instance.expects(:save).never + @inspect.run_command + end + + it "should not try to send files whose content we are not auditing" do + @resource[:audit] = "group" + Puppet::FileBucketFile::Rest.any_instance.expects(:head).never + Puppet::FileBucketFile::Rest.any_instance.expects(:save).never + @inspect.run_command + end + end + + describe "when auditing non-files" do + before :each do + Puppet::Type.newtype(:stub_type) do + newparam(:name) do + desc "The name var" + isnamevar + end + + newproperty(:content) do + desc "content" + def retrieve + :whatever + end + end + end + + @resource = Puppet::Resource.new(:stub_type, 'foo', :parameters => {:audit => "all"}) + @catalog.add_resource(@resource) + end + + after :each do + Puppet::Type.rmtype(:stub_type) + end + + it "should not try to send non-files" do + Puppet::FileBucketFile::Rest.any_instance.expects(:head).never + Puppet::FileBucketFile::Rest.any_instance.expects(:save).never + @inspect.run_command + end + end + end end after :all do -- cgit From 17843d5e86a8841728a77c77d5f2ea4661c0f417 Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Thu, 13 Jan 2011 16:03:41 -0800 Subject: (#5882) Added error-handling to puppet inspect when auditing If auditing a resource fails, the report will contain failure events for every audited property of the resource. The report itself will also be marked as "failed". Paired-With: Paul Berry --- lib/puppet/application/inspect.rb | 39 ++++++++++++++++------- spec/unit/application/inspect_spec.rb | 60 ++++++++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 13 deletions(-) diff --git a/lib/puppet/application/inspect.rb b/lib/puppet/application/inspect.rb index b4d263545..5254afbd0 100644 --- a/lib/puppet/application/inspect.rb +++ b/lib/puppet/application/inspect.rb @@ -64,30 +64,45 @@ class Puppet::Application::Inspect < Puppet::Application audited_attributes = ral_resource[:audit] next unless audited_attributes - audited_resource = ral_resource.to_resource - status = Puppet::Resource::Status.new(ral_resource) - audited_attributes.each do |name| - next if audited_resource[name].nil? - # Skip :absent properties of :absent resources. Really, it would be nicer if the RAL returned nil for those, but it doesn't. ~JW - if name == :ensure or audited_resource[:ensure] != :absent or audited_resource[name] != :absent + + begin + audited_resource = ral_resource.to_resource + rescue StandardError => detail + puts detail.backtrace if Puppet[:trace] + ral_resource.err "Could not inspect #{ral_resource}; skipping: #{detail}" + audited_attributes.each do |name| event = ral_resource.event( - :previous_value => audited_resource[name], - :property => name, - :status => "audit", - :audited => true, - :message => "inspected value is #{audited_resource[name].inspect}" + :property => name, + :status => "failure", + :audited => true, + :message => "failed to inspect #{name}" ) status.add_event(event) end + else + audited_attributes.each do |name| + next if audited_resource[name].nil? + # Skip :absent properties of :absent resources. Really, it would be nicer if the RAL returned nil for those, but it doesn't. ~JW + if name == :ensure or audited_resource[:ensure] != :absent or audited_resource[name] != :absent + event = ral_resource.event( + :previous_value => audited_resource[name], + :property => name, + :status => "audit", + :audited => true, + :message => "inspected value is #{audited_resource[name].inspect}" + ) + status.add_event(event) + end + end end - @report.add_resource_status(status) if Puppet[:archive_files] and ral_resource.type == :file and audited_attributes.include?(:content) path = ral_resource[:path] if File.readable?(path) dipper.backup(path) end end + @report.add_resource_status(status) end finishtime = Time.now diff --git a/spec/unit/application/inspect_spec.rb b/spec/unit/application/inspect_spec.rb index 0c7b61f59..15b0c40ad 100644 --- a/spec/unit/application/inspect_spec.rb +++ b/spec/unit/application/inspect_spec.rb @@ -144,7 +144,6 @@ describe Puppet::Application::Inspect do end it "should not send unreadable files" do - pending "see bug #5882" File.open(@file, 'w') { |f| f.write('stuff') } File.chmod(0, @file) Puppet::FileBucketFile::Rest.any_instance.expects(:head).never @@ -197,6 +196,65 @@ describe Puppet::Application::Inspect do end end end + + describe "when there are failures" do + before :each do + Puppet::Type.newtype(:stub_type) do + newparam(:name) do + desc "The name var" + isnamevar + end + + newproperty(:content) do + desc "content" + def retrieve + raise "failed" + end + end + end + + @catalog = Puppet::Resource::Catalog.new + Puppet::Resource::Catalog::Yaml.any_instance.stubs(:find).returns(@catalog) + + Puppet::Transaction::Report::Rest.any_instance.expects(:save).with do |request| + @report = request.instance + end + end + + after :each do + Puppet::Type.rmtype(:stub_type) + end + + it "should mark the report failed and create failed events for each property" do + @resource = Puppet::Resource.new(:stub_type, 'foo', :parameters => {:audit => "all"}) + @catalog.add_resource(@resource) + + @inspect.run_command + + @report.status.should == "failed" + #@report.logs.select{|log| log.message =~ /Could not inspect/}.count.should == 1 + @report.resource_statuses.count.should == 1 + @report.resource_statuses['Stub_type[foo]'].events.count.should == 1 + + event = @report.resource_statuses['Stub_type[foo]'].events.first + event.property.should == "content" + event.status.should == "failure" + event.audited.should == true + event.instance_variables.should_not include("@previous_value") + end + + it "should continue to the next resource" do + @resource = Puppet::Resource.new(:stub_type, 'foo', :parameters => {:audit => "all"}) + @other_resource = Puppet::Resource.new(:stub_type, 'bar', :parameters => {:audit => "all"}) + @catalog.add_resource(@resource) + @catalog.add_resource(@other_resource) + + @inspect.run_command + + @report.resource_statuses.count.should == 2 + @report.resource_statuses.keys.should =~ ['Stub_type[foo]', 'Stub_type[bar]'] + end + end end after :all do -- cgit From 79b633220376bf0503f926b9371283cece17a9e6 Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Thu, 13 Jan 2011 16:57:01 -0800 Subject: (#5882) Added error-handling for bucketing files in puppet inspect Paired-With: Paul Berry --- lib/puppet/application/inspect.rb | 6 +++++- spec/unit/application/inspect_spec.rb | 18 ++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/lib/puppet/application/inspect.rb b/lib/puppet/application/inspect.rb index 5254afbd0..77e8476a2 100644 --- a/lib/puppet/application/inspect.rb +++ b/lib/puppet/application/inspect.rb @@ -99,7 +99,11 @@ class Puppet::Application::Inspect < Puppet::Application if Puppet[:archive_files] and ral_resource.type == :file and audited_attributes.include?(:content) path = ral_resource[:path] if File.readable?(path) - dipper.backup(path) + begin + dipper.backup(path) + rescue StandardError => detail + Puppet.warning detail + end end end @report.add_resource_status(status) diff --git a/spec/unit/application/inspect_spec.rb b/spec/unit/application/inspect_spec.rb index 15b0c40ad..1d99c6ca9 100644 --- a/spec/unit/application/inspect_spec.rb +++ b/spec/unit/application/inspect_spec.rb @@ -32,7 +32,7 @@ describe Puppet::Application::Inspect do describe "when executing" do before :each do Puppet[:report] = true - Puppet::Util::Log.stubs(:newdestination) + @inspect.options[:logset] = true Puppet::Transaction::Report::Rest.any_instance.stubs(:save) @inspect.setup end @@ -163,6 +163,20 @@ describe Puppet::Application::Inspect do Puppet::FileBucketFile::Rest.any_instance.expects(:save).never @inspect.run_command end + + it "should continue if bucketing a file fails" do + File.open(@file, 'w') { |f| f.write('stuff') } + Puppet::FileBucketFile::Rest.any_instance.stubs(:head).returns false + Puppet::FileBucketFile::Rest.any_instance.stubs(:save).raises "failure" + Puppet::Transaction::Report::Rest.any_instance.expects(:save).with do |request| + @report = request.instance + end + + @inspect.run_command + + @report.logs.count.should == 1 + @report.logs.first.message.should =~ /Could not back up/ + end end describe "when auditing non-files" do @@ -232,7 +246,7 @@ describe Puppet::Application::Inspect do @inspect.run_command @report.status.should == "failed" - #@report.logs.select{|log| log.message =~ /Could not inspect/}.count.should == 1 + @report.logs.select{|log| log.message =~ /Could not inspect/}.count.should == 1 @report.resource_statuses.count.should == 1 @report.resource_statuses['Stub_type[foo]'].events.count.should == 1 -- cgit From f9bfb968fac70eca9fe29bbd8afbcabfa1c27b9e Mon Sep 17 00:00:00 2001 From: Nick Lewis Date: Fri, 14 Jan 2011 10:55:05 -0800 Subject: (#5900) Include ResourceStatus#failed in serialized reports This property was excluded from serialization because it was believed that its value could be determined by looking at the events of a ResourceStatus. However, if a resource can't be retrieved, it will have no events created, and will have no other way of determining its status. Reviewed-By: Paul Berry --- lib/puppet/resource/status.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/puppet/resource/status.rb b/lib/puppet/resource/status.rb index a9a64425e..dea8c105d 100644 --- a/lib/puppet/resource/status.rb +++ b/lib/puppet/resource/status.rb @@ -12,7 +12,7 @@ module Puppet attr_reader :source_description, :default_log_level, :time, :resource attr_reader :change_count, :out_of_sync_count, :resource_type, :title - YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count @out_of_sync_count @tags @time @events @out_of_sync @changed @resource_type @title @skipped} + YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count @out_of_sync_count @tags @time @events @out_of_sync @changed @resource_type @title @skipped @failed} # Provide a boolean method for each of the states. STATES.each do |attr| @@ -52,6 +52,7 @@ module Puppet @changed = false @out_of_sync = false @skipped = false + @failed = false [:file, :line].each do |attr| send(attr.to_s + "=", resource.send(attr)) -- cgit