(PUP-10942) Don't leak environment state when creating a new instance

If an enviroment was expired and a new instance of the environment created, it
was possible for the new instance to see the cached modulepath from the old
instance.

This only could happen if the old environment was created via the `Cached#list`
or `Cached#get_conf` method, but not `Cached#get`. The reason is in the `get`
case the entry is cached, so settings are correctly cleared when the environment
is removed from the cache.

It also makes `list` and `get` behave consistently with respect to expired
environments.

Author: joshcooper

lib/puppet/environments.rb

@@ -225,6 +225,10 @@ def get_conf(name)
     private
 
     def create_environment(name)
+      # interpolated modulepaths may be cached from prior environment
+      # instances, eg prior to a code deploy
+      Puppet.settings.clear_environment_settings(name)
+
       env_symbol = name.intern
       setting_values = Puppet.settings.values(env_symbol, Puppet.settings.preferred_run_mode)
       env = Puppet::Node::Environment.create(
@@ -350,6 +354,9 @@ def initialize(loader)
 
     # @!macro loader_list
     def list
+      # Evict all that have expired, in the same way as `get`
+      clear_all_expired
+
       @loader.list
     end
 

lib/puppet/settings/environment_conf.rb

@@ -29,6 +29,7 @@ def self.load_from(path_to_env, global_module_path)
       section = config.sections[:main]
     rescue Errno::ENOENT
       # environment.conf is an optional file
+      Puppet.debug { "Path to #{path_to_env} does not exist, using default environment.conf" }
     end
 
     new(path_to_env, section, global_module_path)

spec/unit/environments_spec.rb

@@ -7,6 +7,7 @@
 module PuppetEnvironments
 describe Puppet::Environments do
   include Matchers::Include
+  include PuppetSpec::Files
 
   FS = Puppet::FileSystem
 
@@ -436,6 +437,56 @@ module PuppetEnvironments
           end
         end
       end
+
+      it "'get' should see module directories added after 'list', but before 'clear_all'" do
+        base_dir = tmpdir("environments")
+        envdir = File.join(base_dir, "a_race")
+        Puppet::FileSystem.mkpath(envdir)
+
+        default_dir = File.join(envdir, "modules")
+        site_dir = File.join(envdir, "site")
+
+        loader = Puppet::Environments::Cached.new(Puppet::Environments::Directories.new(base_dir, []))
+        Puppet.override(:environments => loader) do
+          loader.list
+
+          expect(Puppet.settings.value(:modulepath, :a_race)).to eq(default_dir)
+
+          File.write(File.join(envdir, "environment.conf"), <<-END)
+            modulepath=site#{File::PATH_SEPARATOR}$basemodulepath
+            environment_timeout=unlimited
+          END
+          Puppet::FileSystem.mkpath(site_dir)
+          loader.clear_all
+
+          expect(loader.get(:a_race).modulepath.first).to eq(site_dir)
+        end
+      end
+
+      it "'get' should see module directories added after 'get_conf', but before 'clear_all'" do
+        base_dir = tmpdir("environments")
+        envdir = File.join(base_dir, "a_race")
+        Puppet::FileSystem.mkpath(envdir)
+
+        default_dir = File.join(envdir, "modules")
+        site_dir = File.join(envdir, "site")
+
+        loader = Puppet::Environments::Cached.new(Puppet::Environments::Directories.new(base_dir, []))
+        Puppet.override(:environments => loader) do
+          loader.get_conf(:a_race).modulepath
+
+          expect(Puppet.settings.value(:modulepath, :a_race)).to eq(default_dir)
+
+          File.write(File.join(envdir, "environment.conf"), <<-END)
+            modulepath=site#{File::PATH_SEPARATOR}$basemodulepath
+            environment_timeout=unlimited
+          END
+          Puppet::FileSystem.mkpath(site_dir)
+          loader.clear_all
+
+          expect(loader.get(:a_race).modulepath.first).to eq(site_dir)
+        end
+      end
     end
   end
 
@@ -647,8 +698,6 @@ module PuppetEnvironments
       end
 
       it "evicts an expired environment" do
-        service = ReplayExpirationService.new
-
         expect(service).to receive(:expired?).and_return(true)
 
         with_environment_loaded(service) do |cached|
@@ -703,6 +752,16 @@ module PuppetEnvironments
         expect(service.created_envs).to eq([:an_environment, :an_environment])
         expect(service.evicted_envs).to eq([:an_environment])
       end
+
+      it "evicts expired environments when listing" do
+        expect(service).to receive(:expired?).with(:an_environment).and_return(true)
+
+        with_environment_loaded(service) do |cached|
+          cached.list
+        end
+
+        expect(service.evicted_envs).to eq([:an_environment])
+      end
     end
 
     it "gets an environment.conf" do