Return HTTPBadRequest instead of HTTPNotFound

If you pass non-existing volume-type or service to the volume manage
API, then it returns 404 HTTPNotFound error.

According to the api guidelines [1] it should return 400
HTTPBadRequest.

[1] https://github.com/openstack/api-wg/blob/master/guidelines/http.rst#failure-code-clarifications

APIImpact: For volume manage API, it will return 400 error instead
of 404 if user passes non-existing volume-type or service in the
request.

Closes-Bug: #1632727
Change-Id: Iffa1824454c765d8cb732e93503420bafca17f08

cinder/api/contrib/volume_manage.py

@@ -20,6 +20,7 @@ from cinder.api import extensions
 from cinder.api.openstack import wsgi
 from cinder.api.v2.views import volumes as volume_views
 from cinder.api.views import manageable_volumes as list_manageable_view
+from cinder import exception
 from cinder.i18n import _
 from cinder import utils
 from cinder import volume as cinder_volume
@@ -118,9 +119,13 @@ class VolumeManageController(wsgi.Controller):
         kwargs = {}
         req_volume_type = volume.get('volume_type', None)
         if req_volume_type:
-            # Not found exception will be handled at the wsgi level
+            try:
-            kwargs['volume_type'] = (
+                kwargs['volume_type'] = volume_types.get_by_name_or_id(
-                volume_types.get_by_name_or_id(context, req_volume_type))
+                    context, req_volume_type)
+            except exception.VolumeTypeNotFound:
+                msg = _("Cannot find requested '%s' "
+                        "volume type") % req_volume_type
+                raise exception.InvalidVolumeType(reason=msg)
         else:
             kwargs['volume_type'] = {}
 
@@ -132,11 +137,14 @@ class VolumeManageController(wsgi.Controller):
 
         utils.check_metadata_properties(kwargs['metadata'])
 
-        # Not found exception will be handled at wsgi level
+        try:
-        new_volume = self.volume_api.manage_existing(context,
+            new_volume = self.volume_api.manage_existing(context,
-                                                     volume['host'],
+                                                         volume['host'],
-                                                     volume['ref'],
+                                                         volume['ref'],
-                                                     **kwargs)
+                                                         **kwargs)
+        except exception.ServiceNotFound:
+            msg = _("Host '%s' not found") % volume['host']
+            raise exception.ServiceUnavailable(message=msg)
 
         utils.add_visible_admin_metadata(new_volume)
 

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

@@ -224,6 +224,17 @@ class VolumeManageTest(test.TestCase):
         res = self._get_resp_post(body)
         self.assertEqual(400, res.status_int)
 
+    @mock.patch('cinder.objects.Service.get_by_args')
+    def test_manage_volume_service_not_found_on_host(self, mock_service):
+        """Test correct failure when host having no volume service on it."""
+        body = {'volume': {'host': 'host_ok',
+                           'ref': 'fake_ref'}}
+        mock_service.side_effect = exception.ServiceNotFound(
+            service_id='cinder-volume',
+            host='host_ok')
+        res = self._get_resp_post(body)
+        self.assertEqual(400, res.status_int)
+
     def test_manage_volume_missing_ref(self):
         """Test correct failure when the ref is not specified."""
         body = {'volume': {'host': 'host_ok'}}
@@ -297,7 +308,7 @@ class VolumeManageTest(test.TestCase):
                            'ref': 'fake_ref',
                            'volume_type': fake.WILL_NOT_BE_FOUND_ID}}
         res = self._get_resp_post(body)
-        self.assertEqual(404, res.status_int)
+        self.assertEqual(400, res.status_int)
 
     def test_manage_volume_bad_volume_type_by_name(self):
         """Test failure on nonexistent volume type specified by name."""
@@ -305,7 +316,7 @@ class VolumeManageTest(test.TestCase):
                            'ref': 'fake_ref',
                            'volume_type': 'bad_fakevt'}}
         res = self._get_resp_post(body)
-        self.assertEqual(404, res.status_int)
+        self.assertEqual(400, res.status_int)
 
     def _get_resp_get(self, host, detailed, paging, admin=True):
         """Helper to execute a GET os-volume-manage API call."""

cinder/tests/unit/api/v3/test_volume_manage.py

@@ -81,7 +81,7 @@ class VolumeManageTest(test.TestCase):
     def test_manage_volume_previous_version(self):
         body = {'volume': {'host': 'host_ok', 'ref': 'fake_ref'}}
         res = self._get_resp_post(body)
-        self.assertEqual(404, res.status_int, res)
+        self.assertEqual(400, res.status_int, res)
 
     def _get_resp_get(self, host, detailed, paging, version="3.8"):
         """Helper to execute a GET os-volume-manage API call."""