Change daemon Pidfile class to not use root_helper

Some users of the Pidfile class don't specify root_helper,
which then defaults to 'sudo', which will generate an error.
Remove root_helper altogether since we actually don't need
root priveleges to read /proc/$pid/cmdline.

Changed code to use open.readline() instead of a shell, and
tweaked tests accordingly.

Also cleaned-up the rootwrap filters that allow it as they
are not used anymore.

Fixes bug 1218142

Change-Id: I6691feb1c9f7bfa261a7ec464fd8f3f92168c302

diff --git a/etc/neutron/rootwrap.d/dhcp.filters b/etc/neutron/rootwrap.d/dhcp.filters
index 26e2e56643beaccf4e364b747e1dd7123cbe394d..02e091375a10a1fb4abefcc4e68e444c0e13d9b3 100644 (file)
--- a/etc/neutron/rootwrap.d/dhcp.filters
+++ b/etc/neutron/rootwrap.d/dhcp.filters
@@ -16,8 +16,6 @@ dnsmasq: EnvFilter, dnsmasq, root, NEUTRON_NETWORK_ID=
 kill_dnsmasq: KillFilter, root, /sbin/dnsmasq, -9, -HUP
 kill_dnsmasq_usr: KillFilter, root, /usr/sbin/dnsmasq, -9, -HUP
 
-# dhcp-agent uses cat
-cat: RegExpFilter, cat, root, cat, /proc/\d+/cmdline
 ovs-vsctl: CommandFilter, ovs-vsctl, root
 ivs-ctl: CommandFilter, ivs-ctl, root
 dhcp_release: CommandFilter, dhcp_release, root

diff --git a/etc/neutron/rootwrap.d/lbaas-haproxy.filters b/etc/neutron/rootwrap.d/lbaas-haproxy.filters
index 4475604022bd0fb97295c4aab5f4ce9906d2a452..aaf6be399f8ee924f297bd97060bf5e5f8d55e7c 100644 (file)
--- a/etc/neutron/rootwrap.d/lbaas-haproxy.filters
+++ b/etc/neutron/rootwrap.d/lbaas-haproxy.filters
@@ -14,9 +14,6 @@ haproxy: CommandFilter, haproxy, root
 # lbaas-agent uses kill as well, that's handled by the generic KillFilter
 kill_haproxy_usr: KillFilter, root, /usr/sbin/haproxy, -9, -HUP
 
-# lbaas-agent uses cat
-cat: RegExpFilter, cat, root, cat, /proc/\d+/cmdline
-
 ovs-vsctl: CommandFilter, ovs-vsctl, root
 
 # ip_lib

diff --git a/neutron/agent/linux/daemon.py b/neutron/agent/linux/daemon.py
index 2825679a3a7f38b6ea6acfa4452fc200b1e94af3..d1118674e03dc6588217b2ebd88bd1271e54c174 100644 (file)
--- a/neutron/agent/linux/daemon.py
+++ b/neutron/agent/linux/daemon.py
@@ -21,14 +21,13 @@ import fcntl
 import os
 import sys
 
-from neutron.agent.linux import utils
 from neutron.openstack.common import log as logging
 
 LOG = logging.getLogger(__name__)
 
 
 class Pidfile(object):
-    def __init__(self, pidfile, procname, uuid=None, root_helper='sudo'):
+    def __init__(self, pidfile, procname, uuid=None):
         try:
             self.fd = os.open(pidfile, os.O_CREAT | os.O_RDWR)
         except IOError:
@@ -37,7 +36,6 @@ class Pidfile(object):
         self.pidfile = pidfile
         self.procname = procname
         self.uuid = uuid
-        self.root_helper = root_helper
         if not not fcntl.flock(self.fd, fcntl.LOCK_EX):
             raise IOError(_('Unable to lock pid file'))
 
@@ -66,12 +64,13 @@ class Pidfile(object):
         if not pid:
             return False
 
-        cmd = ['cat', '/proc/%s/cmdline' % pid]
+        cmdline = '/proc/%s/cmdline' % pid
         try:
-            exec_out = utils.execute(cmd, self.root_helper)
+            with open(cmdline, "r") as f:
+                exec_out = f.readline()
             return self.procname in exec_out and (not self.uuid or
                                                   self.uuid in exec_out)
-        except RuntimeError:
+        except IOError:
             return False
 
 
@@ -81,13 +80,12 @@ class Daemon(object):
     Usage: subclass the Daemon class and override the run() method
     """
     def __init__(self, pidfile, stdin='/dev/null', stdout='/dev/null',
-                 stderr='/dev/null', procname='python', uuid=None,
-                 root_helper='sudo'):
+                 stderr='/dev/null', procname='python', uuid=None):
         self.stdin = stdin
         self.stdout = stdout
         self.stderr = stderr
         self.procname = procname
-        self.pidfile = Pidfile(pidfile, procname, uuid, root_helper)
+        self.pidfile = Pidfile(pidfile, procname, uuid)
 
     def _fork(self):
         try:

diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py
index 86e2b52301fe348a20fb1b307bcbcf54d40e7dbb..5b320f6a6c616ed7e33acabc8038a1ef878865d5 100644 (file)
--- a/neutron/agent/linux/dhcp.py
+++ b/neutron/agent/linux/dhcp.py
@@ -228,10 +228,11 @@ class DhcpLocalProcess(DhcpBase):
         if pid is None:
             return False
 
-        cmd = ['cat', '/proc/%s/cmdline' % pid]
+        cmdline = '/proc/%s/cmdline' % pid
         try:
-            return self.network.id in utils.execute(cmd, self.root_helper)
-        except RuntimeError:
+            with open(cmdline, "r") as f:
+                return self.network.id in f.readline()
+        except IOError:
             return False
 
     @property

diff --git a/neutron/agent/linux/external_process.py b/neutron/agent/linux/external_process.py
index 1748c6c4eddd15a53b754518c1864ff17f54d338..9c62f2f87be03daaf49e529940e04f73c2f8fcba 100644 (file)
--- a/neutron/agent/linux/external_process.py
+++ b/neutron/agent/linux/external_process.py
@@ -100,8 +100,9 @@ class ProcessManager(object):
         if pid is None:
             return False
 
-        cmd = ['cat', '/proc/%s/cmdline' % pid]
+        cmdline = '/proc/%s/cmdline' % pid
         try:
-            return self.uuid in utils.execute(cmd, self.root_helper)
-        except RuntimeError:
+            with open(cmdline, "r") as f:
+                return self.uuid in f.readline()
+        except IOError:
             return False

diff --git a/neutron/tests/unit/test_linux_daemon.py b/neutron/tests/unit/test_linux_daemon.py
index 2fc27a4aab1d0c86b7747d0df6c728f41cb78d87..6f4782bf2d1c5b5c1932302d93fbdf899a5800f2 100644 (file)
--- a/neutron/tests/unit/test_linux_daemon.py
+++ b/neutron/tests/unit/test_linux_daemon.py
@@ -84,40 +84,43 @@ class TestPidfile(base.BaseTestCase):
         self.assertEqual(34, p.read())
 
     def test_is_running(self):
-        with mock.patch('neutron.agent.linux.utils.execute') as execute:
-            execute.return_value = 'python'
+        with mock.patch('__builtin__.open') as mock_open:
             p = daemon.Pidfile('thefile', 'python')
+            mock_open.return_value.__enter__ = lambda s: s
+            mock_open.return_value.__exit__ = mock.Mock()
+            mock_open.return_value.readline.return_value = 'python'
 
             with mock.patch.object(p, 'read') as read:
                 read.return_value = 34
                 self.assertTrue(p.is_running())
 
-            execute.assert_called_once_with(
-                ['cat', '/proc/34/cmdline'], 'sudo')
+            mock_open.assert_called_once_with('/proc/34/cmdline', 'r')
 
     def test_is_running_uuid_true(self):
-        with mock.patch('neutron.agent.linux.utils.execute') as execute:
-            execute.return_value = 'python 1234'
+        with mock.patch('__builtin__.open') as mock_open:
             p = daemon.Pidfile('thefile', 'python', uuid='1234')
+            mock_open.return_value.__enter__ = lambda s: s
+            mock_open.return_value.__exit__ = mock.Mock()
+            mock_open.return_value.readline.return_value = 'python 1234'
 
             with mock.patch.object(p, 'read') as read:
                 read.return_value = 34
                 self.assertTrue(p.is_running())
 
-            execute.assert_called_once_with(
-                ['cat', '/proc/34/cmdline'], 'sudo')
+            mock_open.assert_called_once_with('/proc/34/cmdline', 'r')
 
     def test_is_running_uuid_false(self):
-        with mock.patch('neutron.agent.linux.utils.execute') as execute:
-            execute.return_value = 'python 1234'
+        with mock.patch('__builtin__.open') as mock_open:
             p = daemon.Pidfile('thefile', 'python', uuid='6789')
+            mock_open.return_value.__enter__ = lambda s: s
+            mock_open.return_value.__exit__ = mock.Mock()
+            mock_open.return_value.readline.return_value = 'python 1234'
 
             with mock.patch.object(p, 'read') as read:
                 read.return_value = 34
                 self.assertFalse(p.is_running())
 
-            execute.assert_called_once_with(
-                ['cat', '/proc/34/cmdline'], 'sudo')
+            mock_open.assert_called_once_with('/proc/34/cmdline', 'r')
 
 
 class TestDaemon(base.BaseTestCase):

diff --git a/neutron/tests/unit/test_linux_dhcp.py b/neutron/tests/unit/test_linux_dhcp.py
index 34f16267fa5a5735dbc8ed3ed5f8b88c8238dc5d..d6924902e3f45308328e88278d237a1377cc389a 100644 (file)
--- a/neutron/tests/unit/test_linux_dhcp.py
+++ b/neutron/tests/unit/test_linux_dhcp.py
@@ -363,14 +363,18 @@ class TestDhcpBase(TestBase):
 
 class TestDhcpLocalProcess(TestBase):
     def test_active(self):
-        dummy_cmd_line = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'
-        self.execute.return_value = (dummy_cmd_line, '')
-        with mock.patch.object(LocalChild, 'pid') as pid:
-            pid.__get__ = mock.Mock(return_value=4)
-            lp = LocalChild(self.conf, FakeV4Network())
-            self.assertTrue(lp.active)
-            self.execute.assert_called_once_with(['cat', '/proc/4/cmdline'],
-                                                 'sudo')
+        with mock.patch('__builtin__.open') as mock_open:
+            mock_open.return_value.__enter__ = lambda s: s
+            mock_open.return_value.__exit__ = mock.Mock()
+            mock_open.return_value.readline.return_value = \
+                'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'
+
+            with mock.patch.object(LocalChild, 'pid') as pid:
+                pid.__get__ = mock.Mock(return_value=4)
+                lp = LocalChild(self.conf, FakeV4Network())
+                self.assertTrue(lp.active)
+
+            mock_open.assert_called_once_with('/proc/4/cmdline', 'r')
 
     def test_active_none(self):
         dummy_cmd_line = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'
@@ -381,14 +385,18 @@ class TestDhcpLocalProcess(TestBase):
             self.assertFalse(lp.active)
 
     def test_active_cmd_mismatch(self):
-        dummy_cmd_line = 'bbbbbbbb-bbbb-bbbb-aaaa-aaaaaaaaaaaa'
-        self.execute.return_value = (dummy_cmd_line, '')
-        with mock.patch.object(LocalChild, 'pid') as pid:
-            pid.__get__ = mock.Mock(return_value=4)
-            lp = LocalChild(self.conf, FakeV4Network())
-            self.assertFalse(lp.active)
-            self.execute.assert_called_once_with(['cat', '/proc/4/cmdline'],
-                                                 'sudo')
+        with mock.patch('__builtin__.open') as mock_open:
+            mock_open.return_value.__enter__ = lambda s: s
+            mock_open.return_value.__exit__ = mock.Mock()
+            mock_open.return_value.readline.return_value = \
+                'bbbbbbbb-bbbb-bbbb-aaaa-aaaaaaaaaaaa'
+
+            with mock.patch.object(LocalChild, 'pid') as pid:
+                pid.__get__ = mock.Mock(return_value=4)
+                lp = LocalChild(self.conf, FakeV4Network())
+                self.assertFalse(lp.active)
+
+            mock_open.assert_called_once_with('/proc/4/cmdline', 'r')
 
     def test_get_conf_file_name(self):
         tpl = '/dhcp/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/dev'
@@ -873,24 +881,28 @@ tag:tag1,249,%s,%s""".lstrip() % (fake_v6,
                                   fake_v6_cidr, fake_v6,
                                   fake_v6_cidr, fake_v6)
 
-        exp_args = ['cat', '/proc/5/cmdline']
+        with mock.patch('__builtin__.open') as mock_open:
+            mock_open.return_value.__enter__ = lambda s: s
+            mock_open.return_value.__exit__ = mock.Mock()
+            mock_open.return_value.readline.return_value = None
 
-        with mock.patch('os.path.isdir') as isdir:
-            isdir.return_value = True
-            with mock.patch.object(dhcp.Dnsmasq, 'pid') as pid:
-                pid.__get__ = mock.Mock(return_value=5)
-                dm = dhcp.Dnsmasq(self.conf, FakeDualNetwork(),
-                                  version=float(2.59))
+            with mock.patch('os.path.isdir') as isdir:
+                isdir.return_value = True
+                with mock.patch.object(dhcp.Dnsmasq, 'pid') as pid:
+                    pid.__get__ = mock.Mock(return_value=5)
+                    dm = dhcp.Dnsmasq(self.conf, FakeDualNetwork(),
+                                      version=float(2.59))
 
-                method_name = '_make_subnet_interface_ip_map'
-                with mock.patch.object(dhcp.Dnsmasq, method_name) as ip_map:
-                    ip_map.return_value = {}
-                    dm.reload_allocations()
-                    self.assertTrue(ip_map.called)
+                    method_name = '_make_subnet_interface_ip_map'
+                    with mock.patch.object(dhcp.Dnsmasq, method_name) as ipmap:
+                        ipmap.return_value = {}
+                        dm.reload_allocations()
+                        self.assertTrue(ipmap.called)
 
-        self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data),
-                                    mock.call(exp_opt_name, exp_opt_data)])
-        self.execute.assert_called_once_with(exp_args, 'sudo')
+            self.safe.assert_has_calls([mock.call(exp_host_name,
+                                                  exp_host_data),
+                                        mock.call(exp_opt_name, exp_opt_data)])
+            mock_open.assert_called_once_with('/proc/5/cmdline', 'r')
 
     def test_make_subnet_interface_ip_map(self):
         with mock.patch('neutron.agent.linux.ip_lib.IPDevice') as ip_dev:

diff --git a/neutron/tests/unit/test_linux_external_process.py b/neutron/tests/unit/test_linux_external_process.py
index 452827460b6c9880ab3d5d9aaf796666f7b356e5..081aabe16aacb09e0990806bead58143193f875c 100644 (file)
--- a/neutron/tests/unit/test_linux_external_process.py
+++ b/neutron/tests/unit/test_linux_external_process.py
@@ -167,14 +167,17 @@ class TestProcessManager(base.BaseTestCase):
             self.assertIsNone(manager.pid)
 
     def test_active(self):
-        dummy_cmd_line = 'python foo --router_id=uuid'
-        self.execute.return_value = dummy_cmd_line
-        with mock.patch.object(ep.ProcessManager, 'pid') as pid:
-            pid.__get__ = mock.Mock(return_value=4)
-            manager = ep.ProcessManager(self.conf, 'uuid')
-            self.assertTrue(manager.active)
-            self.execute.assert_called_once_with(['cat', '/proc/4/cmdline'],
-                                                 'sudo')
+        with mock.patch('__builtin__.open') as mock_open:
+            mock_open.return_value.__enter__ = lambda s: s
+            mock_open.return_value.__exit__ = mock.Mock()
+            mock_open.return_value.readline.return_value = \
+                'python foo --router_id=uuid'
+            with mock.patch.object(ep.ProcessManager, 'pid') as pid:
+                pid.__get__ = mock.Mock(return_value=4)
+                manager = ep.ProcessManager(self.conf, 'uuid')
+                self.assertTrue(manager.active)
+
+            mock_open.assert_called_once_with('/proc/4/cmdline', 'r')
 
     def test_active_none(self):
         dummy_cmd_line = 'python foo --router_id=uuid'
@@ -185,11 +188,14 @@ class TestProcessManager(base.BaseTestCase):
             self.assertFalse(manager.active)
 
     def test_active_cmd_mismatch(self):
-        dummy_cmd_line = 'python foo --router_id=anotherid'
-        self.execute.return_value = dummy_cmd_line
-        with mock.patch.object(ep.ProcessManager, 'pid') as pid:
-            pid.__get__ = mock.Mock(return_value=4)
-            manager = ep.ProcessManager(self.conf, 'uuid')
-            self.assertFalse(manager.active)
-            self.execute.assert_called_once_with(['cat', '/proc/4/cmdline'],
-                                                 'sudo')
+        with mock.patch('__builtin__.open') as mock_open:
+            mock_open.return_value.__enter__ = lambda s: s
+            mock_open.return_value.__exit__ = mock.Mock()
+            mock_open.return_value.readline.return_value = \
+                'python foo --router_id=anotherid'
+            with mock.patch.object(ep.ProcessManager, 'pid') as pid:
+                pid.__get__ = mock.Mock(return_value=4)
+                manager = ep.ProcessManager(self.conf, 'uuid')
+                self.assertFalse(manager.active)
+
+            mock_open.assert_called_once_with('/proc/4/cmdline', 'r')