exceptions.check_message does not work

[Ryan Rossiter]

When using exceptions.check_message to check if a connection was refused by an API, check_message will check to see if the exception's message contains "Connection" and "refused". When a connection is refused, the message does not contain exactly "Connection" and "refused", but rather "Connection" and "refused)". In this situation check_message does nothing and the exception is re-raised.

To reproduce:

Stop the Nova API
Navigate to http://<ip>/admin/info/

Other notes:
1. Even if the error message contains "Connection" and "refused" exactly, the same error screen is shown, because the exception is re-raised in both situations, and is not being handled properly higher up.

2. The exception being raised when a connection is refused is ConnectionError, a Python built-in exception.

3. The only other situation where check_message is used within Horizon is in https://github.com/openstack/horizon/blob/master/openstack_dashboard/dashboards/project/volumes/volumes/tables.py when a volume is deleted. When this occurs, the re-raised exception is properly handled, and a red error message is shown correctly.

4. The current use of check_message is not compatible with localization. The keywords checked for are not translated, so if the exception message is translated, the words will never match.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/106864

[OpenStack Infra (hudson-openstack)]

Change abandoned by David Lyle (<email address hidden>) on branch: master
Review: https://review.openstack.org/106864
Reason: This review is > 4 weeks without comment, and failed Jenkins the last time it was checked. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

[Robin Cernin (rcernin)]

> /opt/stack/horizon/openstack_dashboard/dashboards/admin/info/tabs.py(71)get_nova_services_data()
-> exceptions.handle(self.request, msg)
(Pdb) l
 66 services = nova.service_list(self.tab_group.request)
 67 except Exception:
 68 msg = _('Unable to get nova services list.')
 69 from remote_pdb import RemotePdb; RemotePdb('127.0.0.1', 444).set_trace()
 70 exceptions.check_message(["Connection", "refused"], msg)
 71 -> exceptions.handle(self.request, msg)
 72 services = []
 73 return services
 74
 75
 76 class CinderServicesTab(tabs.TableTab):
(Pdb) exceptions.handle(self.request, msg)
<class 'horizon.exceptions.RecoverableError'>

Exceptions are handled at L#71.

The following is skipped since the exception (msg) doesn't contain the Connection refused:

We are setting msg = _('Unable to get nova services list.') and expect ["Connection", "refused"]

Either we remove the check_message or fix the exception.

[Robin Cernin (rcernin)]

We use the following check_message in:

horizon/exceptions.py
openstack_dashboard/dashboards/admin/aggregates/workflows.py
openstack_dashboard/dashboards/admin/flavors/workflows.py
openstack_dashboard/dashboards/admin/info/tabs.py
openstack_dashboard/dashboards/project/cgroups/workflows.py
openstack_dashboard/dashboards/project/volume_groups/workflows.py

The definition is:

96 def check_message(keywords, message):
197 """Checks an exception for given keywords and raises an error if found.
198
199 It raises a new ``ActionError`` with the desired message if the
200 keywords are found. This allows selective
201 control over API error messages.
202 """
203 exc_type, exc_value, exc_traceback = sys.exc_info()
204 if set(str(exc_value).split(" ")).issuperset(set(keywords)):
205 exc_value.message = message
206 # NOTE: This function is intended to call inside an except clause.
207 # pylint: disable=misplaced-bare-raise ...

[Robin Cernin (rcernin)]

Use handle instead of check_message

horizon/exceptions.py

249 def handle(request, message=None, redirect=None, ignore=False,
250 escalate=False, log_level=None, force_log=None):
251 """Centralized error handling for Horizon.
252
253 Because Horizon consumes so many different APIs with completely
254 different ``Exception`` types, it's necessary to have a centralized
255 place for handling exceptions which may be raised.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/652427

[Robin Cernin (rcernin)]

@Akihiro, Thank you so much for your time. After further review it seems that I have misunderstood the check_message function as it checks the original exception from Nova in the background against the keywords and returns message.

If that fails to raise an exception then exception.handle is called. I have a feeling we will never raise exception in the check messages, as if we shutdown nova services we can only get ERROR 503 and there is no Connection refused string.

Do you think it's possible to ever get that Connection refused? I was thinking about it and maybe we should simply close the bug as WON'T FIX?

https://github.com/openstack/horizon/blob/master/horizon/exceptions.py#L196

(Pdb) set(keywords)
{'refused', 'Connection'}
(Pdb) p set(str(exc_value).split(" "))
{'(HTTP', 'Error', '503)', 'Unknown'}
(Pdb) p message
'Unable to get nova services list.'
(Pdb)

[Robin Cernin (rcernin)]

I just faked that check_message to raise exception and Horizon failed completely, it seems to me lucky we are checking for something that won't trigger, so handle function actually can properly error out with recoverable error.

https://github.com/zerodayz/openstack/blob/master/changes/Change_652427.md#check_message-function

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/655119

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/655119
Committed: https://git.openstack.org/cgit/openstack/manila-ui/commit/?id=e81a424f8715fa00e49cae902da4cd1824deaef4
Submitter: Zuul
Branch: master

commit e81a424f8715fa00e49cae902da4cd1824deaef4
Author: Robin Cernin <email address hidden>
Date: Tue Apr 23 22:12:11 2019 +1000

    Error handling: Use exceptions.handle instead of check_message

    We already have a centralized place for handling exceptions in horizon.
    We should use horizon/exceptions.py(249)handle() instead of old
    horizon/exceptions.py(96)check_message().

    Change-Id: I4add35f6ca407d8c3fa8ab5bd1b96221b1f5b043
    Closes-Bug: #1339885

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/652427
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=31f7fc6bb24995376bb74d7de6866f5bf2b322c8
Submitter: Zuul
Branch: master

commit 31f7fc6bb24995376bb74d7de6866f5bf2b322c8
Author: Robin Cernin <email address hidden>
Date: Mon Apr 15 10:22:15 2019 +1000

    Deprecate exceptions.check_message

    exceptions.handle() is used in most cases consistently.
    check_message() is a legacy which was introduced when exceptions
    were not well classified. exceptions.handle() should cover all
    common error scenarios and there is no role played by check_messages().
    Let's clean up its usage and deprecate it for the future removal.

    Co-Authored-By: Akihiro Motoki <email address hidden>
    Change-Id: I545b6c666d13d39cf5287ccc7c972dc746faf2fb
    Closes-Bug: #1339885

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/horizon 18.0.0 release.