unarchive - do not fail in init when trying to find required binary (#74892)

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

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__()``

lib/ansible/modules/unarchive.py

@@ -290,14 +290,8 @@ class ZipArchive(object):
         self.excludes = module.params['exclude']
         self.includes = []
         self.include_files = self.module.params['include']
-        try:
-            self.cmd_path = get_bin_path('unzip')
-        except ValueError:
-            self.module.fail_json(msg="Unable to find required 'unzip' binary in the path")
-        try:
-            self.zipinfocmd_path = get_bin_path('zipinfo')
-        except ValueError:
-            self.module.fail_json(msg="Unable to find required 'zipinfo' binary in the path")
+        self.cmd_path = None
+        self.zipinfo_cmd_path = None
         self._files_in_archive = []
         self._infodict = dict()
 
@@ -387,7 +381,7 @@ 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)
@@ -708,8 +702,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:
@@ -729,23 +735,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
-        try:
-            self.cmd_path = get_bin_path('gtar')
-        except ValueError:
-            # Fallback to tar
-            try:
-                self.cmd_path = get_bin_path('tar')
-            except ValueError:
-                self.module.fail_json(msg="Unable to find required 'gtar' or 'tar' binary in the path")
+        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)
@@ -873,6 +867,18 @@ class TgzArchive(object):
         return dict(cmd=cmd, rc=rc, out=out, err=err)
 
     def can_handle_archive(self):
+        # 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)
 

test/integration/targets/unarchive/handlers/main.yml

@@ -0,0 +1,3 @@
+- name: restore packages
+  package:
+    name: "{{ unarchive_packages }}"

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

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

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

test/integration/targets/unarchive/vars/Darwin.yml

@@ -0,0 +1 @@
+unarchive_packages: []

test/integration/targets/unarchive/vars/FreeBSD.yml

@@ -0,0 +1,4 @@
+unarchive_packages:
+  - unzip
+  - zip
+  - zstd

test/integration/targets/unarchive/vars/Linux.yml

@@ -0,0 +1,4 @@
+unarchive_packages:
+  - tar
+  - unzip
+  - zip

test/units/modules/test_unarchive.py

@@ -40,21 +40,31 @@ class FakeAnsibleModule:
 
 
 class TestCaseZipArchive:
-    def test_no_zip_binary(self, mocker, fake_ansible_module):
-        mocker.patch("ansible.modules.unarchive.get_bin_path", side_effect=ValueError)
+    @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": "",
         }
-        with pytest.raises(FailJson) as exec_info:
-            z = ZipArchive(
-                src="",
-                b_dest="",
-                file_args="",
-                module=fake_ansible_module,
-            )
-        assert "Unable to find required" in exec_info.value.kwargs["msg"]
+
+        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:
@@ -66,11 +76,16 @@ class TestCaseTgzArchive:
             "include": "",
         }
         fake_ansible_module.check_mode = False
-        with pytest.raises(FailJson) as exec_info:
-            z = TgzArchive(
-                src="",
-                b_dest="",
-                file_args="",
-                module=fake_ansible_module,
-            )
-        assert "Unable to find required" in exec_info.value.kwargs["msg"]
+
+        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