From cab17e72537b4188a2df4e2bedc6afa92d4f8810 Mon Sep 17 00:00:00 2001
From: David Douard <david.douard@sdfa3.org>
Date: Fri, 5 Aug 2022 19:16:21 +0200
Subject: [PATCH] Instanciate the SMTP class only when needed
instead of creating it in the VaultBackend constructor: when configured
(with host and port), SMTP.connect() is immediately called, which makes
it mandatory to have the smtp server up and running to be able to create
the VaultBackend object (which makes it hard to run properly in an elastic
environment like docker or k8s).
This also removes the fallback to hardcoded 'localhost:25' smtp server;
if the smtp server is not configured or not reachable, the call to
_smtp_send() will fail logging the failure (both in logging and sentry).
---
swh/vault/backend.py | 29 ++++++++++----
swh/vault/tests/test_backend.py | 69 +++++++++++++++++++++++++++++++--
2 files changed, 87 insertions(+), 11 deletions(-)
diff --git a/swh/vault/backend.py b/swh/vault/backend.py
index b7d9bf3..bc51160 100644
--- a/swh/vault/backend.py
+++ b/swh/vault/backend.py
@@ -5,11 +5,13 @@
import collections
from email.mime.text import MIMEText
+import logging
import smtplib
from typing import Any, Dict, List, Optional, Tuple
import psycopg2.extras
import psycopg2.pool
+import sentry_sdk
from swh.core.db import BaseDb
from swh.core.db.common import db_transaction
@@ -21,6 +23,7 @@ from swh.vault.cache import VaultCache
from swh.vault.cookers import COOKER_TYPES, get_cooker_cls
from swh.vault.exc import NotFoundExc
+logger = logging.getLogger(__name__)
cooking_task_name = "swh.vault.cooking_tasks.SWHCookingTask"
NOTIF_EMAIL_FROM = '"Software Heritage Vault" ' "<bot@softwareheritage.org>"
@@ -75,7 +78,6 @@ class VaultBackend:
self.cache = VaultCache(**config["cache"])
self.scheduler = get_scheduler(**config["scheduler"])
self.storage = get_storage(**config["storage"])
- self.smtp_server = smtplib.SMTP(**config.get("smtp", {}))
if "db" not in self.config:
raise ValueError(
@@ -487,16 +489,29 @@ class VaultBackend:
)
def _smtp_send(self, msg: MIMEText):
- # Reconnect if needed
+ smtp_server = smtplib.SMTP(**self.config.get("smtp", {}))
try:
- status = self.smtp_server.noop()[0]
+ status = smtp_server.noop()[0]
except smtplib.SMTPException:
status = -1
if status != 250:
- self.smtp_server.connect("localhost", 25)
-
- # Send the message
- self.smtp_server.send_message(msg)
+ error_message = (
+ f"Unable to send SMTP message '{msg['Subject']}' to "
+ f"{msg['To']}: cannot connect to server"
+ )
+ logger.error(error_message)
+ sentry_sdk.capture_message(error_message, "error")
+ else:
+ try:
+ # Send the message
+ smtp_server.send_message(msg)
+ except smtplib.SMTPException as exc:
+ logger.exception(exc)
+ error_message = (
+ f"Unable to send SMTP message '{msg['Subject']}' to "
+ f"{msg['To']}: {exc}"
+ )
+ sentry_sdk.capture_message(error_message, "error")
@db_transaction()
def _cache_expire(self, cond, *args, db=None, cur=None) -> None:
diff --git a/swh/vault/tests/test_backend.py b/swh/vault/tests/test_backend.py
index 98c5025..b06b435 100644
--- a/swh/vault/tests/test_backend.py
+++ b/swh/vault/tests/test_backend.py
@@ -5,12 +5,15 @@
import contextlib
import datetime
+import re
+import smtplib
from unittest.mock import MagicMock, patch
import attr
import psycopg2
import pytest
+from swh.core.sentry import init_sentry
from swh.model.model import Content
from swh.model.swhids import CoreSWHID
from swh.vault.exc import NotFoundExc
@@ -212,10 +215,10 @@ def test_send_all_emails(swh_vault):
swh_vault.set_status(TEST_TYPE, TEST_SWHID, "done")
- with patch.object(swh_vault, "smtp_server") as m:
+ with patch.object(swh_vault, "_smtp_send") as m:
swh_vault.send_notif(TEST_TYPE, TEST_SWHID)
- sent_emails = {k[0][0] for k in m.send_message.call_args_list}
+ sent_emails = {k[0][0] for k in m.call_args_list}
assert {k["To"] for k in sent_emails} == set(emails)
for e in sent_emails:
@@ -234,6 +237,64 @@ def test_send_all_emails(swh_vault):
m.assert_not_called()
+def test_send_email_error_no_smtp(swh_vault):
+ reports = []
+ init_sentry("http://example.org", extra_kwargs={"transport": reports.append})
+
+ emails = ("a@example.com", "billg@example.com", "test+42@example.org")
+ with mock_cooking(swh_vault):
+ for email in emails:
+ swh_vault.cook(TEST_TYPE, TEST_SWHID, email=email)
+ swh_vault.set_status(TEST_TYPE, TEST_SWHID, "done")
+ swh_vault.send_notif(TEST_TYPE, TEST_SWHID)
+
+ assert len(reports) == 6
+ for i, email in enumerate(emails):
+ # first report is the logger.error
+ assert reports[2 * i]["level"] == "error"
+ assert reports[2 * i]["logger"] == "swh.vault.backend"
+ reg = re.compile(
+ "Unable to send SMTP message 'Bundle ready: gitfast [0-9a-f]{7}' "
+ f"to {email.replace('+', '[+]')}: cannot connect to server"
+ )
+ assert reg.match(reports[2 * i]["logentry"]["message"])
+ # second is the sentry_sdk.capture_message
+ assert reports[2 * i + 1]["level"] == "error"
+ assert reg.match(reports[2 * i + 1]["message"])
+
+
+def test_send_email_error_send_failed(swh_vault):
+ reports = []
+ init_sentry("http://example.org", extra_kwargs={"transport": reports.append})
+
+ emails = ("a@example.com", "billg@example.com", "test+42@example.org")
+ with mock_cooking(swh_vault):
+ for email in emails:
+ swh_vault.cook(TEST_TYPE, TEST_SWHID, email=email)
+ swh_vault.set_status(TEST_TYPE, TEST_SWHID, "done")
+
+ with patch("smtplib.SMTP") as MockSMTP:
+ smtp = MockSMTP.return_value
+ smtp.noop.return_value = [250]
+ smtp.send_message.side_effect = smtplib.SMTPHeloError(404, "HELO Failed")
+
+ swh_vault.send_notif(TEST_TYPE, TEST_SWHID)
+
+ assert len(reports) == 4
+ # first one is the captured exception
+ assert reports[0]["level"] == "error"
+ assert reports[0]["exception"]["values"][0]["type"] == "SMTPHeloError"
+
+ # the following 3 ones are the sentry_sdk.capture_message() calls
+ for i, email in enumerate(emails, start=1):
+ assert reports[i]["level"] == "error"
+ reg = re.compile(
+ "Unable to send SMTP message 'Bundle ready: gitfast [0-9a-f]{7}' "
+ f"to {email.replace('+', '[+]')}: [(]404, 'HELO Failed'[)]"
+ )
+ assert reg.match(reports[i]["message"])
+
+
def test_available(swh_vault):
assert not swh_vault.is_available(TEST_TYPE, TEST_SWHID)
@@ -327,10 +388,10 @@ def test_send_failure_email(swh_vault):
swh_vault.set_status(TEST_TYPE, TEST_SWHID, "failed")
swh_vault.set_progress(TEST_TYPE, TEST_SWHID, "test error")
- with patch.object(swh_vault, "smtp_server") as m:
+ with patch.object(swh_vault, "_smtp_send") as m:
swh_vault.send_notif(TEST_TYPE, TEST_SWHID)
- e = [k[0][0] for k in m.send_message.call_args_list][0]
+ e = [k[0][0] for k in m.call_args_list][0]
assert e["To"] == "a@example.com"
assert "bot@softwareheritage.org" in e["From"]
--
GitLab