[Fix] Send background VPN errors and recovery as generic_message notifications #1049

Modified tasks and tasks zerotier consistently with utils. Tested via manual testing.

Fixes #1049

Author: stktyagi

openwisp_controller/config/tasks.py

@@ -9,6 +9,8 @@
 
 from openwisp_utils.tasks import OpenwispCeleryTask
 
+from .utils import handle_error_notification, handle_recovery_notification
+
 logger = logging.getLogger(__name__)
 
 
@@ -130,18 +132,32 @@ def trigger_vpn_server_endpoint(endpoint, auth_token, vpn_id):
 
     # Cache the configuration here makes downloading the configuration faster.
     vpn.get_cached_configuration()
-    response = requests.post(
-        endpoint,
-        params={"key": auth_token},
-        verify=False if getattr(settings, "DEBUG") else True,
-    )
-    if response.status_code == 200:
-        logger.info(f"Triggered update webhook of VPN Server UUID: {vpn_id}")
+    task_key = f"vpn_update_task:{vpn_id}"
+    try:
+        response = requests.post(
+            endpoint,
+            params={"key": auth_token},
+            verify=False if getattr(settings, "DEBUG") else True,
+        )
+        response.raise_for_status()
+    except requests.RequestException as e:
+        logger.warning(
+            f"Failed to update VPN Server configuration. "
+            f"Error: {str(e)}, "
+            f"VPN Server UUID: {vpn_id}"
+        )
+        handle_error_notification(
+            task_key,
+            exception=e,
+            instance=vpn,
+            action="update",
+        )
     else:
-        logger.error(
-            "Failed to update VPN Server configuration. "
-            f"Response status code: {response.status_code}, "
-            f"VPN Server UUID: {vpn_id}",
+        logger.info(f"Triggered update webhook of VPN Server UUID: {vpn_id}")
+        handle_recovery_notification(
+            task_key,
+            instance=vpn,
+            action="update",
         )
 
 

openwisp_controller/config/tasks_zerotier.py

@@ -14,6 +14,7 @@
 from openwisp_utils.tasks import OpenwispCeleryTask
 
 from .settings import API_TASK_RETRY_OPTIONS
+from .utils import handle_error_notification, handle_recovery_notification
 
 logger = logging.getLogger(__name__)
 
@@ -27,48 +28,6 @@ class OpenwispApiTask(OpenwispCeleryTask):
         HTTPStatus.GATEWAY_TIMEOUT,  # 504
     ]
 
-    def _send_api_task_notification(self, type, **kwargs):
-        vpn = kwargs.get("instance")
-        action = kwargs.get("action").replace("_", " ")
-        status_code = kwargs.get("status_code")
-        # Adding some delay here to prevent overlapping
-        # of the django success message container
-        # with the ow-notification container
-        # https://github.com/openwisp/openwisp-notifications/issues/264
-        sleep(2)
-        message_map = {
-            "error": {
-                "verb": _("encountered an unrecoverable error"),
-                "message": _(
-                    "Unable to perform {action} operation on the "
-                    "{target} VPN server due to an "
-                    "unrecoverable error "
-                    "(status code: {status_code})"
-                ),
-                "level": "error",
-            },
-            "recovery": {
-                "verb": _("has been completed successfully"),
-                "message": _("The {action} operation on {target} {verb}."),
-                "level": "info",
-            },
-        }
-        meta = message_map[type]
-        notify.send(
-            type="generic_message",
-            sender=vpn,
-            target=vpn,
-            action=action,
-            verb=meta["verb"],
-            message=meta["message"].format(
-                action=action,
-                target=str(vpn),
-                status_code=status_code,
-                verb=meta["verb"],
-            ),
-            level=meta["level"],
-        )
-
     def handle_api_call(self, fn, *args, send_notification=True, **kwargs):
         """
         This method handles API calls and their responses
@@ -105,13 +64,10 @@ def handle_api_call(self, fn, *args, send_notification=True, **kwargs):
             response.raise_for_status()
             logger.info(info_msg)
             if send_notification:
-                task_result = cache.get(task_key)
-                if task_result == "error":
-                    self._send_api_task_notification("recovery", **kwargs)
-                    cache.set(task_key, "success", None)
+                handle_recovery_notification(task_key, **kwargs)
         except RequestException as e:
             if response.status_code in self._RECOVERABLE_API_CODES:
-                retry_logger = logger.warn
+                retry_logger = logger.warning
                 # When retry limit is reached, use error logging
                 if self.request.retries == self.max_retries:
                     retry_logger = logger.error
@@ -122,12 +78,7 @@ def handle_api_call(self, fn, *args, send_notification=True, **kwargs):
                 raise e
             logger.error(f"{err_msg}, Error: {e}")
             if send_notification:
-                task_result = cache.get(task_key)
-                if task_result in (None, "success"):
-                    cache.set(task_key, "error", None)
-                    self._send_api_task_notification(
-                        "error", status_code=response.status_code, **kwargs
-                    )
+                handle_error_notification(task_key, exception=e, **kwargs)
         return (response, updated_config) if updated_config else response
 
 

openwisp_controller/config/tests/test_vpn.py

@@ -3,6 +3,7 @@
 from subprocess import CalledProcessError, TimeoutExpired
 from unittest import mock
 
+import requests
 from celery.exceptions import Retry, SoftTimeLimitExceeded
 from django.conf import settings
 from django.core.exceptions import ValidationError
@@ -728,9 +729,12 @@ def test_trigger_vpn_server_endpoint_invalid_vpn_id(self):
 
 
 class TestWireguardTransaction(BaseTestVpn, TestWireguardVpnMixin, TransactionTestCase):
+    mock_response = mock.Mock(spec=requests.Response)
+    mock_response.status_code = 200
+    mock_response.raise_for_status = mock.Mock()
+
     @mock.patch(
-        "openwisp_controller.config.tasks.requests.post",
-        return_value=HttpResponse(status=200),
+        "openwisp_controller.config.tasks.requests.post", return_value=mock_response
     )
     def test_auto_peer_configuration(self, *args):
         self.assertEqual(IpAddress.objects.count(), 0)
@@ -985,9 +989,12 @@ def test_auto_client(self):
 class TestVxlanTransaction(
     BaseTestVpn, TestVxlanWireguardVpnMixin, TransactionTestCase
 ):
+    mock_response = mock.Mock(spec=requests.Response)
+    mock_response.status_code = 200
+    mock_response.raise_for_status = mock.Mock()
+
     @mock.patch(
-        "openwisp_controller.config.tasks.requests.post",
-        return_value=HttpResponse(status=200),
+        "openwisp_controller.config.tasks.requests.post", return_value=mock_response
     )
     def test_auto_peer_configuration(self, *args):
         self.assertEqual(IpAddress.objects.count(), 0)

openwisp_controller/config/utils.py

@@ -1,10 +1,15 @@
 import logging
 
+from django.core.cache import cache
 from django.core.exceptions import ValidationError
 from django.db.models import Q
 from django.http import Http404, HttpResponse
 from django.shortcuts import get_object_or_404 as base_get_object_or_404
 from django.urls import path, re_path
+from django.utils.translation import gettext_lazy as _
+
+# from time import sleep
+from openwisp_notifications.signals import notify
 from openwisp_notifications.utils import _get_object_link
 
 logger = logging.getLogger(__name__)
@@ -206,3 +211,61 @@ def get_default_templates_queryset(
 def get_config_error_notification_target_url(obj, field, absolute_url=True):
     url = _get_object_link(obj._related_object(field), absolute_url)
     return f"{url}#config-group"
+
+
+def send_api_task_notification(type, **kwargs):
+    vpn = kwargs.get("instance")
+    action = kwargs.get("action").replace("_", " ")
+    exception = kwargs.get("exception")
+    # Adding some delay here to prevent overlapping
+    # of the django success message container
+    # with the ow-notification container
+    # https://github.com/openwisp/openwisp-notifications/issues/264
+    # sleep(2)
+    message_map = {
+        "error": {
+            "verb": _("encountered an unrecoverable error"),
+            "message": _(
+                "Unable to perform {action} operation on the "
+                "{target} VPN server due to an "
+                "unrecoverable error "
+                "({error_type})"
+            ),
+            "level": "error",
+        },
+        "success": {
+            "verb": _("has been completed successfully"),
+            "message": _("The {action} operation on {target} {verb}."),
+            "level": "info",
+        },
+    }
+    meta = message_map[type]
+    notify.send(
+        sender=vpn,
+        target=vpn,
+        type="generic_message",
+        action_object=vpn,
+        verb=meta["verb"],
+        message=meta["message"].format(
+            action=action,
+            target=str(vpn),
+            error_type=exception.__class__.__name__ if exception else "",
+            verb=meta["verb"],
+        ),
+        description=str(exception) if exception else "",
+        level=meta["level"],
+    )
+
+
+def handle_recovery_notification(task_key, **kwargs):
+    task_result = cache.get(task_key)
+    if task_result == "error":
+        send_api_task_notification("success", **kwargs)
+    cache.set(task_key, "success", timeout=None)
+
+
+def handle_error_notification(task_key, **kwargs):
+    cached_value = cache.get(task_key)
+    if cached_value != "error":
+        cache.set(task_key, "error", timeout=None)
+        send_api_task_notification("error", **kwargs)