Fix #2507 - Exported resources were not correctly collected.

#2507 contains two issues:
* a crash when we filters-out an unwanted resource which had edges
pointing to it.

* resources are losing their virtuality when they are transformed from
Puppet::Parser::Resource to Puppet::Resource. This means we weren't able
to distinguish anymore between an exported resource collected in the same
node as it was exported and an exported resource collected in another node.
The net result is that we can't apply exported resources that are
collected in the same node because they are filtered out by the catalog
filter (see the commits for #2391 for more information).

The fix is to keep the virtuality of the resources so that we can
differentiate those two types of exported resources. We keep this until
the catalog is ready to be sent, where we filter out the virtual resouces
only, the other still exported ones needs to be sent to the client.
To be real sure, the transaction also skips virtual resources.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

12 files changed, 46 insertions, 19 deletions

diff --git a/lib/puppet/indirector/catalog/compiler.rb b/lib/puppet/indirector/catalog/compiler.rb
index 12da4de32..4f6b0602a 100644
--- a/lib/puppet/indirector/catalog/compiler.rb
+++ b/lib/puppet/indirector/catalog/compiler.rb
@@ -43,7 +43,7 @@ class Puppet::Resource::Catalog::Compiler < Puppet::Indirector::Code
 
     # filter-out a catalog to remove exported resources
     def filter(catalog)
-        return catalog.filter { |r| r.exported? } if catalog.respond_to?(:filter)
+        return catalog.filter { |r| r.virtual? } if catalog.respond_to?(:filter)
         catalog
     end
 
diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
index 6632d2bba..7218ac0c0 100644
--- a/lib/puppet/parser/resource.rb
+++ b/lib/puppet/parser/resource.rb
@@ -268,6 +268,7 @@ class Puppet::Parser::Resource
         result.file = self.file
         result.line = self.line
         result.exported = self.exported
+        result.virtual = self.virtual
         result.tag(*self.tags)
 
         return result
diff --git a/lib/puppet/resource.rb b/lib/puppet/resource.rb
index 0de089cd7..134076580 100644
--- a/lib/puppet/resource.rb
+++ b/lib/puppet/resource.rb
@@ -9,7 +9,7 @@ class Puppet::Resource
     include Puppet::Util::Tagging
     extend Puppet::Util::Json
     include Enumerable
-    attr_accessor :file, :line, :catalog, :exported
+    attr_accessor :file, :line, :catalog, :exported, :virtual
     attr_writer :type, :title
 
     ATTRIBUTES = [:file, :line, :exported]
@@ -106,8 +106,10 @@ class Puppet::Resource
         @parameters.each { |p,v| yield p, v }
     end
 
-    def exported?
-        exported
+    %w{exported virtual}.each do |m|
+        define_method(m+"?") do
+            self.send(m)
+        end
     end
 
     # Create our resource.
diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index bb82906f3..6ccfe73ad 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -576,8 +576,11 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
         message = convert.to_s.gsub "_", " "
         edges.each do |edge|
             # Skip edges between virtual resources.
-            next if edge.source.respond_to?(:virtual?) and edge.source.virtual?
-            next if edge.target.respond_to?(:virtual?) and edge.target.virtual?
+            next if virtual_not_exported?(edge.source)
+            next if block_given? and yield edge.source
+
+            next if virtual_not_exported?(edge.target)
+            next if block_given? and yield edge.target
 
             unless source = map[edge.source.ref]
                 raise Puppet::DevError, "Could not find resource %s when converting %s resources" % [edge.source.ref, message]
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index f57eda6c9..d04856d39 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -590,8 +590,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"
+        elsif resource.virtual?
+            resource.debug "Skipping because virtual"
         else
             return false
         end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 33b4e92da..ee87c2680 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1855,6 +1855,9 @@ class Type
     # is the resource exported
     attr_accessor :exported
 
+    # is the resource virtual (it should not :-))
+    attr_accessor :virtual
+
     # create a log at specified level
     def log(msg)
         Puppet::Util::Log.create(
@@ -1888,7 +1891,7 @@ class Type
             self.title = resource.ref
         end
 
-        [:file, :line, :catalog, :exported].each do |getter|
+        [:file, :line, :catalog, :exported, :virtual].each do |getter|
             setter = getter.to_s + "="
             if val = resource.send(getter)
                 self.send(setter, val)
@@ -2069,8 +2072,10 @@ class Type
         return trans
     end
 
-    def exported?
-        exported
+    %w{exported virtual}.each do |m|
+        define_method(m+"?") do
+            self.send(m)
+        end
     end
 
 end # Puppet::Type
diff --git a/spec/integration/indirector/catalog/compiler.rb b/spec/integration/indirector/catalog/compiler.rb
index f3ace8d1b..211b7c237 100755
--- a/spec/integration/indirector/catalog/compiler.rb
+++ b/spec/integration/indirector/catalog/compiler.rb
@@ -11,7 +11,7 @@ describe Puppet::Resource::Catalog::Compiler do
         @catalog = Puppet::Resource::Catalog.new
 
         @one = Puppet::Resource.new(:file, "/one")
-        @one.exported = true
+        @one.virtual = true
 
         @two = Puppet::Resource.new(:file, "/two")
         @catalog.add_resource(@one, @two)
diff --git a/spec/unit/indirector/catalog/compiler.rb b/spec/unit/indirector/catalog/compiler.rb
index 78a8028a8..7f0942221 100755
--- a/spec/unit/indirector/catalog/compiler.rb
+++ b/spec/unit/indirector/catalog/compiler.rb
@@ -236,8 +236,8 @@ describe Puppet::Resource::Catalog::Compiler do
             @compiler.filter(@catalog)
         end
 
-        it "should filter out exported resources" do
-            resource = mock 'resource', :exported? => true
+        it "should filter out virtual resources" do
+            resource = mock 'resource', :virtual? => true
             @catalog.stubs(:filter).yields(resource)
 
             @compiler.filter(@catalog)
diff --git a/spec/unit/parser/resource.rb b/spec/unit/parser/resource.rb
index 8005e204c..9eb8b13d9 100755
--- a/spec/unit/parser/resource.rb
+++ b/spec/unit/parser/resource.rb
@@ -419,6 +419,11 @@ describe Puppet::Parser::Resource do
             @parser_resource.to_resource.exported.should be_true
         end
 
+        it "should copy over the 'virtual' value" do
+            @parser_resource.virtual = true
+            @parser_resource.to_resource.virtual.should be_true
+        end
+
         it "should convert any parser resource references to Puppet::Resource::Reference instances" do
             ref = Puppet::Parser::Resource::Reference.new(:title => "/my/file", :type => "file")
             @parser_resource = mkresource :source => @source, :params => {:foo => "bar", :fee => ref}
diff --git a/spec/unit/resource/catalog.rb b/spec/unit/resource/catalog.rb
index 97b6ad7cc..af399aa0f 100755
--- a/spec/unit/resource/catalog.rb
+++ b/spec/unit/resource/catalog.rb
@@ -323,9 +323,9 @@ describe Puppet::Resource::Catalog, "when compiling" do
         end
 
         it "should scan each catalog resource in turn and apply filtering block" do
-            @resources.each { |r| r.expects(:exported?) }
+            @resources.each { |r| r.expects(:test?) }
             @original.filter do |r|
-                r.exported?
+                r.test?
             end
         end
 
@@ -334,6 +334,13 @@ describe Puppet::Resource::Catalog, "when compiling" do
                 r == @r1
             end.resource("File[/a]").should be_nil
         end
+
+        it "should not consider edges against resources that were filtered out" do
+            @original.add_edge(@r1,@r2)
+            @original.filter do |r|
+                r == @r1
+            end.edge(@r1,@r2).should be_empty
+        end
     end
 
     describe "when functioning as a resource container" do
diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb
index 0e3674775..7966c7a65 100755
--- a/spec/unit/transaction.rb
+++ b/spec/unit/transaction.rb
@@ -75,8 +75,8 @@ describe Puppet::Transaction do
             @transaction.skip?(@resource).should be_true
         end
 
-        it "should skip exported resource" do
-            @resource.stubs(:exported?).returns true
+        it "should skip virtual resource" do
+            @resource.stubs(:virtual?).returns true
             @transaction.skip?(@resource).should be_true
         end
     end
diff --git a/spec/unit/type.rb b/spec/unit/type.rb
index b179677f9..fe2788ec6 100755
--- a/spec/unit/type.rb
+++ b/spec/unit/type.rb
@@ -72,6 +72,10 @@ describe Puppet::Type do
         Puppet::Type.type(:mount).new(:name => "foo").should respond_to(:exported?)
     end
 
+    it "should have a method to know if the resource is virtual" do
+        Puppet::Type.type(:mount).new(:name => "foo").should respond_to(:virtual?)
+    end
+
     it "should consider its version to be its catalog version" do
         resource = Puppet::Type.type(:mount).new(:name => "foo")
         catalog = Puppet::Resource::Catalog.new
@@ -121,7 +125,7 @@ describe Puppet::Type do
                 Puppet::Type.type(:mount).new(resource).title.should == "User[foo]"
             end
 
-            [:line, :file, :catalog, :exported].each do |param|
+            [:line, :file, :catalog, :exported, :virtual].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")