Merge pull request #8733 from GabrielNagy/PUP-11201/refine-last-used-env

(PUP-11201) Refine when the last used environment is used

Author: GabrielNagy

acceptance/tests/environment/feature_branch_configured_environment.rb

@@ -0,0 +1,27 @@
+test_name "Agent should use set environment after running with specified environment" do
+  require 'puppet/acceptance/environment_utils'
+  extend Puppet::Acceptance::EnvironmentUtils
+
+  tag 'audit:high',
+      'server'
+
+  # Remove all traces of the last used environment
+  teardown do
+    agents.each do |agent|
+      on(agent, puppet('config print lastrunfile')) do |command_result|
+        agent.rm_rf(command_result.stdout)
+      end
+    end
+  end
+
+  tmp_environment = mk_tmp_environment_with_teardown(master, 'special')
+  agents.each do |agent|
+    on(agent, "puppet agent -t --environment #{tmp_environment}") do |result|
+      assert_match(/Info: Using environment 'special_\w+'/, result.stdout)
+    end
+
+    on(agent, "puppet agent -t") do |result|
+      assert_match(/Info: Using environment 'production'/, result.stdout)
+    end
+  end
+end

acceptance/tests/environment/use_last_server_specified_environment.rb

@@ -0,0 +1,78 @@
+test_name "Agent should use the last server-specified environment if server is authoritative" do
+  require 'puppet/acceptance/environment_utils'
+  extend Puppet::Acceptance::EnvironmentUtils
+
+  tag 'audit:high',
+      'server'
+
+  # Remove all traces of the last used environment
+  teardown do
+    agents.each do |agent|
+      on(agent, puppet('config print lastrunfile')) do |command_result|
+        agent.rm_rf(command_result.stdout)
+      end
+    end
+  end
+
+  testdir = create_tmpdir_for_user(master, 'use_enc_env')
+
+  create_remote_file(master, "#{testdir}/enc.rb", <<END)
+#!#{master['privatebindir'] || '/opt/puppetlabs/puppet/bin'}/ruby
+puts <<YAML
+parameters:
+environment: special
+YAML
+END
+  on(master, "chmod 755 '#{testdir}/enc.rb'")
+
+  apply_manifest_on(master, <<-MANIFEST, :catch_failures => true)
+  File {
+    ensure => directory,
+    mode => "0770",
+    owner => #{master.puppet['user']},
+    group => #{master.puppet['group']},
+  }
+  file {
+    '#{testdir}/environments':;
+    '#{testdir}/environments/production':;
+    '#{testdir}/environments/production/manifests':;
+    '#{testdir}/environments/special/':;
+    '#{testdir}/environments/special/manifests':;
+  }
+  file { '#{testdir}/environments/production/manifests/site.pp':
+    ensure => file,
+    mode => "0640",
+    content => 'notify { "production environment": }',
+  }
+  file { '#{testdir}/environments/special/manifests/different.pp':
+    ensure => file,
+    mode => "0640",
+    content => 'notify { "special environment": }',
+  }
+  MANIFEST
+
+  master_opts = {
+    'main' => {
+      'environmentpath' => "#{testdir}/environments",
+    },
+  }
+  master_opts['master'] = {
+    'node_terminus'  => 'exec',
+    'external_nodes' => "#{testdir}/enc.rb",
+  } if !master.is_pe?
+
+  with_puppet_running_on(master, master_opts, testdir) do
+    agents.each do |agent|
+      run_agent_on(agent, '--no-daemonize --onetime --verbose') do |result|
+        assert_match(/Info: Using environment 'production'/, result.stdout)
+        assert_match(/Local environment: 'production' doesn't match server specified environment 'special', restarting agent run with environment 'special'/, result.stdout)
+        assert_match(/Notice: special environment/, result.stdout)
+      end
+
+      run_agent_on(agent, '--no-daemonize --onetime --verbose') do |result|
+        assert_match(/Info: Using environment 'special'/, result.stdout)
+        assert_match(/Notice: special environment/, result.stdout)
+      end
+    end
+  end
+end

lib/puppet/configurer.rb

@@ -257,6 +257,7 @@ def run(options = {})
 
   def run_internal(options)
     report = options[:report]
+    report.initial_environment = Puppet[:environment]
 
     if options[:start_time]
       startup_time = Time.now - options[:start_time]
@@ -296,14 +297,18 @@ def run_internal(options)
       configured_environment = Puppet[:environment] if Puppet.settings.set_by_config?(:environment)
 
       # We only need to find out the environment to run in if we don't already have a catalog
-      unless (cached_catalog || options[:catalog] || configured_environment)
-        Puppet.debug(_("No environment configured, attempting to find out the last used environment"))
-        if last_agent_environment
-          @environment = last_agent_environment
-          report.environment = last_agent_environment
+      unless (cached_catalog || options[:catalog] || Puppet.settings.set_by_cli?(:environment) || Puppet[:strict_environment_mode])
+        Puppet.debug(_("Environment not passed via CLI and no catalog was given, attempting to find out the last server-specified environment"))
+        if last_server_specified_environment
+          @environment = last_server_specified_environment
+          report.environment = last_server_specified_environment
+        else
+          Puppet.debug(_("Could not find a usable environment in the lastrunfile. Either the file does not exist, does not have the required keys, or the values of 'initial_environment' and 'converged_environment' are identical."))
         end
       end
 
+      Puppet.info _("Using environment '%{env}'") % { env: @environment }
+
       # This is to maintain compatibility with anyone using this class
       # aside from agent, apply, device.
       unless Puppet.lookup(:loaders) { nil }
@@ -467,21 +472,23 @@ def find_functional_server
   end
   private :find_functional_server
 
-  def last_agent_environment
-    return @last_agent_environment if @last_agent_environment
+  def last_server_specified_environment
+    return @last_server_specified_environment if @last_server_specified_environment
     if Puppet::FileSystem.exist?(Puppet[:lastrunfile])
       summary = Puppet::Util::Yaml.safe_load_file(Puppet[:lastrunfile])
       return unless summary.dig('application', 'run_mode') == 'agent'
-      @last_agent_environment = summary.dig('application', 'environment')
+      initial_environment = summary.dig('application', 'initial_environment')
+      converged_environment = summary.dig('application', 'converged_environment')
+      @last_server_specified_environment = converged_environment if initial_environment != converged_environment
     end
 
-    Puppet.debug(_("Found last used environment: %{environment}") % { environment: @last_agent_environment }) if @last_agent_environment
-    @last_agent_environment
+    Puppet.debug(_("Found last server-specified environment: %{environment}") % { environment: @last_server_specified_environment }) if @last_server_specified_environment
+    @last_server_specified_environment
   rescue => detail
-    Puppet.debug(_("Unable to get last used environment: %{detail}") % { detail: detail })
+    Puppet.debug(_("Could not find last server-specified environment: %{detail}") % { detail: detail })
     nil
   end
-  private :last_agent_environment
+  private :last_server_specified_environment
 
   def send_report(report)
     puts report.summary if Puppet[:summarize]

lib/puppet/transaction/report.rb

@@ -77,6 +77,10 @@ class Puppet::Transaction::Report
   # @return [String] the environment name
   attr_accessor :environment
 
+  # The name of the environment the agent initially started in
+  # @return [String] the environment name
+  attr_accessor :initial_environment
+
   # Whether there are changes that we decided not to apply because of noop
   # @return [Boolean]
   #
@@ -384,7 +388,8 @@ def raw_summary
       },
       "application" => {
         "run_mode" => Puppet.run_mode.name.to_s,
-        "environment" => environment
+        "initial_environment" => initial_environment,
+        "converged_environment" => environment
       }
     }
 

spec/unit/configurer_spec.rb

@@ -1093,8 +1093,7 @@ def expects_neither_new_or_cached_catalog
     include PuppetSpec::Settings
 
     describe "when the last used environment is available" do
-      let(:last_used_environment) { 'development' }
-      let(:default_environment) { 'production' }
+      let(:last_server_specified_environment) { 'development' }
 
       before do
         Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', <<~SUMMARY)
@@ -1103,7 +1102,8 @@ def expects_neither_new_or_cached_catalog
           config: 1624882680
           puppet: 6.24.0
         application:
-          environment: development
+          initial_environment: #{Puppet[:environment]}
+          converged_environment: #{last_server_specified_environment}
           run_mode: agent
         SUMMARY
       end
@@ -1128,64 +1128,88 @@ def expects_neither_new_or_cached_catalog
         expect(configurer.environment).to eq('usethis')
       end
 
-      it "uses the default environment if given a catalog" do
+      it "uses environment from Puppet[:environment] if given a catalog" do
         configurer.run(catalog: catalog)
 
-        expect(configurer.environment).to eq(default_environment)
+        expect(configurer.environment).to eq(Puppet[:environment])
       end
 
-      it "uses the default environment if use_cached_catalog = true" do
+      it "uses environment from Puppet[:environment] if use_cached_catalog = true" do
         Puppet[:use_cached_catalog] = true
         expects_cached_catalog_only(catalog)
         configurer.run
 
-        expect(configurer.environment).to eq(default_environment)
+        expect(configurer.environment).to eq(Puppet[:environment])
       end
 
-      describe "when the environment is not configured" do
+      describe "when the environment is not set via CLI" do
         it "uses the environment found in lastrunfile if the key exists" do
           configurer.run
 
-          expect(configurer.environment).to eq(last_used_environment)
+          expect(configurer.environment).to eq(last_server_specified_environment)
         end
 
-        it "uses the default environment if the run mode doesn't match" do
+        it "uses environment from Puppet[:environment] if strict_environment_mode is set" do
+          Puppet[:strict_environment_mode] = true
+          configurer.run
+
+          expect(configurer.environment).to eq(Puppet[:environment])
+        end
+
+        it "uses environment from Puppet[:environment] if initial_environment is the same as converged_environment" do
+          Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', <<~SUMMARY)
+          ---
+          version:
+            config: 1624882680
+            puppet: 6.24.0
+          application:
+            initial_environment: development
+            converged_environment: development
+            run_mode: agent
+          SUMMARY
+          configurer.run
+
+          expect(configurer.environment).to eq(Puppet[:environment])
+        end
+
+        it "uses environment from Puppet[:environment] if the run mode doesn't match" do
           Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', <<~SUMMARY)
           ---
           version:
             config: 1624882680
             puppet: 6.24.0
           application:
-            environment: development
+            initial_environment: #{Puppet[:environment]}
+            converged_environment: #{last_server_specified_environment}
             run_mode: user
           SUMMARY
           configurer.run
 
-          expect(configurer.environment).to eq(default_environment)
+          expect(configurer.environment).to eq(Puppet[:environment])
         end
 
-        it "uses the default environment if lastrunfile is invalid YAML" do
+        it "uses environment from Puppet[:environment] if lastrunfile is invalid YAML" do
           Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', <<~SUMMARY)
           Key: 'this is my very very very ' +
                'long string'
           SUMMARY
           configurer.run
 
-          expect(configurer.environment).to eq(default_environment)
+          expect(configurer.environment).to eq(Puppet[:environment])
         end
 
-        it "uses the default environment if lastrunfile exists but is empty" do
+        it "uses environment from Puppet[:environment] if lastrunfile exists but is empty" do
           Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', '')
           configurer.run
 
-          expect(configurer.environment).to eq(default_environment)
+          expect(configurer.environment).to eq(Puppet[:environment])
         end
 
-        it "uses the default environment if the last used one cannot be found" do
+        it "uses environment from Puppet[:environment] if the last used one cannot be found" do
           Puppet[:lastrunfile] = tmpfile('last_run_summary.yaml')
           configurer.run
 
-          expect(configurer.environment).to eq(default_environment)
+          expect(configurer.environment).to eq(Puppet[:environment])
         end
       end
     end