Entirely refactoring http keep-alive. There's now

a central module responsible for managing the http pool
(Puppet::Network::HttpPool), and it also handles
setting certificate information.  This gets rid of
what were otherwise long chains of method calls,
and it makes the code paths much clearer.

13 files changed, 372 insertions, 281 deletions

diff --git a/CHANGELOG b/CHANGELOG
index bf563dbce..91654b6a4 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,9 @@
+    Refactored http keep-alive so it actually works again.
+    This should be sufficient enough that we no longer need the
+    ability to disable keep-alive.  There is now a central
+    module responsible for managing HTTP instances, along with
+    all certificates in those instances.
+
     Fixed a backward compatibility issue when running 0.23.x
     clients against 0.24.0 servers -- relationships would
     consistently not work. (#967)
diff --git a/bin/puppetd b/bin/puppetd
index 1235a36fe..297d4876d 100755
--- a/bin/puppetd
+++ b/bin/puppetd
@@ -328,7 +328,7 @@ if Puppet[:daemonize]
     client.daemonize
 end
 
-unless client.read_cert
+unless Puppet::Network::HttpPool.read_cert
     # If we don't already have the certificate, then create a client to
     # request one.  Use the special ca stuff, don't use the normal server and port.
     caclient = Puppet::Network::Client.ca.new()
@@ -350,7 +350,9 @@ unless client.read_cert
     end
 
     # Now read the new cert in.
-    if client.read_cert
+    if Puppet::Network::HttpPool.read_cert
+        # If we read it in, then get rid of our existing http connection.
+        client.recycle_connection
         Puppet.notice "Got signed certificate"
     else
         Puppet.err "Could not read certificates after retrieving them"
diff --git a/lib/puppet/network/client.rb b/lib/puppet/network/client.rb
index 52431e227..9a861fdf2 100644
--- a/lib/puppet/network/client.rb
+++ b/lib/puppet/network/client.rb
@@ -122,13 +122,8 @@ class Puppet::Network::Client
     end
 
     # Make sure we set the driver up when we read the cert in.
-    def read_cert
-        if super
-            @driver.recycle_connection(self) if @driver.respond_to?(:recycle_connection)
-            return true
-        else
-            return false
-        end
+    def recycle_connection
+        @driver.recycle_connection if @driver.respond_to?(:recycle_connection)
     end
 
     # A wrapper method to run and then store the last run time
@@ -141,9 +136,7 @@ class Puppet::Network::Client
             self.run
             self.lastrun = Time.now.to_i
         rescue => detail
-            if Puppet[:trace]
-                puts detail.backtrace
-            end
+            puts detail.backtrace if Puppet[:trace]
             Puppet.err "Could not run %s: %s" % [self.class, detail]
         end
     end
diff --git a/lib/puppet/network/client/master.rb b/lib/puppet/network/client/master.rb
index 341192740..8d768018f 100644
--- a/lib/puppet/network/client/master.rb
+++ b/lib/puppet/network/client/master.rb
@@ -1,6 +1,7 @@
 # The client for interacting with the puppetmaster config server.
 require 'sync'
 require 'timeout'
+require 'puppet/network/http_pool'
 
 class Puppet::Network::Client::Master < Puppet::Network::Client
     unless defined? @@sync
@@ -274,7 +275,7 @@ class Puppet::Network::Client::Master < Puppet::Network::Client
 
                 # Now close all of our existing http connections, since there's no
                 # reason to leave them lying open.
-                Puppet::Network::XMLRPCClient.clear_http_instances
+                Puppet::Network::HttpPool.clear_http_instances
             end
             
             lockfile.unlock
diff --git a/lib/puppet/network/http_pool.rb b/lib/puppet/network/http_pool.rb
new file mode 100644
index 000000000..f6038f189
--- /dev/null
+++ b/lib/puppet/network/http_pool.rb
@@ -0,0 +1,92 @@
+require 'puppet/sslcertificates/support'
+require 'net/https'
+
+# Manage Net::HTTP instances for keep-alive.
+module Puppet::Network::HttpPool
+    # This handles reading in the key and such-like.
+    extend Puppet::SSLCertificates::Support
+    @http_cache = {}
+
+    # Clear our http cache, closing all connections.
+    def self.clear_http_instances
+        @http_cache.each do |name, connection|
+            connection.finish if connection.started?
+        end
+        @http_cache.clear
+        @cert = nil
+        @key = nil
+    end
+
+    # Make sure we set the driver up when we read the cert in.
+    def self.read_cert
+        if val = super # This calls read_cert from the Puppet::SSLCertificates::Support module.
+            # Clear out all of our connections, since they previously had no cert and now they
+            # should have them.
+            clear_http_instances
+            return val
+        else
+            return false
+        end
+    end
+
+    # Use cert information from a Puppet client to set up the http object.
+    def self.cert_setup(http)
+        # Just no-op if we don't have certs.
+        return false unless (defined?(@cert) and @cert) or self.read_cert
+
+        store = OpenSSL::X509::Store.new
+        store.add_file Puppet[:localcacert]
+        store.purpose = OpenSSL::X509::PURPOSE_SSL_CLIENT
+
+        http.cert_store = store
+        http.ca_file = Puppet[:localcacert]
+        http.cert = self.cert
+        http.verify_mode = OpenSSL::SSL::VERIFY_PEER
+        http.key = self.key
+    end
+
+    # Retrieve a cached http instance of caching is enabled, else return
+    # a new one.
+    def self.http_instance(host, port, reset = false)
+        # We overwrite the uninitialized @http here with a cached one.
+        key = "%s:%s" % [host, port]
+
+        # Return our cached instance if keepalive is enabled and we've got
+        # a cache, as long as we're not resetting the instance.
+        return @http_cache[key] if ! reset and Puppet[:http_keepalive] and @http_cache[key]
+
+        # Clean up old connections if we have them.
+        if http = @http_cache[key]
+            @http_cache.delete(key)
+            http.finish if http.started?
+        end
+
+        args = [host, port]
+        if Puppet[:http_proxy_host] == "none"
+            args << nil << nil
+        else
+            args << Puppet[:http_proxy_host] << Puppet[:http_proxy_port]
+        end
+        http = Net::HTTP.new(*args)
+
+        # Pop open the http client a little; older versions of Net::HTTP(s) didn't
+        # give us a reader for ca_file... Grr...
+        class << http; attr_accessor :ca_file; end
+
+        http.use_ssl = true
+        http.read_timeout = 120
+        http.open_timeout = 120
+        # JJM Configurable fix for #896.
+        if Puppet[:http_enable_post_connection_check]
+            http.enable_post_connection_check = true
+        else
+            http.enable_post_connection_check = false
+        end
+
+        cert_setup(http)
+
+        @http_cache[key] = http if Puppet[:http_keepalive]
+
+        return http
+    end
+end
diff --git a/lib/puppet/network/xmlrpc/client.rb b/lib/puppet/network/xmlrpc/client.rb
index 5048a040a..27bb3dc5e 100644
--- a/lib/puppet/network/xmlrpc/client.rb
+++ b/lib/puppet/network/xmlrpc/client.rb
@@ -1,4 +1,5 @@
 require 'puppet/sslcertificates'
+require 'puppet/network/http_pool'
 require 'openssl'
 require 'puppet/external/base64'
 
@@ -10,66 +11,15 @@ module Puppet::Network
     class ClientError < Puppet::Error; end
     class XMLRPCClientError < Puppet::Error; end
     class XMLRPCClient < ::XMLRPC::Client
+
         attr_accessor :puppet_server, :puppet_port
         @clients = {}
-        @@http_cache = {}
 
         class << self
             include Puppet::Util
             include Puppet::Util::ClassGen
         end
 
-        # Clear our http cache, closing all connections.
-        def self.clear_http_instances
-            @@http_cache.each do |name, connection|
-                connection.finish if connection.started?
-            end
-            @@http_cache.clear
-        end
-
-        # Retrieve a cached http instance of caching is enabled, else return
-        # a new one.
-        def self.http_instance(host, port, reset = false)
-            # We overwrite the uninitialized @http here with a cached one.
-            key = "%s:%s" % [host, port]
-
-            # Return our cached instance if keepalive is enabled and we've got
-            # a cache, as long as we're not resetting the instance.
-            return @@http_cache[key] if ! reset and Puppet[:http_keepalive] and @@http_cache[key]
-
-            # Clean up old connections if we have them.
-            if http = @@http_cache[key]
-                @@http_cache.delete(key)
-                http.finish if http.started?
-            end
-
-            args = [host, port]
-            if Puppet[:http_proxy_host] == "none"
-                args << nil << nil
-            else
-                args << Puppet[:http_proxy_host] << Puppet[:http_proxy_port]
-            end
-            http = Net::HTTP.new(*args)
-
-            # Pop open the http client a little; older versions of Net::HTTP(s) didn't
-            # give us a reader for ca_file... Grr...
-            class << http; attr_accessor :ca_file; end
-
-            http.use_ssl = true
-            http.read_timeout = 120
-            http.open_timeout = 120
-            # JJM Configurable fix for #896.
-            if Puppet[:http_enable_post_connection_check]
-                http.enable_post_connection_check = true
-            else
-                http.enable_post_connection_check = false
-            end
-
-            @@http_cache[key] = http if Puppet[:http_keepalive]
-
-            return http
-        end
-
         # Create a netclient for each handler
         def self.mkclient(handler)
             interface = handler.interface
@@ -81,8 +31,7 @@ module Puppet::Network
             # they want.
             constant = handler.name.to_s.capitalize
             name = namespace.downcase
-            newclient = genclass(name, :hash => @clients,
-                :constant => constant)
+            newclient = genclass(name, :hash => @clients, :constant => constant)
 
             interface.methods.each { |ary|
                 method = ary[0]
@@ -97,7 +46,7 @@ module Puppet::Network
                     rescue OpenSSL::SSL::SSLError => detail
                         if detail.message =~ /bad write retry/
                             Puppet.warning "Transient SSL write error; restarting connection and retrying"
-                            self.recycle_connection(@cert_client)
+                            self.recycle_connection
                             retry
                         end
                         raise XMLRPCClientError,
@@ -118,7 +67,7 @@ module Puppet::Network
                         raise error
                     rescue Errno::EPIPE, EOFError
                         Puppet.warning "Other end went away; restarting connection and retrying"
-                        self.recycle_connection(@cert_client)
+                        self.recycle_connection
                         retry
                     rescue => detail
                         if detail.message =~ /^Wrong size\. Was \d+, should be \d+$/
@@ -141,30 +90,6 @@ module Puppet::Network
             @clients[handler] || self.mkclient(handler)
         end
 
-        # Use cert information from a Puppet client to set up the http object.
-        def cert_setup(client)
-            # Cache it for next time
-            @cert_client = client
-            
-            unless FileTest.exist?(Puppet[:localcacert])
-                raise Puppet::SSLCertificates::Support::MissingCertificate,
-                    "Could not find ca certificate %s" % Puppet[:localcacert]
-            end
-
-            # We can't overwrite certificates, @http will freeze itself
-            # once started.
-            unless @http.ca_file
-                @http.ca_file = Puppet[:localcacert]
-                store = OpenSSL::X509::Store.new
-                store.add_file Puppet[:localcacert]
-                store.purpose = OpenSSL::X509::PURPOSE_SSL_CLIENT
-                @http.cert_store = store
-                @http.cert = client.cert
-                @http.verify_mode = OpenSSL::SSL::VERIFY_PEER
-                @http.key = client.key
-            end
-        end
-
         def initialize(hash = {})
             hash[:Path] ||= "/RPC2"
             hash[:Server] ||= Puppet[:server]
@@ -188,13 +113,15 @@ module Puppet::Network
                 true, # use_ssl
                 120 # a two minute timeout, instead of 30 seconds
             )
-            @http = self.class.http_instance(@host, @port)
+            @http = Puppet::Network::HttpPool.http_instance(@host, @port)
         end
  
-        def recycle_connection(client)
-            @http = self.class.http_instance(@host, @port, true) # reset the instance
-
-            cert_setup(client)
+        # Get rid of our existing connection, replacing it with a new one.
+        # This should only happen if we lose our connection somehow (e.g., an EPIPE)
+        # or we've just downloaded certs and we need to create new http instances
+        # with the certs added.
+        def recycle_connection
+            @http = Puppet::Network::HttpPool.http_instance(@host, @port, true) # reset the instance
         end
         
         def start
diff --git a/lib/puppet/sslcertificates/support.rb b/lib/puppet/sslcertificates/support.rb
index e63458b33..1d692c994 100644
--- a/lib/puppet/sslcertificates/support.rb
+++ b/lib/puppet/sslcertificates/support.rb
@@ -30,25 +30,23 @@ module Puppet::SSLCertificates::Support
         define_method(reader) do
             return nil unless FileTest.exists?(Puppet[param])
             begin
-                instance_variable_set(var,
-                    klass.new(File.read(Puppet[param])))
+                instance_variable_set(var, klass.new(File.read(Puppet[param])))
             rescue => detail
-                raise InvalidCertificate, "Could not read %s: %s" %
-                    [param, detail]
+                raise InvalidCertificate, "Could not read %s: %s" % [param, detail]
             end
         end
 
         # Define the overall method, which just calls the reader and maker
         # as appropriate.
         define_method(name) do
-            unless instance_variable_get(var)
+            unless cert = instance_variable_get(var)
                 unless cert = send(reader)
                     cert = send(maker)
                     Puppet.settings.write(param) { |f| f.puts cert.to_pem }
                 end
                 instance_variable_set(var, cert)
             end
-            instance_variable_get(var)
+            cert
         end
     end
 
diff --git a/spec/unit/network/http_pool.rb b/spec/unit/network/http_pool.rb
new file mode 100755
index 000000000..c6647158f
--- /dev/null
+++ b/spec/unit/network/http_pool.rb
@@ -0,0 +1,237 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke Kanies on 2007-11-26.
+#  Copyright (c) 2007. All rights reserved.
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+require 'puppet/network/http_pool'
+
+describe Puppet::Network::HttpPool, " when adding certificate information to http instances" do
+    before do
+        @http = mock 'http'
+    end
+
+    it "should do nothing if no certificate is available" do
+        Puppet::Network::HttpPool.expects(:read_cert).returns(false)
+        @http.expects(:cert=).never
+        Puppet::Network::HttpPool.cert_setup(@http)
+    end
+
+    it "should add a certificate store" do
+        Puppet::Network::HttpPool.stubs(:read_cert).returns(true)
+        store = stub "store"
+        OpenSSL::X509::Store.expects(:new).returns(store)
+        store.stubs(:add_file)
+        store.stubs(:purpose=)
+        [:verify_mode=, :ca_file=, :cert=, :key=].each { |method| @http.stubs(method) }
+        @http.expects(:cert_store=).with(store)
+
+        Puppet::Network::HttpPool.cert_setup(@http)
+    end
+
+    it "should add the local CA cert to the certificate store" do
+        Puppet::Network::HttpPool.stubs(:read_cert).returns(true)
+        store = stub "store"
+        OpenSSL::X509::Store.expects(:new).returns(store)
+        store.stubs(:purpose=)
+        @http.stubs(:cert_store=)
+        Puppet.settings.stubs(:value).with(:localcacert).returns("/some/file")
+        Puppet.settings.stubs(:value).with(:localcacert).returns("/some/file")
+        store.expects(:add_file).with("/some/file")
+        [:store=, :verify_mode=, :ca_file=, :cert=, :key=].each { |method| @http.stubs(method) }
+
+        Puppet::Network::HttpPool.stubs(:key).returns(:whatever)
+
+        Puppet::Network::HttpPool.cert_setup(@http)
+    end
+
+    it "should set the purpose of the cert store to OpenSSL::X509::PURPOSE_SSL_CLIENT" do
+        Puppet::Network::HttpPool.stubs(:read_cert).returns(true)
+        store = stub "store"
+        OpenSSL::X509::Store.expects(:new).returns(store)
+        store.stubs(:add_file)
+        [:cert_store=, :verify_mode=, :ca_file=, :cert=, :key=].each { |method| @http.stubs(method) }
+
+        store.expects(:purpose=).with(OpenSSL::X509::PURPOSE_SSL_CLIENT)
+
+        Puppet::Network::HttpPool.cert_setup(@http)
+    end
+
+    it "should add the client certificate" do
+        Puppet::Network::HttpPool.stubs(:read_cert).returns(true)
+        Puppet::Network::HttpPool.stubs(:cert).returns(:mycert)
+        [:cert_store=, :verify_mode=, :ca_file=, :key=].each { |method| @http.stubs(method) }
+
+        @http.expects(:cert=).with(:mycert)
+
+        Puppet::Network::HttpPool.cert_setup(@http)
+    end
+
+    it "should add the client key" do
+        Puppet::Network::HttpPool.stubs(:read_cert).returns(true)
+        Puppet::Network::HttpPool.stubs(:key).returns(:mykey)
+        [:cert_store=, :verify_mode=, :cert=, :ca_file=].each { |method| @http.stubs(method) }
+
+        @http.expects(:key=).with(:mykey)
+
+        Puppet::Network::HttpPool.cert_setup(@http)
+    end
+
+    it "should set the verify mode to OpenSSL::SSL::VERIFY_PEER" do
+        Puppet::Network::HttpPool.stubs(:read_cert).returns(true)
+        [:key=, :cert=, :cert_store=, :ca_file=].each { |method| @http.stubs(method) }
+
+        @http.expects(:verify_mode=).with(OpenSSL::SSL::VERIFY_PEER)
+
+        Puppet::Network::HttpPool.cert_setup(@http)
+    end
+
+    it "should set the ca file" do
+        Puppet::Network::HttpPool.stubs(:read_cert).returns(true)
+        Puppet.settings.stubs(:value).with(:localcacert).returns("/some/file")
+        [:key=, :cert=, :cert_store=, :verify_mode=].each { |method| @http.stubs(method) }
+
+        store = stub "store"
+        OpenSSL::X509::Store.expects(:new).returns(store)
+        store.stubs(:purpose=)
+        store.stubs(:add_file)
+
+        @http.expects(:ca_file=).with("/some/file")
+
+        Puppet::Network::HttpPool.stubs(:key).returns(:whatever)
+
+        Puppet::Network::HttpPool.cert_setup(@http)
+    end
+
+    it "should set up certificate information when creating http instances" do
+        Puppet::Network::HttpPool.expects(:cert_setup).with { |i| i.is_a?(Net::HTTP) }
+        Puppet::Network::HttpPool.http_instance("one", "two")
+    end
+
+    after do
+        Puppet::Network::HttpPool.clear_http_instances
+    end
+end
+
+describe Puppet::Network::HttpPool, " when managing http instances" do
+    def stub_settings(settings)
+        settings.each do |param, value|
+            Puppet.settings.stubs(:value).with(param).returns(value)
+        end
+    end
+
+    before do
+        # All of hte cert stuff is tested elsewhere
+        Puppet::Network::HttpPool.stubs(:cert_setup)
+    end
+
+    it "should return an http instance created with the passed host and port" do
+        http = stub 'http', :use_ssl= => nil, :read_timeout= => nil, :open_timeout= => nil, :enable_post_connection_check= => nil, :started? => false
+        Net::HTTP.expects(:new).with("me", 54321, nil, nil).returns(http)
+        Puppet::Network::HttpPool.http_instance("me", 54321).should equal(http)
+    end
+
+    it "should enable ssl on the http instance" do
+        Puppet::Network::HttpPool.http_instance("me", 54321).instance_variable_get("@use_ssl").should be_true
+    end
+
+    it "should set the read timeout" do
+        Puppet::Network::HttpPool.http_instance("me", 54321).read_timeout.should == 120
+    end
+
+    it "should set the open timeout" do
+        Puppet::Network::HttpPool.http_instance("me", 54321).open_timeout.should == 120
+    end
+
+    it "should default to http_enable_post_connection_check being enabled" do
+        Puppet.settings[:http_enable_post_connection_check].should be_true
+    end
+
+    # JJM: I'm not sure if this is correct, as this really follows the
+    # configuration option.
+    it "should set enable_post_connection_check true " do
+        Puppet::Network::HttpPool.http_instance("me", 54321).instance_variable_get("@enable_post_connection_check").should be_true
+    end
+
+    it "should create the http instance with the proxy host and port set if the http_proxy is not set to 'none'" do
+        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+        Puppet::Network::HttpPool.http_instance("me", 54321).open_timeout.should == 120
+    end
+
+    it "should default to keep-alive being enabled" do
+        Puppet.settings[:http_keepalive].should be_true
+    end
+
+    it "should cache http instances if keepalive is enabled" do
+        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+        old = Puppet::Network::HttpPool.http_instance("me", 54321)
+        Puppet::Network::HttpPool.http_instance("me", 54321).should equal(old)
+    end
+
+    it "should not cache http instances if keepalive is not enabled" do
+        stub_settings :http_keepalive => false, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+        old = Puppet::Network::HttpPool.http_instance("me", 54321)
+        Puppet::Network::HttpPool.http_instance("me", 54321).should_not equal(old)
+    end
+
+    it "should have a mechanism for getting a new http instance instead of the cached instance" do
+        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+        old = Puppet::Network::HttpPool.http_instance("me", 54321)
+        Puppet::Network::HttpPool.http_instance("me", 54321, true).should_not equal(old)
+    end
+
+    it "should close existing, open connections when requesting a new connection" do
+        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+        old = Puppet::Network::HttpPool.http_instance("me", 54321)
+        old.expects(:started?).returns(true)
+        old.expects(:finish)
+        Puppet::Network::HttpPool.http_instance("me", 54321, true)
+    end
+
+    it "should have a mechanism for clearing the http cache" do
+        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+        old = Puppet::Network::HttpPool.http_instance("me", 54321)
+        Puppet::Network::HttpPool.http_instance("me", 54321).should equal(old)
+        old = Puppet::Network::HttpPool.http_instance("me", 54321)
+        Puppet::Network::HttpPool.clear_http_instances
+        Puppet::Network::HttpPool.http_instance("me", 54321).should_not equal(old)
+    end
+
+    it "should close open http connections when clearing the cache" do
+        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+        one = Puppet::Network::HttpPool.http_instance("me", 54321)
+        one.expects(:started?).returns(true)
+        one.expects(:finish).returns(true)
+        Puppet::Network::HttpPool.clear_http_instances
+    end
+
+    it "should not close unopened http connections when clearing the cache" do
+        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
+        one = Puppet::Network::HttpPool.http_instance("me", 54321)
+        one.expects(:started?).returns(false)
+        one.expects(:finish).never
+        Puppet::Network::HttpPool.clear_http_instances
+    end
+
+    # We mostly have to do this for testing, since in real life people
+    # won't change certs within a single process.
+    it "should remove its loaded certificate when clearing the cache" do
+        Puppet::Network::HttpPool.instance_variable_set("@cert", :yay)
+        Puppet::Network::HttpPool.clear_http_instances
+        # Can't use the accessor, because it will read the cert in
+        Puppet::Network::HttpPool.instance_variable_get("@cert").should be_nil
+    end
+
+    # We mostly have to do this for testing, since in real life people
+    # won't change certs within a single process.
+    it "should remove its loaded key when clearing the cache" do
+        Puppet::Network::HttpPool.instance_variable_set("@key", :yay)
+        Puppet::Network::HttpPool.clear_http_instances
+        # Can't use the accessor, because it will read the cert in
+        Puppet::Network::HttpPool.instance_variable_get("@key").should be_nil
+    end
+
+    after do
+        Puppet::Network::HttpPool.clear_http_instances
+    end
+end
diff --git a/spec/unit/network/xmlrpc/client.rb b/spec/unit/network/xmlrpc/client.rb
deleted file mode 100755
index b40486e13..000000000
--- a/spec/unit/network/xmlrpc/client.rb
+++ /dev/null
@@ -1,107 +0,0 @@
-#!/usr/bin/env ruby
-#
-#  Created by Luke Kanies on 2007-11-26.
-#  Copyright (c) 2007. All rights reserved.
-
-require File.dirname(__FILE__) + '/../../../spec_helper'
-require 'puppet/network/xmlrpc/client'
-
-describe Puppet::Network::XMLRPCClient, " when managing http instances" do
-    def stub_settings(settings)
-        settings.each do |param, value|
-            Puppet.settings.stubs(:value).with(param).returns(value)
-        end
-    end
-
-    it "should return an http instance created with the passed host and port" do
-        http = stub 'http', :use_ssl= => nil, :read_timeout= => nil, :open_timeout= => nil, :enable_post_connection_check= => nil, :started? => false
-        Net::HTTP.expects(:new).with("me", 54321, nil, nil).returns(http)
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).should equal(http)
-    end
-
-    it "should enable ssl on the http instance" do
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).instance_variable_get("@use_ssl").should be_true
-    end
-
-    it "should set the read timeout" do
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).read_timeout.should == 120
-    end
-
-    it "should set the open timeout" do
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).open_timeout.should == 120
-    end
-
-    it "should default to http_enable_post_connection_check being enabled" do
-        Puppet.settings[:http_enable_post_connection_check].should be_true
-    end
-
-    # JJM: I'm not sure if this is correct, as this really follows the
-    # configuration option.
-    it "should set enable_post_connection_check true " do
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).instance_variable_get("@enable_post_connection_check").should be_true
-    end
-
-    it "should create the http instance with the proxy host and port set if the http_proxy is not set to 'none'" do
-        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).open_timeout.should == 120
-    end
-
-    it "should default to keep-alive being enabled" do
-        Puppet.settings[:http_keepalive].should be_true
-    end
-
-    it "should cache http instances if keepalive is enabled" do
-        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        old = Puppet::Network::XMLRPCClient.http_instance("me", 54321)
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).should equal(old)
-    end
-
-    it "should not cache http instances if keepalive is not enabled" do
-        stub_settings :http_keepalive => false, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        old = Puppet::Network::XMLRPCClient.http_instance("me", 54321)
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).should_not equal(old)
-    end
-
-    it "should have a mechanism for getting a new http instance instead of the cached instance" do
-        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        old = Puppet::Network::XMLRPCClient.http_instance("me", 54321)
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321, true).should_not equal(old)
-    end
-
-    it "should close existing, open connections when requesting a new connection" do
-        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        old = Puppet::Network::XMLRPCClient.http_instance("me", 54321)
-        old.expects(:started?).returns(true)
-        old.expects(:finish)
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321, true)
-    end
-
-    it "should have a mechanism for clearing the http cache" do
-        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        old = Puppet::Network::XMLRPCClient.http_instance("me", 54321)
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).should equal(old)
-        old = Puppet::Network::XMLRPCClient.http_instance("me", 54321)
-        Puppet::Network::XMLRPCClient.clear_http_instances
-        Puppet::Network::XMLRPCClient.http_instance("me", 54321).should_not equal(old)
-    end
-
-    it "should close open http connections when clearing the cache" do
-        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        one = Puppet::Network::XMLRPCClient.http_instance("me", 54321)
-        one.expects(:started?).returns(true)
-        one.expects(:finish).returns(true)
-        Puppet::Network::XMLRPCClient.clear_http_instances
-    end
-
-    it "should not close unopened http connections when clearing the cache" do
-        stub_settings :http_keepalive => true, :http_proxy_host => "myhost", :http_proxy_port => 432, :http_enable_post_connection_check => true
-        one = Puppet::Network::XMLRPCClient.http_instance("me", 54321)
-        one.expects(:started?).returns(false)
-        one.expects(:finish).never
-        Puppet::Network::XMLRPCClient.clear_http_instances
-    end
-
-    after do
-        Puppet::Network::XMLRPCClient.clear_http_instances
-    end
-end
diff --git a/test/network/client/client.rb b/test/network/client/client.rb
index 2588b9be5..b6b915d31 100755
--- a/test/network/client/client.rb
+++ b/test/network/client/client.rb
@@ -221,34 +221,4 @@ class TestClient < Test::Unit::TestCase
             end
         end
     end
-
-    # Make sure that reading the cert in also sets up the cert stuff for the driver
-    def test_read_cert
-        Puppet::Util::SUIDManager.stubs(:asuser).yields
-
-        ca = Puppet::Network::Handler.ca.new
-        caclient = Puppet::Network::Client.ca.new :CA => ca
-
-        caclient.request_cert
-
-        # First make sure it doesn't get called when the driver doesn't support :cert_setup
-        client = FakeClient.new :Test => FakeDriver.new
-        driver = client.driver
-
-        assert_nothing_raised("Could not read cert") do
-            client.read_cert
-        end
-
-        # And then that it does when the driver supports it
-        client = FakeClient.new :Test => FakeDriver.new
-
-        driver = client.driver
-        driver.meta_def(:recycle_connection) { |c| }
-        driver.expects(:recycle_connection).with(client)
-
-        assert_nothing_raised("Could not read cert") do
-            client.read_cert
-        end
-    end
 end
-
diff --git a/test/network/server/webrick.rb b/test/network/server/webrick.rb
index 3561cd41a..d3408c166 100755
--- a/test/network/server/webrick.rb
+++ b/test/network/server/webrick.rb
@@ -14,6 +14,11 @@ class TestWebrickServer < Test::Unit::TestCase
         super
     end
 
+    def teardown
+        super
+        Puppet::Network::HttpPool.clear_http_instances
+    end
+
     # Make sure we can create a server, and that it knows how to create its
     # certs by default.
     def test_basics
@@ -102,7 +107,7 @@ class TestWebrickServer < Test::Unit::TestCase
 
         assert_nothing_raised() {
             client = Puppet::Network::Client.status.new(
-                :Server => Facter.value(:fqdn),
+                :Server => "localhost",
                 :Port => @@port
             )
         }
@@ -111,6 +116,7 @@ class TestWebrickServer < Test::Unit::TestCase
 
     def mk_status_server
         server = nil
+        Puppet[:certdnsnames] = "localhost"
         assert_nothing_raised() {
             server = Puppet::Network::HTTPServer::WEBrick.new(
                 :Port => @@port,
diff --git a/test/network/xmlrpc/client.rb b/test/network/xmlrpc/client.rb
index f6d234324..53be5ca07 100755
--- a/test/network/xmlrpc/client.rb
+++ b/test/network/xmlrpc/client.rb
@@ -42,43 +42,4 @@ class TestXMLRPCClient < Test::Unit::TestCase
 
         assert(net, "did not get net client")
     end
-
-    # Make sure the xmlrpc client is correctly reading all of the cert stuff
-    # and setting it into the @http var
-    def test_cert_setup
-        client = nil
-        assert_nothing_raised do
-            client = Puppet::Network::XMLRPCClient.new()
-        end
-
-        caclient = mock 'client', :cert => :ccert, :key => :ckey
-
-        FileTest.expects(:exist?).with(Puppet[:localcacert]).returns(true)
-
-        store = mock 'sslstore'
-        OpenSSL::X509::Store.expects(:new).returns(store)
-        store.expects(:add_file).with(Puppet[:localcacert])
-        store.expects(:purpose=).with(OpenSSL::X509::PURPOSE_SSL_CLIENT)
-
-        class << client
-            attr_accessor :http
-        end
-
-        http = mock 'http'
-        client.http = http
-
-        http.expects(:ca_file).returns(false)
-        http.expects(:ca_file=).with(Puppet[:localcacert])
-        http.expects(:cert=).with(:ccert)
-        http.expects(:key=).with(:ckey)
-        http.expects(:verify_mode=).with(OpenSSL::SSL::VERIFY_PEER)
-        http.expects(:cert_store=)
-
-        assert_nothing_raised do
-            client.cert_setup(caclient)
-        end
-    end
-
-    def test_http_cache
-    end
 end
diff --git a/test/ral/types/filesources.rb b/test/ral/types/filesources.rb
index 0f283be79..02bf8a5b3 100755
--- a/test/ral/types/filesources.rb
+++ b/test/ral/types/filesources.rb
@@ -22,6 +22,11 @@ class TestFileSources < Test::Unit::TestCase
         Puppet[:filetimeout] = -1
         Puppet::Util::SUIDManager.stubs(:asuser).yields 
     end
+
+    def teardown
+        super
+        Puppet::Network::HttpPool.clear_http_instances
+    end
     
     def use_storage
         begin