From 5112feeace0079266ce9bfa7ed226b4b6561496f Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Thu, 30 Jan 2020 12:54:25 -0500 Subject: [PATCH] Make get_bin_path() always raise an exception (#56813) This makes it behave in a more idiomatic way * Fix bug in Darwin facts for free memory If the vm_stat command is not found, fact gathering would fail with an unhelpful error message. Handle this gracefully and return a default value for free memory. * Add unit tests --- ...e-get_bin_path-always-raise-exception.yaml | 2 ++ lib/ansible/module_utils/basic.py | 7 ++-- lib/ansible/module_utils/common/process.py | 11 +++--- .../module_utils/facts/hardware/darwin.py | 13 ++++--- .../module_utils/facts/network/iscsi.py | 36 +++++++++++-------- lib/ansible/module_utils/facts/packages.py | 7 ++-- lib/ansible/modules/files/copy.py | 2 +- .../modules/packaging/os/package_facts.py | 16 +++++++-- lib/ansible/playbook/role/requirement.py | 2 +- lib/ansible/plugins/connection/chroot.py | 8 ++--- .../plugins/inventory/docker_machine.py | 4 +-- lib/ansible/plugins/inventory/nmap.py | 7 ++-- lib/ansible/plugins/inventory/virtualbox.py | 2 +- .../common/process/test_get_bin_path.py | 33 +++++++++++++++++ 14 files changed, 106 insertions(+), 44 deletions(-) create mode 100644 changelogs/fragments/change-get_bin_path-always-raise-exception.yaml create mode 100644 test/units/module_utils/common/process/test_get_bin_path.py diff --git a/changelogs/fragments/change-get_bin_path-always-raise-exception.yaml b/changelogs/fragments/change-get_bin_path-always-raise-exception.yaml new file mode 100644 index 00000000000..9835fa3aaa3 --- /dev/null +++ b/changelogs/fragments/change-get_bin_path-always-raise-exception.yaml @@ -0,0 +1,2 @@ +minor_changes: + - get_bin_path() - change the interface to always raise ``ValueError`` if the command is not found (https://github.com/ansible/ansible/pull/56813) diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index 94a1d3c08dc..e55ac189db4 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -1973,9 +1973,12 @@ class AnsibleModule(object): bin_path = None try: - bin_path = get_bin_path(arg, required, opt_dirs) + bin_path = get_bin_path(arg=arg, opt_dirs=opt_dirs) except ValueError as e: - self.fail_json(msg=to_text(e)) + if required: + self.fail_json(msg=to_text(e)) + else: + return bin_path return bin_path diff --git a/lib/ansible/module_utils/common/process.py b/lib/ansible/module_utils/common/process.py index 2c9b150b945..91e818a023f 100644 --- a/lib/ansible/module_utils/common/process.py +++ b/lib/ansible/module_utils/common/process.py @@ -9,13 +9,14 @@ import os from ansible.module_utils.common.file import is_executable -def get_bin_path(arg, required=False, opt_dirs=None): +def get_bin_path(arg, opt_dirs=None, required=None): ''' - find system executable in PATH. + Find system executable in PATH. Raises ValueError if executable is not found. Optional arguments: - - required: if executable is not found and required is true it produces an Exception + - required: [Deprecated] Prior to 2.10, if executable is not found and required is true it raises an Exception. + In 2.10 and later, an Exception is always raised. This parameter will be removed in 2.14. - opt_dirs: optional list of directories to search in addition to PATH - if found return full path; otherwise return None + If found return full path, otherwise raise ValueError. ''' opt_dirs = [] if opt_dirs is None else opt_dirs @@ -37,7 +38,7 @@ def get_bin_path(arg, required=False, opt_dirs=None): if os.path.exists(path) and not os.path.isdir(path) and is_executable(path): bin_path = path break - if required and bin_path is None: + if bin_path is None: raise ValueError('Failed to find required executable %s in paths: %s' % (arg, os.pathsep.join(paths))) return bin_path diff --git a/lib/ansible/module_utils/facts/hardware/darwin.py b/lib/ansible/module_utils/facts/hardware/darwin.py index bd7ebd488eb..f2cb6d68f0c 100644 --- a/lib/ansible/module_utils/facts/hardware/darwin.py +++ b/lib/ansible/module_utils/facts/hardware/darwin.py @@ -85,12 +85,17 @@ class DarwinHardware(Hardware): def get_memory_facts(self): memory_facts = { - 'memtotal_mb': int(self.sysctl['hw.memsize']) // 1024 // 1024 + 'memtotal_mb': int(self.sysctl['hw.memsize']) // 1024 // 1024, + 'memfree_mb': 0, } total_used = 0 page_size = 4096 - vm_stat_command = get_bin_path('vm_stat') + try: + vm_stat_command = get_bin_path('vm_stat') + except ValueError: + return memory_facts + rc, out, err = self.module.run_command(vm_stat_command) if rc == 0: # Free = Total - (Wired + active + inactive) @@ -104,7 +109,7 @@ class DarwinHardware(Hardware): for k, v in memory_stats.items(): try: memory_stats[k] = int(v) - except ValueError as ve: + except ValueError: # Most values convert cleanly to integer values but if the field does # not convert to an integer, just leave it alone. pass @@ -116,7 +121,7 @@ class DarwinHardware(Hardware): if memory_stats.get('Pages inactive'): total_used += memory_stats['Pages inactive'] * page_size - memory_facts['memfree_mb'] = memory_facts['memtotal_mb'] - (total_used // 1024 // 1024) + memory_facts['memfree_mb'] = memory_facts['memtotal_mb'] - (total_used // 1024 // 1024) return memory_facts diff --git a/lib/ansible/module_utils/facts/network/iscsi.py b/lib/ansible/module_utils/facts/network/iscsi.py index 98e62a2aa6d..33b2737bd57 100644 --- a/lib/ansible/module_utils/facts/network/iscsi.py +++ b/lib/ansible/module_utils/facts/network/iscsi.py @@ -80,22 +80,30 @@ class IscsiInitiatorNetworkCollector(NetworkCollector): iscsi_facts['iscsi_iqn'] = line.split('=', 1)[1] break elif sys.platform.startswith('aix'): - cmd = get_bin_path('lsattr') - if cmd: - cmd += " -E -l iscsi0" - rc, out, err = module.run_command(cmd) - if rc == 0 and out: - line = self.findstr(out, 'initiator_name') - iscsi_facts['iscsi_iqn'] = line.split()[1].rstrip() + try: + cmd = get_bin_path('lsattr') + except ValueError: + return iscsi_facts + + cmd += " -E -l iscsi0" + rc, out, err = module.run_command(cmd) + if rc == 0 and out: + line = self.findstr(out, 'initiator_name') + iscsi_facts['iscsi_iqn'] = line.split()[1].rstrip() + elif sys.platform.startswith('hp-ux'): # try to find it in the default PATH and opt_dirs - cmd = get_bin_path('iscsiutil', opt_dirs=['/opt/iscsi/bin']) - if cmd: - cmd += " -l" - rc, out, err = module.run_command(cmd) - if out: - line = self.findstr(out, 'Initiator Name') - iscsi_facts['iscsi_iqn'] = line.split(":", 1)[1].rstrip() + try: + cmd = get_bin_path('iscsiutil', opt_dirs=['/opt/iscsi/bin']) + except ValueError: + return iscsi_facts + + cmd += " -l" + rc, out, err = module.run_command(cmd) + if out: + line = self.findstr(out, 'Initiator Name') + iscsi_facts['iscsi_iqn'] = line.split(":", 1)[1].rstrip() + return iscsi_facts def findstr(self, text, match): diff --git a/lib/ansible/module_utils/facts/packages.py b/lib/ansible/module_utils/facts/packages.py index b287b8ff9a6..808a41b6b08 100644 --- a/lib/ansible/module_utils/facts/packages.py +++ b/lib/ansible/module_utils/facts/packages.py @@ -79,5 +79,8 @@ class CLIMgr(PkgMgr): super(CLIMgr, self).__init__() def is_available(self): - self._cli = get_bin_path(self.CLI, False) - return bool(self._cli) + try: + self._cli = get_bin_path(self.CLI) + except ValueError: + return False + return True diff --git a/lib/ansible/modules/files/copy.py b/lib/ansible/modules/files/copy.py index e69b9de41fd..afd9aabfbea 100644 --- a/lib/ansible/modules/files/copy.py +++ b/lib/ansible/modules/files/copy.py @@ -291,7 +291,7 @@ class AnsibleModuleError(Exception): # basic::AnsibleModule() until then but if so, make it a private function so that we don't have to # keep it for backwards compatibility later. def clear_facls(path): - setfacl = get_bin_path('setfacl', True) + setfacl = get_bin_path('setfacl') # FIXME "setfacl -b" is available on Linux and FreeBSD. There is "setfacl -D e" on z/OS. Others? acl_command = [setfacl, '-b', path] b_acl_command = [to_bytes(x) for x in acl_command] diff --git a/lib/ansible/modules/packaging/os/package_facts.py b/lib/ansible/modules/packaging/os/package_facts.py index 24fe660554b..7943e6554a8 100644 --- a/lib/ansible/modules/packaging/os/package_facts.py +++ b/lib/ansible/modules/packaging/os/package_facts.py @@ -229,8 +229,14 @@ class RPM(LibMgr): def is_available(self): ''' we expect the python bindings installed, but this gives warning if they are missing and we have rpm cli''' we_have_lib = super(RPM, self).is_available() - if not we_have_lib and get_bin_path('rpm'): - module.warn('Found "rpm" but %s' % (missing_required_lib('rpm'))) + + try: + get_bin_path('rpm') + if not we_have_lib: + module.warn('Found "rpm" but %s' % (missing_required_lib('rpm'))) + except ValueError: + pass + return we_have_lib @@ -255,7 +261,11 @@ class APT(LibMgr): we_have_lib = super(APT, self).is_available() if not we_have_lib: for exe in ('apt', 'apt-get', 'aptitude'): - if get_bin_path(exe): + try: + get_bin_path(exe) + except ValueError: + continue + else: module.warn('Found "%s" but %s' % (exe, missing_required_lib('apt'))) break return we_have_lib diff --git a/lib/ansible/playbook/role/requirement.py b/lib/ansible/playbook/role/requirement.py index 084f4beedca..3a206befc75 100644 --- a/lib/ansible/playbook/role/requirement.py +++ b/lib/ansible/playbook/role/requirement.py @@ -155,7 +155,7 @@ class RoleRequirement(RoleDefinition): raise AnsibleError("- scm %s is not currently supported" % scm) try: - scm_path = get_bin_path(scm, required=True) + scm_path = get_bin_path(scm) except (ValueError, OSError, IOError): raise AnsibleError("could not find/use %s, it is required to continue with installing %s" % (scm, src)) diff --git a/lib/ansible/plugins/connection/chroot.py b/lib/ansible/plugins/connection/chroot.py index 727dd406498..d95497b42b9 100644 --- a/lib/ansible/plugins/connection/chroot.py +++ b/lib/ansible/plugins/connection/chroot.py @@ -101,10 +101,10 @@ class Connection(ConnectionBase): if os.path.isabs(self.get_option('chroot_exe')): self.chroot_cmd = self.get_option('chroot_exe') else: - self.chroot_cmd = get_bin_path(self.get_option('chroot_exe')) - - if not self.chroot_cmd: - raise AnsibleError("chroot command (%s) not found in PATH" % to_native(self.get_option('chroot_exe'))) + try: + self.chroot_cmd = get_bin_path(self.get_option('chroot_exe')) + except ValueError as e: + raise AnsibleError(to_native(e)) super(Connection, self)._connect() if not self._connected: diff --git a/lib/ansible/plugins/inventory/docker_machine.py b/lib/ansible/plugins/inventory/docker_machine.py index 4b6007868ec..2c4d4f50546 100644 --- a/lib/ansible/plugins/inventory/docker_machine.py +++ b/lib/ansible/plugins/inventory/docker_machine.py @@ -108,9 +108,9 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): def _run_command(self, args): if not self.DOCKER_MACHINE_PATH: try: - self.DOCKER_MACHINE_PATH = get_bin_path('docker-machine', required=True) + self.DOCKER_MACHINE_PATH = get_bin_path('docker-machine') except ValueError as e: - raise AnsibleError('Unable to locate the docker-machine binary.', orig_exc=e) + raise AnsibleError(to_native(e)) command = [self.DOCKER_MACHINE_PATH] command.extend(args) diff --git a/lib/ansible/plugins/inventory/nmap.py b/lib/ansible/plugins/inventory/nmap.py index 5c84f27560d..d2c54fe84e9 100644 --- a/lib/ansible/plugins/inventory/nmap.py +++ b/lib/ansible/plugins/inventory/nmap.py @@ -86,12 +86,9 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): def parse(self, inventory, loader, path, cache=False): try: - self._nmap = get_bin_path('nmap', True) + self._nmap = get_bin_path('nmap') except ValueError as e: - raise AnsibleParserError(e) - - if self._nmap is None: - raise AnsibleParserError('nmap inventory plugin requires the nmap cli tool to work') + raise AnsibleParserError('nmap inventory plugin requires the nmap cli tool to work: {0}'.format(to_native(e))) super(InventoryModule, self).parse(inventory, loader, path, cache=cache) diff --git a/lib/ansible/plugins/inventory/virtualbox.py b/lib/ansible/plugins/inventory/virtualbox.py index 16ed7962dc4..edfd14fe005 100644 --- a/lib/ansible/plugins/inventory/virtualbox.py +++ b/lib/ansible/plugins/inventory/virtualbox.py @@ -229,7 +229,7 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): def parse(self, inventory, loader, path, cache=True): try: - self._vbox_path = get_bin_path(self.VBOX, True) + self._vbox_path = get_bin_path(self.VBOX) except ValueError as e: raise AnsibleParserError(e) diff --git a/test/units/module_utils/common/process/test_get_bin_path.py b/test/units/module_utils/common/process/test_get_bin_path.py new file mode 100644 index 00000000000..96b649de3f1 --- /dev/null +++ b/test/units/module_utils/common/process/test_get_bin_path.py @@ -0,0 +1,33 @@ +# -*- coding: utf-8 -*- +# Copyright (c) 2020 Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +import pytest + +from ansible.module_utils.common.process import get_bin_path + + +def test_get_bin_path(mocker): + path = '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin' + mocker.patch.dict('os.environ', {'PATH': path}) + mocker.patch('os.pathsep', ':') + + mocker.patch('os.path.exists', side_effect=[False, True]) + mocker.patch('os.path.isdir', return_value=False) + mocker.patch('ansible.module_utils.common.process.is_executable', return_value=True) + + assert '/usr/local/bin/notacommand' == get_bin_path('notacommand') + + +def test_get_path_path_raise_valueerror(mocker): + mocker.patch.dict('os.environ', {'PATH': ''}) + + mocker.patch('os.path.exists', return_value=False) + mocker.patch('os.path.isdir', return_value=False) + mocker.patch('ansible.module_utils.common.process.is_executable', return_value=True) + + with pytest.raises(ValueError, match='Failed to find required executable notacommand'): + get_bin_path('notacommand')