From 16082ddfd632d12f604cda628e4515dba0d2c448 Mon Sep 17 00:00:00 2001
From: Dinesh Bhor <dinesh.bhor@nttdata.com>
Date: Wed, 8 Feb 2017 20:43:09 +0530
Subject: [PATCH] Fix action on services raises TypeError

Freeze, thaw and failover_host actions on a service raises TypeError
when non-existent host or cluster name is passed to the service
update API. This TypeError exception raised is converted into
HTTPBadRequest at wsgi layer.

Fixed this issue by formatting error message when a service is not
found for a given host or cluster. Caught the ServiceNotFound at API
layer and raised InvalidInput error so a user will see 400 error
only instead of 404.

Closes-Bug: #1663205
Change-Id: I54b9575f293f64a5f31246d0345c753b8011d95f
---
 cinder/api/contrib/services.py                | 16 ++++++---
 .../tests/unit/api/contrib/test_services.py   | 34 +++++++++++++++++++
 cinder/volume/api.py                          | 10 +++---
 3 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/cinder/api/contrib/services.py b/cinder/api/contrib/services.py
index 23cae982194..6b959f20ea2 100644
--- a/cinder/api/contrib/services.py
+++ b/cinder/api/contrib/services.py
@@ -112,13 +112,21 @@ class ServiceController(wsgi.Controller):
 
         return True
 
+    def _volume_api_proxy(self, fun, *args):
+        try:
+            return fun(*args)
+        except exception.ServiceNotFound as ex:
+            raise exception.InvalidInput(ex.msg)
+
     def _freeze(self, context, req, body):
         cluster_name, host = common.get_cluster_host(req, body, '3.26')
-        return self.volume_api.freeze_host(context, host, cluster_name)
+        return self._volume_api_proxy(self.volume_api.freeze_host, context,
+                                      host, cluster_name)
 
     def _thaw(self, context, req, body):
         cluster_name, host = common.get_cluster_host(req, body, '3.26')
-        return self.volume_api.thaw_host(context, host, cluster_name)
+        return self._volume_api_proxy(self.volume_api.thaw_host, context,
+                                      host, cluster_name)
 
     def _failover(self, context, req, body, clustered):
         # We set version to None to always get the cluster name from the body,
@@ -126,8 +134,8 @@ class ServiceController(wsgi.Controller):
         # it if the requested version is 3.26 or higher.
         version = '3.26' if clustered else False
         cluster_name, host = common.get_cluster_host(req, body, version)
-        self.volume_api.failover(context, host, cluster_name,
-                                 body.get('backend_id'))
+        self._volume_api_proxy(self.volume_api.failover, context, host,
+                               cluster_name, body.get('backend_id'))
         return webob.Response(status_int=http_client.ACCEPTED)
 
     def update(self, req, id, body):
diff --git a/cinder/tests/unit/api/contrib/test_services.py b/cinder/tests/unit/api/contrib/test_services.py
index d5187c277f4..97ffecf2d55 100644
--- a/cinder/tests/unit/api/contrib/test_services.py
+++ b/cinder/tests/unit/api/contrib/test_services.py
@@ -795,6 +795,40 @@ class ServicesTest(test.TestCase):
                                               mock.sentinel.backend_id)
         self.assertEqual(202, res.status_code)
 
+    @ddt.data(('failover_host', {'host': mock.sentinel.host,
+                                 'backend_id': mock.sentinel.backend_id}),
+              ('freeze', {'host': mock.sentinel.host}),
+              ('thaw', {'host': mock.sentinel.host}))
+    @ddt.unpack
+    @mock.patch('cinder.objects.ServiceList.get_all')
+    def test_services_action_host_not_found(self, method, body,
+                                            mock_get_all_services):
+        url = '/v2/%s/os-services/%s' % (fake.PROJECT_ID, method)
+        req = fakes.HTTPRequest.blank(url)
+        mock_get_all_services.return_value = []
+        msg = 'No service found with host=%s' % mock.sentinel.host
+        result = self.assertRaises(exception.InvalidInput,
+                                   self.controller.update,
+                                   req, method, body)
+        self.assertEqual(msg, result.msg)
+
+    @ddt.data(('failover', {'cluster': mock.sentinel.cluster,
+                            'backend_id': mock.sentinel.backend_id}),
+              ('freeze', {'cluster': mock.sentinel.cluster}),
+              ('thaw', {'cluster': mock.sentinel.cluster}))
+    @ddt.unpack
+    @mock.patch('cinder.objects.ServiceList.get_all')
+    def test_services_action_cluster_not_found(self, method, body,
+                                               mock_get_all_services):
+        url = '/v3/%s/os-services/%s' % (fake.PROJECT_ID, method)
+        req = fakes.HTTPRequest.blank(url, version='3.26')
+        mock_get_all_services.return_value = []
+        msg = 'No service found with cluster=%s' % mock.sentinel.cluster
+        result = self.assertRaises(exception.InvalidInput,
+                                   self.controller.update, req,
+                                   method, body)
+        self.assertEqual(msg, result.msg)
+
     def test_services_freeze(self):
         url = '/v2/%s/os-services/freeze' % fake.PROJECT_ID
         req = fakes.HTTPRequest.blank(url)
diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index f523d2e5814..60ccbd12761 100644
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -1718,10 +1718,12 @@ class API(base.Base):
                            'binary': constants.VOLUME_BINARY})
 
         if not services:
-            msg = _('No service found with ') + (
-                'host=%(host)s' if host else 'cluster=%(cluster_name)s')
-            raise exception.ServiceNotFound(msg, host=host,
-                                            cluster_name=cluster_name)
+            if host:
+                msg = _("No service found with host=%s") % host
+            else:
+                msg = _("No service found with cluster=%s") % cluster_name
+
+            raise exception.ServiceNotFound(msg)
 
         cluster = services[0].cluster
         # Check that the host or cluster we received only results in 1 host or