Fixing #1544 - plugins in modules now works again

We had to fix the fileserving plumbing to use the request
environment instead of trying to use the node environment.
This was apparently never fixed after we added the environment
to the URI in REST calls.

There's still a bit of refactoring left to clean up the APIs used
in some of this code.

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

11 files changed, 137 insertions, 68 deletions

diff --git a/lib/puppet/file_serving/mount.rb b/lib/puppet/file_serving/mount.rb
index 5ab6d8688..7ee11a99b 100644
--- a/lib/puppet/file_serving/mount.rb
+++ b/lib/puppet/file_serving/mount.rb
@@ -16,15 +16,6 @@ class Puppet::FileServing::Mount < Puppet::Network::AuthStore
 
     attr_reader :name
 
-    # Determine the environment to use, if any.
-    def environment(node_name)
-        if node_name and node = Puppet::Node.find(node_name)
-            Puppet::Node::Environment.new(node.environment)
-        else
-            Puppet::Node::Environment.new
-        end
-    end
-
     def find(path, options)
         raise NotImplementedError
     end
diff --git a/lib/puppet/file_serving/mount/plugins.rb b/lib/puppet/file_serving/mount/plugins.rb
index c7a3ee6ff..02b838c84 100644
--- a/lib/puppet/file_serving/mount/plugins.rb
+++ b/lib/puppet/file_serving/mount/plugins.rb
@@ -5,18 +5,18 @@ require 'puppet/file_serving/mount'
 # many directories into one.
 class Puppet::FileServing::Mount::Plugins < Puppet::FileServing::Mount
     # Return an instance of the appropriate class.
-    def find(relative_path, options = {})
-        return nil unless mod = environment(options[:node]).modules.find { |mod|  mod.plugin(relative_path) }
+    def find(relative_path, environment)
+        return nil unless mod = environment.modules.find { |mod|  mod.plugin(relative_path) }
 
         path = mod.plugin(relative_path)
 
         return path
     end
 
-    def search(relative_path, options = {})
+    def search(relative_path, environment)
         # We currently only support one kind of search on plugins - return
         # them all.
-        paths = environment(options[:node]).modules.find_all { |mod|  mod.plugins? }.collect { |mod| mod.plugin_directory }
+        paths = environment.modules.find_all { |mod| mod.plugins? }.collect { |mod| mod.plugin_directory }
         return nil if paths.empty?
         return paths
     end
diff --git a/lib/puppet/indirector/file_server.rb b/lib/puppet/indirector/file_server.rb
index 5fe744a0e..fe4d4aa1c 100644
--- a/lib/puppet/indirector/file_server.rb
+++ b/lib/puppet/indirector/file_server.rb
@@ -30,7 +30,7 @@ class Puppet::Indirector::FileServer < Puppet::Indirector::Terminus
 
         # The mount checks to see if the file exists, and returns nil
         # if not.
-        return nil unless path = mount.find(relative_path, :node => request.node)
+        return nil unless path = mount.find(relative_path, request.environment)
         result = model.new(path)
         result.links = request.options[:links] if request.options[:links]
         result.collect
@@ -42,9 +42,10 @@ class Puppet::Indirector::FileServer < Puppet::Indirector::Terminus
     def search(request)
         mount, relative_path = configuration.split_path(request)
 
-        return nil unless mount
-
-        return nil unless paths = mount.search(relative_path, :node => request.node)
+        unless mount and paths = mount.search(relative_path, request.environment)
+            Puppet.info "Could not find filesystem info for file '%s' in environment %s" % [request.key, request.environment]
+            return nil
+        end
 
         filesets = paths.collect do |path|
             # Filesets support indirector requests as an options collection
diff --git a/lib/puppet/module.rb b/lib/puppet/module.rb
index 3a6f04d15..2b6777c43 100644
--- a/lib/puppet/module.rb
+++ b/lib/puppet/module.rb
@@ -49,6 +49,8 @@ class Puppet::Module
     end
 
     attr_reader :name, :environment
+    attr_writer :environment
+
     def initialize(name, environment = nil)
         @name = name
         if environment.is_a?(Puppet::Node::Environment)
diff --git a/lib/puppet/node/environment.rb b/lib/puppet/node/environment.rb
index c053e0d5f..323f2793a 100644
--- a/lib/puppet/node/environment.rb
+++ b/lib/puppet/node/environment.rb
@@ -71,6 +71,7 @@ class Puppet::Node::Environment
     cached_attr(:modules, :ttl => Puppet[:filetimeout]) do
         result = []
         Puppet::Module.each_module(modulepath) do |mod|
+            mod.environment = self
             result << mod
         end
         result
diff --git a/spec/integration/indirector/file_content/file_server.rb b/spec/integration/indirector/file_content/file_server.rb
index 965bd8fd1..b3c63fc33 100755
--- a/spec/integration/indirector/file_content/file_server.rb
+++ b/spec/integration/indirector/file_content/file_server.rb
@@ -8,11 +8,36 @@ require File.dirname(__FILE__) + '/../../../spec_helper'
 require 'puppet/indirector/file_content/file_server'
 require 'shared_behaviours/file_server_terminus'
 
+require 'puppet_spec/files'
+
 describe Puppet::Indirector::FileContent::FileServer, " when finding files" do
     it_should_behave_like "Puppet::Indirector::FileServerTerminus"
+    include PuppetSpec::Files
 
     before do
         @terminus = Puppet::Indirector::FileContent::FileServer.new
         @test_class = Puppet::FileServing::Content
     end
+
+    it "should find file content in the environment specified in the request" do
+        path = tmpfile("file_content_with_env")
+
+        Dir.mkdir(path)
+
+        modpath = File.join(path, "mod")
+        FileUtils.mkdir_p(File.join(modpath, "lib"))
+        file = File.join(modpath, "lib", "file.rb")
+        File.open(file, "w") { |f| f.puts "1" }
+
+        env = Puppet::Node::Environment.new("foo")
+        env.stubs(:modulepath).returns [path]
+        Puppet.settings[:modulepath] = "/no/such/file"
+
+        result = Puppet::FileServing::Content.search("plugins", :environment => "foo", :recurse => true)
+
+        result.should_not be_nil
+        result.length.should == 2
+        result[1].should be_instance_of(Puppet::FileServing::Content)
+        result[1].content.should == "1\n"
+    end
 end
diff --git a/spec/integration/node/environment.rb b/spec/integration/node/environment.rb
new file mode 100755
index 000000000..68dfd93a3
--- /dev/null
+++ b/spec/integration/node/environment.rb
@@ -0,0 +1,58 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'puppet_spec/files'
+
+describe Puppet::Node::Environment do
+    include PuppetSpec::Files
+
+    it "should be able to return each module from its environment with the environment, name, and path set correctly" do
+        base = tmpfile("env_modules")
+        Dir.mkdir(base)
+
+        dirs = []
+        mods = {}
+        %w{1 2}.each do |num|
+            dir = File.join(base, "dir#{num}")
+            dirs << dir
+            Dir.mkdir(dir)
+            mod = "mod#{num}"
+            moddir = File.join(dir, mod)
+            mods[mod] = moddir
+            Dir.mkdir(moddir)
+        end
+
+        environment = Puppet::Node::Environment.new("foo")
+        environment.stubs(:modulepath).returns dirs
+
+        environment.modules.each do |mod|
+            mod.environment.should == environment
+            mod.path.should == mods[mod.name]
+        end
+    end
+
+    it "should not yield the same module from different module paths" do
+        base = tmpfile("env_modules")
+        Dir.mkdir(base)
+
+        dirs = []
+        mods = {}
+        %w{1 2}.each do |num|
+            dir = File.join(base, "dir#{num}")
+            dirs << dir
+            Dir.mkdir(dir)
+            mod = "mod"
+            moddir = File.join(dir, mod)
+            mods[mod] = moddir
+            Dir.mkdir(moddir)
+        end
+
+        environment = Puppet::Node::Environment.new("foo")
+        environment.stubs(:modulepath).returns dirs
+
+        mods = environment.modules
+        mods.length.should == 1
+        mods[0].path.should == File.join(base, "dir1", "mod")
+    end
+end
diff --git a/spec/unit/file_serving/mount.rb b/spec/unit/file_serving/mount.rb
index e9e1f0860..cbb97b0d9 100755
--- a/spec/unit/file_serving/mount.rb
+++ b/spec/unit/file_serving/mount.rb
@@ -4,26 +4,8 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/file_serving/mount'
 
 describe Puppet::FileServing::Mount do
-    before do
-        @mount = Puppet::FileServing::Mount.new("foo")
-    end
-
-    it "should be able to look up a node's environment" do
-        Puppet::Node.expects(:find).with("mynode").returns mock('node', :environment => "myenv")
-        Puppet::Node::Environment.expects(:new).with("myenv").returns "eh"
-
-        @mount.environment("mynode").should == "eh"
-    end
-
-    it "should use the default environment if no node information is provided" do
-        Puppet::Node.expects(:find).with("mynode").returns nil
-        Puppet::Node::Environment.expects(:new).with(nil).returns "eh"
-
-        @mount.environment("mynode").should == "eh"
-    end
-
     it "should use 'mount[$name]' as its string form" do
-        @mount.to_s.should == "mount[foo]"
+        Puppet::FileServing::Mount.new("foo").to_s.should == "mount[foo]"
     end
 end
 
diff --git a/spec/unit/file_serving/mount/plugins.rb b/spec/unit/file_serving/mount/plugins.rb
index ef842d12b..b3a32b70d 100755
--- a/spec/unit/file_serving/mount/plugins.rb
+++ b/spec/unit/file_serving/mount/plugins.rb
@@ -6,65 +6,57 @@ require 'puppet/file_serving/mount/plugins'
 describe Puppet::FileServing::Mount::Plugins, "when finding files" do
     before do
         @mount = Puppet::FileServing::Mount::Plugins.new("modules")
-
-        @environment = stub 'environment', :module => nil
-        @mount.stubs(:environment).returns @environment
     end
 
-    it "should use the node's environment to find the modules" do
+    it "should use the provided environment to find the modules" do
         env = mock 'env'
-        @mount.expects(:environment).with("mynode").returns env
         env.expects(:modules).returns []
 
-        @mount.find("foo", :node => "mynode")
+        @mount.find("foo", env)
     end
 
     it "should return nil if no module can be found with a matching plugin" do
         mod = mock 'module'
         mod.stubs(:plugin).with("foo/bar").returns nil
 
-        @environment.expects(:modules).returns [mod]
-        @mount.find("foo/bar").should be_nil
+        env = stub 'env', :modules => []
+        @mount.find("foo/bar", env).should be_nil
     end
 
     it "should return the file path from the module" do
         mod = mock 'module'
         mod.stubs(:plugin).with("foo/bar").returns "eh"
 
-        @environment.expects(:modules).returns [mod]
-        @mount.find("foo/bar").should == "eh"
+        env = stub 'env', :modules => [mod]
+        @mount.find("foo/bar", env).should == "eh"
     end
 end
 
 describe Puppet::FileServing::Mount::Plugins, "when searching for files" do
     before do
         @mount = Puppet::FileServing::Mount::Plugins.new("modules")
-
-        @environment = stub 'environment', :module => nil
-        @mount.stubs(:environment).returns @environment
     end
 
     it "should use the node's environment to find the modules" do
         env = mock 'env'
-        @mount.expects(:environment).with("mynode").returns env
         env.expects(:modules).returns []
 
-        @mount.search("foo", :node => "mynode")
+        @mount.search("foo", env)
     end
 
     it "should return nil if no modules can be found that have plugins" do
         mod = mock 'module'
         mod.stubs(:plugins?).returns false
 
-        @environment.expects(:modules).returns [mod]
-        @mount.search("foo/bar").should be_nil
+        env = stub 'env', :modules => []
+        @mount.search("foo/bar", env).should be_nil
     end
 
     it "should return the plugin paths for each module that has plugins" do
         one = stub 'module', :plugins? => true, :plugin_directory => "/one"
         two = stub 'module', :plugins? => true, :plugin_directory => "/two"
 
-        @environment.expects(:modules).returns [one, two]
-        @mount.search("foo/bar").should == %w{/one /two}
+        env = stub 'env', :modules => [one, two]
+        @mount.search("foo/bar", env).should == %w{/one /two}
     end
 end
diff --git a/spec/unit/indirector/file_server.rb b/spec/unit/indirector/file_server.rb
index 7dab320c1..a2e2ff811 100755
--- a/spec/unit/indirector/file_server.rb
+++ b/spec/unit/indirector/file_server.rb
@@ -28,7 +28,7 @@ describe Puppet::Indirector::FileServer do
         @configuration = mock 'configuration'
         Puppet::FileServing::Configuration.stubs(:create).returns(@configuration)
 
-        @request = Puppet::Indirector::Request.new(:myind, :mymethod, @uri)
+        @request = Puppet::Indirector::Request.new(:myind, :mymethod, @uri, :environment => "myenv")
     end
 
     describe "when finding files" do
@@ -52,7 +52,15 @@ describe Puppet::Indirector::FileServer do
         it "should use the mount to find the full path" do
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:find).with { |key, options| key == "rel/path" }
+            @mount.expects(:find).with { |key, env| key == "rel/path" }
+
+            @file_server.find(@request)
+        end
+
+        it "should pass the request's environment when finding a file" do
+            @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
+
+            @mount.expects(:find).with { |key, env| env == @request.environment }
 
             @file_server.find(@request)
         end
@@ -60,7 +68,7 @@ describe Puppet::Indirector::FileServer do
         it "should return nil if it cannot find a full path" do
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:find).with { |key, options| key == "rel/path" }.returns nil
+            @mount.expects(:find).with { |key, env| key == "rel/path" }.returns nil
 
             @file_server.find(@request).should be_nil
         end
@@ -68,7 +76,7 @@ describe Puppet::Indirector::FileServer do
         it "should create an instance with the found path" do
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:find).with { |key, options| key == "rel/path" }.returns "/my/file"
+            @mount.expects(:find).with { |key, env| key == "rel/path" }.returns "/my/file"
 
             @model.expects(:new).with("/my/file").returns @instance
 
@@ -79,7 +87,7 @@ describe Puppet::Indirector::FileServer do
             @request.options[:links] = true
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:find).with { |key, options| key == "rel/path" }.returns "/my/file"
+            @mount.expects(:find).with { |key, env| key == "rel/path" }.returns "/my/file"
 
             @model.expects(:new).with("/my/file").returns @instance
 
@@ -92,7 +100,7 @@ describe Puppet::Indirector::FileServer do
             @request.options[:links] = true
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:find).with { |key, options| key == "rel/path" }.returns "/my/file"
+            @mount.expects(:find).with { |key, env| key == "rel/path" }.returns "/my/file"
 
             @model.expects(:new).with("/my/file").returns @instance
 
@@ -123,7 +131,15 @@ describe Puppet::Indirector::FileServer do
         it "should use the mount to search for the full paths" do
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:search).with { |key, options| key == "rel/path" }
+            @mount.expects(:search).with { |key, env| key == "rel/path" }
+
+            @file_server.search(@request)
+        end
+
+        it "should pass the request's environment" do
+            @configuration.stubs(:split_path).returns([@mount, "rel/path"])
+
+            @mount.expects(:search).with { |key, env| env == @request.environment }
 
             @file_server.search(@request)
         end
@@ -131,7 +147,7 @@ describe Puppet::Indirector::FileServer do
         it "should return nil if searching does not find any full paths" do
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:search).with { |key, options| key == "rel/path" }.returns nil
+            @mount.expects(:search).with { |key, env| key == "rel/path" }.returns nil
 
             @file_server.search(@request).should be_nil
         end
@@ -139,7 +155,7 @@ describe Puppet::Indirector::FileServer do
         it "should create a fileset with each returned path and merge them" do
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:search).with { |key, options| key == "rel/path" }.returns %w{/one /two}
+            @mount.expects(:search).with { |key, env| key == "rel/path" }.returns %w{/one /two}
 
             FileTest.stubs(:exist?).returns true
 
@@ -156,7 +172,7 @@ describe Puppet::Indirector::FileServer do
         it "should create an instance with each path resulting from the merger of the filesets" do
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:search).with { |key, options| key == "rel/path" }.returns []
+            @mount.expects(:search).with { |key, env| key == "rel/path" }.returns []
 
             FileTest.stubs(:exist?).returns true
 
@@ -178,7 +194,7 @@ describe Puppet::Indirector::FileServer do
         it "should set 'links' on the instances if it is set in the request options" do
             @configuration.expects(:split_path).with(@request).returns([@mount, "rel/path"])
 
-            @mount.expects(:search).with { |key, options| key == "rel/path" }.returns []
+            @mount.expects(:search).with { |key, env| key == "rel/path" }.returns []
 
             FileTest.stubs(:exist?).returns true
 
diff --git a/spec/unit/node/environment.rb b/spec/unit/node/environment.rb
index f8b2dea7f..18747d1b7 100755
--- a/spec/unit/node/environment.rb
+++ b/spec/unit/node/environment.rb
@@ -105,9 +105,10 @@ describe Puppet::Node::Environment do
             module_path = %w{/one /two}.join(File::PATH_SEPARATOR)
             env.expects(:modulepath).returns module_path
 
-            Puppet::Module.expects(:each_module).with(module_path).multiple_yields("mod1", "mod2")
+            mods = [Puppet::Module.new('mod1'), Puppet::Module.new("mod2")]
+            Puppet::Module.expects(:each_module).with(module_path).multiple_yields(*mods)
 
-            env.modules.should == %w{mod1 mod2}
+            env.modules.should == mods
         end
 
         it "should be able to return an individual module that exists in its module path" do