Fix: create image from volume command

Currently the command ``openstack image create --volume`` calls cinderclient
to upload the volume to image service (glance) but OSC passes ``visibility``
and ``protected`` fields which are only available in microversion 3.1 or
greater. This generates an error if the user is using volume microversion
< 3.1 and wants to create an image from volume.
This patch fixes that by only passing ``visibility`` and ``protected`` fields
when the volume microversion is 3.1 or greater and fail otherwise i.e. the
following 3 cases:
1) visibility/protected argument + mv >= 3.1 = pass
2) visibility/protected argument + mv < 3.1 = fail
3) not visibility/protected argument + any mv = pass

Story: 2010060
Task: 45511
Change-Id: I568a0ea0af8f7f82b16d49a6a1bb0391b99c50dc

3 files changed, 128 insertions, 3 deletions

diff --git a/openstackclient/image/v2/image.py b/openstackclient/image/v2/image.py
index 2f2f64ed..38c64db9 100644
--- a/openstackclient/image/v2/image.py
+++ b/openstackclient/image/v2/image.py
@@ -21,6 +21,7 @@ import logging
 import os
 import sys
 
+from cinderclient import api_versions
 from openstack.image import image_signer
 from osc_lib.api import utils as api_utils
 from osc_lib.cli import format_columns
@@ -483,14 +484,27 @@ class CreateImage(command.ShowOne):
                 volume_client.volumes,
                 parsed_args.volume,
             )
+            mv_kwargs = {}
+            if volume_client.api_version >= api_versions.APIVersion('3.1'):
+                mv_kwargs.update(
+                    visibility=kwargs.get('visibility', 'private'),
+                    protected=bool(parsed_args.protected)
+                )
+            else:
+                if kwargs.get('visibility') or parsed_args.protected:
+                    msg = _(
+                        '--os-volume-api-version 3.1 or greater is required '
+                        'to support the --public, --private, --community, '
+                        '--shared or --protected option.'
+                    )
+                    raise exceptions.CommandError(msg)
             response, body = volume_client.volumes.upload_to_image(
                 source_volume.id,
                 parsed_args.force,
                 parsed_args.name,
                 parsed_args.container_format,
                 parsed_args.disk_format,
-                visibility=kwargs.get('visibility', 'private'),
-                protected=True if parsed_args.protected else False
+                **mv_kwargs
             )
             info = body['os-volume_upload_image']
             try:
diff --git a/openstackclient/tests/unit/image/v2/test_image.py b/openstackclient/tests/unit/image/v2/test_image.py
index 444717a7..9241b0a4 100644
--- a/openstackclient/tests/unit/image/v2/test_image.py
+++ b/openstackclient/tests/unit/image/v2/test_image.py
@@ -18,6 +18,7 @@ import os
 import tempfile
 from unittest import mock
 
+from cinderclient import api_versions
 from openstack import exceptions as sdk_exceptions
 from osc_lib.cli import format_columns
 from osc_lib import exceptions
@@ -25,9 +26,10 @@ from osc_lib import exceptions
 from openstackclient.image.v2 import image
 from openstackclient.tests.unit.identity.v3 import fakes as identity_fakes
 from openstackclient.tests.unit.image.v2 import fakes as image_fakes
+from openstackclient.tests.unit.volume.v3 import fakes as volume_fakes
 
 
-class TestImage(image_fakes.TestImagev2):
+class TestImage(image_fakes.TestImagev2, volume_fakes.TestVolume):
 
     def setUp(self):
         super(TestImage, self).setUp()
@@ -40,6 +42,13 @@ class TestImage(image_fakes.TestImagev2):
         self.project_mock.reset_mock()
         self.domain_mock = self.app.client_manager.identity.domains
         self.domain_mock.reset_mock()
+        self.volumes_mock = self.app.client_manager.volume.volumes
+        fake_body = {
+            'os-volume_upload_image':
+                {'volume_type': {'name': 'fake_type'}}}
+        self.volumes_mock.upload_to_image.return_value = (
+            200, fake_body)
+        self.volumes_mock.reset_mock()
 
     def setup_images_mock(self, count):
         images = image_fakes.create_images(count=count)
@@ -287,6 +296,101 @@ class TestImageCreate(TestImage):
             use_import=True
         )
 
+    @mock.patch('osc_lib.utils.find_resource')
+    @mock.patch('openstackclient.image.v2.image.get_data_file')
+    def test_image_create_from_volume(self, mock_get_data_f, mock_get_vol):
+
+        fake_vol_id = 'fake-volume-id'
+        mock_get_data_f.return_value = (None, None)
+
+        class FakeVolume:
+            id = fake_vol_id
+
+        mock_get_vol.return_value = FakeVolume()
+
+        arglist = [
+            '--volume', fake_vol_id,
+            self.new_image.name,
+        ]
+        verifylist = [
+            ('name', self.new_image.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        self.volumes_mock.upload_to_image.assert_called_with(
+            fake_vol_id,
+            False,
+            self.new_image.name,
+            'bare',
+            'raw'
+        )
+
+    @mock.patch('osc_lib.utils.find_resource')
+    @mock.patch('openstackclient.image.v2.image.get_data_file')
+    def test_image_create_from_volume_fail(self, mock_get_data_f,
+                                           mock_get_vol):
+
+        fake_vol_id = 'fake-volume-id'
+        mock_get_data_f.return_value = (None, None)
+
+        class FakeVolume:
+            id = fake_vol_id
+
+        mock_get_vol.return_value = FakeVolume()
+
+        arglist = [
+            '--volume', fake_vol_id,
+            self.new_image.name,
+            '--public'
+        ]
+        verifylist = [
+            ('name', self.new_image.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        self.assertRaises(exceptions.CommandError, self.cmd.take_action,
+                          parsed_args)
+
+    @mock.patch('osc_lib.utils.find_resource')
+    @mock.patch('openstackclient.image.v2.image.get_data_file')
+    def test_image_create_from_volume_v31(self, mock_get_data_f,
+                                          mock_get_vol):
+
+        self.app.client_manager.volume.api_version = (
+            api_versions.APIVersion('3.1'))
+
+        fake_vol_id = 'fake-volume-id'
+        mock_get_data_f.return_value = (None, None)
+
+        class FakeVolume:
+            id = fake_vol_id
+
+        mock_get_vol.return_value = FakeVolume()
+
+        arglist = [
+            '--volume', fake_vol_id,
+            self.new_image.name,
+            '--public'
+        ]
+        verifylist = [
+            ('name', self.new_image.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        self.volumes_mock.upload_to_image.assert_called_with(
+            fake_vol_id,
+            False,
+            self.new_image.name,
+            'bare',
+            'raw',
+            visibility='public',
+            protected=False
+        )
+
 
 class TestAddProjectToImage(TestImage):
 
diff --git a/releasenotes/notes/fix-image-create-from-volume-c573e553161605c4.yaml b/releasenotes/notes/fix-image-create-from-volume-c573e553161605c4.yaml
new file mode 100644
index 00000000..92fc7419
--- /dev/null
+++ b/releasenotes/notes/fix-image-create-from-volume-c573e553161605c4.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fixed create image from volume command. If user wants to
+    pass ``visibility`` and ``protected`` fields, they need to
+    specify volume microversion 3.1 or greater by passing
+    ``os-volume-api-version 3.1`` with the command.