From 004c33d9c5fdb20a46b9ef8fb0e203dd34bed3b3 Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Mon, 7 Jun 2021 12:59:06 -0400 Subject: [PATCH] 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 --- .../fragments/unarchive-fix-bin-checking.yml | 2 + lib/ansible/modules/unarchive.py | 56 ++++++++++--------- .../targets/unarchive/handlers/main.yml | 3 + .../targets/unarchive/tasks/main.yml | 1 + .../targets/unarchive/tasks/prepare_tests.yml | 16 ++---- .../unarchive/tasks/test_missing_binaries.yml | 56 +++++++++++++++++++ .../targets/unarchive/vars/Darwin.yml | 1 + .../targets/unarchive/vars/FreeBSD.yml | 4 ++ .../targets/unarchive/vars/Linux.yml | 4 ++ test/units/modules/test_unarchive.py | 51 +++++++++++------ 10 files changed, 140 insertions(+), 54 deletions(-) create mode 100644 changelogs/fragments/unarchive-fix-bin-checking.yml create mode 100644 test/integration/targets/unarchive/handlers/main.yml create mode 100644 test/integration/targets/unarchive/tasks/test_missing_binaries.yml create mode 100644 test/integration/targets/unarchive/vars/Darwin.yml create mode 100644 test/integration/targets/unarchive/vars/FreeBSD.yml create mode 100644 test/integration/targets/unarchive/vars/Linux.yml diff --git a/changelogs/fragments/unarchive-fix-bin-checking.yml b/changelogs/fragments/unarchive-fix-bin-checking.yml new file mode 100644 index 00000000000..30ea77fb930 --- /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 978fe78dabe..3cd37497640 100644 --- a/lib/ansible/modules/unarchive.py +++ b/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) diff --git a/test/integration/targets/unarchive/handlers/main.yml b/test/integration/targets/unarchive/handlers/main.yml new file mode 100644 index 00000000000..cb8b671ef09 --- /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 52b4bff4926..035a5561505 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 798c3f82892..98a8ba1118b 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 00000000000..f8e4536701b --- /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 00000000000..902feed6550 --- /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 00000000000..0f5c401c901 --- /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 00000000000..61109348f02 --- /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 index 3550e4cc342..c3300372766 100644 --- a/test/units/modules/test_unarchive.py +++ b/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