Fixing #2149 - Facts are passed as part of the catalog request

This removes the requirement of shared fact caching
on the servers, since the server responding to the catalog
request will receive the facts as part of the request.

The facts are serialized as a parameter to the request,
rather than each being set as a separate request parameter.

This hard-codes yaml as the serialization format for the
facts, because I couldn't get marshal to work and it's just not
as big a deal for such a small amount of data.

Signed-off-by: Luke Kanies <luke@madstop.com>

8 files changed, 150 insertions, 40 deletions

diff --git a/lib/puppet/application/puppetd.rb b/lib/puppet/application/puppetd.rb
index cacb84361..7a92db11d 100644
--- a/lib/puppet/application/puppetd.rb
+++ b/lib/puppet/application/puppetd.rb
@@ -222,7 +222,6 @@ Puppet::Application.new(:puppetd) do
         Puppet::Resource::Catalog.cache_class = :yaml
 
         Puppet::Node::Facts.terminus_class = :facter
-        Puppet::Node::Facts.cache_class = :rest
 
         # We need tomake the client either way, we just don't start it
         # if --no-client is set.
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index e6fdbee15..b74456de4 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -75,8 +75,6 @@ class Puppet::Configurer
         download_plugins()
 
         download_fact_plugins()
-
-        upload_facts()
     end
 
     # Get the remote catalog, yo.  Returns nil if no catalog can be found.
@@ -84,11 +82,16 @@ class Puppet::Configurer
         name = Facter.value("hostname")
         catalog_class = Puppet::Resource::Catalog
 
+        # This is a bit complicated.  We need the serialized and escaped facts,
+        # and we need to know which format they're encoded in.  Thus, we
+        # get a hash with both of these pieces of information.
+        fact_options = facts_for_uploading()
+
         # First try it with no cache, then with the cache.
         result = nil
         begin
             duration = thinmark do
-                result = catalog_class.find(name, :ignore_cache => true)
+                result = catalog_class.find(name, fact_options.merge(:ignore_cache => true))
             end
         rescue => detail
             puts detail.backtrace if Puppet[:trace]
@@ -98,7 +101,7 @@ class Puppet::Configurer
         unless result
             begin
                 duration = thinmark do
-                    result = catalog_class.find(name, :ignore_terminus => true)
+                    result = catalog_class.find(name, fact_options.merge(:ignore_terminus => true))
                 end
             rescue => detail
                 puts detail.backtrace if Puppet[:trace]
diff --git a/lib/puppet/configurer/fact_handler.rb b/lib/puppet/configurer/fact_handler.rb
index 8a6de5e9f..4135427c9 100644
--- a/lib/puppet/configurer/fact_handler.rb
+++ b/lib/puppet/configurer/fact_handler.rb
@@ -10,9 +10,7 @@ module Puppet::Configurer::FactHandler
         Puppet[:factsync]
     end
 
-    def upload_facts
-        # XXX down = Puppet[:downcasefacts]
-
+    def find_facts
         reload_facter()
 
         # This works because puppetd configures Facts to use 'facter' for
@@ -22,10 +20,22 @@ module Puppet::Configurer::FactHandler
             Puppet::Node::Facts.find(Puppet[:certname])
         rescue => detail
             puts detail.backtrace if Puppet[:trace]
-            Puppet.err("Could not retrieve local facts: %s" % detail)
+            raise Puppet::Error, "Could not retrieve local facts: %s" % detail
         end
     end
 
+    def facts_for_uploading
+        facts = find_facts
+        #format = facts.class.default_format
+
+        # Hard-code yaml, because I couldn't get marshal to work.
+        format = :yaml
+
+        text = facts.render(format)
+
+        return {:facts_format => format, :facts => URI.escape(text)}
+    end
+
     # Retrieve facts from the central server.
     def download_fact_plugins
         return unless download_fact_plugins?
diff --git a/lib/puppet/indirector/catalog/compiler.rb b/lib/puppet/indirector/catalog/compiler.rb
index 47635d88c..c9a216da1 100644
--- a/lib/puppet/indirector/catalog/compiler.rb
+++ b/lib/puppet/indirector/catalog/compiler.rb
@@ -12,17 +12,25 @@ class Puppet::Resource::Catalog::Compiler < Puppet::Indirector::Code
 
     attr_accessor :code
 
+    def extract_facts_from_request(request)
+        return unless text_facts = request.options[:facts]
+        raise ArgumentError, "Facts but no fact format provided for %s" % request.name unless format = request.options[:facts_format]
+
+        # If the facts were encoded as yaml, then the param reconstitution system
+        # in Network::HTTP::Handler will automagically deserialize the value.
+        if text_facts.is_a?(Puppet::Node::Facts)
+            facts = text_facts
+        else
+            facts = Puppet::Node::Facts.convert_from(format, text_facts)
+        end
+        facts.save
+    end
+
     # Compile a node's catalog.
     def find(request)
-        unless node = request.options[:use_node]
-            # If the request is authenticated, then the 'node' info will
-            # be available; if not, then we use the passed-in key.  We rely
-            # on our authorization system to determine whether this is allowed.
-            name = request.node || request.key
-            unless node = find_node(name)
-                raise ArgumentError, "Could not find node '%s'; cannot compile" % name
-            end
-        end
+        extract_facts_from_request(request)
+
+        node = node_from_request(request)
 
         if catalog = compile(node)
             return catalog
@@ -102,6 +110,24 @@ class Puppet::Resource::Catalog::Compiler < Puppet::Indirector::Code
         node
     end
 
+    # Extract the node from the request, or use the request
+    # to find the node.
+    def node_from_request(request)
+        if node = request.options[:use_node]
+            return node
+        end
+
+        # If the request is authenticated, then the 'node' info will
+        # be available; if not, then we use the passed-in key.  We rely
+        # on our authorization system to determine whether this is allowed.
+        name = request.node || request.key
+        if node = find_node(name)
+            return node
+        end
+
+        raise ArgumentError, "Could not find node '%s'; cannot compile" % name
+    end
+
     # Initialize our server fact hash; we add these to each client, and they
     # won't change while we're running, so it's safe to cache the values.
     def set_server_facts
diff --git a/spec/unit/application/puppetd.rb b/spec/unit/application/puppetd.rb
index d93a52761..dcd86185c 100755
--- a/spec/unit/application/puppetd.rb
+++ b/spec/unit/application/puppetd.rb
@@ -20,7 +20,6 @@ describe "puppetd" do
         Puppet::Node.stubs(:terminus_class=)
         Puppet::Node.stubs(:cache_class=)
         Puppet::Node::Facts.stubs(:terminus_class=)
-        Puppet::Node::Facts.stubs(:cache_class=)
     end
 
     it "should ask Puppet::Application to parse Puppet configuration file" do
@@ -164,7 +163,6 @@ describe "puppetd" do
             Puppet::Resource::Catalog.stubs(:terminus_class=)
             Puppet::Resource::Catalog.stubs(:cache_class=)
             Puppet::Node::Facts.stubs(:terminus_class=)
-            Puppet::Node::Facts.stubs(:cache_class=)
             @host = stub_everything 'host'
             Puppet::SSL::Host.stubs(:new).returns(@host)
             Puppet.stubs(:settraps)
@@ -297,12 +295,6 @@ describe "puppetd" do
             @puppetd.run_setup
         end
 
-        it "should tell the facts cache facts through REST" do
-            Puppet::Node::Facts.expects(:cache_class=).with(:rest)
-
-            @puppetd.run_setup
-        end
-
         it "should create an agent" do
             Puppet::Agent.stubs(:new).with(Puppet::Configurer)
 
diff --git a/spec/unit/configurer.rb b/spec/unit/configurer.rb
index 067283a03..712cbf715 100755
--- a/spec/unit/configurer.rb
+++ b/spec/unit/configurer.rb
@@ -25,6 +25,7 @@ describe Puppet::Configurer, "when executing a catalog run" do
     before do
         Puppet.settings.stubs(:use).returns(true)
         @agent = Puppet::Configurer.new
+        @agent.stubs(:facts_for_uploading).returns({})
     end
 
     it "should prepare for the run" do
@@ -70,6 +71,7 @@ describe Puppet::Configurer, "when retrieving a catalog" do
     before do
         Puppet.settings.stubs(:use).returns(true)
         @agent = Puppet::Configurer.new
+        @agent.stubs(:facts_for_uploading).returns({})
 
         @catalog = Puppet::Resource::Catalog.new
 
@@ -90,6 +92,13 @@ describe Puppet::Configurer, "when retrieving a catalog" do
         @agent.retrieve_catalog
     end
 
+    it "should pass the prepared facts and the facts format as arguments when retrieving the catalog" do
+        @agent.expects(:facts_for_uploading).returns(:facts => "myfacts", :facts_format => :foo)
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:facts] == "myfacts" and options[:facts_format] == :foo }.returns @catalog
+
+        @agent.retrieve_catalog
+    end
+
     it "should default to returning a catalog retrieved directly from the server, skipping the cache" do
         Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.returns @catalog
 
@@ -177,7 +186,6 @@ describe Puppet::Configurer, "when preparing for a run" do
         Puppet.settings.stubs(:use).returns(true)
         @agent = Puppet::Configurer.new
         @agent.stubs(:dostorage)
-        @agent.stubs(:upload_facts)
         @facts = {"one" => "two", "three" => "four"}
     end
 
@@ -200,10 +208,4 @@ describe Puppet::Configurer, "when preparing for a run" do
 
         @agent.prepare
     end
-
-    it "should upload facts to use for catalog retrieval" do
-        @agent.stubs(:dostorage)
-        @agent.expects(:upload_facts)
-        @agent.prepare
-    end
 end
diff --git a/spec/unit/configurer/fact_handler.rb b/spec/unit/configurer/fact_handler.rb
index 43f6e4f60..504eaaf04 100755
--- a/spec/unit/configurer/fact_handler.rb
+++ b/spec/unit/configurer/fact_handler.rb
@@ -52,29 +52,59 @@ describe Puppet::Configurer::FactHandler do
         @facthandler.download_fact_plugins
     end
 
-    it "should have a method for uploading facts" do
-        @facthandler.should respond_to(:upload_facts)
+    it "should have a method for retrieving facts" do
+        @facthandler.should respond_to(:find_facts)
     end
 
-    it "should reload Facter and find local facts when asked to upload facts" do
+    it "should use the Facts class with the :certname to find the facts" do
+        Puppet.settings.expects(:value).with(:certname).returns "foo"
+        Puppet::Node::Facts.expects(:find).with("foo").returns "myfacts"
+        @facthandler.stubs(:reload_facter)
+        @facthandler.find_facts.should == "myfacts"
+    end
+
+    it "should reload Facter and find local facts when asked to find facts" do
         @facthandler.expects(:reload_facter)
 
         Puppet.settings.expects(:value).with(:certname).returns "myhost"
         Puppet::Node::Facts.expects(:find).with("myhost")
 
-        @facthandler.upload_facts
+        @facthandler.find_facts
     end
 
-    it "should not fail if uploading facts fails" do
+    it "should fail if finding facts fails" do
         @facthandler.stubs(:reload_facter)
 
         Puppet.settings.stubs(:value).with(:trace).returns false
         Puppet.settings.stubs(:value).with(:certname).returns "myhost"
         Puppet::Node::Facts.expects(:find).raises RuntimeError
 
-        Puppet.expects(:err)
+        lambda { @facthandler.find_facts }.should raise_error(Puppet::Error)
+    end
+
+    it "should have a method to prepare the facts for uploading" do
+        @facthandler.should respond_to(:facts_for_uploading)
+    end
+
+    # I couldn't get marshal to work for this, only yaml, so we hard-code yaml.
+    it "should serialize and URI escape the fact values for uploading" do
+        facts = stub 'facts'
+        facts.expects(:render).returns "my text"
+        text = URI.escape("my text")
+
+        @facthandler.expects(:find_facts).returns facts
+
+        @facthandler.facts_for_uploading.should == {:facts_format => :yaml, :facts => text}
+    end
+
+    it "should hard-code yaml as the serialization" do
+        facts = stub 'facts'
+        facts.expects(:render).with(:yaml).returns "my text"
+        text = URI.escape("my text")
+
+        @facthandler.expects(:find_facts).returns facts
 
-        lambda { @facthandler.upload_facts }.should_not raise_error
+        @facthandler.facts_for_uploading
     end
 
     describe "when reloading Facter" do
diff --git a/spec/unit/indirector/catalog/compiler.rb b/spec/unit/indirector/catalog/compiler.rb
index 5518169b1..9dac76f0e 100755
--- a/spec/unit/indirector/catalog/compiler.rb
+++ b/spec/unit/indirector/catalog/compiler.rb
@@ -110,6 +110,12 @@ describe Puppet::Resource::Catalog::Compiler do
             @compiler.find(@request)
         end
 
+        it "should extract and save any facts from the request" do
+            @compiler.expects(:extract_facts_from_request).with(@request)
+            @compiler.interpreter.stubs(:compile)
+            @compiler.find(@request)
+        end
+
         it "should return the results of compiling as the catalog" do
             Puppet::Node.stubs(:find).returns(@node)
             config = mock 'config'
@@ -130,6 +136,48 @@ describe Puppet::Resource::Catalog::Compiler do
         end
     end
 
+    describe "when extracting facts from the request" do
+        before do
+            @compiler = Puppet::Resource::Catalog::Compiler.new
+            @request = stub 'request', :options => {}
+
+            @facts = stub 'facts', :save => nil
+        end
+
+        it "should do nothing if no facts are provided" do
+            Puppet::Node::Facts.expects(:convert_from).never
+            @request.options[:facts] = nil
+
+            @compiler.extract_facts_from_request(@request)
+        end
+
+        it "should use the Facts class to deserialize the provided facts" do
+            @request.options[:facts_format] = "foo"
+            @request.options[:facts] = "bar"
+            Puppet::Node::Facts.expects(:convert_from).returns @facts
+
+            @compiler.extract_facts_from_request(@request)
+        end
+
+        it "should use the provided fact format" do
+            @request.options[:facts_format] = "foo"
+            @request.options[:facts] = "bar"
+            Puppet::Node::Facts.expects(:convert_from).with { |format, text| format == "foo" }.returns @facts
+
+            @compiler.extract_facts_from_request(@request)
+        end
+
+        it "should convert the facts into a fact instance and save it" do
+            @request.options[:facts_format] = "foo"
+            @request.options[:facts] = "bar"
+            Puppet::Node::Facts.expects(:convert_from).returns @facts
+
+            @facts.expects(:save)
+
+            @compiler.extract_facts_from_request(@request)
+        end
+    end
+
     describe "when finding nodes" do
         before do
             Facter.stubs(:value).returns("whatever")