Improve error handling

Send clear error message to client if an external service request fails
and log the error.

Simplifies mocking network requests with requests-mock library.

JIRA: RHELWF-10400

greenwave/resources.py

@@ -49,6 +49,18 @@ def _requests_timeout():
     return timeout
 
 
+def _raise_for_status(response):
+    if response.ok:
+        return
+
+    msg = (
+        f"Got unexpected status code {response.status_code} for"
+        f" {response.url}: {response.text}"
+    )
+    log.error(msg)
+    raise BadGateway(msg)
+
+
 class BaseRetriever:
     ignore_ids: List[int]
     url: str
@@ -69,7 +81,7 @@ def retrieve(self, *args, **kwargs):
 
     def _retrieve_data(self, params):
         response = self._make_request(params)
-        response.raise_for_status()
+        _raise_for_status(response)
         return response.json()['data']
 
 
@@ -286,5 +298,5 @@ def retrieve_yaml_remote_rule(url: str):
 
     # remote rule file found...
     response = requests_session.request('GET', url)
-    response.raise_for_status()
+    _raise_for_status(response)
     return response.content

greenwave/tests/test_policies.py

@@ -689,7 +689,7 @@ def test_remote_rule_with_multiple_contexts(tmpdir):
             assert answer_types(decision.answers) == ['fetched-gating-yaml', 'test-result-passed']
 
 
-def test_get_sub_policies_multiple_urls(tmpdir):
+def test_get_sub_policies_multiple_urls(tmpdir, requests_mock):
     """ Testing the RemoteRule with the koji interaction when on_demand policy is given.
     In this case we are just mocking koji """
 
@@ -719,32 +719,25 @@ def test_get_sub_policies_multiple_urls(tmpdir):
     with app.app_context():
         with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm:
             scm.return_value = ('rpms', 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb')
-            with mock.patch('greenwave.resources.requests_session') as session:
-                response = mock.MagicMock()
-                response.status_code = 404
-                session.request.return_value = response
+            urls = [
+                'https://src{0}.fp.org/{1}/{2}/raw/{3}/f/gating.yaml'.format(i, *scm.return_value)
+                for i in range(1, 3)
+            ]
+            for url in urls:
+                requests_mock.head(url, status_code=404)
 
-                policy = OnDemandPolicy.create_from_json(serverside_json)
-                assert isinstance(policy.rules[0], RemoteRule)
-                assert policy.rules[0].required
+            policy = OnDemandPolicy.create_from_json(serverside_json)
+            assert isinstance(policy.rules[0], RemoteRule)
+            assert policy.rules[0].required
 
-                results = DummyResultsRetriever()
-                decision = Decision(None, 'fedora-26')
-                decision.check(subject, [policy], results)
-                expected_call1 = mock.call(
-                    'HEAD', 'https://src1.fp.org/{0}/{1}/raw/{2}/f/gating.yaml'.format(
-                        *scm.return_value
-                    )
-                )
-                expected_call2 = mock.call(
-                    'HEAD', 'https://src2.fp.org/{0}/{1}/raw/{2}/f/gating.yaml'.format(
-                        *scm.return_value
-                    )
-                )
-                assert session.request.mock_calls == [expected_call1, expected_call2]
-                assert answer_types(decision.answers) == ['missing-gating-yaml']
-                assert not decision.answers[0].is_satisfied
-                assert decision.answers[0].subject.identifier == subject.identifier
+            results = DummyResultsRetriever()
+            decision = Decision(None, 'fedora-26')
+            decision.check(subject, [policy], results)
+            request_history = [(r.method, r.url) for r in requests_mock.request_history]
+            assert request_history == [('HEAD', urls[0]), ('HEAD', urls[1])]
+            assert answer_types(decision.answers) == ['missing-gating-yaml']
+            assert not decision.answers[0].is_satisfied
+            assert decision.answers[0].subject.identifier == subject.identifier
 
 
 def test_get_sub_policies_scm_error(tmpdir):

greenwave/tests/test_retrieve_gating_yaml.py

@@ -1,11 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0+
 
+import re
 import socket
-from requests.exceptions import ConnectionError, HTTPError
+from requests.exceptions import ConnectionError
 
 import pytest
-import mock
-from werkzeug.exceptions import NotFound
+from werkzeug.exceptions import BadGateway, NotFound
 
 from greenwave.resources import (
     NoSourceException,
@@ -119,43 +119,39 @@ def test_retrieve_scm_from_build_with_missing_rev(app, koji_proxy):
         retrieve_scm_from_koji(nvr)
 
 
-def test_retrieve_yaml_remote_rule_no_namespace(app):
-    with mock.patch('greenwave.resources.requests_session') as session:
-        # Return 404, because we are only interested in the URL in the request
-        # and whether it is correct even with empty namespace.
-        response = mock.MagicMock()
-        response.status_code = 404
-        session.request.return_value = response
-        returned_file = retrieve_yaml_remote_rule(
-            app.config['REMOTE_RULE_POLICIES']['*'].format(
-                rev='deadbeaf', pkg_name='pkg', pkg_namespace=''
-            )
+def test_retrieve_yaml_remote_rule_no_namespace(app, requests_mock):
+    requests_mock.head(
+        'https://src.fedoraproject.org/pkg/raw/deadbeaf/f/gating.yaml', status_code=404)
+    # Return 404, because we are only interested in the URL in the request
+    # and whether it is correct even with empty namespace.
+    returned_file = retrieve_yaml_remote_rule(
+        app.config['REMOTE_RULE_POLICIES']['*'].format(
+            rev='deadbeaf', pkg_name='pkg', pkg_namespace=''
         )
+    )
+
+    request_history = [(r.method, r.url) for r in requests_mock.request_history]
+    assert request_history == [(
+        'HEAD', 'https://src.fedoraproject.org/pkg/raw/deadbeaf/f/gating.yaml'
+    )]
+    assert returned_file is None
 
-        assert session.request.mock_calls == [mock.call(
-            'HEAD', 'https://src.fedoraproject.org/pkg/raw/deadbeaf/f/gating.yaml'
-        )]
-        assert returned_file is None
-
-
-def test_retrieve_yaml_remote_rule_connection_error(app):
-    with mock.patch('requests.Session.request') as mocked_request:
-        response = mock.MagicMock()
-        response.status_code = 200
-        mocked_request.side_effect = [
-            response, ConnectionError('Something went terribly wrong...')
-        ]
-
-        with pytest.raises(HTTPError) as excinfo:
-            retrieve_yaml_remote_rule(
-                app.config['REMOTE_RULE_POLICIES']['*'].format(
-                    rev='deadbeaf', pkg_name='pkg', pkg_namespace=''
-                )
-            )
 
-        assert str(excinfo.value) == (
-            '502 Server Error: Something went terribly wrong... for url: '
-            'https://src.fedoraproject.org/pkg/raw/deadbeaf/f/gating.yaml'
+def test_retrieve_yaml_remote_rule_connection_error(app, requests_mock):
+    requests_mock.head('https://src.fedoraproject.org/pkg/raw/deadbeaf/f/gating.yaml')
+    exc = ConnectionError('Something went terribly wrong...')
+    requests_mock.get('https://src.fedoraproject.org/pkg/raw/deadbeaf/f/gating.yaml', exc=exc)
+
+    expected_error = re.escape(
+        'Got unexpected status code 502 for'
+        ' https://src.fedoraproject.org/pkg/raw/deadbeaf/f/gating.yaml:'
+        ' {"message": "Something went terribly wrong..."}'
+    )
+    with pytest.raises(BadGateway, match=expected_error):
+        retrieve_yaml_remote_rule(
+            app.config['REMOTE_RULE_POLICIES']['*'].format(
+                rev='deadbeaf', pkg_name='pkg', pkg_namespace=''
+            )
         )
 
 

pyproject.toml

@@ -38,6 +38,7 @@ flake8 = {version = "^5.0.4", optional = true}
 pytest = {version = "^7.4.4", optional = true}
 pytest-cov = {version = "^4.1.0", optional = true}
 mock = {version = "^5.1.0", optional = true}
+requests-mock = {version = "^1.11.0", optional = true}
 
 SQLAlchemy = {version = "^2.0.24", optional = true}
 psycopg2-binary = {version = "^2.9.9", optional = true}