Skip to content

Commit

Permalink
apps.webhooks.tasks.trigger_webhook.execute_webhook task - don't re…
Browse files Browse the repository at this point in the history
…try on `requests.exceptions.SSLError` (#4796)

# Which issue(s) this PR closes

Address retrying `apps.webhooks.tasks.trigger_webhook.execute_webhook`
task when `requests.exceptions.SSLError` is raised
([logs](https://ops.grafana-ops.net/goto/vqrouqrIR?orgId=1)). Don't
retry the task on these exceptions as retrying will not help. From the
[`request`'s
docs](https://requests.readthedocs.io/en/latest/user/advanced/#ssl-cert-verification):
> By default, SSL verification is enabled, and Requests will throw a
SSLError if it’s unable to verify the certificate

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] Added the relevant release notes label (see labels prefixed w/
`release:`). These labels dictate how your PR will
    show up in the autogenerated release notes.
  • Loading branch information
joeyorlando authored Aug 9, 2024
1 parent 92ac1d8 commit 6eac05a
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 0 deletions.
7 changes: 7 additions & 0 deletions engine/apps/webhooks/tasks/trigger_webhook.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,13 @@ def make_request(
status["request_headers"] = error = e.message
except InvalidWebhookData as e:
status["request_data"] = error = e.message
except requests.exceptions.SSLError as e:
# Don't raise an exception for SSL errors, as they are out of our control and retrying
# isn't going to help. Just show the error to the user and give up
#
# from the docs (https://requests.readthedocs.io/en/latest/user/advanced/#ssl-cert-verification)
# "Requests will throw a SSLError if it’s unable to verify the certificate"
status["content"] = error = str(e)
except Exception as e:
status["content"] = error = str(e)
exception = e
Expand Down
50 changes: 50 additions & 0 deletions engine/apps/webhooks/tests/test_trigger_webhook.py
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,56 @@ def test_execute_webhook_errors(
)


@patch(
"apps.webhooks.models.webhook.WebhookSession.request",
side_effect=requests.exceptions.SSLError("SSL error - foo bar"),
)
@patch("apps.webhooks.utils.socket.gethostbyname", return_value="8.8.8.8") # make it a valid URL when resolving name
@pytest.mark.django_db
def test_execute_webhook_ssl_error(
_mock_socket_gethostbyname,
mock_request,
make_organization,
make_alert_receive_channel,
make_alert_group,
make_custom_webhook,
):
url = "https://something.cool/"
expected_error = "SSL error - foo bar"

organization = make_organization()
alert_receive_channel = make_alert_receive_channel(organization)
alert_group = make_alert_group(alert_receive_channel, resolved_at=timezone.now(), resolved=True)
webhook = make_custom_webhook(
organization=organization,
http_method="POST",
trigger_type=Webhook.TRIGGER_RESOLVE,
forward_all=False,
url=url,
)

execute_webhook(webhook.pk, alert_group.pk, None, None)

mock_request.assert_has_calls([call("POST", url, timeout=4, headers={})])

log = webhook.responses.all()[0]
assert log.status_code is None
assert log.content == expected_error

# check log record
log_record = alert_group.log_records.last()
assert log_record.type == AlertGroupLogRecord.ERROR_ESCALATION_TRIGGER_CUSTOM_WEBHOOK_ERROR
assert log_record.step_specific_info == {
"trigger": "resolve",
"webhook_id": webhook.public_primary_key,
"webhook_name": webhook.name,
}
assert log_record.reason == expected_error
assert (
log_record.rendered_log_line_action() == f"skipped resolve outgoing webhook `{webhook.name}`: {expected_error}"
)


@pytest.mark.django_db
def test_response_content_limit(
make_organization, make_user_for_organization, make_alert_receive_channel, make_alert_group, make_custom_webhook
Expand Down

0 comments on commit 6eac05a

Please sign in to comment.