diff --git a/docs/installation/configuration/env_config.md b/docs/installation/configuration/env_config.md index 4f08563f..db0dfa91 100644 --- a/docs/installation/configuration/env_config.md +++ b/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. diff --git a/docs/installation/configuration/opennotifs_config_cli.rst b/docs/installation/configuration/opennotifs_config_cli.rst index f05059d9..d5224915 100644 --- a/docs/installation/configuration/opennotifs_config_cli.rst +++ b/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 ========= diff --git a/src/nrc/api/tasks.py b/src/nrc/api/tasks.py index b633a5ae..681190dc 100644 --- a/src/nrc/api/tasks.py +++ b/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, +) diff --git a/src/nrc/api/tests/test_notificatie.py b/src/nrc/api/tests/test_notificatie.py index 804122a7..d6a66e38 100644 --- a/src/nrc/api/tests/test_notificatie.py +++ b/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) diff --git a/src/nrc/conf/includes/base.py b/src/nrc/conf/includes/base.py index 745788ef..1ced75cc 100644 --- a/src/nrc/conf/includes/base.py +++ b/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 +) diff --git a/src/nrc/config/notification_retry.py b/src/nrc/config/notification_retry.py new file mode 100644 index 00000000..15b3e3de --- /dev/null +++ b/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): ... diff --git a/src/nrc/tests/commands/test_setup_configuration.py b/src/nrc/tests/commands/test_setup_configuration.py index 14489f9a..23421145 100644 --- a/src/nrc/tests/commands/test_setup_configuration.py +++ b/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.", ] diff --git a/src/nrc/tests/config/test_notification_retry_configuration.py b/src/nrc/tests/config/test_notification_retry_configuration.py new file mode 100644 index 00000000..6185bc11 --- /dev/null +++ b/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())