Hi, Here's a new version of this patch, including 2 indirection tests (one for the filtering at transaction evaluation time, the other for filtering after compilation). This patch also fixes the unit tests which were broken.
Please review and comment accordingly (especially the two integration tests). Thanks, Brice Original commit msg: The issue is that when we convert Puppet::Parser::Resource catalog to a Puppet::Resource catalog before storing it to the database, we don't allow virtual resource to be converted. Unfortunately exported resources are virtual by design, and as such aren't converted, and we lose them, so it isn't possible to store them in the database. Unfortunately, the client will get the exported resources too. The fix is dual-fold: * we make sure exported resource are skipped when the transaction is applied as a last safeguard * we filter-out the catalog through the catalog compiler terminus before the catalog is returned to the client Signed-off-by: Brice Figureau <[email protected]> --- lib/puppet/indirector/catalog/compiler.rb | 6 +++ lib/puppet/indirector/indirection.rb | 2 +- lib/puppet/resource/catalog.rb | 14 +++++++- lib/puppet/transaction.rb | 2 + lib/puppet/type.rb | 9 ++++- spec/integration/indirector/catalog/compiler.rb | 34 +++++++++++++++++++ spec/integration/transaction.rb | 13 +++++++ spec/unit/indirector/catalog/compiler.rb | 34 +++++++++++++++++++ spec/unit/indirector/indirection.rb | 8 ++++ spec/unit/resource/catalog.rb | 41 +++++++++++++++++++++++ spec/unit/transaction.rb | 28 +++++++++++++++ spec/unit/type.rb | 6 +++- 12 files changed, 193 insertions(+), 4 deletions(-) create mode 100755 spec/integration/indirector/catalog/compiler.rb diff --git a/lib/puppet/indirector/catalog/compiler.rb b/lib/puppet/indirector/catalog/compiler.rb index c9a216d..12da4de 100644 --- a/lib/puppet/indirector/catalog/compiler.rb +++ b/lib/puppet/indirector/catalog/compiler.rb @@ -41,6 +41,12 @@ class Puppet::Resource::Catalog::Compiler < Puppet::Indirector::Code end end + # filter-out a catalog to remove exported resources + def filter(catalog) + return catalog.filter { |r| r.exported? } if catalog.respond_to?(:filter) + catalog + end + def initialize set_server_facts end diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb index f16a9b5..dc7e58f 100644 --- a/lib/puppet/indirector/indirection.rb +++ b/lib/puppet/indirector/indirection.rb @@ -202,7 +202,7 @@ class Puppet::Indirector::Indirection cache.save request(:save, result, *args) end - return result + return terminus.respond_to?(:filter) ? terminus.filter(result) : result end return nil diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb index 42e92f4..bb82906 100644 --- a/lib/puppet/resource/catalog.rb +++ b/lib/puppet/resource/catalog.rb @@ -468,6 +468,13 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph to_catalog :to_resource end + # filter out the catalog, applying +block+ to each resource. + # If the block result is false, the resource will + # be kept otherwise it will be skipped + def filter(&block) + to_catalog :to_resource, &block + end + # Store the classes in the classfile. def write_class_file begin @@ -534,7 +541,8 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph map = {} vertices.each do |resource| - next if resource.respond_to?(:virtual?) and resource.virtual? + next if virtual_not_exported?(resource) + next if block_given? and yield resource #This is hackity hack for 1094 #Aliases aren't working in the ral catalog because the current instance of the resource @@ -589,4 +597,8 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph return result end + + def virtual_not_exported?(resource) + resource.respond_to?(:virtual?) and resource.virtual? and (resource.respond_to?(:exported?) and not resource.exported?) + end end diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb index f09ca80..ea283d8 100644 --- a/lib/puppet/transaction.rb +++ b/lib/puppet/transaction.rb @@ -592,6 +592,8 @@ class Transaction resource.debug "Not scheduled" elsif failed_dependencies?(resource) resource.warning "Skipping because of failed dependencies" + elsif resource.exported? + resource.debug "Skipping because exported" else return false end diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb index d300338..e97cb57 100644 --- a/lib/puppet/type.rb +++ b/lib/puppet/type.rb @@ -1839,6 +1839,9 @@ class Type # The catalog that this resource is stored in. attr_accessor :catalog + # is the resource exported + attr_accessor :exported + # create a log at specified level def log(msg) Puppet::Util::Log.create( @@ -1872,7 +1875,7 @@ class Type self.title = resource.ref end - [:file, :line, :catalog].each do |getter| + [:file, :line, :catalog, :exported].each do |getter| setter = getter.to_s + "=" if val = resource.send(getter) self.send(setter, val) @@ -2053,6 +2056,10 @@ class Type return trans end + def exported? + exported + end + end # Puppet::Type end diff --git a/spec/integration/indirector/catalog/compiler.rb b/spec/integration/indirector/catalog/compiler.rb new file mode 100755 index 0000000..74dd2c4 --- /dev/null +++ b/spec/integration/indirector/catalog/compiler.rb @@ -0,0 +1,34 @@ +#!/usr/bin/env ruby + +Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") } + +require 'puppet/resource/catalog' + +Puppet::Resource::Catalog.indirection.terminus(:compiler) + +describe Puppet::Resource::Catalog::Compiler do + before do + @catalog = Puppet::Resource::Catalog.new + + @one = Puppet::Resource.new(:file, "/one") + @one.exported = true + + @two = Puppet::Resource.new(:file, "/two") + @catalog.add_resource(@one, @two) + end + + after { Puppet.settings.clear } + + it "should remove exported resources when filtering" do + Puppet::Resource::Catalog.indirection.terminus.filter(@catalog).resources == [ @two ] + end + + it "should filter out exported resources when finding a catalog" do + request = stub 'request' + Puppet::Resource::Catalog.indirection.terminus.stubs(:extract_facts_from_request) + Puppet::Resource::Catalog.indirection.terminus.stubs(:node_from_request) + Puppet::Resource::Catalog.indirection.terminus.stubs(:compile).returns(@catalog) + + Puppet::Resource::Catalog.indirection.terminus.find(request).resources == [ @two ] + end +end diff --git a/spec/integration/transaction.rb b/spec/integration/transaction.rb index c06a43d..2b8a5d9 100755 --- a/spec/integration/transaction.rb +++ b/spec/integration/transaction.rb @@ -22,4 +22,17 @@ describe Puppet::Transaction do transaction.evaluate end + + it "should not apply exported resources" do + catalog = Puppet::Resource::Catalog.new + resource = Puppet::Type.type(:file).new :path => "/foo/bar", :backup => false + resource.exported = true + catalog.add_resource resource + + transaction = Puppet::Transaction.new(catalog) + + resource.expects(:evaluate).never + + transaction.evaluate + end end diff --git a/spec/unit/indirector/catalog/compiler.rb b/spec/unit/indirector/catalog/compiler.rb index 6e49a65..78a8028 100755 --- a/spec/unit/indirector/catalog/compiler.rb +++ b/spec/unit/indirector/catalog/compiler.rb @@ -223,4 +223,38 @@ describe Puppet::Resource::Catalog::Compiler do @compiler.find(@request) end end + + describe "when filtering resources" do + before :each do + @compiler = Puppet::Resource::Catalog::Compiler.new + @catalog = stub_everything 'catalog' + @catalog.stubs(:respond_to?).with(:filter).returns(true) + end + + it "should delegate to the catalog instance filtering" do + @catalog.expects(:filter) + @compiler.filter(@catalog) + end + + it "should filter out exported resources" do + resource = mock 'resource', :exported? => true + @catalog.stubs(:filter).yields(resource) + + @compiler.filter(@catalog) + end + + it "should return the same catalog if it doesn't support filtering" do + @catalog.stubs(:respond_to?).with(:filter).returns(false) + + @compiler.filter(@catalog).should == @catalog + end + + it "should return the filtered catalog" do + catalog = stub 'filtered catalog' + @catalog.stubs(:filter).returns(catalog) + + @compiler.filter(@catalog).should == catalog + end + + end end diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/indirector/indirection.rb index 5d9efd2..220aa24 100755 --- a/spec/unit/indirector/indirection.rb +++ b/spec/unit/indirector/indirection.rb @@ -243,6 +243,14 @@ describe Puppet::Indirector::Indirection do @indirection.find("/my/key") end + it "should filter the result instance if the terminus supports it" do + @terminus.stubs(:find).returns(@instance) + @terminus.stubs(:respond_to?).with(:filter).returns(true) + + @terminus.expects(:filter).with(@instance) + + @indirection.find("/my/key") + end describe "when caching is enabled" do before do @indirection.cache_class = :cache_terminus diff --git a/spec/unit/resource/catalog.rb b/spec/unit/resource/catalog.rb index 2f4476a..97b6ad7 100755 --- a/spec/unit/resource/catalog.rb +++ b/spec/unit/resource/catalog.rb @@ -295,6 +295,47 @@ describe Puppet::Resource::Catalog, "when compiling" do end end + describe "when filtering" do + before :each do + @original = Puppet::Resource::Catalog.new("mynode") + @original.tag(*%w{one two three}) + @original.add_class *%w{four five six} + + @r1 = stub_everything 'r1', :ref => "File[/a]" + @r1.stubs(:respond_to?).with(:ref).returns(true) + @r1.stubs(:dup).returns(@r1) + @r1.stubs(:is_a?).returns(Puppet::Resource).returns(true) + + @r2 = stub_everything 'r2', :ref => "File[/b]" + @r2.stubs(:respond_to?).with(:ref).returns(true) + @r2.stubs(:dup).returns(@r2) + @r2.stubs(:is_a?).returns(Puppet::Resource).returns(true) + + @resources = [...@r1,@r2] + + @original.add_resource(@r1,@r2) + end + + it "should transform the catalog to a resource catalog" do + @original.expects(:to_catalog).with { |h,b| h == :to_resource } + + @original.filter + end + + it "should scan each catalog resource in turn and apply filtering block" do + @resources.each { |r| r.expects(:exported?) } + @original.filter do |r| + r.exported? + end + end + + it "should filter out resources which produce true when the filter block is evaluated" do + @original.filter do |r| + r == @r1 + end.resource("File[/a]").should be_nil + end + end + describe "when functioning as a resource container" do before do @catalog = Puppet::Resource::Catalog.new("host") diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb index 37870f1..0e36747 100755 --- a/spec/unit/transaction.rb +++ b/spec/unit/transaction.rb @@ -52,6 +52,34 @@ describe Puppet::Transaction do @transaction.generate_additional_resources(generator, :generate).should be_empty end end + + describe "when skipping a resource" do + before :each do + @resource = stub_everything 'res' + @catalog = Puppet::Resource::Catalog.new + @transaction = Puppet::Transaction.new(@catalog) + end + + it "should skip resource with missing tags" do + @transaction.stubs(:missing_tags?).returns(true) + @transaction.skip?(@resource).should be_true + end + + it "should skip not scheduled resources" do + @transaction.stubs(:scheduled?).returns(false) + @transaction.skip?(@resource).should be_true + end + + it "should skip resources with failed dependencies" do + @transaction.stubs(:failed_dependencies?).returns(false) + @transaction.skip?(@resource).should be_true + end + + it "should skip exported resource" do + @resource.stubs(:exported?).returns true + @transaction.skip?(@resource).should be_true + end + end end describe Puppet::Transaction, " when determining tags" do diff --git a/spec/unit/type.rb b/spec/unit/type.rb index a9e48a2..a55009d 100755 --- a/spec/unit/type.rb +++ b/spec/unit/type.rb @@ -68,6 +68,10 @@ describe Puppet::Type do resource.tags = [:tag1,:tag2] end + it "should have a method to know if the resource is exported" do + Puppet::Type.type(:mount).new(:name => "foo").should respond_to(:exported?) + end + describe "when initializing" do describe "and passed a TransObject" do it "should fail" do @@ -87,7 +91,7 @@ describe Puppet::Type do Puppet::Type.type(:mount).new(resource).title.should == "User[foo]" end - [:line, :file, :catalog].each do |param| + [:line, :file, :catalog, :exported].each do |param| it "should copy '#{param}' from the resource if present" do resource = Puppet::Resource.new(:mount, "/foo") resource.send(param.to_s + "=", "foo") -- 1.6.0.2 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
