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

diff --git a/cinder/api/contrib/quotas.py b/cinder/api/contrib/quotas.py
index 788d3f830d0e3e5350967d9ef1ef345192d91dea..6ea9234ab49f85c146041cb9cd9ce0a2bc615357 100644 (file)
--- 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,6 +380,16 @@ class QuotaSetsController(wsgi.Controller):
         target_project = quota_utils.get_project_hierarchy(
             ctxt, proj_id)
         parent_id = target_project.parent_id
+        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.
+            context_project = quota_utils.get_project_hierarchy(
+                ctxt, ctxt.project_id, subtree_as_ids=True)
+            self._authorize_update_or_delete(context_project,
+                                             target_project.id,
+                                             parent_id)
+
+        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.
@@ -387,26 +399,20 @@ class QuotaSetsController(wsgi.Controller):
                     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)
 
-        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.
-            context_project = quota_utils.get_project_hierarchy(
-                ctxt, ctxt.project_id, subtree_as_ids=True)
-            self._authorize_update_or_delete(context_project,
-                                             target_project.id,
-                                             parent_id)
-
-            try:
-                db.quota_destroy_by_project(ctxt, target_project.id)
-            except exception.AdminRequired:
-                raise webob.exc.HTTPForbidden()
+        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)
+        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 b691dc2498c07f58bc3ca950d5549cd9ca094632..b5fc19ccdacf4492ec1bf075d580b1dbec094b3a 100644 (file)
--- 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}