Always using the cert name to store yaml files, which fixes #1178.

The Master handler previously provided the support for the :node_name
setting, and that functionality has now been moved into the Node
class.  At the same time, the names to search through have been
changed somewhat:  Previously, the certificate name and the
hostname were both used for searching, but now, the cert name
is always searched first (unless node_name == facter), but only
the Facter hostname, domain, and fqdn are used otherwise.  We no
longer split the cert name, only the hostname/domain/fqdn.

In the general case, this provides no behaviour change, because
people's hostname is the same as their certname.  This only
results in a change in behaviour if you specify a certificate
name that is a normal node name, and you want to look that node
up by something other than the full name in the certificate.

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

6 files changed, 148 insertions, 157 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 7edb7bd3d..da9227fc2 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,13 @@
+    Always using the cert name to store yaml files, which fixes #1178.
+    The Master handler previously provided the support for the :node_name
+    setting, and that functionality has now been moved into the Node
+    class.  At the same time, the names to search through have been
+    changed somewhat:  Previously, the certificate name and the 
+    hostname were both used for searching, but now, the cert name
+    is always searched first (unless node_name == facter), but only
+    the Facter hostname, domain, and fqdn are used otherwise.  We no
+    longer split the cert name, only the hostname/domain/fqdn.
+
     Fixing transaction support for prefetching generated resources.
 
     Adding support for settings within the existing Facter provider confines.
diff --git a/lib/puppet/network/handler/master.rb b/lib/puppet/network/handler/master.rb
index 851ccc7b2..a050b089b 100644
--- a/lib/puppet/network/handler/master.rb
+++ b/lib/puppet/network/handler/master.rb
@@ -56,7 +56,8 @@ class Puppet::Network::Handler
         # Call our various handlers; this handler is getting deprecated.
         def getconfig(facts, format = "marshal", client = nil, clientip = nil)
             facts = decode_facts(facts)
-            client, clientip = clientname(client, clientip, facts)
+
+            client ||= facts["hostname"]
 
             # Pass the facts to the fact handler
             Puppet::Node::Facts.new(client, facts).save unless local?
@@ -66,27 +67,6 @@ class Puppet::Network::Handler
             return translate(catalog.extract)
         end
 
-        private
-
-        # Manipulate the client name as appropriate.
-        def clientname(name, ip, facts)
-            # Always use the hostname from Facter.
-            client = facts["hostname"]
-            clientip = facts["ipaddress"]
-            if Puppet[:node_name] == 'cert'
-                if name
-                    client = name
-                    facts["fqdn"] = client
-                    facts["hostname"], facts["domain"] = client.split('.', 2)
-                end
-                if ip
-                    clientip = ip
-                end
-            end
-
-            return client, clientip
-        end
-
         # 
         def decode_facts(facts)
             if @local
diff --git a/lib/puppet/node.rb b/lib/puppet/node.rb
index c0628ecdc..576e2265d 100644
--- a/lib/puppet/node.rb
+++ b/lib/puppet/node.rb
@@ -18,9 +18,8 @@ class Puppet::Node
     def self.find_by_any_name(key)
         return nil unless key
 
-        facts = node_facts(key)
         node = nil
-        names = node_names(key, facts)
+        names = node_names(key)
         names.each do |name|
             name = name.to_s if name.is_a?(Symbol)
             break if node = find(name)
@@ -34,13 +33,16 @@ class Puppet::Node
             end
         end
 
-        if node
-            node.names = names
+        return nil unless node
 
-            return node
-        else
-            return nil
-        end
+        node.names = names
+
+        # This is critical, because it forces our node's name to always
+        # be the key, which is nearly always the node's certificate.
+        # This is how the node instance is linked to the Facts instance,
+        # so it quite matters.
+        node.name = key
+        return node
     end
 
     private
@@ -60,33 +62,34 @@ class Puppet::Node
         facts ||= node_facts(key)
         names = []
 
-        if hostname = facts["hostname"]
-            unless hostname == key
-                names << hostname
+        # First, get the fqdn
+        unless fqdn = facts["fqdn"]
+            if domain = facts["domain"]
+                fqdn = facts["hostname"] + "." + facts["domain"]
             end
-        else
-            hostname = key
-        end
-
-        if fqdn = facts["fqdn"]
-            hostname = fqdn
-            names << fqdn
         end
 
-        # Make sure both the fqdn and the short name of the
-        # host can be used in the manifest
-        if hostname =~ /\./
-            names << hostname.sub(/\..+/,'')
-        elsif domain = facts['domain']
-            names << hostname + "." + domain
+        # Now that we (might) have the fqdn, add each piece to the name
+        # list to search, in order of longest to shortest.
+        if fqdn
+            list = fqdn.split(".")
+            tmp = []
+            list.each_with_index do |short, i|
+                tmp << list[0..i].join(".")
+            end
+            names += tmp.reverse
         end
 
-        # Sort the names inversely by name length.
-        names.sort! { |a,b| b.length <=> a.length }
-
         # And make sure the key is first, since that's the most
         # likely usage.
-        ([key] + names).uniq
+        #   The key is usually the Certificate CN, but it can be
+        # set to the 'facter' hostname instead.
+        if Puppet[:node_name] == 'cert'
+            names.unshift key
+        else
+            names.unshift facts["hostname"]
+        end
+        names.uniq
     end
 
     public
diff --git a/spec/unit/node.rb b/spec/unit/node.rb
index 421fcd635..6603f6b58 100755
--- a/spec/unit/node.rb
+++ b/spec/unit/node.rb
@@ -2,7 +2,7 @@
 
 require File.dirname(__FILE__) + '/../spec_helper'
 
-describe Puppet::Node, " when initializing" do
+describe Puppet::Node, "when initializing" do
     before do
         @node = Puppet::Node.new("testnode")
     end
@@ -61,7 +61,7 @@ describe Puppet::Node, " when initializing" do
     end
 end
 
-describe Puppet::Node, " when returning the environment" do
+describe Puppet::Node, "when returning the environment" do
     before do
         Puppet.settings.stubs(:value).with(:environments).returns("one,two")
         Puppet.settings.stubs(:value).with(:environment).returns("one")
@@ -90,7 +90,7 @@ describe Puppet::Node, " when returning the environment" do
     end
 end
 
-describe Puppet::Node, " when merging facts" do
+describe Puppet::Node, "when merging facts" do
     before do
         @node = Puppet::Node.new("testnode")
         Puppet::Node::Facts.stubs(:find).with(@node.name).returns(Puppet::Node::Facts.new(@node.name, "one" => "c", "two" => "b"))
@@ -115,7 +115,7 @@ describe Puppet::Node, " when merging facts" do
     end
 end
 
-describe Puppet::Node, " when indirecting" do
+describe Puppet::Node, "when indirecting" do
     it "should redirect to the indirection" do
         @indirection = stub 'indirection', :name => :node
         Puppet::Node.stubs(:indirection).returns(@indirection)
@@ -143,68 +143,92 @@ describe Puppet::Node do
     it "should provide a method for noting that the node has connected"
 end
 
-describe Puppet::Node, " when searching for nodes" do
+describe Puppet::Node, "when generating the list of names to search through" do
     before do
-        @searcher = Puppet::Node
         @facts = Puppet::Node::Facts.new("foo", "hostname" => "yay", "domain" => "domain.com")
         @node = Puppet::Node.new("foo")
-        Puppet::Node::Facts.stubs(:find).with("foo").returns(@facts)
+
+        Puppet::Node.stubs(:node_facts).returns @facts.values
     end
 
-    it "should return the first node found using the generated list of names" do
-        @searcher.expects(:find).with("foo").returns(nil)
-        @searcher.expects(:find).with("yay.domain.com").returns(@node)
-        @searcher.find_by_any_name("foo").should equal(@node)
+    it "should return an array of names" do
+        Puppet::Node.node_names("foo").should be_instance_of(Array)
     end
 
-    it "should search for the node by its key first" do
-        names = []
-        @searcher.expects(:find).with do |name|
-            names << name
-            names == %w{foo}
-        end.returns(@node)
-        @searcher.find_by_any_name("foo").should equal(@node)
+    it "should have the node's fqdn as the second name" do
+        Puppet::Node.node_names("foo.domain.com")[1].should == "yay.domain.com"
     end
 
-    it "should search for the rest of the names inversely by length" do
-        names = []
-        @facts.values["fqdn"] = "longer.than.the.normal.fqdn.com"
-        @searcher.stubs(:find).with do |name|
-            names << name
+    it "should set the fqdn to the node's 'fqdn' fact if it is available" do
+        @facts.values["fqdn"] = "boo.domain.com"
+        Puppet::Node.node_names("foo")[1].should == "boo.domain.com"
+    end
+
+    it "should set the fqdn to the node's hostname and domain if no fqdn is available" do
+        Puppet::Node.node_names("foo")[1].should == "yay.domain.com"
+    end
+
+    it "should contain an entry for each name available by stripping a segment of the fqdn" do
+        @facts.values["fqdn"] = "foo.deep.sub.domain.com"
+        Puppet::Node.node_names("foo")[2].should == "foo.deep.sub.domain"
+        Puppet::Node.node_names("foo")[3].should == "foo.deep.sub"
+    end
+
+    describe "and :node_name is set to 'cert'" do
+        before do
+            Puppet.settings.stubs(:value).with(:node_name).returns "cert"
         end
-        @searcher.find_by_any_name("foo")
-        # Strip off the key
-        names.shift
 
-        # And the 'default'
-        names.pop
+        it "should use the passed-in key as the first value" do
+            Puppet::Node.node_names("foo")[0].should == "foo"
+        end
+    end
 
-        length = 100
-        names.each do |name|
-            (name.length < length).should be_true
-            length = name.length
+    describe "and :node_name is set to 'facter'" do
+        before do
+            Puppet.settings.stubs(:value).with(:node_name).returns "facter"
         end
+
+        it "should use the node's 'hostname' fact as the first value" do
+            Puppet::Node.node_names("foo")[0].should == "yay"
+        end
+    end
+end
+
+describe Puppet::Node, "when searching for nodes" do
+    before do
+        @facts = Puppet::Node::Facts.new("foo", "hostname" => "yay", "domain" => "domain.com")
+        @node = Puppet::Node.new("foo")
+        Puppet::Node::Facts.stubs(:find).with("foo").returns(@facts)
+    end
+
+    it "should use the 'node_names' method to get its list of names to search" do
+        Puppet::Node.expects(:node_names).with{ |*args| args[0] == "foo" }.returns %w{a b}
+        Puppet::Node.stubs(:find)
+        Puppet::Node.find_by_any_name("foo")
+    end
+
+    it "should return the first node found using the generated list of names" do
+        Puppet::Node.expects(:node_names).returns %w{a b}
+        Puppet::Node.expects(:find).with("a").returns(nil)
+        Puppet::Node.expects(:find).with("b").returns(@node)
+        Puppet::Node.find_by_any_name("foo").should equal(@node)
     end
 
     it "should attempt to find a default node if no names are found" do
         names = []
-        @searcher.stubs(:find).with do |name|
+        Puppet::Node.stubs(:find).with do |name|
             names << name
         end.returns(nil)
-        @searcher.find_by_any_name("foo")
+        Puppet::Node.find_by_any_name("foo")
         names[-1].should == "default"
     end
 
-    it "should flush the node cache using the :filetimeout parameter" do
-        node2 = Puppet::Node.new("foo2")
-        Puppet[:filetimeout] = -1
-        # I couldn't get this to work with :expects
-        @searcher.stubs(:find).returns(@node, node2).then.raises(ArgumentError)
-        @searcher.find_by_any_name("foo").should equal(@node)
-        @searcher.find_by_any_name("foo").should equal(node2)
-    end
+    it "should set the node name to the provided key" do
+        Puppet::Node.stubs(:node_names).returns %w{a b}
+        Puppet::Node.stubs(:find).returns @node
 
-    after do
-        Puppet.settings.clear
+        @node.expects(:name=).with("foo")
+        Puppet::Node.find_by_any_name("foo")
     end
 end
diff --git a/test/network/client/master.rb b/test/network/client/master.rb
index dc2140e62..9f71247fa 100755
--- a/test/network/client/master.rb
+++ b/test/network/client/master.rb
@@ -472,17 +472,8 @@ end
 
         Puppet::Node::Facts.indirection.stubs(:save)
 
-        master = client = nil
-        assert_nothing_raised() {
-            master = Puppet::Network::Handler.master.new(
-                :Local => false
-            )
-        }
-        assert_nothing_raised() {
-            client = Puppet::Network::Client.master.new(
-                :Master => master
-            )
-        }
+        master = Puppet::Network::Handler.master.new( :Local => false)
+        client = Puppet::Network::Client.master.new( :Master => master)
 
         # Fake that it's local, so it creates the class file
         client.local = false
@@ -491,6 +482,7 @@ end
         client.expects(:setclasses).with do |array|
             array.length == 2 and array.include?("yaytest") and array.include?("bootest")
         end
+
         assert_nothing_raised {
             client.getconfig
         }
diff --git a/test/network/handler/master.rb b/test/network/handler/master.rb
index e91ec2f47..448667b73 100755
--- a/test/network/handler/master.rb
+++ b/test/network/handler/master.rb
@@ -8,69 +8,51 @@ require 'puppet/network/handler/master'
 class TestMaster < Test::Unit::TestCase
     include PuppetTest::ServerTest
 
+    def setup
+        super
+        @master = Puppet::Network::Handler.master.new(:Manifest => tempfile)
+
+        @catalog = stub 'catalog', :extract => ""
+        Puppet::Node::Catalog.stubs(:find).returns(@catalog)
+    end
+
     def teardown
         super
         Puppet::Indirector::Indirection.clear_cache
     end
 
     def test_freshness_is_always_now
-        master = Puppet::Network::Handler.master.new(
-            :Manifest => tempfile,
-            :UseNodes => true,
-            :Local => true
-        )
-
         now1 = mock 'now1'
         Time.expects(:now).returns(now1)
 
-        assert_equal(master.freshness, now1, "Did not return current time as freshness")
+        assert_equal(@master.freshness, now1, "Did not return current time as freshness")
     end
 
-    # Make sure we're correctly doing clientname manipulations.
-    # Testing to make sure we always get a hostname and IP address.
-    def test_clientname
-        # create our master
-        master = Puppet::Network::Handler.master.new(
-            :Manifest => tempfile,
-            :UseNodes => true,
-            :Local => true
-        )
-
-
-        # First check that 'cert' works
-        Puppet[:node_name] = "cert"
-
-        # Make sure we get the fact data back when nothing is set
-        facts = {
-            "hostname" => "fact_hostname",
-            "domain" => "fact_domain",
-            "fqdn" => "fact_hostname.fact_domain",
-            "ipaddress" => "fact_ip"
-        }
-        certhostname = "cert_hostname"
-        certdomain = "cert_domain"
-        certname = certhostname + "." + certdomain
-        certip = "cert_ip"
-
-        resname, resip = master.send(:clientname, nil, nil, facts)
-        assert_equal(facts["hostname"], resname, "Did not use fact hostname when no certname was present")
-        assert_equal(facts["ipaddress"], resip, "Did not use fact ip when no certname was present")
-        assert_equal(facts["domain"], "fact_domain", "Did not use fact domain when no certname was present")
-        assert_equal(facts["fqdn"], "fact_hostname.fact_domain", "Did not use fact fqdn when no certname was present")
-
-        # Now try it with the cert stuff present
-        resname, resip = master.send(:clientname, certname, certip, facts)
-        assert_equal(certname, resname, "Did not use cert hostname when certname was present")
-        assert_equal(certip, resip, "Did not use cert ip when certname was present")
-        assert_equal(facts["domain"], certdomain, "Did not use cert domain when certname was present")
-        assert_equal(facts["fqdn"], certname, "Did not use cert fqdn when certname was present")
-
-        # And reset the node_name stuff and make sure we use it.
-        Puppet[:node_name] = :facter
-        resname, resip = master.send(:clientname, certname, certip, facts)
-        assert_equal(facts["hostname"], resname, "Did not use fact hostname when nodename was set to facter")
-        assert_equal(facts["ipaddress"], resip, "Did not use fact ip when nodename was set to facter")
+    def test_hostname_is_used_if_client_is_missing
+        @master.expects(:decode_facts).returns("hostname" => "yay")
+        Puppet::Node::Facts.expects(:new).with { |name, facts| name == "yay" }.returns(stub('facts', :save => nil))
+
+        @master.getconfig("facts")
     end
-end
 
+    def test_facts_are_saved
+        facts = mock('facts')
+        Puppet::Node::Facts.expects(:new).returns(facts)
+        facts.expects(:save)
+
+        @master.stubs(:decode_facts)
 
+        @master.getconfig("facts", "yaml", "foo.com")
+    end
+
+    def test_catalog_is_used_for_compiling
+        facts = stub('facts', :save => nil)
+        Puppet::Node::Facts.stubs(:new).returns(facts)
+
+        @master.stubs(:decode_facts)
+
+        Puppet::Node::Catalog.expects(:find).with("foo.com").returns(@catalog)
+
+        @master.getconfig("facts", "yaml", "foo.com")
+    end
+end