Allow re-negotiation of the latest version supplied by CLI

Currently --os-baremetal-api-version=latest results in the latest known API
version being send to the server. This does not work, if the server is older
than the client. This patch enables the version to be downgraded to the latest
server version in this case.

Change-Id: I3c48b3dbfef9ff3ee6001d27c8e1eb04341e98ec
Partial-Bug: #1671145

ironicclient/osc/plugin.py

@@ -42,6 +42,10 @@ MISSING_VERSION_WARNING = (
     "--os-baremetal-api-version argument with the desired version or using "
     "the OS_BAREMETAL_API_VERSION environment variable."
 )
+# NOTE(dtantsur): flag to indicate that the requested version was "latest".
+# Due to how OSC works we cannot just add "latest" to the list of supported
+# versions - it breaks the major version detection.
+OS_BAREMETAL_API_LATEST = False
 
 
 def make_client(instance):
@@ -50,15 +54,22 @@ def make_client(instance):
             utils.env('OS_BAREMETAL_API_VERSION')):
         LOG.warning(MISSING_VERSION_WARNING, http.DEFAULT_VER)
 
+    requested_api_version = instance._api_version[API_NAME]
+
     baremetal_client_class = utils.get_client_class(
         API_NAME,
-        instance._api_version[API_NAME],
+        requested_api_version,
         API_VERSIONS)
     LOG.debug('Instantiating baremetal client: %s', baremetal_client_class)
-    LOG.debug('Baremetal API version: %s', http.DEFAULT_VER)
+    LOG.debug('Baremetal API version: %s',
+              requested_api_version if not OS_BAREMETAL_API_LATEST
+              else "latest")
 
     client = baremetal_client_class(
-        os_ironic_api_version=instance._api_version[API_NAME],
+        os_ironic_api_version=requested_api_version,
+        # NOTE(dtantsur): enable re-negotiation of the latest version, if CLI
+        # latest is too high for the server we're talking to.
+        allow_api_version_downgrade=OS_BAREMETAL_API_LATEST,
         session=instance.session,
         region_name=instance._region_name,
         # NOTE(vdrok): This will be set as endpoint_override, and the Client
@@ -92,19 +103,29 @@ def build_option_parser(parser):
 
 
 def _get_environment_version(default):
+    global OS_BAREMETAL_API_LATEST
     env_value = utils.env('OS_BAREMETAL_API_VERSION')
     if not env_value:
-        return default
+        env_value = default
     if env_value == 'latest':
         env_value = LATEST_VERSION
+        OS_BAREMETAL_API_LATEST = True
     return env_value
 
 
 class ReplaceLatestVersion(argparse.Action):
-    """Replaces `latest` keyword by last known version."""
+    """Replaces `latest` keyword by last known version.
+
+    OSC cannot accept the literal "latest" as a supported API version as it
+    breaks the major version detection (OSC tries to load configuration options
+    from setuptools entrypoint openstack.baremetal.vlatest). This action
+    replaces "latest" with the latest known version, and sets the global
+    OS_BAREMETAL_API_LATEST flag to True.
+    """
     def __call__(self, parser, namespace, values, option_string=None):
-        global OS_BAREMETAL_API_VERSION_SPECIFIED
+        global OS_BAREMETAL_API_VERSION_SPECIFIED, OS_BAREMETAL_API_LATEST
         OS_BAREMETAL_API_VERSION_SPECIFIED = True
         if values == 'latest':
             values = LATEST_VERSION
+            OS_BAREMETAL_API_LATEST = True
         setattr(namespace, self.dest, values)

ironicclient/tests/unit/osc/test_plugin.py

@@ -24,6 +24,7 @@ from ironicclient.v1 import client
 class MakeClientTest(testtools.TestCase):
 
     @mock.patch.object(plugin.utils, 'env', lambda x: None)
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=False)
     @mock.patch.object(plugin, 'OS_BAREMETAL_API_VERSION_SPECIFIED', new=True)
     @mock.patch.object(plugin.LOG, 'warning', autospec=True)
     @mock.patch.object(client, 'Client', autospec=True)
@@ -33,6 +34,7 @@ class MakeClientTest(testtools.TestCase):
             return_value='endpoint')
         plugin.make_client(instance)
         mock_client.assert_called_once_with(os_ironic_api_version='1.6',
+                                            allow_api_version_downgrade=False,
                                             session=instance.session,
                                             region_name=instance._region_name,
                                             endpoint='endpoint')
@@ -42,6 +44,7 @@ class MakeClientTest(testtools.TestCase):
             interface=instance.interface)
 
     @mock.patch.object(plugin.utils, 'env', lambda x: None)
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=False)
     @mock.patch.object(plugin, 'OS_BAREMETAL_API_VERSION_SPECIFIED', new=False)
     @mock.patch.object(plugin.LOG, 'warning', autospec=True)
     @mock.patch.object(client, 'Client', autospec=True)
@@ -54,6 +57,7 @@ class MakeClientTest(testtools.TestCase):
         plugin.make_client(instance)
         mock_client.assert_called_once_with(
             os_ironic_api_version=http.DEFAULT_VER,
+            allow_api_version_downgrade=False,
             session=instance.session,
             region_name=instance._region_name,
             endpoint='endpoint')
@@ -62,7 +66,28 @@ class MakeClientTest(testtools.TestCase):
             'baremetal', region_name=instance._region_name,
             interface=instance.interface)
 
+    @mock.patch.object(plugin.utils, 'env', lambda x: None)
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=True)
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_VERSION_SPECIFIED', new=True)
+    @mock.patch.object(plugin.LOG, 'warning', autospec=True)
+    @mock.patch.object(client, 'Client', autospec=True)
+    def test_make_client_latest(self, mock_client, mock_warning):
+        instance = fakes.FakeClientManager()
+        instance.get_endpoint_for_service_type = mock.Mock(
+            return_value='endpoint')
+        plugin.make_client(instance)
+        mock_client.assert_called_once_with(os_ironic_api_version='1.6',
+                                            allow_api_version_downgrade=True,
+                                            session=instance.session,
+                                            region_name=instance._region_name,
+                                            endpoint='endpoint')
+        self.assertFalse(mock_warning.called)
+        instance.get_endpoint_for_service_type.assert_called_once_with(
+            'baremetal', region_name=instance._region_name,
+            interface=instance.interface)
+
     @mock.patch.object(plugin.utils, 'env', lambda x: '1.29')
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=False)
     @mock.patch.object(plugin, 'OS_BAREMETAL_API_VERSION_SPECIFIED', new=False)
     @mock.patch.object(plugin.LOG, 'warning', autospec=True)
     @mock.patch.object(client, 'Client', autospec=True)
@@ -122,6 +147,7 @@ class BuildOptionParserTest(testtools.TestCase):
 
 class ReplaceLatestVersionTest(testtools.TestCase):
 
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=False)
     @mock.patch.object(plugin.utils, 'env', lambda x: None)
     def test___call___latest(self):
         parser = argparse.ArgumentParser()
@@ -132,7 +158,9 @@ class ReplaceLatestVersionTest(testtools.TestCase):
         self.assertEqual(plugin.LATEST_VERSION,
                          namespace.os_baremetal_api_version)
         self.assertTrue(plugin.OS_BAREMETAL_API_VERSION_SPECIFIED)
+        self.assertTrue(plugin.OS_BAREMETAL_API_LATEST)
 
+    @mock.patch.object(plugin, 'OS_BAREMETAL_API_LATEST', new=False)
     @mock.patch.object(plugin.utils, 'env', lambda x: None)
     def test___call___specific_version(self):
         parser = argparse.ArgumentParser()
@@ -142,3 +170,4 @@ class ReplaceLatestVersionTest(testtools.TestCase):
                                 namespace)
         self.assertEqual('1.4', namespace.os_baremetal_api_version)
         self.assertTrue(plugin.OS_BAREMETAL_API_VERSION_SPECIFIED)
+        self.assertFalse(plugin.OS_BAREMETAL_API_LATEST)

ironicclient/tests/unit/v1/test_client.py

@@ -35,6 +35,20 @@ class ClientTest(utils.BaseTestCase):
             os_ironic_api_version=os_ironic_api_version,
             api_version_select_state='user')
 
+    def test_client_user_api_version_with_downgrade(self, http_client_mock):
+        endpoint = 'http://ironic:6385'
+        token = 'safe_token'
+        os_ironic_api_version = '1.15'
+
+        client.Client(endpoint, token=token,
+                      os_ironic_api_version=os_ironic_api_version,
+                      allow_api_version_downgrade=True)
+
+        http_client_mock.assert_called_once_with(
+            endpoint, token=token,
+            os_ironic_api_version=os_ironic_api_version,
+            api_version_select_state='default')
+
     @mock.patch.object(filecache, 'retrieve_data', autospec=True)
     def test_client_cache_api_version(self, cache_mock, http_client_mock):
         endpoint = 'http://ironic:6385'

ironicclient/v1/client.py

@@ -39,8 +39,14 @@ class Client(object):
 
     def __init__(self, endpoint=None, *args, **kwargs):
         """Initialize a new client for the Ironic v1 API."""
+        allow_downgrade = kwargs.pop('allow_api_version_downgrade', False)
         if kwargs.get('os_ironic_api_version'):
-            kwargs['api_version_select_state'] = "user"
+            if allow_downgrade:
+                # NOTE(dtantsur): here we allow the HTTP client to negotiate a
+                # lower version if the requested is too high
+                kwargs['api_version_select_state'] = "default"
+            else:
+                kwargs['api_version_select_state'] = "user"
         else:
             if not endpoint:
                 raise exc.EndpointException(

releasenotes/notes/latest-renegotiation-55daa01b3fc261be.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    When using ``--os-baremetal-api-version=latest``, the resulting API version
+    is now the maximum API version supported by both the client and the server.
+    Previously, the maximum API version supported by the client was used,
+    which prevented ``--os-baremetal-api-version=latest`` from working with
+    older servers.