Handle downgraded client for snapshot-create

When a CLI user specifies --os-volume api-version 3.66, the shell
will execute the appropriate shell code, but if the server only
supports < 3.66, the client is automatically downgraded and correctly
uses the pre-3.66 SnapshotManager.create() method.

In that case, the 'force' parameter, which is technically not allowed
in mv 3.66 (but which silently accepts a True value for backward
compatibility), will have a value of None, which the pre-3.66 code
happily passes to cinder as '"force": null' in the request body, and
which then fails the Block Storage API request-schema check.

Handle this situation by detecting a None 'force' value and setting
it to its pre-3.66 default value of False.

Change-Id: I3ad8283c2a9aaac58c8d2b50fa7ac86b617e5dd3
Closes-bug: #1995883

cinderclient/tests/unit/v3/test_shell.py

@@ -99,6 +99,14 @@ class ShellTest(utils.TestCase):
                               api_versions.APIVersion('3.99'))):
             self.shell.main(cmd.split())
 
+    def run_command_with_server_api_max(self, api_max, cmd):
+        # version negotiation will use the supplied api_max, which must be
+        # a string value, as the server's max supported version
+        with mock.patch('cinderclient.api_versions._get_server_version_range',
+                return_value=(api_versions.APIVersion('3.0'),
+                              api_versions.APIVersion(api_max))):
+            self.shell.main(cmd.split())
+
     def assert_called(self, method, url, body=None,
                       partial_body=None, **kwargs):
         return self.shell.cs.assert_called(method, url, body,
@@ -918,6 +926,41 @@ class ShellTest(utils.TestCase):
                          f'snapshot-create --force {force_value} 123456')
         self.assert_called_anytime('POST', '/snapshots', body=snap_body_3_65)
 
+    @mock.patch('cinderclient.shell.CinderClientArgumentParser.exit')
+    def test_snapshot_create_pre_3_66_with_naked_force(
+            self, mock_exit):
+        mock_exit.side_effect = Exception("mock exit")
+        try:
+            self.run_command('--os-volume-api-version 3.65 '
+                             'snapshot-create --force 123456')
+        except Exception as e:
+            # ignore the exception (it's raised to simulate an exit),
+            # but make sure it's the exception we expect
+            self.assertEqual('mock exit', str(e))
+
+        exit_code = mock_exit.call_args.args[0]
+        self.assertEqual(2, exit_code)
+
+    @mock.patch('cinderclient.utils.find_resource')
+    def test_snapshot_create_pre_3_66_with_force_None(
+            self, mock_find_vol):
+        """We will let the API detect the problematic value."""
+        mock_find_vol.return_value = volumes.Volume(
+            self, {'id': '123456'}, loaded=True)
+        snap_body_3_65 = {
+            'snapshot': {
+                'volume_id': '123456',
+                # note: this is a string, NOT None!
+                'force': 'None',
+                'name': None,
+                'description': None,
+                'metadata': {}
+            }
+        }
+        self.run_command('--os-volume-api-version 3.65 '
+                         'snapshot-create --force None 123456')
+        self.assert_called_anytime('POST', '/snapshots', body=snap_body_3_65)
+
     SNAP_BODY_3_66 = {
         'snapshot': {
             'volume_id': '123456',
@@ -952,6 +995,17 @@ class ShellTest(utils.TestCase):
                                 f'snapshot-create --force {f_val} 123456')
         self.assertIn('not allowed after microversion 3.65', str(uae))
 
+    @mock.patch('cinderclient.utils.find_resource')
+    def test_snapshot_create_3_66_with_force_None(
+            self, mock_find_vol):
+        mock_find_vol.return_value = volumes.Volume(
+            self, {'id': '123456'}, loaded=True)
+        uae = self.assertRaises(exceptions.UnsupportedAttribute,
+                                self.run_command,
+                                '--os-volume-api-version 3.66 '
+                                'snapshot-create --force None 123456')
+        self.assertIn('not allowed after microversion 3.65', str(uae))
+
     @mock.patch('cinderclient.utils.find_resource')
     def test_snapshot_create_3_66(self, mock_find_vol):
         mock_find_vol.return_value = volumes.Volume(
@@ -961,6 +1015,28 @@ class ShellTest(utils.TestCase):
         self.assert_called_anytime('POST', '/snapshots',
                                    body=self.SNAP_BODY_3_66)
 
+    @mock.patch('cinderclient.utils.find_resource')
+    def test_snapshot_create_3_66_not_supported(self, mock_find_vol):
+        mock_find_vol.return_value = volumes.Volume(
+            self, {'id': '123456'}, loaded=True)
+        self.run_command_with_server_api_max(
+            '3.64',
+            '--os-volume-api-version 3.66 snapshot-create 123456')
+        # call should be made, but will use the pre-3.66 request body
+        # because the client in use has been downgraded to 3.64
+        pre_3_66_request_body = {
+            'snapshot': {
+                'volume_id': '123456',
+                # default value is False
+                'force': False,
+                'name': None,
+                'description': None,
+                'metadata': {}
+            }
+        }
+        self.assert_called_anytime('POST', '/snapshots',
+                                   body=pre_3_66_request_body)
+
     def test_snapshot_manageable_list(self):
         self.run_command('--os-volume-api-version 3.8 '
                          'snapshot-manageable-list fakehost')

cinderclient/v3/shell.py

@@ -2213,6 +2213,7 @@ def do_snapshot_list(cs, args):
            'than forcing it to be available. From microversion 3.66, '
            'all snapshots are "forced" and this option is invalid. '
            'Default=False.')
+# FIXME: is this second declaration of --force really necessary?
 @utils.arg('--force',
            metavar='<True>',
            nargs='?',
@@ -2253,6 +2254,7 @@ def do_snapshot_create(cs, args):
         snapshot_metadata = shell_utils.extract_metadata(args)
 
     volume = utils.find_volume(cs, args.volume)
+
     snapshot = cs.volume_snapshots.create(volume.id,
                                           args.force,
                                           args.name,

cinderclient/v3/volume_snapshots.py

@@ -108,6 +108,20 @@ class SnapshotManager(base.ManagerWithFind):
         else:
             snapshot_metadata = metadata
 
+        # Bug #1995883: it's possible for the shell to use the user-
+        # specified 3.66 do_snapshot_create function, but if the server
+        # only supports < 3.66, the client will have been downgraded and
+        # will use this function.  In that case, the 'force' parameter will
+        # be None, which means that the user didn't specify a value for it,
+        # so we set it to the pre-3.66 default value of False.
+        #
+        # NOTE: we know this isn't a problem for current client consumers
+        # because a null value for 'force' has never been allowed by the
+        # Block Storage API v3, so there's no reason for anyone to directly
+        # call this method passing force=None.
+        if force is None:
+            force = False
+
         body = {'snapshot': {'volume_id': volume_id,
                              'force': force,
                              'name': name,

releasenotes/notes/bug-1995883-force-flag-none-3a7bb87f655bcf42.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    `Bug #1995883
+    <https://bugs.launchpad.net/python-cinderclient/+bug/1995883>`_:
+    Fixed bad format request body generated for the snapshot-create
+    action when the client supports mv 3.66 or greater but the Block
+    Storage API being contacted supports < 3.66.