(FACT-2793) Time limit for Facter::Core::Execute

.rubocop_todo.yml

@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config --exclude-limit 1000`
-# on 2020-08-26 01:26:00 +0300 using RuboCop version 0.81.0.
+# on 2020-09-22 15:44:45 +0300 using RuboCop version 0.81.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new

acceptance/tests/custom_facts/time_limit_for_execute_command.rb

@@ -0,0 +1,47 @@
+test_name 'Facter::Core::Execution accepts and correctly sets a time limit option' do
+  tag 'risk:high'
+
+  first_file_content = <<-EOM
+    Facter.add(:foo) do
+      setcode do
+        Facter::Core::Execution.execute("sleep 3", {:limit => 2})
+      end
+    end
+  EOM
+
+  second_file_content = <<-EOM
+    Facter.add(:custom_fact) do
+      setcode do
+         Facter::Core::Execution.execute("sleep 2")
+      end
+    end
+  EOM
+
+
+  agents.each do |agent|
+
+    custom_dir = agent.tmpdir('arbitrary_dir')
+    fact_file1 = File.join(custom_dir, 'file1.rb')
+    fact_file2 = File.join(custom_dir, 'file2.rb')
+    create_remote_file(agent, fact_file1, first_file_content)
+    create_remote_file(agent, fact_file2, second_file_content)
+
+    teardown do
+      agent.rm_rf(custom_dir)
+    end
+
+    step "Facter: Logs that command of the first custom fact had timeout after setted time limit" do
+      on agent, facter('--custom-dir', custom_dir, 'foo --debug') do |facter_output|
+        assert_match(/DEBUG Facter::Core::Execution.* - Timeout encounter after 2s, killing process with pid:/,
+                     facter_output.stderr.chomp)
+      end
+    end
+
+    step "Facter: Logs that command of the second custom fact had timeout after befault time limit" do
+      on agent, facter('--custom-dir', custom_dir, 'custom_fact --debug') do |facter_output|
+        assert_match(/DEBUG Facter::Core::Execution.* - Timeout encounter after 1.5s, killing process with pid:/,
+                     facter_output.stderr.chomp)
+      end
+    end
+  end
+end

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

@@ -6,6 +6,10 @@ module Execution
       class Base
         STDERR_MESSAGE = 'Command %s resulted with the following stderr message: %s'
 
+        def initialize
+          @log = Log.new(self)
+        end
+
         # This is part of the public API. No race condition can happen
         # here because custom facts are executed sequentially
         def with_env(values)
@@ -36,9 +40,7 @@ def with_env(values)
         end
 
         def execute(command, options = {})
-          on_fail = options.fetch(:on_fail, :raise)
-          expand = options.fetch(:expand, true)
-          logger = options[:logger]
+          on_fail, expand, logger, time_limit = extract_options(options)
 
           expanded_command = if !expand && builtin_command?(command) || logger
                                command
@@ -55,11 +57,21 @@ def execute(command, options = {})
             return on_fail
           end
 
-          execute_command(expanded_command, on_fail, logger)
+          execute_command(expanded_command, on_fail, logger, time_limit)
         end
 
         private
 
+        def extract_options(options)
+          on_fail = options.fetch(:on_fail, :raise)
+          expand = options.fetch(:expand, true)
+          logger = options[:logger]
+          time_limit = options[:limit].to_i
+          time_limit = time_limit.positive? ? time_limit : nil
+
+          [on_fail, expand, logger, time_limit]
+        end
+
         def log_stderr(msg, command, logger)
           return if !msg || msg.empty?
 
@@ -77,12 +89,30 @@ def builtin_command?(command)
           output.chomp =~ /builtin/ ? true : false
         end
 
-        def execute_command(command, on_fail, logger = nil)
+        def execute_command(command, on_fail, logger = nil, time_limit = nil)
+          time_limit ||= 1.5
           begin
             # Set LC_ALL and LANG to force i18n to C for the duration of this exec;
             # this ensures that any code that parses the
             # output of the command can expect it to be in a consistent / predictable format / locale
-            out, stderr, _status_ = Open3.capture3({ 'LC_ALL' => 'C', 'LANG' => 'C' }, command.to_s)
+            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
+              output = +''
+              err = +''
+              begin
+                Timeout.timeout(time_limit) do
+                  output << stdout.read
+                  err << stderr.read
+                end
+              rescue Timeout::Error
+                @log.debug("Timeout encounter after #{time_limit}s, killing process with pid: #{pid}")
+                Process.kill('KILL', pid)
+              end
+              [output, err]
+            end
             log_stderr(stderr, command, logger)
           rescue StandardError => e
             return '' if logger

lib/facter/facts/aix/kernel.rb

@@ -6,7 +6,7 @@ class Kernel
       FACT_NAME = 'kernel'
 
       def call_the_resolver
-        fact_value = Facter::Resolvers::OsLevel.resolve(:kernel)
+        fact_value = Facter::Resolvers::Aix::OsLevel.resolve(:kernel)
 
         Facter::ResolvedFact.new(FACT_NAME, fact_value)
       end

lib/facter/facts/aix/kernelmajversion.rb

@@ -6,7 +6,7 @@ class Kernelmajversion
       FACT_NAME = 'kernelmajversion'
 
       def call_the_resolver
-        fact_value = Facter::Resolvers::OsLevel.resolve(:build)
+        fact_value = Facter::Resolvers::Aix::OsLevel.resolve(:build)
         kernelmajversion = fact_value.split('-')[0]
 
         Facter::ResolvedFact.new(FACT_NAME, kernelmajversion)

lib/facter/facts/aix/kernelrelease.rb

@@ -6,7 +6,7 @@ class Kernelrelease
       FACT_NAME = 'kernelrelease'
 
       def call_the_resolver
-        fact_value = Facter::Resolvers::OsLevel.resolve(:build).strip
+        fact_value = Facter::Resolvers::Aix::OsLevel.resolve(:build).strip
 
         Facter::ResolvedFact.new(FACT_NAME, fact_value)
       end

lib/facter/facts/aix/kernelversion.rb

@@ -6,7 +6,7 @@ class Kernelversion
       FACT_NAME = 'kernelversion'
 
       def call_the_resolver
-        fact_value = Facter::Resolvers::OsLevel.resolve(:build)
+        fact_value = Facter::Resolvers::Aix::OsLevel.resolve(:build)
         kernelversion = fact_value.split('-')[0]
 
         Facter::ResolvedFact.new(FACT_NAME, kernelversion)

lib/facter/facts/aix/os/release.rb

@@ -8,7 +8,7 @@ class Release
         ALIASES = %w[operatingsystemmajrelease operatingsystemrelease].freeze
 
         def call_the_resolver
-          fact_value = Facter::Resolvers::OsLevel.resolve(:build)
+          fact_value = Facter::Resolvers::Aix::OsLevel.resolve(:build)
           major = fact_value.split('-')[0]
 
           [Facter::ResolvedFact.new(FACT_NAME, full: fact_value.strip, major: major),

lib/facter/resolvers/aix/os_level.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module Facter
+  module Resolvers
+    module Aix
+      class OsLevel < BaseResolver
+        @fact_list ||= {}
+
+        class << self
+          private
+
+          def post_resolve(fact_name)
+            @fact_list.fetch(fact_name) { read_oslevel(fact_name) }
+          end
+
+          def read_oslevel(fact_name)
+            output = Facter::Core::Execution.execute('/usr/bin/oslevel -s', { limit: 2, logger: log })
+            @fact_list[:build] = output unless output.empty?
+            @fact_list[:kernel] = 'AIX'
+
+            @fact_list[fact_name]
+          end
+        end
+      end
+    end
+  end
+end

spec/custom_facts/core/execution/fact_manager_spec.rb

@@ -84,7 +84,7 @@ 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(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return('')
+      expect(Open3).to receive(:popen3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return('')
       executor.execute('foo')
     end
 
@@ -100,7 +100,7 @@ def handy_method
       end
 
       it 'does not expant builtin command' do
-        allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/bin/foo').and_return('')
+        allow(Open3).to receive(:popen3).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 +118,7 @@ def handy_method
       end
 
       it 'throws exception' do
-        allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, 'foo').and_return('')
+        allow(Open3).to receive(:popen3).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 +131,9 @@ def handy_method
       let(:command) { '/bin/foo' }
 
       before do
-        allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, command)
-                                          .and_return(['', 'some error'])
+        allow(Open3).to receive(:popen3).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)
 
         allow(File).to receive(:executable?).with(command).and_return(true)
@@ -163,7 +164,7 @@ def handy_method
 
     describe 'when command execution fails' do
       before do
-        allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/bin/foo').and_raise('kaboom!')
+        allow(Open3).to receive(:popen3).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)
@@ -188,13 +189,13 @@ def handy_method
       end
 
       it 'returns the output of the command' do
-        allow(Open3).to receive(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return('hi')
+        allow(Open3).to receive(:popen3).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(:capture3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return "hi\n"
+        allow(Open3).to receive(:popen3).with({ 'LC_ALL' => 'C', 'LANG' => 'C' }, '/sbin/foo').and_return "hi\n"
 
         expect(executor.execute('foo')).to eq 'hi'
       end

spec/facter/facts/aix/kernel_spec.rb

@@ -7,12 +7,12 @@
     let(:value) { 'AIX' }
 
     before do
-      allow(Facter::Resolvers::OsLevel).to receive(:resolve).with(:kernel).and_return(value)
+      allow(Facter::Resolvers::Aix::OsLevel).to receive(:resolve).with(:kernel).and_return(value)
     end
 
     it 'calls Facter::Resolvers::OsLevel' do
       fact.call_the_resolver
-      expect(Facter::Resolvers::OsLevel).to have_received(:resolve).with(:kernel)
+      expect(Facter::Resolvers::Aix::OsLevel).to have_received(:resolve).with(:kernel)
     end
 
     it 'returns kernel fact' do

spec/facter/facts/aix/kernelmajversion_spec.rb

@@ -8,12 +8,12 @@
     let(:resolver_value) { '6100-09-00-0000' }
 
     before do
-      allow(Facter::Resolvers::OsLevel).to receive(:resolve).with(:build).and_return(resolver_value)
+      allow(Facter::Resolvers::Aix::OsLevel).to receive(:resolve).with(:build).and_return(resolver_value)
     end
 
     it 'calls Facter::Resolvers::OsLevel' do
       fact.call_the_resolver
-      expect(Facter::Resolvers::OsLevel).to have_received(:resolve).with(:build)
+      expect(Facter::Resolvers::Aix::OsLevel).to have_received(:resolve).with(:build)
     end
 
     it 'returns kernelmajversion fact' do

spec/facter/facts/aix/kernelrelease_spec.rb

@@ -7,12 +7,12 @@
     let(:value) { '6100-09-00-0000' }
 
     before do
-      allow(Facter::Resolvers::OsLevel).to receive(:resolve).with(:build).and_return(value)
+      allow(Facter::Resolvers::Aix::OsLevel).to receive(:resolve).with(:build).and_return(value)
     end
 
     it 'calls Facter::Resolvers::OsLevel' do
       fact.call_the_resolver
-      expect(Facter::Resolvers::OsLevel).to have_received(:resolve).with(:build)
+      expect(Facter::Resolvers::Aix::OsLevel).to have_received(:resolve).with(:build)
     end
 
     it 'returns kernelrelease fact' do

spec/facter/facts/aix/kernelversion_spec.rb

@@ -8,12 +8,12 @@
     let(:fact_value) { '6100' }
 
     before do
-      allow(Facter::Resolvers::OsLevel).to receive(:resolve).with(:build).and_return(resolver_value)
+      allow(Facter::Resolvers::Aix::OsLevel).to receive(:resolve).with(:build).and_return(resolver_value)
     end
 
     it 'calls Facter::Resolvers::OsLevel' do
       fact.call_the_resolver
-      expect(Facter::Resolvers::OsLevel).to have_received(:resolve).with(:build)
+      expect(Facter::Resolvers::Aix::OsLevel).to have_received(:resolve).with(:build)
     end
 
     it 'returns kernelversion fact' do

spec/facter/facts/aix/os/release_spec.rb

@@ -7,11 +7,11 @@
     let(:value) { '12.0.1 ' }
 
     before do
-      allow(Facter::Resolvers::OsLevel).to receive(:resolve).with(:build).and_return(value)
+      allow(Facter::Resolvers::Aix::OsLevel).to receive(:resolve).with(:build).and_return(value)
     end
 
     it 'calls Facter::Resolvers::OsLevel' do
-      expect(Facter::Resolvers::OsLevel).to receive(:resolve).with(:build)
+      expect(Facter::Resolvers::Aix::OsLevel).to receive(:resolve).with(:build)
       fact.call_the_resolver
     end