Added a test for Type#remove, and fixed the method so it actually works. I was missing every other object, because i was iterating over the array being modified. This caused the Config stuff to often fail, because objects were not correctly being removed. All fixed now, though.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@1053 980ebf18-57e1-0310-9a29-db15c13687c0

6 files changed, 114 insertions, 62 deletions

diff --git a/lib/puppet/config.rb b/lib/puppet/config.rb
index 638f1d34f..303c45ed9 100644
--- a/lib/puppet/config.rb
+++ b/lib/puppet/config.rb
@@ -95,6 +95,11 @@ class Config
         @used = []
     end
 
+    # This is mostly just used for testing.
+    def clearused
+        @used = []
+    end
+
     def symbolize(param)
         case param
         when String: return param.intern
diff --git a/lib/puppet/server/ca.rb b/lib/puppet/server/ca.rb
index 8b0a1a46f..064fd496b 100644
--- a/lib/puppet/server/ca.rb
+++ b/lib/puppet/server/ca.rb
@@ -79,35 +79,12 @@ class Server
                 return ""
             end
 
-            # okay, we're now going to store the public key if we don't already
-            # have it
-            public_key = csr.public_key
-            #unless FileTest.directory?(Puppet[:publickeydir])
-            #    Puppet.recmkdir(Puppet[:publickeydir])
-            #end
-            pkeyfile = File.join(Puppet[:publickeydir], [hostname, "pem"].join('.'))
+            # We used to save the public key, but it's basically unnecessary
+            # and it mucks with the permissions requirements.
+            # save_pk(hostname, csr.public_key)
 
-            if FileTest.exists?(pkeyfile)
-                currentkey = File.open(pkeyfile) { |k| k.read }
-                unless currentkey == public_key.to_s
-                    raise Puppet::Error, "public keys for %s differ" % hostname
-                end
-            else
-                File.open(pkeyfile, "w", 0644) { |f|
-                    f.print public_key.to_s
-                }
-            end
-            #unless FileTest.directory?(Puppet[:certdir])
-            #    Puppet.recmkdir(Puppet[:certdir], 0770)
-            #end
             certfile = File.join(Puppet[:certdir], [hostname, "pem"].join("."))
 
-            #puts hostname
-            #puts certfile
-
-            #unless FileTest.directory?(Puppet[:csrdir])
-            #    Puppet.recmkdir(Puppet[:csrdir], 0770)
-            #end
             # first check to see if we already have a signed cert for the host
             cert, cacert = ca.getclientcert(hostname)
             if cert and cacert
@@ -139,6 +116,26 @@ class Server
                 raise "huh?"
             end
         end
+
+        private
+
+        # Save the public key.
+        def save_pk(hostname, public_key)
+            pkeyfile = File.join(Puppet[:publickeydir], [hostname, "pem"].join('.'))
+
+            if FileTest.exists?(pkeyfile)
+                currentkey = File.open(pkeyfile) { |k| k.read }
+                unless currentkey == public_key.to_s
+                    raise Puppet::Error, "public keys for %s differ" % hostname
+                end
+            else
+                File.open(pkeyfile, "w", 0644) { |f|
+                    f.print public_key.to_s
+                }
+            end
+        end
     end
 end
 end
+
+# $Id$
diff --git a/lib/puppet/sslcertificates/ca.rb b/lib/puppet/sslcertificates/ca.rb
index 8d74cdf3e..fa6e9e368 100644
--- a/lib/puppet/sslcertificates/ca.rb
+++ b/lib/puppet/sslcertificates/ca.rb
@@ -4,34 +4,56 @@ class Puppet::SSLCertificates::CA
 
     Puppet.setdefaults(:ca,
         :cadir => {  :default => "$ssldir/ca",
+            :owner => "$user",
+            :group => "$group",
             :mode => 0770,
             :desc => "The root directory for the certificate authority."
         },
         :cacert => { :default => "$cadir/ca_crt.pem",
+            :owner => "$user",
+            :group => "$group",
             :mode => 0660,
             :desc => "The CA certificate."
         },
         :cakey => { :default => "$cadir/ca_key.pem",
+            :owner => "$user",
+            :group => "$group",
             :mode => 0660,
             :desc => "The CA private key."
         },
-        :capub => ["$cadir/ca_pub.pem", "The CA public key."],
+        :capub => { :default => "$cadir/ca_pub.pem",
+            :owner => "$user",
+            :group => "$group",
+            :desc => "The CA public key."
+        },
         :caprivatedir => { :default => "$cadir/private",
+            :owner => "$user",
+            :group => "$group",
             :mode => 0770,
             :desc => "Where the CA stores private certificate information."
         },
-        :csrdir => ["$cadir/requests",
-            "Where the CA stores certificate requests"],
+        :csrdir => { :default => "$cadir/requests",
+            :owner => "$user",
+            :group => "$group",
+            :desc => "Where the CA stores certificate requests"
+        },
         :signeddir => { :default => "$cadir/signed",
+            :owner => "$user",
+            :group => "$group",
             :mode => 0770,
             :desc => "Where the CA stores signed certificates."
         },
         :capass => { :default => "$caprivatedir/ca.pass",
+            :owner => "$user",
+            :group => "$group",
             :mode => 0660,
             :desc => "Where the CA stores the password for the private key"
         },
-        :serial => ["$cadir/serial",
-            "Where the serial number for certificates is stored."],
+        :serial => { :default => "$cadir/serial",
+            :owner => "$user",
+            :group => "$group",
+            :desc => "Where the serial number for certificates is stored."
+        },
         :autosign => { :default => "$confdir/autosign.conf",
             :mode => 0640,
             :desc => "Whether to enable autosign.  Valid values are true (which
@@ -88,9 +110,9 @@ class Puppet::SSLCertificates::CA
 
         self.getcert
         unless FileTest.exists?(@config[:serial])
-            File.open(@config[:serial], "w") { |f|
+            Puppet.config.write(:serial) do |f|
                 f << "%04X" % 1
-            }
+            end
         end
     end
 
@@ -99,12 +121,12 @@ class Puppet::SSLCertificates::CA
         20.times { pass += (rand(74) + 48).chr }
 
         # FIXME It's a hack that this still needs to be here :/
-        unless FileTest.exists?(File.dirname(@config[:capass]))
-            Puppet::Util.recmkdir(File.dirname(@config[:capass]), 0770)
-        end
+        #unless FileTest.exists?(File.dirname(@config[:capass]))
+        #    Puppet::Util.recmkdir(File.dirname(@config[:capass]), 0770)
+        #end
 
         begin
-            File.open(@config[:capass], "w", 0600) { |f| f.print pass }
+            Puppet.config.write(:capass) { |f| f.print pass }
         rescue Errno::EACCES => detail
             raise Puppet::Error, detail.to_s
         end
@@ -165,10 +187,14 @@ class Puppet::SSLCertificates::CA
             :length => 1825,
             :type => :ca
         )
-        @cert = cert.mkselfsigned
-        File.open(@config[:cacert], "w", 0660) { |f|
+
+        # This creates the cakey file
+        Puppet::Util.asuser(Puppet[:user], Puppet[:group]) do
+            @cert = cert.mkselfsigned
+        end
+        Puppet.config.write(:cacert) do |f|
             f.puts @cert.to_pem
-        }
+        end
         @key = cert.key
         return cert
     end
@@ -238,7 +264,6 @@ class Puppet::SSLCertificates::CA
                 File.read(@config[:cakey]), @config[:password]
             )
         else
-            system("ls -al %s" % Puppet[:capass])
             cakey = OpenSSL::PKey::RSA.new(
                 File.read(@config[:cakey])
             )
@@ -259,9 +284,9 @@ class Puppet::SSLCertificates::CA
         )
 
         # increment the serial
-        File.open(@config[:serial], "w") { |f|
+        Puppet.config.write(:serial) do |f|
             f << "%04X" % (serial + 1)
-        }
+        end
 
         newcert.sign(cakey, OpenSSL::Digest::SHA1.new)
 
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index be96f2194..b25cf87b3 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -62,7 +62,6 @@ class Type < Puppet::Element
         include Enumerable
 
         def inspect
-            Puppet.info "inspecting class with name %s" % self.name
             "Type(%s)" % self.name
         end
 
@@ -1010,7 +1009,11 @@ class Type < Puppet::Element
     # Remove an object.  The argument determines whether the object's
     # subscriptions get eliminated, too.
     def remove(rmdeps = true)
-        @children.each { |child|
+        # Our children remove themselves from our @children array (else the object
+        # we called this on at the top would not be removed), so we duplicate the
+        # array and iterate over that.  If we don't do this, only half of the
+        # objects get removed.
+        @children.dup.each { |child|
             child.remove(rmdeps)
         }
 
diff --git a/test/client/client.rb b/test/client/client.rb
index 117d30426..fe116bf56 100644
--- a/test/client/client.rb
+++ b/test/client/client.rb
@@ -109,24 +109,12 @@ class TestClient < Test::Unit::TestCase
             )
         }
 
-        # clean up the existing certs, so the server creates a new CA
-        #system("rm -rf %s" % Puppet[:ssldir])
+        # Create a new ssl root.
         confdir = tempfile()
-        Puppet[:confdir] = confdir
-
-        # Now we need to recreate the directory structure
-        [:certificates, :ca].each { |section|
-            Puppet.config.params(section).each { |param|
-                val = Puppet[param]
-                if val =~ /^#{File::SEPARATOR}/
-                    if param.to_s =~ /dir/
-                        Puppet::Util.recmkdir(val)
-                    else
-                        Puppet::Util.recmkdir(File.dirname(val))
-                    end
-                end
-            }
-        }
+        Puppet[:ssldir] = confdir
+        Puppet.config.mkdir(:ssldir)
+        Puppet.config.clearused
+        Puppet.config.use(:certificates, :ca)
 
         mkserver
 
diff --git a/test/types/type.rb b/test/types/type.rb
index b0da6da29..f98ca2355 100644
--- a/test/types/type.rb
+++ b/test/types/type.rb
@@ -251,6 +251,40 @@ class TestType < Test::Unit::TestCase
         # and make sure managed objects start with them
         assert(user.state(:ensure), "User did not get an ensure state")
     end
+
+    # Make sure removal works
+    def test_remove
+        objects = {}
+        top = Puppet.type(:component).create(:name => "top")
+        objects[top.class] = top
+
+        base = tempfile()
+
+        # now make a two-tier, 5 piece tree
+        %w{a b}.each do |letter|
+            name = "comp%s" % letter
+            comp = Puppet.type(:component).create(:name => name)
+            top.push comp
+            objects[comp.class] = comp
+
+            5.times do |i|
+                file = base + letter + i.to_s
+
+                obj = Puppet.type(:file).create(:name => file, :ensure => "file")
+
+                comp.push obj
+                objects[obj.class] = obj
+            end
+        end
+
+        assert_nothing_raised do
+            top.remove
+        end
+
+        objects.each do |klass, obj|
+            assert_nil(klass[obj.name], "object %s was not removed" % obj.name)
+        end
+    end
 end
 
 # $Id$