Nonfatal facts (#73804)

 continue with local facts vs at script error
 actually capture execution errors
 better error messages in general
 add more local facts tests

 fixes #52427

7 files changed, 68 insertions, 41 deletions

diff --git a/changelogs/fragments/local_facts_continue.yml b/changelogs/fragments/local_facts_continue.yml
new file mode 100644
index 0000000000..eb9b5898f7
--- /dev/null
+++ b/changelogs/fragments/local_facts_continue.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - setup, don't give up on all local facts gathering if one script file fails.
diff --git a/lib/ansible/module_utils/facts/system/local.py b/lib/ansible/module_utils/facts/system/local.py
index fe33a3232f..e80fca9eac 100644
--- a/lib/ansible/module_utils/facts/system/local.py
+++ b/lib/ansible/module_utils/facts/system/local.py
@@ -21,12 +21,10 @@ import json
 import os
 import stat
 
-from ansible.module_utils.six.moves import configparser
-from ansible.module_utils.six.moves import StringIO
-
+from ansible.module_utils._text import to_text
 from ansible.module_utils.facts.utils import get_file_content
-
 from ansible.module_utils.facts.collector import BaseFactCollector
+from ansible.module_utils.six.moves import configparser, StringIO
 
 
 class LocalFactCollector(BaseFactCollector):
@@ -46,36 +44,47 @@ class LocalFactCollector(BaseFactCollector):
             return local_facts
 
         local = {}
+        # go over .fact files, run executables, read rest, skip bad with warning and note
         for fn in sorted(glob.glob(fact_path + '/*.fact')):
-            # where it will sit under local facts
+            # use filename for key where it will sit under local facts
             fact_base = os.path.basename(fn).replace('.fact', '')
             if stat.S_IXUSR & os.stat(fn)[stat.ST_MODE]:
-                # run it
-                # try to read it as json first
-                # if that fails read it with ConfigParser
-                # if that fails, skip it
+                failed = None
                 try:
+                    # run it
                     rc, out, err = module.run_command(fn)
-                except UnicodeError:
-                    fact = 'error loading fact - output of running %s was not utf-8' % fn
-                    local[fact_base] = fact
-                    local_facts['local'] = local
-                    module.warn(fact)
-                    return local_facts
+                    if rc != 0:
+                        failed = 'Failure executing fact script (%s), rc: %s, err: %s' % (fn, rc, err)
+                except (IOError, OSError) as e:
+                    failed = 'Could not execute fact script (%s): %s' % (fn, to_text(e))
+
+                if failed is not None:
+                    local[fact_base] = failed
+                    module.warn(failed)
+                    continue
             else:
+                # ignores exceptions and returns empty
                 out = get_file_content(fn, default='')
 
-            # load raw json
-            fact = 'loading %s' % fact_base
+            try:
+                # ensure we have unicode
+                out = to_text(out, errors='surrogate_or_strict')
+            except UnicodeError:
+                fact = 'error loading fact - output of running "%s" was not utf-8' % fn
+                local[fact_base] = fact
+                module.warn(fact)
+                continue
+
+            # try to read it as json first
             try:
                 fact = json.loads(out)
             except ValueError:
-                # load raw ini
+                # if that fails read it with ConfigParser
                 cp = configparser.ConfigParser()
                 try:
                     cp.readfp(StringIO(out))
                 except configparser.Error:
-                    fact = "error loading fact - please check content"
+                    fact = "error loading facts as JSON or ini - please check content: %s" % fn
                     module.warn(fact)
                 else:
                     fact = {}
@@ -85,6 +94,9 @@ class LocalFactCollector(BaseFactCollector):
                         for opt in cp.options(sect):
                             val = cp.get(sect, opt)
                             fact[sect][opt] = val
+            except Exception as e:
+                fact = "Failed to convert (%s) to JSON: %s" % (fn, to_text(e))
+                module.warn(fact)
 
             local[fact_base] = fact
 
diff --git a/test/integration/targets/facts_d/files/basdscript.fact b/test/integration/targets/facts_d/files/basdscript.fact
new file mode 100644
index 0000000000..2bb8d868bd
--- /dev/null
+++ b/test/integration/targets/facts_d/files/basdscript.fact
@@ -0,0 +1,3 @@
+#!/bin/sh
+
+exit 1
diff --git a/test/integration/targets/facts_d/files/goodscript.fact b/test/integration/targets/facts_d/files/goodscript.fact
new file mode 100644
index 0000000000..6ee866cfb6
--- /dev/null
+++ b/test/integration/targets/facts_d/files/goodscript.fact
@@ -0,0 +1,3 @@
+#!/bin/sh
+
+echo '{"script_ran": true}'
diff --git a/test/integration/targets/facts_d/files/preferences.fact b/test/integration/targets/facts_d/files/preferences.fact
new file mode 100644
index 0000000000..c32583d4b2
--- /dev/null
+++ b/test/integration/targets/facts_d/files/preferences.fact
@@ -0,0 +1,2 @@
+[general]
+bar=loaded
diff --git a/test/integration/targets/facts_d/files/unreadable.fact b/test/integration/targets/facts_d/files/unreadable.fact
new file mode 100644
index 0000000000..98f562be6b
--- /dev/null
+++ b/test/integration/targets/facts_d/files/unreadable.fact
@@ -0,0 +1 @@
+wontbeseen=ever
diff --git a/test/integration/targets/facts_d/tasks/main.yml b/test/integration/targets/facts_d/tasks/main.yml
index ca23544fbe..aadef4c693 100644
--- a/test/integration/targets/facts_d/tasks/main.yml
+++ b/test/integration/targets/facts_d/tasks/main.yml
@@ -1,35 +1,36 @@
-# Test code for facts.d and setup filters
 # (c) 2014, James Tanner <tanner.jc@gmail.com>
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
-# This file is part of Ansible
-#
-# Ansible is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# Ansible is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+- name: prep for local facts tests
+  block:
+    - name: set factdir var
+      set_fact: fact_dir={{output_dir}}/facts.d
 
-- set_fact: fact_dir={{output_dir}}/facts.d
+    - name: create fact dir
+      file: path={{ fact_dir }} state=directory
 
-- file: path={{ fact_dir }} state=directory
-- shell: echo "[general]" > {{ fact_dir }}/preferences.fact
-- shell: echo "bar=loaded" >> {{ fact_dir }}/preferences.fact
+    - name: copy local facts test files
+      copy: src={{ item['name'] }}.fact dest={{ fact_dir }}/ mode={{ item['mode']|default(omit) }}
+      loop:
+        - name: preferences
+        - name: basdscript
+          mode: '0775'
+        - name: goodscript
+          mode: '0775'
+        - name: unreadable
+          mode: '0000'
 
-- setup:
+- name: force fact gather to get ansible_local
+  setup:
     fact_path: "{{ fact_dir | expanduser }}"
     filter: "*local*"
   register: setup_result
 
-- debug: var=setup_result
+- name: show gathering results if rerun with -vvv
+  debug: var=setup_result verbosity=3
 
-- assert:
+- name: check for expected results from local facts
+  assert:
     that:
         - "'ansible_facts' in setup_result"
         - "'ansible_local' in setup_result.ansible_facts"
@@ -39,3 +40,6 @@
         - "'general' in setup_result.ansible_facts['ansible_local']['preferences']"
         - "'bar' in setup_result.ansible_facts['ansible_local']['preferences']['general']"
         - "setup_result.ansible_facts['ansible_local']['preferences']['general']['bar'] == 'loaded'"
+        - setup_result['ansible_facts']['ansible_local']['goodscript']['script_ran']|bool
+        - setup_result['ansible_facts']['ansible_local']['basdscript'].startswith("Failure executing fact script")
+        - setup_result['ansible_facts']['ansible_local']['unreadable'].startswith('error loading facts')