Revert "(PUP-10216) Use last environment instead of requesting the node's environment"

Author: GabrielNagy

lib/puppet/configurer.rb

@@ -91,7 +91,7 @@ def retrieve_catalog(facts, query_options)
 
         if result
           # don't use use cached catalog if it doesn't match server specified environment
-          if result.environment != @environment
+          if @node_environment && result.environment != @environment
             Puppet.err _("Not using cached catalog because its environment '%{catalog_env}' does not match '%{local_env}'") % { catalog_env: result.environment, local_env: @environment }
             return nil
           end
@@ -296,11 +296,50 @@ 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 (env = last_used_environment)
-          @environment = env
-          report.environment = env
+      unless (cached_catalog || options[:catalog] || Puppet[:strict_environment_mode])
+        begin
+          node = nil
+          node_retr_time = thinmark do
+            node = Puppet::Node.indirection.find(Puppet[:node_name_value],
+              :environment => Puppet::Node::Environment.remote(@environment),
+              :configured_environment => configured_environment,
+              :ignore_cache => true,
+              :transaction_uuid => @transaction_uuid,
+              :fail_on_404 => true)
+          end
+          options[:report].add_times(:node_retrieval, node_retr_time)
+
+          if node
+            # If we have deserialized a node from a rest call, we want to set
+            # an environment instance as a simple 'remote' environment reference.
+            if !node.has_environment_instance? && node.environment_name
+              node.environment = Puppet::Node::Environment.remote(node.environment_name)
+            end
+
+            @node_environment = node.environment.to_s
+
+            if node.environment.to_s != @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: node.environment }
+              @environment = node.environment.to_s
+              report.environment = @environment
+              query_options = nil
+              facts = nil
+
+              new_env = Puppet::Node::Environment.remote(@environment)
+              Puppet.push_context(
+                {
+                  current_environment: new_env,
+                  loaders: Puppet::Pops::Loaders.new(new_env, true)
+                },
+                "Local node environment #{@environment} for configurer transaction"
+              )
+            else
+              Puppet.info _("Using configured environment '%{env}'") % { env: @environment }
+            end
+          end
+        rescue StandardError => detail
+          Puppet.warning(_("Unable to fetch my node definition, but the agent run will continue:"))
+          Puppet.warning(detail)
         end
       end
 
@@ -319,6 +358,7 @@ def run_internal(options)
 
       query_options, facts = get_facts(options) unless query_options
       query_options[:configured_environment] = configured_environment
+      options[:convert_for_node] = node
 
       catalog = prepare_and_retrieve_catalog(cached_catalog, facts, options, query_options)
       unless catalog
@@ -432,22 +472,6 @@ def find_functional_server
   end
   private :find_functional_server
 
-  def last_used_environment
-    if Puppet::FileSystem.exist?(Puppet[:lastrunfile])
-      last_run_environment = Puppet::Util::Yaml.safe_load_file(Puppet[:lastrunfile]).dig('version', 'environment') rescue nil
-    end
-
-    if !last_run_environment && Puppet::FileSystem.exist?(Puppet[:lastrunreport])
-      last_run_environment = Puppet::Util::Yaml.safe_load_file(Puppet[:lastrunreport], [Puppet::Transaction::Report]).environment
-    end
-    Puppet.debug(_("Found last used environment: %{environment}") % { environment: last_run_environment }) if last_run_environment
-    last_run_environment
-  rescue => detail
-    Puppet.debug(_("Unable to get last used environment: %{detail}") % { detail: detail })
-    nil
-  end
-  private :last_used_environment
-
   def send_report(report)
     puts report.summary if Puppet[:summarize]
     save_last_run_summary(report)

lib/puppet/transaction/report.rb

@@ -377,13 +377,7 @@ def summary
   # @api public
   #
   def raw_summary
-    report = {
-      "version" => {
-        "config" => configuration_version,
-        "environment" => environment,
-        "puppet" => Puppet.version
-      }
-    }
+    report = { "version" => { "config" => configuration_version, "puppet" => Puppet.version  } }
 
     @metrics.each do |name, metric|
       key = metric.name.to_s

spec/fixtures/unit/configurer/last_run_report.yaml

@@ -572,7 +572,7 @@
        ).and matching(
          /Notify\[a message\]\/message:/
        )).to_stdout
-       .and output(/Could not retrieve catalog from remote server/).to_stderr
+       .and output(/the agent run will continue/).to_stderr
     end
 
     it 'preserves the old cached catalog if validation fails with the old one' do
@@ -591,7 +591,7 @@
           agent.command_line.args << '--test'
           agent.run
         }.to exit_with(1)
-         .and output(/Retrieving plugin/).to_stdout
+         .and output(/Using configured environment/).to_stdout
          .and output(%r{Validation of Exec\[unqualified_command\] failed: 'unqualified_command' is not qualified and no path was specified}).to_stderr
       end
 

spec/integration/application/agent_spec.rb

@@ -78,6 +78,12 @@
       configurer.run(:pluginsync => false)
     end
 
+    it "should carry on when it can't fetch its node definition" do
+      error = Net::HTTPError.new(400, 'dummy server communication error')
+      expect(Puppet::Node.indirection).to receive(:find).and_raise(error)
+      expect(configurer.run).to eq(0)
+    end
+
     it "fails the run if pluginsync fails when usecacheonfailure is false" do
       Puppet[:ignore_plugin_errors] = false
 
@@ -119,6 +125,7 @@
     it "applies a cached catalog when it can't connect to the master" do
       error = Errno::ECONNREFUSED.new('Connection refused - connect(2)')
 
+      expect(Puppet::Node.indirection).to receive(:find).and_raise(error)
       expect(Puppet::Resource::Catalog.indirection).to receive(:find).with(anything, hash_including(:ignore_cache => true)).and_raise(error)
       expect(Puppet::Resource::Catalog.indirection).to receive(:find).with(anything, hash_including(:ignore_terminus => true)).and_return(catalog)
 
@@ -546,6 +553,24 @@ def method_missing(*args)
     end
   end
 
+  describe "when requesting a node" do
+    it "uses the transaction uuid in the request" do
+      expect(Puppet::Node.indirection).to receive(:find).with(anything, hash_including(transaction_uuid: anything)).twice
+      configurer.run
+    end
+
+    it "sends an explicitly configured environment request" do
+      expect(Puppet.settings).to receive(:set_by_config?).with(:environment).and_return(true)
+      expect(Puppet::Node.indirection).to receive(:find).with(anything, hash_including(configured_environment: Puppet[:environment])).twice
+      configurer.run
+    end
+
+    it "does not send a configured_environment when using the default" do
+      expect(Puppet::Node.indirection).to receive(:find).with(anything, hash_including(configured_environment: nil)).twice
+      configurer.run
+    end
+  end
+
   def expects_pluginsync
     metadata = "[{\"path\":\"/etc/puppetlabs/code\",\"relative_path\":\".\",\"links\":\"follow\",\"owner\":0,\"group\":0,\"mode\":420,\"checksum\":{\"type\":\"ctime\",\"value\":\"{ctime}2020-07-10 14:00:00 -0700\"},\"type\":\"directory\",\"destination\":null}]"
     stub_request(:get, %r{/puppet/v3/file_metadatas/(plugins|locales)}).to_return(status: 200, body: metadata, headers: {'Content-Type' => 'application/json'})
@@ -597,13 +622,21 @@ def expects_neither_new_or_cached_catalog
         configurer.run
       end
 
-      it "should not pluginsync when a cached catalog is successfully retrieved" do
+      it "should not make a node request or pluginsync when a cached catalog is successfully retrieved" do
+        expect(Puppet::Node.indirection).not_to receive(:find)
         expects_cached_catalog_only(catalog)
         expect(configurer).not_to receive(:download_plugins)
 
         configurer.run
       end
 
+      it "should make a node request and pluginsync when a cached catalog cannot be retrieved" do
+        expect(Puppet::Node.indirection).to receive(:find).and_return(nil)
+        expects_fallback_to_new_catalog(catalog)
+
+        configurer.run
+      end
+
       it "should set its cached_catalog_status to 'explicitly_requested'" do
         expects_cached_catalog_only(catalog)
 
@@ -635,6 +668,7 @@ def expects_neither_new_or_cached_catalog
       end
 
       it "should not attempt to retrieve a cached catalog again if the first attempt failed" do
+        expect(Puppet::Node.indirection).to receive(:find).and_return(nil)
         expects_neither_new_or_cached_catalog
         expects_pluginsync
 
@@ -690,6 +724,16 @@ def expects_neither_new_or_cached_catalog
         Puppet.settings[:strict_environment_mode] = true
       end
 
+      it "should not make a node request" do
+        stub_request(:get, %r{/puppet/v3/file_metadatas?/plugins}).to_return(:status => 404)
+        stub_request(:get, %r{/puppet/v3/file_metadatas?/pluginfacts}).to_return(:status => 404)
+        expects_new_catalog_only(catalog)
+
+        expect(Puppet::Node.indirection).not_to receive(:find)
+
+        configurer.run
+      end
+
       it "should return nil when the catalog's environment doesn't match the agent specified environment" do
         Puppet[:environment] = 'second_env'
         configurer = Puppet::Configurer.new
@@ -1063,105 +1107,20 @@ def expects_neither_new_or_cached_catalog
       expect(configurer.run(options)).to eq(0)
       expect(options[:report].server_used).to be_nil
     end
-  end
-
-  describe "when selecting an environment" do
-    include PuppetSpec::Files
-    include PuppetSpec::Settings
-
-    describe "when the last used environment is available" do
-      let(:last_used_environment) { 'development' }
-      let(:default_environment) { 'production' }
-
-      before do
-        Puppet[:lastrunreport] = my_fixture('last_run_report.yaml')
-        Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', <<~SUMMARY)
-        ---
-        version:
-          config: 1624882680
-          environment: development
-          puppet: 6.24.0
-        SUMMARY
-      end
 
-      it "prefers the environment set via cli" do
-        Puppet.settings.handlearg('--environment', 'usethis')
-        configurer.run
-
-        expect(configurer.environment).to eq('usethis')
-      end
-
-      it "prefers the environment set via config" do
-        FileUtils.mkdir_p(Puppet[:confdir])
-        set_puppet_conf(Puppet[:confdir], <<~CONF)
-        [main]
-        environment = usethis
-        CONF
-
-        Puppet.initialize_settings
-        configurer.run
-
-        expect(configurer.environment).to eq('usethis')
-      end
-
-      it "uses the default environment if given a catalog" do
-        configurer.run(catalog: catalog)
-
-        expect(configurer.environment).to eq(default_environment)
-      end
-
-      it "uses the default 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)
-      end
-
-      describe "when the environment is not configured" do
-        it "uses the environment found in lastrunfile if the key exists" do
-          configurer.run
-
-          expect(configurer.environment).to eq(last_used_environment)
-        end
-
-        it "falls back to the environment from lastrunreport if lastrunfile does not contain the environment" do
-          Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', <<~SUMMARY)
-          ---
-          version:
-            config: 1624882680
-            puppet: 6.24.0
-          SUMMARY
-          configurer.run
-
-          expect(configurer.environment).to eq(last_used_environment)
-        end
-
-        it "falls back to the environment from lastrunreport 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(last_used_environment)
-        end
+    it "should not make multiple node requests when the server is found" do
+      Puppet.settings[:server_list] = ["myserver:123"]
 
-        it "falls back to the environment from lastrunreport if lastrunfile exists but is empty" do
-          Puppet[:lastrunfile] = file_containing('last_run_summary.yaml', '')
-          configurer.run
+      Puppet::Node.indirection.terminus_class = :rest
+      Puppet::Resource::Catalog.indirection.terminus_class = :rest
 
-          expect(configurer.environment).to eq(last_used_environment)
-        end
+      stub_request(:get, 'https://myserver:123/status/v1/simple/master').to_return(status: 200)
+      stub_request(:post, %r{https://myserver:123/puppet/v3/catalog}).to_return(status: 200)
+      node_request = stub_request(:get, %r{https://myserver:123/puppet/v3/node/}).to_return(status: 200)
 
-        it "uses the default environment if the last used one cannot be found" do
-          Puppet[:lastrunfile] = tmpfile('last_run_summary.yaml')
-          Puppet[:lastrunreport] = tmpfile('last_run_report.yaml')
-          configurer.run
+      configurer.run
 
-          expect(configurer.environment).to eq(default_environment)
-        end
-      end
+      expect(node_request).to have_been_requested.once
     end
   end
 end