Correctly decode error messages from ironic API

Knowing a status code is simply not enough for debugging.

Change-Id: If1d3f182ab028948ff05aea7e8024d4e7bc3d53c

3 files changed, 89 insertions, 12 deletions

diff --git a/ironic_python_agent/ironic_api_client.py b/ironic_python_agent/ironic_api_client.py
index 141ed7bd..81573324 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 07f6f0cb..5de49f4e 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 00000000..e47f6e8f
--- /dev/null
+++ b/releasenotes/notes/ironic-error-97e76d9ddacff039.yaml
@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    Correctly decodes error messages from ironic API.