From 576e1f48fedd742b1101c7a489c69cee9271940b Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 19 Nov 2024 10:05:12 -0500 Subject: [PATCH] feat!: allow disabling notifications (#15509) Resolves https://github.com/coder/coder/issues/15513 Disables notifications when both `$CODER_NOTIFICATIONS_WEBHOOK_ENDPOINT` and `$CODER_EMAIL_SMARTHOST` are unset. Breaking change: `$CODER_EMAIL_SMARTHOST` is no longer set by default as `localhost:587`, meaning any deployments that make use of this default value will need to add it back. --------- Co-authored-by: Danny Kopping Co-authored-by: Mathias Fredriksson --- cli/server.go | 78 +++++++++++-------- cli/testdata/coder_server_--help.golden | 4 +- cli/testdata/server-config.yaml.golden | 8 +- coderd/apidoc/docs.go | 6 +- coderd/apidoc/swagger.json | 6 +- coderd/notifications/dispatch/smtp.go | 24 +++--- coderd/notifications/dispatch/smtp_test.go | 2 +- coderd/notifications/notifications_test.go | 6 +- codersdk/deployment.go | 7 +- codersdk/deployment_test.go | 66 ++++++++++++++++ docs/admin/monitoring/notifications/index.md | 13 ++-- docs/reference/api/general.md | 5 +- docs/reference/api/schemas.md | 22 ++---- docs/reference/cli/server.md | 5 +- .../cli/testdata/coder_server_--help.golden | 4 +- 15 files changed, 155 insertions(+), 101 deletions(-) diff --git a/cli/server.go b/cli/server.go index 5962a71ca7..ff8b2963e0 100644 --- a/cli/server.go +++ b/cli/server.go @@ -876,31 +876,39 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. } // Manage notifications. - cfg := options.DeploymentValues.Notifications - metrics := notifications.NewMetrics(options.PrometheusRegistry) - helpers := templateHelpers(options) + var ( + notificationsCfg = options.DeploymentValues.Notifications + notificationsManager *notifications.Manager + ) - // The enqueuer is responsible for enqueueing notifications to the given store. - enqueuer, err := notifications.NewStoreEnqueuer(cfg, options.Database, helpers, logger.Named("notifications.enqueuer"), quartz.NewReal()) - if err != nil { - return xerrors.Errorf("failed to instantiate notification store enqueuer: %w", err) + if notificationsCfg.Enabled() { + metrics := notifications.NewMetrics(options.PrometheusRegistry) + helpers := templateHelpers(options) + + // The enqueuer is responsible for enqueueing notifications to the given store. + enqueuer, err := notifications.NewStoreEnqueuer(notificationsCfg, options.Database, helpers, logger.Named("notifications.enqueuer"), quartz.NewReal()) + if err != nil { + return xerrors.Errorf("failed to instantiate notification store enqueuer: %w", err) + } + options.NotificationsEnqueuer = enqueuer + + // The notification manager is responsible for: + // - creating notifiers and managing their lifecycles (notifiers are responsible for dequeueing/sending notifications) + // - keeping the store updated with status updates + notificationsManager, err = notifications.NewManager(notificationsCfg, options.Database, helpers, metrics, logger.Named("notifications.manager")) + if err != nil { + return xerrors.Errorf("failed to instantiate notification manager: %w", err) + } + + // nolint:gocritic // We need to run the manager in a notifier context. + notificationsManager.Run(dbauthz.AsNotifier(ctx)) + + // Run report generator to distribute periodic reports. + notificationReportGenerator := reports.NewReportGenerator(ctx, logger.Named("notifications.report_generator"), options.Database, options.NotificationsEnqueuer, quartz.NewReal()) + defer notificationReportGenerator.Close() + } else { + cliui.Info(inv.Stdout, "Notifications are currently disabled as there are no configured delivery methods. See https://coder.com/docs/admin/monitoring/notifications#delivery-methods for more details.") } - options.NotificationsEnqueuer = enqueuer - - // The notification manager is responsible for: - // - creating notifiers and managing their lifecycles (notifiers are responsible for dequeueing/sending notifications) - // - keeping the store updated with status updates - notificationsManager, err := notifications.NewManager(cfg, options.Database, helpers, metrics, logger.Named("notifications.manager")) - if err != nil { - return xerrors.Errorf("failed to instantiate notification manager: %w", err) - } - - // nolint:gocritic // We need to run the manager in a notifier context. - notificationsManager.Run(dbauthz.AsNotifier(ctx)) - - // Run report generator to distribute periodic reports. - notificationReportGenerator := reports.NewReportGenerator(ctx, logger.Named("notifications.report_generator"), options.Database, options.NotificationsEnqueuer, quartz.NewReal()) - defer notificationReportGenerator.Close() // Since errCh only has one buffered slot, all routines // sending on it must be wrapped in a select/default to @@ -1077,17 +1085,19 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. // Cancel any remaining in-flight requests. shutdownConns() - // Stop the notification manager, which will cause any buffered updates to the store to be flushed. - // If the Stop() call times out, messages that were sent but not reflected as such in the store will have - // their leases expire after a period of time and will be re-queued for sending. - // See CODER_NOTIFICATIONS_LEASE_PERIOD. - cliui.Info(inv.Stdout, "Shutting down notifications manager..."+"\n") - err = shutdownWithTimeout(notificationsManager.Stop, 5*time.Second) - if err != nil { - cliui.Warnf(inv.Stderr, "Notifications manager shutdown took longer than 5s, "+ - "this may result in duplicate notifications being sent: %s\n", err) - } else { - cliui.Info(inv.Stdout, "Gracefully shut down notifications manager\n") + if notificationsManager != nil { + // Stop the notification manager, which will cause any buffered updates to the store to be flushed. + // If the Stop() call times out, messages that were sent but not reflected as such in the store will have + // their leases expire after a period of time and will be re-queued for sending. + // See CODER_NOTIFICATIONS_LEASE_PERIOD. + cliui.Info(inv.Stdout, "Shutting down notifications manager..."+"\n") + err = shutdownWithTimeout(notificationsManager.Stop, 5*time.Second) + if err != nil { + cliui.Warnf(inv.Stderr, "Notifications manager shutdown took longer than 5s, "+ + "this may result in duplicate notifications being sent: %s\n", err) + } else { + cliui.Info(inv.Stdout, "Gracefully shut down notifications manager\n") + } } // Shut down provisioners before waiting for WebSockets diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 7e4088ccb6..6f524dff9a 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -118,7 +118,7 @@ Configure how emails are sent. --email-hello string, $CODER_EMAIL_HELLO (default: localhost) The hostname identifying the SMTP server. - --email-smarthost host:port, $CODER_EMAIL_SMARTHOST (default: localhost:587) + --email-smarthost string, $CODER_EMAIL_SMARTHOST The intermediary SMTP host through which emails are sent. EMAIL / EMAIL AUTHENTICATION OPTIONS: @@ -413,7 +413,7 @@ Configure how email notifications are sent. The hostname identifying the SMTP server. DEPRECATED: Use --email-hello instead. - --notifications-email-smarthost host:port, $CODER_NOTIFICATIONS_EMAIL_SMARTHOST + --notifications-email-smarthost string, $CODER_NOTIFICATIONS_EMAIL_SMARTHOST The intermediary SMTP host through which emails are sent. DEPRECATED: Use --email-smarthost instead. diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index 38b2b68c24..72f39997b5 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -524,8 +524,8 @@ email: # (default: , type: string) from: "" # The intermediary SMTP host through which emails are sent. - # (default: localhost:587, type: host:port) - smarthost: localhost:587 + # (default: , type: string) + smarthost: "" # The hostname identifying the SMTP server. # (default: localhost, type: string) hello: localhost @@ -577,8 +577,8 @@ notifications: # (default: , type: string) from: "" # The intermediary SMTP host through which emails are sent. - # (default: , type: host:port) - smarthost: localhost:587 + # (default: , type: string) + smarthost: "" # The hostname identifying the SMTP server. # (default: , type: string) hello: localhost diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 2c046dc8d9..7c96af792e 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -11553,11 +11553,7 @@ const docTemplate = `{ }, "smarthost": { "description": "The intermediary SMTP host through which emails are sent (host:port).", - "allOf": [ - { - "$ref": "#/definitions/serpent.HostPort" - } - ] + "type": "string" }, "tls": { "description": "TLS details.", diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 4baae0c356..56b36a4580 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -10370,11 +10370,7 @@ }, "smarthost": { "description": "The intermediary SMTP host through which emails are sent (host:port).", - "allOf": [ - { - "$ref": "#/definitions/serpent.HostPort" - } - ] + "type": "string" }, "tls": { "description": "TLS details.", diff --git a/coderd/notifications/dispatch/smtp.go b/coderd/notifications/dispatch/smtp.go index dfb628b62e..14ce6b63b4 100644 --- a/coderd/notifications/dispatch/smtp.go +++ b/coderd/notifications/dispatch/smtp.go @@ -34,11 +34,10 @@ import ( ) var ( - ValidationNoFromAddressErr = xerrors.New("no 'from' address defined") - ValidationNoToAddressErr = xerrors.New("no 'to' address(es) defined") - ValidationNoSmarthostHostErr = xerrors.New("smarthost 'host' is not defined, or is invalid") - ValidationNoSmarthostPortErr = xerrors.New("smarthost 'port' is not defined, or is invalid") - ValidationNoHelloErr = xerrors.New("'hello' not defined") + ValidationNoFromAddressErr = xerrors.New("'from' address not defined") + ValidationNoToAddressErr = xerrors.New("'to' address(es) not defined") + ValidationNoSmarthostErr = xerrors.New("'smarthost' address not defined") + ValidationNoHelloErr = xerrors.New("'hello' not defined") //go:embed smtp/html.gotmpl htmlTemplate string @@ -521,15 +520,14 @@ func (s *SMTPHandler) validateToAddrs(to string) ([]string, error) { // Does not allow overriding. // nolint:revive // documented. func (s *SMTPHandler) smarthost() (string, string, error) { - host := s.cfg.Smarthost.Host - port := s.cfg.Smarthost.Port - - // We don't validate the contents themselves; this will be done by the underlying SMTP library. - if host == "" { - return "", "", ValidationNoSmarthostHostErr + smarthost := strings.TrimSpace(string(s.cfg.Smarthost)) + if smarthost == "" { + return "", "", ValidationNoSmarthostErr } - if port == "" { - return "", "", ValidationNoSmarthostPortErr + + host, port, err := net.SplitHostPort(string(s.cfg.Smarthost)) + if err != nil { + return "", "", xerrors.Errorf("split host port: %w", err) } return host, port, nil diff --git a/coderd/notifications/dispatch/smtp_test.go b/coderd/notifications/dispatch/smtp_test.go index c9a60b426a..b448dd2582 100644 --- a/coderd/notifications/dispatch/smtp_test.go +++ b/coderd/notifications/dispatch/smtp_test.go @@ -440,7 +440,7 @@ func TestSMTP(t *testing.T) { var hp serpent.HostPort require.NoError(t, hp.Set(listen.Addr().String())) - tc.cfg.Smarthost = hp + tc.cfg.Smarthost = serpent.String(hp.String()) handler := dispatch.NewSMTPHandler(tc.cfg, logger.Named("smtp")) diff --git a/coderd/notifications/notifications_test.go b/coderd/notifications/notifications_test.go index 4747e6a673..aedbcaf844 100644 --- a/coderd/notifications/notifications_test.go +++ b/coderd/notifications/notifications_test.go @@ -154,7 +154,7 @@ func TestSMTPDispatch(t *testing.T) { cfg := defaultNotificationsConfig(method) cfg.SMTP = codersdk.NotificationsEmailConfig{ From: from, - Smarthost: serpent.HostPort{Host: "localhost", Port: fmt.Sprintf("%d", mockSMTPSrv.PortNumber())}, + Smarthost: serpent.String(fmt.Sprintf("localhost:%d", mockSMTPSrv.PortNumber())), Hello: "localhost", } handler := newDispatchInterceptor(dispatch.NewSMTPHandler(cfg.SMTP, logger.Named("smtp"))) @@ -1112,7 +1112,7 @@ func TestNotificationTemplates_Golden(t *testing.T) { var hp serpent.HostPort require.NoError(t, hp.Set(listen.Addr().String())) - smtpConfig.Smarthost = hp + smtpConfig.Smarthost = serpent.String(hp.String()) // Start mock SMTP server in the background. var wg sync.WaitGroup @@ -1523,7 +1523,7 @@ func TestCustomNotificationMethod(t *testing.T) { cfg.SMTP = codersdk.NotificationsEmailConfig{ From: "danny@coder.com", Hello: "localhost", - Smarthost: serpent.HostPort{Host: "localhost", Port: fmt.Sprintf("%d", mockSMTPSrv.PortNumber())}, + Smarthost: serpent.String(fmt.Sprintf("localhost:%d", mockSMTPSrv.PortNumber())), } cfg.Webhook = codersdk.NotificationsWebhookConfig{ Endpoint: *serpent.URLOf(endpoint), diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 0a97da7d39..7de961ec86 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -686,11 +686,15 @@ type NotificationsConfig struct { Webhook NotificationsWebhookConfig `json:"webhook" typescript:",notnull"` } +func (n *NotificationsConfig) Enabled() bool { + return n.SMTP.Smarthost != "" || n.Webhook.Endpoint != serpent.URL{} +} + type NotificationsEmailConfig struct { // The sender's address. From serpent.String `json:"from" typescript:",notnull"` // The intermediary SMTP host through which emails are sent (host:port). - Smarthost serpent.HostPort `json:"smarthost" typescript:",notnull"` + Smarthost serpent.String `json:"smarthost" typescript:",notnull"` // The hostname identifying the SMTP server. Hello serpent.String `json:"hello" typescript:",notnull"` @@ -1028,7 +1032,6 @@ when required by your organization's security policy.`, Description: "The intermediary SMTP host through which emails are sent.", Flag: "email-smarthost", Env: "CODER_EMAIL_SMARTHOST", - Default: "localhost:587", // To pass validation. Value: &c.Notifications.SMTP.Smarthost, Group: &deploymentGroupEmail, YAML: "smarthost", diff --git a/codersdk/deployment_test.go b/codersdk/deployment_test.go index 61474a3b77..7a84fcbbd8 100644 --- a/codersdk/deployment_test.go +++ b/codersdk/deployment_test.go @@ -568,3 +568,69 @@ func TestPremiumSuperSet(t *testing.T) { require.NotContains(t, enterprise.Features(), "", "enterprise should not contain empty string") require.NotContains(t, premium.Features(), "", "premium should not contain empty string") } + +func TestNotificationsCanBeDisabled(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + expectNotificationsEnabled bool + environment []serpent.EnvVar + }{ + { + name: "NoDeliveryMethodSet", + environment: []serpent.EnvVar{}, + expectNotificationsEnabled: false, + }, + { + name: "SMTP_DeliveryMethodSet", + environment: []serpent.EnvVar{ + { + Name: "CODER_EMAIL_SMARTHOST", + Value: "localhost:587", + }, + }, + expectNotificationsEnabled: true, + }, + { + name: "Webhook_DeliveryMethodSet", + environment: []serpent.EnvVar{ + { + Name: "CODER_NOTIFICATIONS_WEBHOOK_ENDPOINT", + Value: "https://example.com/webhook", + }, + }, + expectNotificationsEnabled: true, + }, + { + name: "WebhookAndSMTP_DeliveryMethodSet", + environment: []serpent.EnvVar{ + { + Name: "CODER_NOTIFICATIONS_WEBHOOK_ENDPOINT", + Value: "https://example.com/webhook", + }, + { + Name: "CODER_EMAIL_SMARTHOST", + Value: "localhost:587", + }, + }, + expectNotificationsEnabled: true, + }, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + dv := codersdk.DeploymentValues{} + opts := dv.Options() + + err := opts.ParseEnv(tt.environment) + require.NoError(t, err) + + require.Equal(t, tt.expectNotificationsEnabled, dv.Notifications.Enabled()) + }) + } +} diff --git a/docs/admin/monitoring/notifications/index.md b/docs/admin/monitoring/notifications/index.md index eabc09438d..a9e6a87d78 100644 --- a/docs/admin/monitoring/notifications/index.md +++ b/docs/admin/monitoring/notifications/index.md @@ -74,7 +74,8 @@ flags. Notifications can currently be delivered by either SMTP or webhook. Each message can only be delivered to one method, and this method is configured globally with [`CODER_NOTIFICATIONS_METHOD`](../../../reference/cli/server.md#--notifications-method) -(default: `smtp`). +(default: `smtp`). When there are no delivery methods configured, notifications +will be disabled. Premium customers can configure which method to use for each of the supported [Events](#workspace-events); see the @@ -89,11 +90,11 @@ existing one. **Server Settings:** -| Required | CLI | Env | Type | Description | Default | -| :------: | ------------------- | ----------------------- | ----------- | ----------------------------------------- | ------------- | -| ✔️ | `--email-from` | `CODER_EMAIL_FROM` | `string` | The sender's address to use. | | -| ✔️ | `--email-smarthost` | `CODER_EMAIL_SMARTHOST` | `host:port` | The SMTP relay to send messages through. | localhost:587 | -| ✔️ | `--email-hello` | `CODER_EMAIL_HELLO` | `string` | The hostname identifying the SMTP server. | localhost | +| Required | CLI | Env | Type | Description | Default | +| :------: | ------------------- | ----------------------- | -------- | ----------------------------------------- | --------- | +| ✔️ | `--email-from` | `CODER_EMAIL_FROM` | `string` | The sender's address to use. | | +| ✔️ | `--email-smarthost` | `CODER_EMAIL_SMARTHOST` | `string` | The SMTP relay to send messages | +| ✔️ | `--email-hello` | `CODER_EMAIL_HELLO` | `string` | The hostname identifying the SMTP server. | localhost | **Authentication Settings:** diff --git a/docs/reference/api/general.md b/docs/reference/api/general.md index b645254584..c65d9ec417 100644 --- a/docs/reference/api/general.md +++ b/docs/reference/api/general.md @@ -266,10 +266,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "force_tls": true, "from": "string", "hello": "string", - "smarthost": { - "host": "string", - "port": "string" - }, + "smarthost": "string", "tls": { "ca_file": "string", "cert_file": "string", diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index fe8db822aa..d40fe8e240 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -1884,10 +1884,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "force_tls": true, "from": "string", "hello": "string", - "smarthost": { - "host": "string", - "port": "string" - }, + "smarthost": "string", "tls": { "ca_file": "string", "cert_file": "string", @@ -2311,10 +2308,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "force_tls": true, "from": "string", "hello": "string", - "smarthost": { - "host": "string", - "port": "string" - }, + "smarthost": "string", "tls": { "ca_file": "string", "cert_file": "string", @@ -3417,10 +3411,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "force_tls": true, "from": "string", "hello": "string", - "smarthost": { - "host": "string", - "port": "string" - }, + "smarthost": "string", "tls": { "ca_file": "string", "cert_file": "string", @@ -3505,10 +3496,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "force_tls": true, "from": "string", "hello": "string", - "smarthost": { - "host": "string", - "port": "string" - }, + "smarthost": "string", "tls": { "ca_file": "string", "cert_file": "string", @@ -3528,7 +3516,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o | `force_tls` | boolean | false | | Force tls causes a TLS connection to be attempted. | | `from` | string | false | | The sender's address. | | `hello` | string | false | | The hostname identifying the SMTP server. | -| `smarthost` | [serpent.HostPort](#serpenthostport) | false | | The intermediary SMTP host through which emails are sent (host:port). | +| `smarthost` | string | false | | The intermediary SMTP host through which emails are sent (host:port). | | `tls` | [codersdk.NotificationsEmailTLSConfig](#codersdknotificationsemailtlsconfig) | false | | Tls details. | ## codersdk.NotificationsEmailTLSConfig diff --git a/docs/reference/cli/server.md b/docs/reference/cli/server.md index 3b3d2376c9..bdb338076c 100644 --- a/docs/reference/cli/server.md +++ b/docs/reference/cli/server.md @@ -1231,10 +1231,9 @@ The sender's address to use. | | | | ----------- | ----------------------------------- | -| Type | host:port | +| Type | string | | Environment | $CODER_EMAIL_SMARTHOST | | YAML | email.smarthost | -| Default | localhost:587 | The intermediary SMTP host through which emails are sent. @@ -1395,7 +1394,7 @@ The sender's address to use. | | | | ----------- | ------------------------------------------------- | -| Type | host:port | +| Type | string | | Environment | $CODER_NOTIFICATIONS_EMAIL_SMARTHOST | | YAML | notifications.email.smarthost | diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index aaa4725c65..8c12249a8b 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/enterprise/cli/testdata/coder_server_--help.golden @@ -119,7 +119,7 @@ Configure how emails are sent. --email-hello string, $CODER_EMAIL_HELLO (default: localhost) The hostname identifying the SMTP server. - --email-smarthost host:port, $CODER_EMAIL_SMARTHOST (default: localhost:587) + --email-smarthost string, $CODER_EMAIL_SMARTHOST The intermediary SMTP host through which emails are sent. EMAIL / EMAIL AUTHENTICATION OPTIONS: @@ -414,7 +414,7 @@ Configure how email notifications are sent. The hostname identifying the SMTP server. DEPRECATED: Use --email-hello instead. - --notifications-email-smarthost host:port, $CODER_NOTIFICATIONS_EMAIL_SMARTHOST + --notifications-email-smarthost string, $CODER_NOTIFICATIONS_EMAIL_SMARTHOST The intermediary SMTP host through which emails are sent. DEPRECATED: Use --email-smarthost instead.