Pass env_vars to pip.freeze

pip.installed state calls pip.freeze to check for existing installation
and to verify installation post-install. This patch propagates the
env_vars passed to pip.installed to the pip.freeze function. It also modifies
existing pip unit tests to test new functionality and adds an integration test
that verifies the expected correct behavior

Fixes #46127

salt/modules/pip.py

@@ -335,6 +335,21 @@ def _process_requirements(requirements, cmd, cwd, saltenv, user):
     return cleanup_requirements, None
 
 
+def _pass_env_vars_to_cmd(env_vars, cmd_kwargs):
+    '''
+    Process passed env_vars and add them to command env.
+    '''
+    if env_vars:
+        if isinstance(env_vars, dict):
+            for key, val in six.iteritems(env_vars):
+                if not isinstance(val, six.string_types):
+                    val = str(val)
+                cmd_kwargs.setdefault('env', {})[key] = val
+        else:
+            raise CommandExecutionError(
+                'env_vars {0} is not a dictionary'.format(env_vars))
+
+
 def install(pkgs=None,  # pylint: disable=R0912,R0913,R0914
             requirements=None,
             bin_env=None,
@@ -803,15 +818,7 @@ def install(pkgs=None,  # pylint: disable=R0912,R0913,R0914
 
     cmd_kwargs = dict(saltenv=saltenv, use_vt=use_vt, runas=user)
 
-    if env_vars:
-        if isinstance(env_vars, dict):
-            for key, val in six.iteritems(env_vars):
-                if not isinstance(val, six.string_types):
-                    val = str(val)
-                cmd_kwargs.setdefault('env', {})[key] = val
-        else:
-            raise CommandExecutionError(
-                'env_vars {0} is not a dictionary'.format(env_vars))
+    _pass_env_vars_to_cmd(env_vars, cmd_kwargs)
 
     try:
         if cwd:
@@ -966,7 +973,8 @@ def uninstall(pkgs=None,
 def freeze(bin_env=None,
            user=None,
            cwd=None,
-           use_vt=False):
+           use_vt=False,
+           env_vars=None):
     '''
     Return a list of installed packages either globally or in the specified
     virtualenv
@@ -1019,6 +1027,7 @@ def freeze(bin_env=None,
     cmd_kwargs = dict(runas=user, cwd=cwd, use_vt=use_vt, python_shell=False)
     if bin_env and os.path.isdir(bin_env):
         cmd_kwargs['env'] = {'VIRTUAL_ENV': bin_env}
+    _pass_env_vars_to_cmd(env_vars, cmd_kwargs)
     result = __salt__['cmd.run_all'](cmd, **cmd_kwargs)
 
     if result['retcode'] > 0:
@@ -1030,7 +1039,8 @@ def freeze(bin_env=None,
 def list_(prefix=None,
           bin_env=None,
           user=None,
-          cwd=None):
+          cwd=None,
+          env_vars=None):
     '''
     Filter list of installed apps from ``freeze`` and check to see if
     ``prefix`` exists in the list of packages installed.
@@ -1059,7 +1069,7 @@ def list_(prefix=None,
     if prefix is None or 'pip'.startswith(prefix):
         packages['pip'] = version(bin_env)
 
-    for line in freeze(bin_env=bin_env, user=user, cwd=cwd):
+    for line in freeze(bin_env=bin_env, user=user, cwd=cwd, env_vars=env_vars):
         if line.startswith('-f') or line.startswith('#'):
             # ignore -f line as it contains --find-links directory
             # ignore comment lines

salt/states/pip_state.py

@@ -182,8 +182,7 @@ def _check_pkg_version_format(pkg):
 
 def _check_if_installed(prefix, state_pkg_name, version_spec,
                         ignore_installed, force_reinstall,
-                        upgrade, user, cwd, bin_env):
-
+                        upgrade, user, cwd, bin_env, env_vars):
     # result: None means the command failed to run
     # result: True means the package is installed
     # result: False means the package is not installed
@@ -192,7 +191,8 @@ def _check_if_installed(prefix, state_pkg_name, version_spec,
     # Check if the requested package is already installed.
     try:
         pip_list = __salt__['pip.list'](prefix, bin_env=bin_env,
-                                        user=user, cwd=cwd)
+                                        user=user, cwd=cwd,
+                                        env_vars=env_vars)
         prefix_realname = _find_key(prefix, pip_list)
     except (CommandNotFoundError, CommandExecutionError) as err:
         ret['result'] = None
@@ -683,7 +683,7 @@ def installed(name,
                 version_spec = version_spec
                 out = _check_if_installed(prefix, state_pkg_name, version_spec,
                                           ignore_installed, force_reinstall,
-                                          upgrade, user, cwd, bin_env)
+                                          upgrade, user, cwd, bin_env, env_vars)
                 # If _check_if_installed result is None, something went wrong with
                 # the command running. This way we keep stateful output.
                 if out['result'] is None:
@@ -824,7 +824,8 @@ def installed(name,
                 # Case for packages that are not an URL
                 if prefix:
                     pipsearch = __salt__['pip.list'](prefix, bin_env,
-                                                     user=user, cwd=cwd)
+                                                     user=user, cwd=cwd,
+                                                     env_vars=env_vars)
 
                     # If we didnt find the package in the system after
                     # installing it report it

tests/integration/files/file/base/issue-46127-pip-env-vars.sls

@@ -0,0 +1,38 @@
+{%- set virtualenv_base = salt['runtests_helpers.get_salt_temp_dir_for_path']('virtualenv-12-base-1') -%}
+{%- set virtualenv_test = salt['runtests_helpers.get_salt_temp_dir_for_path']('issue-46127-pip-env-vars') -%}
+
+{{ virtualenv_base }}:
+  virtualenv.managed:
+    - system_site_packages: False
+    - distribute: True
+
+install_older_venv_1:
+  pip.installed:
+    - name: 'virtualenv < 13.0'
+    - bin_env: {{ virtualenv_base }}
+    - require:
+      - virtualenv: {{ virtualenv_base }}
+
+# For this test we need to make sure that the virtualenv used in the
+# 'issue-46127-setup' pip.installed state below was created using
+# virtualenv < 13.0. virtualenvs created using later versions make
+# packages with custom setuptools prefixes relative to the virtualenv
+# itself, which makes the use of env_vars obsolete.
+# Thus, the two states above ensure that the 'base' venv has
+# a version old enough to exhibit the behavior we want to test.
+
+setup_test_virtualenv_1:
+  cmd.run:
+    - name: {{ virtualenv_base }}/bin/virtualenv {{ virtualenv_test }}
+    - onchanges:
+      - pip: install_older_venv_1
+
+issue-46127-setup:
+  pip.installed:
+    - name: 'carbon < 1.1'
+    - no_deps: True
+    - env_vars:
+        PYTHONPATH: "/opt/graphite/lib/:/opt/graphite/webapp/"
+    - bin_env: {{ virtualenv_test }}
+    - require:
+      - cmd: setup_test_virtualenv_1

tests/integration/states/test_pip.py

@@ -522,3 +522,78 @@ class PipStateTest(ModuleCase, SaltReturnAssertsMixin):
         finally:
             if os.path.isdir(venv_dir):
                 shutil.rmtree(venv_dir)
+
+    def test_46127_pip_env_vars(self):
+        '''
+        Test that checks if env_vars passed to pip.installed are also passed
+        to pip.freeze while checking for existing installations
+        '''
+        # This issue is most easily checked while installing carbon
+        # Much of the code here comes from the test_weird_install function above
+        ographite = '/opt/graphite'
+        if os.path.isdir(ographite):
+            self.skipTest(
+                'You already have \'{0}\'. This test would overwrite this '
+                'directory'.format(ographite)
+            )
+        try:
+            os.makedirs(ographite)
+        except OSError as err:
+            if err.errno == errno.EACCES:
+                # Permission denied
+                self.skipTest(
+                    'You don\'t have the required permissions to run this test'
+                )
+        finally:
+            if os.path.isdir(ographite):
+                shutil.rmtree(ographite)
+
+        venv_dir = os.path.join(RUNTIME_VARS.TMP, 'issue-46127-pip-env-vars')
+        try:
+            # We may be able to remove this, I had to add it because the custom
+            # modules from the test suite weren't available in the jinja
+            # context when running the call to state.sls that comes after.
+            self.run_function('saltutil.sync_modules')
+            # Since we don't have the virtualenv created, pip.installed will
+            # thrown and error.
+            ret = self.run_function(
+                'state.sls', mods='issue-46127-pip-env-vars'
+            )
+            self.assertSaltTrueReturn(ret)
+            for key in six.iterkeys(ret):
+                self.assertTrue(ret[key]['result'])
+                if ret[key]['name'] != 'carbon < 1.1':
+                    continue
+                self.assertEqual(
+                    ret[key]['comment'],
+                    'All packages were successfully installed'
+                )
+                break
+            else:
+                raise Exception('Expected state did not run')
+            # Run the state again. Now the already installed message should
+            # appear
+            ret = self.run_function(
+                'state.sls', mods='issue-46127-pip-env-vars'
+            )
+            self.assertSaltTrueReturn(ret)
+            # We cannot use assertInSaltComment here because we need to skip
+            # some of the state return parts
+            for key in six.iterkeys(ret):
+                self.assertTrue(ret[key]['result'])
+                # As we are re-running the formula, some states will not be run
+                # and "name" may or may not be present, so we use .get() pattern
+                if ret[key].get('name', '') != 'carbon < 1.1':
+                    continue
+                self.assertEqual(
+                    ret[key]['comment'],
+                    ('Python package carbon < 1.1 was already installed\n'
+                     'All packages were successfully installed'))
+                break
+            else:
+                raise Exception('Expected state did not run')
+        finally:
+            if os.path.isdir(ographite):
+                shutil.rmtree(ographite)
+            if os.path.isdir(venv_dir):
+                shutil.rmtree(venv_dir)

tests/unit/modules/test_pip.py

@@ -938,6 +938,27 @@ class PipTestCase(TestCase, LoaderModuleMockMixin):
                 )
                 self.assertEqual(ret, eggs)
 
+        mock = MagicMock(
+            return_value={
+                'retcode': 0,
+                'stdout': '\n'.join(eggs)
+            }
+        )
+        # Passing env_vars passes them to underlying command?
+        with patch.dict(pip.__salt__, {'cmd.run_all': mock}):
+            with patch('salt.modules.pip.version',
+                       MagicMock(return_value='6.1.1')):
+                ret = pip.freeze(env_vars={"foo": "bar"})
+                mock.assert_called_once_with(
+                    ['pip', 'freeze'],
+                    cwd=None,
+                    runas=None,
+                    use_vt=False,
+                    python_shell=False,
+                    env={"foo": "bar"}
+                )
+                self.assertEqual(ret, eggs)
+
         # Non zero returncode raises exception?
         mock = MagicMock(return_value={'retcode': 1, 'stderr': 'CABOOOOMMM!'})
         with patch.dict(pip.__salt__, {'cmd.run_all': mock}):