From 53dbc87a358aab04aa3417ee747c381be552dcb0 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 7 Dec 2020 18:57:08 +0100 Subject: [PATCH] Correctly decode error messages from ironic API Knowing a status code is simply not enough for debugging. Change-Id: If1d3f182ab028948ff05aea7e8024d4e7bc3d53c --- ironic_python_agent/ironic_api_client.py | 35 ++++++++--- .../tests/unit/test_ironic_api_client.py | 62 +++++++++++++++++-- .../notes/ironic-error-97e76d9ddacff039.yaml | 4 ++ 3 files changed, 89 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/ironic-error-97e76d9ddacff039.yaml diff --git a/ironic_python_agent/ironic_api_client.py b/ironic_python_agent/ironic_api_client.py index 141ed7bd8..815733243 100644 --- a/ironic_python_agent/ironic_api_client.py +++ b/ironic_python_agent/ironic_api_client.py @@ -117,6 +117,26 @@ class APIClient(object): def supports_auto_tls(self): return self._get_ironic_api_version() >= AGENT_VERIFY_CA_IRONIC_VERSION + def _error_from_response(self, response): + try: + body = response.json() + except ValueError: + text = response.text + else: + body = body.get('error_message', body) + if not isinstance(body, dict): + # Old ironic format + try: + body = jsonutils.loads(body) + except ValueError: + body = {} + + text = (body.get('faultstring') + or body.get('title') + or response.text) + + return 'Error %d: %s' % (response.status_code, text) + def heartbeat(self, uuid, advertise_address, advertise_protocol='http', generated_cert=None): path = self.heartbeat_api.format(uuid=uuid) @@ -148,11 +168,11 @@ class APIClient(object): raise errors.HeartbeatError(str(e)) if response.status_code == requests.codes.CONFLICT: - data = jsonutils.loads(response.content) - raise errors.HeartbeatConflictError(data.get('faultstring')) + error = self._error_from_response(response) + raise errors.HeartbeatConflictError(error) elif response.status_code != requests.codes.ACCEPTED: - msg = 'Invalid status code: {}'.format(response.status_code) - raise errors.HeartbeatError(msg) + error = self._error_from_response(response) + raise errors.HeartbeatError(error) def lookup_node(self, hardware_info, timeout, starting_interval, node_uuid=None, max_interval=30): @@ -225,9 +245,10 @@ class APIClient(object): if response.status_code != requests.codes.OK: LOG.warning( - 'Failed looking up node with addresses %r at %s, ' - 'status code: %s. Check if inspection has completed.', - params['addresses'], self.api_url, response.status_code, + 'Failed looking up node with addresses %r at %s. ' + '%s. Check if inspection has completed.', + params['addresses'], self.api_url, + self._error_from_response(response) ) return False diff --git a/ironic_python_agent/tests/unit/test_ironic_api_client.py b/ironic_python_agent/tests/unit/test_ironic_api_client.py index 07f6f0cba..5de49f4ed 100644 --- a/ironic_python_agent/tests/unit/test_ironic_api_client.py +++ b/ironic_python_agent/tests/unit/test_ironic_api_client.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import json from unittest import mock from oslo_config import cfg @@ -32,7 +33,9 @@ CONF = cfg.CONF class FakeResponse(object): def __init__(self, content=None, status_code=200, headers=None): content = content or {} - self.content = jsonutils.dumps(content) + self.text = json.dumps(content) + # TODO(dtantsur): remove in favour of using text/json() + self.content = self.text.encode('utf-8') self._json = content self.status_code = status_code self.headers = headers or {} @@ -260,13 +263,62 @@ class TestBaseIronicPythonAgent(base.IronicAgentTest): def test_heartbeat_invalid_status_code(self): response = FakeResponse(status_code=404) + response.text = 'Not a JSON' self.api_client.session.request = mock.Mock() self.api_client.session.request.return_value = response - self.assertRaises(errors.HeartbeatError, - self.api_client.heartbeat, - uuid='deadbeef-dabb-ad00-b105-f00d00bab10c', - advertise_address=('192.0.2.1', '9999')) + self.assertRaisesRegex(errors.HeartbeatError, + 'Error 404: Not a JSON', + self.api_client.heartbeat, + uuid='deadbeef-dabb-ad00-b105-f00d00bab10c', + advertise_address=('192.0.2.1', '9999')) + + def test_heartbeat_error_format_1(self): + response = FakeResponse( + status_code=404, + content={'error_message': '{"faultcode": "Client", ' + '"faultstring": "Resource could not be found.", ' + '"debuginfo": null}'}) + self.api_client.session.request = mock.Mock() + self.api_client.session.request.return_value = response + + self.assertRaisesRegex(errors.HeartbeatError, + 'Error 404: Resource could not be found', + self.api_client.heartbeat, + uuid='deadbeef-dabb-ad00-b105-f00d00bab10c', + advertise_address=('192.0.2.1', '9999')) + + def test_heartbeat_error_format_2(self): + response = FakeResponse( + status_code=404, + content={'error_message': { + "faultcode\\": "Client", + "faultstring": "Resource could not be found.", + "debuginfo": None}}) + self.api_client.session.request = mock.Mock() + self.api_client.session.request.return_value = response + + self.assertRaisesRegex(errors.HeartbeatError, + 'Error 404: Resource could not be found', + self.api_client.heartbeat, + uuid='deadbeef-dabb-ad00-b105-f00d00bab10c', + advertise_address=('192.0.2.1', '9999')) + + def test_heartbeat_error_format_3(self): + response = FakeResponse( + status_code=404, + content={'error_message': { + "code": 404, + "title": "Resource could not be found.", + "description": None}}) + self.api_client.session.request = mock.Mock() + self.api_client.session.request.return_value = response + + self.assertRaisesRegex(errors.HeartbeatError, + 'Error 404: Resource could not be found', + self.api_client.heartbeat, + uuid='deadbeef-dabb-ad00-b105-f00d00bab10c', + advertise_address=('192.0.2.1', '9999')) def test_heartbeat_409_status_code(self): response = FakeResponse(status_code=409) diff --git a/releasenotes/notes/ironic-error-97e76d9ddacff039.yaml b/releasenotes/notes/ironic-error-97e76d9ddacff039.yaml new file mode 100644 index 000000000..e47f6e8f4 --- /dev/null +++ b/releasenotes/notes/ironic-error-97e76d9ddacff039.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + Correctly decodes error messages from ironic API.