coder · DanielleMaywood · Nov 19, 2024 · Nov 12, 2024 · Nov 12, 2024 · Nov 12, 2024
diff --git a/cli/server.go b/cli/server.go
@@ -897,31 +897,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)
-			}
-			options.NotificationsEnqueuer = enqueuer
+			if notificationsCfg.Enabled() {
+				metrics := notifications.NewMetrics(options.PrometheusRegistry)
+				helpers := templateHelpers(options)
 
-			// 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)
-			}
+				// 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))
+				// 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()
+				// 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.")
+			}
 
 			// Since errCh only has one buffered slot, all routines
 			// sending on it must be wrapped in a select/default to
@@ -1098,17 +1106,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
@@ -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
@@ -524,8 +524,8 @@ email:
   # (default: <unset>, type: string)
   from: ""
   # The intermediary SMTP host through which emails are sent.
-  # (default: localhost:587, type: host:port)
-  smarthost: localhost:587
+  # (default: <unset>, type: string)
+  smarthost: ""
   # The hostname identifying the SMTP server.
   # (default: localhost, type: string)
   hello: localhost
@@ -577,8 +577,8 @@ notifications:
     # (default: <unset>, type: string)
     from: ""
     # The intermediary SMTP host through which emails are sent.
-    # (default: <unset>, type: host:port)
-    smarthost: localhost:587
+    # (default: <unset>, type: string)
+    smarthost: ""
     # The hostname identifying the SMTP server.
     # (default: <unset>, type: string)
     hello: localhost

diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git 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
@@ -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
@@ -155,7 +155,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")))
@@ -1113,7 +1113,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
@@ -1524,7 +1524,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
@@ -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
@@ -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
@@ -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