Handle inventory files that are executable but not runnable

src/ansiblecmdb/ansible.py

@@ -104,7 +104,15 @@ def _handle_inventory(self, inventory_path):
                     inventory_path
                 )
             )
-            self._parse_dyn_inventory(inventory_path)
+            if not self._parse_dyn_inventory(inventory_path):
+                # It has the executable bit set but isn't actually runnable.
+                # Most likely a normal inventory file that ended up with the
+                # wrong permissions, so read it as one.
+                self.log.debug(
+                    "{} is not executable in practice. Handle as static "
+                    "inventory file".format(inventory_path)
+                )
+                self._parse_hosts_inventory(inventory_path)
         elif os.path.isfile(inventory_path):
             # Static inventory hosts file
             self.log.debug(
@@ -346,6 +354,11 @@ def _parse_fact_dir(self, fact_dir, fact_cache=False):
     def _parse_dyn_inventory(self, script):
         """
         Execute a dynamic inventory script and parse the results.
+
+        Returns False if the file could not be executed at all, so the caller
+        can fall back to reading it as a static inventory file. Returns True
+        in every other case, including when the script itself failed -- its
+        output should not be reinterpreted as an ini inventory.
         """
         self.log.debug("Reading dynamic inventory {0}".format(script))
         try:
@@ -355,25 +368,29 @@ def _parse_dyn_inventory(self, script):
                 stderr=subprocess.PIPE,
                 close_fds=True,
             )
-            stdout, stderr = proc.communicate(input)
-            if proc.returncode != 0:
-                sys.stderr.write(
-                    "Dynamic inventory script '{0}' returned "
-                    "exitcode {1}\n".format(script, proc.returncode)
-                )
-                for line in stderr:
-                    sys.stderr.write(line)
-
-            dyninv_parser = parser.DynInvParser(stdout.decode("utf8"))
-            for hostname, key_values in dyninv_parser.hosts.items():
-                self.update_host(hostname, key_values)
         except OSError as err:
+            # The executable bit is set, but the OS can't run it (ENOEXEC for
+            # a plain text inventory that was chmod +x'ed, ENOENT for a bad
+            # shebang, ...). Let the caller retry it as a static inventory.
+            self.log.debug(
+                "Could not execute '{0}' ({1}). Not a dynamic "
+                "inventory script.".format(script, err)
+            )
+            return False
+
+        stdout, stderr = proc.communicate()
+        if proc.returncode != 0:
             sys.stderr.write(
-                "Exception while executing dynamic inventory script '{0}':\n\n".format(
-                    script
-                )
+                "Dynamic inventory script '{0}' returned "
+                "exitcode {1}\n".format(script, proc.returncode)
             )
-            sys.stderr.write(str(err) + "\n")
+            sys.stderr.write(stderr.decode("utf8", errors="replace"))
+            return True
+
+        dyninv_parser = parser.DynInvParser(stdout.decode("utf8"))
+        for hostname, key_values in dyninv_parser.hosts.items():
+            self.update_host(hostname, key_values)
+        return True
 
     def update_host(self, hostname, key_values, overwrite=True):
         """

test/f_inventory/dyninv_failing.py

@@ -0,0 +1,4 @@
+#!/bin/sh
+# A dynamic inventory script that fails, e.g. missing credentials.
+echo "dyninv: something went wrong" >&2
+exit 1

test/f_inventory/hosts_executable

@@ -0,0 +1,5 @@
+# A normal ini inventory that has wrongly ended up with the executable
+# bit set (e.g. copied from a FAT/NTFS filesystem, or a stray chmod +x).
+[execfallback]
+execfallback01.dev.local dtap=dev
+execfallback02.dev.local

test/test.py

@@ -1,6 +1,7 @@
 import sys
 import unittest
 import importlib.util
+import io
 import sys
 import os
 
@@ -120,6 +121,43 @@ def testMixedDir(self):
         # INI file ignored.
         self.assertNotIn("ini_setting", ansible.hosts)
 
+    def testExecutableStaticInventory(self):
+        """
+        Verify that an inventory file which has the executable bit set but
+        isn't actually runnable falls back to being read as a static
+        inventory, instead of being silently dropped.
+        """
+        fact_dirs = ["f_inventory/out"]
+        inventories = ["f_inventory/hosts_executable"]
+        ansible = ansiblecmdb.Ansible(fact_dirs, inventories)
+        self.assertIn("execfallback01.dev.local", ansible.hosts)
+        self.assertIn("execfallback02.dev.local", ansible.hosts)
+        host = ansible.hosts["execfallback01.dev.local"]
+        self.assertEqual(host["hostvars"]["dtap"], "dev")
+        self.assertIn("execfallback", host["groups"])
+
+    def testFailingDynInventory(self):
+        """
+        Verify that a dynamic inventory script which exits non-zero is
+        reported rather than raising, and that its output is not reinterpreted
+        as a static inventory.
+        """
+        fact_dirs = ["f_inventory/out"]
+        inventories = ["f_inventory/dyninv_failing.py"]
+        stderr = sys.stderr
+        sys.stderr = io.StringIO()
+        try:
+            ansible = ansiblecmdb.Ansible(fact_dirs, inventories)
+            captured = sys.stderr.getvalue()
+        finally:
+            sys.stderr = stderr
+
+        # The script's own error message must reach the user.
+        self.assertIn("dyninv: something went wrong", captured)
+        self.assertIn("exitcode 1", captured)
+        # Nothing from the script should have been parsed as an inventory.
+        self.assertNotIn("dyninv", ansible.hosts)
+
 
 class FactCacheTestCase(unittest.TestCase):
     """