diff options
author | Brian Rosmaita <rosmaita.fossdev@gmail.com> | 2022-11-08 11:58:38 -0500 |
---|---|---|
committer | Brian Rosmaita <rosmaita.fossdev@gmail.com> | 2022-11-08 18:48:18 -0500 |
commit | 9df653571d4da06c25222189be27e87a6da75628 (patch) | |
tree | 86a222edebb9fc3a763a7ef1fd71507fef4d1715 | |
parent | 6f67187b8255ae231f82a9deaaf9156c868153a0 (diff) | |
download | python-cinderclient-9df653571d4da06c25222189be27e87a6da75628.tar.gz |
Handle downgraded client for snapshot-create9.2.0
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
-rw-r--r-- | cinderclient/tests/unit/v3/test_shell.py | 76 | ||||
-rw-r--r-- | cinderclient/v3/shell.py | 2 | ||||
-rw-r--r-- | cinderclient/v3/volume_snapshots.py | 14 | ||||
-rw-r--r-- | releasenotes/notes/bug-1995883-force-flag-none-3a7bb87f655bcf42.yaml | 8 |
4 files changed, 100 insertions, 0 deletions
diff --git a/cinderclient/tests/unit/v3/test_shell.py b/cinderclient/tests/unit/v3/test_shell.py index 9eb6ce3..7c5f110 100644 --- a/cinderclient/tests/unit/v3/test_shell.py +++ b/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', @@ -953,6 +996,17 @@ class ShellTest(utils.TestCase): 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( self, {'id': '123456'}, loaded=True) @@ -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') diff --git a/cinderclient/v3/shell.py b/cinderclient/v3/shell.py index 60239c7..2ea1848 100644 --- a/cinderclient/v3/shell.py +++ b/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, diff --git a/cinderclient/v3/volume_snapshots.py b/cinderclient/v3/volume_snapshots.py index 9a94422..cb1c3ba 100644 --- a/cinderclient/v3/volume_snapshots.py +++ b/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, diff --git a/releasenotes/notes/bug-1995883-force-flag-none-3a7bb87f655bcf42.yaml b/releasenotes/notes/bug-1995883-force-flag-none-3a7bb87f655bcf42.yaml new file mode 100644 index 0000000..87964d1 --- /dev/null +++ b/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. |