Return error message if validate fail when clearing hook

There are some situations:
1. User can run the command "heat hook-clear" on any
resource in the stack, even if there is no hook
defined on it, and no error or warning message is returned.
2. User run the command "heat hook-clear" on resource
to clear the hook which resource unset, and no error or
warning message is returned.
3. User run the command "heat hook-clear" on resource
even if the resource's action is not support to signal, and
no error or warning message is returned.
4. User run the command "heat hook-clear" to clear invalid
hooks, and no error or warnning message is returned.

This patch will check the situations above before call
resource.signal, and will return error messages to user.

Change-Id: Ifb9befad864ebe1bb5f8b419b95d1b3a95530573
Closes-Bug: #1472515

heat/api/aws/exception.py

@@ -302,6 +302,8 @@ def map_remote_error(ex):
         'MissingCredentialError',
         'ResourcePropertyConflict',
         'PropertyUnspecifiedError',
+        'NotSupported',
+        'InvalidBreakPointHook',
     )
     denied_errors = ('Forbidden', 'NotAuthorized')
     already_exists_errors = ('StackExists')

heat/api/middleware/fault.py

@@ -92,6 +92,7 @@ class FaultWrapper(wsgi.Middleware):
         'OrphanedObjectError': webob.exc.HTTPBadRequest,
         'UnsupportedObjectError': webob.exc.HTTPBadRequest,
         'ResourceTypeUnavailable': webob.exc.HTTPBadRequest,
+        'InvalidBreakPointHook': webob.exc.HTTPBadRequest,
     }
 
     def _map_exception_to_error(self, class_exception):

heat/engine/resource.py

@@ -1478,17 +1478,40 @@ class Resource(object):
         '''
         return base64.b64encode(data)
 
-    def signal(self, details=None):
+    def _signal_check_action(self):
-        '''
-        signal the resource. Subclasses should provide a handle_signal() method
-        to implement the signal, the base-class raise an exception if no
-        handler is implemented.
-        '''
         if self.action in self.no_signal_actions:
             self._add_event(self.action, self.status,
                             'Cannot signal resource during %s' % self.action)
-            ex = Exception(_('Cannot signal resource during %s') % self.action)
+            msg = _('Signal resource during %s') % self.action
-            raise exception.ResourceFailure(ex, self)
+            raise exception.NotSupported(feature=msg)
+
+    def _signal_check_hook(self, details):
+        if details and 'unset_hook' in details:
+            hook = details['unset_hook']
+            if not environment.valid_hook_type(hook):
+                msg = (_('Invalid hook type "%(hook)s" for %(resource)s') %
+                       {'hook': hook, 'resource': six.text_type(self)})
+                raise exception.InvalidBreakPointHook(message=msg)
+
+            if not self.has_hook(hook):
+                msg = (_('The "%(hook)s" hook is not defined '
+                         'on %(resource)s') %
+                       {'hook': hook, 'resource': six.text_type(self)})
+                raise exception.InvalidBreakPointHook(message=msg)
+
+    def _unset_hook(self, details):
+        # Clear the hook without interfering with resources'
+        # `handle_signal` callbacks:
+        hook = details['unset_hook']
+        self.clear_hook(hook)
+        LOG.info(_LI('Clearing %(hook)s hook on %(resource)s'),
+                 {'hook': hook, 'resource': six.text_type(self)})
+        self._add_event(self.action, self.status,
+                        "Hook %s is cleared" % hook)
+
+    def _handle_signal(self, details):
+        if not callable(getattr(self, 'handle_signal', None)):
+            raise exception.ResourceActionNotSupported(action='signal')
 
         def get_string_details():
             if details is None:
@@ -1507,22 +1530,6 @@ class Resource(object):
 
             return 'Unknown'
 
-        # Clear the hook without interfering with resources'
-        # `handle_signal` callbacks:
-        if (details and 'unset_hook' in details and
-                environment.valid_hook_type(details.get('unset_hook'))):
-            hook = details['unset_hook']
-            if self.has_hook(hook):
-                self.clear_hook(hook)
-                LOG.info(_LI('Clearing %(hook)s hook on %(resource)s'),
-                         {'hook': hook, 'resource': six.text_type(self)})
-                self._add_event(self.action, self.status,
-                                "Hook %s is cleared" % hook)
-                return
-
-        if not callable(getattr(self, 'handle_signal', None)):
-            raise exception.ResourceActionNotSupported(action='signal')
-
         try:
             signal_result = self.handle_signal(details)
             if signal_result:
@@ -1539,6 +1546,20 @@ class Resource(object):
             failure = exception.ResourceFailure(ex, self)
             raise failure
 
+    def signal(self, details=None, need_check=True):
+        '''
+        signal the resource. Subclasses should provide a handle_signal() method
+        to implement the signal, the base-class raise an exception if no
+        handler is implemented.
+        '''
+        if need_check:
+            self._signal_check_action()
+            self._signal_check_hook(details)
+        if details and 'unset_hook' in details:
+            self._unset_hook(details)
+            return
+        self._handle_signal(details)
+
     def handle_update(self, json_snippet=None, tmpl_diff=None, prop_diff=None):
         if prop_diff:
             raise exception.UpdateReplace(self.name)

heat/engine/service.py

@@ -1329,9 +1329,9 @@ class EngineService(service.Service):
                           implementation.
         '''
 
-        def _resource_signal(stack, rsrc, details):
+        def _resource_signal(stack, rsrc, details, need_check):
             LOG.debug("signaling resource %s:%s" % (stack.name, rsrc.name))
-            rsrc.signal(details)
+            rsrc.signal(details, need_check)
 
             if not rsrc.signal_needs_metadata_updates:
                 return
@@ -1355,13 +1355,16 @@ class EngineService(service.Service):
         self._verify_stack_resource(stack, resource_name)
 
         rsrc = stack[resource_name]
+
         if callable(rsrc.signal):
+            rsrc._signal_check_action()
+            rsrc._signal_check_hook(details)
             if sync_call:
-                _resource_signal(stack, rsrc, details)
+                _resource_signal(stack, rsrc, details, False)
                 return rsrc.metadata_get()
             else:
                 self.thread_group_mgr.start(stack.id, _resource_signal,
-                                            stack, rsrc, details)
+                                            stack, rsrc, details, False)
 
     @context.request_context
     def find_physical_resource(self, cnxt, physical_resource_id):

heat/tests/test_engine_service.py

@@ -1240,21 +1240,24 @@ class StackServiceTest(common.HeatTestCase):
 
         self.m.VerifyAll()
 
-    def test_signal_reception_async(self):
+    def _stack_create(self, stack_name):
-        self.eng.thread_group_mgr = tools.DummyThreadGroupMgrLogStart()
-        stack_name = 'signal_reception_async'
         stack = tools.get_stack(stack_name, self.ctx, policy_template)
-        self.stack = stack
         tools.setup_keystone_mocks(self.m, stack)
         self.m.ReplayAll()
         stack.store()
         stack.create()
-        test_data = {'food': 'yum'}
-
         self.m.StubOutWithMock(service.EngineService, '_get_stack')
-        s = stack_object.Stack.get_by_id(self.ctx, self.stack.id)
+        s = stack_object.Stack.get_by_id(self.ctx, stack.id)
         service.EngineService._get_stack(self.ctx,
-                                         self.stack.identifier()).AndReturn(s)
+                                         stack.identifier()).AndReturn(s)
+        self.m.ReplayAll()
+        return stack
+
+    def test_signal_reception_async(self):
+        self.eng.thread_group_mgr = tools.DummyThreadGroupMgrLogStart()
+        stack_name = 'signal_reception_async'
+        self.stack = self._stack_create(stack_name)
+        test_data = {'food': 'yum'}
 
         self.m.ReplayAll()
 
@@ -1269,21 +1272,11 @@ class StackServiceTest(common.HeatTestCase):
 
     def test_signal_reception_sync(self):
         stack_name = 'signal_reception_sync'
-        stack = tools.get_stack(stack_name, self.ctx, policy_template)
+        self.stack = self._stack_create(stack_name)
-        self.stack = stack
-        tools.setup_keystone_mocks(self.m, stack)
-        self.m.ReplayAll()
-        stack.store()
-        stack.create()
         test_data = {'food': 'yum'}
 
-        self.m.StubOutWithMock(service.EngineService, '_get_stack')
-        s = stack_object.Stack.get_by_id(self.ctx, self.stack.id)
-        service.EngineService._get_stack(self.ctx,
-                                         self.stack.identifier()).AndReturn(s)
-
         self.m.StubOutWithMock(res.Resource, 'signal')
-        res.Resource.signal(mox.IgnoreArg()).AndReturn(None)
+        res.Resource.signal(mox.IgnoreArg(), False).AndReturn(None)
         self.m.ReplayAll()
 
         self.eng.resource_signal(self.ctx,
@@ -1295,20 +1288,9 @@ class StackServiceTest(common.HeatTestCase):
 
     def test_signal_reception_no_resource(self):
         stack_name = 'signal_reception_no_resource'
-        stack = tools.get_stack(stack_name, self.ctx, policy_template)
+        self.stack = self._stack_create(stack_name)
-        tools.setup_keystone_mocks(self.m, stack)
-        self.stack = stack
-        self.m.ReplayAll()
-        stack.store()
-        stack.create()
         test_data = {'food': 'yum'}
 
-        self.m.StubOutWithMock(service.EngineService, '_get_stack')
-        s = stack_object.Stack.get_by_id(self.ctx, self.stack.id)
-        service.EngineService._get_stack(self.ctx,
-                                         self.stack.identifier()).AndReturn(s)
-        self.m.ReplayAll()
-
         ex = self.assertRaises(dispatcher.ExpectedException,
                                self.eng.resource_signal, self.ctx,
                                dict(self.stack.identifier()),
@@ -1345,23 +1327,13 @@ class StackServiceTest(common.HeatTestCase):
         self.m.VerifyAll()
 
     def test_signal_returns_metadata(self):
-        stack = tools.get_stack('signal_reception', self.ctx, policy_template)
+        self.stack = self._stack_create('signal_reception')
-        self.stack = stack
+        rsrc = self.stack['WebServerScaleDownPolicy']
-        tools.setup_keystone_mocks(self.m, stack)
-        self.m.ReplayAll()
-        stack.store()
-        stack.create()
         test_metadata = {'food': 'yum'}
-        rsrc = stack['WebServerScaleDownPolicy']
         rsrc.metadata_set(test_metadata)
 
-        self.m.StubOutWithMock(service.EngineService, '_get_stack')
-        s = stack_object.Stack.get_by_id(self.ctx, self.stack.id)
-        service.EngineService._get_stack(self.ctx,
-                                         self.stack.identifier()).AndReturn(s)
-
         self.m.StubOutWithMock(res.Resource, 'signal')
-        res.Resource.signal(mox.IgnoreArg()).AndReturn(None)
+        res.Resource.signal(mox.IgnoreArg(), False).AndReturn(None)
         self.m.ReplayAll()
 
         md = self.eng.resource_signal(self.ctx,
@@ -1369,6 +1341,40 @@ class StackServiceTest(common.HeatTestCase):
                                       'WebServerScaleDownPolicy', None,
                                       sync_call=True)
         self.assertEqual(test_metadata, md)
+
+        self.m.VerifyAll()
+
+    def test_signal_unset_invalid_hook(self):
+        self.stack = self._stack_create('signal_unset_invalid_hook')
+        details = {'unset_hook': 'invalid_hook'}
+
+        ex = self.assertRaises(dispatcher.ExpectedException,
+                               self.eng.resource_signal,
+                               self.ctx,
+                               dict(self.stack.identifier()),
+                               'WebServerScaleDownPolicy',
+                               details)
+        msg = 'Invalid hook type "invalid_hook"'
+        self.assertIn(msg, six.text_type(ex.exc_info[1]))
+        self.assertEqual(exception.InvalidBreakPointHook,
+                         ex.exc_info[0])
+        self.m.VerifyAll()
+
+    def test_signal_unset_not_defined_hook(self):
+        self.stack = self._stack_create('signal_unset_not_defined_hook')
+        details = {'unset_hook': 'pre-update'}
+        ex = self.assertRaises(dispatcher.ExpectedException,
+                               self.eng.resource_signal,
+                               self.ctx,
+                               dict(self.stack.identifier()),
+                               'WebServerScaleDownPolicy',
+                               details)
+        msg = ('The "pre-update" hook is not defined on '
+               'AWSScalingPolicy "WebServerScaleDownPolicy"')
+        self.assertIn(msg, six.text_type(ex.exc_info[1]))
+        self.assertEqual(exception.InvalidBreakPointHook,
+                         ex.exc_info[0])
+
         self.m.VerifyAll()
 
     def test_signal_calls_metadata_update(self):
@@ -1385,7 +1391,7 @@ class StackServiceTest(common.HeatTestCase):
                                          self.stack.identifier()).AndReturn(s)
 
         self.m.StubOutWithMock(res.Resource, 'signal')
-        res.Resource.signal(mox.IgnoreArg()).AndReturn(None)
+        res.Resource.signal(mox.IgnoreArg(), False).AndReturn(None)
         self.m.StubOutWithMock(res.Resource, 'metadata_update')
         # this will be called once for the Random resource
         res.Resource.metadata_update().AndReturn(None)
@@ -1413,7 +1419,7 @@ class StackServiceTest(common.HeatTestCase):
                                          self.stack.identifier()).AndReturn(s)
 
         self.m.StubOutWithMock(res.Resource, 'signal')
-        res.Resource.signal(mox.IgnoreArg()).AndReturn(None)
+        res.Resource.signal(mox.IgnoreArg(), False).AndReturn(None)
         # this will never be called
         self.m.StubOutWithMock(res.Resource, 'metadata_update')
         self.m.ReplayAll()

heat/tests/test_resource.py

@@ -199,11 +199,10 @@ class ResourceTest(common.HeatTestCase):
             for status in res.STATUSES:
                 res.state_set(action, status)
                 ev = self.patchobject(res, '_add_event')
-                ex = self.assertRaises(exception.ResourceFailure,
+                ex = self.assertRaises(exception.NotSupported,
                                        res.signal)
-                self.assertEqual('Exception: resources.res: '
+                self.assertEqual('Signal resource during %s is not '
-                                 'Cannot signal resource during '
+                                 'supported.' % action, six.text_type(ex))
-                                 '%s' % action, six.text_type(ex))
                 ev.assert_called_with(
                     action, status,
                     'Cannot signal resource during %s' % action)
@@ -2643,10 +2642,10 @@ class ResourceHookTest(common.HeatTestCase):
         self.assertFalse(res.clear_hook.called)
 
         self.assertRaises(exception.ResourceActionNotSupported,
-                          res.signal, {})
+                          res.signal, {'other_hook': 'alarm'})
         self.assertFalse(res.clear_hook.called)
 
-        self.assertRaises(exception.ResourceActionNotSupported,
+        self.assertRaises(exception.InvalidBreakPointHook,
                           res.signal, {'unset_hook': 'unknown_hook'})
         self.assertFalse(res.clear_hook.called)
 
@@ -2657,7 +2656,7 @@ class ResourceHookTest(common.HeatTestCase):
         res.clear_hook.assert_called_with('pre-update')
 
         res.has_hook = mock.Mock(return_value=False)
-        self.assertRaises(exception.ResourceActionNotSupported,
+        self.assertRaises(exception.InvalidBreakPointHook,
                           res.signal, {'unset_hook': 'pre-create'})
 
 

heat/tests/test_signal.py

@@ -618,9 +618,7 @@ class SignalTest(common.HeatTestCase):
 
         self.m.VerifyAll()
 
-    def test_signal_reception_wrong_state(self):
+    def _test_signal_not_supported_action(self, action='DELETE'):
-        # assert that we get the correct exception when calling a
-        # resource.signal() that is in having a destructive action.
         self.stack = self.create_stack()
 
         self.m.ReplayAll()
@@ -629,14 +627,25 @@ class SignalTest(common.HeatTestCase):
         rsrc = self.stack['signal_handler']
         self.assertEqual((rsrc.CREATE, rsrc.COMPLETE), rsrc.state)
         # manually override the action to DELETE
-        rsrc.action = rsrc.DELETE
+        rsrc.action = action
 
         err_metadata = {'Data': 'foo', 'Status': 'SUCCESS', 'UniqueId': '123'}
-        self.assertRaises(exception.ResourceFailure, rsrc.signal,
+        msg = 'Signal resource during %s is not supported.' % action
-                          details=err_metadata)
+        exc = self.assertRaises(exception.NotSupported, rsrc.signal,
-
+                                details=err_metadata)
+        self.assertEqual(msg, six.text_type(exc))
         self.m.VerifyAll()
 
+    def test_signal_in_delete_state(self):
+        # assert that we get the correct exception when calling a
+        # resource.signal() that is in delete action.
+        self._test_signal_not_supported_action()
+
+    def test_signal_in_suspend_state(self):
+        # assert that we get the correct exception when calling a
+        # resource.signal() that is in suspend action.
+        self._test_signal_not_supported_action(action='SUSPEND')
+
     def test_signal_reception_failed_call(self):
         # assert that we get the correct exception from resource.signal()
         # when resource.handle_signal() raises an exception.

heat_integrationtests/functional/test_autoscaling.py

@@ -13,7 +13,9 @@
 import copy
 import json
 
+from heatclient import exc
 from oslo_log import log as logging
+import six
 from testtools import matchers
 
 from heat_integrationtests.common import test
@@ -728,8 +730,13 @@ outputs:
         self._wait_for_resource_status(
             stack_identifier, 'JobServerGroup', 'SUSPEND_COMPLETE')
 
-        # Send a signal and confirm nothing happened.
+        # Send a signal and a exception will raise
-        self.client.resources.signal(stack_identifier, 'ScaleUpPolicy')
+        ex = self.assertRaises(exc.BadRequest,
+                               self.client.resources.signal,
+                               stack_identifier, 'ScaleUpPolicy')
+
+        error_msg = 'Signal resource during SUSPEND is not supported'
+        self.assertIn(error_msg, six.text_type(ex))
         ev = self.wait_for_event_with_reason(
             stack_identifier,
             reason='Cannot signal resource during SUSPEND',