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.

2 files changed, 237 insertions, 107 deletions

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