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

@@ -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:

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"]