(FACT-2934)Set last exit code after Execution.exec

.rubocop.yml

@@ -66,6 +66,7 @@ Metrics/AbcSize:
     - 'install.rb'
     - 'scripts/generate_changelog.rb'
     - 'lib/facter/resolvers/aix/ffi/ffi_helper.rb'
+    - 'lib/facter/custom_facts/core/execution/popen3.rb'
 
 Metrics/PerceivedComplexity:
   Exclude:

acceptance/tests/custom_facts/execution_execute_sets_status_env_variable.rb

@@ -0,0 +1,37 @@
+test_name "(FACT-2934) Facter::Core::Execution sets $?" do
+  tag 'risk:high'
+
+  agents.each do |agent|
+    command = if agent['platform'] =~/windows/
+                'cmd /c exit 1'
+              else
+                `which false`.chomp
+              end
+
+    content = <<-EOM
+    Facter.add(:foo) do
+      setcode do
+         Facter::Util::Resolution.exec('#{command}')
+         "#{command} exited with code: %{status}" % {status: $?.exitstatus}
+      end
+    end
+    EOM
+
+
+    fact_dir = agent.tmpdir('facter')
+    fact_file = File.join(fact_dir, 'test_facts.rb')
+    create_remote_file(agent, fact_file, content)
+
+    teardown do
+      agent.rm_rf(fact_dir)
+    end
+
+    step "Facter: should resolve the custom fact" do
+      on(agent, facter('--custom-dir', fact_dir, 'foo')) do |facter_output|
+        assert_match(/exited with code: 1/, facter_output.stdout.chomp)
+      end
+    end
+  end
+end
+
+

lib/facter/custom_facts/core/execution/base.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require_relative 'popen3'
+
 module Facter
   module Core
     module Execution
@@ -70,8 +72,7 @@ def execute_command(command, on_fail = nil, logger = nil, time_limit = nil)
             opts = { 'LC_ALL' => 'C', 'LANG' => 'C' }
             require 'timeout'
             @log.debug("Executing command: #{command}")
-            out, stderr = Open3.popen3(opts, command.to_s) do |_, stdout, stderr, wait_thr|
-              pid = wait_thr.pid
+            out, stderr = Popen3.popen3e(opts, command.to_s) do |_, stdout, stderr, pid|
               stdout_messages = +''
               stderr_messages = +''
               out_reader = Thread.new { stdout.read }

lib/facter/custom_facts/core/execution/popen3.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+#
+# Because Open3 uses Process.detach the env $? is not set so
+# this class reimplements Open3.popen3 with Process.wait instead.
+# [FACT-2934] When calling Facter::Core::Execution, $? and $CHILD_STATUS
+# ruby env variables should be set.
+#
+
+module Facter
+  module Core
+    module Execution
+      class Popen3
+        def self.popen_rune(cmd, opts, child_io, parent_io) # :nodoc:
+          pid = spawn(*cmd, opts)
+          child_io.each(&:close)
+          result = [*parent_io, pid]
+          if defined? yield
+            begin
+              return yield(*result)
+            ensure
+              parent_io.each(&:close)
+              Process.wait(pid)
+            end
+          end
+          result
+        end
+
+        def self.popen3e(*cmd, &block)
+          opts = if cmd.last.is_a? Hash
+                   cmd.pop.dup
+                 else
+                   {}
+                 end
+          in_r, in_w = IO.pipe
+          opts[:in] = in_r
+          in_w.sync = true
+          out_r, out_w = IO.pipe
+          opts[:out] = out_w
+          err_r, err_w = IO.pipe
+          opts[:err] = err_w
+          popen_rune(cmd, opts, [in_r, out_w, err_w], [in_w, out_r, err_r], &block)
+        end
+      end
+    end
+  end
+end

spec/custom_facts/core/execution/fact_manager_spec.rb

@@ -84,7 +84,8 @@ def handy_method
       allow(FileTest).to receive(:file?).and_return(false)
       allow(File).to receive(:executable?).with('/sbin/foo').and_return(true)
       allow(FileTest).to receive(:file?).with('/sbin/foo').and_return(true)
-      expect(Open3).to receive(:popen3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return('')
+      expect(Facter::Core::Execution::Popen3).to receive(:popen3e).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo')
+                                                                  .and_return('')
       executor.execute('foo')
     end
 
@@ -100,7 +101,8 @@ def handy_method
       end
 
       it 'does not expant builtin command' do
-        allow(Open3).to receive(:popen3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/bin/foo').and_return('')
+        allow(Facter::Core::Execution::Popen3).to receive(:popen3e).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/bin/foo')
+                                                                   .and_return('')
         allow(Open3).to receive(:capture2).with('type /bin/foo').and_return('builtin')
         executor.execute('/bin/foo', expand: false)
       end
@@ -118,7 +120,8 @@ def handy_method
       end
 
       it 'throws exception' do
-        allow(Open3).to receive(:popen3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'foo').and_return('')
+        allow(Facter::Core::Execution::Popen3).to receive(:popen3e).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'foo')
+                                                                   .and_return('')
         allow(Open3).to receive(:capture2).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'type foo').and_return('builtin')
         expect { execution_base.execute('foo', expand: false) }
           .to raise_error(ArgumentError,
@@ -131,8 +134,8 @@ def handy_method
       let(:command) { '/bin/foo' }
 
       before do
-        allow(Open3).to receive(:popen3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, command)
-                                        .and_return(['', 'some error'])
+        allow(Facter::Core::Execution::Popen3).to receive(:popen3e).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, command)
+                                                                   .and_return(['', 'some error'])
         allow(Facter::Log).to receive(:new).with(executor).and_return(logger)
         allow(Facter::Log).to receive(:new).with('foo').and_return(logger)
 
@@ -164,7 +167,8 @@ def handy_method
 
     describe 'when command execution fails' do
       before do
-        allow(Open3).to receive(:popen3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/bin/foo').and_raise('kaboom!')
+        allow(Facter::Core::Execution::Popen3).to receive(:popen3e).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/bin/foo')
+                                                                   .and_raise('kaboom!')
         allow(File).to receive(:executable?).and_return(false)
         allow(FileTest).to receive(:file?).and_return(false)
         allow(File).to receive(:executable?).with('/bin/foo').and_return(true)
@@ -189,13 +193,17 @@ def handy_method
       end
 
       it 'returns the output of the command' do
-        allow(Open3).to receive(:popen3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return('hi')
+        allow(Facter::Core::Execution::Popen3).to receive(:popen3e)
+          .with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo')
+          .and_return('hi')
 
         expect(executor.execute('foo')).to eq 'hi'
       end
 
       it 'strips off trailing newlines' do
-        allow(Open3).to receive(:popen3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return "hi\n"
+        allow(Facter::Core::Execution::Popen3).to receive(:popen3e)
+          .with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo')
+          .and_return "hi\n"
 
         expect(executor.execute('foo')).to eq 'hi'
       end

spec/facter/resolvers/lsb_release_spec.rb

@@ -7,10 +7,10 @@
 
   context 'when system is ubuntu' do
     before do
-      allow(Open3).to receive(:popen3)
+      allow(Facter::Core::Execution::Popen3).to receive(:popen3e)
         .with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, 'which lsb_release')
         .and_return(['/usr/bin/lsb_release', '', 0])
-      allow(Open3).to receive(:popen3)
+      allow(Facter::Core::Execution::Popen3).to receive(:popen3e)
         .with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, 'lsb_release -a')
         .and_return(["Distributor ID:\tUbuntu\nDescription:\tUbuntu 18.04.1 LTS\nRelease:\t18.04\nCodename:\tbionic\n",
                      '', 0])
@@ -43,10 +43,10 @@
 
   context 'when system is centos' do
     before do
-      allow(Open3).to receive(:popen3)
+      allow(Facter::Core::Execution::Popen3).to receive(:popen3e)
         .with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, 'which lsb_release')
         .and_return(['/usr/bin/lsb_release', '', 0])
-      allow(Open3).to receive(:popen3)
+      allow(Facter::Core::Execution::Popen3).to receive(:popen3e)
         .with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, 'lsb_release -a')
         .and_return([load_fixture('centos_lsb_release').read, '', 0])
     end
@@ -84,7 +84,7 @@
 
   context 'when lsb_release is not installed on system' do
     before do
-      allow(Open3).to receive(:popen3)
+      allow(Facter::Core::Execution::Popen3).to receive(:popen3e)
         .with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, 'which lsb_release')
         .and_return(['', 'no lsb_release in (PATH:usr/sbin)', 1])
     end