From cafe657c11f444c514f8eed44feb7dd7de23e62c Mon Sep 17 00:00:00 2001 From: Ryan McNair Date: Thu, 10 Mar 2016 20:15:24 +0000 Subject: [PATCH] Disallow quota deletes if default under usage For nested quotas, currently we don't allow the limit to be updated below the current usage since this could make a portion of the quota tree "invalid". This is explicitly disallowed for quota update but not currently checked on quota delete. This patch adds the same checking behavior for quota-deletes. Also, the quota delete authentication check is being moved above the validation code, so that a user not allowed to perform a delete will always see an unauthorized error instead of any other invalid errors with the request. Change-Id: I390ffa38b6c03c034614756ca410dfca9e52f2ce Closes-Bug: #1555802 --- cinder/api/contrib/quotas.py | 44 +++++++++------- cinder/tests/unit/api/contrib/test_quotas.py | 54 +++++++++++++------- 2 files changed, 60 insertions(+), 38 deletions(-) diff --git a/cinder/api/contrib/quotas.py b/cinder/api/contrib/quotas.py index 788d3f830d0..6ea9234ab49 100644 --- a/cinder/api/contrib/quotas.py +++ b/cinder/api/contrib/quotas.py @@ -69,6 +69,8 @@ class QuotaSetsController(wsgi.Controller): if QUOTAS.using_nested_quotas(): used += v.get('allocated', 0) if value < used: + # TODO(mc_nair): after N opens, update error message to include + # the current usage and requested limit msg = _("Quota %s limit must be equal or greater than existing " "resources.") % key raise webob.exc.HTTPBadRequest(explanation=msg) @@ -378,16 +380,6 @@ class QuotaSetsController(wsgi.Controller): target_project = quota_utils.get_project_hierarchy( ctxt, proj_id) parent_id = target_project.parent_id - # If the project which is being deleted has allocated part of its - # quota to its subprojects, then subprojects' quotas should be - # deleted first. - for res, value in project_quotas.items(): - if 'allocated' in project_quotas[res].keys(): - if project_quotas[res]['allocated'] != 0: - msg = _("About to delete child projects having " - "non-zero quota. This should not be performed") - raise webob.exc.HTTPBadRequest(explanation=msg) - if parent_id: # Get the children of the project which the token is scoped to # in order to know if the target_project is in its hierarchy. @@ -397,16 +389,30 @@ class QuotaSetsController(wsgi.Controller): target_project.id, parent_id) - try: - db.quota_destroy_by_project(ctxt, target_project.id) - except exception.AdminRequired: - raise webob.exc.HTTPForbidden() + defaults = QUOTAS.get_defaults(ctxt, proj_id) + # If the project which is being deleted has allocated part of its + # quota to its subprojects, then subprojects' quotas should be + # deleted first. + for res, value in project_quotas.items(): + if 'allocated' in project_quotas[res].keys(): + if project_quotas[res]['allocated'] != 0: + msg = _("About to delete child projects having " + "non-zero quota. This should not be performed") + raise webob.exc.HTTPBadRequest(explanation=msg) + # Ensure quota usage wouldn't exceed limit on a delete + self._validate_existing_resource( + res, defaults[res], project_quotas) - for res, limit in project_quotas.items(): - # Update child limit to 0 so the parent hierarchy gets it's - # allocated values updated properly - self._update_nested_quota_allocated( - ctxt, target_project, project_quotas, res, 0) + try: + db.quota_destroy_by_project(ctxt, target_project.id) + except exception.AdminRequired: + raise webob.exc.HTTPForbidden() + + for res, limit in project_quotas.items(): + # Update child limit to 0 so the parent hierarchy gets it's + # allocated values updated properly + self._update_nested_quota_allocated( + ctxt, target_project, project_quotas, res, 0) def validate_setup_for_nested_quota_use(self, req): """Validates that the setup supports using nested quotas. diff --git a/cinder/tests/unit/api/contrib/test_quotas.py b/cinder/tests/unit/api/contrib/test_quotas.py index b691dc2498c..b5fc19ccdac 100644 --- a/cinder/tests/unit/api/contrib/test_quotas.py +++ b/cinder/tests/unit/api/contrib/test_quotas.py @@ -752,6 +752,35 @@ class QuotaSetsControllerNestedQuotasTest(QuotaSetsControllerTestBase): self.assertRaises(webob.exc.HTTPBadRequest, self.controller.update, self.req, self.A.id, body) + def test_project_delete_with_default_quota_less_than_in_use(self): + quota = {'volumes': 11} + body = {'quota_set': quota} + self.controller.update(self.req, self.A.id, body) + quotas.QUOTAS._driver.reserve( + self.req.environ['cinder.context'], quotas.QUOTAS.resources, + quota, project_id=self.A.id) + # Should not be able to delete if it will cause the used values to go + # over quota when nested quotas are used + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.delete, + self.req, + self.A.id) + + def test_subproject_delete_with_default_quota_less_than_in_use(self): + quota = {'volumes': 1} + body = {'quota_set': quota} + self.controller.update(self.req, self.B.id, body) + quotas.QUOTAS._driver.reserve( + self.req.environ['cinder.context'], quotas.QUOTAS.resources, + quota, project_id=self.B.id) + + # Should not be able to delete if it will cause the used values to go + # over quota when nested quotas are used + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller.delete, + self.req, + self.B.id) + def test_subproject_delete(self): self.req.environ['cinder.context'].project_id = self.A.id @@ -814,14 +843,6 @@ class QuotaSetsControllerNestedQuotasTest(QuotaSetsControllerTestBase): self.req, self.A.id) def test_subproject_delete_with_child_updates_parent_allocated(self): - def _assert_delete_updates_allocated(): - res = 'volumes' - self._assert_quota_show(self.A.id, res, allocated=2, limit=5) - self._assert_quota_show(self.B.id, res, allocated=2, limit=-1) - self.controller.delete(self.req, self.D.id) - self._assert_quota_show(self.A.id, res, allocated=0, limit=5) - self._assert_quota_show(self.B.id, res, allocated=0, limit=-1) - quota = {'volumes': 5} body = {'quota_set': quota} self.controller.update(self.req, self.A.id, body) @@ -832,17 +853,12 @@ class QuotaSetsControllerNestedQuotasTest(QuotaSetsControllerTestBase): quota['volumes'] = 2 self.controller.update(self.req, self.D.id, body) - _assert_delete_updates_allocated() - - # Allocate some of that quota to a child project by using volumes - quota['volumes'] = -1 - self.controller.update(self.req, self.D.id, body) - for x in range(2): - quotas.QUOTAS._driver.reserve( - self.req.environ['cinder.context'], quotas.QUOTAS.resources, - {'volumes': 1}, project_id=self.D.id) - - _assert_delete_updates_allocated() + res = 'volumes' + self._assert_quota_show(self.A.id, res, allocated=2, limit=5) + self._assert_quota_show(self.B.id, res, allocated=2, limit=-1) + self.controller.delete(self.req, self.D.id) + self._assert_quota_show(self.A.id, res, allocated=0, limit=5) + self._assert_quota_show(self.B.id, res, allocated=0, limit=-1) def test_negative_child_limit_not_affecting_parents_free_quota(self): quota = {'volumes': -1}