x/vmware-nsx
Code Issues Proposed changes

Change daemon Pidfile class to not use root_helper

Browse Source 
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
This commit is contained in:
Brian Haley 2013-08-27 17:36:43 -04:00
parent 0304017da4
commit 3693c68e8b
8 changed files with 95 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
etc/neutron/rootwrap.d/dhcp.filters
View File

@ -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

3
etc/neutron/rootwrap.d/lbaas-haproxy.filters
View File

@ -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

16
neutron/agent/linux/daemon.py
View File

@ -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:

7
neutron/agent/linux/dhcp.py
View File

@ -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

7
neutron/agent/linux/external_process.py
View File

@ -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

27
neutron/tests/unit/test_linux_daemon.py
View File

@ -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):

40
neutron/tests/unit/test_linux_dhcp.py
View File

@ -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('__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)
self.execute.assert_called_once_with(['cat', '/proc/4/cmdline'],
'sudo')
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('__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)
self.execute.assert_called_once_with(['cat', '/proc/4/cmdline'],
'sudo')
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,7 +881,10 @@ 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
@ -883,14 +894,15 @@ tag:tag1,249,%s,%s""".lstrip() % (fake_v6,
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 = {}
with mock.patch.object(dhcp.Dnsmasq, method_name) as ipmap:
ipmap.return_value = {}
dm.reload_allocations()
self.assertTrue(ip_map.called)
self.assertTrue(ipmap.called)
self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data),
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')
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:

22
neutron/tests/unit/test_linux_external_process.py
View File

@ -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('__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)
self.execute.assert_called_once_with(['cat', '/proc/4/cmdline'],
'sudo')
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('__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)
self.execute.assert_called_once_with(['cat', '/proc/4/cmdline'],
'sudo')
mock_open.assert_called_once_with('/proc/4/cmdline', 'r')