Преглед на файлове

fix(api): overhaul of exception handling

This fixes some faulty behavior, such as
- returning 503 when actually 500 is appropriate and
- counting db-unrelated errors towards the db outage metric.
Nils Wisiol преди 5 години
родител
ревизия
3d30ba248a
променени са 2 файла, в които са добавени 40 реда и са изтрити 23 реда
  1. 39 22
      api/desecapi/exception_handlers.py
  2. 1 1
      api/desecapi/tests/test_domains.py

+ 39 - 22
api/desecapi/exception_handlers.py

@@ -10,7 +10,6 @@ from desecapi import metrics
 from desecapi.exceptions import PDNSException
 
 
-
 def exception_handler(exc, context):
     """
     desecapi specific exception handling. If no special treatment is applied,
@@ -18,39 +17,57 @@ def exception_handler(exc, context):
     https://www.django-rest-framework.org/api-guide/exceptions/#custom-exception-handling
     """
 
-    def _perform_handling(name):
+    def _log():
         logger = logging.getLogger('django.request')
-        logger.error('{} Supplementary Information'.format(name),
+        logger.error('{} Supplementary Information'.format(exc.__class__),
                      exc_info=exc, stack_info=False)
 
-        # Gracefully let clients know that we cannot connect to the database
-        response =  Response({'detail': 'Please try again later.'},
-                        status=status.HTTP_503_SERVICE_UNAVAILABLE)
-        metrics.get('desecapi_database_unavailable').inc()
+    def _500():
+        _log()
+
+        # Let clients know that there is a problem
+        response = Response({'detail': 'Internal Server Error. We\'re on it!'},
+                            status=status.HTTP_500_INTERNAL_SERVER_ERROR)
+        return response
+
+    def _503():
+        _log()
+
+        # Let clients know that there is a temporary problem
+        response = Response({'detail': 'Please try again later.'},
+                            status=status.HTTP_503_SERVICE_UNAVAILABLE)
         return response
 
     # Catch DB exception and log an extra error for additional context
-    if isinstance(exc, OperationalError):
-        if isinstance(exc.args, (list, dict, tuple)) and exc.args and \
-            exc.args[0] in (
-                2002,  # Connection refused (Socket)
-                2003,  # Connection refused (TCP)
-                2005,  # Unresolved host name
-                2007,  # Server protocol mismatch
-                2009,  # Wrong host info
-                2026,  # SSL connection error
-        ):
-            return _perform_handling('OperationalError')
+    if (
+        isinstance(exc, OperationalError) and
+        isinstance(exc.args, (list, dict, tuple)) and
+        exc.args and
+        exc.args[0] in (
+            2002,  # Connection refused (Socket)
+            2003,  # Connection refused (TCP)
+            2005,  # Unresolved host name
+            2007,  # Server protocol mismatch
+            2009,  # Wrong host info
+            2026,  # SSL connection error
+        )
+    ):
+        metrics.get('desecapi_database_unavailable').inc()
+        return _503()
 
     # OSError happens on system-related errors, like full disk or getaddrinfo() failure.
-    # Catch it and log an extra error for additional context.
     if isinstance(exc, OSError):
-        return _perform_handling('OSError')
+        # TODO add metrics
+        return _500()
 
+    # The PSL encountered an unsupported rule
     if isinstance(exc, UnsupportedRule):
-        return _perform_handling('UnsupportedRule')
+        # TODO add metrics
+        return _500()
 
+    # nslord/nsmaster returned an error
     if isinstance(exc, PDNSException):
-        return _perform_handling('PDNSException')
+        # TODO add metrics
+        return _500()
 
     return drf_exception_handler(exc, context)

+ 1 - 1
api/desecapi/tests/test_domains.py

@@ -330,7 +330,7 @@ class DomainOwnerTestCase1(DomainOwnerTestCase):
         psl_cm = self.get_psl_context_manager(UnsupportedRule)
         with psl_cm, self.assertPdnsRequests():
             response = self.client.post(self.reverse('v1:domain-list'), {'name': name})
-            self.assertStatus(response, status.HTTP_503_SERVICE_UNAVAILABLE)
+            self.assertStatus(response, status.HTTP_500_INTERNAL_SERVER_ERROR)
 
     def test_create_domain_policy(self):
         for name in ['1.2.3..4.test.dedyn.io', 'test..de', '*.' + self.random_domain_name(), 'a' * 64 + '.bla.test']: