From 8077486b3c15012f4dbf270cd8c9fa3f48cb3d36 Mon Sep 17 00:00:00 2001 From: jakedahn Date: Thu, 9 Aug 2012 14:28:28 -0700 Subject: Default behavior should restrict admins to tenant for volumes. * NOTE: This is a port from cinder to nova volumes * Now to view all volumes or volume snapshots across all tenants you need to include the all_tenants=1 GET param in your api request. * Fixes remaining issues blocking bug #967882 Change-Id: I7fe15e792b62e59973c7faa2cf1c52929ae5864f --- nova/api/openstack/volume/snapshots.py | 6 +++- nova/api/openstack/volume/volumes.py | 27 ++++++++++++++- nova/tests/api/ec2/test_cloud.py | 13 ++++--- nova/tests/api/openstack/fakes.py | 34 ++++++++++++++++++- nova/tests/api/openstack/volume/test_snapshots.py | 41 ++++++++++++++++++++--- nova/tests/api/openstack/volume/test_volumes.py | 38 ++++++++++++++++++++- nova/volume/api.py | 26 ++++++++++---- 7 files changed, 165 insertions(+), 20 deletions(-) diff --git a/nova/api/openstack/volume/snapshots.py b/nova/api/openstack/volume/snapshots.py index 209f78d13..91823de79 100644 --- a/nova/api/openstack/volume/snapshots.py +++ b/nova/api/openstack/volume/snapshots.py @@ -130,7 +130,11 @@ class SnapshotsController(object): """Returns a list of snapshots, transformed through entity_maker.""" context = req.environ['nova.context'] - snapshots = self.volume_api.get_all_snapshots(context) + search_opts = {} + search_opts.update(req.GET) + + snapshots = self.volume_api.get_all_snapshots(context, + search_opts=search_opts) limited_list = common.limited(snapshots, req) res = [entity_maker(context, snapshot) for snapshot in limited_list] return {'snapshots': res} diff --git a/nova/api/openstack/volume/volumes.py b/nova/api/openstack/volume/volumes.py index 83a2b2f63..4c12638b4 100644 --- a/nova/api/openstack/volume/volumes.py +++ b/nova/api/openstack/volume/volumes.py @@ -196,9 +196,15 @@ class VolumeController(object): def _items(self, req, entity_maker): """Returns a list of volumes, transformed through entity_maker.""" + + search_opts = {} + search_opts.update(req.GET) + context = req.environ['nova.context'] + remove_invalid_options(context, + search_opts, self._get_volume_search_options()) - volumes = self.volume_api.get_all(context) + volumes = self.volume_api.get_all(context, search_opts=search_opts) limited_list = common.limited(volumes, req) res = [entity_maker(context, vol) for vol in limited_list] return {'volumes': res} @@ -253,6 +259,25 @@ class VolumeController(object): return wsgi.ResponseObject(result, headers=dict(location=location)) + def _get_volume_search_options(self): + """Return volume search options allowed by non-admin.""" + return ('name', 'status') + def create_resource(): return wsgi.Resource(VolumeController()) + + +def remove_invalid_options(context, search_options, allowed_search_options): + """Remove search options that are not valid for non-admin API/context.""" + if context.is_admin: + # Allow all options + return + # Otherwise, strip out all unknown options + unknown_options = [opt for opt in search_options + if opt not in allowed_search_options] + bad_options = ", ".join(unknown_options) + log_msg = _("Removing options '%(bad_options)s' from query") % locals() + LOG.debug(log_msg) + for opt in unknown_options: + search_options.pop(opt, None) diff --git a/nova/tests/api/ec2/test_cloud.py b/nova/tests/api/ec2/test_cloud.py index f2684efc3..b2d552f84 100644 --- a/nova/tests/api/ec2/test_cloud.py +++ b/nova/tests/api/ec2/test_cloud.py @@ -616,8 +616,8 @@ class CloudTestCase(test.TestCase): def test_describe_volumes(self): """Makes sure describe_volumes works and filters results.""" - vol1 = db.volume_create(self.context, {}) - vol2 = db.volume_create(self.context, {}) + vol1 = db.volume_create(self.context, {'project_id': self.project_id}) + vol2 = db.volume_create(self.context, {'project_id': self.project_id}) result = self.cloud.describe_volumes(self.context) self.assertEqual(len(result['volumeSet']), 2) volume_id = ec2utils.id_to_ec2_vol_id(vol2['id']) @@ -653,7 +653,8 @@ class CloudTestCase(test.TestCase): def test_create_volume_from_snapshot(self): """Makes sure create_volume works when we specify a snapshot.""" - vol = db.volume_create(self.context, {'size': 1}) + vol = db.volume_create(self.context, {'size': 1, + 'project_id': self.project_id}) snap = db.snapshot_create(self.context, {'volume_id': vol['id'], 'volume_size': vol['size'], 'status': "available"}) @@ -690,8 +691,10 @@ class CloudTestCase(test.TestCase): def test_describe_snapshots(self): """Makes sure describe_snapshots works and filters results.""" vol = db.volume_create(self.context, {}) - snap1 = db.snapshot_create(self.context, {'volume_id': vol['id']}) - snap2 = db.snapshot_create(self.context, {'volume_id': vol['id']}) + snap1 = db.snapshot_create(self.context, + {'volume_id': vol['id'], 'project_id': self.project_id}) + snap2 = db.snapshot_create(self.context, + {'volume_id': vol['id'], 'project_id': self.project_id}) result = self.cloud.describe_snapshots(self.context) self.assertEqual(len(result['snapshotSet']), 2) snapshot_id = ec2utils.id_to_ec2_snap_id(snap2['id']) diff --git a/nova/tests/api/openstack/fakes.py b/nova/tests/api/openstack/fakes.py index 62722b34d..a5eb57b1b 100644 --- a/nova/tests/api/openstack/fakes.py +++ b/nova/tests/api/openstack/fakes.py @@ -540,5 +540,37 @@ def stub_volume_get_notfound(self, context, volume_id): raise exc.NotFound -def stub_volume_get_all(self, context, search_opts=None): +def stub_volume_get_all(context, search_opts=None): + return [stub_volume(100, project_id='fake'), + stub_volume(101, project_id='superfake'), + stub_volume(102, project_id='superduperfake')] + + +def stub_volume_get_all_by_project(self, context, search_opts=None): return [stub_volume_get(self, context, '1')] + + +def stub_snapshot(id, **kwargs): + snapshot = { + 'id': id, + 'volume_id': 12, + 'status': 'available', + 'volume_size': 100, + 'created_at': None, + 'display_name': 'Default name', + 'display_description': 'Default description', + 'project_id': 'fake' + } + + snapshot.update(kwargs) + return snapshot + + +def stub_snapshot_get_all(self): + return [stub_snapshot(100, project_id='fake'), + stub_snapshot(101, project_id='superfake'), + stub_snapshot(102, project_id='superduperfake')] + + +def stub_snapshot_get_all_by_project(self, context): + return [stub_snapshot(1)] diff --git a/nova/tests/api/openstack/volume/test_snapshots.py b/nova/tests/api/openstack/volume/test_snapshots.py index ccd3a70b4..e7370c548 100644 --- a/nova/tests/api/openstack/volume/test_snapshots.py +++ b/nova/tests/api/openstack/volume/test_snapshots.py @@ -17,6 +17,7 @@ from lxml import etree import webob from nova.api.openstack.volume import snapshots +from nova import db from nova import exception from nova import flags from nova.openstack.common import log as logging @@ -63,7 +64,7 @@ def stub_snapshot_get(self, context, snapshot_id): return param -def stub_snapshot_get_all(self, context): +def stub_snapshot_get_all(self, context, search_opts=None): param = _get_default_snapshot_param() return [param] @@ -73,9 +74,10 @@ class SnapshotApiTest(test.TestCase): super(SnapshotApiTest, self).setUp() self.controller = snapshots.SnapshotsController() - self.stubs.Set(volume.api.API, "get_snapshot", stub_snapshot_get) - self.stubs.Set(volume.api.API, "get_all_snapshots", - stub_snapshot_get_all) + self.stubs.Set(db, 'snapshot_get_all_by_project', + fakes.stub_snapshot_get_all_by_project) + self.stubs.Set(db, 'snapshot_get_all', + fakes.stub_snapshot_get_all) def test_snapshot_create(self): self.stubs.Set(volume.api.API, "create_snapshot", stub_snapshot_create) @@ -113,6 +115,7 @@ class SnapshotApiTest(test.TestCase): snapshot['display_description']) def test_snapshot_delete(self): + self.stubs.Set(volume.api.API, "get_snapshot", stub_snapshot_get) self.stubs.Set(volume.api.API, "delete_snapshot", stub_snapshot_delete) snapshot_id = 123 @@ -130,6 +133,7 @@ class SnapshotApiTest(test.TestCase): snapshot_id) def test_snapshot_show(self): + self.stubs.Set(volume.api.API, "get_snapshot", stub_snapshot_get) req = fakes.HTTPRequest.blank('/v1/snapshots/123') resp_dict = self.controller.show(req, 123) @@ -145,6 +149,8 @@ class SnapshotApiTest(test.TestCase): snapshot_id) def test_snapshot_detail(self): + self.stubs.Set(volume.api.API, "get_all_snapshots", + stub_snapshot_get_all) req = fakes.HTTPRequest.blank('/v1/snapshots/detail') resp_dict = self.controller.detail(req) @@ -155,6 +161,33 @@ class SnapshotApiTest(test.TestCase): resp_snapshot = resp_snapshots.pop() self.assertEqual(resp_snapshot['id'], '123') + def test_admin_list_snapshots_limited_to_project(self): + req = fakes.HTTPRequest.blank('/v1/fake/snapshots', + use_admin_context=True) + res = self.controller.index(req) + + self.assertTrue('snapshots' in res) + self.assertEqual(1, len(res['snapshots'])) + + def test_admin_list_snapshots_all_tenants(self): + req = fakes.HTTPRequest.blank('/v2/fake/snapshots?all_tenants=1', + use_admin_context=True) + res = self.controller.index(req) + self.assertTrue('snapshots' in res) + self.assertEqual(3, len(res['snapshots'])) + + def test_all_tenants_non_admin_gets_all_tenants(self): + req = fakes.HTTPRequest.blank('/v2/fake/snapshots?all_tenants=1') + res = self.controller.index(req) + self.assertTrue('snapshots' in res) + self.assertEqual(1, len(res['snapshots'])) + + def test_non_admin_get_by_project(self): + req = fakes.HTTPRequest.blank('/v2/fake/snapshots') + res = self.controller.index(req) + self.assertTrue('snapshots' in res) + self.assertEqual(1, len(res['snapshots'])) + class SnapshotSerializerTest(test.TestCase): def _verify_snapshot(self, snap, tree): diff --git a/nova/tests/api/openstack/volume/test_volumes.py b/nova/tests/api/openstack/volume/test_volumes.py index 672c08d88..6f9a6557b 100644 --- a/nova/tests/api/openstack/volume/test_volumes.py +++ b/nova/tests/api/openstack/volume/test_volumes.py @@ -19,6 +19,7 @@ from lxml import etree import webob from nova.api.openstack.volume import volumes +from nova import db from nova import exception from nova import flags from nova.openstack.common import timeutils @@ -35,7 +36,9 @@ class VolumeApiTest(test.TestCase): super(VolumeApiTest, self).setUp() self.controller = volumes.VolumeController() - self.stubs.Set(volume_api.API, 'get_all', fakes.stub_volume_get_all) + self.stubs.Set(db, 'volume_get_all', fakes.stub_volume_get_all) + self.stubs.Set(db, 'volume_get_all_by_project', + fakes.stub_volume_get_all_by_project) self.stubs.Set(volume_api.API, 'get', fakes.stub_volume_get) self.stubs.Set(volume_api.API, 'delete', fakes.stub_volume_delete) @@ -89,6 +92,9 @@ class VolumeApiTest(test.TestCase): body) def test_volume_list(self): + self.stubs.Set(volume_api.API, 'get_all', + fakes.stub_volume_get_all_by_project) + req = fakes.HTTPRequest.blank('/v1/volumes') res_dict = self.controller.index(req) expected = {'volumes': [{'status': 'fakestatus', @@ -110,6 +116,9 @@ class VolumeApiTest(test.TestCase): self.assertEqual(res_dict, expected) def test_volume_list_detail(self): + self.stubs.Set(volume_api.API, 'get_all', + fakes.stub_volume_get_all_by_project) + req = fakes.HTTPRequest.blank('/v1/volumes/detail') res_dict = self.controller.index(req) expected = {'volumes': [{'status': 'fakestatus', @@ -194,6 +203,33 @@ class VolumeApiTest(test.TestCase): req, 1) + def test_admin_list_volumes_limited_to_project(self): + req = fakes.HTTPRequest.blank('/v2/fake/volumes', + use_admin_context=True) + res = self.controller.index(req) + + self.assertTrue('volumes' in res) + self.assertEqual(1, len(res['volumes'])) + + def test_admin_list_volumes_all_tenants(self): + req = fakes.HTTPRequest.blank('/v2/fake/volumes?all_tenants=1', + use_admin_context=True) + res = self.controller.index(req) + self.assertTrue('volumes' in res) + self.assertEqual(3, len(res['volumes'])) + + def test_all_tenants_non_admin_gets_all_tenants(self): + req = fakes.HTTPRequest.blank('/v2/fake/volumes?all_tenants=1') + res = self.controller.index(req) + self.assertTrue('volumes' in res) + self.assertEqual(1, len(res['volumes'])) + + def test_non_admin_get_by_project(self): + req = fakes.HTTPRequest.blank('/v2/fake/volumes') + res = self.controller.index(req) + self.assertTrue('volumes' in res) + self.assertEqual(1, len(res['volumes'])) + class VolumeSerializerTest(test.TestCase): def _verify_volume_attachment(self, attach, tree): diff --git a/nova/volume/api.py b/nova/volume/api.py index a9ebeda28..d98c8d6eb 100644 --- a/nova/volume/api.py +++ b/nova/volume/api.py @@ -201,14 +201,19 @@ class API(base.Base): check_policy(context, 'get', volume) return volume - def get_all(self, context, search_opts={}): + def get_all(self, context, search_opts=None): check_policy(context, 'get_all') - if context.is_admin: + + if search_opts is None: + search_opts = {} + + if (context.is_admin and 'all_tenants' in search_opts): + # Need to remove all_tenants to pass the filtering below. + del search_opts['all_tenants'] volumes = self.db.volume_get_all(context) else: volumes = self.db.volume_get_all_by_project(context, - context.project_id) - + context.project_id) if search_opts: LOG.debug(_("Searching by: %s") % str(search_opts)) @@ -247,11 +252,18 @@ class API(base.Base): rv = self.db.snapshot_get(context, snapshot_id) return dict(rv.iteritems()) - def get_all_snapshots(self, context): + def get_all_snapshots(self, context, search_opts=None): check_policy(context, 'get_all_snapshots') - if context.is_admin: + + search_opts = search_opts or {} + + if (context.is_admin and 'all_tenants' in search_opts): + # Need to remove all_tenants to pass the filtering below. + del search_opts['all_tenants'] return self.db.snapshot_get_all(context) - return self.db.snapshot_get_all_by_project(context, context.project_id) + else: + return self.db.snapshot_get_all_by_project(context, + context.project_id) @wrap_check_policy def check_attach(self, context, volume): -- cgit