Integrating most of Matt Palmer's from

http://theshed.hezmatt.org/mattshacks/puppet/_patches/puppet-0.23.2/.

There are still a few that haven't made it in, notably those related
to the plugins module, which I'm planning on integrating separately.

6 files changed, 144 insertions, 87 deletions

diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index f8fd23ea8..8edbe31fe 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -384,6 +384,8 @@ module Puppet
             may need to use a FQDN for the server hostname when using a proxy."],
         :http_proxy_port => [3128,
             "The HTTP proxy port to use for outgoing connections"],
+        :http_keepalive => [true,
+            "Whether to reuse http connections, thus enabling http-keepalive."],
         :server => ["puppet",
             "The server to which server puppetd should connect"],
         :ignoreschedules => [false,
diff --git a/lib/puppet/network/xmlrpc/client.rb b/lib/puppet/network/xmlrpc/client.rb
index a4df4fec8..39f149aa8 100644
--- a/lib/puppet/network/xmlrpc/client.rb
+++ b/lib/puppet/network/xmlrpc/client.rb
@@ -3,6 +3,7 @@ require 'openssl'
 require 'puppet/external/base64'
 
 require 'xmlrpc/client'
+require 'net/https'
 require 'yaml'
 
 module Puppet::Network
@@ -18,6 +19,42 @@ module Puppet::Network
             include Puppet::Util::ClassGen
         end
 
+        # Clear our http cache.
+        def self.clear_http_instances
+            @@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]
+
+            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 @http 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
+
+            @@http_cache[key] = @http if Puppet[:http_keepalive]
+
+            return @http
+        end
+
         # Create a netclient for each handler
         def self.mkclient(handler)
             interface = handler.interface
@@ -25,7 +62,7 @@ module Puppet::Network
 
             # Create a subclass for every client type.  This is
             # so that all of the methods are on their own class,
-            # so that they namespaces can define the same methods if
+            # so that their namespaces can define the same methods if
             # they want.
             constant = handler.name.to_s.capitalize
             name = namespace.downcase
@@ -94,26 +131,22 @@ module Puppet::Network
             # Cache it for next time
             @cert_client = client
             
-            unless FileTest.exists?(Puppet[:localcacert])
+            unless FileTest.exist?(Puppet[:localcacert])
                 raise Puppet::SSLCertificates::Support::MissingCertificate,
                     "Could not find ca certificate %s" % Puppet[:localcacert]
             end
 
-            # Pop open @http 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
-
-            # Don't want to overwrite certificates, @http will freeze itself
+            # 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
+                @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
 
@@ -129,9 +162,6 @@ module Puppet::Network
                 hash[:HTTPProxyPort] = nil
             end
 
-            @puppet_server = hash[:Server]
-            @puppet_port = hash[:Port]
-
             super(
                 hash[:Server],
                 hash[:Path],
@@ -143,34 +173,12 @@ module Puppet::Network
                 true, # use_ssl
                 120 # a two minute timeout, instead of 30 seconds
             )
-            initialize_connection
+            @http = self.class.http_instance(@host, @port)
         end
  
-        def initialize_connection
-            # Yes, this may well be redoing what the XMLRPC::Client constructor
-            # did, but sometimes it won't be, because of the occasional retry.
-            @http = Net::HTTP.new(@host, @port, @proxy_host, @proxy_port)
-            @http.use_ssl = @use_ssl if @use_ssl
-            @http.read_timeout = @timeout
-            @http.open_timeout = @timeout
-
-            # We overwrite the uninitialized @http here with a cached one.
-            key = "%s:%s" % [@host, @port]
-
-            # We overwrite the uninitialized @http here with a cached one.
-            key = "%s%s" % [hash[:Server], hash[:Port]]
-            if @@http_cache[key]
-                @http = @@http_cache[key]
-            else
-                @@http_cache[key] = @http
-            end
-        end
-
         def recycle_connection(client)
-            conn_key = "%s:%s" % [@host, @port]
-            @@http_cache.delete(conn_key)
-            
-            initialize_connection
+            @http = self.class.http_instance(@host, @port, true) # reset the instance
+
             cert_setup(client)
         end
         
diff --git a/spec/unit/network/xmlrpc/client.rb b/spec/unit/network/xmlrpc/client.rb
new file mode 100755
index 000000000..e20c66c25
--- /dev/null
+++ b/spec/unit/network/xmlrpc/client.rb
@@ -0,0 +1,69 @@
+#!/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
+    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
+        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).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 create the http instance with the proxy host and port set if the http_proxy is not set to 'none'" do
+        Puppet.settings.stubs(:value).with(:http_keepalive).returns(true)
+        Puppet.settings.stubs(:value).with(:http_proxy_host).returns("myhost")
+        Puppet.settings.stubs(:value).with(:http_proxy_port).returns(432)
+        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
+        Puppet.settings.stubs(:value).with(:http_keepalive).returns(true)
+        Puppet.settings.stubs(:value).with(:http_proxy_host).returns("myhost")
+        Puppet.settings.stubs(:value).with(:http_proxy_port).returns(432)
+        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
+        Puppet.settings.stubs(:value).with(:http_keepalive).returns(false)
+        Puppet.settings.stubs(:value).with(:http_proxy_host).returns("myhost")
+        Puppet.settings.stubs(:value).with(:http_proxy_port).returns(432)
+        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 clearing the http cache" do
+        Puppet.settings.stubs(:value).with(:http_keepalive).returns(true)
+        Puppet.settings.stubs(:value).with(:http_proxy_host).returns("myhost")
+        Puppet.settings.stubs(:value).with(:http_proxy_port).returns(432)
+        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
+
+    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 4a7e9cdb6..918b9e86a 100755
--- a/test/network/client/client.rb
+++ b/test/network/client/client.rb
@@ -243,8 +243,8 @@ class TestClient < Test::Unit::TestCase
         client = FakeClient.new :Test => FakeDriver.new
 
         driver = client.driver
-        driver.meta_def(:cert_setup) { |c| }
-        driver.expects(:cert_setup).with(client)
+        driver.meta_def(:recycle_connection) { |c| }
+        driver.expects(:recycle_connection).with(client)
 
         assert_nothing_raised("Could not read cert") do
             client.read_cert
diff --git a/test/network/handler/bucket.rb b/test/network/handler/bucket.rb
index d72206e29..1a7063366 100755
--- a/test/network/handler/bucket.rb
+++ b/test/network/handler/bucket.rb
@@ -203,38 +203,6 @@ class TestBucket < Test::Unit::TestCase
         checkfiles(client)
     end
 
-    # test that things work over the wire
-    def test_webxmlmix
-        Puppet::Util::SUIDManager.stubs(:asuser).yields
-
-        files = filelist()
-
-        tmpdir = File.join(tmpdir(),"tmpfiledir")
-        @@tmpfiles << tmpdir
-        FileUtils.mkdir_p(tmpdir)
-
-        Puppet[:autosign] = true
-        Puppet[:certname] = "localhost"
-        client = nil
-        port = Puppet[:masterport]
-
-        pid = mkserver(:CA => {}, :FileBucket => { :Path => @bucket})
-
-        assert_nothing_raised {
-            client = Puppet::Network::Client.dipper.new(
-                :Server => "localhost",
-                :Port => @@port
-            )
-        }
-
-        checkfiles(client)
-
-        unless pid
-            raise "Uh, we don't have a child pid"
-        end
-        Process.kill("TERM", pid)
-    end
-
     def test_no_path_duplicates
         bucket = nil
         assert_nothing_raised {
diff --git a/test/network/xmlrpc/client.rb b/test/network/xmlrpc/client.rb
index e740e57b4..f6d234324 100755
--- a/test/network/xmlrpc/client.rb
+++ b/test/network/xmlrpc/client.rb
@@ -51,24 +51,34 @@ class TestXMLRPCClient < Test::Unit::TestCase
             client = Puppet::Network::XMLRPCClient.new()
         end
 
-        ca = Puppet::Network::Handler.ca.new
-        caclient = Puppet::Network::Client.ca.new :CA => ca
-        caclient.request_cert
+        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
 
-        client.http.expects(:ca_file=).with(Puppet[:localcacert])
-        client.http.expects(:cert=).with(caclient.cert)
-        client.http.expects(:key=).with(caclient.key)
-        client.http.expects(:verify_mode=).with(OpenSSL::SSL::VERIFY_PEER)
-        client.http.expects(:cert_store=)
+        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
-end
-
 
+    def test_http_cache
+    end
+end