openstack/cinder
Code Issues Proposed changes

api: Simplify enable/disable APIs (clusters)

Browse Source 
I'm using simplify a little liberally here: what we are actually
simplifying is our later addition of API schemas. This change migrates
us from using route's "update" method to using a proper "action" method
like we use for most other actions. This allows us to work towards the 1
schema 1 method model that is easiest to introspect.

Note that to do this, we must migrate this resource away from
'mapper.resource'. What we wanted to do was this:

  mapper.resource(
      'cluster',
      'clusters',
      controller=self.resources['clusters'],
      collection={
          'detail': 'GET',
          'enable': 'PUT',
          'disable': 'PUT',
      },
  )

However, this doesn't work because Routes has a feature/bug where if we
pass non-GET actions via the 'collection' argument, then the first of
these will be marked as the "primary" route. When this happens, we don't
use the action to lookup the controller method nor do we suffix the path
with the action [1]. In concrete terms, instead of getting a 'PUT
/clusters/enable' route that resolves to 'ClusterController.enable', we
get a 'PUT /clusters' route that resolves to a non-existent
'ClusterController.update' method. The solution to this is to use
'mapper.connect' - or more specifically a newly introduced
'mapper.resource' wrapper based on nova's equivalent [2] - to manually
specify each path instead.

[1] https://github.com/bbangert/routes/blob/v2.5.1/routes/mapper.py#L1157-L1169
[2] https://github.com/openstack/nova/blob/29.0.1/nova/api/openstack/__init__.py#L203-L215

Change-Id: I364d92738da7fecbc9c278dbeedaa74c63ee1595
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This commit is contained in:
Stephen Finucane
2024-04-23 10:50:55 +01:00
parent beba53bf02
commit 91b5501585
4 changed files with 139 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cinder/api/openstack/wsgi.py
+23
View File
@@ -21,6 +21,7 @@ import inspect
import math
import time
from oslo_config import cfg
from oslo_log import log as logging
from oslo_serialization import jsonutils
from oslo_utils import encodeutils
@@ -39,6 +40,7 @@ from cinder.i18n import _
from cinder import utils
from cinder.wsgi import common as wsgi
CONF = cfg.CONF
LOG = logging.getLogger(__name__)
SUPPORTED_CONTENT_TYPES = (
@@ -1292,6 +1294,7 @@ class Controller(object, metaclass=ControllerMetaclass):
class APIMapper(routes.Mapper):
def routematch(self, url=None, environ=None):
if url == "":
result = self._match("", environ)
@@ -1306,6 +1309,26 @@ class APIMapper(routes.Mapper):
kwargs['requirements']['format'] = 'json'
return routes.Mapper.connect(self, *args, **kwargs)
def create_route(self, path, method, controller, action):
# NOTE: project_id parameter is only valid if its hex or hex + dashes
# (note, integers are a subset of this). This is required to handle
# our overlapping routes issues.
project_id_regex = CONF.project_id_regex
project_id_token = '{project_id:%s}' % project_id_regex
self.connect(
'/%s%s' % (project_id_token, path),
conditions={"method": [method]},
controller=controller,
action=action,
)
self.connect(
path,
conditions={"method": [method]},
controller=controller,
action=action,
)
class Fault(webob.exc.HTTPException):
"""Wrap webob.exc.HTTPException to provide API friendly response."""
cinder/api/v3/clusters.py
+50 -31
View File
@@ -15,7 +15,7 @@
from cinder.api import microversions as mv
from cinder.api.openstack import wsgi
from cinder.api.schemas import clusters as cluster
from cinder.api.schemas import clusters as schema
from cinder.api.v3.views import clusters as clusters_view
from cinder.api import validation
from cinder.common import constants
@@ -90,28 +90,14 @@ class ClusterController(wsgi.Controller):
return clusters_view.ViewBuilder.list(clusters, detail,
replication_data)
@wsgi.Controller.api_version(mv.CLUSTER_SUPPORT)
def update(self, req, id, body):
def _update(
self, context, name, binary, disabled, disabled_reason=None,
show_replication_data=False,
):
"""Enable/Disable scheduling for a cluster."""
# NOTE(geguileo): This method tries to be consistent with services
# update endpoint API.
# Let the wsgi middleware convert NotAuthorized exceptions
context = req.environ['cinder.context']
context.authorize(policy.UPDATE_POLICY)
if id not in ('enable', 'disable'):
raise exception.NotFound(message=_("Unknown action"))
disabled = id != 'enable'
disabled_reason = self._disable_cluster(
req, body=body) if disabled else self._enable_cluster(
req, body=body)
name = body['name']
binary = body.get('binary', constants.VOLUME_BINARY)
# Let wsgi handle NotFound exception
cluster = objects.Cluster.get_by_id(context, None, binary=binary,
name=name)
@@ -120,23 +106,56 @@ class ClusterController(wsgi.Controller):
cluster.save()
# We return summary data plus the disabled reason
replication_data = req.api_version_request.matches(
mv.REPLICATION_CLUSTER)
ret_val = clusters_view.ViewBuilder.summary(cluster, replication_data)
ret_val = clusters_view.ViewBuilder.summary(
cluster, show_replication_data,
)
ret_val['cluster']['disabled_reason'] = disabled_reason
return ret_val
@validation.schema(cluster.disable_cluster)
def _disable_cluster(self, req, body):
reason = body.get('disabled_reason')
if reason:
reason = reason.strip()
return reason
@wsgi.Controller.api_version(mv.CLUSTER_SUPPORT)
@validation.schema(schema.disable_cluster)
def disable(self, req, body):
# NOTE(geguileo): This method tries to be consistent with services
# disable endpoint API.
@validation.schema(cluster.enable_cluster)
def _enable_cluster(self, req, body):
pass
# Let the wsgi middleware convert NotAuthorized exceptions
context = req.environ['cinder.context']
context.authorize(policy.UPDATE_POLICY)
name = body['name']
binary = body.get('binary', constants.VOLUME_BINARY)
disabled_reason = body.get('disabled_reason')
if disabled_reason:
disabled_reason = disabled_reason.strip()
show_replication_data = req.api_version_request.matches(
mv.REPLICATION_CLUSTER)
return self._update(
context, name=name, binary=binary, disabled=True,
disabled_reason=disabled_reason,
show_replication_data=show_replication_data)
@wsgi.Controller.api_version(mv.CLUSTER_SUPPORT)
@validation.schema(schema.enable_cluster)
def enable(self, req, body):
# NOTE(geguileo): This method tries to be consistent with services
# enable endpoint API.
# Let the wsgi middleware convert NotAuthorized exceptions
context = req.environ['cinder.context']
context.authorize(policy.UPDATE_POLICY)
name = body['name']
binary = body.get('binary', constants.VOLUME_BINARY)
show_replication_data = req.api_version_request.matches(
mv.REPLICATION_CLUSTER)
return self._update(
context, name=name, binary=binary, disabled=False,
show_replication_data=show_replication_data)
def create_resource():
cinder/api/v3/router.py
+10 -3
View File
@@ -181,9 +181,16 @@ class APIRouter(base_wsgi.Router):
controller=self.resources['messages'])
self.resources['clusters'] = clusters.create_resource()
mapper.resource('cluster', 'clusters',
controller=self.resources['clusters'],
collection={'detail': 'GET'})
mapper.create_route(
'/clusters', 'GET', self.resources['clusters'], 'index')
mapper.create_route(
'/clusters/detail', 'GET', self.resources['clusters'], 'detail')
mapper.create_route(
'/clusters/{id}', 'GET', self.resources['clusters'], 'show')
mapper.create_route(
'/clusters/enable', 'PUT', self.resources['clusters'], 'enable')
mapper.create_route(
'/clusters/disable', 'PUT', self.resources['clusters'], 'disable')
self.resources['types'] = types.create_resource()
mapper.resource("type", "types",
cinder/tests/unit/api/v3/test_cluster.py
+56 -38
View File
@@ -305,16 +305,15 @@ class ClustersTestCase(test.TestCase):
side_effect=fake_db_api_cluster_update)
@mock.patch('cinder.db.sqlalchemy.api.cluster_get',
side_effect=fake_db_api_cluster_get)
def test_update_enable(self, get_mock, update_mock):
def test_enable(self, get_mock, update_mock):
req = FakeRequest()
expected = {'cluster': {'name': 'cluster_name',
'binary': 'cinder-volume',
'state': 'up',
'status': 'enabled',
'disabled_reason': None}}
res = self.controller.update(req, 'enable',
body={'name': 'cluster_name',
'binary': 'cinder-volume'})
res = self.controller.enable(
req, body={'name': 'cluster_name', 'binary': 'cinder-volume'})
self.assertEqual(expected, res)
ctxt = req.environ['cinder.context']
get_mock.assert_called_once_with(ctxt,
@@ -328,7 +327,7 @@ class ClustersTestCase(test.TestCase):
side_effect=fake_db_api_cluster_update)
@mock.patch('cinder.db.sqlalchemy.api.cluster_get',
side_effect=fake_db_api_cluster_get)
def test_update_disable(self, get_mock, update_mock):
def test_disable(self, get_mock, update_mock):
req = FakeRequest()
disabled_reason = 'For testing'
expected = {'cluster': {'name': 'cluster_name',
@@ -336,10 +335,9 @@ class ClustersTestCase(test.TestCase):
'binary': 'cinder-volume',
'status': 'disabled',
'disabled_reason': disabled_reason}}
res = self.controller.update(req, 'disable',
body={'name': 'cluster_name',
'binary': 'cinder-volume',
'disabled_reason': disabled_reason})
res = self.controller.disable(
req, body={'name': 'cluster_name', 'binary': 'cinder-volume',
'disabled_reason': disabled_reason})
self.assertEqual(expected, res)
ctxt = req.environ['cinder.context']
get_mock.assert_called_once_with(ctxt,
@@ -349,46 +347,66 @@ class ClustersTestCase(test.TestCase):
ctxt, 1,
{'disabled': True, 'disabled_reason': disabled_reason})
def test_update_wrong_action(self):
def test_enable_missing_name(self):
req = FakeRequest()
self.assertRaises(exception.NotFound, self.controller.update,
req, 'action', body={'name': 'cluster_name'})
self.assertRaises(exception.ValidationError, self.controller.enable,
req, body={'binary': 'cinder-volume'})
@ddt.data('enable', 'disable')
def test_update_missing_name(self, action):
def test_disable_missing_name(self):
req = FakeRequest()
self.assertRaises(exception.ValidationError, self.controller.update,
req, action, body={'binary': 'cinder-volume'})
self.assertRaises(
exception.ValidationError,
self.controller.disable,
req,
body={'binary': 'cinder-volume'})
def test_update_with_binary_more_than_255_characters(self):
def test_enable_with_binary_more_than_255_characters(self):
req = FakeRequest()
self.assertRaises(exception.ValidationError, self.controller.update,
req, 'enable', body={'name': 'cluster_name',
'binary': 'a' * 256})
self.assertRaises(
exception.ValidationError,
self.controller.enable,
req,
body={'name': 'cluster_name', 'binary': 'a' * 256})
def test_update_with_name_more_than_255_characters(self):
def test_enable_with_name_more_than_255_characters(self):
req = FakeRequest()
self.assertRaises(exception.ValidationError, self.controller.update,
req, 'enable', body={'name': 'a' * 256,
'binary': 'cinder-volume'})
self.assertRaises(
exception.ValidationError,
self.controller.enable,
req,
body={'name': 'a' * 256, 'binary': 'cinder-volume'})
@ddt.data('a' * 256, ' ')
def test_update_wrong_disabled_reason(self, disabled_reason):
def test_enable_wrong_disabled_reason(self, disabled_reason):
req = FakeRequest()
self.assertRaises(exception.ValidationError, self.controller.update,
req, 'disable',
body={'name': 'cluster_name',
'disabled_reason': disabled_reason})
self.assertRaises(
exception.ValidationError,
self.controller.enable,
req,
body={'name': 'cluster_name', 'disabled_reason': disabled_reason})
@ddt.data('enable', 'disable')
def test_update_unauthorized(self, action):
def test_enable_unauthorized(self):
req = FakeRequest(is_admin=False)
self.assertRaises(exception.PolicyNotAuthorized,
self.controller.update, req, action,
body={'name': 'fake_name'})
self.assertRaises(
exception.PolicyNotAuthorized,
self.controller.enable,
req, body={'name': 'fake_name'})
@ddt.data('enable', 'disable')
def test_update_wrong_version(self, action):
def test_disable_unauthorized(self):
req = FakeRequest(is_admin=False)
self.assertRaises(
exception.PolicyNotAuthorized,
self.controller.disable,
req, body={'name': 'fake_name'})
def test_enable_wrong_version(self):
req = FakeRequest(version=mv.get_prior_version(mv.CLUSTER_SUPPORT))
self.assertRaises(exception.VersionNotFoundForAPIMethod,
self.controller.update, req, action, {})
self.assertRaises(
exception.VersionNotFoundForAPIMethod,
self.controller.enable, req, {})
def test_disable_wrong_version(self):
req = FakeRequest(version=mv.get_prior_version(mv.CLUSTER_SUPPORT))
self.assertRaises(
exception.VersionNotFoundForAPIMethod,
self.controller.disable, req, {})