Fix #2391 - Exported resources never make to the storeconfigs db

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 <brice-puppet@daysofwonder.com>

10 files changed, 145 insertions, 4 deletions

diff --git a/lib/puppet/indirector/catalog/compiler.rb b/lib/puppet/indirector/catalog/compiler.rb
index c9a216da1..12da4de32 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 f16a9b5dc..dc7e58f36 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 42e92f407..bb82906f3 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 f09ca804b..ea283d8bb 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 6c41d79e3..91f991814 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1847,6 +1847,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(
@@ -1880,7 +1883,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)
@@ -2061,6 +2064,10 @@ class Type
         return trans
     end
 
+    def exported?
+        exported
+    end
+
 end # Puppet::Type
 end
 
diff --git a/spec/unit/indirector/catalog/compiler.rb b/spec/unit/indirector/catalog/compiler.rb
index 6e49a6529..78a8028a8 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 5d9efd2ea..220aa24fe 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 2f4476a2b..97b6ad7cc 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 37870f126..26154e9b9 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -52,6 +52,33 @@ 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', :exported? => true
+            @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
+            @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 a9e48a274..a55009dfc 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")