From 0df884ac931c99f50ce020b18fd42a1b94d96481 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Mon, 23 Feb 2015 14:56:44 -0600 Subject: [PATCH] Add rootwrap daemon mode support This patch introduces support for rootwrap daemon mode. It adds a new config option, AGENT.root_helper_daemon with no default. To enable, set to something like: root_helper_daemon = sudo neutron-rootwrap-daemon /etc/neutron/rootwrap.conf The patch currently assumes that the root_helper_daemon value, and specifically the rootwrap config, will not change once calls to execute() happen. While it would not be hard to generate a rootwrap daemon client for each new config, I couldn't think of a legitimate reason to support it and left it out as YAGNI. This patch does change the behavior of the addl_env argument to create_process and execute. Previously, an environment dict would be passed to Popen. If a root helper was used, this environemnt would actually be passed to 'sudo' which would filter it before passing it to the underlying command. In the case of daemon mode, this would cause a problem as the enviornment is filtered by sudo only once, at daemon startup. Any environment variables added at execute time would then just be passed directly to the underyling command unfiltered. oslo.rootwrap 1.6.0 fixes this issue by denying the passing of environment variables to the daemon altogether. Instead, anything using rootwrap and needing to pass additional environment variables should define an EnvFilter and run the command with env var=val cmd. utils.execute/create_process have been modified to run code in this way (which netns.execute already did). No code in neutron currently uses both run_as_root=True and addl_env, so this change does not require any change in code or filters. DocImpact Implements: blueprint rootwrap-daemon-mode Change-Id: I567334bb611253c7b9d830d50c5be308a5153baf --- neutron/agent/common/config.py | 6 ++ neutron/agent/linux/utils.py | 87 ++++++++++++++----- .../tests/functional/agent/test_l3_agent.py | 2 +- neutron/tests/functional/base.py | 3 + neutron/tests/unit/test_agent_config.py | 7 ++ setup.cfg | 1 + tools/configure_for_func_testing.sh | 2 + tox.ini | 1 + 8 files changed, 85 insertions(+), 24 deletions(-) diff --git a/neutron/agent/common/config.py b/neutron/agent/common/config.py index 4862cffe9c0..950f951dd87 100644 --- a/neutron/agent/common/config.py +++ b/neutron/agent/common/config.py @@ -31,6 +31,12 @@ ROOT_HELPER_OPTS = [ default=True, help=_('Use the root helper to read the namespaces from ' 'the operating system.')), + # We can't just use root_helper=sudo neutron-rootwrap-daemon $cfg because + # it isn't appropriate for long-lived processes spawned with create_process + # Having a bool use_rootwrap_daemon option precludes specifying the + # rootwrap daemon command, which may be necessary for Xen? + cfg.StrOpt('root_helper_daemon', + help=_('Root helper daemon application to use when possible.')), ] AGENT_STATE_OPTS = [ diff --git a/neutron/agent/linux/utils.py b/neutron/agent/linux/utils.py index 6bf4bea09f1..3795522f00b 100644 --- a/neutron/agent/linux/utils.py +++ b/neutron/agent/linux/utils.py @@ -20,12 +20,14 @@ import shlex import socket import struct import tempfile +import threading import eventlet from eventlet.green import subprocess from eventlet import greenthread from oslo_config import cfg from oslo_log import log as logging +from oslo_rootwrap import client from oslo_utils import excutils from neutron.agent.common import config @@ -38,56 +40,95 @@ LOG = logging.getLogger(__name__) config.register_root_helper(cfg.CONF) +class RootwrapDaemonHelper(object): + __client = None + __lock = threading.Lock() + + def __new__(cls): + """There is no reason to instantiate this class""" + raise NotImplementedError() + + @classmethod + def get_client(cls): + with cls.__lock: + if cls.__client is None: + cls.__client = client.Client( + shlex.split(cfg.CONF.AGENT.root_helper_daemon)) + return cls.__client + + +def addl_env_args(addl_env): + """Build arugments for adding additional environment vars with env""" + + # NOTE (twilson) If using rootwrap, an EnvFilter should be set up for the + # command instead of a CommandFilter. + if addl_env is None: + return [] + return ['env'] + ['%s=%s' % pair for pair in addl_env.items()] + + def create_process(cmd, run_as_root=False, addl_env=None): """Create a process object for the given command. The return value will be a tuple of the process object and the list of command arguments used to create it. """ + cmd = map(str, addl_env_args(addl_env) + cmd) if run_as_root: cmd = shlex.split(config.get_root_helper(cfg.CONF)) + cmd - cmd = map(str, cmd) - LOG.debug("Running command: %s", cmd) - env = os.environ.copy() - if addl_env: - env.update(addl_env) - obj = utils.subprocess_popen(cmd, shell=False, stdin=subprocess.PIPE, stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - env=env) + stderr=subprocess.PIPE) return obj, cmd +def execute_rootwrap_daemon(cmd, process_input, addl_env): + cmd = map(str, addl_env_args(addl_env) + cmd) + # NOTE(twilson) oslo_rootwrap.daemon will raise on filter match + # errors, whereas oslo_rootwrap.cmd converts them to return codes. + # In practice, no neutron code should be trying to execute something that + # would throw those errors, and if it does it should be fixed as opposed to + # just logging the execution error. + LOG.debug("Running command (rootwrap daemon): %s", cmd) + client = RootwrapDaemonHelper.get_client() + return client.execute(cmd, process_input) + + def execute(cmd, process_input=None, addl_env=None, check_exit_code=True, return_stderr=False, log_fail_as_error=True, extra_ok_codes=None, run_as_root=False): try: - obj, cmd = create_process(cmd, run_as_root=run_as_root, - addl_env=addl_env) - _stdout, _stderr = obj.communicate(process_input) - obj.stdin.close() - m = _("\nCommand: %(cmd)s\nExit code: %(code)s\nStdin: %(stdin)s\n" - "Stdout: %(stdout)s\nStderr: %(stderr)s") % \ - {'cmd': cmd, - 'code': obj.returncode, - 'stdin': process_input or '', - 'stdout': _stdout, - 'stderr': _stderr} + if run_as_root and cfg.CONF.AGENT.root_helper_daemon: + returncode, _stdout, _stderr = ( + execute_rootwrap_daemon(cmd, process_input, addl_env)) + else: + obj, cmd = create_process(cmd, run_as_root=run_as_root, + addl_env=addl_env) + _stdout, _stderr = obj.communicate(process_input) + returncode = obj.returncode + obj.stdin.close() + + m = _("\nCommand: {cmd}\nExit code: {code}\nStdin: {stdin}\n" + "Stdout: {stdout}\nStderr: {stderr}").format( + cmd=cmd, + code=returncode, + stdin=process_input or '', + stdout=_stdout, + stderr=_stderr) extra_ok_codes = extra_ok_codes or [] - if obj.returncode and obj.returncode in extra_ok_codes: - obj.returncode = None + if returncode and returncode in extra_ok_codes: + returncode = None - if obj.returncode and log_fail_as_error: + if returncode and log_fail_as_error: LOG.error(m) else: LOG.debug(m) - if obj.returncode and check_exit_code: + if returncode and check_exit_code: raise RuntimeError(m) finally: # NOTE(termie): this appears to be necessary to let the subprocess diff --git a/neutron/tests/functional/agent/test_l3_agent.py b/neutron/tests/functional/agent/test_l3_agent.py index dc93a7394b6..8e81f9608b8 100755 --- a/neutron/tests/functional/agent/test_l3_agent.py +++ b/neutron/tests/functional/agent/test_l3_agent.py @@ -92,7 +92,7 @@ class L3AgentTestFramework(base.BaseOVSLinuxTestCase): get_temp_file_path('external/pids')) conf.set_override('host', host) agent = neutron_l3_agent.L3NATAgentWithStateReport(host, conf) - mock.patch.object(ip_lib, 'send_gratuitous_arp').start() + mock.patch.object(ip_lib, '_arping').start() return agent diff --git a/neutron/tests/functional/base.py b/neutron/tests/functional/base.py index c790f561583..95efa0c3ed6 100644 --- a/neutron/tests/functional/base.py +++ b/neutron/tests/functional/base.py @@ -56,3 +56,6 @@ class BaseSudoTestCase(base.BaseTestCase): config.register_root_helper(cfg.CONF) self.config(group='AGENT', root_helper=os.environ.get('OS_ROOTWRAP_CMD', SUDO_CMD)) + self.config(group='AGENT', + root_helper_daemon=os.environ.get( + 'OS_ROOTWRAP_DAEMON_CMD')) diff --git a/neutron/tests/unit/test_agent_config.py b/neutron/tests/unit/test_agent_config.py index cf717f66da4..b5a580efd24 100644 --- a/neutron/tests/unit/test_agent_config.py +++ b/neutron/tests/unit/test_agent_config.py @@ -34,3 +34,10 @@ class TestRootHelper(base.BaseTestCase): conf = config.setup_conf() config.register_root_helper(conf) self.assertEqual(config.get_root_helper(conf), 'sudo') + + def test_agent_root_helper_daemon(self): + conf = config.setup_conf() + config.register_root_helper(conf) + rhd = 'my_root_helper_daemon' + conf.set_override('root_helper_daemon', rhd, 'AGENT') + self.assertEqual(rhd, conf.AGENT.root_helper_daemon) diff --git a/setup.cfg b/setup.cfg index 44d7b4c1797..e5bd655dcf1 100644 --- a/setup.cfg +++ b/setup.cfg @@ -107,6 +107,7 @@ console_scripts = neutron-restproxy-agent = neutron.plugins.bigswitch.agent.restproxy_agent:main neutron-server = neutron.cmd.eventlet.server:main neutron-rootwrap = oslo_rootwrap.cmd:main + neutron-rootwrap-daemon = oslo_rootwrap.cmd:daemon neutron-usage-audit = neutron.cmd.usage_audit:main neutron-metering-agent = neutron.cmd.eventlet.services.metering_agent:main neutron-sriov-nic-agent = neutron.plugins.sriovnicagent.sriov_nic_agent:main diff --git a/tools/configure_for_func_testing.sh b/tools/configure_for_func_testing.sh index 38c1ffe5b24..39fb7484740 100755 --- a/tools/configure_for_func_testing.sh +++ b/tools/configure_for_func_testing.sh @@ -178,6 +178,7 @@ function _install_rootwrap_sudoers { VENV_NAME=${venv:-dsvm-functional} VENV_PATH=$NEUTRON_PATH/.tox/$VENV_NAME ROOTWRAP_SUDOER_CMD="$VENV_PATH/bin/neutron-rootwrap $VENV_PATH/etc/neutron/rootwrap.conf *" + ROOTWRAP_DAEMON_SUDOER_CMD="$VENV_PATH/bin/neutron-rootwrap-daemon $VENV_PATH/etc/neutron/rootwrap.conf" TEMPFILE=$(mktemp) cat << EOF > $TEMPFILE # A bug in oslo.rootwrap [1] prevents commands executed with 'ip netns @@ -194,6 +195,7 @@ function _install_rootwrap_sudoers { # Defaults:$STACK_USER secure_path="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:$VENV_PATH/bin" $STACK_USER ALL=(root) NOPASSWD: $ROOTWRAP_SUDOER_CMD +$STACK_USER ALL=(root) NOPASSWD: $ROOTWRAP_DAEMON_SUDOER_CMD EOF chmod 0440 $TEMPFILE sudo chown root:root $TEMPFILE diff --git a/tox.ini b/tox.ini index 229dc2f1fe8..fea55455954 100644 --- a/tox.ini +++ b/tox.ini @@ -39,6 +39,7 @@ deps = setenv = OS_TEST_PATH=./neutron/tests/functional OS_SUDO_TESTING=1 OS_ROOTWRAP_CMD=sudo {envbindir}/neutron-rootwrap {envdir}/etc/neutron/rootwrap.conf + OS_ROOTWRAP_DAEMON_CMD=sudo {envbindir}/neutron-rootwrap-daemon {envdir}/etc/neutron/rootwrap.conf OS_FAIL_ON_MISSING_DEPS=1 OS_TEST_TIMEOUT=90 sitepackages=True