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

@@ -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)

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)

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 cleanup_tmp_file, makedirs_safe, unfrackpath
-from ansible.utils.path import makedirs_safe
 
 
 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):

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

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
+# 'view' command must fail when config file is missing or has an invalid file extension
-ansible-config view -c ./ansible-non-existent.cfg && exit 1 || echo 'Failure is expected'
+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

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)