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

cmdmod: New group option for command execution & MacOS shell arg fix #43901

Merged
merged 1 commit into from Jan 5, 2018

Conversation

Projects
None yet
7 participants
@boltronics
Copy link
Contributor

commented Oct 4, 2017

What does this PR do?

Adds the group argument to various functions in cmdmod for non-Windows hosts. Also enables the shell option on OS X, which looked to have been silently ignored previously (although I don't have a Mac to test with).

What issues does this PR fix or reference?

#43900

Previous Behavior

The group option did not exist. The shell option was ignored on OS X.

New Behavior

The group option exists. The shell option should be honoured on OS X.

Tests written?

No

Please review Salt's Contributing Guide for best practices.

@boltronics boltronics changed the title Have cmdmod support a custom shell on MacOS Add group option to execute commands under a different grou Oct 4, 2017

@boltronics boltronics changed the title Add group option to execute commands under a different grou Add group option to execute commands under a different group Oct 4, 2017

@boltronics boltronics changed the title Add group option to execute commands under a different group cmdmod: New group option for command execution & MacOS shell arg fix Oct 4, 2017

@boltronics

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2017

Indecisive on the issue name. 😄

Here are some test runs on my local dev box:

(salt-develop) root@abolte-desktop:~# salt-call cmd.run 'echo (uname)' group=adm shell=/usr/bin/fish
local:
    Linux
(salt-develop) root@abolte-desktop:~#

This command should fail on other shells that are not fishy enough. 😉

(salt-develop) root@abolte-desktop:~# salt-call cmd.run 'echo (uname)' group=adm shell=/bin/bash
[ERROR   ] Command 'echo (uname)' failed with return code: 1
[ERROR   ] output: /bin/bash: -c: line 0: syntax error near unexpected token `uname'
/bin/bash: -c: line 0: `echo (uname)'
local:
    /bin/bash: -c: line 0: syntax error near unexpected token `uname'
    /bin/bash: -c: line 0: `echo (uname)'
(salt-develop) root@abolte-desktop:~#

Bash wasn't fishy enough.

(salt-develop) root@abolte-desktop:~# salt-call cmd.run 'echo (uname)' group=adm
[ERROR   ] Command 'echo (uname)' failed with return code: 1
[ERROR   ] output: /bin/bash: -c: line 0: syntax error near unexpected token `uname'
/bin/bash: -c: line 0: `echo (uname)'
local:
    /bin/bash: -c: line 0: syntax error near unexpected token `uname'
    /bin/bash: -c: line 0: `echo (uname)'
(salt-develop) root@abolte-desktop:~#

If the shell argument is not provided, internally sudo -i will be called instead of sudo -s /path/to/some/shell -c. It usually makes no difference as per the above run.

(salt-develop) root@abolte-desktop:~# salt-call cmd.run id group=adm
local:
    uid=0(root) gid=4(adm) groups=4(adm),0(root)
(salt-develop) root@abolte-desktop:~# salt-call cmd.run id runas=stunnel4 group=adm
sudo: unable to change directory to /var/run/stunnel4: No such file or directory
sudo: unable to execute /bin/false: No such file or directory
local:
    uid=133(stunnel4) gid=4(adm) groups=4(adm),145(stunnel4)
(salt-develop) root@abolte-desktop:~#

This shows a few more runs mixing runas and group arguments. The output is as expected.

(salt-develop) root@abolte-desktop:~# salt-call cmd.run 'touch /TEST' runas=root group=adm
local:
(salt-develop) root@abolte-desktop:~# ls -l /TEST
-rw-r--r-- 1 root adm 0 Oct  4 15:55 /TEST
(salt-develop) root@abolte-desktop:~#

Files created whilst running under a different group inherit the group ownership, which can be particularly useful in some situations.

(salt-develop) root@abolte-desktop:~# echo -e '#!/bin/bash\nid' > /tmp/test.sh
(salt-develop) root@abolte-desktop:~# chmod +x /tmp/test.sh 
(salt-develop) root@abolte-desktop:~# salt-call cmd.script '/tmp/test.sh' runas=mysql group=adm
sudo: unable to change directory to /nonexistent: No such file or directory
sudo: unable to execute /bin/false: No such file or directory
local:
    ----------
    pid:
        6672
    retcode:
        0
    stderr:
    stdout:
        uid=135(mysql) gid=4(adm) groups=4(adm),147(mysql)
(salt-develop) root@abolte-desktop:~#

This demonstrates the new group argument working for scripts as well.

@boltronics

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2017

Is that my bad? It's not obvious to me what the problem is (if it's something I did).

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

re-run py3

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

@boltronics I'm not sure, so I've restarted the py3 test to see if we get the same result. Offhand, it looks to me like the sub_minion test daemon didn't start, so let's see how the second run goes. :)

@garethgreenaway

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

There was a small merge conflict that I fixed by hand.

@thatch45
Copy link
Member

left a comment

looks good, did not miss any of the entry points and covers multi OS issues correctly. Nicely done @boltronics

@boltronics

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2017

Thanks all.

I note this PR has a conflict at this point. Looks trivial to fix though since it's just a matter of relocating the changes in chugid_and_umask() and chugid() over to their new home in salt/utils/user.py.

Normally I would just rebase against develop and force-push, but since everything has been approved at this point, I don't want to invalidate that. 😄 So should someone else be making that adjustment?

@boltronics

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2017

To clarify, I'm happy to make the changes myself if preferred. Whatever works. Please let me know how you wish to proceed.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2017

Hi @boltronics - If you could handle the rebasing, that would be great. If you have any questions, please let us know. Thank you!

@terminalmage
Copy link
Contributor

left a comment

Needs a rebase and a few changes.

@@ -22,6 +22,7 @@
import base64
import re
import tempfile
from distutils.spawn import find_executable

This comment has been minimized.

Copy link
@terminalmage

terminalmage Nov 6, 2017

Contributor

Please import this as a private function (i.e. from distutils.spawn import find_executable as _find_executable). Importing a function into the global namespace of the module will both a) cause it to be picked up by the loader (and thus end up in the __salt__ dunder dictionary), and b) add it to the docs when they are built using Sphinx.

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Nov 6, 2017

Contributor

Is there a reason we shouldn't just use salt.utils.path.which_bin?

This comment has been minimized.

Copy link
@boltronics

boltronics Nov 9, 2017

Author Contributor

Thanks for the tip. I hadn't noticed it.

if salt.utils.platform.is_windows():
msg = 'group is not currently available on Windows'
raise SaltInvocationError(msg)
if not find_executable('sudo'):

This comment has been minimized.

Copy link
@terminalmage

terminalmage Nov 6, 2017

Contributor

As with the comment on the import above, let's make sure this is a private function (i.e. _find_executable).

This comment has been minimized.

Copy link
@boltronics

boltronics Nov 9, 2017

Author Contributor

Good catch! But I'll try to switch this over to which_bin since that's already imported.

@@ -1732,6 +1732,106 @@ def appendproctitle(name):
setproctitle.setproctitle(setproctitle.getproctitle() + ' ' + name)


def chugid(runas, group=None):

This comment has been minimized.

Copy link
@terminalmage

terminalmage Nov 6, 2017

Contributor

It looks like these functions were added separately in a different PR and already exist in develop. However, they have been moved to salt/utils/user.py. Please check the functions you are adding here against their counterparts in that file and make any changes there. We should not be adding new functions to salt/utils/__init__.py.

This comment has been minimized.

Copy link
@boltronics

boltronics Nov 9, 2017

Author Contributor

Actually, they already existed when I originally submitted the PR but have since been moved. That was the original reason for needing to rebase after the initial approval, so will be sure to fix that up.

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Nov 13, 2017

@boltronics I asked @terminalmage to swing past here again today but we also do need a rebase here, please. There are a couple of merge conflicts. Thanks.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2017

@boltronics I'll re-review once you get that rebase in. I do like the idea of using which_bin as suggested by @gtmanfred.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

Hi @boltronics - Any chance you were able to come back to this?

@boltronics boltronics force-pushed the sitepoint:cmd_group_feature branch from 0b99ae2 to a78b082 Jan 3, 2018

@boltronics

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2018

@rallytime @terminalmage I think that's sorted now. Sorry it took so long.

@rallytime rallytime requested a review from terminalmage Jan 3, 2018

@terminalmage
Copy link
Contributor

left a comment

There's one minor thing here but it's not important enough to stand in the way of merging.

@@ -372,10 +375,11 @@ def _get_stripped(cmd):
# requested. The command output is what will be controlled by the
# 'loglevel' parameter.
msg = (
'Executing command {0}{1}{0} {2}in directory \'{3}\'{4}'.format(
u'Executing command {0}{1}{0} {2}{3}in directory \'{4}\'{5}'.format(

This comment has been minimized.

Copy link
@terminalmage

terminalmage Jan 4, 2018

Contributor

The u is unnecessary here since we're going to be using unicode_literals.

This comment has been minimized.

Copy link
@boltronics

boltronics Jan 4, 2018

Author Contributor

I should have noticed it wasn't in the original string (or rather, not the one there after rebasing). Sorry I didn't pick up on that. Fixed it anyway.

@boltronics boltronics force-pushed the sitepoint:cmd_group_feature branch from a78b082 to f867eff Jan 4, 2018

Add new group param to cmd execution methods
This has the side effect of having cmdmod support a custom shell on
MacOS.

@boltronics boltronics force-pushed the sitepoint:cmd_group_feature branch from f867eff to c574bff Jan 4, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2018

re-run py

@rallytime rallytime merged commit e0434bb into saltstack:develop Jan 5, 2018

4 of 9 checks passed

codeclimate 15 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #18378 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #5381 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #14936 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #20875 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #13340 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #17936 — SUCCESS
Details

@boltronics boltronics deleted the sitepoint:cmd_group_feature branch Jan 7, 2018

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