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

@@ -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:
+        defaults = QUOTAS.get_defaults(ctxt, proj_id)
-                db.quota_destroy_by_project(ctxt, target_project.id)
+        # If the project which is being deleted has allocated part of its
-            except exception.AdminRequired:
+        # quota to its subprojects, then subprojects' quotas should be
-                raise webob.exc.HTTPForbidden()
+        # 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():
+        try:
-                # Update child limit to 0 so the parent hierarchy gets it's
+            db.quota_destroy_by_project(ctxt, target_project.id)
-                # allocated values updated properly
+        except exception.AdminRequired:
-                self._update_nested_quota_allocated(
+            raise webob.exc.HTTPForbidden()
-                    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.

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()
+        res = 'volumes'
-
+        self._assert_quota_show(self.A.id, res, allocated=2, limit=5)
-        # Allocate some of that quota to a child project by using volumes
+        self._assert_quota_show(self.B.id, res, allocated=2, limit=-1)
-        quota['volumes'] = -1
+        self.controller.delete(self.req, self.D.id)
-        self.controller.update(self.req, self.D.id, body)
+        self._assert_quota_show(self.A.id, res, allocated=0, limit=5)
-        for x in range(2):
+        self._assert_quota_show(self.B.id, res, allocated=0, limit=-1)
-            quotas.QUOTAS._driver.reserve(
-                self.req.environ['cinder.context'], quotas.QUOTAS.resources,
-                {'volumes': 1}, project_id=self.D.id)
-
-        _assert_delete_updates_allocated()
 
     def test_negative_child_limit_not_affecting_parents_free_quota(self):
         quota = {'volumes': -1}