From 6cf6f5a34bebe01f96782a171db8d83d4e7828ca Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Fri, 28 Jun 2019 16:19:27 -0400 Subject: [PATCH] Use atexit to cleanup tmp dirs (#56532) * Wrap everything in try/except to avoid leaving files behind * Add unit tests, integration tests, and changelog * Do text the correct way --- bin/ansible | 3 -- .../fragments/config-cleanup-tmp-dirs.yaml | 2 + lib/ansible/config/manager.py | 5 +- lib/ansible/utils/path.py | 27 +++++++++++ test/integration/targets/ansible/runme.sh | 25 +++++++++- test/units/utils/test_cleanup_tmp_file.py | 48 +++++++++++++++++++ 6 files changed, 103 insertions(+), 7 deletions(-) create mode 100644 changelogs/fragments/config-cleanup-tmp-dirs.yaml create mode 100644 test/units/utils/test_cleanup_tmp_file.py diff --git a/bin/ansible b/bin/ansible index 0290ee9dfec..03891b728f5 100755 --- a/bin/ansible +++ b/bin/ansible @@ -149,8 +149,5 @@ if __name__ == '__main__': log_only = True display.display(u"the full traceback was:\n\n%s" % to_text(traceback.format_exc()), log_only=log_only) exit_code = 250 - finally: - # Remove ansible tmpdir - shutil.rmtree(C.DEFAULT_LOCAL_TMP, True) sys.exit(exit_code) diff --git a/changelogs/fragments/config-cleanup-tmp-dirs.yaml b/changelogs/fragments/config-cleanup-tmp-dirs.yaml new file mode 100644 index 00000000000..6aa9025ba33 --- /dev/null +++ b/changelogs/fragments/config-cleanup-tmp-dirs.yaml @@ -0,0 +1,2 @@ +bugfixes: + - remove all temporary directories created by ansible-config (https://github.com/ansible/ansible/issues/56488) diff --git a/lib/ansible/config/manager.py b/lib/ansible/config/manager.py index c951e82d58c..15931e36d1e 100644 --- a/lib/ansible/config/manager.py +++ b/lib/ansible/config/manager.py @@ -4,6 +4,7 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +import atexit import io import os import os.path @@ -28,8 +29,7 @@ from ansible.module_utils._text import to_text, to_bytes, to_native from ansible.module_utils.parsing.convert_bool import boolean from ansible.parsing.quoting import unquote from ansible.utils import py3compat -from ansible.utils.path import unfrackpath -from ansible.utils.path import makedirs_safe +from ansible.utils.path import cleanup_tmp_file, makedirs_safe, unfrackpath Plugin = namedtuple('Plugin', 'name type') @@ -110,6 +110,7 @@ def ensure_type(value, value_type, origin=None): makedirs_safe(value, 0o700) prefix = 'ansible-local-%s' % os.getpid() value = tempfile.mkdtemp(prefix=prefix, dir=value) + atexit.register(cleanup_tmp_file, value, warn=True) elif value_type == 'pathspec': if isinstance(value, string_types): diff --git a/lib/ansible/utils/path.py b/lib/ansible/utils/path.py index 37dd4c9139f..4fd416c2808 100644 --- a/lib/ansible/utils/path.py +++ b/lib/ansible/utils/path.py @@ -18,6 +18,7 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type import os +import shutil from errno import EEXIST from ansible.errors import AnsibleError @@ -103,3 +104,29 @@ def basedir(source): dname = os.path.abspath(dname) return to_text(dname, errors='surrogate_or_strict') + + +def cleanup_tmp_file(path, warn=False): + """ + Removes temporary file or directory. Optionally display a warning if unable + to remove the file or directory. + + :arg path: Path to file or directory to be removed + :kwarg warn: Whether or not to display a warning when the file or directory + cannot be removed + """ + try: + if os.path.exists(path): + try: + if os.path.isdir(path): + shutil.rmtree(path) + elif os.path.isfile(path): + os.unlink(path) + except Exception as e: + if warn: + # Importing here to avoid circular import + from ansible.utils.display import Display + display = Display() + display.display(u'Unable to remove temporary file {0}'.format(to_text(e))) + except Exception: + pass diff --git a/test/integration/targets/ansible/runme.sh b/test/integration/targets/ansible/runme.sh index b3ddb65fd7c..e9554934bad 100755 --- a/test/integration/targets/ansible/runme.sh +++ b/test/integration/targets/ansible/runme.sh @@ -13,5 +13,26 @@ ansible-config dump -c ./ansible-testé.cfg | grep 'DEFAULT_REMOTE_USER([^)]*) = ANSIBLE_REMOTE_USER=administrator ansible-config dump| grep 'DEFAULT_REMOTE_USER([^)]*) = administrator\>' ansible-config list | grep 'DEFAULT_REMOTE_USER' -# 'view' command must fail when config file is missing -ansible-config view -c ./ansible-non-existent.cfg && exit 1 || echo 'Failure is expected' +# 'view' command must fail when config file is missing or has an invalid file extension +ansible-config view -c ./ansible-non-existent.cfg 2> err1.txt || grep -Eq '(FileNotFoundError|IOError): \[Errno [0-9]+\] No such file or directory' err1.txt || (cat err*.txt; rm -f err1.txt; exit 1) +ansible-config view -c ./no-extension 2> err2.txt || grep -q 'Unsupported configuration file extension' err2.txt || (cat err2.txt; rm -f err*.txt; exit 1) +rm -f err*.txt + +# Test that no tmp dirs are left behind when running ansible-config +TMP_DIR=~/.ansible/tmptest +if [[ -d "$TMP_DIR" ]]; then + rm -rf "$TMP_DIR" +fi +ANSIBLE_LOCAL_TEMP="$TMP_DIR" ansible-config list > /dev/null +ANSIBLE_LOCAL_TEMP="$TMP_DIR" ansible-config dump > /dev/null +ANSIBLE_LOCAL_TEMP="$TMP_DIR" ansible-config view > /dev/null + +# wc on macOS is dumb and returns leading spaces +file_count=$(find "$TMP_DIR" -type d -maxdepth 1 | wc -l | sed 's/^ *//') +if [[ $file_count -ne 1 ]]; then + echo "$file_count temporary files were left behind by ansible-config" + if [[ -d "$TMP_DIR" ]]; then + rm -rf "$TMP_DIR" + fi + exit 1 +fi diff --git a/test/units/utils/test_cleanup_tmp_file.py b/test/units/utils/test_cleanup_tmp_file.py new file mode 100644 index 00000000000..5bc3900a558 --- /dev/null +++ b/test/units/utils/test_cleanup_tmp_file.py @@ -0,0 +1,48 @@ +# -*- coding: utf-8 -*- +# Copyright: (c) 2019, 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) +__metaclass__ = type + +import os +import pytest +import tempfile + +from ansible.utils.path import cleanup_tmp_file + + +def raise_error(): + raise OSError + + +def test_cleanup_tmp_file_file(): + tmp_fd, tmp = tempfile.mkstemp() + cleanup_tmp_file(tmp) + + assert not os.path.exists(tmp) + + +def test_cleanup_tmp_file_dir(): + tmp = tempfile.mkdtemp() + cleanup_tmp_file(tmp) + + assert not os.path.exists(tmp) + + +def test_cleanup_tmp_file_nonexistant(): + assert None is cleanup_tmp_file('nope') + + +def test_cleanup_tmp_file_failure(mocker): + tmp = tempfile.mkdtemp() + with pytest.raises(Exception): + mocker.patch('shutil.rmtree', side_effect=raise_error()) + cleanup_tmp_file(tmp) + + +def test_cleanup_tmp_file_failure_warning(mocker, capsys): + tmp = tempfile.mkdtemp() + with pytest.raises(Exception): + mocker.patch('shutil.rmtree', side_effect=raise_error()) + cleanup_tmp_file(tmp, warn=True)