From 0e6486be8b4b2e1d2af6f8a7b44dd82956ccc5f3 Mon Sep 17 00:00:00 2001
From: Andrey Volkov <av903u@att.com>
Date: Fri, 28 Sep 2018 14:54:19 -0700
Subject: [PATCH] Return non-200 response if Airflow log request failed

This patch modifies API behavior for
GET /v1.0/actions/{action_id}/steps/{step_id}/logs
such way:
- it returns the same status code as Airflow HTTP request returned
  if Airflow responds with a status code of 400 or greater,
- it returns 500 error status code if an exception happens during
  Airflow HTTP request (200 was before).

Warning: this change breaks API backward compatibility, now a client
could get 4xx or 5xx codes proxied from Airflow.

Change-Id: Ic5dceb3abc34415d21b4d8d4e71b4e5661a7363d
---
 doc/source/API.rst                            |  4 +++
 .../action/actions_steps_id_logs_api.py       | 28 +++++++++++++------
 .../control/test_actions_steps_id_logs_api.py | 25 +++++++++++++++++
 3 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/doc/source/API.rst b/doc/source/API.rst
index 7e065aca..fdc71ab9 100644
--- a/doc/source/API.rst
+++ b/doc/source/API.rst
@@ -1151,6 +1151,10 @@ try={int try_number}
 Responses
 '''''''''
 200 OK
+4xx or 5xx
+
+A 4xx or 5xx code will be returned if some error happens during
+Airflow HTTP request or Airflow responds with a status code of 400 or greater.
 
 Example
 '''''''
diff --git a/src/bin/shipyard_airflow/shipyard_airflow/control/action/actions_steps_id_logs_api.py b/src/bin/shipyard_airflow/shipyard_airflow/control/action/actions_steps_id_logs_api.py
index 770ea1a5..7943f76b 100644
--- a/src/bin/shipyard_airflow/shipyard_airflow/control/action/actions_steps_id_logs_api.py
+++ b/src/bin/shipyard_airflow/shipyard_airflow/control/action/actions_steps_id_logs_api.py
@@ -21,6 +21,7 @@ from oslo_config import cfg
 from shipyard_airflow import policy
 from shipyard_airflow.control.base import BaseResource
 from shipyard_airflow.control.helpers.action_helper import ActionsHelper
+from shipyard_airflow.errors import ApiError
 
 CONF = cfg.CONF
 LOG = logging.getLogger(__name__)
@@ -116,18 +117,29 @@ class ActionsStepsLogsResource(BaseResource):
         """
         Retrieve Logs
         """
-        try:
-            LOG.debug("Retrieving Airflow logs...")
 
+        LOG.debug("Retrieving Airflow logs...")
+        try:
             response = requests.get(
                 log_endpoint,
                 timeout=(
                     CONF.requests_config.airflow_log_connect_timeout,
                     CONF.requests_config.airflow_log_read_timeout))
-
-            return response.text
-
         except requests.exceptions.RequestException as e:
-            LOG.info(e)
-            LOG.info("Unable to retrieve requested logs")
-            return []
+            LOG.exception(e)
+            raise ApiError(
+                title='Log retrieval error',
+                description='Exception happened during Airflow API request',
+                status=falcon.HTTP_500)
+        if response.status_code >= 400:
+            LOG.info('Airflow endpoint returned error status code %s, '
+                     'content %s. Response code will be bubbled up',
+                     response.status_code, response.text)
+            raise ApiError(
+                title='Log retrieval error',
+                description='Airflow endpoint returned error status code',
+                status=getattr(
+                    falcon,
+                    'HTTP_%d' % response.status_code,
+                    falcon.HTTP_500))
+        return response.text
diff --git a/src/bin/shipyard_airflow/tests/unit/control/test_actions_steps_id_logs_api.py b/src/bin/shipyard_airflow/tests/unit/control/test_actions_steps_id_logs_api.py
index 3afc977d..2b69629d 100644
--- a/src/bin/shipyard_airflow/tests/unit/control/test_actions_steps_id_logs_api.py
+++ b/src/bin/shipyard_airflow/tests/unit/control/test_actions_steps_id_logs_api.py
@@ -15,8 +15,13 @@ from datetime import datetime
 from unittest import mock
 from unittest.mock import patch
 
+import falcon
+import pytest
+import requests
+
 from shipyard_airflow.control.action.actions_steps_id_logs_api import \
     ActionsStepsLogsResource
+from shipyard_airflow.errors import ApiError
 from tests.unit.control import common
 
 # Define Global Variables
@@ -194,3 +199,23 @@ class TestActionsStepsLogsEndpoint():
         result = action_logs_resource.retrieve_logs(log_endpoint)
 
         assert result == XCOM_RUN_LOGS
+
+    @mock.patch('requests.get')
+    def test_retrieve_logs_404(self, mock_get):
+        mock_get.return_value.status_code = 404
+        action_logs_resource = ActionsStepsLogsResource()
+        with pytest.raises(ApiError) as e:
+            action_logs_resource.retrieve_logs(None)
+        assert ('Airflow endpoint returned error status code' in
+                e.value.description)
+        assert falcon.HTTP_404 == e.value.status
+
+    @mock.patch('requests.get')
+    def test_retrieve_logs_error(self, mock_get):
+        mock_get.side_effect = requests.exceptions.ConnectionError
+        action_logs_resource = ActionsStepsLogsResource()
+        with pytest.raises(ApiError) as e:
+            action_logs_resource.retrieve_logs(None)
+        assert ("Exception happened during Airflow API request" in
+                e.value.description)
+        assert falcon.HTTP_500 == e.value.status