Merge pull request #140 from open-zaak/feature/1203-setup-config-retry-vars

 ✨ [open-zaak/open-zaak#1203] Setup configuration for retry variables

Author: stevenbal

docs/installation/configuration/env_config.md

@@ -100,16 +100,6 @@ on Docker, since `localhost` is contained within the container:
 
 * `CELERY_RESULT_BACKEND`: the backend where the results of tasks will be stored (default: `redis://localhost:6379/1`)
 
-* `NOTIFICATION_DELIVERY_MAX_RETRIES`: the maximum number of retries Celery will do if sending a notification failed.
-
-* `NOTIFICATION_DELIVERY_RETRY_BACKOFF`: a boolean or a number. If this option is set to
-  `True`, autoretries will be delayed following the rules of exponential backoff. If
-  this option is set to a number, it is used as a delay factor.
-
-* `NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX`: an integer, specifying number of seconds.
-  If ``retry_backoff`` is enabled, this option will set a maximum delay in seconds
-  between task autoretries. By default, this option is set to 48 seconds.
-
 ### Cross-Origin-Resource-Sharing
 
 The following parameters control the CORS policy.
@@ -130,7 +120,7 @@ The following parameters control the CORS policy.
 ### Initial configuration
 
 Open Notificaties supports `setup_configuration` management command, which allows configuration via
-environment variables. 
+environment variables.
 All these environment variables are described at CLI configuration.
 
 

docs/installation/configuration/opennotifs_config_cli.rst

@@ -27,8 +27,8 @@ Preparation
 
 The command executes the list of pluggable configuration steps, and each step
 requires specific environment variables, that should be prepared.
-Here is the description of all available configuration steps and the environment variables, 
-use by each step. 
+Here is the description of all available configuration steps and the environment variables,
+use by each step.
 
 Sites configuration
 -------------------
@@ -65,6 +65,23 @@ Make sure that the correct permissions are configured in Open Zaak Autorisaties
   for example, ``open-zaak``. Required.
 * ``OPENZAAK_NOTIF_SECRET``: some random string. Required.
 
+Notification retry configuration
+--------------------------------
+
+Open Notifications has a retry mechanism to guarantee notification delivery, this mechanism
+is described in :ref:`delivery_guarantees`. The parameters for this behavior can be configured via the
+following environment variables.
+
+* ``NOTIFICATION_DELIVERY_MAX_RETRIES``: the maximum number of retries Celery will do if sending a notification failed.
+* ``NOTIFICATION_DELIVERY_RETRY_BACKOFF``: a boolean or a number. If this option is set to
+  ``True``, autoretries will be delayed following the rules of exponential backoff. If
+  this option is set to a number, it is used as a delay factor.
+* ``NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX``: an integer, specifying number of seconds.
+  If ``retry_backoff`` is enabled, this option will set a maximum delay in seconds
+  between task autoretries. By default, this option is set to 48 seconds.
+
+These settings can also later be changed via the admin interface, under ``Configuration > Notification component configuration``.
+
 Execution
 =========
 

src/nrc/api/tasks.py

@@ -1,12 +1,12 @@
 import json
 import logging
 
-from django.conf import settings
 from django.core.management import call_command
 from django.core.serializers.json import DjangoJSONEncoder
 from django.utils.translation import gettext_lazy as _
 
 import requests
+from notifications_api_common.autoretry import add_autoretry_behaviour
 
 from nrc.celery import app
 from nrc.datamodel.models import Abonnement, NotificatieResponse
@@ -18,15 +18,7 @@ class NotificationException(Exception):
     pass
 
 
-@app.task(
-    autoretry_for=(
-        NotificationException,
-        requests.RequestException,
-    ),
-    max_retries=settings.NOTIFICATION_DELIVERY_MAX_RETRIES,
-    retry_backoff=settings.NOTIFICATION_DELIVERY_RETRY_BACKOFF,
-    retry_backoff_max=settings.NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX,
-)
+@app.task
 def deliver_message(sub_id: int, msg: dict, **kwargs) -> None:
     """
     send msg to subscriber
@@ -81,3 +73,13 @@ def clean_old_notifications() -> None:
     cleans up old "Notificatie" and "NotificatieResponse"
     """
     call_command("clean_old_notifications")
+
+
+add_autoretry_behaviour(
+    deliver_message,
+    autoretry_for=(
+        NotificationException,
+        requests.RequestException,
+    ),
+    retry_jitter=False,
+)

src/nrc/api/tests/test_notificatie.py

@@ -1,14 +1,18 @@
 from unittest.mock import patch
 
-from django.test import override_settings
+from django.test import TestCase, override_settings
 from django.utils.timezone import now
 
+import requests_mock
+from celery.exceptions import Retry
+from notifications_api_common.models import NotificationsConfig
 from rest_framework import status
 from rest_framework.reverse import reverse
 from rest_framework.test import APITestCase
 from vng_api_common.conf.api import BASE_REST_FRAMEWORK
 from vng_api_common.tests import JWTAuthMixin
 
+from nrc.api.tasks import deliver_message
 from nrc.datamodel.models import Notificatie
 from nrc.datamodel.tests.factories import (
     AbonnementFactory,
@@ -142,3 +146,56 @@ def test_notificatie_send_empty_kenmerk_value(self, mock_task):
         mock_task.assert_called_once_with(
             abon.id, msg, notificatie_id=Notificatie.objects.get().id, attempt=1
         )
+
+
+@patch("notifications_api_common.autoretry.get_exponential_backoff_interval")
+@patch("notifications_api_common.autoretry.NotificationsConfig.get_solo")
+@patch("nrc.api.serializers.deliver_message.retry")
+class NotificatieRetryTests(TestCase):
+    def test_notificatie_retry_use_global_config(
+        self, mock_retry, mock_config, mock_get_exponential_backoff
+    ):
+        """
+        Verify that retry variables configured on `NotificationsConfig` override the
+        variables from the settings
+        """
+        mock_config.return_value = NotificationsConfig(
+            notification_delivery_max_retries=4,
+            notification_delivery_retry_backoff=4,
+            notification_delivery_retry_backoff_max=28,
+        )
+        kanaal = KanaalFactory.create(
+            naam="zaken", filters=["bron", "zaaktype", "vertrouwelijkheidaanduiding"]
+        )
+        abon = AbonnementFactory.create(callback_url="https://example.com/callback")
+        filter_group = FilterGroupFactory.create(kanaal=kanaal, abonnement=abon)
+        FilterFactory.create(
+            filter_group=filter_group, key="bron", value="082096752011"
+        )
+        msg = {
+            "kanaal": "zaken",
+            "hoofdObject": "https://ref.tst.vng.cloud/zrc/api/v1/zaken/d7a22",
+            "resource": "status",
+            "resourceUrl": "https://ref.tst.vng.cloud/zrc/api/v1/statussen/d7a22/721c9",
+            "actie": "create",
+            "aanmaakdatum": now(),
+            "kenmerken": {
+                "bron": "082096752011",
+                "zaaktype": "example.com/api/v1/zaaktypen/5aa5c",
+                "vertrouwelijkheidaanduiding": "openbaar",
+            },
+        }
+
+        mock_retry.side_effect = Retry()
+        with requests_mock.Mocker() as m:
+            m.post(abon.callback_url, status_code=404)
+            with self.assertRaises(Retry):
+                deliver_message(abon.id, msg)
+
+        mock_get_exponential_backoff.assert_called_once_with(
+            factor=4,
+            retries=0,
+            maximum=28,
+            full_jitter=False,
+        )
+        self.assertEqual(deliver_message.max_retries, 4)

src/nrc/conf/includes/base.py

@@ -496,13 +496,6 @@
     },
 }
 
-# Retry settings for delivering notifications to subscriptions
-NOTIFICATION_DELIVERY_MAX_RETRIES = config("NOTIFICATION_DELIVERY_MAX_RETRIES", 5)
-NOTIFICATION_DELIVERY_RETRY_BACKOFF = config("NOTIFICATION_DELIVERY_RETRY_BACKOFF", 3)
-NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX = config(
-    "NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX", 48
-)
-
 #
 # DJANGO-ADMIN-INDEX
 #
@@ -565,6 +558,7 @@
     "nrc.config.site.SiteConfigurationStep",
     "nrc.config.authorization.AuthorizationStep",
     "nrc.config.authorization.OpenZaakAuthStep",
+    "nrc.config.notification_retry.NotificationRetryConfigurationStep",
 ]
 
 #
@@ -585,3 +579,16 @@
 OPENZAAK_NOTIF_CONFIG_ENABLE = config("OPENZAAK_NOTIF_CONFIG_ENABLE", default=True)
 OPENZAAK_NOTIF_CLIENT_ID = config("OPENZAAK_NOTIF_CLIENT_ID", "")
 OPENZAAK_NOTIF_SECRET = config("OPENZAAK_NOTIF_SECRET", "")
+
+# setup configuration for Notification retry
+# Retry settings for delivering notifications to subscriptions
+NOTIFICATION_RETRY_CONFIG_ENABLE = config(
+    "NOTIFICATION_RETRY_CONFIG_ENABLE", default=True
+)
+NOTIFICATION_DELIVERY_MAX_RETRIES = config("NOTIFICATION_DELIVERY_MAX_RETRIES", None)
+NOTIFICATION_DELIVERY_RETRY_BACKOFF = config(
+    "NOTIFICATION_DELIVERY_RETRY_BACKOFF", None
+)
+NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX = config(
+    "NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX", None
+)

src/nrc/config/notification_retry.py

@@ -0,0 +1,37 @@
+from django.conf import settings
+
+from django_setup_configuration.configuration import BaseConfigurationStep
+from notifications_api_common.models import NotificationsConfig
+
+
+class NotificationRetryConfigurationStep(BaseConfigurationStep):
+    """
+    Configure the notifications retry behaviour.
+    """
+
+    verbose_name = "Notification retry configuration"
+    required_settings = []
+    optional_settings = [
+        "NOTIFICATION_DELIVERY_MAX_RETRIES",
+        "NOTIFICATION_DELIVERY_RETRY_BACKOFF",
+        "NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX",
+    ]
+    enable_setting = "NOTIFICATION_RETRY_CONFIG_ENABLE"
+
+    def is_configured(self) -> bool:
+        config = NotificationsConfig.get_solo()
+        for setting_name in self.optional_settings:
+            # It is considered configured if one or more fields have non-default values
+            model_field = getattr(NotificationsConfig, setting_name.lower()).field
+            if getattr(config, setting_name.lower()) != model_field.default:
+                return True
+        return False
+
+    def configure(self):
+        config = NotificationsConfig.get_solo()
+        for setting_name in self.optional_settings:
+            if (setting_value := getattr(settings, setting_name)) is not None:
+                setattr(config, setting_name.lower(), setting_value)
+        config.save()
+
+    def test_configuration(self): ...

src/nrc/tests/commands/test_setup_configuration.py

@@ -16,6 +16,7 @@
 from zgw_consumers.test import mock_service_oas_get
 
 from nrc.config.authorization import AuthorizationStep, OpenZaakAuthStep
+from nrc.config.notification_retry import NotificationRetryConfigurationStep
 from nrc.config.site import SiteConfigurationStep
 
 
@@ -29,6 +30,8 @@
     OPENZAAK_NOTIF_SECRET="oz-secret",
 )
 class SetupConfigurationTests(TestCase):
+    maxDiff = None
+
     def setUp(self):
         super().setUp()
 
@@ -60,19 +63,21 @@ def test_setup_configuration(self, m):
             },
         )
 
-        call_command("setup_configuration", stdout=stdout)
+        call_command("setup_configuration", stdout=stdout, no_color=True)
 
         with self.subTest("Command output"):
             command_output = stdout.getvalue().splitlines()
             expected_output = [
                 f"Configuration will be set up with following steps: [{SiteConfigurationStep()}, "
-                f"{AuthorizationStep()}, {OpenZaakAuthStep()}]",
+                f"{AuthorizationStep()}, {OpenZaakAuthStep()}, {NotificationRetryConfigurationStep()}]",
                 f"Configuring {SiteConfigurationStep()}...",
                 f"{SiteConfigurationStep()} is successfully configured",
                 f"Configuring {AuthorizationStep()}...",
                 f"{AuthorizationStep()} is successfully configured",
                 f"Configuring {OpenZaakAuthStep()}...",
                 f"{OpenZaakAuthStep()} is successfully configured",
+                f"Configuring {NotificationRetryConfigurationStep()}...",
+                f"{NotificationRetryConfigurationStep()} is successfully configured",
                 "Instance configuration completed.",
             ]
 

src/nrc/tests/config/test_notification_retry_configuration.py

@@ -0,0 +1,31 @@
+from django.test import TestCase, override_settings
+
+from notifications_api_common.models import NotificationsConfig
+
+from nrc.config.notification_retry import NotificationRetryConfigurationStep
+
+
+@override_settings(
+    NOTIFICATION_DELIVERY_MAX_RETRIES=4,
+    NOTIFICATION_DELIVERY_RETRY_BACKOFF=5,
+    NOTIFICATION_DELIVERY_RETRY_BACKOFF_MAX=6,
+)
+class NotificationRetryConfigurationTests(TestCase):
+    def test_configure(self):
+        configuration = NotificationRetryConfigurationStep()
+        configuration.configure()
+
+        config = NotificationsConfig.get_solo()
+
+        self.assertEqual(config.notification_delivery_max_retries, 4)
+        self.assertEqual(config.notification_delivery_retry_backoff, 5)
+        self.assertEqual(config.notification_delivery_retry_backoff_max, 6)
+
+    def test_is_configured(self):
+        configuration = NotificationRetryConfigurationStep()
+
+        self.assertFalse(configuration.is_configured())
+
+        configuration.configure()
+
+        self.assertTrue(configuration.is_configured())