Skip to content

Commit

Permalink
Check for incident enabled before making mobile app request (#4947)
Browse files Browse the repository at this point in the history 
Related to some issues we have noticed in our logs
([example](https://ops.grafana-ops.net/goto/8sa28TqIR?orgId=1)).
Also make sure unexpected responses are logged too.
  • Loading branch information
@matiasb
matiasb authored Aug 28, 2024
1 parent 5862053 commit c281813
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 22 deletions.
91 changes: 72 additions & 19 deletions engine/apps/mobile_app/tests/test_mobile_app_gateway.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,28 @@ def enable_mobile_app_gateway(settings):
settings.GRAFANA_CLOUD_AUTH_API_SYSTEM_TOKEN = "zxcvzx"


@pytest.fixture()
def make_organization_with_mobile_app_auth_token(
make_organization_and_user_with_mobile_app_auth_token,
):
def _make_organization_with_mobile_app_auth_token(*args, grafana_incident_enabled=True, **kwargs):
org, user, auth_token = make_organization_and_user_with_mobile_app_auth_token(*args, **kwargs)
org.is_grafana_incident_enabled = grafana_incident_enabled
org.save()
return org, user, auth_token

return _make_organization_with_mobile_app_auth_token


class MockResponse:
def __init__(self, status_code=status.HTTP_200_OK, data=MOCK_DOWNSTREAM_RESPONSE_DATA):
self.status_code = status_code
self.data = data

def raise_for_status(self):
if self.status_code >= status.HTTP_400_BAD_REQUEST:
raise requests.exceptions.HTTPError()

def json(self):
return self.data

Expand All @@ -43,12 +60,12 @@ def json(self):
def test_mobile_app_gateway_properly_proxies_paths(
_mock_get_downstream_headers,
mock_requests,
make_organization_and_user_with_mobile_app_auth_token,
make_organization_with_mobile_app_auth_token,
path,
):
mock_requests.post.return_value = MockResponse()

org, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
org, _, auth_token = make_organization_with_mobile_app_auth_token()
org.grafana_incident_backend_url = MOCK_DOWNSTREAM_INCIDENT_API_URL
org.save()

Expand Down Expand Up @@ -77,13 +94,13 @@ def test_mobile_app_gateway_supports_all_methods(
_mock_get_downstream_headers,
_mock_get_downstream_url,
mock_requests,
make_organization_and_user_with_mobile_app_auth_token,
make_organization_with_mobile_app_auth_token,
method,
):
mock_http_verb_method = getattr(mock_requests, method.lower())
mock_http_verb_method.return_value = MockResponse()

_, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
_, _, auth_token = make_organization_with_mobile_app_auth_token()

client = APIClient()
url = reverse("mobile_app:gateway", kwargs={"downstream_backend": DOWNSTREAM_BACKEND, "downstream_path": "test"})
Expand All @@ -102,11 +119,11 @@ def test_mobile_app_gateway_proxies_query_params(
_mock_get_downstream_headers,
_mock_get_downstream_url,
mock_requests,
make_organization_and_user_with_mobile_app_auth_token,
make_organization_with_mobile_app_auth_token,
):
mock_requests.post.return_value = MockResponse()

_, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
_, _, auth_token = make_organization_with_mobile_app_auth_token()

client = APIClient()
url = reverse("mobile_app:gateway", kwargs={"downstream_backend": DOWNSTREAM_BACKEND, "downstream_path": "test"})
Expand Down Expand Up @@ -139,12 +156,12 @@ def test_mobile_app_gateway_properly_proxies_request_body(
_mock_get_downstream_headers,
_mock_get_downstream_url,
mock_requests,
make_organization_and_user_with_mobile_app_auth_token,
make_organization_with_mobile_app_auth_token,
upstream_request_body,
):
mock_requests.post.return_value = MockResponse()

_, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
_, _, auth_token = make_organization_with_mobile_app_auth_token()

client = APIClient()
url = reverse("mobile_app:gateway", kwargs={"downstream_backend": DOWNSTREAM_BACKEND, "downstream_path": "test"})
Expand Down Expand Up @@ -182,13 +199,13 @@ def test_mobile_app_gateway_supported_downstream_backends(
_mock_get_downstream_headers,
_mock_get_downstream_url,
mock_requests,
make_organization_and_user_with_mobile_app_auth_token,
make_organization_with_mobile_app_auth_token,
downstream_backend,
expected_status,
):
mock_requests.post.return_value = MockResponse()

_, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
_, _, auth_token = make_organization_with_mobile_app_auth_token()

client = APIClient()
url = reverse(
Expand Down Expand Up @@ -218,7 +235,7 @@ def test_mobile_app_gateway_catches_errors_from_downstream_server(
_mock_get_downstream_headers,
_mock_get_downstream_url,
mock_requests_post,
make_organization_and_user_with_mobile_app_auth_token,
make_organization_with_mobile_app_auth_token,
ExceptionClass,
exception_args,
expected_status,
Expand All @@ -228,7 +245,7 @@ def _raise_exception(*args, **kwargs):

mock_requests_post.side_effect = _raise_exception

_, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
_, _, auth_token = make_organization_with_mobile_app_auth_token()

client = APIClient()
url = reverse("mobile_app:gateway", kwargs={"downstream_backend": DOWNSTREAM_BACKEND, "downstream_path": "test"})
Expand All @@ -237,6 +254,42 @@ def _raise_exception(*args, **kwargs):
assert response.status_code == expected_status


@pytest.mark.django_db
@patch("apps.mobile_app.views.requests.post")
@patch("apps.mobile_app.views.MobileAppGatewayView._get_downstream_url", return_value=MOCK_DOWNSTREAM_URL)
@patch("apps.mobile_app.views.MobileAppGatewayView._get_downstream_headers", return_value=MOCK_DOWNSTREAM_HEADERS)
def test_mobile_app_gateway_catches_and_forwards_error_from_downstream_server(
_mock_get_downstream_headers,
_mock_get_downstream_url,
mock_requests_post,
make_organization_with_mobile_app_auth_token,
):
mock_requests_post.return_value = MockResponse(status_code=status.HTTP_400_BAD_REQUEST, data={"error": "foo"})

_, _, auth_token = make_organization_with_mobile_app_auth_token()

client = APIClient()
url = reverse("mobile_app:gateway", kwargs={"downstream_backend": DOWNSTREAM_BACKEND, "downstream_path": "test"})

response = client.post(url, HTTP_AUTHORIZATION=auth_token)
# forward status and response from downstream
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert response.json() == {"error": "foo"}


@pytest.mark.django_db
def test_mobile_app_gateway_not_found_if_incident_disabled(
make_organization_with_mobile_app_auth_token,
):
_, _, auth_token = make_organization_with_mobile_app_auth_token(grafana_incident_enabled=False)

client = APIClient()
url = reverse("mobile_app:gateway", kwargs={"downstream_backend": DOWNSTREAM_BACKEND, "downstream_path": "test"})

response = client.post(url, HTTP_AUTHORIZATION=auth_token)
assert response.status_code == status.HTTP_404_NOT_FOUND


@pytest.mark.django_db
@patch("apps.mobile_app.views.requests")
@patch("apps.mobile_app.views.MobileAppGatewayView._get_downstream_url", return_value=MOCK_DOWNSTREAM_URL)
Expand All @@ -245,11 +298,11 @@ def test_mobile_app_gateway_mobile_app_auth_token(
_mock_get_downstream_headers,
_mock_get_downstream_url,
mock_requests,
make_organization_and_user_with_mobile_app_auth_token,
make_organization_with_mobile_app_auth_token,
):
mock_requests.post.return_value = MockResponse()

_, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
_, _, auth_token = make_organization_with_mobile_app_auth_token()

client = APIClient()
url = reverse("mobile_app:gateway", kwargs={"downstream_backend": DOWNSTREAM_BACKEND, "downstream_path": "test"})
Expand All @@ -267,7 +320,7 @@ def test_mobile_app_gateway_mobile_app_auth_token(
def test_mobile_app_gateway_incident_api_url(
_mock_get_downstream_headers,
mock_requests,
make_organization_and_user_with_mobile_app_auth_token,
make_organization_with_mobile_app_auth_token,
):
mock_incident_backend_url = "https://mockincidentbackend.com"
mock_requests.post.return_value = MockResponse()
Expand All @@ -276,14 +329,14 @@ def test_mobile_app_gateway_incident_api_url(
url = reverse("mobile_app:gateway", kwargs={"downstream_backend": DOWNSTREAM_BACKEND, "downstream_path": "test"})

# Organization has no incident backend URL saved
organization, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
organization, _, auth_token = make_organization_with_mobile_app_auth_token()
assert organization.grafana_incident_backend_url is None

response = client.post(url, HTTP_AUTHORIZATION=auth_token)
assert response.status_code == status.HTTP_400_BAD_REQUEST

# Organization already has incident backend URL saved
organization, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
organization, _, auth_token = make_organization_with_mobile_app_auth_token()
organization.grafana_incident_backend_url = mock_incident_backend_url
organization.save()

Expand All @@ -299,11 +352,11 @@ def test_mobile_app_gateway_proxies_headers(
_mock_get_downstream_url,
_mock_get_auth_token,
mock_requests,
make_organization_and_user_with_mobile_app_auth_token,
make_organization_with_mobile_app_auth_token,
):
mock_requests.post.return_value = MockResponse()

_, _, auth_token = make_organization_and_user_with_mobile_app_auth_token()
_, _, auth_token = make_organization_with_mobile_app_auth_token()

client = APIClient()
url = reverse("mobile_app:gateway", kwargs={"downstream_backend": DOWNSTREAM_BACKEND, "downstream_path": "test"})
Expand Down
16 changes: 13 additions & 3 deletions engine/apps/mobile_app/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,17 +197,23 @@ def _proxy_request(self, request: Request, *args, **kwargs) -> Response:
downstream_path = kwargs["downstream_path"]
method = request.method
user = request.user
org = user.organization

if downstream_backend not in self.ALL_SUPPORTED_DOWNSTREAM_BACKENDS:
raise NotFound(f"Downstream backend {downstream_backend} not supported")

if downstream_backend == self.SupportedDownstreamBackends.INCIDENT and not org.is_grafana_incident_enabled:
raise NotFound(f"Incident is not enabled for organization {org.pk}")

downstream_url = self._get_downstream_url(user.organization, downstream_backend, downstream_path)

log_msg_common = f"{downstream_backend} request to {method} {downstream_url}"
logger.info(f"Proxying {log_msg_common}")

downstream_request_handler = getattr(requests, method.lower())

final_status = None
response_data = None
try:
downstream_response = downstream_request_handler(
downstream_url,
Expand All @@ -217,8 +223,12 @@ def _proxy_request(self, request: Request, *args, **kwargs) -> Response:
timeout=PROXY_REQUESTS_TIMEOUT, # set a timeout to prevent hanging
)
final_status = downstream_response.status_code
response_data = downstream_response.json()
logger.info(f"Successfully proxied {log_msg_common}")
return Response(status=downstream_response.status_code, data=downstream_response.json())
# raise an exception if the response status code is not 2xx
# (exception handler will log and still return the response)
downstream_response.raise_for_status()
return Response(status=downstream_response.status_code, data=response_data)
except (
requests.exceptions.RequestException,
requests.exceptions.JSONDecodeError,
Expand All @@ -229,7 +239,7 @@ def _proxy_request(self, request: Request, *args, **kwargs) -> Response:
final_status = status.HTTP_400_BAD_REQUEST
elif isinstance(e, requests.exceptions.Timeout):
final_status = status.HTTP_504_GATEWAY_TIMEOUT
else:
elif final_status is None:
final_status = status.HTTP_502_BAD_GATEWAY

logger.error(
Expand All @@ -243,7 +253,7 @@ def _proxy_request(self, request: Request, *args, **kwargs) -> Response:
),
exc_info=True,
)
return Response(status=final_status)
return Response(status=final_status, data=response_data)
finally:
request_end = time.perf_counter()
seconds = request_end - request_start
Expand Down

0 comments on commit c281813

Please sign in to comment.