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

@@ -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``.

doc/source/devref/index.rst

@@ -59,6 +59,7 @@ API Reference
    api
    api_microversion_dev
    api_microversion_history
+   experimental_apis
 
 Module Reference
 ----------------

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})
 

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):

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()

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)

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())

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)
+        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))