Moving the webrick/mongrel "servers" over to HTTPServer module instead of Server. Using Server as the master class for client connections. Server (former RESTServer) will instantiate the appropriate subclass based upon Puppet configurator setting. There are now tests broken in the network section which I can't seem to figure out yet. Not a happy place to be.

14 files changed, 147 insertions, 75 deletions

diff --git a/bin/puppetd b/bin/puppetd
index a09d3c73b..1235a36fe 100755
--- a/bin/puppetd
+++ b/bin/puppetd
@@ -390,10 +390,10 @@ if Puppet[:listen] and ! options[:onetime]
     args[:Handlers] = handlers
     args[:Port] = Puppet[:puppetport]
 
-    require 'puppet/network/server/webrick'
+    require 'puppet/network/http_server/webrick'
 
     begin
-        server = Puppet::Network::Server::WEBrick.new(args)
+        server = Puppet::Network::HTTPServer::WEBrick.new(args)
     rescue => detail
         $stderr.puts detail
         puts detail.backtrace
diff --git a/bin/puppetmasterd b/bin/puppetmasterd
index f764b1901..39802f229 100755
--- a/bin/puppetmasterd
+++ b/bin/puppetmasterd
@@ -238,11 +238,11 @@ begin
     case Puppet[:servertype]
     when "webrick"
         # use the default, um, everything
-        require 'puppet/network/server/webrick'
-        webserver = server = Puppet::Network::Server::WEBrick.new(:Handlers => handlers)
+        require 'puppet/network/http_server/webrick'
+        webserver = server = Puppet::Network::HTTPServer::WEBrick.new(:Handlers => handlers)
     when "mongrel":
-        require 'puppet/network/server/mongrel'
-        server = Puppet::Network::Server::Mongrel.new(handlers)
+        require 'puppet/network/http_server/mongrel'
+        server = Puppet::Network::HTTPServer::Mongrel.new(handlers)
         addr = Puppet[:bindaddress]
         if addr == ""
             addr =  "127.0.0.1"
diff --git a/lib/puppet/feature/base.rb b/lib/puppet/feature/base.rb
index 6368d0931..518e9b419 100644
--- a/lib/puppet/feature/base.rb
+++ b/lib/puppet/feature/base.rb
@@ -18,7 +18,7 @@ Puppet.features.add(:libshadow, :libs => ["shadow"])
 Puppet.features.add(:root) { require 'puppet/util/suidmanager'; Puppet::Util::SUIDManager.uid == 0 }
 
 # We've got mongrel available
-Puppet.features.add(:mongrel, :libs => %w{rubygems mongrel puppet/network/server/mongrel})
+Puppet.features.add(:mongrel, :libs => %w{rubygems mongrel puppet/network/http_server/mongrel})
 
 # We have lcs diff
 Puppet.features.add :diff, :libs => %w{diff/lcs diff/lcs/hunk}
diff --git a/lib/puppet/network/http_server.rb b/lib/puppet/network/http_server.rb
new file mode 100644
index 000000000..1ca01b0a6
--- /dev/null
+++ b/lib/puppet/network/http_server.rb
@@ -0,0 +1,2 @@
+module Puppet::Network::HTTPServer
+end
diff --git a/lib/puppet/network/server/mongrel.rb b/lib/puppet/network/http_server/mongrel.rb
index 8cd7e807b..ba4b048b3 100644
--- a/lib/puppet/network/server/mongrel.rb
+++ b/lib/puppet/network/http_server/mongrel.rb
@@ -31,7 +31,7 @@ require 'rubygems'
 require 'mongrel'
 require 'xmlrpc/server'
 require 'puppet/network/xmlrpc/server'
-require 'puppet/network/server'
+require 'puppet/network/http_server'
 require 'puppet/network/client_request'
 require 'puppet/daemon'
 
@@ -49,7 +49,7 @@ require 'resolv'
 # handler.xmlrpc_server.add_handler("my.add") { |a, b| a.to_i + b.to_i }
 # </pre>
 module Puppet::Network
-    class Server::Mongrel < ::Mongrel::HttpHandler
+    class HTTPServer::Mongrel < ::Mongrel::HttpHandler
         include Puppet::Daemon
         attr_reader :xmlrpc_server
 
diff --git a/lib/puppet/network/server/webrick.rb b/lib/puppet/network/http_server/webrick.rb
index dd9a91c7c..2cf85b7b6 100644
--- a/lib/puppet/network/server/webrick.rb
+++ b/lib/puppet/network/http_server/webrick.rb
@@ -6,7 +6,7 @@ require 'fcntl'
 
 require 'puppet/sslcertificates/support'
 require 'puppet/network/xmlrpc/webrick_servlet'
-require 'puppet/network/server'
+require 'puppet/network/http_server'
 require 'puppet/network/client'
 
 module Puppet
@@ -14,7 +14,7 @@ module Puppet
     module Network
         # The old-school, pure ruby webrick server, which is the default serving
         # mechanism.
-        class Server::WEBrick < WEBrick::HTTPServer
+        class HTTPServer::WEBrick < WEBrick::HTTPServer
             include Puppet::Daemon
             include Puppet::SSLCertificates::Support
 
diff --git a/lib/puppet/network/rest_server.rb b/lib/puppet/network/rest_server.rb
deleted file mode 100644
index d1206928c..000000000
--- a/lib/puppet/network/rest_server.rb
+++ /dev/null
@@ -1,37 +0,0 @@
-class Puppet::Network::RESTServer
-  attr_reader :server
-  
-  def initialize(args = {})
-    raise(ArgumentError, "requires :server to be specified") unless args[:server]
-    @routes = {}
-    @listening = false
-    @server = args[:server]
-  end
-
-  def register(*indirections)
-    raise ArgumentError, "indirection names are required" if indirections.empty?
-    indirections.flatten.each { |i| @routes[i.to_sym] = true }
-  end
-  
-  def unregister(*indirections)
-    indirections = @routes.keys if indirections.empty?
-    indirections.flatten.each do |i|
-      raise(ArgumentError, "indirection [%s] is not known" % i) unless @routes[i.to_sym]
-      @routes.delete(i.to_sym)
-    end
-  end
-
-  def listening?
-    @listening
-  end
-  
-  def listen
-    raise "Cannot listen -- already listening" if listening?
-    @listening = true
-  end
-  
-  def unlisten
-    raise "Cannot unlisten -- not currently listening" unless listening?
-    @listening = false
-  end
-end
diff --git a/lib/puppet/network/server.rb b/lib/puppet/network/server.rb
index 0dee30b10..84a71a6b4 100644
--- a/lib/puppet/network/server.rb
+++ b/lib/puppet/network/server.rb
@@ -1,4 +1,66 @@
-# Just a stub, so we can correctly scope other classes.
-module Puppet::Network::Server # :nodoc:
+class Puppet::Network::Server
+  attr_reader :server_type
+
+  # which HTTP server subclass actually handles web requests of a certain type?  (e.g., :rest => RESTServer)
+  def self.server_class_by_name(name)
+    klass = (name.to_s + 'Server').to_sym
+    const_get klass
+  end
+  
+  # we will actually return an instance of the Server subclass which handles the HTTP web server, instead of 
+  # an instance of this generic Server class.  A tiny bit of sleight-of-hand is necessary to make this happen.
+  def self.new(args = {})
+    server_type = Puppet[:servertype] or raise "No servertype configuration found."
+    obj = self.server_class_by_name(server_type).allocate
+    obj.send :initialize, args.merge(:server_type => server_type)
+    obj
+  end
+  
+  def initialize(args = {})
+    @routes = {}
+    @listening = false
+    @server_type = args[:server_type]
+    self.register(args[:handlers]) if args[:handlers]
+  end
+
+  def register(*indirections)
+    raise ArgumentError, "indirection names are required" if indirections.empty?
+    indirections.flatten.each { |i| @routes[i.to_sym] = true }
+  end
+  
+  def unregister(*indirections)
+    indirections = @routes.keys if indirections.empty?
+    indirections.flatten.each do |i|
+      raise(ArgumentError, "indirection [%s] is not known" % i) unless @routes[i.to_sym]
+      @routes.delete(i.to_sym)
+    end
+  end
+
+  def listening?
+    @listening
+  end
+  
+  def listen
+    raise "Cannot listen -- already listening" if listening?
+    start_web_server
+    @listening = true
+  end
+  
+  def unlisten
+    raise "Cannot unlisten -- not currently listening" unless listening?
+    stop_web_server
+    @listening = false
+  end
+
+  private
+  
+  def start_web_server
+    raise NotImplementedError, "this method needs to be implemented by the actual web server (sub)class"
+  end
+  
+  def stop_web_server
+    raise NotImplementedError, "this method needs to be implemented by the actual web server (sub)class"
+  end
 end
 
+
diff --git a/spec/unit/network/rest_server.rb b/spec/unit/network/server.rb
index 408bb36b3..5a1980fec 100644
--- a/spec/unit/network/rest_server.rb
+++ b/spec/unit/network/server.rb
@@ -5,17 +5,48 @@
 
 require File.dirname(__FILE__) + '/../../spec_helper'
 
-require 'puppet/network/rest_server'
+require 'puppet/network/server'
 
-describe Puppet::Network::RESTServer, "when initializing" do
-  it "should require specifying which HTTP server will be used to provide access to clients" do
-    Proc.new { Puppet::Network::RESTServer.new }.should raise_error(ArgumentError)
+# a fake server class, so we don't have to implement full autoloading etc. (or at least just yet) just to do testing
+class TestServer < Puppet::Network::Server
+  def start_web_server
+  end
+  
+  def stop_web_server
   end
 end
 
-describe Puppet::Network::RESTServer, "in general" do
+describe Puppet::Network::Server, "when initializing" do
   before do
-    @server = Puppet::Network::RESTServer.new(:server => :mongrel)
+    Puppet::Network::Server.stubs(:server_class_by_name).returns(TestServer)
+  end
+  
+  it "should use the Puppet configurator to determine which HTTP server will be used to provide access to clients" do
+    Puppet.expects(:[]).with(:servertype).returns(:suparserver)
+    @server = Puppet::Network::Server.new
+    @server.server_type.should == :suparserver
+  end
+  
+  it "should fail to initialize if there is no HTTP server known to the Puppet configurator" do
+    Puppet.expects(:[]).with(:servertype).returns(nil)
+    Proc.new { Puppet::Network::Server.new }.should raise_error
+  end
+  
+  it "should allow registering indirections" do
+    @server = Puppet::Network::Server.new(:handlers => [ :foo, :bar, :baz])
+    Proc.new { @server.unregister(:foo, :bar, :baz) }.should_not raise_error
+  end
+  
+  it "should not be listening after initialization" do
+    Puppet::Network::Server.new.should_not be_listening
+  end
+end
+
+describe Puppet::Network::Server, "in general" do
+  before do
+    Puppet::Network::Server.stubs(:server_class_by_name).returns(TestServer)
+    Puppet.stubs(:[]).with(:servertype).returns(:suparserver)
+    @server = Puppet::Network::Server.new
   end
   
   it "should allow registering an indirection for client access by specifying its indirection name" do
@@ -70,23 +101,25 @@ describe Puppet::Network::RESTServer, "in general" do
   end
   
   it "should provide a means of determining which HTTP server will be used to provide access to clients" do
-    @server.server.should == :mongrel
+    @server.server_type.should == :suparserver
   end
 
-  it "should allow for multiple configurations, each allowing a different HTTP server handling different indirections" do
-    @server2 = Puppet::Network::RESTServer.new(:server => :webrick)
+  it "should allow for multiple configurations, each handling different indirections" do
+    @server2 = Puppet::Network::Server.new
     @server.register(:foo, :bar)
     @server2.register(:foo, :xyzzy)
     @server.unregister(:foo, :bar)
     @server2.unregister(:foo, :xyzzy)
     Proc.new { @server.unregister(:xyzzy) }.should raise_error(ArgumentError)
-    Proc.new { @server2.unregister(:bar)}.should raise_error(ArgumentError)
+    Proc.new { @server2.unregister(:bar) }.should raise_error(ArgumentError)
   end  
 end
 
-describe Puppet::Network::RESTServer, "when listening is not turned on" do
+describe Puppet::Network::Server, "when listening is turned off" do
   before do
-    @server = Puppet::Network::RESTServer.new(:server => :mongrel)
+    Puppet::Network::Server.stubs(:server_class_by_name).returns(TestServer)
+    Puppet.stubs(:[]).with(:servertype).returns(:suparserver)
+    @server = Puppet::Network::Server.new
   end
   
   it "should allow listening to be turned on" do
@@ -101,6 +134,11 @@ describe Puppet::Network::RESTServer, "when listening is not turned on" do
     @server.should_not be_listening
   end
   
+  it "should cause the HTTP server to listen when listening is turned on" do
+    @server.expects(:start_web_server)
+    @server.listen
+  end
+  
   it "should not route HTTP GET requests on indirector's name to indirector find for the specified HTTP server"
   it "should not route HTTP GET requests on indirector's plural name to indirector search for the specified HTTP server"
   it "should not route HTTP DELETE requests on indirector's name to indirector destroy for the specified HTTP server"
@@ -109,9 +147,11 @@ describe Puppet::Network::RESTServer, "when listening is not turned on" do
   # TODO: FIXME write integrations which fire up actual webrick / mongrel servers and are thus webrick / mongrel specific?]
 end
 
-describe Puppet::Network::RESTServer, "when listening is turned on" do
+describe Puppet::Network::Server, "when listening is turned on" do
   before do
-    @server = Puppet::Network::RESTServer.new(:server => :mongrel)
+    Puppet::Network::Server.stubs(:server_class_by_name).returns(TestServer)
+    Puppet.stubs(:[]).with(:servertype).returns(:suparserver)
+    @server = Puppet::Network::Server.new
     @server.listen
   end
   
@@ -123,9 +163,14 @@ describe Puppet::Network::RESTServer, "when listening is turned on" do
     Proc.new { @server.listen }.should raise_error(RuntimeError)
   end
   
-  it "should indicate that it is listening" do
+  it "should indicate that listening is turned off" do
     @server.should be_listening
   end
+
+  it "should cause the HTTP server to stop listening when listening is turned off" do
+    @server.expects(:stop_web_server)
+    @server.unlisten
+  end
   
   it "should route HTTP GET requests on indirector's name to indirector find for the specified HTTP server"
   it "should route HTTP GET requests on indirector's plural name to indirector search for the specified HTTP server"
diff --git a/test/lib/puppettest/servertest.rb b/test/lib/puppettest/servertest.rb
index 3d1381111..11c24f5b4 100644
--- a/test/lib/puppettest/servertest.rb
+++ b/test/lib/puppettest/servertest.rb
@@ -1,5 +1,5 @@
 require 'puppettest'
-require 'puppet/network/server/webrick'
+require 'puppet/network/http_server/webrick'
 
 module PuppetTest::ServerTest
     include PuppetTest
@@ -47,7 +47,7 @@ module PuppetTest::ServerTest
         # then create the actual server
         server = nil
         assert_nothing_raised {
-            server = Puppet::Network::Server::WEBrick.new(
+            server = Puppet::Network::HTTPServer::WEBrick.new(
                 :Port => @@port,
                 :Handlers => handlers
             )
diff --git a/test/network/handler/ca.rb b/test/network/handler/ca.rb
index 3e74f8e05..e349f9c4c 100755
--- a/test/network/handler/ca.rb
+++ b/test/network/handler/ca.rb
@@ -191,7 +191,7 @@ class TestCA < Test::Unit::TestCase
         server = nil
         Puppet[:name] = "puppetmasterd"
         assert_nothing_raised {
-            server = Puppet::Network::Server::WEBrick.new(
+            server = Puppet::Network::HTTPServer::WEBrick.new(
                 :Port => @@port,
                 :Handlers => {
                     :CA => {}, # so that certs autogenerate
diff --git a/test/network/server/mongrel_test.rb b/test/network/server/mongrel_test.rb
index 05d18257b..52d8790eb 100755
--- a/test/network/server/mongrel_test.rb
+++ b/test/network/server/mongrel_test.rb
@@ -12,7 +12,7 @@ class TestMongrelServer < PuppetTest::TestCase
 
     def mkserver(handlers = nil)
         handlers ||= { :Status => nil }
-        mongrel = Puppet::Network::Server::Mongrel.new(handlers)
+        mongrel = Puppet::Network::HTTPServer::Mongrel.new(handlers)
     end
 
     # Make sure client info is correctly extracted.
diff --git a/test/network/server/webrick.rb b/test/network/server/webrick.rb
index 0f76659b1..5919461ed 100755
--- a/test/network/server/webrick.rb
+++ b/test/network/server/webrick.rb
@@ -3,7 +3,7 @@
 $:.unshift("../../lib") if __FILE__ =~ /\.rb$/
 
 require 'puppettest'
-require 'puppet/network/server/webrick'
+require 'puppet/network/http_server/webrick'
 require 'mocha'
 
 class TestWebrickServer < Test::Unit::TestCase
@@ -19,7 +19,7 @@ class TestWebrickServer < Test::Unit::TestCase
     def test_basics
         server = nil
         assert_raise(Puppet::Error, "server succeeded with no cert") do
-            server = Puppet::Network::Server::WEBrick.new(
+            server = Puppet::Network::HTTPServer::WEBrick.new(
                 :Port => @@port,
                 :Handlers => {
                     :Status => nil
@@ -28,7 +28,7 @@ class TestWebrickServer < Test::Unit::TestCase
         end
 
         assert_nothing_raised("Could not create simple server") do
-            server = Puppet::Network::Server::WEBrick.new(
+            server = Puppet::Network::HTTPServer::WEBrick.new(
                 :Port => @@port,
                 :Handlers => {
                     :CA => {}, # so that certs autogenerate
@@ -112,7 +112,7 @@ class TestWebrickServer < Test::Unit::TestCase
     def mk_status_server
         server = nil
         assert_nothing_raised() {
-            server = Puppet::Network::Server::WEBrick.new(
+            server = Puppet::Network::HTTPServer::WEBrick.new(
                 :Port => @@port,
                 :Handlers => {
                     :CA => {}, # so that certs autogenerate
diff --git a/test/ral/types/filesources.rb b/test/ral/types/filesources.rb
index 9517db5a5..489bc4e78 100755
--- a/test/ral/types/filesources.rb
+++ b/test/ral/types/filesources.rb
@@ -548,7 +548,7 @@ class TestFileSources < Test::Unit::TestCase
 
         serverpid = nil
         assert_nothing_raised() {
-            server = Puppet::Network::Server::WEBrick.new(
+            server = Puppet::Network::HTTPServer::WEBrick.new(
                 :Handlers => {
                     :CA => {}, # so that certs autogenerate
                     :FileServer => {
@@ -593,7 +593,7 @@ class TestFileSources < Test::Unit::TestCase
 
         serverpid = nil
         assert_nothing_raised("Could not start on port %s" % @port) {
-            server = Puppet::Network::Server::WEBrick.new(
+            server = Puppet::Network::HTTPServer::WEBrick.new(
                 :Port => @port,
                 :Handlers => {
                     :CA => {}, # so that certs autogenerate