Merge "Fix microversion header parsing for service type prefix"

cyborg/api/controllers/base.py

@@ -17,16 +17,21 @@ import datetime
 import functools
 import inspect
 
+import microversion_parse
 import pecan
 import wsme
 
+from oslo_log import log as logging
 from pecan import rest
 from webob import exc
 from wsme import types as wtypes
 
 
+LOG = logging.getLogger(__name__)
+
+
 API_V2 = 'v2'
-# name of attribute to keep version method information
+SERVICE_TYPE = 'accelerator'
 
 
 class APIBase(wtypes.Base):
@@ -104,6 +109,18 @@ class Version:
     def parse_headers(headers, default_version, latest_version):
         """Determine the API version requested based on the headers supplied.
 
+        Per the `OpenStack API-WG guideline`_ the ``OpenStack-API-Version``
+        header carries the service type followed by the requested version,
+        for example ``OpenStack-API-Version: accelerator 2.1``.
+        ``microversion_parse`` handles this standard format.  A bare
+        version string without the service type prefix is also accepted
+        for backward compatibility with older clients but triggers a
+        deprecation warning.
+
+        .. _OpenStack API-WG guideline:
+           https://specs.openstack.org/openstack/api-wg/guidelines/
+           microversion_specification.html
+
         :param headers: webob headers
         :param default_version: version to use if not specified in headers
         :param latest_version: version to use if latest is requested
@@ -111,12 +128,32 @@ class Version:
         :raises: webob.HTTPNotAcceptable
 
         """
-        version_str = headers.get(Version.current_api_version, default_version)
         minimal_version = (2, 0)
+        service_type = SERVICE_TYPE
+
+        # Try the standard format first:
+        #   OpenStack-API-Version: accelerator <version>
+        version_str = microversion_parse.get_version(
+            headers, service_type=service_type
+        )
+
+        if version_str is None:
+            # Fall back to a bare version string without the service
+            # type prefix for backward compatibility.
+            raw = headers.get(Version.current_api_version)
+            if raw is not None:
+                LOG.warning(
+                    'Received OpenStack-API-Version header without '
+                    'the service type prefix.  Clients should send '
+                    "'OpenStack-API-Version: %s <version>' "
+                    'per the API-WG guideline.',
+                    service_type,
+                )
+                version_str = raw
+            else:
+                version_str = default_version
 
         if version_str is None:
-            # If requested header is wrong, Cyborg answers with the minimal
-            # supported version.
             return minimal_version
 
         if version_str.lower() == 'latest':
@@ -125,15 +162,12 @@ class Version:
             parse_str = version_str
 
         try:
-            version = tuple(int(i) for i in parse_str.split('.'))
-        except ValueError:
-            version = minimal_version
-
-        if len(version) != 2:
+            version = microversion_parse.parse_version_string(parse_str)
+        except TypeError:
             raise exc.HTTPNotAcceptable(
                 "Invalid value for %s header" % Version.current_api_version
             )
-        return version
+        return (version.major, version.minor)
 
     def __gt__(self, other):
         return (self.major, self.minor) > (other.major, other.minor)

cyborg/api/controllers/v2/__init__.py

@@ -34,7 +34,10 @@ from cyborg.api.controllers.v2 import versions
 
 def min_version():
     return base.Version(
-        {base.Version.current_api_version: versions.min_version_string()},
+        {
+            base.Version.current_api_version: '%s %s'
+            % (versions.service_type_string(), versions.min_version_string())
+        },
         versions.min_version_string(),
         versions.max_version_string(),
     )
@@ -42,7 +45,10 @@ def min_version():
 
 def max_version():
     return base.Version(
-        {base.Version.current_api_version: versions.max_version_string()},
+        {
+            base.Version.current_api_version: '%s %s'
+            % (versions.service_type_string(), versions.max_version_string())
+        },
         versions.min_version_string(),
         versions.max_version_string(),
     )
@@ -145,7 +151,10 @@ class Controller(rest.RestController):
 
         # assert that requested version is supported
         self._check_version(v, pecan.response.headers)
-        pecan.response.headers[base.Version.current_api_version] = str(v)
+        pecan.response.headers[base.Version.current_api_version] = "%s %s" % (
+            versions.service_type_string(),
+            v,
+        )
         pecan.request.version = v
 
         return super()._route(args, request)

cyborg/api/controllers/v2/versions.py

@@ -14,6 +14,9 @@
 #    under the License.
 
 
+from cyborg.api.controllers import base
+
+
 # This is the version 2 API
 BASE_VERSION = 2
 
@@ -45,7 +48,7 @@ _MAX_VERSION_STRING = '{}.{}'.format(BASE_VERSION, MINOR_MAX_VERSION)
 
 
 def service_type_string():
-    return 'accelerator'
+    return base.SERVICE_TYPE
 
 
 def min_version_string():

cyborg/tests/unit/api/controllers/v2/test_microversion.py

@@ -20,6 +20,9 @@ H_MAX_VER = 'openstack-api-maximum-version'
 H_RESP_VER = 'openstack-api-version'
 MIN_VER = versions.min_version_string()
 MAX_VER = versions.max_version_string()
+# Response header value includes the service type prefix per API-WG spec.
+RESP_MIN = '%s %s' % (SERVICE_TYPE, MIN_VER)
+RESP_MAX = '%s %s' % (SERVICE_TYPE, MAX_VER)
 
 
 class TestMicroversions(api_base.BaseApiTest):
@@ -54,7 +57,7 @@ class TestMicroversions(api_base.BaseApiTest):
         response = self.get_json('/v2', return_json=False)
         self.assertEqual(response.headers[H_MIN_VER], MIN_VER)
         self.assertEqual(response.headers[H_MAX_VER], MAX_VER)
-        self.assertEqual(response.headers[H_RESP_VER], MIN_VER)
+        self.assertEqual(response.headers[H_RESP_VER], RESP_MIN)
         self.assertTrue(
             all(
                 x in response.json.keys()
@@ -68,7 +71,7 @@ class TestMicroversions(api_base.BaseApiTest):
         )
         self.assertEqual(response.headers[H_MIN_VER], MIN_VER)
         self.assertEqual(response.headers[H_MAX_VER], MAX_VER)
-        self.assertEqual(response.headers[H_RESP_VER], '2.0')
+        self.assertEqual(response.headers[H_RESP_VER], RESP_MIN)
         self.assertTrue(
             all(
                 x in response.json.keys()
@@ -84,7 +87,7 @@ class TestMicroversions(api_base.BaseApiTest):
         )
         self.assertEqual(response.headers[H_MIN_VER], MIN_VER)
         self.assertEqual(response.headers[H_MAX_VER], MAX_VER)
-        self.assertEqual(response.headers[H_RESP_VER], MAX_VER)
+        self.assertEqual(response.headers[H_RESP_VER], RESP_MAX)
         self.assertTrue(
             all(
                 x in response.json.keys()
@@ -109,3 +112,28 @@ class TestMicroversions(api_base.BaseApiTest):
         )
         self.assertTrue(response.json['error_message'])
         self.assertIn(expected_error_msg, response.json['error_message'])
+
+    def test_standard_header_with_service_type(self):
+        """The API-WG standard format includes the service type.
+
+        OpenStack-API-Version: accelerator 2.0
+        """
+        response = self.get_json(
+            '/v2',
+            headers={'OpenStack-API-Version': 'accelerator 2.0'},
+            return_json=False,
+        )
+        self.assertEqual(response.headers[H_MIN_VER], MIN_VER)
+        self.assertEqual(response.headers[H_MAX_VER], MAX_VER)
+        self.assertEqual(response.headers[H_RESP_VER], RESP_MIN)
+
+    def test_standard_header_with_service_type_latest(self):
+        """The API-WG standard format with 'latest'."""
+        response = self.get_json(
+            '/v2',
+            headers={'OpenStack-API-Version': 'accelerator latest'},
+            return_json=False,
+        )
+        self.assertEqual(response.headers[H_MIN_VER], MIN_VER)
+        self.assertEqual(response.headers[H_MAX_VER], MAX_VER)
+        self.assertEqual(response.headers[H_RESP_VER], RESP_MAX)

releasenotes/notes/fix-microversion-header-service-type-parsing-a1b2c3d4e5f6g7h8.yaml

@@ -0,0 +1,14 @@
+---
+fixes:
+  - |
+    Restored support for the standard ``OpenStack-API-Version`` header
+    format that includes the service type prefix
+    (``OpenStack-API-Version: accelerator 2.1``), as specified by the
+    `API-WG microversion guideline
+    <https://specs.openstack.org/openstack/api-wg/guidelines/microversion_specification.html>`_.
+    A previous change inadvertently removed parsing of the service type
+    prefix, causing clients that follow the standard (including
+    keystoneauth and openstacksdk) to always fall back to the minimum
+    API version.  Both the prefixed and bare version formats are now
+    accepted; a deprecation warning is logged when the service type
+    prefix is omitted.

requirements.txt

@@ -25,6 +25,7 @@ SQLAlchemy>=1.4.0 # MIT
 alembic>=0.8.10 # MIT
 stevedore>=1.5.0 # Apache-2.0
 keystonemiddleware>=4.17.0 # Apache-2.0
+microversion-parse>=1.0.0 # Apache-2.0
 jsonpatch>=1.16,!=1.20 # BSD
 psutil>=3.2.2 # BSD
 python-glanceclient>=2.3.0 # Apache-2.0