From 4e4e5607e2d544b3f951cef5095028fc719302bf Mon Sep 17 00:00:00 2001 From: Heikki Hellgren Date: Wed, 24 Apr 2024 09:18:35 +0300 Subject: [PATCH] fix: review findings for email notification processor Signed-off-by: Heikki Hellgren --- .../README.md | 12 +- .../config.d.ts | 8 +- .../package.json | 3 +- .../NotificationsEmailProcessor.test.ts | 92 ++++++------ .../processor/NotificationsEmailProcessor.ts | 133 +++++++++--------- .../src/processor/transports/sendmail.ts | 8 +- .../src/processor/transports/ses.ts | 16 ++- .../src/processor/transports/smtp.ts | 20 +-- .../src/types.ts | 72 ---------- yarn.lock | 1 + 10 files changed, 161 insertions(+), 204 deletions(-) delete mode 100644 plugins/notifications-backend-module-email/src/types.ts diff --git a/plugins/notifications-backend-module-email/README.md b/plugins/notifications-backend-module-email/README.md index 008174e5d3..7bbac05b62 100644 --- a/plugins/notifications-backend-module-email/README.md +++ b/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. diff --git a/plugins/notifications-backend-module-email/config.d.ts b/plugins/notifications-backend-module-email/config.d.ts index 54de2f6ab4..90fb43e046 100644 --- a/plugins/notifications-backend-module-email/config.d.ts +++ b/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; }; }; }; diff --git a/plugins/notifications-backend-module-email/package.json b/plugins/notifications-backend-module-email/package.json index 29cc9d7c24..3d44db6a45 100644 --- a/plugins/notifications-backend-module-email/package.json +++ b/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:^", diff --git a/plugins/notifications-backend-module-email/src/processor/NotificationsEmailProcessor.test.ts b/plugins/notifications-backend-module-email/src/processor/NotificationsEmailProcessor.test.ts index f4db00869e..3af1e498ca 100644 --- a/plugins/notifications-backend-module-email/src/processor/NotificationsEmailProcessor.test.ts +++ b/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, + }, }, }, }, diff --git a/plugins/notifications-backend-module-email/src/processor/NotificationsEmailProcessor.ts b/plugins/notifications-backend-module-email/src/processor/NotificationsEmailProcessor.ts index 296cf83657..a52495a8ea 100644 --- a/plugins/notifications-backend-module-email/src/processor/NotificationsEmailProcessor.ts +++ b/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('user-emails:all'); + const cached = await this.cache?.get('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( diff --git a/plugins/notifications-backend-module-email/src/processor/transports/sendmail.ts b/plugins/notifications-backend-module-email/src/processor/transports/sendmail.ts index 8f093f562d..e16ea37ac9 100644 --- a/plugins/notifications-backend-module-email/src/processor/transports/sendmail.ts +++ b/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', }); }; diff --git a/plugins/notifications-backend-module-email/src/processor/transports/ses.ts b/plugins/notifications-backend-module-email/src/processor/transports/ses.ts index d208001bad..0be23078ab 100644 --- a/plugins/notifications-backend-module-email/src/processor/transports/ses.ts +++ b/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({ diff --git a/plugins/notifications-backend-module-email/src/processor/transports/smtp.ts b/plugins/notifications-backend-module-email/src/processor/transports/smtp.ts index e473ea5ba7..feea417c27 100644 --- a/plugins/notifications-backend-module-email/src/processor/transports/smtp.ts +++ b/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, }); }; diff --git a/plugins/notifications-backend-module-email/src/types.ts b/plugins/notifications-backend-module-email/src/types.ts deleted file mode 100644 index 57c4b05234..0000000000 --- a/plugins/notifications-backend-module-email/src/types.ts +++ /dev/null @@ -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; -} diff --git a/yarn.lock b/yarn.lock index 40554ed539..400095afee 100644 --- a/yarn.lock +++ b/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