[bp-2.11] unarchive - do not fail in init when trying to find required binary (#75363)

Test for the required binaries in the can_handle_archive() method and fail there. This
prevents failures for missing binaries unrelated to the archive type.

* Update missing zip binary message to match tar message
* Update unit tests
* Add integration tests
* Define packages based on the system rather than ignoring failures

(cherry picked from commit 004c33d9c5fdb20a46b9ef8fb0e203dd34bed3b3)

Co-authored-by: Sam Doran <sdoran@redhat.com>

10 files changed, 199 insertions, 28 deletions

diff --git a/changelogs/fragments/unarchive-fix-bin-checking.yml b/changelogs/fragments/unarchive-fix-bin-checking.yml
new file mode 100644
index 0000000000..30ea77fb93
--- /dev/null
+++ b/changelogs/fragments/unarchive-fix-bin-checking.yml
@@ -0,0 +1,2 @@
+bugfixes:
+  - unarchive - move failure for missing binary to ``can_handle_archive()`` rather than ``__init__()``
diff --git a/lib/ansible/modules/unarchive.py b/lib/ansible/modules/unarchive.py
index c05180a9f7..4867f8d79a 100644
--- a/lib/ansible/modules/unarchive.py
+++ b/lib/ansible/modules/unarchive.py
@@ -229,6 +229,7 @@ import traceback
 from zipfile import ZipFile, BadZipfile
 
 from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.common.process import get_bin_path
 from ansible.module_utils.urls import fetch_file
 from ansible.module_utils._text import to_bytes, to_native, to_text
 
@@ -278,8 +279,8 @@ class ZipArchive(object):
         self.excludes = module.params['exclude']
         self.includes = []
         self.include_files = self.module.params['include']
-        self.cmd_path = self.module.get_bin_path('unzip')
-        self.zipinfocmd_path = self.module.get_bin_path('zipinfo')
+        self.cmd_path = None
+        self.zipinfo_cmd_path = None
         self._files_in_archive = []
         self._infodict = dict()
 
@@ -373,7 +374,8 @@ class ZipArchive(object):
 
     def is_unarchived(self):
         # BSD unzip doesn't support zipinfo listings with timestamp.
-        cmd = [self.zipinfocmd_path, '-T', '-s', self.src]
+        cmd = [self.zipinfo_cmd_path, '-T', '-s', self.src]
+
         if self.excludes:
             cmd.extend(['-x', ] + self.excludes)
         if self.include_files:
@@ -693,8 +695,20 @@ class ZipArchive(object):
         return dict(cmd=cmd, rc=rc, out=out, err=err)
 
     def can_handle_archive(self):
-        if not self.cmd_path:
-            return False, 'Command "unzip" not found.'
+        binaries = (
+            ('unzip', 'cmd_path'),
+            ('zipinfo', 'zipinfo_cmd_path'),
+        )
+        missing = []
+        for b in binaries:
+            try:
+                setattr(self, b[1], get_bin_path(b[0]))
+            except ValueError:
+                missing.append(b[0])
+
+        if missing:
+            return False, "Unable to find required '{missing}' binary in the path.".format(missing="' or '".join(missing))
+
         cmd = [self.cmd_path, '-l', self.src]
         rc, out, err = self.module.run_command(cmd)
         if rc == 0:
@@ -714,19 +728,11 @@ class TgzArchive(object):
             self.module.exit_json(skipped=True, msg="remote module (%s) does not support check mode when using gtar" % self.module._name)
         self.excludes = [path.rstrip('/') for path in self.module.params['exclude']]
         self.include_files = self.module.params['include']
-        # Prefer gtar (GNU tar) as it supports the compression options -z, -j and -J
-        self.cmd_path = self.module.get_bin_path('gtar', None)
-        if not self.cmd_path:
-            # Fallback to tar
-            self.cmd_path = self.module.get_bin_path('tar')
+        self.cmd_path = None
+        self.tar_type = None
         self.zipflag = '-z'
         self._files_in_archive = []
 
-        if self.cmd_path:
-            self.tar_type = self._get_tar_type()
-        else:
-            self.tar_type = None
-
     def _get_tar_type(self):
         cmd = [self.cmd_path, '--version']
         (rc, out, err) = self.module.run_command(cmd)
@@ -854,8 +860,17 @@ class TgzArchive(object):
         return dict(cmd=cmd, rc=rc, out=out, err=err)
 
     def can_handle_archive(self):
-        if not self.cmd_path:
-            return False, 'Commands "gtar" and "tar" not found.'
+        # Prefer gtar (GNU tar) as it supports the compression options -z, -j and -J
+        try:
+            self.cmd_path = get_bin_path('gtar')
+        except ValueError:
+            # Fallback to tar
+            try:
+                self.cmd_path = get_bin_path('tar')
+            except ValueError:
+                return False, "Unable to find required 'gtar' or 'tar' binary in the path"
+
+        self.tar_type = self._get_tar_type()
 
         if self.tar_type != 'gnu':
             return False, 'Command "%s" detected as tar type %s. GNU tar required.' % (self.cmd_path, self.tar_type)
diff --git a/test/integration/targets/unarchive/handlers/main.yml b/test/integration/targets/unarchive/handlers/main.yml
new file mode 100644
index 0000000000..cb8b671ef0
--- /dev/null
+++ b/test/integration/targets/unarchive/handlers/main.yml
@@ -0,0 +1,3 @@
+- name: restore packages
+  package:
+    name: "{{ unarchive_packages }}"
diff --git a/test/integration/targets/unarchive/tasks/main.yml b/test/integration/targets/unarchive/tasks/main.yml
index 52b4bff492..035a556150 100644
--- a/test/integration/targets/unarchive/tasks/main.yml
+++ b/test/integration/targets/unarchive/tasks/main.yml
@@ -1,4 +1,5 @@
 - import_tasks: prepare_tests.yml
+- import_tasks: test_missing_binaries.yml
 - import_tasks: test_tar.yml
 - import_tasks: test_tar_gz.yml
 - import_tasks: test_tar_gz_creates.yml
diff --git a/test/integration/targets/unarchive/tasks/prepare_tests.yml b/test/integration/targets/unarchive/tasks/prepare_tests.yml
index 798c3f8289..98a8ba1118 100644
--- a/test/integration/targets/unarchive/tasks/prepare_tests.yml
+++ b/test/integration/targets/unarchive/tasks/prepare_tests.yml
@@ -1,16 +1,10 @@
-# Need unzip for unarchive module, and zip for archive creation.
-- name: Ensure zip & unzip are present
-  package:
-    name:
-      - zip
-      - unzip
-  when: ansible_pkg_mgr in ('yum', 'dnf', 'apt', 'pkgng')
+- name: Include system specific variables
+  include_vars: "{{ ansible_facts.system }}.yml"
 
-- name: Ensure zstd is present, if available
-  ignore_errors: true
+# Need unzip for unarchive module, and zip for archive creation.
+- name: Ensure required binaries are present
   package:
-    name:
-      - zstd
+    name: "{{ unarchive_packages }}"
   when: ansible_pkg_mgr in ('yum', 'dnf', 'apt', 'pkgng')
 
 - name: prep our file
diff --git a/test/integration/targets/unarchive/tasks/test_missing_binaries.yml b/test/integration/targets/unarchive/tasks/test_missing_binaries.yml
new file mode 100644
index 0000000000..f8e4536701
--- /dev/null
+++ b/test/integration/targets/unarchive/tasks/test_missing_binaries.yml
@@ -0,0 +1,56 @@
+- name: Test missing binaries
+  when: ansible_pkg_mgr in ('yum', 'dnf', 'apt', 'pkgng')
+  block:
+    - name: Remove zip binaries
+      package:
+        state: absent
+        name:
+          - zip
+          - unzip
+      notify: restore packages
+
+    - name: create unarchive destinations
+      file:
+        path: '{{ remote_tmp_dir }}/test-unarchive-{{ item }}'
+        state: directory
+      loop:
+        - zip
+        - tar
+
+    # With the zip binaries absent and tar still present, this task should work
+    - name: unarchive a tar file
+      unarchive:
+        src: '{{remote_tmp_dir}}/test-unarchive.tar'
+        dest: '{{remote_tmp_dir}}/test-unarchive-tar'
+        remote_src: yes
+      register: tar
+
+    - name: unarchive a zip file
+      unarchive:
+        src: '{{remote_tmp_dir}}/test-unarchive.zip'
+        dest: '{{remote_tmp_dir}}/test-unarchive-zip'
+        list_files: True
+        remote_src: yes
+      register: zip_fail
+      ignore_errors: yes
+
+    - name: Ensure tasks worked as expected
+      assert:
+        that:
+          - tar is success
+          - zip_fail is failed
+          - zip_fail.msg is search('Unable to find required')
+
+    - name: Remove unarchive destinations
+      file:
+        path: '{{ remote_tmp_dir }}/test-unarchive-{{ item }}'
+        state: absent
+      loop:
+        - zip
+        - tar
+
+    - name: Reinsntall zip binaries
+      package:
+        name:
+          - zip
+          - unzip
diff --git a/test/integration/targets/unarchive/vars/Darwin.yml b/test/integration/targets/unarchive/vars/Darwin.yml
new file mode 100644
index 0000000000..902feed655
--- /dev/null
+++ b/test/integration/targets/unarchive/vars/Darwin.yml
@@ -0,0 +1 @@
+unarchive_packages: []
diff --git a/test/integration/targets/unarchive/vars/FreeBSD.yml b/test/integration/targets/unarchive/vars/FreeBSD.yml
new file mode 100644
index 0000000000..0f5c401c90
--- /dev/null
+++ b/test/integration/targets/unarchive/vars/FreeBSD.yml
@@ -0,0 +1,4 @@
+unarchive_packages:
+  - unzip
+  - zip
+  - zstd
diff --git a/test/integration/targets/unarchive/vars/Linux.yml b/test/integration/targets/unarchive/vars/Linux.yml
new file mode 100644
index 0000000000..61109348f0
--- /dev/null
+++ b/test/integration/targets/unarchive/vars/Linux.yml
@@ -0,0 +1,4 @@
+unarchive_packages:
+  - tar
+  - unzip
+  - zip
diff --git a/test/units/modules/test_unarchive.py b/test/units/modules/test_unarchive.py
new file mode 100644
index 0000000000..c330037276
--- /dev/null
+++ b/test/units/modules/test_unarchive.py
@@ -0,0 +1,91 @@
+from __future__ import absolute_import, division, print_function
+
+__metaclass__ = type
+
+
+import pytest
+
+from ansible.modules.unarchive import ZipArchive, TgzArchive
+
+
+class AnsibleModuleExit(Exception):
+    def __init__(self, *args, **kwargs):
+        self.args = args
+        self.kwargs = kwargs
+
+
+class ExitJson(AnsibleModuleExit):
+    pass
+
+
+class FailJson(AnsibleModuleExit):
+    pass
+
+
+@pytest.fixture
+def fake_ansible_module():
+    return FakeAnsibleModule()
+
+
+class FakeAnsibleModule:
+    def __init__(self):
+        self.params = {}
+        self.tmpdir = None
+
+    def exit_json(self, *args, **kwargs):
+        raise ExitJson(*args, **kwargs)
+
+    def fail_json(self, *args, **kwargs):
+        raise FailJson(*args, **kwargs)
+
+
+class TestCaseZipArchive:
+    @pytest.mark.parametrize(
+        'side_effect, expected_reason', (
+            ([ValueError, '/bin/zipinfo'], "Unable to find required 'unzip'"),
+            (ValueError, "Unable to find required 'unzip' or 'zipinfo'"),
+        )
+    )
+    def test_no_zip_zipinfo_binary(self, mocker, fake_ansible_module, side_effect, expected_reason):
+        mocker.patch("ansible.modules.unarchive.get_bin_path", side_effect=side_effect)
+        fake_ansible_module.params = {
+            "extra_opts": "",
+            "exclude": "",
+            "include": "",
+        }
+
+        z = ZipArchive(
+            src="",
+            b_dest="",
+            file_args="",
+            module=fake_ansible_module,
+        )
+        can_handle, reason = z.can_handle_archive()
+
+        assert can_handle is False
+        assert expected_reason in reason
+        assert z.cmd_path is None
+
+
+class TestCaseTgzArchive:
+    def test_no_tar_binary(self, mocker, fake_ansible_module):
+        mocker.patch("ansible.modules.unarchive.get_bin_path", side_effect=ValueError)
+        fake_ansible_module.params = {
+            "extra_opts": "",
+            "exclude": "",
+            "include": "",
+        }
+        fake_ansible_module.check_mode = False
+
+        t = TgzArchive(
+            src="",
+            b_dest="",
+            file_args="",
+            module=fake_ansible_module,
+        )
+        can_handle, reason = t.can_handle_archive()
+
+        assert can_handle is False
+        assert 'Unable to find required' in reason
+        assert t.cmd_path is None
+        assert t.tar_type is None