Fixing the fileserving terminus selection hook.

It now uses the fact that the indirection request does
URI parsing, rather than doing the parsing on its own.

Signed-off-by: Luke Kanies <luke@madstop.com>

2 files changed, 101 insertions, 100 deletions

diff --git a/lib/puppet/file_serving/indirection_hooks.rb b/lib/puppet/file_serving/indirection_hooks.rb
index 66ed169dc..15564cf3d 100644
--- a/lib/puppet/file_serving/indirection_hooks.rb
+++ b/lib/puppet/file_serving/indirection_hooks.rb
@@ -9,36 +9,37 @@ require 'puppet/file_serving'
 # in file-serving indirections.  This is necessary because
 # the terminus varies based on the URI asked for.
 module Puppet::FileServing::IndirectionHooks
-    PROTOCOL_MAP = {"puppet" => :rest, "file" => :file, "puppetmounts" => :file_server}
+    PROTOCOL_MAP = {"puppet" => :rest, "file" => :file}
 
     # Pick an appropriate terminus based on the protocol.
     def select_terminus(request)
-        full_uri = request.key
-        # Short-circuit to :file if it's a fully-qualified path.
-        return PROTOCOL_MAP["file"] if full_uri =~ /^#{::File::SEPARATOR}/
-        begin
-            uri = URI.parse(URI.escape(full_uri))
-        rescue => detail
-            raise ArgumentError, "Could not understand URI %s: %s" % [full_uri, detail.to_s]
-        end
+        # We rely on the request's parsing of the URI.
 
-        terminus = PROTOCOL_MAP[uri.scheme] || raise(ArgumentError, "URI protocol '%s' is not supported for file serving" % uri.scheme)
+        # Short-circuit to :file if it's a fully-qualified path or specifies a 'file' protocol.
+        return PROTOCOL_MAP["file"] if request.key =~ /^#{::File::SEPARATOR}/
+        return PROTOCOL_MAP["file"] if request.protocol == "file"
 
-        # This provides a convenient mechanism for people to write configurations work
-        # well in both a networked and local setting.
-        if uri.host.nil? and uri.scheme == "puppet" and Puppet.settings[:name] == "puppet"
-            terminus = :file_server
+        # We're heading over the wire the protocol is 'puppet' and we've got a server name or we're not named 'puppet'
+        if request.protocol == "puppet" and (request.server or Puppet.settings[:name] != "puppet")
+            return PROTOCOL_MAP["puppet"]
+        end
+
+        if request.protocol and PROTOCOL_MAP[request.protocol].nil?
+            raise(ArgumentError, "URI protocol '%s' is not currently supported for file serving" % request.protocol)
         end
 
+        # If we're still here, we're using the file_server or modules.
+
         # This is the backward-compatible module terminus.
-        if terminus == :file_server and uri.path =~ %r{^/([^/]+)\b}
-            modname = $1
-            if modname == "modules"
-                terminus = :modules
-            elsif terminus(:modules).find_module(modname, request.options[:node])
-                Puppet.warning "DEPRECATION NOTICE: Found file '%s' in module without using the 'modules' mount; please prefix path with '/modules'" % uri.path
-                terminus = :modules
-            end
+        modname = request.key.split("/")[0]
+
+        if modname == "modules"
+            terminus = :modules
+        elsif terminus(:modules).find_module(modname, request.options[:node])
+            Puppet.warning "DEPRECATION NOTICE: Found file '%s' in module without using the 'modules' mount; please prefix path with 'modules/'" % request.key
+            terminus = :modules
+        else
+            terminus = :file_server
         end
 
         return terminus
diff --git a/spec/unit/file_serving/indirection_hooks.rb b/spec/unit/file_serving/indirection_hooks.rb
index 160e3ff0a..12993bf1b 100755
--- a/spec/unit/file_serving/indirection_hooks.rb
+++ b/spec/unit/file_serving/indirection_hooks.rb
@@ -12,113 +12,113 @@ describe Puppet::FileServing::IndirectionHooks do
         @object = Object.new
         @object.extend(Puppet::FileServing::IndirectionHooks)
 
-        @request = stub 'request', :key => "http://myhost/blah", :options => {:node => "whatever"}
+        @request = stub 'request', :key => "mymod/myfile", :options => {:node => "whatever"}, :server => nil, :protocol => nil
     end
 
     describe "when being used to select termini" do
-        it "should escape the key before parsing" do
-            uri = stub 'uri', :scheme => "puppet", :host => "blah", :path => "/something"
-            URI.expects(:escape).with("http://myhost/blah").returns("escaped_blah")
-            URI.expects(:parse).with("escaped_blah").returns(uri)
-            @object.select_terminus(@request)
+        it "should return :file if the request key is fully qualified" do
+            @request.expects(:key).returns "#{File::SEPARATOR}foo"
+            @object.select_terminus(@request).should == :file
         end
 
-        it "should use the URI class to parse the key" do
-            uri = stub 'uri', :scheme => "puppet", :host => "blah", :path => "/something"
-            URI.expects(:parse).with("http://myhost/blah").returns(uri)
-            @object.select_terminus @request
+        it "should return :file if the URI protocol is set to 'file'" do
+            @request.expects(:protocol).returns "file"
+            @object.select_terminus(@request).should == :file
         end
 
-        it "should choose :rest when the protocol is 'puppet'" do
-            @request.stubs(:key).returns "puppet://host/module/file"
-            @object.select_terminus(@request).should == :rest
+        it "should fail when a protocol other than :puppet or :file is used" do
+            @request.stubs(:protocol).returns "http"
+            proc { @object.select_terminus(@request) }.should raise_error(ArgumentError)
         end
 
-        it "should choose :file_server when the protocol is 'puppetmounts' and the mount name is not 'modules'" do
-            modules = mock 'modules'
-            @object.stubs(:terminus).with(:modules).returns(modules)
-            modules.stubs(:find_module).returns(nil)
+        describe "and the protocol is 'puppet'" do
+            before do
+                @request.stubs(:protocol).returns "puppet"
+            end
 
-            @request.stubs(:key).returns "puppetmounts://host/notmodules/file"
+            it "should choose :rest when a server is specified" do
+                @request.stubs(:protocol).returns "puppet"
+                @request.expects(:server).returns "foo"
+                @object.select_terminus(@request).should == :rest
+            end
 
-            @object.select_terminus(@request).should == :file_server
-        end
+            # This is so a given file location works when bootstrapping with no server.
+            it "should choose :rest when the Settings name isn't 'puppet'" do
+                @request.stubs(:protocol).returns "puppet"
+                @request.stubs(:server).returns "foo"
+                Puppet.settings.stubs(:value).with(:name).returns "foo"
+                @object.select_terminus(@request).should == :rest
+            end
 
-        it "should choose :file_server when no server name is provided, the process name is 'puppet', and the mount name is not 'modules'" do
-            modules = mock 'modules'
-            @object.stubs(:terminus).with(:modules).returns(modules)
-            modules.stubs(:find_module).returns(nil)
+            it "should not choose :rest when the settings name is 'puppet' and no server is specified" do
+                modules = mock 'modules'
 
-            Puppet.settings.expects(:value).with(:name).returns("puppet")
-            @request.stubs(:key).returns "puppet:///notmodules/file"
-            @object.select_terminus(@request).should == :file_server
-        end
+                @object.stubs(:terminus).with(:modules).returns(modules)
+                modules.stubs(:find_module).with("mymod", @request.options[:node]).returns nil
 
-        it "should choose :modules if it would normally choose :file_server but the mount name is 'modules'" do
-            @request.stubs(:key).returns "puppetmounts://host/modules/mymod/file"
-            @object.select_terminus(@request).should == :modules
+                @request.expects(:protocol).returns "puppet"
+                @request.expects(:server).returns nil
+                Puppet.settings.expects(:value).with(:name).returns "puppet"
+                @object.select_terminus(@request).should_not == :rest
+            end
         end
 
-        it "should choose :modules if it would normally choose :file_server but a module exists with the mount name" do
-            modules = mock 'modules'
+        describe "and the terminus is not :rest or :file" do
+            before do
+                @request.stubs(:protocol).returns nil
+            end
 
-            @object.expects(:terminus).with(:modules).returns(modules)
-            modules.expects(:find_module).with("mymod", @request.options[:node]).returns(:thing)
+            it "should choose :modules if the mount name is 'modules'" do
+                @request.stubs(:key).returns "modules/mymod/file"
+                @object.select_terminus(@request).should == :modules
+            end
 
-            @request.stubs(:key).returns "puppetmounts://host/mymod/file"
-            @object.select_terminus(@request).should == :modules
-        end
+            it "should choose :modules and provide a deprecation notice if a module exists with the mount name" do
+                modules = mock 'modules'
 
-        it "should choose :rest when no server name is provided and the process name is not 'puppet'" do
-            Puppet.settings.expects(:value).with(:name).returns("puppetd")
-            @request.stubs(:key).returns "puppet:///module/file"
-            @object.select_terminus(@request).should == :rest
-        end
+                @object.expects(:terminus).with(:modules).returns(modules)
+                modules.expects(:find_module).with("mymod", @request.options[:node]).returns(:thing)
 
-        it "should choose :file when the protocol is 'file'" do
-            @request.stubs(:key).returns "file://host/module/file"
-            @object.select_terminus(@request).should == :file
-        end
+                Puppet.expects(:warning)
 
-        it "should choose :file when the URI is a normal path name" do
-            @request.stubs(:key).returns "/module/file"
-            @object.select_terminus(@request).should == :file
-        end
+                @request.stubs(:key).returns "mymod/file"
+                @object.select_terminus(@request).should == :modules
+            end
 
-        # This is so that we only choose modules over mounts, not file
-        it "should choose :file when the protocol is 'file' and the fully qualified path starts with '/modules'" do
-            @request.stubs(:key).returns "/module/file"
-            @object.select_terminus(@request).should == :file
-        end
+            it "should choose :file_server if the mount name is not 'modules' nor matches a module name" do
+                modules = mock 'modules'
+                @object.stubs(:terminus).with(:modules).returns(modules)
+                modules.stubs(:find_module).returns(nil)
 
-        it "should fail when a protocol other than :puppet, :file, or :puppetmounts is used" do
-            @request.stubs(:key).returns "http:///module/file"
-            proc { @object.select_terminus(@request) }.should raise_error(ArgumentError)
+                @request.stubs(:key).returns "puppetmounts://host/notmodules/file"
+
+                @object.select_terminus(@request).should == :file_server
+            end
         end
-    end
 
-    describe "when looking for a module whose name matches the mount name" do
-        before do
-            @modules = mock 'modules'
-            @object.stubs(:terminus).with(:modules).returns(@modules)
+        describe "when looking for a module whose name matches the mount name" do
+            before do
+                @modules = mock 'modules'
+                @object.stubs(:terminus).with(:modules).returns(@modules)
 
-            @request.stubs(:key).returns "puppetmounts://host/mymod/file"
-        end
+                @request.stubs(:key).returns "mymod/file"
+            end
 
-        it "should use the modules terminus to look up the module" do
-            @modules.expects(:find_module).with("mymod", @request.options[:node])
-            @object.select_terminus @request
-        end
+            it "should use the modules terminus to look up the module" do
+                @modules.expects(:find_module).with("mymod", @request.options[:node])
+                @object.select_terminus @request
+            end
 
-        it "should pass the node name to the modules terminus" do
-            @modules.expects(:find_module).with("mymod", @request.options[:node])
-            @object.select_terminus @request
-        end
+            it "should pass the node name to the modules terminus" do
+                @modules.expects(:find_module).with("mymod", @request.options[:node])
+                @object.select_terminus @request
+            end
 
-        it "should log a deprecation warning if a module is found" do
-            @modules.expects(:find_module).with("mymod", @request.options[:node]).returns(:something)
-            Puppet.expects(:warning)
-            @object.select_terminus @request
+            it "should log a deprecation warning if a module is found" do
+                @modules.expects(:find_module).with("mymod", @request.options[:node]).returns(:something)
+                Puppet.expects(:warning)
+                @object.select_terminus @request
+            end
         end
     end
 end