[ovs-dev,6/8] ovs-dev.py: run operational commands as root
diff mbox

Message ID 1442969477-11026-6-git-send-email-azhou@nicira.com
State Accepted
Headers show

Commit Message

Andy Zhou Sept. 23, 2015, 12:51 a.m. UTC
Switch operational commands, run, kill, reset and modinst directly
or indirectly read and writes files within the RUNDIR. Currently
these commands run in the current user context, with some "sudo"
commands thrown in to ensure daemons such as ovs-vswichd will be
launched as root.

This approach works fine as long as ovs-dev.py is always
run as root, (but then the 'sudo' commands added are redundant).
When invoking ovs-dev.py as non-root, files in RUNDIR will be mixed
with root created file and non-root created files, making it confusing
to decide whether to run ovs-appctl as root or not. Multiple
invocations of ovs-dev.py as root or non-root causes permission issues
since the same file created by a different user may no longer be
accessible when user changes.

This patch improves the situation by always run those four operational
commands as root. When they are invoked as non-root, "sudo" will be
used automatically by re-run the command with sudo.  VARDIR will now
always be access as root. The next patch will add --user and -u option
to allow for downgrading to running all daemons as non-root.

Signed-off-by: Andy Zhou <azhou@nicira.com>
---
 utilities/ovs-dev.py | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Joe Stringer Oct. 1, 2015, 12:02 a.m. UTC | #1
On 22 September 2015 at 17:51, Andy Zhou <azhou@nicira.com> wrote:
> Switch operational commands, run, kill, reset and modinst directly
> or indirectly read and writes files within the RUNDIR. Currently
> these commands run in the current user context, with some "sudo"
> commands thrown in to ensure daemons such as ovs-vswichd will be
> launched as root.
>
> This approach works fine as long as ovs-dev.py is always
> run as root, (but then the 'sudo' commands added are redundant).
> When invoking ovs-dev.py as non-root, files in RUNDIR will be mixed
> with root created file and non-root created files, making it confusing
> to decide whether to run ovs-appctl as root or not. Multiple
> invocations of ovs-dev.py as root or non-root causes permission issues
> since the same file created by a different user may no longer be
> accessible when user changes.
>
> This patch improves the situation by always run those four operational
> commands as root. When they are invoked as non-root, "sudo" will be
> used automatically by re-run the command with sudo.  VARDIR will now
> always be access as root. The next patch will add --user and -u option
> to allow for downgrading to running all daemons as non-root.
>
> Signed-off-by: Andy Zhou <azhou@nicira.com>

Thanks for fixing this. I think I figured out fairly quick that
non-root usage was broken so naturally avoided using it as a common
user.

> @@ -202,8 +209,8 @@ def reset():
>          _sh("ovs-dpctl", "del-dp", dp.strip())
>  commands.append(reset)
>
> -
>  def run():
> +    sudo()
>      kill()
>      for d in ["log", "run"]:
>          d = "%s/%s" % (VARDIR, d)

Unrelated whitespace change. Generally pythonistas use pep8 to
style-check code, and it'll complain about this kind of thing.

Otherwise,
Acked-by: Joe Stringer <joestringer@nicira.com>

Patch
diff mbox

diff --git a/utilities/ovs-dev.py b/utilities/ovs-dev.py
index 82d946d..68c9a42 100755
--- a/utilities/ovs-dev.py
+++ b/utilities/ovs-dev.py
@@ -55,6 +55,11 @@  def uname():
     return _sh("uname", "-r", capture=True)[0].strip()
 
 
+def sudo():
+    if os.geteuid() != 0:
+        _sh(" ".join(["sudo"] + sys.argv), check=True)
+        sys.exit(0)
+
 def conf():
     tag()
 
@@ -186,15 +191,17 @@  commands.append(tag)
 
 
 def kill():
+    sudo()
     for proc in ["ovs-vswitchd", "ovsdb-server"]:
         if os.path.exists("%s/run/openvswitch/%s.pid" % (VARDIR, proc)):
             _sh("ovs-appctl", "-t", proc, "exit", check=False)
             time.sleep(.1)
-        _sh("sudo", "killall", "-q", "-2", proc, check=False)
+        _sh("killall", "-q", "-2", proc, check=False)
 commands.append(kill)
 
 
 def reset():
+    sudo()
     kill()
     if os.path.exists(VARDIR):
         shutil.rmtree(VARDIR)
@@ -202,8 +209,8 @@  def reset():
         _sh("ovs-dpctl", "del-dp", dp.strip())
 commands.append(reset)
 
-
 def run():
+    sudo()
     kill()
     for d in ["log", "run"]:
         d = "%s/%s" % (VARDIR, d)
@@ -257,7 +264,6 @@  def run():
                "--suppressions=%s/tests/glibc.supp" % OVS_SRC,
                "--suppressions=%s/tests/openssl.supp" % OVS_SRC] + cmd
     else:
-        cmd = ["sudo"] + cmd
         opts = opts + ["-vconsole:off", "--detach", "--enable-dummy"]
     _sh(*(cmd + opts))
 commands.append(run)
@@ -268,6 +274,7 @@  def modinst():
         print "Missing modules directory.  Is this a Linux system?"
         sys.exit(1)
 
+    sudo()
     try:
         _sh("rmmod", "openvswitch")
     except subprocess.CalledProcessError, e:
@@ -341,6 +348,10 @@  Commands:
     modinst - Build ovs and install the kernel module.
     env     - Print the required path environment variable.
     doc     - Print this message.
+
+Note:
+    If running as non-root user, "kill", "reset", "run" and "modinst"
+    will always run as the root user, by rerun the commands with "sudo".
 """ % {"ovs": OVS_SRC, "v": sys.argv[0], "run": VARDIR}
     sys.exit(0)
 commands.append(doc)