Remove deleted projects from flavor access list

Previously Nova was unable to remove deleted projects from flavor's
access lists. This patch lifts described limitation and improves
logic of nova/api/openstack/identity.py library by introducing two
separate kinds of exceptions:

- webob.exc.HTTPInternalServerError is raised when Keystone identity
  service version 3.0 was not found.
- webob.exc.HTTPBadRequest is raised when specified project is not
  found.

Closes-bug: #1980845
Change-Id: Icbf3bdd944f9a6c38f25ddea0b521ca48ee87a7f
(cherry picked from commit 8c6daaacbedc33e738ce85aec0ead5f6947d60bf)
(cherry picked from commit 2ea2b556da5f10d662641bd96b0a07735d2b9607)

3 files changed, 45 insertions, 11 deletions

diff --git a/nova/api/openstack/compute/flavor_access.py b/nova/api/openstack/compute/flavor_access.py
index e17e6f0ddc..fc8df15db5 100644
--- a/nova/api/openstack/compute/flavor_access.py
+++ b/nova/api/openstack/compute/flavor_access.py
@@ -93,7 +93,14 @@ class FlavorActionController(wsgi.Controller):
 
         vals = body['removeTenantAccess']
         tenant = vals['tenant']
-        identity.verify_project_id(context, tenant)
+        # It doesn't really matter if project exists or not: we can delete
+        # it from flavor's access list in both cases.
+        try:
+            identity.verify_project_id(context, tenant)
+        except webob.exc.HTTPBadRequest as identity_exc:
+            msg = "Project ID %s is not a valid project." % tenant
+            if msg not in identity_exc.explanation:
+                raise
 
         # NOTE(gibi): We have to load a flavor from the db here as
         # flavor.remove_access() will try to emit a notification and that needs
diff --git a/nova/api/openstack/identity.py b/nova/api/openstack/identity.py
index 7ffc623fed..15ec884aea 100644
--- a/nova/api/openstack/identity.py
+++ b/nova/api/openstack/identity.py
@@ -27,24 +27,27 @@ def verify_project_id(context, project_id):
     """verify that a project_id exists.
 
     This attempts to verify that a project id exists. If it does not,
-    an HTTPBadRequest is emitted.
+    an HTTPBadRequest is emitted. Also HTTPBadRequest is emitted
+    if Keystone identity service version 3.0 is not found.
 
     """
     adap = utils.get_ksa_adapter(
         'identity', ksa_auth=context.get_auth_plugin(),
         min_version=(3, 0), max_version=(3, 'latest'))
 
-    failure = webob.exc.HTTPBadRequest(
-            explanation=_("Project ID %s is not a valid project.") %
-            project_id)
     try:
         resp = adap.get('/projects/%s' % project_id)
     except kse.EndpointNotFound:
         LOG.error(
-            "Keystone identity service version 3.0 was not found. This might "
-            "be because your endpoint points to the v2.0 versioned endpoint "
-            "which is not supported. Please fix this.")
-        raise failure
+            "Keystone identity service version 3.0 was not found. This "
+            "might be caused by Nova misconfiguration or Keystone "
+            "problems.")
+        msg = _("Nova was unable to find Keystone service endpoint.")
+        # TODO(astupnik). It may be reasonable to switch to HTTP 503
+        # (HTTP Service Unavailable) instead of HTTP Bad Request here.
+        # If proper Keystone servie is inaccessible, then technially
+        # this is a server side error and not an error in Nova.
+        raise webob.exc.HTTPBadRequest(explanation=msg)
     except kse.ClientException:
         # something is wrong, like there isn't a keystone v3 endpoint,
         # or nova isn't configured for the interface to talk to it;
@@ -57,7 +60,8 @@ def verify_project_id(context, project_id):
         return True
     elif resp.status_code == 404:
         # we got access, and we know this project is not there
-        raise failure
+        msg = _("Project ID %s is not a valid project.") % project_id
+        raise webob.exc.HTTPBadRequest(explanation=msg)
     elif resp.status_code == 403:
         # we don't have enough permission to verify this, so default
         # to "it's ok".
diff --git a/nova/tests/unit/api/openstack/compute/test_flavor_access.py b/nova/tests/unit/api/openstack/compute/test_flavor_access.py
index 8c25a2efc2..1c5c34e758 100644
--- a/nova/tests/unit/api/openstack/compute/test_flavor_access.py
+++ b/nova/tests/unit/api/openstack/compute/test_flavor_access.py
@@ -353,14 +353,37 @@ class FlavorAccessTestV21(test.NoDBTestCase):
         mock_verify.assert_called_once_with(
             req.environ['nova.context'], 'proj2')
 
+    @mock.patch('nova.objects.Flavor.remove_access')
     @mock.patch('nova.api.openstack.identity.verify_project_id',
                 side_effect=exc.HTTPBadRequest(
                     explanation="Project ID proj2 is not a valid project."))
-    def test_remove_tenant_access_with_invalid_tenant(self, mock_verify):
+    def test_remove_tenant_access_with_invalid_tenant(self,
+                                                      mock_verify,
+                                                      mock_remove_access):
         """Tests the case that the tenant does not exist in Keystone."""
         req = fakes.HTTPRequest.blank(self._prefix + '/flavors/2/action',
                                       use_admin_context=True)
         body = {'removeTenantAccess': {'tenant': 'proj2'}}
+
+        self.flavor_action_controller._remove_tenant_access(
+            req, '2', body=body)
+        mock_verify.assert_called_once_with(
+            req.environ['nova.context'], 'proj2')
+        mock_remove_access.assert_called_once_with('proj2')
+
+    @mock.patch('nova.api.openstack.identity.verify_project_id',
+                side_effect=exc.HTTPBadRequest(
+                    explanation="Nova was unable to find Keystone "
+                                "service endpoint."))
+    def test_remove_tenant_access_missing_keystone_endpoint(self,
+                                                            mock_verify):
+        """Tests the case that Keystone identity service endpoint
+        version 3.0 was not found.
+        """
+        req = fakes.HTTPRequest.blank(self._prefix + '/flavors/2/action',
+                                      use_admin_context=True)
+        body = {'removeTenantAccess': {'tenant': 'proj2'}}
+
         self.assertRaises(exc.HTTPBadRequest,
                           self.flavor_action_controller._remove_tenant_access,
                           req, '2', body=body)