From a2686b1a2c49ecbe95575b2e73e554a4c8f42b7b Mon Sep 17 00:00:00 2001 From: David Wilson Date: Sun, 19 Aug 2018 15:59:35 +0100 Subject: [PATCH] issue #321: simplify temp directory handling. --- ansible_mitogen/connection.py | 8 ++ ansible_mitogen/mixins.py | 42 ++---- ansible_mitogen/target.py | 44 +++--- docs/ansible.rst | 71 +++++++++- docs/changelog.rst | 7 + tests/ansible/ansible.cfg | 4 - .../integration/action/make_tmp_path.yml | 132 ++++++++++++------ tests/ansible/integration/all.yml | 1 - tests/ansible/integration/remote_tmp/all.yml | 2 - .../remote_tmp/readonly_homedir.yml | 20 --- .../custom_python_detect_environment.py | 1 + 11 files changed, 209 insertions(+), 123 deletions(-) delete mode 100644 tests/ansible/integration/remote_tmp/all.yml delete mode 100644 tests/ansible/integration/remote_tmp/readonly_homedir.yml diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index d3ae7f04..9b6a36a7 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -456,6 +456,9 @@ class Connection(ansible.plugins.connection.ConnectionBase): #: Set after connection to the target context's home directory. home_dir = None + #: Set after connection to the target context's home directory. + _temp_dir = None + def __init__(self, play_context, new_stdin, **kwargs): assert ansible_mitogen.process.MuxProcess.unix_listener_path, ( 'Mitogen connection types may only be instantiated ' @@ -635,6 +638,11 @@ class Connection(ansible.plugins.connection.ConnectionBase): self.fork_context = dct['init_child_result']['fork_context'] self.home_dir = dct['init_child_result']['home_dir'] + self._temp_dir = dct['init_child_result']['temp_dir'] + + def get_temp_dir(self): + self._connect() + return self._temp_dir def _connect(self): """ diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py index f2fa7ec7..98cb134e 100644 --- a/ansible_mitogen/mixins.py +++ b/ansible_mitogen/mixins.py @@ -180,48 +180,26 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): """ assert False, "_is_pipelining_enabled() should never be called." - def _get_remote_tmp(self): - """ - Mitogen-only: return the 'remote_tmp' setting. - """ - try: - s = self._connection._shell.get_option('remote_tmp') - except AttributeError: - s = ansible.constants.DEFAULT_REMOTE_TMP # <=2.4.x - - return self._remote_expand_user(s, sudoable=False) - def _make_tmp_path(self, remote_user=None): """ - Replace the base implementation's use of shell to implement mkdtemp() - with an actual call to mkdtemp(). Like vanilla, the directory is always - created in the login account context. + Return the temporary directory created by the persistent interpreter at + startup. """ LOG.debug('_make_tmp_path(remote_user=%r)', remote_user) - # _make_tmp_path() is basically a global stashed away as Shell.tmpdir. - # The copy action plugin violates layering and grabs this attribute - # directly. - self._connection._shell.tmpdir = self._connection.call( - ansible_mitogen.target.make_temp_directory, - base_dir=self._get_remote_tmp(), - use_login_context=True, - ) + self._connection._shell.tmpdir = self._connection.get_temp_dir() LOG.debug('Temporary directory: %r', self._connection._shell.tmpdir) self._cleanup_remote_tmp = True return self._connection._shell.tmpdir def _remove_tmp_path(self, tmp_path): """ - Replace the base implementation's invocation of rm -rf with a call to - shutil.rmtree(). + Stub out the base implementation's invocation of rm -rf, replacing it + with nothing, as the persistent interpreter automatically cleans up + after itself without introducing roundtrips. """ LOG.debug('_remove_tmp_path(%r)', tmp_path) - if tmp_path is None: - tmp_path = self._connection._shell.tmpdir - if self._should_remove_tmp_path(tmp_path): - self.call(shutil.rmtree, tmp_path) - self._connection._shell.tmpdir = None + self._connection._shell.tmpdir = None def _transfer_data(self, remote_path, data): """ @@ -332,7 +310,13 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): env = {} self._compute_environment_string(env) + # Always set _ansible_tmpdir regardless of whether _make_remote_tmp() + # has ever been called. This short-circuits all the .tmpdir logic in + # module_common and ensures no second temporary directory or atexit + # handler is installed. self._connection._connect() + module_args['_ansible_tmpdir'] = self._connection.get_temp_dir() + return ansible_mitogen.planner.invoke( ansible_mitogen.planner.Invocation( action=self, diff --git a/ansible_mitogen/target.py b/ansible_mitogen/target.py index 582bf85e..c5f85c6d 100644 --- a/ansible_mitogen/target.py +++ b/ansible_mitogen/target.py @@ -69,6 +69,19 @@ import ansible_mitogen.runner LOG = logging.getLogger(__name__) +MAKE_TEMP_FAILED_MSG = ( + "Unable to create a temporary directory for the persistent interpreter.\n" + "This likely means no system-supplied TMP directory can be written to.\n" + "\n" + "The following paths were tried:\n" + " %(namelist)s\n" + "\n" + "The original exception was:\n" + "\n" + "%(exception)s" +) + + #: Set by init_child() to the single temporary directory that will exist for #: the duration of the process. temp_dir = None @@ -204,7 +217,14 @@ def reset_temp_dir(econtext): """ global temp_dir # https://github.com/dw/mitogen/issues/239 - temp_dir = tempfile.mkdtemp(prefix='ansible_mitogen_') + + try: + temp_dir = tempfile.mkdtemp(prefix='ansible_mitogen_') + except IOError: + raise IOError(MAKE_TEMP_FAILED_MSG % { + 'namelist': '\n '.join(tempfile._candidate_tempdir_list()), + 'exception': traceback.format_exc() + }) # This must be reinstalled in forked children too, since the Broker # instance from the parent process does not carry over to the new child. @@ -252,6 +272,7 @@ def init_child(econtext, log_level): return { 'fork_context': _fork_parent, 'home_dir': mitogen.core.to_text(os.path.expanduser('~')), + 'temp_dir': temp_dir, } @@ -416,27 +437,6 @@ def run_module_async(kwargs, job_id, timeout_secs, econtext): arunner.run() -def make_temp_directory(base_dir): - """ - Handle creation of `base_dir` if it is absent, in addition to a unique - temporary directory within `base_dir`. This is the temporary directory that - becomes 'remote_tmp', not the one used by Ansiballz. It always uses the - system temporary directory. - - :returns: - Newly created temporary directory. - """ - # issue #301: remote_tmp may contain $vars. - base_dir = os.path.expandvars(base_dir) - - if not os.path.exists(base_dir): - os.makedirs(base_dir, mode=int('0700', 8)) - return tempfile.mkdtemp( - dir=base_dir, - prefix='ansible-mitogen-tmp-', - ) - - def get_user_shell(): """ For commands executed directly via an SSH command-line, SSH looks up the diff --git a/docs/ansible.rst b/docs/ansible.rst index d66743f4..e0ecce50 100644 --- a/docs/ansible.rst +++ b/docs/ansible.rst @@ -271,8 +271,8 @@ command line, or as host and group variables. File Transfer ~~~~~~~~~~~~~ -Normally `sftp `_ or -`scp `_ are used to copy files by the +Normally `sftp(1) `_ or +`scp(1) `_ are used to copy files by the `assemble `_, `copy `_, `patch `_, @@ -302,7 +302,7 @@ to rename over any existing file. This ensures the file remains consistent at all times, in the event of a crash, or when overlapping `ansible-playbook` runs deploy differing file contents. -The `sftp `_ and `scp +The `sftp(1) `_ and `scp(1) `_ tools may cause undetected data corruption in the form of truncated files, or files containing intermingled data segments from overlapping runs. As part of normal operation, both tools expose a window @@ -401,6 +401,67 @@ this precisely, to avoid breaking playbooks that expect text to appear in specific variables with a particular linefeed style. +.. _ansible_tempfiles: + +Temporary Files +~~~~~~~~~~~~~~~ + +Ansible creates a variety of temporary files and directories depending on its +operating mode. + +In the best case when pipelining is enabled and no temporary uploads are +required, for each task Ansible will create one directory below a +system-supplied temporary directory returned by :func:`tempfile.mkdtemp`, owned +by the target user account a new-style module intends to execute in. + +In other cases depending on the task type, whether become is active, whether +the target become user is privileged, whether the associated action plugin +needs to upload files, and whether the associated module needs to store files, +Ansible may: + +* Create a directory owned by the SSH user either under ``remote_tmp``, or a + system-default directory, +* Upload action dependencies such as non-new style modules or rendered + templates to that directory via `sftp(1) `_ + or `scp(1) `_. +* Attempt to modify the directory's access control list to grant access to the + target user using `setfacl(1) `_, + requiring that tool to be installed and a supported filesystem to be in use, + or for the ``allow_world_readable_tmpfiles`` setting to be :data:`True`. +* Create a directory owned by the target user either under ``remote_tmp``, or + a system-default directory, if a new-style module needs a temporary directory + and one was not previously created for a supporting file earlier in the + invocation. + +In summary, for each task Ansible may create one or more of: + +* ``~ssh_user//...`` owned by the login user, +* ``$TMPDIR/ansible-tmp-...`` owned by the login user, +* ``$TMPDIR/ansible-tmp-...`` owned by the login user with ACLs permitting + write access by the become user, +* ``~become_user//...`` owned by the become user, +* ``$TMPDIR/ansible__payload_.../`` owned by the become user, +* ``$TMPDIR/ansible-module-tmp-.../`` owned by the become user. + +The extension must create a temporary directory to maintain compatibility with +Ansible, since many modules introspect :data:`sys.argv` in order to find a +directory where they may write temporary files, however for simplicity only +one such directory exists for the lifetime of each interpreter, stored in a +system-supplied temporary directory, and always privately owned by the target +user account. + +The ``remote_tmp`` path is unused, since Ansible does not make exclusive use of +it, existing semantics are untenable, environments exist with read-only home +directories where the default ``remote_tmp`` path (``~/.ansible/tmp``) cannot +be used, and new-style modules always depended on the existence of a +system-supplied directory anyway, so no requirement is introduced by simply +ignoring ``remote_tmp``. + +As the directory is created once at startup, and its content is managed by code +running remotely, no additional network roundtrips are required to create and +destroy it for each task requiring temporary file storage. + + .. _ansible_process_env: Process Environment Emulation @@ -425,7 +486,7 @@ Modifications to ``/etc/resolv.conf`` cause the glibc resolver configuration to be reloaded via `res_init(3) `_. This isn't necessary on some Linux distributions carrying glibc patches to automatically check ``/etc/resolv.conf`` periodically, however it is necessary -on at least Debian and the BSD derivatives. +on at least Debian and BSD derivatives. ``/etc/environment`` @@ -433,7 +494,7 @@ on at least Debian and the BSD derivatives. When ``become: true`` is active or SSH multiplexing is disabled, modifications by previous tasks to ``/etc/environment`` and ``$HOME/.pam_environment`` are -reflected, since the content of those files is reapplied by `PAM +normally reflected, since the content of those files is reapplied by `PAM `_ via `pam_env` on each authentication of ``sudo`` or ``sshd``. diff --git a/docs/changelog.rst b/docs/changelog.rst index 9e226f96..d4ac261c 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -44,6 +44,13 @@ Mitogen for Ansible extension that had been installed using the documented steps. Now the bundled library always overrides over any system-installed copy. +* `#321 `_: temporary file handling + has been simplified and additional network roundtrips have been removed, + undoing earlier damage caused by compatibility bug fixes. A single directory + is created once at startup for each persistent interpreter, and the + ``remote_tmp`` setting is always ignored. See :ref:`ansible_tempfiles` for a + complete description. + * `#324 `_: plays with a custom ``module_utils`` would fail due to fallout from the Python 3 port and related tests being disabled. diff --git a/tests/ansible/ansible.cfg b/tests/ansible/ansible.cfg index d9224ab7..539964b8 100644 --- a/tests/ansible/ansible.cfg +++ b/tests/ansible/ansible.cfg @@ -17,10 +17,6 @@ timeout = 10 # On Travis, paramiko check fails due to host key checking enabled. host_key_checking = False -# "mitogen-tests" required by integration/runner/remote_tmp.yml -# "$HOME" required by integration/action/make_tmp_path.yml -remote_tmp = $HOME/.ansible/mitogen-tests/ - [ssh_connection] ssh_args = -o ForwardAgent=yes -o ControlMaster=auto -o ControlPersist=60s pipelining = True diff --git a/tests/ansible/integration/action/make_tmp_path.yml b/tests/ansible/integration/action/make_tmp_path.yml index 83261208..779280a3 100644 --- a/tests/ansible/integration/action/make_tmp_path.yml +++ b/tests/ansible/integration/action/make_tmp_path.yml @@ -1,63 +1,115 @@ +# +# Ensure _make_tmp_path returns the same result across invocations for a single +# user account, and that the path returned cleans itself up on connection +# termination. +# +# Related bugs prior to the new-style handling: +# https://github.com/dw/mitogen/issues/239 +# https://github.com/dw/mitogen/issues/301 - name: integration/action/make_tmp_path.yml hosts: test-targets any_errors_fatal: true tasks: - - name: "Find out root's homedir." - # Runs first because it blats regular Ansible facts with junk, so - # non-become run fixes that up. - setup: gather_subset=min - become: true - register: root_facts - - - name: "Find regular homedir" - setup: gather_subset=min - register: user_facts - # - # non-become + # non-root # - - action_passthrough: + - name: "Find regular temp path" + action_passthrough: method: _make_tmp_path - register: out + register: tmp_path + + - name: "Write some junk in regular temp path" + shell: hostname > {{tmp_path.result}}/hostname + + - name: "Verify junk did not persist across tasks" + stat: path={{tmp_path.result}}/hostname + register: junk_stat + + - name: "Verify junk did not persist across tasks" + assert: + that: + - not junk_stat.stat.exists + + - name: "Verify temp path hasn't changed since start" + action_passthrough: + method: _make_tmp_path + register: tmp_path2 + + - name: "Verify temp path hasn't changed since start" + assert: + that: + - tmp_path2.result == tmp_path.result + + - name: "Verify temp path changes across connection reset" + mitogen_shutdown_all: + + - name: "Verify temp path changes across connection reset" + action_passthrough: + method: _make_tmp_path + register: tmp_path2 - - assert: - # This string must match ansible.cfg::remote_tmp - that: out.result.startswith("{{user_facts.ansible_facts.ansible_user_dir}}/.ansible/mitogen-tests/") + - name: "Verify temp path changes across connection reset" + assert: + that: + - tmp_path2.result != tmp_path.result - - stat: - path: "{{out.result}}" - register: st + - name: "Verify old path disappears across connection reset" + stat: path={{tmp_path.result}} + register: junk_stat - - assert: - that: st.stat.exists and st.stat.isdir and st.stat.mode == "0700" + - name: "Verify old path disappears across connection reset" + assert: + that: + - not junk_stat.stat.exists - - file: - path: "{{out.result}}" - state: absent + # + # root + # + + - name: "Find root temp path" + become: true + action_passthrough: + method: _make_tmp_path + register: tmp_path_root + + - name: "Verify root temp path differs from regular path" + assert: + that: + - tmp_path2.result != tmp_path_root.result # - # become. make_tmp_path() must evaluate HOME in the context of the SSH - # user, not the become user. + # readonly homedir # - - action_passthrough: + - name: "Try writing to temp directory for the readonly_homedir user" + become: true + become_user: mitogen__readonly_homedir + action_passthrough: method: _make_tmp_path - register: out + register: tmp_path + + - name: "Try writing to temp directory for the readonly_homedir user" become: true + become_user: mitogen__readonly_homedir + shell: hostname > {{tmp_path.result}}/hostname - - assert: - # This string must match ansible.cfg::remote_tmp - that: out.result.startswith("{{user_facts.ansible_facts.ansible_user_dir}}/.ansible/mitogen-tests/") + # + # modules get the same temp dir + # - - stat: - path: "{{out.result}}" - register: st + - name: "Verify modules get the same tmpdir as the action plugin" + action_passthrough: + method: _make_tmp_path + register: tmp_path - - assert: - that: st.stat.exists and st.stat.isdir and st.stat.mode == "0700" + - name: "Verify modules get the same tmpdir as the action plugin" + custom_python_detect_environment: + register: out - - file: - path: "{{out.result}}" - state: absent + # v2.6 related: https://github.com/ansible/ansible/pull/39833 + - name: "Verify modules get the same tmpdir as the action plugin" + assert: + that: + - out.module_tmpdir == tmp_path.result diff --git a/tests/ansible/integration/all.yml b/tests/ansible/integration/all.yml index ffea7a46..4550e203 100644 --- a/tests/ansible/integration/all.yml +++ b/tests/ansible/integration/all.yml @@ -13,7 +13,6 @@ - import_playbook: local/all.yml - import_playbook: module_utils/all.yml - import_playbook: playbook_semantics/all.yml -- import_playbook: remote_tmp/all.yml - import_playbook: runner/all.yml - import_playbook: ssh/all.yml - import_playbook: strategy/all.yml diff --git a/tests/ansible/integration/remote_tmp/all.yml b/tests/ansible/integration/remote_tmp/all.yml deleted file mode 100644 index 5dff88d8..00000000 --- a/tests/ansible/integration/remote_tmp/all.yml +++ /dev/null @@ -1,2 +0,0 @@ - -- import_playbook: readonly_homedir.yml diff --git a/tests/ansible/integration/remote_tmp/readonly_homedir.yml b/tests/ansible/integration/remote_tmp/readonly_homedir.yml deleted file mode 100644 index ffad455a..00000000 --- a/tests/ansible/integration/remote_tmp/readonly_homedir.yml +++ /dev/null @@ -1,20 +0,0 @@ -# https://github.com/dw/mitogen/issues/239 -# While remote_tmp is used in the context of the SSH user by action code -# running on the controller, Ansiballz ignores it and uses the system default -# instead. - -- name: integration/remote_tmp/readonly_homedir.yml - hosts: test-targets - any_errors_fatal: true - tasks: - - custom_python_detect_environment: - become: true - become_user: mitogen__readonly_homedir - register: out - vars: - ansible_become_pass: readonly_homedir_password - - - name: Verify system temp directory was used. - assert: - that: - - out.__file__.startswith("/tmp/ansible_") diff --git a/tests/ansible/lib/modules/custom_python_detect_environment.py b/tests/ansible/lib/modules/custom_python_detect_environment.py index 8fe50bbc..442a8eff 100644 --- a/tests/ansible/lib/modules/custom_python_detect_environment.py +++ b/tests/ansible/lib/modules/custom_python_detect_environment.py @@ -29,6 +29,7 @@ def main(): mitogen_loaded='mitogen.core' in sys.modules, hostname=socket.gethostname(), username=pwd.getpwuid(os.getuid()).pw_name, + module_tmpdir=getattr(module, 'tmpdir', None), ) if __name__ == '__main__':