fix: review findings for email notification processor

Signed-off-by: Heikki Hellgren <heikki.hellgren@op.fi>

plugins/notifications-backend-module-email/README.md

@@ -2,7 +2,7 @@
 
 Adds support for sending Backstage notifications as emails to users.
 
-Supports sending emails using SMTP, SES, or sendmail.
+Supports sending emails using `SMTP`, `SES`, or `sendmail`.
 
 ## Customizing email content
 
@@ -46,6 +46,7 @@ export const notificationsModuleEmailDecorator = createBackendModule({
 notifications:
   processors:
     email:
+      # Transport config, see options at `config.d.ts`
       transportConfig:
         transport: 'smtp'
         hostname: 'my-smtp-server'
@@ -53,12 +54,19 @@ notifications:
         secure: false
         username: 'my-username'
         password: 'my-password'
+      # The email sender address
       sender: 'sender@mycompany.com'
       replyTo: 'no-reply@mycompany.com'
+      # Who to get email for broadcast notifications
       broadcastConfig:
         receiver: 'users'
+      # How many emails to send concurrently, defaults to 2
+      concurrencyLimit: 10
+      # Cache configuration for email addresses
+      # This is to prevent unnecessary calls to the catalog
       cache:
-        ttl: 60000
+        ttl:
+          days: 1
 ```
 
 See `config.d.ts` for more options for configuration.

plugins/notifications-backend-module-email/config.d.ts

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+import { HumanDuration } from '@backstage/types';
+
 export interface Config {
   /**
    * Configuration options for notifications-backend-module-email */
@@ -85,6 +87,10 @@ export interface Config {
          * Optional reply-to address
          */
         replyTo?: string;
+        /**
+         * Concurrency limit for email sending, defaults to 2
+         */
+        concurrencyLimit?: number;
         /**
          * Configuration for broadcast notifications
          */
@@ -105,7 +111,7 @@ export interface Config {
           /**
            * Email cache TTL, defaults to 1 hour
            */
-          ttl?: number;
+          ttl?: HumanDuration;
         };
       };
     };

plugins/notifications-backend-module-email/package.json

@@ -44,7 +44,8 @@
     "@backstage/plugin-notifications-node": "workspace:^",
     "@backstage/types": "workspace:^",
     "lodash": "^4.17.21",
-    "nodemailer": "^6.9.13"
+    "nodemailer": "^6.9.13",
+    "p-limit": "^3.1.0"
   },
   "devDependencies": {
     "@backstage/backend-test-utils": "workspace:^",

plugins/notifications-backend-module-email/src/processor/NotificationsEmailProcessor.test.ts

@@ -50,15 +50,17 @@ describe('NotificationsEmailProcessor', () => {
       logger,
       new ConfigReader({
         notifications: {
-          email: {
-            transport: {
-              transport: 'smtp',
-              hostname: 'localhost',
-              port: 465,
-              secure: true,
-              requireTls: false,
+          processors: {
+            email: {
+              transport: {
+                transport: 'smtp',
+                hostname: 'localhost',
+                port: 465,
+                secure: true,
+                requireTls: false,
+              },
+              sender: 'backstage@backstage.io',
             },
-            sender: 'backstage@backstage.io',
           },
         },
       }),
@@ -94,12 +96,14 @@ describe('NotificationsEmailProcessor', () => {
       logger,
       new ConfigReader({
         notifications: {
-          email: {
-            transport: {
-              transport: 'ses',
-              region: 'us-west-2',
+          processors: {
+            email: {
+              transport: {
+                transport: 'ses',
+                region: 'us-west-2',
+              },
+              sender: 'backstage@backstage.io',
             },
-            sender: 'backstage@backstage.io',
           },
         },
       }),
@@ -132,12 +136,14 @@ describe('NotificationsEmailProcessor', () => {
       logger,
       new ConfigReader({
         notifications: {
-          email: {
-            transport: {
-              transport: 'sendmail',
-              path: '/usr/local/bin/sendmail',
+          processors: {
+            email: {
+              transport: {
+                transport: 'sendmail',
+                path: '/usr/local/bin/sendmail',
+              },
+              sender: 'backstage@backstage.io',
             },
-            sender: 'backstage@backstage.io',
           },
         },
       }),
@@ -181,12 +187,14 @@ describe('NotificationsEmailProcessor', () => {
       logger,
       new ConfigReader({
         notifications: {
-          email: {
-            transport: {
-              transport: 'sendmail',
-              path: '/usr/local/bin/sendmail',
+          processors: {
+            email: {
+              transport: {
+                transport: 'sendmail',
+                path: '/usr/local/bin/sendmail',
+              },
+              sender: 'backstage@backstage.io',
             },
-            sender: 'backstage@backstage.io',
           },
         },
       }),
@@ -236,14 +244,16 @@ describe('NotificationsEmailProcessor', () => {
       logger,
       new ConfigReader({
         notifications: {
-          email: {
-            transport: {
-              transport: 'sendmail',
-              path: '/usr/local/bin/sendmail',
-            },
-            sender: 'backstage@backstage.io',
-            broadcastConfig: {
-              receiver: 'users',
+          processors: {
+            email: {
+              transport: {
+                transport: 'sendmail',
+                path: '/usr/local/bin/sendmail',
+              },
+              sender: 'backstage@backstage.io',
+              broadcastConfig: {
+                receiver: 'users',
+              },
             },
           },
         },
@@ -294,15 +304,17 @@ describe('NotificationsEmailProcessor', () => {
       logger,
       new ConfigReader({
         notifications: {
-          email: {
-            transport: {
-              transport: 'sendmail',
-              path: '/usr/local/bin/sendmail',
-            },
-            sender: 'backstage@backstage.io',
-            broadcastConfig: {
-              receiver: 'config',
-              receiverEmails: ['broadcast@backstage.io'] as JsonArray,
+          processors: {
+            email: {
+              transport: {
+                transport: 'sendmail',
+                path: '/usr/local/bin/sendmail',
+              },
+              sender: 'backstage@backstage.io',
+              broadcastConfig: {
+                receiver: 'config',
+                receiverEmails: ['broadcast@backstage.io'] as JsonArray,
+              },
             },
           },
         },

plugins/notifications-backend-module-email/src/processor/NotificationsEmailProcessor.ts

@@ -22,26 +22,33 @@ import {
   CacheService,
   LoggerService,
 } from '@backstage/backend-plugin-api';
-import { Config } from '@backstage/config';
-import { JsonArray } from '@backstage/types';
+import { Config, readDurationFromConfig } from '@backstage/config';
+import { durationToMilliseconds, JsonArray } from '@backstage/types';
 import {
   CATALOG_FILTER_EXISTS,
   CatalogClient,
 } from '@backstage/catalog-client';
 import { Notification } from '@backstage/plugin-notifications-common';
-import { createSendmailTransport, createSmtpTransport } from './transports';
-import { createSesTransport } from './transports/ses';
+import {
+  createSendmailTransport,
+  createSesTransport,
+  createSmtpTransport,
+} from './transports';
 import { UserEntity } from '@backstage/catalog-model';
 import { compact } from 'lodash';
 import { DefaultAwsCredentialsManager } from '@backstage/integration-aws-node';
 import { NotificationTemplateRenderer } from '../extensions';
+import Mail from 'nodemailer/lib/mailer';
+import pLimit from 'p-limit';
 
 export class NotificationsEmailProcessor implements NotificationProcessor {
   private transporter: any;
   private readonly broadcastConfig?: Config;
+  private readonly transportConfig: Config;
   private readonly sender: string;
   private readonly replyTo?: string;
   private readonly cacheTtl: number;
+  private readonly concurrencyLimit: number;
 
   constructor(
     private readonly logger: LoggerService,
@@ -51,50 +58,39 @@ export class NotificationsEmailProcessor implements NotificationProcessor {
     private readonly cache?: CacheService,
     private readonly templateRenderer?: NotificationTemplateRenderer,
   ) {
-    this.broadcastConfig = config.getOptionalConfig(
-      'notifications.email.broadcastConfig',
+    const emailProcessorConfig = config.getConfig(
+      'notifications.processors.email',
     );
-    this.sender = config.getString('notifications.email.sender');
-    this.replyTo = config.getOptionalString('notifications.email.replyTo');
-    this.cacheTtl =
-      config.getOptionalNumber('notifications.email.cache.ttl') ?? 3_600_000;
+    this.transportConfig = emailProcessorConfig.getConfig('transport');
+    this.broadcastConfig =
+      emailProcessorConfig.getOptionalConfig('broadcastConfig');
+    this.sender = emailProcessorConfig.getString('sender');
+    this.replyTo = emailProcessorConfig.getOptionalString('replyTo');
+    this.concurrencyLimit =
+      emailProcessorConfig.getOptionalNumber('concurrencyLimit') ?? 2;
+    const cacheConfig = emailProcessorConfig.getOptionalConfig('cache.ttl');
+    this.cacheTtl = cacheConfig
+      ? durationToMilliseconds(readDurationFromConfig(cacheConfig))
+      : 3_600_000;
   }
 
   private async getTransporter() {
     if (this.transporter) {
       return this.transporter;
     }
-    const transportConfig = this.config.getConfig(
-      'notifications.email.transport',
-    );
-    const transport = transportConfig.getString('transport');
+    const transport = this.transportConfig.getString('transport');
     if (transport === 'smtp') {
-      this.transporter = createSmtpTransport({
-        transport: 'smtp',
-        hostname: transportConfig.getString('hostname'),
-        port: transportConfig.getNumber('port'),
-        secure: transportConfig.getOptionalBoolean('secure'),
-        requireTls: transportConfig.getOptionalBoolean('requireTls'),
-        username: transportConfig.getOptionalString('username'),
-        password: transportConfig.getOptionalString('password'),
-      });
+      this.transporter = createSmtpTransport(this.transportConfig);
     } else if (transport === 'ses') {
       const awsCredentialsManager = DefaultAwsCredentialsManager.fromConfig(
         this.config,
       );
-      this.transporter = await createSesTransport({
-        transport: 'ses',
-        credentialsManager: awsCredentialsManager,
-        apiVersion: transportConfig.getOptionalString('apiVersion'),
-        accountId: transportConfig.getOptionalString('accountId'),
-        region: transportConfig.getOptionalString('region'),
-      });
+      this.transporter = await createSesTransport(
+        this.transportConfig,
+        awsCredentialsManager,
+      );
     } else if (transport === 'sendmail') {
-      this.transporter = createSendmailTransport({
-        transport: 'sendmail',
-        path: transportConfig.getOptionalString('path'),
-        newline: transportConfig.getOptionalString('newline'),
-      });
+      this.transporter = createSendmailTransport(this.transportConfig);
     } else {
       throw new Error(`Unsupported transport: ${transport}`);
     }
@@ -122,14 +118,13 @@ export class NotificationsEmailProcessor implements NotificationProcessor {
     }
 
     if (receiver === 'users') {
-      const cached = await this.cache?.get<JsonArray>('user-emails:all');
+      const cached = await this.cache?.get<string[]>('user-emails:all');
       if (cached) {
-        return cached as string[];
+        return cached;
       }
 
-      const credentials = await this.auth.getOwnServiceCredentials();
       const { token } = await this.auth.getPluginRequestToken({
-        onBehalfOf: credentials,
+        onBehalfOf: await this.auth.getOwnServiceCredentials(),
         targetPluginId: 'catalog',
       });
       const entities = await this.catalog.getEntities(
@@ -148,6 +143,7 @@ export class NotificationsEmailProcessor implements NotificationProcessor {
           }),
         ),
       ]);
+
       await this.cache?.set('user-emails:all', ret as JsonArray, {
         ttl: this.cacheTtl,
       });
@@ -163,28 +159,24 @@ export class NotificationsEmailProcessor implements NotificationProcessor {
       return [cached];
     }
 
-    const credentials = await this.auth.getOwnServiceCredentials();
     const { token } = await this.auth.getPluginRequestToken({
-      onBehalfOf: credentials,
+      onBehalfOf: await this.auth.getOwnServiceCredentials(),
       targetPluginId: 'catalog',
     });
     const entity = await this.catalog.getEntityByRef(entityRef, { token });
-    if (!entity) {
-      return [];
+    const ret: string[] = [];
+    if (entity) {
+      const userEntity = entity as UserEntity;
+      if (userEntity.spec.profile?.email) {
+        ret.push(userEntity.spec.profile.email);
+      }
     }
 
-    const userEntity = entity as UserEntity;
-    if (!userEntity.spec.profile?.email) {
-      return [];
-    }
+    await this.cache?.set(`user-emails:${entityRef}`, ret[0], {
+      ttl: this.cacheTtl,
+    });
 
-    await this.cache?.set(
-      `user-emails:${entityRef}`,
-      userEntity.spec.profile.email,
-      { ttl: this.cacheTtl },
-    );
-
-    return [userEntity.spec.profile.email];
+    return ret;
   }
 
   private async getRecipientEmails(
@@ -197,6 +189,23 @@ export class NotificationsEmailProcessor implements NotificationProcessor {
     return await this.getUserEmail(notification.user);
   }
 
+  private async sendMail(options: Mail.Options) {
+    try {
+      await this.transporter.sendMail(options);
+    } catch (e) {
+      this.logger.error(`Failed to send email to ${options.to}: ${e}`);
+    }
+  }
+
+  private async sendMails(options: Mail.Options, emails: string[]) {
+    const limit = pLimit(this.concurrencyLimit);
+    await Promise.all(
+      emails.map(email =>
+        limit(() => this.sendMail({ ...options, to: email })),
+      ),
+    );
+  }
+
   private async sendPlainEmail(notification: Notification, emails: string[]) {
     const contentParts: string[] = [];
     if (notification.payload.description) {
@@ -214,13 +223,7 @@ export class NotificationsEmailProcessor implements NotificationProcessor {
       replyTo: this.replyTo,
     };
 
-    for (const email of emails) {
-      try {
-        await this.transporter.sendMail({ ...mailOptions, to: email });
-      } catch (e) {
-        this.logger.error(`Failed to send email to ${email}: ${e}`);
-      }
-    }
+    await this.sendMails(mailOptions, emails);
   }
 
   private async sendTemplateEmail(
@@ -237,13 +240,7 @@ export class NotificationsEmailProcessor implements NotificationProcessor {
       replyTo: this.replyTo,
     };
 
-    for (const email of emails) {
-      try {
-        await this.transporter.sendMail({ ...mailOptions, to: email });
-      } catch (e) {
-        this.logger.error(`Failed to send email to ${email}: ${e}`);
-      }
-    }
+    await this.sendMails(mailOptions, emails);
   }
 
   async postProcess(

plugins/notifications-backend-module-email/src/processor/transports/sendmail.ts

@@ -14,12 +14,12 @@
  * limitations under the License.
  */
 import { createTransport } from 'nodemailer';
-import { SendmailTransportConfig } from '../../types';
+import { Config } from '@backstage/config';
 
-export const createSendmailTransport = (config: SendmailTransportConfig) => {
+export const createSendmailTransport = (config: Config) => {
   return createTransport({
     sendmail: true,
-    newline: config.newline ?? 'unix',
-    path: config.path ?? '/usr/sbin/sendmail',
+    newline: config.getOptionalString('newline') ?? 'unix',
+    path: config.getOptionalString('path') ?? '/usr/sbin/sendmail',
   });
 };

plugins/notifications-backend-module-email/src/processor/transports/ses.ts

@@ -13,19 +13,23 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { SesTransportConfig } from '../../types';
 import { createTransport } from 'nodemailer';
 import { SendRawEmailCommand, SES } from '@aws-sdk/client-ses';
+import { Config } from '@backstage/config';
+import { AwsCredentialsManager } from '@backstage/integration-aws-node';
 
-export const createSesTransport = async (config: SesTransportConfig) => {
-  const credentials = await config.credentialsManager.getCredentialProvider({
-    accountId: config.accountId,
+export const createSesTransport = async (
+  config: Config,
+  credentialsManager: AwsCredentialsManager,
+) => {
+  const credentials = await credentialsManager.getCredentialProvider({
+    accountId: config.getOptionalString('accountId'),
   });
   const ses = new SES([
     {
-      apiVersion: config.apiVersion ?? '2010-12-01',
+      apiVersion: config.getOptionalString('apiVersion') ?? '2010-12-01',
       credentials: credentials.sdkCredentialProvider,
-      region: config.region,
+      region: config.getOptionalString('region'),
     },
   ]);
   return createTransport({

plugins/notifications-backend-module-email/src/processor/transports/smtp.ts

@@ -13,18 +13,18 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { SmtpTransportConfig } from '../../types';
 import { createTransport } from 'nodemailer';
+import { Config } from '@backstage/config';
+
+export const createSmtpTransport = (config: Config) => {
+  const username = config.getOptionalString('username');
+  const password = config.getOptionalString('password');
 
-export const createSmtpTransport = (config: SmtpTransportConfig) => {
   return createTransport({
-    host: config.hostname,
-    port: config.port,
-    secure: config.secure ?? false,
-    requireTLS: config.requireTls ?? false,
-    auth:
-      config.username && config.password
-        ? { user: config.username, pass: config.password }
-        : undefined,
+    host: config.getString('hostname'),
+    port: config.getNumber('port'),
+    secure: config.getOptionalBoolean('secure') ?? false,
+    requireTLS: config.getOptionalBoolean('requireTls') ?? false,
+    auth: username && password ? { user: username, pass: password } : undefined,
   });
 };

plugins/notifications-backend-module-email/src/types.ts

@@ -1,72 +0,0 @@
-/*
- * Copyright 2024 The Backstage Authors
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-import { AwsCredentialsManager } from '@backstage/integration-aws-node';
-
-export interface TransportConfig {
-  transport: 'smtp' | 'ses' | 'sendmail';
-}
-
-export interface SmtpTransportConfig extends TransportConfig {
-  transport: 'smtp';
-  /**
-   * SMTP server hostname
-   */
-  hostname: string;
-  /**
-   * SMTP server port
-   */
-  port: number;
-  /**
-   * Use secure connection for SMTP, defaults to false
-   */
-  secure?: boolean;
-  /**
-   * Require TLS for SMTP connection, defaults to false
-   */
-  requireTls?: boolean;
-  /**
-   * SMTP username
-   */
-  username?: string;
-  /**
-   * SMTP password
-   * @visibility secret
-   */
-  password?: string;
-}
-
-export interface SesTransportConfig extends TransportConfig {
-  transport: 'ses';
-  /**
-   * SES ApiVersion to use, defaults to 2010-12-01
-   */
-  apiVersion?: string;
-  accountId?: string;
-  region?: string;
-  credentialsManager: AwsCredentialsManager;
-}
-
-export interface SendmailTransportConfig extends TransportConfig {
-  transport: 'sendmail';
-  /**
-   * Sendmail binary path, defaults to /usr/sbin/sendmail
-   */
-  path?: string;
-  /**
-   * Newline style, defaults to 'unix'
-   */
-  newline?: string;
-}

yarn.lock

@@ -6051,6 +6051,7 @@ __metadata:
     "@types/nodemailer": ^6.4.14
     lodash: ^4.17.21
     nodemailer: ^6.9.13
+    p-limit: ^3.1.0
   languageName: unknown
   linkType: soft