From f40fba7b9c160ccc5091d6a0d15e82a2c34170d7 Mon Sep 17 00:00:00 2001 From: Michael Gundlach Date: Tue, 21 Sep 2010 16:57:08 -0400 Subject: [PATCH 01/11] Rewrite rbac tests to use Authorizer middleware --- nova/auth/manager.py | 4 +- nova/tests/access_unittest.py | 108 +++++++++++++--------------------- 2 files changed, 44 insertions(+), 68 deletions(-) diff --git a/nova/auth/manager.py b/nova/auth/manager.py index bc3a8a12..928e0fd6 100644 --- a/nova/auth/manager.py +++ b/nova/auth/manager.py @@ -44,7 +44,7 @@ flags.DEFINE_list('allowed_roles', # NOTE(vish): a user with one of these roles will be a superuser and # have access to all api commands flags.DEFINE_list('superuser_roles', ['cloudadmin'], - 'Roles that ignore rbac checking completely') + 'Roles that ignore authorization checking completely') # NOTE(vish): a user with one of these roles will have it for every # project, even if he or she is not a member of the project @@ -304,7 +304,7 @@ class AuthManager(object): return "%s:%s" % (user.access, Project.safe_id(project)) def is_superuser(self, user): - """Checks for superuser status, allowing user to bypass rbac + """Checks for superuser status, allowing user to bypass authorization @type user: User or uid @param user: User to check. diff --git a/nova/tests/access_unittest.py b/nova/tests/access_unittest.py index 59e1683d..d85f559f 100644 --- a/nova/tests/access_unittest.py +++ b/nova/tests/access_unittest.py @@ -18,12 +18,13 @@ import unittest import logging +import webob from nova import exception from nova import flags from nova import test +from nova.api import ec2 from nova.auth import manager -from nova.auth import rbac FLAGS = flags.FLAGS @@ -72,9 +73,14 @@ class AccessTestCase(test.BaseTestCase): try: self.project.add_role(self.testsys, 'sysadmin') except: pass - self.context = Context() - self.context.project = self.project #user is set in each test + self.mw = ec2.Authorizer(lambda x,y: y('200 OK', []) and '') + self.mw.action_roles = {'str': { + '_allow_all': ['all'], + '_allow_none': [], + '_allow_project_manager': ['projectmanager'], + '_allow_sys_and_net': ['sysadmin', 'netadmin'], + '_allow_sysadmin': ['sysadmin']}} def tearDown(self): um = manager.AuthManager() @@ -87,76 +93,46 @@ class AccessTestCase(test.BaseTestCase): um.delete_user('testsys') super(AccessTestCase, self).tearDown() + def response_status(self, user, methodName): + context = Context() + context.project = self.project + context.user = user + environ = {'ec2.context' : context, + 'ec2.controller': 'some string', + 'ec2.action': methodName} + req = webob.Request.blank('/', environ) + resp = req.get_response(self.mw) + return resp.status_int + + def shouldAllow(self, user, methodName): + self.assertEqual(200, self.response_status(user, methodName)) + + def shouldDeny(self, user, methodName): + self.assertEqual(401, self.response_status(user, methodName)) + def test_001_allow_all(self): - self.context.user = self.testadmin - self.assertTrue(self._allow_all(self.context)) - self.context.user = self.testpmsys - self.assertTrue(self._allow_all(self.context)) - self.context.user = self.testnet - self.assertTrue(self._allow_all(self.context)) - self.context.user = self.testsys - self.assertTrue(self._allow_all(self.context)) + users = [self.testadmin, self.testpmsys, self.testnet, self.testsys] + for user in users: + self.shouldAllow(user, '_allow_all') def test_002_allow_none(self): - self.context.user = self.testadmin - self.assertTrue(self._allow_none(self.context)) - self.context.user = self.testpmsys - self.assertRaises(exception.NotAuthorized, self._allow_none, self.context) - self.context.user = self.testnet - self.assertRaises(exception.NotAuthorized, self._allow_none, self.context) - self.context.user = self.testsys - self.assertRaises(exception.NotAuthorized, self._allow_none, self.context) + self.shouldAllow(self.testadmin, '_allow_none') + users = [self.testpmsys, self.testnet, self.testsys] + for user in users: + self.shouldDeny(user, '_allow_none') def test_003_allow_project_manager(self): - self.context.user = self.testadmin - self.assertTrue(self._allow_project_manager(self.context)) - self.context.user = self.testpmsys - self.assertTrue(self._allow_project_manager(self.context)) - self.context.user = self.testnet - self.assertRaises(exception.NotAuthorized, self._allow_project_manager, self.context) - self.context.user = self.testsys - self.assertRaises(exception.NotAuthorized, self._allow_project_manager, self.context) + for user in [self.testadmin, self.testpmsys]: + self.shouldAllow(user, '_allow_project_manager') + for user in [self.testnet, self.testsys]: + self.shouldDeny(user, '_allow_project_manager') def test_004_allow_sys_and_net(self): - self.context.user = self.testadmin - self.assertTrue(self._allow_sys_and_net(self.context)) - self.context.user = self.testpmsys # doesn't have the per project sysadmin - self.assertRaises(exception.NotAuthorized, self._allow_sys_and_net, self.context) - self.context.user = self.testnet - self.assertTrue(self._allow_sys_and_net(self.context)) - self.context.user = self.testsys - self.assertTrue(self._allow_sys_and_net(self.context)) - - def test_005_allow_sys_no_pm(self): - self.context.user = self.testadmin - self.assertTrue(self._allow_sys_no_pm(self.context)) - self.context.user = self.testpmsys - self.assertRaises(exception.NotAuthorized, self._allow_sys_no_pm, self.context) - self.context.user = self.testnet - self.assertRaises(exception.NotAuthorized, self._allow_sys_no_pm, self.context) - self.context.user = self.testsys - self.assertTrue(self._allow_sys_no_pm(self.context)) - - @rbac.allow('all') - def _allow_all(self, context): - return True - - @rbac.allow('none') - def _allow_none(self, context): - return True - - @rbac.allow('projectmanager') - def _allow_project_manager(self, context): - return True - - @rbac.allow('sysadmin', 'netadmin') - def _allow_sys_and_net(self, context): - return True - - @rbac.allow('sysadmin') - @rbac.deny('projectmanager') - def _allow_sys_no_pm(self, context): - return True + for user in [self.testadmin, self.testnet, self.testsys]: + self.shouldAllow(user, '_allow_sys_and_net') + # denied because it doesn't have the per project sysadmin + for user in [self.testpmsys]: + self.shouldDeny(user, '_allow_sys_and_net') if __name__ == "__main__": # TODO: Implement use_fake as an option From e32eed189e8e55710bd1f9344d5e50a567fa1781 Mon Sep 17 00:00:00 2001 From: Michael Gundlach Date: Tue, 21 Sep 2010 16:58:08 -0400 Subject: [PATCH 02/11] Reenable access_unittest now that it works with new rbac --- run_tests.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/run_tests.py b/run_tests.py index bea97c0b..4121f4c0 100644 --- a/run_tests.py +++ b/run_tests.py @@ -49,8 +49,7 @@ from nova import datastore from nova import flags from nova import twistd -#TODO(gundlach): rewrite and readd this after merge -#from nova.tests.access_unittest import * +from nova.tests.access_unittest import * from nova.tests.auth_unittest import * from nova.tests.api_unittest import * from nova.tests.cloud_unittest import * From 4f4706cbe0854241dc3b69e98eacf5e8107ef13f Mon Sep 17 00:00:00 2001 From: Michael Gundlach Date: Wed, 22 Sep 2010 15:57:24 -0400 Subject: [PATCH 03/11] Responding to eday's feedback -- make a clearer inner wsgi app --- nova/tests/access_unittest.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/nova/tests/access_unittest.py b/nova/tests/access_unittest.py index d85f559f..c8a49d2c 100644 --- a/nova/tests/access_unittest.py +++ b/nova/tests/access_unittest.py @@ -74,7 +74,10 @@ class AccessTestCase(test.BaseTestCase): self.project.add_role(self.testsys, 'sysadmin') except: pass #user is set in each test - self.mw = ec2.Authorizer(lambda x,y: y('200 OK', []) and '') + def noopWSGIApp(environ, start_response): + start_response('200 OK', []) + return [''] + self.mw = ec2.Authorizer(noopWSGIApp) self.mw.action_roles = {'str': { '_allow_all': ['all'], '_allow_none': [], From cc486147a954258168b98d4057b494da8747ac10 Mon Sep 17 00:00:00 2001 From: Eric Day Date: Wed, 22 Sep 2010 17:54:57 -0700 Subject: [PATCH 04/11] Various loose ends for endpoint and tornado removal cleanup, including cloudpipe API addition, rpc.call() cleanup by removing tornado ioloop, and fixing bin/* programs. Tornado still exists as part of some test cases and those should be reworked to not require it. --- bin/nova-api | 47 +++++++++++------------------------- bin/nova-api-new | 45 ---------------------------------- bin/nova-manage | 3 +-- nova/rpc.py | 38 +++++++++++++++-------------- nova/tests/cloud_unittest.py | 1 - 5 files changed, 35 insertions(+), 99 deletions(-) delete mode 100755 bin/nova-api-new diff --git a/bin/nova-api b/bin/nova-api index ede09d38..8625c487 100755 --- a/bin/nova-api +++ b/bin/nova-api @@ -1,31 +1,28 @@ #!/usr/bin/env python +# pylint: disable-msg=C0103 # vim: tabstop=4 shiftwidth=4 softtabstop=4 # Copyright 2010 United States Government as represented by the # Administrator of the National Aeronautics and Space Administration. # All Rights Reserved. # -# Licensed under the Apache License, Version 2.0 (the "License"); you may -# not use this file except in compliance with the License. You may obtain -# a copy of the License at +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at # -# http://www.apache.org/licenses/LICENSE-2.0 +# http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -# License for the specific language governing permissions and limitations -# under the License. - +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. """ -Tornado daemon for the main API endpoint. +Nova API daemon. """ -import logging import os import sys -from tornado import httpserver -from tornado import ioloop # If ../nova/__init__.py exists, add ../ to Python search path, so that # it will override what happens to be installed in /usr/(local/)lib/python... @@ -35,30 +32,14 @@ possible_topdir = os.path.normpath(os.path.join(os.path.abspath(sys.argv[0]), if os.path.exists(os.path.join(possible_topdir, 'nova', '__init__.py')): sys.path.insert(0, possible_topdir) +from nova import api from nova import flags -from nova import server from nova import utils -from nova.endpoint import admin -from nova.endpoint import api -from nova.endpoint import cloud +from nova import wsgi FLAGS = flags.FLAGS - - -def main(_argv): - """Load the controllers and start the tornado I/O loop.""" - controllers = { - 'Cloud': cloud.CloudController(), - 'Admin': admin.AdminController()} - _app = api.APIServerApplication(controllers) - - io_inst = ioloop.IOLoop.instance() - http_server = httpserver.HTTPServer(_app) - http_server.listen(FLAGS.cc_port) - logging.debug('Started HTTP server on %s', FLAGS.cc_port) - io_inst.start() - +flags.DEFINE_integer('api_port', 8773, 'API port') if __name__ == '__main__': utils.default_flagfile() - server.serve('nova-api', main) + wsgi.run_server(api.API(), FLAGS.api_port) diff --git a/bin/nova-api-new b/bin/nova-api-new deleted file mode 100755 index 8625c487..00000000 --- a/bin/nova-api-new +++ /dev/null @@ -1,45 +0,0 @@ -#!/usr/bin/env python -# pylint: disable-msg=C0103 -# vim: tabstop=4 shiftwidth=4 softtabstop=4 - -# Copyright 2010 United States Government as represented by the -# Administrator of the National Aeronautics and Space Administration. -# All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -""" -Nova API daemon. -""" - -import os -import sys - -# If ../nova/__init__.py exists, add ../ to Python search path, so that -# it will override what happens to be installed in /usr/(local/)lib/python... -possible_topdir = os.path.normpath(os.path.join(os.path.abspath(sys.argv[0]), - os.pardir, - os.pardir)) -if os.path.exists(os.path.join(possible_topdir, 'nova', '__init__.py')): - sys.path.insert(0, possible_topdir) - -from nova import api -from nova import flags -from nova import utils -from nova import wsgi - -FLAGS = flags.FLAGS -flags.DEFINE_integer('api_port', 8773, 'API port') - -if __name__ == '__main__': - utils.default_flagfile() - wsgi.run_server(api.API(), FLAGS.api_port) diff --git a/bin/nova-manage b/bin/nova-manage index 824e00ac..baa1cb4d 100755 --- a/bin/nova-manage +++ b/bin/nova-manage @@ -73,7 +73,6 @@ from nova import quota from nova import utils from nova.auth import manager from nova.cloudpipe import pipelib -from nova.endpoint import cloud FLAGS = flags.FLAGS @@ -84,7 +83,7 @@ class VpnCommands(object): def __init__(self): self.manager = manager.AuthManager() - self.pipe = pipelib.CloudPipe(cloud.CloudController()) + self.pipe = pipelib.CloudPipe() def list(self): """Print a listing of the VPNs for all projects.""" diff --git a/nova/rpc.py b/nova/rpc.py index 84a9b559..7e4d91a0 100644 --- a/nova/rpc.py +++ b/nova/rpc.py @@ -94,8 +94,6 @@ class Consumer(messaging.Consumer): injected.start() return injected - attachToTornado = attach_to_tornado - def fetch(self, no_ack=None, auto_ack=None, enable_callbacks=False): """Wraps the parent fetch with some logic for failed connections""" # TODO(vish): the logic for failed connections and logging should be @@ -266,27 +264,31 @@ def call(topic, msg): LOG.debug("MSG_ID is %s" % (msg_id)) conn = Connection.instance() - d = defer.Deferred() + + class WaitMessage(object): + + def __call__(self, data, message): + """Acks message and sets result.""" + message.ack() + if data['failure']: + self.result = RemoteError(*data['failure']) + else: + self.result = data['result'] + + wait_msg = WaitMessage() consumer = DirectConsumer(connection=conn, msg_id=msg_id) - - def deferred_receive(data, message): - """Acks message and callbacks or errbacks""" - message.ack() - if data['failure']: - return d.errback(RemoteError(*data['failure'])) - else: - return d.callback(data['result']) - - consumer.register_callback(deferred_receive) - injected = consumer.attach_to_tornado() - - # clean up after the injected listened and return x - d.addCallback(lambda x: injected.stop() and x or x) + consumer.register_callback(wait_msg) publisher = TopicPublisher(connection=conn, topic=topic) publisher.send(msg) publisher.close() - return d + + try: + consumer.wait(limit=1) + except StopIteration: + pass + consumer.close() + return wait_msg.result def cast(topic, msg): diff --git a/nova/tests/cloud_unittest.py b/nova/tests/cloud_unittest.py index 2f22982e..756ce519 100644 --- a/nova/tests/cloud_unittest.py +++ b/nova/tests/cloud_unittest.py @@ -22,7 +22,6 @@ from M2Crypto import RSA import StringIO import time -from tornado import ioloop from twisted.internet import defer import unittest from xml.etree import ElementTree From f9d2ddbf806457a65a022bab5b433906c6184cff Mon Sep 17 00:00:00 2001 From: Soren Hansen Date: Thu, 23 Sep 2010 11:06:49 +0200 Subject: [PATCH 05/11] Compare project_id to '' using == (equality) rather than 'is' (identity). This is needed because '' isn't the same as u''. --- nova/auth/manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nova/auth/manager.py b/nova/auth/manager.py index bc3a8a12..2ec58641 100644 --- a/nova/auth/manager.py +++ b/nova/auth/manager.py @@ -266,7 +266,7 @@ class AuthManager(object): # NOTE(vish): if we stop using project name as id we need better # logic to find a default project for user - if project_id is '': + if project_id == '': project_id = user.name project = self.get_project(project_id) From c4514bb23c938afc51d9cf61728dec0e409cabaf Mon Sep 17 00:00:00 2001 From: Michael Gundlach Date: Thu, 23 Sep 2010 09:13:27 -0400 Subject: [PATCH 06/11] Apply vish's patch --- bin/nova-api-new | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bin/nova-api-new b/bin/nova-api-new index 8625c487..6f25ad8c 100755 --- a/bin/nova-api-new +++ b/bin/nova-api-new @@ -34,12 +34,11 @@ if os.path.exists(os.path.join(possible_topdir, 'nova', '__init__.py')): from nova import api from nova import flags -from nova import utils from nova import wsgi FLAGS = flags.FLAGS flags.DEFINE_integer('api_port', 8773, 'API port') if __name__ == '__main__': - utils.default_flagfile() + FLAGS(sys.argv) wsgi.run_server(api.API(), FLAGS.api_port) From 54a5a74653f027c6547b2ab40d77cc9c11703302 Mon Sep 17 00:00:00 2001 From: Soren Hansen Date: Thu, 23 Sep 2010 15:47:29 +0200 Subject: [PATCH 07/11] Wrap WSGI container in server.serve to make it properly handle command line arguments as well as daemonise properly. Moved api and wsgi imports in the main() function to delay their inclusion until after python-daemon has closed all the file descriptors. Without this, eventlet's epoll fd gets opened before daemonize is called and thus its fd gets closed leading to very, very, very confusing errors. --- bin/nova-api-new | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/bin/nova-api-new b/bin/nova-api-new index 6f25ad8c..a5027700 100755 --- a/bin/nova-api-new +++ b/bin/nova-api-new @@ -32,13 +32,18 @@ possible_topdir = os.path.normpath(os.path.join(os.path.abspath(sys.argv[0]), if os.path.exists(os.path.join(possible_topdir, 'nova', '__init__.py')): sys.path.insert(0, possible_topdir) -from nova import api from nova import flags -from nova import wsgi +from nova import utils +from nova import server FLAGS = flags.FLAGS flags.DEFINE_integer('api_port', 8773, 'API port') -if __name__ == '__main__': - FLAGS(sys.argv) +def main(_args): + from nova import api + from nova import wsgi wsgi.run_server(api.API(), FLAGS.api_port) + +if __name__ == '__main__': + utils.default_flagfile() + server.serve('nova-api', main) From c85fddf553d7f297e5133d0037b6bb1208290bc8 Mon Sep 17 00:00:00 2001 From: Michael Gundlach Date: Thu, 23 Sep 2010 10:19:27 -0400 Subject: [PATCH 08/11] Spot-fix endpoint reference --- bin/nova-manage | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/nova-manage b/bin/nova-manage index 824e00ac..e9219c51 100755 --- a/bin/nova-manage +++ b/bin/nova-manage @@ -73,7 +73,7 @@ from nova import quota from nova import utils from nova.auth import manager from nova.cloudpipe import pipelib -from nova.endpoint import cloud +from nova.api.ec2 import cloud FLAGS = flags.FLAGS From 3b87313878d2b6448f7b14642fbc70462461d5ce Mon Sep 17 00:00:00 2001 From: Eric Day Date: Thu, 23 Sep 2010 11:56:44 -0700 Subject: [PATCH 10/11] Fixed rpc consumer to use unique return connection to prevent overlap. This could be reworked to share a connection, but it should be a wait operation and not a fast poll like it was before. We could also keep a cache of opened connections to be used between requests. --- nova/rpc.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/nova/rpc.py b/nova/rpc.py index 7e4d91a0..6363335e 100644 --- a/nova/rpc.py +++ b/nova/rpc.py @@ -46,9 +46,9 @@ LOG.setLevel(logging.DEBUG) class Connection(carrot_connection.BrokerConnection): """Connection instance object""" @classmethod - def instance(cls): + def instance(cls, new=False): """Returns the instance""" - if not hasattr(cls, '_instance'): + if new or not hasattr(cls, '_instance'): params = dict(hostname=FLAGS.rabbit_host, port=FLAGS.rabbit_port, userid=FLAGS.rabbit_userid, @@ -60,7 +60,10 @@ class Connection(carrot_connection.BrokerConnection): # NOTE(vish): magic is fun! # pylint: disable-msg=W0142 - cls._instance = cls(**params) + if new: + return cls(**params) + else: + cls._instance = cls(**params) return cls._instance @classmethod @@ -263,8 +266,6 @@ def call(topic, msg): msg.update({'_msg_id': msg_id}) LOG.debug("MSG_ID is %s" % (msg_id)) - conn = Connection.instance() - class WaitMessage(object): def __call__(self, data, message): @@ -276,9 +277,11 @@ def call(topic, msg): self.result = data['result'] wait_msg = WaitMessage() + conn = Connection.instance(True) consumer = DirectConsumer(connection=conn, msg_id=msg_id) consumer.register_callback(wait_msg) + conn = Connection.instance() publisher = TopicPublisher(connection=conn, topic=topic) publisher.send(msg) publisher.close()