issue #338: refactor env handling into class and fix tests.

ansible_mitogen/runner.py

@@ -79,94 +79,134 @@ for symbol in 'res_init', '__res_init':
     except AttributeError:
         pass
 
-# For tasks running on Linux machines, with vanilla Ansible, edits to
-# /etc/environment and ~/.pam_environment are reflected if become:true, due to
-# sudo reinvoking pam_env. If multiplexing is disabled, then edits are also
-# reflected with become:false. Rather than emulate existing semantics, simply
-# always ensure edits are reflects for the next task.
-try:
-    etc_env_st = os.stat('/etc/environment')
-except OSError:
-    etc_env_st = None
-
-try:
-    pam_env_st = os.stat(os.path.expanduser('~/.pam_environment'))
-except OSError:
-    pam_env_st = None
-
-
 iteritems = getattr(dict, 'iteritems', dict.items)
 LOG = logging.getLogger(__name__)
 
 
-def utf8(s):
+class EnvironmentFileWatcher(object):
-    """
-    Coerce an object to bytes if it is Unicode.
     """
-    if isinstance(s, mitogen.core.UnicodeType):
+    Usually Ansible edits to /etc/environment and ~/.pam_environment are
-        s = s.encode('utf-8')
+    reflected in subsequent tasks if become:true or SSH multiplexing is
-    return s
+    disabled, due to sudo and/or SSH reinvoking pam_env. Rather than emulate
+    existing semantics, do our best to ensure edits are always reflected.
 
+    This can't perfectly replicate the existing behaviour, but it can safely
+    update and remove keys that appear to originate in `path`, and that do not
+    conflict with any existing environment key inherited from elsewhere.
 
-def reopen_readonly(fp):
+    A more robust future approach may simply be to arrange for the persistent
-    """
+    interpreter to restart when a change is detected.
-    Replace the file descriptor belonging to the file object `fp` with one
-    open on the same file (`fp.name`), but opened with :py:data:`os.O_RDONLY`.
-    This enables temporary files to be executed on Linux, which usually throws
-    ``ETXTBUSY`` if any writeable handle exists pointing to a file passed to
-    `execve()`.
     """
-    fd = os.open(fp.name, os.O_RDONLY)
+    def __init__(self, path):
-    os.dup2(fd, fp.fileno())
+        self.path = os.path.expanduser(path)
-    os.close(fd)
+        #: Inode data at time of last check.
+        self._st = self._stat()
+        #: List of inherited keys appearing to originated from this file.
+        self._keys = [key for key, value in self._load()
+                      if value == os.environ.get(key)]
+        LOG.debug('%r installed; existing keys: %r', self, self._keys)
+
+    def __repr__(self):
+        return 'EnvironmentFileWatcher(%r)' % (self.path,)
 
+    def _stat(self):
+        try:
+            return os.stat(self.path)
+        except OSError:
+            return None
 
-def parse_env(fp):
+    def _load(self):
+        try:
+            with open(self.path, 'r') as fp:
+                return list(self._parse(fp))
+        except IOError:
+            return []
+
+    def _parse(self, fp):
         """
-    Parse /etc/environ using roughly the same syntax as pam_env.
+        linux-pam-1.3.1/modules/pam_env/pam_env.c#L207
         """
-    # https://github.com/linux-pam/linux-pam/blob/v1.3.1/modules/pam_env/pam_env.c#L207
         for line in fp:
             # '   #export foo=some var  ' -> ['#export', 'foo=some var  ']
             bits = shlex.split(line, comments=True)
-        if not bits:
+            if (not bits) or bits[0].startswith('#'):
                 continue
 
             if bits[0] == 'export':
                 bits.pop(0)
 
             key, sep, value = (' '.join(bits)).partition('=')
-        if sep:
+            if key and sep:
+                yield key, value
+
+    def _on_file_changed(self):
+        LOG.debug('%r: file changed, reloading', self)
+        for key, value in self._load():
+            if key in os.environ:
+                LOG.debug('%r: existing key %r=%r exists, not setting %r',
+                          self, key, os.environ[key], value)
+            else:
+                LOG.debug('%r: setting key %r to %r', self, key, value)
+                self._keys.append(key)
                 os.environ[key] = value
 
+    def _remove_existing(self):
+        """
+        When a change is detected, remove keys that existed in the old file.
+        """
+        for key in self._keys:
+            if key in os.environ:
+                LOG.debug('%r: removing old key %r', self, key)
+                del os.environ[key]
+        self._keys = []
 
-def reload_env(old_st, path):
+    def check(self):
         """
-    Compare the :func:`os.stat` for the pam_env style environmnt file `path`
+        Compare the :func:`os.stat` for the pam_env style environmnt file
-    with the previous result `old_st`, which may be :data:`None` if the
+        `path` with the previous result `old_st`, which may be :data:`None` if
-    previous stat attempt failed. Reload its contents if the file has changed
+        the previous stat attempt failed. Reload its contents if the file has
-    or appeared since last attempt.
+        changed or appeared since last attempt.
 
         :returns:
             New :func:`os.stat` result. The new call to :func:`reload_env` should
             pass it as the value of `old_st`.
         """
-    try:
+        st = self._stat()
-        path = os.path.expanduser(path)
+        if self._st == st:
-        st = os.stat(path)
+            return
-    except OSError:
+
-        return None
+        self._st = st
+        self._remove_existing()
 
-    if old_st == st:
-        return old_st
         if st is None:
-        LOG.debug('reload_env(%r): file has disappeared', path)
+            LOG.debug('%r: file has disappeared', self)
-        return st
+        else:
+            self._on_file_changed()
+
+_pam_env_watcher = EnvironmentFileWatcher('~/.pam_environment')
+_etc_env_watcher = EnvironmentFileWatcher('/etc/environment')
+
 
-    LOG.debug('reload_env(%r): file has changed or appeared, reloading', path)
+def utf8(s):
-    with open(path) as fp:
+    """
-        parse_env(fp)
+    Coerce an object to bytes if it is Unicode.
-    return st
+    """
+    if isinstance(s, mitogen.core.UnicodeType):
+        s = s.encode('utf-8')
+    return s
+
+
+def reopen_readonly(fp):
+    """
+    Replace the file descriptor belonging to the file object `fp` with one
+    open on the same file (`fp.name`), but opened with :py:data:`os.O_RDONLY`.
+    This enables temporary files to be executed on Linux, which usually throws
+    ``ETXTBUSY`` if any writeable handle exists pointing to a file passed to
+    `execve()`.
+    """
+    fd = os.open(fp.name, os.O_RDONLY)
+    os.dup2(fd, fp.fileno())
+    os.close(fd)
 
 
 class Runner(object):
@@ -219,30 +259,29 @@ class Runner(object):
         from the parent, as :meth:`run` may detach prior to beginning
         execution. The base implementation simply prepares the environment.
         """
+        self._setup_cwd()
+        self._setup_environ()
+
+    def _setup_cwd(self):
+        """
+        For situations like sudo to a non-privileged account, CWD could be
+        $HOME of the old account, which could have mode go=, which means it is
+        impossible to restore the old directory, so don't even try.
+        """
         if self.cwd:
-            # For situations like sudo to another non-privileged account, the
-            # CWD could be $HOME of the old account, which could have mode go=,
-            # which means it is impossible to restore the old directory, so
-            # don't even bother.
             os.chdir(self.cwd)
-        env = dict(self.extra_env or {})
-        if self.env:
-            env.update(self.env)
-        self._setup_environ()
-        self._env = TemporaryEnvironment(env)
 
     def _setup_environ(self):
         """
-        Ensure /etc/environment and ~/.pam_environment are reloaded if their
+        Apply changes from /etc/environment files before creating a
-        content appears to differ since execution of the previous task. This
+        TemporaryEnvironment to snapshot environment state prior to module run.
-        must happen before TemporaryEnvironment is installed, to ensure changes
-        persist across tasks.
         """
-        global etc_env_st
+        _pam_env_watcher.check()
-        etc_env_st = reload_env(etc_env_st, '/etc/environment')
+        _etc_env_watcher.check()
-
+        env = dict(self.extra_env or {})
-        global pam_env_st
+        if self.env:
-        pam_env_st = reload_env(pam_env_st, '~/.pam_environment')
+            env.update(self.env)
+        self._env = TemporaryEnvironment(env)
 
     def revert(self):
         """

docs/ansible.rst

@@ -401,6 +401,61 @@ this precisely, to avoid breaking playbooks that expect text to appear in
 specific variables with a particular linefeed style.
 
 
+.. _ansible_process_env:
+
+Process Environment Emulation
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Since Ansible discards processes after each module invocation, follow-up tasks
+often (but not always) receive a new environment that will usually include
+changes made by previous tasks. As such modifications are common, for
+compatibility the extension emulates the existing behaviour as closely as
+possible.
+
+Some scenarios exist where emulation is impossible, for example, applying
+``nsswitch.conf`` changes when ``nscd`` is not in use. If future scenarios
+appear that cannot be solved through emulation, the extension will be updated
+to automatically restart affected interpreters instead.
+
+
+DNS Resolution
+^^^^^^^^^^^^^^
+
+Modifications to ``/etc/resolv.conf`` cause the glibc resolver configuration to
+be reloaded via `res_init(3) <https://linux.die.net/man/3/res_init>`_. 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.
+
+
+``/etc/environment``
+^^^^^^^^^^^^^^^^^^^^
+
+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
+<https://en.wikipedia.org/wiki/Pluggable_authentication_module>`_ via `pam_env`
+on each authentication of ``sudo`` or ``sshd``.
+
+Both files are monitored for changes, and changes are applied where it appears
+safe to do so:
+
+* New keys are added if they did not otherwise exist in the inherited
+  environment, or previously had the same value as found in the file before it
+  changed.
+
+* Given a key (such as ``http_proxy``) added to the file where no such key
+  exists in the environment, the key will be added.
+
+* Given a key (such as ``PATH``) where an existing environment key exists with
+  a different value, the update or deletion will be ignored, as it is likely
+  the key was overridden elsewhere after `pam_env` ran, such as by
+  ``/etc/profile``.
+
+* Given a key removed from the file that had the same value as the existing
+  environment key, the key will be removed.
+
+
 How Modules Execute
 ~~~~~~~~~~~~~~~~~~~
 

docs/changelog.rst

@@ -56,11 +56,10 @@ Mitogen for Ansible
 * `#332 <https://github.com/dw/mitogen/issues/332>`_: support a new
   :data:`sys.excepthook`-based module exit mechanism added in Ansible 2.6.
 
-* `#338 <https://github.com/dw/mitogen/issues/338>`_: compatibility: due to
+* `#338 <https://github.com/dw/mitogen/issues/338>`_: compatibility: changes to
-  Ansible's implementation, changes to ``/etc/environment`` made by a task are
+  ``/etc/environment`` and ``~/.pam_environment`` made by a task are reflected
-  reflected in the runtime environment of subsequent tasks, but only if those
+  in the runtime environment of subsequent tasks. See
-  tasks set ``become: true``, or if SSH multiplexing is disabled. Changes to
+  :ref:`ansible_process_env` for a complete description.
-  ``/etc/environment`` are now monitored and always reflected.
 
 * Runs with many targets executed the module dependency scanner redundantly
   due to missing synchronization, causing significant wasted computation in the

tests/ansible/integration/runner/etc_environment.yml

@@ -13,7 +13,7 @@
         path: ~/.pam_environment
         state: absent
 
-    - shell: echo $MAGIC_NEW_ENV
+    - shell: echo $MAGIC_PAM_ENV
       register: echo
 
     - assert:
@@ -22,9 +22,9 @@
     - copy:
         dest: ~/.pam_environment
         content: |
-          MAGIC_NEW_ENV=321
+          MAGIC_PAM_ENV=321
 
-    - shell: echo $MAGIC_NEW_ENV
+    - shell: echo $MAGIC_PAM_ENV
       register: echo
 
     - assert:
@@ -34,6 +34,12 @@
         path: ~/.pam_environment
         state: absent
 
+    - shell: echo $MAGIC_PAM_ENV
+      register: echo
+
+    - assert:
+        that: echo.stdout == ""
+
 
     # /etc/environment
     - meta: end_play
@@ -56,7 +62,7 @@
           MAGIC_ETC_ENV=555
       become: true
 
-    - shell: echo $MAGIC_ENV_ENV
+    - shell: echo $MAGIC_ETC_ENV
       register: echo
 
     - assert:
@@ -66,3 +72,9 @@
         path: /etc/environment
         state: absent
       become: true
+
+    - shell: echo $MAGIC_ETC_ENV
+      register: echo
+
+    - assert:
+        that: echo.stdout == ""