From ba640a8d0765c9854c187357e69a42b9243830ff Mon Sep 17 00:00:00 2001 From: Clinton Knight Date: Sun, 16 Aug 2015 01:23:04 -0400 Subject: [PATCH] Manila experimental REST APIs Manila uses API microversions to allow natural evolution of its REST APIs over time. But microversions alone cannot solve the question of how to ship APIs that are experimental in nature, are expected to change at any time, and could even be removed entirely without a typical deprecation period. Working in conjunction with microversions, this commit adds a facility for marking individual REST APIs as experimental. Implements bp: manila-experimental-rest-apis Change-Id: I263a0b5579a7eb2fe98ca810ad3dec6719d66e6f --- doc/source/devref/experimental_apis.rst | 75 ++++++++++++ doc/source/devref/index.rst | 1 + manila/api/openstack/api_version_request.py | 50 ++++++-- manila/api/openstack/versioned_method.py | 4 +- manila/api/openstack/wsgi.py | 27 ++++- .../api/openstack/test_api_version_request.py | 80 +++++++++++- .../api/openstack/test_versioned_method.py | 5 +- manila/tests/api/test_versions.py | 114 ++++++++++++++---- 8 files changed, 315 insertions(+), 41 deletions(-) create mode 100644 doc/source/devref/experimental_apis.rst diff --git a/doc/source/devref/experimental_apis.rst b/doc/source/devref/experimental_apis.rst new file mode 100644 index 0000000000..74b8b0885d --- /dev/null +++ b/doc/source/devref/experimental_apis.rst @@ -0,0 +1,75 @@ +Experimental APIs +================= + +Background +---------- + +Manila uses API microversions to allow natural evolution of its REST APIs +over time. But microversions alone cannot solve the question of how to +ship APIs that are experimental in nature, are expected to change at any +time, and could even be removed entirely without a typical deprecation +period. + +In conjunction with microversions, Manila has added a facility for marking +individual REST APIs as experimental. To call an experimental API, clients +must include a specific HTTP header, ``X-OpenStack-Manila-API-Experimental``, +with a value of ``True``. If a user calls an experimental API without +including the experimental header, the server would respond with ``HTTP/404``. +This forces the client to acknowledge the experimental status of the API and +prevents anyone from building an application around a Manila feature without +realizing the feature could change significantly or even disappear. + +On the other hand, if a request is made to a non-experimental Manila API with +``X-OpenStack-Manila-API-Experimental: True``, the server would respond as if +the header had not been included. This is a convenience mechanism, as it +allows the client to specify both the requested API version as well as the +experimental header (if desired) in one place instead of having to set the +headers separately for each API call (although that would be fine, too). + +When do I need to set an API experimental? +------------------------------------------ + +An API should be marked as experimental if any of the following is true: + +- the API is not yet considered a stable, core API + +- the API is expected to change in later releases + +- the API could be removed altogether if a feature is redesigned + +- the API controls a feature that could change or be removed + +When do I need to remove the experimental annotation from an API? +----------------------------------------------------------------- + +When the community is satisfied that an experimental feature and its APIs +have had sufficient time to gather and incorporate user feedback to consider +it stable, which could be one or more OpenStack release cycles, any relevant +APIs must be re-released with a microversion bump and without the experimental +flag. The maturation period can vary between features, but experimental is NOT +a stable state, and an experimental feature should not be left in that state +any longer than necessary. + +Because experimental APIs have no conventional deprecation period, the Manila +core team may optionally choose to remove any experimental versions of an API +at the same time that a microversioned stable version is added. + +In Code +------- + +The ``@api_version`` decorator defined in ``manila/api/openstack/wsgi.py``, +which is used for specifying API versions on top-level Controller methods, +also allows for tagging an API as experimental. For example: + +In the controller class:: + + @wsgi.Controller.api_version("2.4", experimental=True) + def my_api_method(self, req, id): + .... + +This method would only be available if the caller had specified an +``X-OpenStack-Manila-API-Version`` of >= ``2.4``. and had also included +``X-OpenStack-Manila-API-Experimental: True``. If they had specified a +lower version (or not specified it and received a lower default version), +or if they had failed to include the experimental header, the server would +respond with ``HTTP/404``. diff --git a/doc/source/devref/index.rst b/doc/source/devref/index.rst index 79f04f39ee..d2a7592597 100644 --- a/doc/source/devref/index.rst +++ b/doc/source/devref/index.rst @@ -59,6 +59,7 @@ API Reference api api_microversion_dev api_microversion_history + experimental_apis Module Reference ---------------- diff --git a/manila/api/openstack/api_version_request.py b/manila/api/openstack/api_version_request.py index 707fe0e841..d5d703c726 100644 --- a/manila/api/openstack/api_version_request.py +++ b/manila/api/openstack/api_version_request.py @@ -16,7 +16,9 @@ import re +from manila.api.openstack import versioned_method from manila import exception +from manila.i18n import _ from manila import utils # Define the minimum and maximum version of the API across all of the @@ -76,31 +78,55 @@ class APIVersionRequest(utils.ComparableMixin): def __init__(self, version_string=None): """Create an API version request object.""" - self.ver_major = None - self.ver_minor = None + self._ver_major = None + self._ver_minor = None + self._experimental = False if version_string is not None: match = re.match(r"^([1-9]\d*)\.([1-9]\d*|0)$", version_string) if match: - self.ver_major = int(match.group(1)) - self.ver_minor = int(match.group(2)) + self._ver_major = int(match.group(1)) + self._ver_minor = int(match.group(2)) else: raise exception.InvalidAPIVersionString(version=version_string) def __str__(self): """Debug/Logging representation of object.""" return ("API Version Request Major: %(major)s, Minor: %(minor)s" - % {'major': self.ver_major, 'minor': self.ver_minor}) + % {'major': self._ver_major, 'minor': self._ver_minor}) def is_null(self): - return self.ver_major is None and self.ver_minor is None + return self._ver_major is None and self._ver_minor is None def _cmpkey(self): """Return the value used by ComparableMixin for rich comparisons.""" - return self.ver_major, self.ver_minor + return self._ver_major, self._ver_minor - def matches(self, min_version, max_version): + @property + def experimental(self): + return self._experimental + + @experimental.setter + def experimental(self, value): + if type(value) != bool: + msg = _('The experimental property must be a bool value.') + raise exception.InvalidParameterValue(err=msg) + self._experimental = value + + def matches_versioned_method(self, method): + """Compares this version to that of a versioned method.""" + + if type(method) != versioned_method.VersionedMethod: + msg = _('An API version request must be compared ' + 'to a VersionedMethod object.') + raise exception.InvalidParameterValue(err=msg) + + return self.matches(method.start_version, + method.end_version, + method.experimental) + + def matches(self, min_version, max_version, experimental=False): """Compares this version to the specified min/max range. Returns whether the version object represents a version @@ -113,11 +139,17 @@ class APIVersionRequest(utils.ComparableMixin): :param min_version: Minimum acceptable version. :param max_version: Maximum acceptable version. + :param experimental: Whether to match experimental APIs. :returns: boolean """ if self.is_null(): raise ValueError + # NOTE(cknight): An experimental request should still match a + # non-experimental API, so the experimental check isn't just + # looking for equality. + if not self.experimental and experimental: + return False if max_version.is_null() and min_version.is_null(): return True elif max_version.is_null(): @@ -136,5 +168,5 @@ class APIVersionRequest(utils.ComparableMixin): if self.is_null(): raise ValueError return ("%(major)s.%(minor)s" % - {'major': self.ver_major, 'minor': self.ver_minor}) + {'major': self._ver_major, 'minor': self._ver_minor}) diff --git a/manila/api/openstack/versioned_method.py b/manila/api/openstack/versioned_method.py index 178bc408f0..ed096bbbf9 100644 --- a/manila/api/openstack/versioned_method.py +++ b/manila/api/openstack/versioned_method.py @@ -19,7 +19,7 @@ from manila import utils class VersionedMethod(utils.ComparableMixin): - def __init__(self, name, start_version, end_version, func): + def __init__(self, name, start_version, end_version, experimental, func): """Versioning information for a single method. Minimum and maximums are inclusive. @@ -27,11 +27,13 @@ class VersionedMethod(utils.ComparableMixin): :param name: Name of the method :param start_version: Minimum acceptable version :param end_version: Maximum acceptable_version + :param experimental: True if method is experimental :param func: Method to call """ self.name = name self.start_version = start_version self.end_version = end_version + self.experimental = experimental self.func = func def __str__(self): diff --git a/manila/api/openstack/wsgi.py b/manila/api/openstack/wsgi.py index e368226a47..cc5401dbb3 100644 --- a/manila/api/openstack/wsgi.py +++ b/manila/api/openstack/wsgi.py @@ -49,6 +49,7 @@ VER_METHOD_ATTR = 'versioned_methods' # Name of header used by clients to request a specific version # of the REST API API_VERSION_REQUEST_HEADER = 'X-OpenStack-Manila-API-Version' +EXPERIMENTAL_API_REQUEST_HEADER = 'X-OpenStack-Manila-API-Experimental' class Request(webob.Request): @@ -231,6 +232,11 @@ class Request(webob.Request): self.api_version_request = api_version.APIVersionRequest( api_version.DEFAULT_API_VERSION) + # Check if experimental API was requested + if EXPERIMENTAL_API_REQUEST_HEADER in self.headers: + self.api_version_request.experimental = strutils.bool_from_string( + self.headers[EXPERIMENTAL_API_REQUEST_HEADER]) + class ActionDispatcher(object): """Maps method name to local methods through action name.""" @@ -849,6 +855,9 @@ class Resource(wsgi.Application): if not request.api_version_request.is_null(): response.headers[API_VERSION_REQUEST_HEADER] = ( request.api_version_request.get_string()) + if request.api_version_request.experimental: + response.headers[EXPERIMENTAL_API_REQUEST_HEADER] = ( + request.api_version_request.experimental) response.headers['Vary'] = API_VERSION_REQUEST_HEADER return response @@ -1017,13 +1026,13 @@ class Controller(object): # object. The version for the request is attached to the # request object if len(args) == 0: - ver = kwargs['req'].api_version_request + version_request = kwargs['req'].api_version_request else: - ver = args[0].api_version_request + version_request = args[0].api_version_request func_list = self.versioned_methods[key] for func in func_list: - if ver.matches(func.start_version, func.end_version): + if version_request.matches_versioned_method(func): # Update the version_select wrapper function so # other decorator attributes like wsgi.response # are still respected. @@ -1031,7 +1040,8 @@ class Controller(object): return func.func(self, *args, **kwargs) # No version match - raise exception.VersionNotFoundForAPIMethod(version=ver) + raise exception.VersionNotFoundForAPIMethod( + version=version_request) try: version_meth_dict = object.__getattribute__(self, VER_METHOD_ATTR) @@ -1048,7 +1058,7 @@ class Controller(object): # NOTE(cyeoh): This decorator MUST appear first (the outermost # decorator) on an API method for it to work correctly @classmethod - def api_version(cls, min_ver, max_ver=None): + def api_version(cls, min_ver, max_ver=None, experimental=False): """Decorator for versioning API methods. Add the decorator to any method which takes a request object @@ -1057,6 +1067,8 @@ class Controller(object): :param min_ver: string representing minimum version :param max_ver: optional string representing maximum version + :param experimental: flag indicating an API is experimental and is + subject to change or removal at any time """ def decorator(f): @@ -1069,7 +1081,7 @@ class Controller(object): # Add to list of versioned methods registered func_name = f.__name__ new_func = versioned_method.VersionedMethod( - func_name, obj_min_ver, obj_max_ver, f) + func_name, obj_min_ver, obj_max_ver, experimental, f) func_dict = getattr(cls, VER_METHOD_ATTR, {}) if not func_dict: @@ -1145,6 +1157,9 @@ class Fault(webob.exc.HTTPException): if not req.api_version_request.is_null(): self.wrapped_exc.headers[API_VERSION_REQUEST_HEADER] = ( req.api_version_request.get_string()) + if req.api_version_request.experimental: + self.wrapped_exc.headers[EXPERIMENTAL_API_REQUEST_HEADER] = ( + req.api_version_request.experimental) self.wrapped_exc.headers['Vary'] = API_VERSION_REQUEST_HEADER content_type = req.best_match_content_type() diff --git a/manila/tests/api/openstack/test_api_version_request.py b/manila/tests/api/openstack/test_api_version_request.py index 4f4b4267a4..9193a5c821 100644 --- a/manila/tests/api/openstack/test_api_version_request.py +++ b/manila/tests/api/openstack/test_api_version_request.py @@ -18,6 +18,7 @@ import ddt import six from manila.api.openstack import api_version_request +from manila.api.openstack import versioned_method from manila import exception from manila import test @@ -25,6 +26,26 @@ from manila import test @ddt.ddt class APIVersionRequestTests(test.TestCase): + def test_init(self): + + result = api_version_request.APIVersionRequest() + + self.assertIsNone(result._ver_major) + self.assertIsNone(result._ver_minor) + self.assertFalse(result._experimental) + + def test_min_version(self): + + self.assertEqual( + api_version_request.APIVersionRequest(api_version_request._MIN_API_VERSION), + api_version_request.min_api_version()) + + def test_max_api_version(self): + + self.assertEqual( + api_version_request.APIVersionRequest(api_version_request._MAX_API_VERSION), + api_version_request.max_api_version()) + @ddt.data( ('1.1', 1, 1), ('2.10', 2, 10), @@ -38,8 +59,8 @@ class APIVersionRequestTests(test.TestCase): request = api_version_request.APIVersionRequest(version_string) - self.assertEqual(major, request.ver_major) - self.assertEqual(minor, request.ver_minor) + self.assertEqual(major, request._ver_major) + self.assertEqual(minor, request._ver_minor) def test_null_version(self): @@ -59,6 +80,23 @@ class APIVersionRequestTests(test.TestCase): request = api_version_request.APIVersionRequest('1.2') self.assertEqual((1, 2), request._cmpkey()) + @ddt.data(True, False) + def test_experimental_property(self, experimental): + + request = api_version_request.APIVersionRequest() + request.experimental = experimental + + self.assertEqual(experimental, request.experimental) + + def test_experimental_property_value_error(self): + + request = api_version_request.APIVersionRequest() + + def set_non_boolean(): + request.experimental = 'non_bool_value' + + self.assertRaises(exception.InvalidParameterValue, set_non_boolean) + def test_version_comparisons(self): v1 = api_version_request.APIVersionRequest('2.0') v2 = api_version_request.APIVersionRequest('2.5') @@ -100,6 +138,44 @@ class APIVersionRequestTests(test.TestCase): self.assertRaises(ValueError, v_null.matches, v1, v3) + def test_version_matches_experimental_request(self): + + experimental_request = api_version_request.APIVersionRequest('2.0') + experimental_request.experimental = True + + non_experimental_request = api_version_request.APIVersionRequest('2.0') + + experimental_function = versioned_method.VersionedMethod( + 'experimental_function', + api_version_request.APIVersionRequest('2.0'), + api_version_request.APIVersionRequest('2.1'), + True, + None) + + non_experimental_function = versioned_method.VersionedMethod( + 'non_experimental_function', + api_version_request.APIVersionRequest('2.0'), + api_version_request.APIVersionRequest('2.1'), + False, + None) + + self.assertTrue(experimental_request.matches_versioned_method( + experimental_function)) + self.assertTrue(experimental_request.matches_versioned_method( + non_experimental_function)) + self.assertTrue(non_experimental_request.matches_versioned_method( + non_experimental_function)) + self.assertFalse(non_experimental_request.matches_versioned_method( + experimental_function)) + + def test_matches_versioned_method(self): + + request = api_version_request.APIVersionRequest('2.0') + + self.assertRaises(exception.InvalidParameterValue, + request.matches_versioned_method, + 'fake_method') + def test_get_string(self): v1_string = '3.23' v1 = api_version_request.APIVersionRequest(v1_string) diff --git a/manila/tests/api/openstack/test_versioned_method.py b/manila/tests/api/openstack/test_versioned_method.py index fbcb896338..4bf6b725bf 100644 --- a/manila/tests/api/openstack/test_versioned_method.py +++ b/manila/tests/api/openstack/test_versioned_method.py @@ -23,7 +23,7 @@ class VersionedMethodTestCase(test.TestCase): def test_str(self): args = ('fake_name', 'fake_min', 'fake_max') - method = versioned_method.VersionedMethod(*(args + (None,))) + method = versioned_method.VersionedMethod(*(args + (False, None))) method_string = six.text_type(method) self.assertEqual('Version Method %s: min: %s, max: %s' % args, @@ -31,5 +31,6 @@ class VersionedMethodTestCase(test.TestCase): def test_cmpkey(self): method = versioned_method.VersionedMethod( - 'fake_name', 'fake_start_version', 'fake_end_version', 'fake_func') + 'fake_name', 'fake_start_version', 'fake_end_version', False, + 'fake_func') self.assertEqual('fake_start_version', method._cmpkey()) \ No newline at end of file diff --git a/manila/tests/api/test_versions.py b/manila/tests/api/test_versions.py index 458e67107c..7f620623a9 100644 --- a/manila/tests/api/test_versions.py +++ b/manila/tests/api/test_versions.py @@ -25,11 +25,13 @@ from manila import test from manila.tests.api import fakes +version_header_name = 'X-OpenStack-Manila-API-Version' +experimental_header_name = 'X-OpenStack-Manila-API-Experimental' + + @ddt.ddt class VersionsControllerTestCase(test.TestCase): - version_header_name = 'X-OpenStack-Manila-API-Version' - def setUp(self): super(VersionsControllerTestCase, self).setUp() self.wsgi_apps = (versions.VersionsRouter(), router.APIRouter()) @@ -57,7 +59,7 @@ class VersionsControllerTestCase(test.TestCase): req.method = 'GET' req.content_type = 'application/json' if include_header: - req.headers = {self.version_header_name: '1.0'} + req.headers = {version_header_name: '1.0'} for app in self.wsgi_apps: response = req.get_response(app) @@ -66,9 +68,8 @@ class VersionsControllerTestCase(test.TestCase): ids = [v['id'] for v in version_list] self.assertEqual({'v1.0'}, set(ids)) - self.assertEqual('1.0', response.headers[self.version_header_name]) - self.assertEqual(self.version_header_name, - response.headers['Vary']) + self.assertEqual('1.0', response.headers[version_header_name]) + self.assertEqual(version_header_name, response.headers['Vary']) self.assertIsNone(version_list[0].get('min_version')) self.assertIsNone(version_list[0].get('version')) @@ -83,7 +84,7 @@ class VersionsControllerTestCase(test.TestCase): req = fakes.HTTPRequest.blank('/', base_url=base_url) req.method = 'GET' req.content_type = 'application/json' - req.headers = {self.version_header_name: req_version} + req.headers = {version_header_name: req_version} for app in self.wsgi_apps: response = req.get_response(app) @@ -95,13 +96,12 @@ class VersionsControllerTestCase(test.TestCase): if req_version == 'latest': self.assertEqual(api_version_request._MAX_API_VERSION, - response.headers[self.version_header_name]) + response.headers[version_header_name]) else: self.assertEqual(req_version, - response.headers[self.version_header_name]) + response.headers[version_header_name]) - self.assertEqual(self.version_header_name, - response.headers['Vary']) + self.assertEqual(version_header_name, response.headers['Vary']) self.assertEqual(api_version_request._MIN_API_VERSION, version_list[0].get('min_version')) self.assertEqual(api_version_request._MAX_API_VERSION, @@ -112,30 +112,28 @@ class VersionsControllerTestCase(test.TestCase): req = fakes.HTTPRequest.blank('/', base_url=base_url) req.method = 'GET' req.content_type = 'application/json' - req.headers = {self.version_header_name: '2.0'} + req.headers = {version_header_name: '2.0'} for app in self.wsgi_apps: response = req.get_response(app) self.assertEqual(406, response.status_int) - self.assertEqual('2.0', response.headers[self.version_header_name]) - self.assertEqual(self.version_header_name, - response.headers['Vary']) + self.assertEqual('2.0', response.headers[version_header_name]) + self.assertEqual(version_header_name, response.headers['Vary']) @ddt.data('http://localhost/', None) def test_versions_index_invalid_version_request(self, base_url): req = fakes.HTTPRequest.blank('/', base_url=base_url) req.method = 'GET' req.content_type = 'application/json' - req.headers = {self.version_header_name: '2.0.1'} + req.headers = {version_header_name: '2.0.1'} for app in self.wsgi_apps: response = req.get_response(app) self.assertEqual(400, response.status_int) - self.assertEqual('1.0', response.headers[self.version_header_name]) - self.assertEqual(self.version_header_name, - response.headers['Vary']) + self.assertEqual('1.0', response.headers[version_header_name]) + self.assertEqual(version_header_name, response.headers['Vary']) def test_versions_version_not_found(self): api_version_request_3_0 = api_version_request.APIVersionRequest('3.0') @@ -149,8 +147,82 @@ class VersionsControllerTestCase(test.TestCase): return 'off' req = fakes.HTTPRequest.blank('/tests') - req.headers = {self.version_header_name: '2.0'} + req.headers = {version_header_name: '2.0'} app = fakes.TestRouter(Controller()) response = req.get_response(app) - self.assertEqual(404, response.status_int) \ No newline at end of file + self.assertEqual(404, response.status_int) + + +@ddt.ddt +class ExperimentalAPITestCase(test.TestCase): + + class Controller(wsgi.Controller): + @wsgi.Controller.api_version('1.0', '1.0') + def index(self, req): + return {'fake_key': 'fake_value'} + + @wsgi.Controller.api_version('1.1', '1.1', experimental=True) # noqa + def index(self, req): # pylint: disable=E0102 + return {'fake_key': 'fake_value'} + + def setUp(self): + super(ExperimentalAPITestCase, self).setUp() + self.app = fakes.TestRouter(ExperimentalAPITestCase.Controller()) + + @ddt.data(True, False) + def test_stable_api_always_called(self, experimental): + + req = fakes.HTTPRequest.blank('/tests') + req.headers = {version_header_name: '1.0'} + if experimental: + req.headers[experimental_header_name] = experimental + response = req.get_response(self.app) + + self.assertEqual(200, response.status_int) + self.assertEqual('1.0', response.headers[version_header_name]) + + if experimental: + self.assertEqual(experimental, + response.headers.get(experimental_header_name)) + else: + self.assertFalse(experimental_header_name in response.headers) + + def test_experimental_api_called_when_requested(self): + + req = fakes.HTTPRequest.blank('/tests') + req.headers = { + version_header_name: '1.1', + experimental_header_name: 'True', + } + response = req.get_response(self.app) + + self.assertEqual(200, response.status_int) + self.assertEqual('1.1', response.headers[version_header_name]) + self.assertTrue(response.headers.get(experimental_header_name)) + + def test_experimental_api_not_called_when_not_requested(self): + + req = fakes.HTTPRequest.blank('/tests') + req.headers = {version_header_name: '1.1'} + response = req.get_response(self.app) + + self.assertEqual(404, response.status_int) + self.assertFalse(experimental_header_name in response.headers) + + def test_experimental_header_returned_in_exception(self): + + api_version_request_3_0 = api_version_request.APIVersionRequest('3.0') + self.mock_object(api_version_request, + 'max_api_version', + mock.Mock(return_value=api_version_request_3_0)) + + req = fakes.HTTPRequest.blank('/tests') + req.headers = { + version_header_name: '1.2', + experimental_header_name: 'True', + } + response = req.get_response(self.app) + + self.assertEqual(404, response.status_int) + self.assertTrue(response.headers.get(experimental_header_name)) \ No newline at end of file