Improve error reporting for operator-courier related errors

docs/usage/v1.md

@@ -43,15 +43,24 @@ Error messages have following format:
   "error": "<error ID string>",
   "message": "<detailed error description>",
 }
+
+For the following errors, the response will contain extra fields:
+  PackageValidationError:
+    "validation_info": <json with validation warnings and errors, empty if unavailable>
+
+  QuayAuthorizationError, QuayCourierError:
+    "quay_response": <json response from quay.io, empty if unavailable>
 ```
 
 
 | HTTP Code / `status` |  `error`    |  Explanation        |
 |-----------|------------------------|---------------------|
 |400| OMPSUploadedFileError | Uploaded file didn't meet expectations (not a zip file, too big after unzip, corrupted zip file) |
 |400| OMPSExpectedFileError | Expected file hasn't been attached |
+|400| PackageValidationError | Package failed validation (e.g. invalid yaml, missing required fields...) |
 |400| OMPSInvalidVersionFormat | Invalid version format in URL |
 |403| OMPSAuthorizationHeaderRequired| No `Authorization` header found in request|
+|403| QuayAuthorizationError | Unauthorized access to quay |
 |500| QuayCourierError | operator-courier module raised exception during building and pushing manifests to quay|
 |500| QuayPackageError | Failed to get information about application packages from quay |
 

docs/usage/v2.md

@@ -46,15 +46,24 @@ Error messages have following format:
   "error": "<error ID string>",
   "message": "<detailed error description>",
 }
+
+For the following errors, the response will contain extra fields:
+  PackageValidationError:
+    "validation_info": <json with validation warnings and errors, empty if unavailable>
+
+  QuayAuthorizationError, QuayCourierError:
+    "quay_response": <json response from quay.io, empty if unavailable>
 ```
 
 
 | HTTP Code / `status` |  `error`    |  Explanation        |
 |-----------|------------------------|---------------------|
 |400| OMPSUploadedFileError | Uploaded file didn't meet expectations (not a zip file, too big after unzip, corrupted zip file) |
 |400| OMPSExpectedFileError | Expected file hasn't been attached |
+|400| PackageValidationError | Package failed validation (e.g. invalid yaml, missing required fields...) |
 |400| OMPSInvalidVersionFormat | Invalid version format in URL |
 |403| OMPSAuthorizationHeaderRequired| No `Authorization` header found in request|
+|403| QuayAuthorizationError | Unauthorized access to quay |
 |500| QuayCourierError | operator-courier module raised exception during building and pushing manifests to quay|
 |500| QuayPackageError | Failed to get information about application packages from quay |
 

omps/errors.py

@@ -7,12 +7,26 @@
 
 from flask import jsonify
 from werkzeug.exceptions import HTTPException
+from operatorcourier.errors import (
+    OpCourierBadYaml,
+    OpCourierBadArtifact,
+    OpCourierBadBundle,
+    OpCourierQuayErrorResponse
+)
 
 
 class OMPSError(Exception):
     """Base OMPSError"""
     code = 500
 
+    def to_dict(self):
+        """Turn the exception into a dict for use as an error response"""
+        return {
+            'status': self.code,
+            'error': self.__class__.__name__,
+            'message': str(self)
+        }
+
 
 class OMPSUploadedFileError(OMPSError):
     """Uploaded file doesn't meet expectations"""
@@ -28,6 +42,56 @@ class QuayCourierError(OMPSError):
     """Operator-courier library failures"""
     code = 500
 
+    def __init__(self, msg, quay_response=None):
+        """
+        :param msg: the message this exception should have
+        :param quay_response: Quay error response json, if available
+        """
+        super().__init__(msg)
+        self.quay_response = quay_response or {}
+
+    def to_dict(self):
+        data = super().to_dict()
+        data['quay_response'] = self.quay_response
+        return data
+
+
+class PackageValidationError(OMPSError):
+    """Package failed validation"""
+    code = 400
+
+    def __init__(self, msg, validation_info=None):
+        """
+        :param msg: the message this exception should have
+        :param validation_info: errors and warnings found during validation,
+                                if available
+        """
+        super().__init__(msg)
+        self.validation_info = validation_info or {}
+
+    def to_dict(self):
+        data = super().to_dict()
+        data['validation_info'] = self.validation_info
+        return data
+
+
+class QuayAuthorizationError(OMPSError):
+    """Unauthorized access to Quay"""
+    code = 403
+
+    def __init__(self, msg, quay_response):
+        """
+        :param msg: the message this exception should have
+        :param quay_response: Quay error response json
+        """
+        super().__init__(msg)
+        self.quay_response = quay_response
+
+    def to_dict(self):
+        data = super().to_dict()
+        data['quay_response'] = self.quay_response
+        return data
+
 
 class QuayPackageError(OMPSError):
     """Error during getting package information from quay"""
@@ -70,6 +134,28 @@ class KojiError(OMPSError):
     code = 500
 
 
+def raise_for_courier_exception(e, new_msg=None):
+    """React to operator-courier errors by raising the proper OMPS error
+
+    :param e: the operator-courier error
+    :param new_msg: message for the OMPS error (if None, the original error
+                    message will be used)
+    """
+    msg = new_msg if new_msg is not None else str(e)
+
+    if isinstance(e, OpCourierBadBundle):
+        raise PackageValidationError(msg, e.validation_info)
+    elif isinstance(e, (OpCourierBadYaml, OpCourierBadArtifact)):
+        raise PackageValidationError(msg)
+    elif isinstance(e, OpCourierQuayErrorResponse):
+        if e.code == 403:
+            raise QuayAuthorizationError(msg, e.error_response)
+        else:
+            raise QuayCourierError(msg, e.error_response)
+    else:
+        raise QuayCourierError(msg)
+
+
 def json_error(status, error, message):
     response = jsonify(
         {'status': status,
@@ -85,7 +171,9 @@ def init_errors_handling(app):
     @app.errorhandler(OMPSError)
     def omps_errors(e):
         """Handle OMPS application errors"""
-        return json_error(e.code, e.__class__.__name__, str(e))
+        response = jsonify(e.to_dict())
+        response.status_code = e.code
+        return response
 
     @app.errorhandler(HTTPException)
     def standard_http_errors(e):

omps/manifests_util.py

@@ -11,7 +11,7 @@
 import yaml
 
 from operatorcourier.api import build_and_verify
-from omps.errors import QuayCourierError
+from omps.errors import raise_for_courier_exception
 
 logger = logging.getLogger(__name__)
 
@@ -31,7 +31,7 @@ def from_dir(cls, source_dir_path):
             bundle = build_and_verify(source_dir_path)
         except Exception as e:
             msg = "Operator courier failed: {}".format(e)
-            raise QuayCourierError(msg)
+            raise_for_courier_exception(e, new_msg=msg)
         return cls(bundle)
 
     def __init__(self, bundle):

omps/quay.py

@@ -14,9 +14,9 @@
 from operatorcourier import api as courier_api
 
 from .errors import (
-    QuayCourierError,
     QuayPackageNotFound,
     QuayPackageError,
+    raise_for_courier_exception
 )
 
 logger = logging.getLogger(__name__)
@@ -221,10 +221,7 @@ def push_operator_manifest(self, repo, version, source_dir):
                 source_dir=source_dir
             )
         except Exception as e:
-            self.logger.error(
-                "push_operator_manifest: Operator courier call failed: %s", e
-            )
-            raise QuayCourierError("Failed to push manifest: {}".format(e))
+            self._handle_courier_exception(e)
         else:
             if not self.public:
                 self.logger.info(
@@ -239,6 +236,13 @@ def push_operator_manifest(self, repo, version, source_dir):
                 return
             self.publish_repo(repo)
 
+    def _handle_courier_exception(self, e):
+        self.logger.error(
+            "push_operator_manifest: Operator courier call failed: %s", e
+        )
+        msg = "Failed to push manifest: {}".format(e)
+        raise_for_courier_exception(e, new_msg=msg)
+
     def _get_repo_content(self, repo):
         """Return content of repository"""
         endpoint = '/cnr/api/v1/packages/'

setup.py

@@ -62,7 +62,7 @@ def get_project_version(version_file='omps/__init__.py'):
         'koji',
         'PyYAML',
         'requests',
-        'operator-courier',
+        'operator-courier>=1.2.1',
     ],
     extras_require={
         'test': [

tests/conftest.py

@@ -127,6 +127,33 @@ def mocked_op_courier_push():
         operatorcourier.api.build_verify_and_push = orig
 
 
+@pytest.fixture
+def op_courier_push_raising():
+    """Use as a context manager to make courier raise a specific exception
+    from build_verify_and_push()
+
+    e.g.:
+        with op_courier_push_raising(OpCourierBadBundle(*exc_args)):
+            quay_org.push_operator_manifest(*push_args)
+    """
+    class CourierPushCM:
+        def __init__(self, exception):
+            self.e = exception
+            self.original_api = operatorcourier.api
+            self.mocked_api = flexmock(self.original_api)
+
+        def __enter__(self):
+            (self.mocked_api
+                .should_receive('build_verify_and_push')
+                .and_raise(self.e))
+            operatorcourier.api = self.mocked_api
+
+        def __exit__(self, *args):
+            operatorcourier.api = self.original_api
+
+    return CourierPushCM
+
+
 @pytest.fixture
 def mocked_koji_archive_download(valid_manifests_archive):
     """Mock KojiUtil.koji_download_manifest_archive to return valid archive"""

tests/test_manifest_util.py

@@ -7,7 +7,7 @@
 
 import pytest
 
-from omps.errors import QuayCourierError
+from omps.errors import PackageValidationError
 from omps.manifests_util import ManifestBundle
 
 
@@ -18,10 +18,12 @@ def test_invalid_bundle(self):
         """Test if proper exception is raised when source data are invalid.
         Uses empty dir to force operator-courier to raise an exception
         """
-        with pytest.raises(QuayCourierError), \
+        with pytest.raises(PackageValidationError) as exc_info, \
                 tempfile.TemporaryDirectory() as tmpdir:
             ManifestBundle.from_dir(tmpdir)
 
+        assert str(exc_info.value).startswith('Operator courier failed: ')
+
     def test_package_name(self, valid_manifest_dir):
         """Test of property which provides package name"""
         mb = ManifestBundle.from_dir(valid_manifest_dir.path)