Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cmd.run on MacOS (rebased) #54136

Merged
merged 4 commits into from Aug 8, 2019

Conversation

@ScoreUnder
Copy link
Contributor

commented Aug 7, 2019

See #54079 -- this PR is the same, but with commits rebased onto the 2019.2.1 branch

ScoreUnder added 4 commits Jul 31, 2019
Also fix a unit test that was testing for broken behaviour.
- ensure runas user is created when used
- mark runas tests destructive due to user creation/deletion
- fix tmpdir permission issue when cd'ing as runas user
@dwoz

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

re-run full all

@dwoz
dwoz approved these changes Aug 8, 2019
@Akm0d
Akm0d approved these changes Aug 8, 2019
@dwoz dwoz merged commit bf863d0 into saltstack:2019.2.1 Aug 8, 2019
24 checks passed
24 checks passed
WIP Ready for review
Details
ci/docs This commit looks good
Details
ci/lint This commit looks good
Details
ci/py2/amazon2 This commit looks good
Details
ci/py2/centos6 This commit looks good
Details
ci/py2/centos7 This commit looks good
Details
ci/py2/centos7/tcp This commit looks good
Details
ci/py2/debian8 This commit looks good
Details
ci/py2/debian9 This commit looks good
Details
ci/py2/fedora29 This commit looks good
Details
ci/py2/ubuntu1604 This commit looks good
Details
ci/py2/ubuntu1604/tcp This commit looks good
Details
ci/py2/ubuntu1804 This commit looks good
Details
ci/py2/windows2016 This commit looks good
Details
ci/py3/amazon2 This commit looks good
Details
ci/py3/centos7 This commit looks good
Details
ci/py3/centos7/tcp This commit looks good
Details
ci/py3/debian8 This commit looks good
Details
ci/py3/debian9 This commit looks good
Details
ci/py3/fedora29 This commit looks good
Details
ci/py3/ubuntu1604 This commit looks good
Details
ci/py3/ubuntu1604/tcp This commit looks good
Details
ci/py3/ubuntu1804 This commit looks good
Details
ci/py3/windows2016 This commit looks good
Details
Copy link

left a comment

This is not valid for users with a default shell different than /bin/bash. It could be /usr/local/bin/bash or even /usr/local/bin/zsh.

As an example, when the following command is executed:

sudo salt-call --local cmd.run cmd='env' runas='Carlos'

this is the result with line cmd = '/bin/bash -l -c {cmd}'.format(cmd=_cmd_quote(cmd)) uncommented

local:
    /usr/local/share/bash-completion/bash_completion: line 1893: declare: -A: invalid option
    declare: usage: declare [-afFirtx] [-p] [name[=value] ...]
    /usr/local/share/bash-completion/bash_completion: line 2057: complete: -D: invalid option
    complete: usage: complete [-abcdefgjksuv] [-pr] [-o option] [-A action] [-G globpat] [-W wordlist] [-P prefix] [-S suffix] [-X filterpat] [-F function] [-C command] [name ...]
    ls: /Users/Carlos/.bashrc.d/*: No such file or directory
    MANPATH=/usr/share/man:/usr/local/share/man:/usr/local/man
    LESS_TERMCAP_md=
    TERM=xterm-256color                                                                                                                                                                                     SHELL=/usr/local/bin/zsh
    HISTSIZE=32768
    DISABLE_FZF_KEY_BINDINGS=false
    PROMPT_DIRTRIM=3
    LC_ALL=en_US.UTF-8
    HISTFILESIZE=32768
    USER=Carlos
    FZF_DEFAULT_OPTS=--ansi --height 100% --layout=reverse --border --inline-info
        --preview="bat --style=numbers --color=always {} 2> /dev/null"
        --bind ctrl-j:preview-page-down --bind ctrl-k:preview-page-up
        --bind ctrl-p:toggle-preview
    PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/Carlos/bin:/usr/local/sbin
    PWD=/tmp
    EDITOR=/usr/local/bin/mvim --remote-wait-silent
    LANG=en_US.UTF-8
    HISTCONTROL=ignoreboth
    GPG_TTY=/dev/ttys002
    SHLVL=1
    HOME=/Users/Carlos
    LOGNAME=Carlos
    FZF_CTRL_T_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    FZF_DEFAULT_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    DISABLE_FZF_AUTO_COMPLETION=false
    OLDPWD=/Users/Carlos
    _=/usr/bin/env

and this is the result with the same line commented:

local:
    USER=Carlos
    HOME=/Users/Carlos
    SHELL=/usr/local/bin/zsh
    PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/Carlos/bin:/usr/local/sbin
    TERM=xterm-256color
    LOGNAME=Carlos
    SHLVL=0
    PWD=/tmp
    OLDPWD=/Users/Carlos
    LANG=en_US.UTF-8
    LC_ALL=en_US.UTF-8
    MANPATH=/usr/share/man:/usr/local/share/man:/usr/local/man
    GPG_TTY=/dev/ttys002
    FZF_DEFAULT_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    FZF_DEFAULT_OPTS=--ansi --height 100% --layout=reverse --border --inline-info
        --preview="bat --style=numbers --color=always {} 2> /dev/null"
        --bind ctrl-j:preview-page-down --bind ctrl-k:preview-page-up
        --bind ctrl-p:toggle-preview
    FZF_CTRL_T_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    DISABLE_FZF_AUTO_COMPLETION=false
    DISABLE_FZF_KEY_BINDINGS=false
    _=/usr/bin/env

As you can see, with that line commented zsh stills getting the right environment, so a more specific solution is required for bash shells instead of making it the default solution.


# Ensure environment is correct for a newly logged-in user by running
# the command under bash as a login shell
cmd = '/bin/bash -l -c {cmd}'.format(cmd=_cmd_quote(cmd))

This comment has been minimized.

Copy link
@cdalvaro

cdalvaro Aug 8, 2019

This should take into account that the user default shell can be different than /bin/bash

This comment has been minimized.

Copy link
@ScoreUnder

ScoreUnder Aug 8, 2019

Author Contributor

I think this could be worked around by looking at __salt__['user.info'](runas)['shell'], though we may then run into a problem if we are trying to run a command as a user with /usr/bin/false or /sbin/nologin as their shell, or if they have a shell like fish which accepts a different syntax. (actually I think this has always been a problem for this module on macos)

For the record, my main goal in using bash -l is to get the environment into a better state than the one su -l leaves it in (which doesn't have /usr/local/bin in the path, for example), so I still consider this a fair improvement over the previous behaviour.

Would it be best to launch the command under bash/zsh depending on macos version, or under the user's default shell when it is not /bin/sh, or maybe is there a third option I haven't thought of?

This comment has been minimized.

Copy link
@ScoreUnder

ScoreUnder Aug 8, 2019

Author Contributor

Perhaps that mystery third option is to use zsh when the shell is zsh, and bash otherwise...

This comment has been minimized.

Copy link
@cdalvaro

cdalvaro Aug 8, 2019

It seems like zsh is working well just with su -l as you can see in my example above where the PATH is properly set and all my environment variables from .zshenv are available.

So maybe it is only necessary to check wether the user's default shell is bash and only if it is bash then add that line.

Also, an improvement for that line is to call the right bash executable since it could be /usr/local/bin/bash

@cdalvaro

This comment has been minimized.

Copy link

commented Aug 15, 2019

Although this PR is already merged, I would like to know if the issue related with a shell different than /bin/bash will be addressed in PR #54079.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.