V3 jsonschema validation: volume actions

This patch adds jsonschema validation for below volume actions API's
* POST /v3/{project_id}/volumes/{volume_id}/action (attach)
* POST /v3/{project_id}/volumes/{volume_id}/action (detach)
* POST /v3/{project_id}/volumes/{volume_id}/action (volume_upload_image)
* POST /v3/{project_id}/volumes/{volume_id}/action (extend)
* POST /v3/{project_id}/volumes/{volume_id}/action (retype)
* POST /v3/{project_id}/volumes/{volume_id}/action (initialize_connection)
* POST /v3/{project_id}/volumes/{volume_id}/action (terminate_connection)
* POST /v3/{project_id}/volumes/{volume_id}/action (set_bootable_status)
* POST /v3/{project_id}/volumes/{volume_id}/action (update_readonly_flag)

Change-Id: I39ede009d5e909a076860df7305865286caa5352
Partial-Implements: bp json-schema-validation

cinder/api/contrib/volume_actions.py

@@ -16,7 +16,6 @@
 from castellan import key_manager
 from oslo_config import cfg
 import oslo_messaging as messaging
-from oslo_utils import encodeutils
 from oslo_utils import strutils
 import six
 from six.moves import http_client
@@ -25,11 +24,11 @@ import webob
 from cinder.api import extensions
 from cinder.api import microversions as mv
 from cinder.api.openstack import wsgi
+from cinder.api.schemas import volume_actions as volume_action
+from cinder.api import validation
 from cinder import exception
 from cinder.i18n import _
-from cinder.image import image_utils
 from cinder.policies import volume_actions as policy
-from cinder import utils
 from cinder import volume
 
 
@@ -52,6 +51,7 @@ class VolumeActionsController(wsgi.Controller):
 
     @wsgi.response(http_client.ACCEPTED)
     @wsgi.action('os-attach')
+    @validation.schema(volume_action.attach)
     def _attach(self, req, id, body):
         """Add attachment metadata."""
         context = req.environ['cinder.context']
@@ -66,23 +66,9 @@ class VolumeActionsController(wsgi.Controller):
         # Keep API backward compatibility
         if 'host_name' in body['os-attach']:
             host_name = body['os-attach']['host_name']
-        if 'mountpoint' not in body['os-attach']:
-            msg = _("Must specify 'mountpoint'")
-            raise webob.exc.HTTPBadRequest(explanation=msg)
         mountpoint = body['os-attach']['mountpoint']
-        if 'mode' in body['os-attach']:
+        mode = body['os-attach'].get('mode', 'rw')
-            mode = body['os-attach']['mode']
-        else:
-            mode = 'rw'
 
-        if instance_uuid is None and host_name is None:
-            msg = _("Invalid request to attach volume to an invalid target")
-            raise webob.exc.HTTPBadRequest(explanation=msg)
-
-        if mode not in ('rw', 'ro'):
-            msg = _("Invalid request to attach volume with an invalid mode. "
-                    "Attaching mode should be 'rw' or 'ro'")
-            raise webob.exc.HTTPBadRequest(explanation=msg)
         try:
             self.volume_api.attach(context, volume,
                                    instance_uuid, host_name, mountpoint, mode)
@@ -101,6 +87,7 @@ class VolumeActionsController(wsgi.Controller):
 
     @wsgi.response(http_client.ACCEPTED)
     @wsgi.action('os-detach')
+    @validation.schema(volume_action.detach)
     def _detach(self, req, id, body):
         """Clear attachment metadata."""
         context = req.environ['cinder.context']
@@ -108,7 +95,6 @@ class VolumeActionsController(wsgi.Controller):
         volume = self.volume_api.get(context, id)
 
         attachment_id = None
-        if body['os-detach']:
         attachment_id = body['os-detach'].get('attachment_id', None)
 
         try:
@@ -166,16 +152,13 @@ class VolumeActionsController(wsgi.Controller):
         self.volume_api.roll_detaching(context, volume)
 
     @wsgi.action('os-initialize_connection')
+    @validation.schema(volume_action.initialize_connection)
     def _initialize_connection(self, req, id, body):
         """Initialize volume attachment."""
         context = req.environ['cinder.context']
         # Not found exception will be handled at the wsgi level
         volume = self.volume_api.get(context, id)
-        try:
         connector = body['os-initialize_connection']['connector']
-        except KeyError:
-            raise webob.exc.HTTPBadRequest(
-                explanation=_("Must specify 'connector'"))
         try:
             info = self.volume_api.initialize_connection(context,
                                                          volume,
@@ -195,16 +178,13 @@ class VolumeActionsController(wsgi.Controller):
 
     @wsgi.response(http_client.ACCEPTED)
     @wsgi.action('os-terminate_connection')
+    @validation.schema(volume_action.terminate_connection)
     def _terminate_connection(self, req, id, body):
         """Terminate volume attachment."""
         context = req.environ['cinder.context']
         # Not found exception will be handled at the wsgi level
         volume = self.volume_api.get(context, id)
-        try:
         connector = body['os-terminate_connection']['connector']
-        except KeyError:
-            raise webob.exc.HTTPBadRequest(
-                explanation=_("Must specify 'connector'"))
         try:
             self.volume_api.terminate_connection(context, volume, connector)
         except exception.VolumeBackendAPIException:
@@ -213,37 +193,23 @@ class VolumeActionsController(wsgi.Controller):
 
     @wsgi.response(http_client.ACCEPTED)
     @wsgi.action('os-volume_upload_image')
+    @validation.schema(volume_action.volume_upload_image, '2.0', '3.0')
+    @validation.schema(volume_action.volume_upload_image_v31, '3.1')
     def _volume_upload_image(self, req, id, body):
         """Uploads the specified volume to image service."""
         context = req.environ['cinder.context']
         params = body['os-volume_upload_image']
         req_version = req.api_version_request
-        if not params.get("image_name"):
-            msg = _("No image_name was specified in request.")
-            raise webob.exc.HTTPBadRequest(explanation=msg)
 
         force = params.get('force', 'False')
-        try:
         force = strutils.bool_from_string(force, strict=True)
-        except ValueError as error:
-            err_msg = encodeutils.exception_to_unicode(error)
-            msg = _("Invalid value for 'force': '%s'") % err_msg
-            raise webob.exc.HTTPBadRequest(explanation=msg)
 
         # Not found exception will be handled at the wsgi level
         volume = self.volume_api.get(context, id)
 
-        context.authorize(policy.UPLOAD_IMAGE_POLICY, target_obj=volume)
+        context.authorize(policy.UPLOAD_IMAGE_POLICY)
         # check for valid disk-format
         disk_format = params.get("disk_format", "raw")
-        if not image_utils.validate_disk_format(disk_format):
-            msg = _("Invalid disk-format '%(disk_format)s' is specified. "
-                    "Allowed disk-formats are %(allowed_disk_formats)s.") % {
-                "disk_format": disk_format,
-                "allowed_disk_formats": ", ".join(
-                    image_utils.VALID_DISK_FORMATS)
-            }
-            raise webob.exc.HTTPBadRequest(explanation=msg)
 
         image_metadata = {"container_format": params.get(
             "container_format", "bare"),
@@ -267,14 +233,11 @@ class VolumeActionsController(wsgi.Controller):
                 mv.UPLOAD_IMAGE_PARAMS):
 
             image_metadata['visibility'] = params.get('visibility', 'private')
-            image_metadata['protected'] = params.get('protected', 'False')
+            image_metadata['protected'] = strutils.bool_from_string(
+                params.get('protected', 'False'), strict=True)
 
             if image_metadata['visibility'] == 'public':
-                context.authorize(policy.UPLOAD_PUBLIC_POLICY,
+                context.authorize(policy.UPLOAD_PUBLIC_POLICY)
-                                  target_obj=volume)
-
-            image_metadata['protected'] = (
-                utils.get_bool_param('protected', image_metadata))
 
         try:
             response = self.volume_api.copy_volume_to_image(context,
@@ -295,6 +258,7 @@ class VolumeActionsController(wsgi.Controller):
 
     @wsgi.response(http_client.ACCEPTED)
     @wsgi.action('os-extend')
+    @validation.schema(volume_action.extend)
     def _extend(self, req, id, body):
         """Extend size of volume."""
         context = req.environ['cinder.context']
@@ -302,12 +266,7 @@ class VolumeActionsController(wsgi.Controller):
         # Not found exception will be handled at the wsgi level
         volume = self.volume_api.get(context, id)
 
-        try:
         size = int(body['os-extend']['new_size'])
-        except (KeyError, ValueError, TypeError):
-            msg = _("New volume size must be specified as an integer.")
-            raise webob.exc.HTTPBadRequest(explanation=msg)
-
         try:
             if (req_version.matches(mv.VOLUME_EXTEND_INUSE) and
                     volume.status in ['in-use']):
@@ -319,64 +278,43 @@ class VolumeActionsController(wsgi.Controller):
 
     @wsgi.response(http_client.ACCEPTED)
     @wsgi.action('os-update_readonly_flag')
+    @validation.schema(volume_action.volume_readonly_update)
     def _volume_readonly_update(self, req, id, body):
         """Update volume readonly flag."""
         context = req.environ['cinder.context']
         # Not found exception will be handled at the wsgi level
         volume = self.volume_api.get(context, id)
 
-        try:
         readonly_flag = body['os-update_readonly_flag']['readonly']
-        except KeyError:
-            msg = _("Must specify readonly in request.")
-            raise webob.exc.HTTPBadRequest(explanation=msg)
 
-        try:
         readonly_flag = strutils.bool_from_string(readonly_flag,
                                                   strict=True)
-        except ValueError as error:
-            err_msg = encodeutils.exception_to_unicode(error)
-            msg = _("Invalid value for 'readonly': '%s'") % err_msg
-            raise webob.exc.HTTPBadRequest(explanation=msg)
 
         self.volume_api.update_readonly_flag(context, volume, readonly_flag)
 
     @wsgi.response(http_client.ACCEPTED)
     @wsgi.action('os-retype')
+    @validation.schema(volume_action.retype)
     def _retype(self, req, id, body):
         """Change type of existing volume."""
         context = req.environ['cinder.context']
         volume = self.volume_api.get(context, id)
-        try:
         new_type = body['os-retype']['new_type']
-        except KeyError:
-            msg = _("New volume type must be specified.")
-            raise webob.exc.HTTPBadRequest(explanation=msg)
         policy = body['os-retype'].get('migration_policy')
 
         self.volume_api.retype(context, volume, new_type, policy)
 
     @wsgi.response(http_client.OK)
     @wsgi.action('os-set_bootable')
+    @validation.schema(volume_action.set_bootable)
     def _set_bootable(self, req, id, body):
         """Update bootable status of a volume."""
         context = req.environ['cinder.context']
         # Not found exception will be handled at the wsgi level
         volume = self.volume_api.get(context, id)
 
-        try:
+        bootable = strutils.bool_from_string(
-            bootable = body['os-set_bootable']['bootable']
+            body['os-set_bootable']['bootable'], strict=True)
-        except KeyError:
-            msg = _("Must specify bootable in request.")
-            raise webob.exc.HTTPBadRequest(explanation=msg)
-
-        try:
-            bootable = strutils.bool_from_string(bootable,
-                                                 strict=True)
-        except ValueError as error:
-            err_msg = encodeutils.exception_to_unicode(error)
-            msg = _("Invalid value for 'bootable': '%s'") % err_msg
-            raise webob.exc.HTTPBadRequest(explanation=msg)
 
         update_dict = {'bootable': bootable}
 

cinder/api/schemas/volume_actions.py

@@ -0,0 +1,203 @@
+# Copyright (C) 2018 NTT DATA
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""
+Schema for V3 volume_actions API.
+
+"""
+
+import copy
+
+from cinder.api.validation import parameter_types
+
+
+container_format = parameter_types.description
+
+extend = {
+    'type': 'object',
+    'properties': {
+        'os-extend': {
+            'type': 'object',
+            'properties': {
+                'new_size': parameter_types.volume_size,
+            },
+            'required': ['new_size'],
+            'additionalProperties': False,
+        },
+    },
+    'required': ['os-extend'],
+    'additionalProperties': False,
+}
+
+
+attach = {
+    'type': 'object',
+    'properties': {
+        'os-attach': {
+            'type': 'object',
+            'properties': {
+                'instance_uuid': parameter_types.uuid,
+                'mountpoint': {
+                    'type': 'string', 'minLength': 1,
+                    'maxLength': 255
+                },
+                'host_name': {'type': 'string', 'maxLength': 255},
+                'mode': {'type': 'string', 'enum': ['rw', 'ro']}
+            },
+            'required': ['mountpoint'],
+            'anyOf': [{'required': ['instance_uuid']},
+                      {'required': ['host_name']}],
+            'additionalProperties': False,
+        },
+    },
+    'required': ['os-attach'],
+    'additionalProperties': False,
+}
+
+
+detach = {
+    'type': 'object',
+    'properties': {
+        'os-detach': {
+            'type': ['object', 'null'],
+            'properties': {
+                'attachment_id': parameter_types.uuid,
+            },
+            'additionalProperties': False,
+        },
+    },
+    'required': ['os-detach'],
+    'additionalProperties': False,
+}
+
+
+retype = {
+    'type': 'object',
+    'properties': {
+        'os-retype': {
+            'type': 'object',
+            'properties': {
+                'new_type': {'type': 'string'},
+                'migration_policy': {
+                    'type': ['string', 'null'],
+                    'enum': ['on-demand', 'never']},
+            },
+            'required': ['new_type'],
+            'additionalProperties': False,
+        },
+    },
+    'required': ['os-retype'],
+    'additionalProperties': False,
+}
+
+
+set_bootable = {
+    'type': 'object',
+    'properties': {
+        'os-set_bootable': {
+            'type': 'object',
+            'properties': {
+                'bootable': parameter_types.boolean
+            },
+            'required': ['bootable'],
+            'additionalProperties': False,
+        },
+    },
+    'required': ['os-set_bootable'],
+    'additionalProperties': False,
+}
+
+
+volume_upload_image = {
+    'type': 'object',
+    'properties': {
+        'os-volume_upload_image': {
+            'type': 'object',
+            'properties': {
+                'image_name': {
+                    'type': 'string', 'minLength': 1, 'maxLength': 255
+                },
+                'force': parameter_types.boolean,
+                'disk_format': {
+                    'type': 'string',
+                    'enum': ['raw', 'vmdk', 'vdi', 'qcow2',
+                             'vhd', 'vhdx', 'ploop']
+                },
+                'container_format': container_format
+            },
+            'required': ['image_name'],
+            'additionalProperties': False,
+        },
+    },
+    'required': ['os-volume_upload_image'],
+    'additionalProperties': False,
+}
+
+volume_upload_image_v31 = copy.deepcopy(volume_upload_image)
+volume_upload_image_v31['properties']['os-volume_upload_image']['properties'][
+    'visibility'] = {'type': 'string',
+                     'enum': ['community', 'public', 'private', 'shared']}
+volume_upload_image_v31['properties']['os-volume_upload_image']['properties'][
+    'protected'] = parameter_types.boolean
+
+
+initialize_connection = {
+    'type': 'object',
+    'properties': {
+        'os-initialize_connection': {
+            'type': 'object',
+            'properties': {
+                'connector': {'type': ['object', 'string']},
+            },
+            'required': ['connector'],
+            'additionalProperties': False,
+        },
+    },
+    'required': ['os-initialize_connection'],
+    'additionalProperties': False,
+}
+
+
+terminate_connection = {
+    'type': 'object',
+    'properties': {
+        'os-terminate_connection': {
+            'type': 'object',
+            'properties': {
+                'connector': {'type': ['string', 'object', 'null']},
+            },
+            'required': ['connector'],
+            'additionalProperties': False,
+        },
+    },
+    'required': ['os-terminate_connection'],
+    'additionalProperties': False,
+}
+
+volume_readonly_update = {
+    'type': 'object',
+    'properties': {
+        'os-update_readonly_flag': {
+            'type': 'object',
+            'properties': {
+                'readonly': parameter_types.boolean
+            },
+            'required': ['readonly'],
+            'additionalProperties': False,
+        },
+    },
+    'required': ['os-update_readonly_flag'],
+    'additionalProperties': False,
+}

cinder/api/validation/parameter_types.py

@@ -201,3 +201,10 @@ backup_service = {'type': 'string', 'minLength': 0, 'maxLength': 255}
 nullable_string = {
     'type': ('string', 'null'), 'minLength': 0, 'maxLength': 255
 }
+
+
+volume_size = {
+    'type': ['integer', 'string'],
+    'pattern': '^[0-9]+$',
+    'minimum': 1
+}

cinder/image/image_utils.py

@@ -60,8 +60,6 @@ QEMU_IMG_LIMITS = processutils.ProcessLimits(
     cpu_time=8,
     address_space=1 * units.Gi)
 
-VALID_DISK_FORMATS = ('raw', 'vmdk', 'vdi', 'qcow2',
-                      'vhd', 'vhdx', 'ploop')
 
 QEMU_IMG_FORMAT_MAP = {
     # Convert formats of Glance images to how they are processed with qemu-img.
@@ -76,10 +74,6 @@ QEMU_IMG_MIN_FORCE_SHARE_VERSION = [2, 10, 0]
 QEMU_IMG_MIN_CONVERT_LUKS_VERSION = '2.10'
 
 
-def validate_disk_format(disk_format):
-    return disk_format in VALID_DISK_FORMATS
-
-
 def fixup_disk_format(disk_format):
     """Return the format to be provided to qemu-img convert."""
 

cinder/tests/unit/api/contrib/test_volume_actions.py

@@ -20,11 +20,13 @@ import mock
 from oslo_config import cfg
 import oslo_messaging as messaging
 from oslo_serialization import jsonutils
+import six
 from six.moves import http_client
 import webob
 
 from cinder.api.contrib import volume_actions
 from cinder.api import microversions as mv
+from cinder.api.openstack import api_version_request as api_version
 from cinder import context
 from cinder import db
 from cinder import exception
@@ -55,7 +57,7 @@ class VolumeActionsTest(test.TestCase):
         super(VolumeActionsTest, self).setUp()
         self.context = context.RequestContext(fake.USER_ID, fake.PROJECT_ID,
                                               is_admin=False)
-        self.UUID = uuid.uuid4()
+        self.UUID = six.text_type(uuid.uuid4())
         self.controller = volume_actions.VolumeActionsController()
         self.api_patchers = {}
         for _meth in self._methods:
@@ -101,7 +103,8 @@ class VolumeActionsTest(test.TestCase):
         with mock.patch.object(volume_api.API,
                                'initialize_connection') as init_conn:
             init_conn.return_value = {}
-            body = {'os-initialize_connection': {'connector': 'fake'}}
+            body = {'os-initialize_connection': {'connector': {
+                'fake': 'fake'}}}
             req = webob.Request.blank('/v2/%s/volumes/%s/action' %
                                       (fake.PROJECT_ID, fake.VOLUME_ID))
             req.method = "POST"
@@ -147,7 +150,8 @@ class VolumeActionsTest(test.TestCase):
                                'initialize_connection') as init_conn:
             init_conn.side_effect = \
                 exception.VolumeBackendAPIException(data=None)
-            body = {'os-initialize_connection': {'connector': 'fake'}}
+            body = {'os-initialize_connection': {'connector': {
+                'fake': 'fake'}}}
             req = webob.Request.blank('/v2/%s/volumes/%s/action' %
                                       (fake.PROJECT_ID, fake.VOLUME_ID))
             req.method = "POST"
@@ -262,7 +266,7 @@ class VolumeActionsTest(test.TestCase):
                               self.controller._attach,
                               req,
                               id,
-                              body)
+                              body=body)
 
     def test_volume_attach_to_instance_raises_db_error(self):
         # In case of DB error 500 error code is returned to user
@@ -281,7 +285,7 @@ class VolumeActionsTest(test.TestCase):
                               self.controller._attach,
                               req,
                               id,
-                              body)
+                              body=body)
 
     def test_detach(self):
         body = {'os-detach': {'attachment_id': fake.ATTACHMENT_ID}}
@@ -309,7 +313,7 @@ class VolumeActionsTest(test.TestCase):
                               self.controller._detach,
                               req,
                               id,
-                              body)
+                              body=body)
 
     def test_volume_detach_raises_db_error(self):
         # In case of DB error 500 error code is returned to user
@@ -326,7 +330,7 @@ class VolumeActionsTest(test.TestCase):
                               self.controller._detach,
                               req,
                               id,
-                              body)
+                              body=body)
 
     def test_attach_with_invalid_arguments(self):
         # Invalid request to attach volume an invalid target
@@ -793,7 +797,6 @@ class VolumeImageActionsTest(test.TestCase):
         vol = {
             "container_format": 'bare',
             "disk_format": 'raw',
-            "updated_at": datetime.datetime(1, 1, 1, 1, 1, 1),
             "image_name": 'image_name',
             "force": True}
         body = {"os-volume_upload_image": vol}
@@ -853,7 +856,7 @@ class VolumeImageActionsTest(test.TestCase):
         body = {"os-volume_upload_image": img}
         req = fakes.HTTPRequest.blank('/v2/%s/volumes/%s/action' %
                                       (fake.PROJECT_ID, id))
-        res_dict = self.controller._volume_upload_image(req, id, body)
+        res_dict = self.controller._volume_upload_image(req, id, body=body)
         expected = {'os-volume_upload_image':
                     {'id': id,
                      'updated_at': datetime.datetime(1, 1, 1, 1, 1, 1),
@@ -887,7 +890,7 @@ class VolumeImageActionsTest(test.TestCase):
                           self.controller._volume_upload_image,
                           req,
                           id,
-                          body)
+                          body=body)
 
     @mock.patch.object(volume_api.API, 'get', fake_volume_get_obj)
     @mock.patch.object(volume_api.API, 'copy_volume_to_image',
@@ -905,7 +908,7 @@ class VolumeImageActionsTest(test.TestCase):
                           self.controller._volume_upload_image,
                           req,
                           id,
-                          body)
+                          body=body)
 
     @mock.patch.object(volume_api.API, 'get', fake_volume_get)
     def test_copy_volume_to_image_invalid_disk_format(self):
@@ -917,11 +920,11 @@ class VolumeImageActionsTest(test.TestCase):
         body = {"os-volume_upload_image": vol}
         req = fakes.HTTPRequest.blank('/v2/%s/volumes/%s/action'
                                       % (fake.PROJECT_ID, id))
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.ValidationError,
                           self.controller._volume_upload_image,
                           req,
                           id,
-                          body)
+                          body=body)
 
     @mock.patch.object(volume_api.API, "copy_volume_to_image")
     def test_copy_volume_to_image_disk_format_ploop(self,
@@ -938,7 +941,7 @@ class VolumeImageActionsTest(test.TestCase):
         image_metadata = {'container_format': 'bare',
                           'disk_format': 'ploop',
                           'name': 'image_name'}
-        self.controller._volume_upload_image(req, volume.id, body)
+        self.controller._volume_upload_image(req, volume.id, body=body)
 
         mock_copy_to_image.assert_called_once_with(
             req.environ['cinder.context'], volume, image_metadata, False)
@@ -959,7 +962,7 @@ class VolumeImageActionsTest(test.TestCase):
                           self.controller._volume_upload_image,
                           req,
                           id,
-                          body)
+                          body=body)
 
     @mock.patch.object(volume_api.API, 'get', fake_volume_get_obj)
     @mock.patch.object(volume_api.API, 'copy_volume_to_image',
@@ -977,7 +980,35 @@ class VolumeImageActionsTest(test.TestCase):
                           self.controller._volume_upload_image,
                           req,
                           id,
-                          body)
+                          body=body)
+
+    @mock.patch.object(volume_api.API, 'get', fake_volume_get_obj)
+    @mock.patch.object(volume_api.API, 'copy_volume_to_image',
+                       side_effect=messaging.RemoteError)
+    @ddt.data(
+        ({"image_name": 'image_name', "protected": None},
+         exception.ValidationError),
+        ({"image_name": 'image_name', "protected": ' '},
+         exception.ValidationError),
+        ({"image_name": 'image_name', "protected": 'test'},
+         exception.ValidationError),
+        ({"image_name": 'image_name', "visibility": 'test'},
+         exception.ValidationError),
+        ({"image_name": 'image_name', "visibility": ' '},
+         exception.ValidationError),
+        ({"image_name": 'image_name', "visibility": None},
+         exception.ValidationError))
+    @ddt.unpack
+    def test_copy_volume_to_image_invalid_request_body(
+            self, vol, exception, mock_copy):
+        id = fake.VOLUME2_ID
+        body = {"os-volume_upload_image": vol}
+        req = fakes.HTTPRequest.blank('/v3/%s/volumes/%s/action' %
+                                      (fake.PROJECT_ID, id))
+        req.api_version_request = api_version.APIVersionRequest("3.1")
+        self.assertRaises(exception,
+                          self.controller._volume_upload_image,
+                          req, id, body=body)
 
     def test_volume_upload_image_typeerror(self):
         id = fake.VOLUME2_ID
@@ -1011,11 +1042,11 @@ class VolumeImageActionsTest(test.TestCase):
         body = {'os-extend': {'new_size': 'fake'}}
         req = fakes.HTTPRequest.blank('/v2/%s/volumes/%s/action' %
                                       (fake.PROJECT_ID, id))
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.ValidationError,
                           self.controller._extend,
                           req,
                           id,
-                          body)
+                          body=body)
 
     @ddt.data({'version': mv.get_prior_version(mv.VOLUME_EXTEND_INUSE),
                'status': 'available'},
@@ -1036,7 +1067,7 @@ class VolumeImageActionsTest(test.TestCase):
         req = fakes.HTTPRequest.blank('/v3/%s/volumes/%s/action' %
                                       (fake.PROJECT_ID, vol['id']))
         req.api_version_request = mv.get_api_version(version)
-        self.controller._extend(req, vol['id'], body)
+        self.controller._extend(req, vol['id'], body=body)
         if version == mv.VOLUME_EXTEND_INUSE and status == 'in-use':
             mock_extend.assert_called_with(req.environ['cinder.context'],
                                            vol, 2, attached=True)
@@ -1053,11 +1084,11 @@ class VolumeImageActionsTest(test.TestCase):
         body = {"os-volume_upload_image": vol}
         req = fakes.HTTPRequest.blank('/v2/%s/volumes/%s/action' %
                                       (fake.PROJECT_ID, id))
-        self.assertRaises(webob.exc.HTTPBadRequest,
+        self.assertRaises(exception.ValidationError,
                           self.controller._volume_upload_image,
                           req,
                           id,
-                          body)
+                          body=body)
 
     def _create_volume_with_type(self, status='available',
                                  display_description='displaydesc', **kwargs):
@@ -1104,7 +1135,8 @@ class VolumeImageActionsTest(test.TestCase):
             use_admin_context=self.context.is_admin)
         body = self._get_os_volume_upload_image()
 
-        res_dict = self.controller._volume_upload_image(req, volume.id, body)
+        res_dict = self.controller._volume_upload_image(req, volume.id,
+                                                        body=body)
 
         self.assertDictEqual(expected, res_dict)
         vol_db = objects.Volume.get_by_id(self.context, volume.id)
@@ -1123,7 +1155,7 @@ class VolumeImageActionsTest(test.TestCase):
         body['os-volume_upload_image']['visibility'] = 'public'
         self.assertRaises(exception.PolicyNotAuthorized,
                           self.controller._volume_upload_image,
-                          req, id, body)
+                          req, id, body=body)
 
     @mock.patch.object(volume_api.API, "get_volume_image_metadata")
     @mock.patch.object(glance.GlanceImageService, "create")
@@ -1145,7 +1177,8 @@ class VolumeImageActionsTest(test.TestCase):
             '/v2/%s/volumes/%s/action' % (fake.PROJECT_ID, volume.id),
             use_admin_context=self.context.is_admin)
         body = self._get_os_volume_upload_image()
-        res_dict = self.controller._volume_upload_image(req, volume.id, body)
+        res_dict = self.controller._volume_upload_image(req, volume.id,
+                                                        body=body)
 
         self.assertDictEqual(expected, res_dict)
         vol_db = objects.Volume.get_by_id(self.context, volume.id)
@@ -1171,7 +1204,7 @@ class VolumeImageActionsTest(test.TestCase):
         body = self._get_os_volume_upload_image()
         self.assertRaises(webob.exc.HTTPBadRequest,
                           self.controller._volume_upload_image, req, volume.id,
-                          body)
+                          body=body)
 
         self.assertFalse(mock_copy_to_image.called)
         vol_db = objects.Volume.get_by_id(self.context, volume.id)
@@ -1199,7 +1232,7 @@ class VolumeImageActionsTest(test.TestCase):
         body['os-volume_upload_image']['force'] = False
         self.assertRaises(webob.exc.HTTPBadRequest,
                           self.controller._volume_upload_image, req, volume.id,
-                          body)
+                          body=body)
 
         self.assertFalse(mock_copy_to_image.called)
         vol_db = objects.Volume.get_by_id(self.context, volume.id)
@@ -1227,7 +1260,7 @@ class VolumeImageActionsTest(test.TestCase):
         body = self._get_os_volume_upload_image()
         self.assertRaises(webob.exc.HTTPBadRequest,
                           self.controller._volume_upload_image, req, volume.id,
-                          body)
+                          body=body)
 
         self.assertFalse(mock_copy_to_image.called)
         vol_db = objects.Volume.get_by_id(self.context, volume.id)
@@ -1235,7 +1268,8 @@ class VolumeImageActionsTest(test.TestCase):
         self.assertIsNone(vol_db.previous_status)
 
         CONF.set_default('enable_force_upload', True)
-        res_dict = self.controller._volume_upload_image(req, volume.id, body)
+        res_dict = self.controller._volume_upload_image(req, volume.id,
+                                                        body=body)
 
         self.assertDictEqual(expected, res_dict)
 
@@ -1259,7 +1293,8 @@ class VolumeImageActionsTest(test.TestCase):
             use_admin_context=self.context.is_admin)
 
         body = self._get_os_volume_upload_image()
-        res_dict = self.controller._volume_upload_image(req, volume.id, body)
+        res_dict = self.controller._volume_upload_image(req, volume.id,
+                                                        body=body)
 
         self.assertDictEqual(expected, res_dict)
         vol_db = objects.Volume.get_by_id(self.context, volume.id)
@@ -1281,7 +1316,8 @@ class VolumeImageActionsTest(test.TestCase):
             use_admin_context=self.context.is_admin)
 
         body = self._get_os_volume_upload_image()
-        res_dict = self.controller._volume_upload_image(req, volume.id, body)
+        res_dict = self.controller._volume_upload_image(req, volume.id,
+                                                        body=body)
 
         self.assertDictEqual(expected, res_dict)
         vol_db = objects.Volume.get_by_id(self.context, volume.id)
@@ -1305,7 +1341,8 @@ class VolumeImageActionsTest(test.TestCase):
             '/v2/%s/volumes/%s/action' % (fake.PROJECT_ID, volume.id),
             use_admin_context=self.context.is_admin)
         body = self._get_os_volume_upload_image()
-        res_dict = self.controller._volume_upload_image(req, volume.id, body)
+        res_dict = self.controller._volume_upload_image(req, volume.id,
+                                                        body=body)
         self.assertDictEqual(expected, res_dict)
 
     @mock.patch.object(volume_api.API, "get_volume_image_metadata")
@@ -1334,11 +1371,12 @@ class VolumeImageActionsTest(test.TestCase):
         req.headers = mv.get_mv_header(mv.UPLOAD_IMAGE_PARAMS)
         req.api_version_request = mv.get_api_version(mv.UPLOAD_IMAGE_PARAMS)
         body = self._get_os_volume_upload_image()
+        body = self._get_os_volume_upload_image()
         body['os-volume_upload_image']['visibility'] = 'public'
         body['os-volume_upload_image']['protected'] = True
         res_dict = self.controller._volume_upload_image(req,
                                                         volume.id,
-                                                        body)
+                                                        body=body)
 
         expected['os-volume_upload_image'].update(visibility='public',
                                                   protected=True)
@@ -1360,7 +1398,8 @@ class VolumeImageActionsTest(test.TestCase):
         body['os-volume_upload_image']['container_format'] = 'bare'
         body['os-volume_upload_image']['disk_format'] = 'vhd'
 
-        res_dict = self.controller._volume_upload_image(req, volume.id, body)
+        res_dict = self.controller._volume_upload_image(req, volume.id,
+                                                        body=body)
 
         self.assertDictEqual(expected, res_dict)
         vol_db = objects.Volume.get_by_id(self.context, volume.id)
@@ -1383,7 +1422,8 @@ class VolumeImageActionsTest(test.TestCase):
         body['os-volume_upload_image']['container_format'] = 'bare'
         body['os-volume_upload_image']['disk_format'] = 'vhdx'
 
-        res_dict = self.controller._volume_upload_image(req, volume.id, body)
+        res_dict = self.controller._volume_upload_image(req, volume.id,
+                                                        body=body)
 
         self.assertDictEqual(expected, res_dict)
         vol_db = objects.Volume.get_by_id(self.context, volume.id)

cinder/volume/api.py

@@ -1614,11 +1614,6 @@ class API(base.Base):
     def retype(self, context, volume, new_type, migration_policy=None):
         """Attempt to modify the type associated with an existing volume."""
         context.authorize(vol_action_policy.RETYPE_POLICY, target_obj=volume)
-        if migration_policy and migration_policy not in ('on-demand', 'never'):
-            msg = _('migration_policy must be \'on-demand\' or \'never\', '
-                    'passed: %s') % new_type
-            LOG.error(msg)
-            raise exception.InvalidInput(reason=msg)
 
         # Support specifying volume type by ID or name
         try: