(PUP-11328) Fallback to node request in configurer

If we can't load our last server specified environment, then fallback to
making a node request like we used to. This way if we're supposed to use
a server specified environment and we upgrade, then we don't revert to
"production" and redownload plugins.

A node request will not be made if the `initial_environment` and the
`converged_environment` in the lastrunfile are identical, and the
`run_mode` is `agent`.

The logic is similar to what was removed in commit 39df438 except
we're using `fail_on_404` so `Puppet::Node.indirection.find` will raise
instead of returning nil.

We also don't clear facts/query_options.

Co-authored-by: Josh Cooper <[email protected]>

Author: 2 people

lib/puppet/configurer.rb

@@ -302,9 +302,16 @@ def run_internal(options)
       # We only need to find out the environment to run in if we don't already have a catalog
       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
+        initial_environment, loaded_last_environment = last_server_specified_environment
+
+        unless loaded_last_environment
+          Puppet.debug(_("Requesting environment from the server"))
+          initial_environment = current_server_specified_environment(@environment, configured_environment, options)
+        end
+
+        if initial_environment
+          @environment = initial_environment
+          report.environment = initial_environment
 
           push_current_environment_and_loaders
         else
@@ -463,24 +470,67 @@ def find_functional_server
   end
   private :find_functional_server
 
+  #
+  # @api private
+  #
+  # Read the last server-specified environment from the lastrunfile. The
+  # environment is considered to be server-specified if the values of
+  # `initial_environment` and `converged_environment` are different.
+  #
+  # @return [String, Boolean] An array containing a string with the environment
+  #   read from the lastrunfile in case the server is authoritative, and a
+  #   boolean marking whether the last environment was correctly loaded.
   def last_server_specified_environment
-    return @last_server_specified_environment if @last_server_specified_environment
+    return @last_server_specified_environment, @loaded_last_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'
-      initial_environment = summary.dig('application', 'initial_environment')
-      converged_environment = summary.dig('application', 'converged_environment')
+      return [nil, nil] unless summary['application']['run_mode'] == 'agent'
+      initial_environment = summary['application']['initial_environment']
+      converged_environment = summary['application']['converged_environment']
       @last_server_specified_environment = converged_environment if initial_environment != converged_environment
+      Puppet.debug(_("Successfully loaded last environment from the lastrunfile"))
+      @loaded_last_environment = true
     end
 
     Puppet.debug(_("Found last server-specified environment: %{environment}") % { environment: @last_server_specified_environment }) if @last_server_specified_environment
-    @last_server_specified_environment
+    [@last_server_specified_environment, @loaded_last_environment]
   rescue => detail
     Puppet.debug(_("Could not find last server-specified environment: %{detail}") % { detail: detail })
-    nil
+    [nil, nil]
   end
   private :last_server_specified_environment
 
+  def current_server_specified_environment(current_environment, configured_environment, options)
+    return @server_specified_environment if @server_specified_environment
+
+    begin
+      node_retr_time = thinmark do
+        node = Puppet::Node.indirection.find(Puppet[:node_name_value],
+                                             :environment => Puppet::Node::Environment.remote(current_environment),
+                                             :configured_environment => configured_environment,
+                                             :ignore_cache => true,
+                                             :transaction_uuid => @transaction_uuid,
+                                             :fail_on_404 => true)
+
+        @server_specified_environment = node.environment.name.to_s
+
+        if @server_specified_environment != @environment
+          Puppet.notice _("Local environment: '%{local_env}' doesn't match server specified node environment '%{node_env}', switching agent to '%{node_env}'.") % { local_env: @environment, node_env: @server_specified_environment }
+        end
+      end
+
+      options[:report].add_times(:node_retrieval, node_retr_time)
+
+      @server_specified_environment
+    rescue => detail
+      Puppet.warning(_("Unable to fetch my node definition, but the agent run will continue:"))
+      Puppet.warning(detail)
+      nil
+    end
+  end
+  private :current_server_specified_environment
+
   def send_report(report)
     puts report.summary if Puppet[:summarize]
     save_last_run_summary(report)

spec/integration/application/agent_spec.rb

@@ -642,6 +642,27 @@ def with_another_agent_running(&block)
   end
 
   context "environment convergence" do
+    it "falls back to making a node request if the last server-specified environment cannot be loaded" do
+      server.start_server do |port|
+        Puppet[:serverport] = port
+        Puppet[:log_level] = 'debug'
+
+        expect {
+          agent.command_line.args << '--test'
+          agent.run
+        }.to exit_with(0)
+         .and output(a_string_matching(%r{Debug: Requesting environment from the server})).to_stdout
+
+        Puppet::Application.clear!
+
+        expect {
+          agent.command_line.args << '--test'
+          agent.run
+        }.to exit_with(0)
+         .and output(a_string_matching(%r{Debug: Successfully loaded last environment from the lastrunfile})).to_stdout
+      end
+    end
+
     it "switches to 'newenv' environment and retries the run" do
       first_run = true
       libdir = File.join(my_fixture_dir, 'lib')

spec/unit/configurer_spec.rb

@@ -1116,6 +1116,9 @@ def expects_neither_new_or_cached_catalog
           converged_environment: #{last_server_specified_environment}
           run_mode: agent
         SUMMARY
+
+        expect(Puppet::Node.indirection).not_to receive(:find)
+          .with(anything, hash_including(:ignore_cache => true, :fail_on_404 => true))
       end
 
       it "prefers the environment set via cli" do
@@ -1125,26 +1128,27 @@ def expects_neither_new_or_cached_catalog
         expect(configurer.environment).to eq('usethis')
       end
 
-      it "prefers the environment set via config" do
+      it "prefers the environment set via lastrunfile over config" do
         FileUtils.mkdir_p(Puppet[:confdir])
         set_puppet_conf(Puppet[:confdir], <<~CONF)
         [main]
         environment = usethis
+        lastrunfile = #{Puppet[:lastrunfile]}
         CONF
 
         Puppet.initialize_settings
         configurer.run
 
-        expect(configurer.environment).to eq('usethis')
+        expect(configurer.environment).to eq(last_server_specified_environment)
       end
 
-      it "uses environment from Puppet[:environment] if given a catalog" do
+      it "uses the environment from Puppet[:environment] if given a catalog" do
         configurer.run(catalog: catalog)
 
         expect(configurer.environment).to eq(Puppet[:environment])
       end
 
-      it "uses environment from Puppet[:environment] if use_cached_catalog = true" do
+      it "uses the environment from Puppet[:environment] if use_cached_catalog = true" do
         Puppet[:use_cached_catalog] = true
         expects_cached_catalog_only(catalog)
         configurer.run
@@ -1173,14 +1177,14 @@ def expects_neither_new_or_cached_catalog
           configurer.run
         end
 
-        it "uses environment from Puppet[:environment] if strict_environment_mode is set" do
+        it "uses the 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
+        it "uses the environment from Puppet[:environment] if initial_environment is the same as converged_environment" do
           Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', <<~SUMMARY)
           ---
           version:
@@ -1195,41 +1199,84 @@ def expects_neither_new_or_cached_catalog
 
           expect(configurer.environment).to eq(Puppet[:environment])
         end
+      end
+    end
+
+    describe "when the last used environment is not available" do
+      describe "when the node request succeeds" do
+        let(:node_environment) { Puppet::Node::Environment.remote(:salam) }
+        let(:node) { double(Puppet::Node, :environment => node_environment) }
+        let(:last_server_specified_environment) { 'development' }
+
+        before do
+          allow(Puppet::Node.indirection).to receive(:find)
+          allow(Puppet::Node.indirection).to receive(:find)
+            .with(anything, hash_including(:ignore_cache => true, :fail_on_404 => true))
+            .and_return(node)
+        end
 
-        it "uses environment from Puppet[:environment] if the run mode doesn't match" do
+        it "uses the environment from the node request 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:
-            initial_environment: #{Puppet[:environment]}
-            converged_environment: #{last_server_specified_environment}
-            run_mode: user
+            ---
+            version:
+              config: 1624882680
+              puppet: 6.24.0
+            application:
+              initial_environment: #{Puppet[:environment]}
+              converged_environment: #{last_server_specified_environment}
+              run_mode: user
           SUMMARY
           configurer.run
 
-          expect(configurer.environment).to eq(Puppet[:environment])
+          expect(configurer.environment).to eq(node_environment.name.to_s)
         end
 
-        it "uses environment from Puppet[:environment] if lastrunfile is invalid YAML" do
+        it "uses the environment from the node request if lastrunfile does not contain the expected keys" do
           Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', <<~SUMMARY)
-          Key: 'this is my very very very ' +
-               'long string'
+            ---
+            version:
+              config: 1624882680
+              puppet: 6.24.0
           SUMMARY
           configurer.run
 
-          expect(configurer.environment).to eq(Puppet[:environment])
+          expect(configurer.environment).to eq(node_environment.name.to_s)
         end
 
-        it "uses environment from Puppet[:environment] if lastrunfile exists but is empty" do
+        it "uses the environment from the node request 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(node_environment.name.to_s)
+        end
+
+        it "uses the environment from the node request if lastrunfile exists but is empty" do
           Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', '')
           configurer.run
 
-          expect(configurer.environment).to eq(Puppet[:environment])
+          expect(configurer.environment).to eq(node_environment.name.to_s)
+        end
+
+        it "uses the environment from the node request if the last used one cannot be found" do
+          Puppet[:lastrunfile] = tmpfile('last_run_summary.yaml')
+          configurer.run
+
+          expect(configurer.environment).to eq(node_environment.name.to_s)
+        end
+      end
+
+      describe "when the node request fails" do
+        before do
+          allow(Puppet::Node.indirection).to receive(:find).and_call_original
+          allow(Puppet::Node.indirection).to receive(:find)
+            .with(anything, hash_including(:ignore_cache => true, :fail_on_404 => true))
+            .and_raise(Puppet::Error)
         end
 
-        it "uses environment from Puppet[:environment] if the last used one cannot be found" do
+        it "uses the environment from Puppet[:environment] if the last used one cannot be found" do
           Puppet[:lastrunfile] = tmpfile('last_run_summary.yaml')
           configurer.run