From f06c3e73254ffa5389079052fb45f1998a6ac182 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Mon, 8 Apr 2024 11:25:48 -0300 Subject: [PATCH] Check for backsync updates before triggering connected webhooks (#4149) Avoid triggering a webhook if it is from a connected integration and the triggering event was caused by a backsync update. Related to https://github.com/grafana/oncall-private/issues/2615 --- engine/apps/webhooks/listeners.py | 16 ++++++++++++-- .../apps/webhooks/tasks/alert_group_status.py | 19 ++++++++++++---- engine/apps/webhooks/tasks/trigger_webhook.py | 5 ++++- .../tests/test_alert_group_status_change.py | 22 +++++++++++++------ .../webhooks/tests/test_trigger_webhook.py | 17 +++++++++++++- 5 files changed, 64 insertions(+), 15 deletions(-) diff --git a/engine/apps/webhooks/listeners.py b/engine/apps/webhooks/listeners.py index ebbed55b..b74ba258 100644 --- a/engine/apps/webhooks/listeners.py +++ b/engine/apps/webhooks/listeners.py @@ -7,10 +7,17 @@ logger.setLevel(logging.DEBUG) def on_alert_group_created(**kwargs): - alert_group_created.apply_async((kwargs["alert_group"].id,)) + alert_group = kwargs["alert_group"] + + # if we have an external_id, this alert_group was just created from a backsync update + external_id = alert_group.external_ids.filter(source_alert_receive_channel=alert_group.channel).first() + is_backsync = external_id is not None + + alert_group_created.apply_async((kwargs["alert_group"].id,), kwargs={"is_backsync": is_backsync}) def on_action_triggered(**kwargs): + from apps.alerts.constants import ActionSource from apps.alerts.models import AlertGroupLogRecord log_record = kwargs["log_record"] @@ -20,4 +27,9 @@ def on_action_triggered(**kwargs): except AlertGroupLogRecord.DoesNotExist as e: logger.warning(f"Webhook action triggered: log record {log_record} never created or has been deleted") raise e - alert_group_status_change.apply_async((log_record.type, log_record.alert_group_id, log_record.author_id)) + + # keep track if this status change was triggered by a backsync event + is_backsync = log_record.action_source == ActionSource.BACKSYNC + alert_group_status_change.apply_async( + (log_record.type, log_record.alert_group_id, log_record.author_id), kwargs={"is_backsync": is_backsync} + ) diff --git a/engine/apps/webhooks/tasks/alert_group_status.py b/engine/apps/webhooks/tasks/alert_group_status.py index 8a528410..fec7b046 100644 --- a/engine/apps/webhooks/tasks/alert_group_status.py +++ b/engine/apps/webhooks/tasks/alert_group_status.py @@ -28,7 +28,7 @@ ACTION_TO_TRIGGER_TYPE = { @shared_dedicated_queue_retry_task( bind=True, autoretry_for=(Exception,), retry_backoff=True, max_retries=1 if settings.DEBUG else MAX_RETRIES ) -def alert_group_created(self, alert_group_id): +def alert_group_created(self, alert_group_id, is_backsync=False): try: alert_group = AlertGroup.objects.get(pk=alert_group_id) except AlertGroup.DoesNotExist: @@ -38,17 +38,24 @@ def alert_group_created(self, alert_group_id): organization_id = alert_group.channel.organization_id webhooks = Webhook.objects.filter(trigger_type=trigger_type, organization_id=organization_id) + if is_backsync: + # only consider non-connected integration webhooks for backsync events + webhooks = webhooks.filter(is_from_connected_integration=False) + # check if there are any webhooks before going on if not webhooks: return - send_webhook_event.apply_async((trigger_type, alert_group_id), kwargs={"organization_id": organization_id}) + send_webhook_event.apply_async( + (trigger_type, alert_group_id), + kwargs={"organization_id": organization_id, "is_backsync": is_backsync}, + ) @shared_dedicated_queue_retry_task( bind=True, autoretry_for=(Exception,), retry_backoff=True, max_retries=1 if settings.DEBUG else MAX_RETRIES ) -def alert_group_status_change(self, action_type, alert_group_id, user_id): +def alert_group_status_change(self, action_type, alert_group_id, user_id, is_backsync=False): try: alert_group = AlertGroup.objects.get(pk=alert_group_id) except AlertGroup.DoesNotExist: @@ -63,11 +70,15 @@ def alert_group_status_change(self, action_type, alert_group_id, user_id): trigger_type=trigger_type, organization_id=organization_id ) | Webhook.objects.filter(trigger_type=Webhook.TRIGGER_STATUS_CHANGE, organization_id=organization_id) + if is_backsync: + # only consider non-connected integration webhooks for backsync events + webhooks = webhooks.filter(is_from_connected_integration=False) + # check if there are any webhooks before going on if not webhooks: return send_webhook_event.apply_async( (trigger_type, alert_group_id), - kwargs={"organization_id": organization_id, "user_id": user_id}, + kwargs={"organization_id": organization_id, "user_id": user_id, "is_backsync": is_backsync}, ) diff --git a/engine/apps/webhooks/tasks/trigger_webhook.py b/engine/apps/webhooks/tasks/trigger_webhook.py index 99da8047..f8322f9d 100644 --- a/engine/apps/webhooks/tasks/trigger_webhook.py +++ b/engine/apps/webhooks/tasks/trigger_webhook.py @@ -62,7 +62,7 @@ class WebhookRequestStatus(typing.TypedDict): @shared_dedicated_queue_retry_task( autoretry_for=(Exception,), retry_backoff=True, max_retries=1 if settings.DEBUG else None ) -def send_webhook_event(trigger_type, alert_group_id, organization_id=None, user_id=None): +def send_webhook_event(trigger_type, alert_group_id, organization_id=None, user_id=None, is_backsync=False): from apps.webhooks.models import Webhook webhooks_qs = Webhook.objects.filter( @@ -76,6 +76,9 @@ def send_webhook_event(trigger_type, alert_group_id, organization_id=None, user_ organization_id=organization_id, ).exclude(is_webhook_enabled=False) + if is_backsync: + webhooks_qs = webhooks_qs.filter(is_from_connected_integration=False) + for webhook in webhooks_qs: execute_webhook.apply_async((webhook.pk, alert_group_id, user_id, None), kwargs={"trigger_type": trigger_type}) diff --git a/engine/apps/webhooks/tests/test_alert_group_status_change.py b/engine/apps/webhooks/tests/test_alert_group_status_change.py index 9f101579..3160be92 100644 --- a/engine/apps/webhooks/tests/test_alert_group_status_change.py +++ b/engine/apps/webhooks/tests/test_alert_group_status_change.py @@ -9,7 +9,10 @@ from apps.webhooks.tasks import alert_group_created, alert_group_status_change @pytest.mark.django_db -def test_alert_group_created(make_organization, make_alert_receive_channel, make_alert_group, make_custom_webhook): +@pytest.mark.parametrize("is_backsync", [True, False, None]) +def test_alert_group_created( + make_organization, make_alert_receive_channel, make_alert_group, make_custom_webhook, is_backsync +): organization = make_organization() alert_receive_channel = make_alert_receive_channel(organization) alert_group = make_alert_group(alert_receive_channel) @@ -17,12 +20,13 @@ def test_alert_group_created(make_organization, make_alert_receive_channel, make make_custom_webhook(organization=organization, trigger_type=Webhook.TRIGGER_ALERT_GROUP_CREATED) with patch("apps.webhooks.tasks.trigger_webhook.send_webhook_event.apply_async") as mock_send_event: - alert_group_created(alert_group.pk) + kwargs = {} if is_backsync is None else {"is_backsync": is_backsync} + alert_group_created(alert_group.pk, **kwargs) assert mock_send_event.called assert mock_send_event.call_args == call( (Webhook.TRIGGER_ALERT_GROUP_CREATED, alert_group.pk), - kwargs={"organization_id": organization.pk}, + kwargs={"organization_id": organization.pk, "is_backsync": bool(is_backsync)}, ) @@ -43,7 +47,7 @@ def test_alert_group_created_for_team( assert mock_send_event.called assert mock_send_event.call_args == call( (Webhook.TRIGGER_ALERT_GROUP_CREATED, alert_group.pk), - kwargs={"organization_id": organization.pk}, + kwargs={"organization_id": organization.pk, "is_backsync": False}, ) @@ -72,6 +76,7 @@ def test_alert_group_created_does_not_exist(make_organization, make_custom_webho (AlertGroupLogRecord.TYPE_UN_ACK, Webhook.TRIGGER_UNACKNOWLEDGE), ], ) +@pytest.mark.parametrize("is_backsync", [True, False, None]) def test_alert_group_status_change( make_organization, make_user_for_organization, @@ -80,6 +85,7 @@ def test_alert_group_status_change( make_custom_webhook, action_type, webhook_type, + is_backsync, ): organization = make_organization() user = make_user_for_organization(organization) @@ -89,10 +95,12 @@ def test_alert_group_status_change( make_custom_webhook(organization=organization, trigger_type=webhook_type) with patch("apps.webhooks.tasks.trigger_webhook.send_webhook_event.apply_async") as mock_send_event: - alert_group_status_change(action_type, alert_group.pk, user.pk) + kwargs = {} if is_backsync is None else {"is_backsync": is_backsync} + alert_group_status_change(action_type, alert_group.pk, user.pk, **kwargs) assert mock_send_event.call_args == call( - (webhook_type, alert_group.pk), kwargs={"organization_id": organization.pk, "user_id": user.pk} + (webhook_type, alert_group.pk), + kwargs={"organization_id": organization.pk, "user_id": user.pk, "is_backsync": bool(is_backsync)}, ) @@ -125,5 +133,5 @@ def test_alert_group_status_change_for_team( assert mock_send_event.call_args == call( (Webhook.TRIGGER_RESOLVE, alert_group.pk), - kwargs={"organization_id": organization.pk, "user_id": None}, + kwargs={"organization_id": organization.pk, "user_id": None, "is_backsync": False}, ) diff --git a/engine/apps/webhooks/tests/test_trigger_webhook.py b/engine/apps/webhooks/tests/test_trigger_webhook.py index b2413f1f..200d7ce5 100644 --- a/engine/apps/webhooks/tests/test_trigger_webhook.py +++ b/engine/apps/webhooks/tests/test_trigger_webhook.py @@ -42,7 +42,10 @@ def test_send_webhook_event_filters( webhooks = {} for trigger_type in trigger_types: webhooks[trigger_type] = make_custom_webhook( - organization=organization, trigger_type=trigger_type, team=make_team(organization) + organization=organization, + trigger_type=trigger_type, + team=make_team(organization), + is_from_connected_integration=(trigger_type != Webhook.TRIGGER_ACKNOWLEDGE), ) for trigger_type in trigger_types: @@ -52,6 +55,18 @@ def test_send_webhook_event_filters( (webhooks[trigger_type].pk, alert_group.pk, None, None), kwargs={"trigger_type": trigger_type} ) + # backsync event exclude connected integration webhooks + for trigger_type in trigger_types: + with patch("apps.webhooks.tasks.trigger_webhook.execute_webhook.apply_async") as mock_execute: + send_webhook_event(trigger_type, alert_group.pk, organization_id=organization.pk, is_backsync=True) + if trigger_type == Webhook.TRIGGER_ACKNOWLEDGE: + assert mock_execute.call_args == call( + (webhooks[trigger_type].pk, alert_group.pk, None, None), kwargs={"trigger_type": trigger_type} + ) + else: + # except for the acknowledge webhook (not connected integration set), the webhook is not triggered + mock_execute.assert_not_called() + # other org other_org_webhook = make_custom_webhook( organization=other_organization, trigger_type=Webhook.TRIGGER_ALERT_GROUP_CREATED