Salt: Minion should refresh environment variables

[bkeroack]

On Windows (and apparently Linux), the minion never refreshes any environment variables (such as PATH). So if an environment variables changes (for example, after software installation) the only way to pick up the change is to restart the salt-minion.

This should be fixed to refresh environment variables on the minion prior to each salt command.

[basepi]

I think "before each salt command" is overkill. That said, we should have a routine to do this easily.

[bkeroack]

I disagree.

If I install software with a state, I need subsequent states to deterministically know that they have access to the most recent environment variables immediately thereafter.

"Wait a little while and it might show up" is not a satisfactory response.

[basepi]

I guess it depends on how expensive the operation turns out to be. How often are you really modifying the environment variables? In Linux, the answer in most cases is "not very often". Perhaps Windows is different.

In any case, when we get the routine in there, we can add a config option for running it before every command.

[belawaeckerlig]

this feature would be really nice. running into the same problem!

[belawaeckerlig]

anything new about this? this is really frustrating because i can't compile some pip modules because mingw is not in PATH or use git or python without passing the whole path...

[basepi]

Nothing new that I'm aware of. We don't have any core devs focusing on this at the moment, as we have other higher priority issues to handle.

[gtmacdonald]

I would like this feature as well...

[cachedout]

This is an interesting request. There are a few things to keep in mind here, though.

Doing something like this effectively means that any change in an environment variable would need to make its way back up to the parent minion process so that it could be used in the creation of processes further down the line. To do this, we'd have to have child processes that could somehow communicate their changes back up to their parents. We'd have to deal with conflicts, (what happens if two children want to change the same variable?) There are issues with inter-children communication because if one changes an env, the other one would need to (somehow) hear about it. There are also potential race conditions.

If somebody would like to take on this challenge, we would certainly look at a pull-request but it would need to be carefully crafted to avoid potential security issues and it would need to deal with the above concerns.

I think this request might be better served by understanding a little more about the particular use cases that are breaking and seeing if there might be other ways to address those issues.

[gtmacdonald]

My use case is, I install a set of tools that add themselves to the system path, then try and use those tools in a subsequent shell command through the minion. This isn't a very high priority but it would be nice. Why not wait for registry change notifications for the system and user environment variables? And when they change update the local environment?

[gtmacdonald]

On windows you could gather a new environment from the registry and use that when calling popen instead of defaulting the parent process' current environment.

[ghostsquad]

@gtmacdonald - a workaround - restart the minion after states that update the environment take place, but before the calls to those tools happen. There's some problems with that, like concurrent jobs, and preventing child jobs from terminating prematurely, etc.

[bkeroackdsc]

@ghostsquad Not a great workaround because (IIRC), restarting the minion within a state run either causes the state run to fail or causes it to hang indefinitely.

[ghostsquad]

@bkeroack - what version of salt?

[famousgarkin]

+1 Can't get NPM installed and then used in a single run.

[m03]

@famousgarkin, To avoid restarting the minion, you could use the environ.setenv state with update_minion set to True in order to propagate the change to the current running minion. As an example for adding Git to the Windows System Path:

{% set git_dir = 'C:\Program Files\Git\cmd' %}
{% set env_path = salt['environ.get']('PATH') %}
{% set env_path = env_path ~ ';' ~ git_dir %}

win-path-exists-git:
  win_path.exists:
    - name: {{ git_dir }}

environ-setenv-path-git:
  environ.setenv:
    - name: PATH
    - value: {{ env_path }}
    - update_minion: True
    - onchanges:
      - win_path: win-path-exists-git

[are-prabhu]

@m03 It work with root $PATH, Im still experiencing this issue with non-root user do we have any fix or workaround

[damon-atkins]

Only way I can think of doing this is to add path to the minion configuration files. Then the minion checks minion configuration files for changes (time stamp) it reloads the configuration file settings.

See also #32806

[kedare]

I confirm this is required on Windows...
Typically when I bootstrap my systems with SaltStack I need to restart it at least twice because it doesn't get the new PATH variable, for example first after installing chocolatey, then after installing git using chocolatey. because they each install in different path and update the PATH variables that salt-minion doesn't see until restarting

[damon-atkins]

@kedare See https://support.microsoft.com/en-us/help/104011/how-to-propagate-environment-variables-to-the-system or setx should do the same thing ( you could use setx afterwords to set any variable even one which does not matter).

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[basepi]

@saltstack/team-triage

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[nils-ballmann]

Chocolatey has the command refreshenv which (at least on Windows) does an environment refresh for pwsh and cmd.exe. Maybe this a good starting point?

[xxsim3lu]

For Windows minion, I created a custom module as a workaround to update the minion PATH environment variables.

Here's the code snippet:

from __future__ import absolute_import
import logging
import salt.utils.platform
import re

log = logging.getLogger(__name__)

__virtualname__ = 'myenviron'

def __virtual__():
  if salt.utils.platform.is_windows():
    return __virtualname__

def refresh():
  ret = {}
  # ====== Get the environment from registry ====== 
  cmd = 'reg query "HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\Environment"'
  res = __salt__['cmd.run'](cmd=cmd)
  tmp = res.splitlines()
  environ = {}
  for item in tmp:
    val = item.strip().split('    ')
    if len(val) == 3:
      environ.update({val[0].strip().upper():val[2].strip()})

  # ====== Resolve the environment values and replace them ======
  # ====== e.g. %systemroot% to 'C:\Windows' ======
  for key, value in environ.items():
    matches = re.findall(r'%(.+?)%', value)
    match = set([i.upper() for i in matches])
    for item in match:
      a = environ.get(item)
      if not a:
        a = __salt__['environ.get'](item)

      if a:
        redata = re.compile(re.escape('%{}%'.format(item)), re.IGNORECASE)
        environ[key] = redata.sub(a, value)

  # ====== Only set the PATH environment ====== 
  setenviron = {'PATH':environ.get('PATH')}
  ret = __salt__['environ.setenv'](environ=setenviron,update_minion=True)
  return ret

With this custom module, after installation of Windows packages that changes the PATH variables, by running sudo salt -G 'os:Windows' myenviron.refresh;
I am able to run command with cmd.run such as cmd.run 'java -version' from salt master.

Note: Windows package is installed with SaltStack Windows package manager instead of Chocolatey.

[jayvdb]

Im not sure if this will help salt, but there is a need for Chocolatey to have a sh-compatible refreshenv . See https://github.com/chocolatey/choco/issues/1851 .

[onnodb]

For Windows minion, I created a custom module as a workaround to update the minion PATH environment variables.

For me, the custom module didn't quite work (Windows minion, Salt 2019.2.2 PY3). My use case is installing a package and then calling one of its executables, within the same job run. So I'd like to install program X, add X's directory to the PATH, and then call it using the cmd.run state:

Install program X:
  pkg.installed:
    - name: X
    - version: 1.0
  win.path.exists:
    - name: 'C:\Program Files\X'
  module.run:
    - name: win_minion_env.refresh    # xxsim3lu's custom module

Run program X:
  cmd.run:
    - name: 'x'

What I noticed, is that xxsim3lu's module only works if I run the highstate twice. So, somehow, the changes to the environment do not get propagated to the process that's applying the highstate, but only to the next job process?

After some digging in the code of salt.modules.cmdmod, I noticed that its _run method pulls the environment from nt.environ. So we can amend xxsim3lu's code with a few extra lines, which do seem to make things work:

def refresh():
    # ...

    # Only set the PATH environment
    setenviron = {'PATH': environ.get('PATH')}
    ret = __salt__['environ.setenv'](environ=setenviron, update_minion=True)

    # NEW LINES:
    import nt
    for k, v in setenviron.items():
        nt.environ[k] = v
    # END OF NEW LINES

    return ret

Note that, due to cmdmod's curious use of nt.environ instead of os.environ, I really need to use nt.environ in the code above to make it work.

More generally, I'm puzzled by how difficult it is to get environment changes propagated. Maybe it is a Windows-specific issue, but even so, it's been incredibly frustrating to get this to work. (The obvious environ.setenv with update_minion=True doesn't work on a Windows minion.)

EDIT: My bad, the above actually causes _another_ problem: it drops the directories that Salt itself added to the PATH (such as the directory containing Salt executables), and therefore corrupts the Minion's environment. Instead, the new lines above should be merging the new and old PATH, or appending them.

[muddman]

I agree completely -- it's way too difficult to get this working. I don't think it's a windows thing, I think it's a lack of consistency, documentation and examples with saltstack.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[zbarr]

Unmark as stale - running into this issue right now.