Configurable SMTP Timeout (GSI-986) (#20)

* Add smtp timeout config option and new error class

* Include a test for the timeout

* Bump version from 2.0.0 -> 2.1.0

* Add debug log statements

* Use PositiveFloat instead of PositiveInt

* Set default to 60 seconds

.pre-commit-config.yaml

@@ -48,7 +48,7 @@ repos:
       - id: no-commit-to-branch
         args: [--branch, dev, --branch, int, --branch, main]
   - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: v0.6.2
+    rev: v0.6.4
     hooks:
       - id: ruff
         args: [--fix, --exit-non-zero-on-fix]

.pyproject_generation/pyproject_custom.toml

@@ -1,6 +1,6 @@
 [project]
 name = "ns"
-version = "2.0.0"
+version = "2.1.0"
 description = "The Notification Service (NS) handles notification kafka events."
 dependencies = [
     "typer>=0.12",

README.md

@@ -33,21 +33,21 @@ We recommend using the provided Docker container.
 
 A pre-build version is available at [docker hub](https://hub.docker.com/repository/docker/ghga/notification-service):
 ```bash
-docker pull ghga/notification-service:2.0.0
+docker pull ghga/notification-service:2.1.0
 ```
 
 Or you can build the container yourself from the [`./Dockerfile`](./Dockerfile):
 ```bash
 # Execute in the repo's root dir:
-docker build -t ghga/notification-service:2.0.0 .
+docker build -t ghga/notification-service:2.1.0 .
 ```
 
 For production-ready deployment, we recommend using Kubernetes, however,
 for simple use cases, you could execute the service using docker
 on a single server:
 ```bash
 # The entrypoint is preconfigured:
-docker run -p 8080:8080 ghga/notification-service:2.0.0 --help
+docker run -p 8080:8080 ghga/notification-service:2.1.0 --help
 ```
 
 If you prefer not to use containers, you may install the service from source:
@@ -141,6 +141,14 @@ The service requires the following configuration parameters:
 
 - **`use_starttls`** *(boolean)*: Boolean flag indicating the use of STARTTLS. Default: `true`.
 
+- **`smtp_timeout`**: The maximum amount of time (in seconds) to wait for a connection to the SMTP server. If set to `None`, the operation will wait indefinitely. Default: `60`.
+
+  - **Any of**
+
+    - *number*: Exclusive minimum: `0.0`.
+
+    - *null*
+
 - **`notification_event_topic`** *(string, required)*: Name of the event topic used to track notification events.
 
 

config_schema.json

@@ -139,6 +139,20 @@
       "title": "Use Starttls",
       "type": "boolean"
     },
+    "smtp_timeout": {
+      "anyOf": [
+        {
+          "exclusiveMinimum": 0.0,
+          "type": "number"
+        },
+        {
+          "type": "null"
+        }
+      ],
+      "default": 60,
+      "description": "The maximum amount of time (in seconds) to wait for a connection to the SMTP server. If set to `None`, the operation will wait indefinitely.",
+      "title": "Smtp Timeout"
+    },
     "notification_event_topic": {
       "description": "Name of the event topic used to track notification events",
       "examples": [

example_config.yaml

@@ -35,4 +35,5 @@ smtp_auth:
   username: [email protected]
 smtp_host: 127.0.0.1
 smtp_port: 587
+smtp_timeout: 60.0
 use_starttls: false

pyproject.toml

@@ -21,7 +21,7 @@ classifiers = [
     "Intended Audience :: Developers",
 ]
 name = "ns"
-version = "2.0.0"
+version = "2.1.0"
 description = "The Notification Service (NS) handles notification kafka events."
 dependencies = [
     "typer>=0.12",

src/ns/adapters/outbound/smtp_client.py

@@ -23,7 +23,7 @@
 from email.message import EmailMessage
 from smtplib import SMTP, SMTPAuthenticationError, SMTPException
 
-from pydantic import BaseModel, Field, SecretStr
+from pydantic import BaseModel, Field, PositiveFloat, SecretStr
 from pydantic_settings import BaseSettings
 
 from ns.ports.outbound.smtp_client import SmtpClientPort
@@ -51,6 +51,13 @@ class SmtpClientConfig(BaseSettings):
     use_starttls: bool = Field(
         default=True, description="Boolean flag indicating the use of STARTTLS"
     )
+    smtp_timeout: PositiveFloat | None = Field(
+        default=60,
+        description=(
+            "The maximum amount of time (in seconds) to wait for a connection to the"
+            + " SMTP server. If set to `None`, the operation will wait indefinitely."
+        ),
+    )
 
 
 class SmtpClient(SmtpClientPort):
@@ -63,8 +70,21 @@ def __init__(self, *, config: SmtpClientConfig):
     @contextmanager
     def get_connection(self) -> Generator[SMTP, None, None]:
         """Establish a connection to the SMTP server"""
-        with SMTP(self._config.smtp_host, self._config.smtp_port) as server:
-            yield server
+        timeout = self._config.smtp_timeout if self._config.smtp_timeout else None
+
+        try:
+            log.debug("Attempting to establish SMTP connection (timeout=%s).", timeout)
+            with (
+                SMTP(self._config.smtp_host, self._config.smtp_port, timeout=timeout)
+                if timeout
+                else SMTP(self._config.smtp_host, self._config.smtp_port) as server
+            ):
+                log.debug("STMP connection successfully established.")
+                yield server
+        except OSError as err:
+            # TimeoutError is a subclass of OSError, but a blocked port can result
+            # in a generic OSError without exhausting smtp_timeout
+            raise self.ConnectionAttemptError() from err
 
     def send_email_message(self, message: EmailMessage):
         """Send an email message.
@@ -85,19 +105,23 @@ def send_email_message(self, message: EmailMessage):
                     username = self._config.smtp_auth.username
                     password = self._config.smtp_auth.password.get_secret_value()
                     try:
+                        log.debug("Authenticating against the SMTP server.")
                         server.login(username, password)
                     except SMTPAuthenticationError as err:
                         login_error = self.FailedLoginError()
                         log.critical(login_error)
                         raise login_error from err
 
                 # check for a connection
+                log.debug("Performing NOOP to verify SMTP connection.")
                 if server.noop()[0] != 250:
-                    connection_error = self.ConnectionError()
+                    connection_error = self.ServerPingError()
                     log.critical(connection_error)
                     raise connection_error
 
+                log.debug("NOOP successful, sending email.")
                 server.send_message(msg=message)
+                log.debug("Email sent.")
         except SMTPException as exc:
             error = self.GeneralSmtpException(error_info=exc.args[0])
             log.error(error, extra={"error_info": exc.args[0]})

src/ns/core/notifier.py

@@ -115,6 +115,7 @@ async def send_notification(self, *, notification: event_schemas.Notification):
         await self._register_new_notification(notification_record=notification_record)
 
         message = self._construct_email(notification=notification)
+        log.info("Sending notification")
         self._smtp_client.send_email_message(message)
 
         # update the notification record to show that the notification has been sent.

src/ns/ports/outbound/smtp_client.py

@@ -22,7 +22,14 @@
 class SmtpClientPort(ABC):
     """Abstract description of an SMTP client that can send email"""
 
-    class ConnectionError(RuntimeError):
+    class ConnectionAttemptError(RuntimeError):
+        """Raised when the attempt to reach the SMTP server times out or is blocked."""
+
+        def __init__(self):
+            message = "Attempt to connect to the SMTP server failed."
+            super().__init__(message)
+
+    class ServerPingError(RuntimeError):
         """To be raised when testing the connection fails"""
 
         def __init__(self):

tests/fixtures/test_config.yaml

@@ -10,6 +10,7 @@ smtp_auth:
   username: "[email protected]"
   password: test
 use_starttls: false
+smtp_timeout: 2
 from_address: "[email protected]"
 db_connection_str: "mongodb://mongodb:27017"
 db_name: "dev_db"

tests/test_basic.py

@@ -22,10 +22,11 @@
 from email.message import EmailMessage
 from hashlib import sha256
 from typing import cast
-from unittest.mock import Mock
+from unittest.mock import AsyncMock, Mock
 
 import pytest
 from hexkit.correlation import correlation_id_var
+from hexkit.protocols.dao import ResourceNotFoundError
 from pydantic import SecretStr
 
 from ns.adapters.outbound.smtp_client import (
@@ -35,6 +36,7 @@
 )
 from ns.core.models import NotificationRecord
 from ns.core.notifier import Notifier
+from tests.fixtures.config import get_config
 from tests.fixtures.joint import JointFixture
 from tests.fixtures.server import DummyServer
 from tests.fixtures.utils import make_notification
@@ -210,7 +212,7 @@ async def test_consume_thru_send(joint_fixture: JointFixture):
         topic=joint_fixture.config.notification_event_topic,
     )
 
-    with pytest.raises(ConnectionRefusedError):
+    with pytest.raises(SmtpClient.ConnectionAttemptError):
         # the connection error tells us that the smtp_client tried to connect, which
         # means that the consumer successfully passed the event through the notifier
         # and on to the client for emailing.
@@ -375,3 +377,23 @@ async def test_html_escaping(joint_fixture: JointFixture):
         + "</body></html>"
     )
     assert html_content.strip() == expected_html
+
+
+@pytest.mark.parametrize("port", [443, 0])
+async def test_timeout(port: int):
+    """Test that the SMTP timeout works as expected."""
+    client_config = SmtpClientConfig(
+        smtp_auth=None, smtp_host="localhost", smtp_port=port, smtp_timeout=1
+    )
+    config = get_config(sources=[client_config])
+    smtp_client = SmtpClient(config=config)
+    dao_mock = AsyncMock()
+    dao_mock.get_by_id.side_effect = ResourceNotFoundError(id_="test")
+
+    notifier = Notifier(
+        config=config, smtp_client=smtp_client, notification_record_dao=dao_mock
+    )
+    with pytest.raises(smtp_client.ConnectionAttemptError):
+        await notifier.send_notification(
+            notification=make_notification(sample_notification)
+        )