Clarifying how node names are used during catalog compilation

We now use any passed in node (used by bin/puppet), then any
authenticated node info (e.g., the cert name), then any
passed in information (usually the host name).

The third option will never be used in Puppet's architecture,
but simplifies using the API for other users.

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

2 files changed, 75 insertions, 61 deletions

diff --git a/lib/puppet/indirector/catalog/compiler.rb b/lib/puppet/indirector/catalog/compiler.rb
index df1808d73..aba53bff8 100644
--- a/lib/puppet/indirector/catalog/compiler.rb
+++ b/lib/puppet/indirector/catalog/compiler.rb
@@ -14,8 +14,14 @@ class Puppet::Resource::Catalog::Compiler < Puppet::Indirector::Code
 
     # Compile a node's catalog.
     def find(request)
-        unless node = request.options[:use_node] || find_node(request.key)
-            raise ArgumentError, "Could not find node '%s'; cannot compile" % request.key
+        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
 
         if catalog = compile(node)
@@ -81,15 +87,8 @@ class Puppet::Resource::Catalog::Compiler < Puppet::Indirector::Code
     end
 
     # Turn our host name into a node object.
-    def find_node(key)
-        # If we want to use the cert name as our key
-        # LAK:FIXME This needs to be figured out somehow, but it requires the routing.
-        # This should be able to use the request, yay.
-        #if Puppet[:node_name] == 'cert' and client
-        #    key = client
-        #end
-
-        return nil unless node = Puppet::Node.find(key)
+    def find_node(name)
+        return nil unless node = Puppet::Node.find(name)
 
         # Add any external data to the node.
         add_node_data(node)
diff --git a/spec/unit/indirector/catalog/compiler.rb b/spec/unit/indirector/catalog/compiler.rb
index 0dd07092b..6d1dc761c 100755
--- a/spec/unit/indirector/catalog/compiler.rb
+++ b/spec/unit/indirector/catalog/compiler.rb
@@ -27,8 +27,8 @@ describe Puppet::Resource::Catalog::Compiler do
             Puppet::Node.stubs(:find).with('node1').returns(node1)
             Puppet::Node.stubs(:find).with('node2').returns(node2)
 
-            compiler.find(stub('request', :key => 'node1', :options => {}))
-            compiler.find(stub('node2request', :key => 'node2', :options => {}))
+            compiler.find(stub('request', :node => 'node1', :options => {}))
+            compiler.find(stub('node2request', :node => 'node2', :options => {}))
         end
 
         it "should provide a method for determining if the catalog is networked" do
@@ -58,53 +58,7 @@ describe Puppet::Resource::Catalog::Compiler do
         end
     end
 
-    describe "when finding nodes" do
-        before do
-            Facter.stubs(:value).returns("whatever")
-            @compiler = Puppet::Resource::Catalog::Compiler.new
-            @name = "me"
-            @node = mock 'node'
-            @request = stub 'request', :key => @name, :options => {}
-            @compiler.stubs(:compile)
-        end
-
-        it "should look node information up via the Node class with the provided key" do
-            @node.stubs :merge 
-            Puppet::Node.expects(:find).with(@name).returns(@node)
-            @compiler.find(@request)
-        end
-    end
-
-    describe "after finding nodes" do
-        before do
-            Puppet.expects(:version).returns(1)
-            Facter.expects(:value).with('fqdn').returns("my.server.com")
-            Facter.expects(:value).with('ipaddress').returns("my.ip.address")
-            @compiler = Puppet::Resource::Catalog::Compiler.new
-            @name = "me"
-            @node = mock 'node'
-            @request = stub 'request', :key => @name, :options => {}
-            @compiler.stubs(:compile)
-            Puppet::Node.stubs(:find).with(@name).returns(@node)
-        end
-
-        it "should add the server's Puppet version to the node's parameters as 'serverversion'" do
-            @node.expects(:merge).with { |args| args["serverversion"] == "1" }
-            @compiler.find(@request)
-        end
-
-        it "should add the server's fqdn to the node's parameters as 'servername'" do
-            @node.expects(:merge).with { |args| args["servername"] == "my.server.com" }
-            @compiler.find(@request)
-        end
-
-        it "should add the server's IP address to the node's parameters as 'serverip'" do
-            @node.expects(:merge).with { |args| args["serverip"] == "my.ip.address" }
-            @compiler.find(@request)
-        end
-    end
-
-    describe "when creating catalogs" do
+    describe "when finding catalogs" do
         before do
             Facter.stubs(:value).returns("whatever")
             env = stub 'environment', :name => "yay"
@@ -114,7 +68,7 @@ describe Puppet::Resource::Catalog::Compiler do
             @name = "me"
             @node = Puppet::Node.new @name
             @node.stubs(:merge)
-            @request = stub 'request', :key => @name, :options => {}
+            @request = stub 'request', :key => "does not matter", :node => @name, :options => {}
         end
 
         it "should directly use provided nodes" do
@@ -124,6 +78,21 @@ describe Puppet::Resource::Catalog::Compiler do
             @compiler.find(@request)
         end
 
+        it "should use the request's node name if no explicit node is provided" do
+            Puppet::Node.expects(:find).with(@name).returns(@node)
+            @compiler.expects(:compile).with(@node)
+            @compiler.find(@request)
+        end
+
+        it "should use the provided node name if no explicit node is provided and no authenticated node information is available" do
+            @request.expects(:node).returns nil
+            @request.expects(:key).returns "my_node"
+
+            Puppet::Node.expects(:find).with("my_node").returns @node
+            @compiler.expects(:compile).with(@node)
+            @compiler.find(@request)
+        end
+
         it "should fail if no node is passed and none can be found" do
             Puppet::Node.stubs(:find).with(@name).returns(nil)
             proc { @compiler.find(@request) }.should raise_error(ArgumentError)
@@ -155,4 +124,50 @@ describe Puppet::Resource::Catalog::Compiler do
             @compiler.find(@request)
         end
     end
+
+    describe "when finding nodes" do
+        before do
+            Facter.stubs(:value).returns("whatever")
+            @compiler = Puppet::Resource::Catalog::Compiler.new
+            @name = "me"
+            @node = mock 'node'
+            @request = stub 'request', :node => @name, :options => {}
+            @compiler.stubs(:compile)
+        end
+
+        it "should look node information up via the Node class with the provided key" do
+            @node.stubs :merge 
+            Puppet::Node.expects(:find).with(@name).returns(@node)
+            @compiler.find(@request)
+        end
+    end
+
+    describe "after finding nodes" do
+        before do
+            Puppet.expects(:version).returns(1)
+            Facter.expects(:value).with('fqdn').returns("my.server.com")
+            Facter.expects(:value).with('ipaddress').returns("my.ip.address")
+            @compiler = Puppet::Resource::Catalog::Compiler.new
+            @name = "me"
+            @node = mock 'node'
+            @request = stub 'request', :node => @name, :options => {}
+            @compiler.stubs(:compile)
+            Puppet::Node.stubs(:find).with(@name).returns(@node)
+        end
+
+        it "should add the server's Puppet version to the node's parameters as 'serverversion'" do
+            @node.expects(:merge).with { |args| args["serverversion"] == "1" }
+            @compiler.find(@request)
+        end
+
+        it "should add the server's fqdn to the node's parameters as 'servername'" do
+            @node.expects(:merge).with { |args| args["servername"] == "my.server.com" }
+            @compiler.find(@request)
+        end
+
+        it "should add the server's IP address to the node's parameters as 'serverip'" do
+            @node.expects(:merge).with { |args| args["serverip"] == "my.ip.address" }
+            @compiler.find(@request)
+        end
+    end
 end