Skip to content

Commit 576e1f4

Browse files
DanielleMaywooddannykoppingmafredri
authored
feat!: allow disabling notifications (coder#15509)
Resolves coder#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 <danny@coder.com> Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
1 parent 1c08580 commit 576e1f4

File tree

15 files changed

+153
-99
lines changed

15 files changed

+153
-99
lines changed

‎cli/server.go

Lines changed: 42 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -876,31 +876,39 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
876876
}
877877

878878
// Manage notifications.
879-
cfg := options.DeploymentValues.Notifications
880-
metrics := notifications.NewMetrics(options.PrometheusRegistry)
881-
helpers := templateHelpers(options)
879+
var (
880+
notificationsCfg = options.DeploymentValues.Notifications
881+
notificationsManager *notifications.Manager
882+
)
882883

883-
// The enqueuer is responsible for enqueueing notifications to the given store.
884-
enqueuer, err := notifications.NewStoreEnqueuer(cfg, options.Database, helpers, logger.Named("notifications.enqueuer"), quartz.NewReal())
885-
if err != nil {
886-
return xerrors.Errorf("failed to instantiate notification store enqueuer: %w", err)
887-
}
888-
options.NotificationsEnqueuer = enqueuer
884+
if notificationsCfg.Enabled() {
885+
metrics := notifications.NewMetrics(options.PrometheusRegistry)
886+
helpers := templateHelpers(options)
889887

890-
// The notification manager is responsible for:
891-
// - creating notifiers and managing their lifecycles (notifiers are responsible for dequeueing/sending notifications)
892-
// - keeping the store updated with status updates
893-
notificationsManager, err := notifications.NewManager(cfg, options.Database, helpers, metrics, logger.Named("notifications.manager"))
894-
if err != nil {
895-
return xerrors.Errorf("failed to instantiate notification manager: %w", err)
896-
}
888+
// The enqueuer is responsible for enqueueing notifications to the given store.
889+
enqueuer, err := notifications.NewStoreEnqueuer(notificationsCfg, options.Database, helpers, logger.Named("notifications.enqueuer"), quartz.NewReal())
890+
if err != nil {
891+
return xerrors.Errorf("failed to instantiate notification store enqueuer: %w", err)
892+
}
893+
options.NotificationsEnqueuer = enqueuer
894+
895+
// The notification manager is responsible for:
896+
// - creating notifiers and managing their lifecycles (notifiers are responsible for dequeueing/sending notifications)
897+
// - keeping the store updated with status updates
898+
notificationsManager, err = notifications.NewManager(notificationsCfg, options.Database, helpers, metrics, logger.Named("notifications.manager"))
899+
if err != nil {
900+
return xerrors.Errorf("failed to instantiate notification manager: %w", err)
901+
}
897902

898-
// nolint:gocritic // We need to run the manager in a notifier context.
899-
notificationsManager.Run(dbauthz.AsNotifier(ctx))
903+
// nolint:gocritic // We need to run the manager in a notifier context.
904+
notificationsManager.Run(dbauthz.AsNotifier(ctx))
900905

901-
// Run report generator to distribute periodic reports.
902-
notificationReportGenerator := reports.NewReportGenerator(ctx, logger.Named("notifications.report_generator"), options.Database, options.NotificationsEnqueuer, quartz.NewReal())
903-
defer notificationReportGenerator.Close()
906+
// Run report generator to distribute periodic reports.
907+
notificationReportGenerator := reports.NewReportGenerator(ctx, logger.Named("notifications.report_generator"), options.Database, options.NotificationsEnqueuer, quartz.NewReal())
908+
defer notificationReportGenerator.Close()
909+
} else {
910+
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.")
911+
}
904912

905913
// Since errCh only has one buffered slot, all routines
906914
// 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.
10771085
// Cancel any remaining in-flight requests.
10781086
shutdownConns()
10791087

1080-
// Stop the notification manager, which will cause any buffered updates to the store to be flushed.
1081-
// If the Stop() call times out, messages that were sent but not reflected as such in the store will have
1082-
// their leases expire after a period of time and will be re-queued for sending.
1083-
// See CODER_NOTIFICATIONS_LEASE_PERIOD.
1084-
cliui.Info(inv.Stdout, "Shutting down notifications manager..."+"\n")
1085-
err = shutdownWithTimeout(notificationsManager.Stop, 5*time.Second)
1086-
if err != nil {
1087-
cliui.Warnf(inv.Stderr, "Notifications manager shutdown took longer than 5s, "+
1088-
"this may result in duplicate notifications being sent: %s\n", err)
1089-
} else {
1090-
cliui.Info(inv.Stdout, "Gracefully shut down notifications manager\n")
1088+
if notificationsManager != nil {
1089+
// Stop the notification manager, which will cause any buffered updates to the store to be flushed.
1090+
// If the Stop() call times out, messages that were sent but not reflected as such in the store will have
1091+
// their leases expire after a period of time and will be re-queued for sending.
1092+
// See CODER_NOTIFICATIONS_LEASE_PERIOD.
1093+
cliui.Info(inv.Stdout, "Shutting down notifications manager..."+"\n")
1094+
err = shutdownWithTimeout(notificationsManager.Stop, 5*time.Second)
1095+
if err != nil {
1096+
cliui.Warnf(inv.Stderr, "Notifications manager shutdown took longer than 5s, "+
1097+
"this may result in duplicate notifications being sent: %s\n", err)
1098+
} else {
1099+
cliui.Info(inv.Stdout, "Gracefully shut down notifications manager\n")
1100+
}
10911101
}
10921102

10931103
// Shut down provisioners before waiting for WebSockets

‎cli/testdata/coder_server_--help.golden

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ Configure how emails are sent.
118118
--email-hello string, $CODER_EMAIL_HELLO (default: localhost)
119119
The hostname identifying the SMTP server.
120120

121-
--email-smarthost host:port, $CODER_EMAIL_SMARTHOST (default: localhost:587)
121+
--email-smarthost string, $CODER_EMAIL_SMARTHOST
122122
The intermediary SMTP host through which emails are sent.
123123

124124
EMAIL / EMAIL AUTHENTICATION OPTIONS:
@@ -413,7 +413,7 @@ Configure how email notifications are sent.
413413
The hostname identifying the SMTP server.
414414
DEPRECATED: Use --email-hello instead.
415415

416-
--notifications-email-smarthost host:port, $CODER_NOTIFICATIONS_EMAIL_SMARTHOST
416+
--notifications-email-smarthost string, $CODER_NOTIFICATIONS_EMAIL_SMARTHOST
417417
The intermediary SMTP host through which emails are sent.
418418
DEPRECATED: Use --email-smarthost instead.
419419

‎cli/testdata/server-config.yaml.golden

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -524,8 +524,8 @@ email:
524524
# (default: <unset>, type: string)
525525
from: ""
526526
# The intermediary SMTP host through which emails are sent.
527-
# (default: localhost:587, type: host:port)
528-
smarthost: localhost:587
527+
# (default: <unset>, type: string)
528+
smarthost: ""
529529
# The hostname identifying the SMTP server.
530530
# (default: localhost, type: string)
531531
hello: localhost
@@ -577,8 +577,8 @@ notifications:
577577
# (default: <unset>, type: string)
578578
from: ""
579579
# The intermediary SMTP host through which emails are sent.
580-
# (default: <unset>, type: host:port)
581-
smarthost: localhost:587
580+
# (default: <unset>, type: string)
581+
smarthost: ""
582582
# The hostname identifying the SMTP server.
583583
# (default: <unset>, type: string)
584584
hello: localhost

‎coderd/apidoc/docs.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/notifications/dispatch/smtp.go

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,10 @@ import (
3434
)
3535

3636
var (
37-
ValidationNoFromAddressErr = xerrors.New("no 'from' address defined")
38-
ValidationNoToAddressErr = xerrors.New("no 'to' address(es) defined")
39-
ValidationNoSmarthostHostErr = xerrors.New("smarthost 'host' is not defined, or is invalid")
40-
ValidationNoSmarthostPortErr = xerrors.New("smarthost 'port' is not defined, or is invalid")
41-
ValidationNoHelloErr = xerrors.New("'hello' not defined")
37+
ValidationNoFromAddressErr = xerrors.New("'from' address not defined")
38+
ValidationNoToAddressErr = xerrors.New("'to' address(es) not defined")
39+
ValidationNoSmarthostErr = xerrors.New("'smarthost' address not defined")
40+
ValidationNoHelloErr = xerrors.New("'hello' not defined")
4241

4342
//go:embed smtp/html.gotmpl
4443
htmlTemplate string
@@ -521,15 +520,14 @@ func (s *SMTPHandler) validateToAddrs(to string) ([]string, error) {
521520
// Does not allow overriding.
522521
// nolint:revive // documented.
523522
func (s *SMTPHandler) smarthost() (string, string, error) {
524-
host := s.cfg.Smarthost.Host
525-
port := s.cfg.Smarthost.Port
526-
527-
// We don't validate the contents themselves; this will be done by the underlying SMTP library.
528-
if host == "" {
529-
return "", "", ValidationNoSmarthostHostErr
523+
smarthost := strings.TrimSpace(string(s.cfg.Smarthost))
524+
if smarthost == "" {
525+
return "", "", ValidationNoSmarthostErr
530526
}
531-
if port == "" {
532-
return "", "", ValidationNoSmarthostPortErr
527+
528+
host, port, err := net.SplitHostPort(string(s.cfg.Smarthost))
529+
if err != nil {
530+
return "", "", xerrors.Errorf("split host port: %w", err)
533531
}
534532

535533
return host, port, nil

‎coderd/notifications/dispatch/smtp_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,7 @@ func TestSMTP(t *testing.T) {
440440

441441
var hp serpent.HostPort
442442
require.NoError(t, hp.Set(listen.Addr().String()))
443-
tc.cfg.Smarthost = hp
443+
tc.cfg.Smarthost = serpent.String(hp.String())
444444

445445
handler := dispatch.NewSMTPHandler(tc.cfg, logger.Named("smtp"))
446446

‎coderd/notifications/notifications_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ func TestSMTPDispatch(t *testing.T) {
154154
cfg := defaultNotificationsConfig(method)
155155
cfg.SMTP = codersdk.NotificationsEmailConfig{
156156
From: from,
157-
Smarthost: serpent.HostPort{Host: "localhost", Port: fmt.Sprintf("%d", mockSMTPSrv.PortNumber())},
157+
Smarthost: serpent.String(fmt.Sprintf("localhost:%d", mockSMTPSrv.PortNumber())),
158158
Hello: "localhost",
159159
}
160160
handler := newDispatchInterceptor(dispatch.NewSMTPHandler(cfg.SMTP, logger.Named("smtp")))
@@ -1112,7 +1112,7 @@ func TestNotificationTemplates_Golden(t *testing.T) {
11121112

11131113
var hp serpent.HostPort
11141114
require.NoError(t, hp.Set(listen.Addr().String()))
1115-
smtpConfig.Smarthost = hp
1115+
smtpConfig.Smarthost = serpent.String(hp.String())
11161116

11171117
// Start mock SMTP server in the background.
11181118
var wg sync.WaitGroup
@@ -1523,7 +1523,7 @@ func TestCustomNotificationMethod(t *testing.T) {
15231523
cfg.SMTP = codersdk.NotificationsEmailConfig{
15241524
From: "danny@coder.com",
15251525
Hello: "localhost",
1526-
Smarthost: serpent.HostPort{Host: "localhost", Port: fmt.Sprintf("%d", mockSMTPSrv.PortNumber())},
1526+
Smarthost: serpent.String(fmt.Sprintf("localhost:%d", mockSMTPSrv.PortNumber())),
15271527
}
15281528
cfg.Webhook = codersdk.NotificationsWebhookConfig{
15291529
Endpoint: *serpent.URLOf(endpoint),

‎codersdk/deployment.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -686,11 +686,15 @@ type NotificationsConfig struct {
686686
Webhook NotificationsWebhookConfig `json:"webhook" typescript:",notnull"`
687687
}
688688

689+
func (n *NotificationsConfig) Enabled() bool {
690+
return n.SMTP.Smarthost != "" || n.Webhook.Endpoint != serpent.URL{}
691+
}
692+
689693
type NotificationsEmailConfig struct {
690694
// The sender's address.
691695
From serpent.String `json:"from" typescript:",notnull"`
692696
// The intermediary SMTP host through which emails are sent (host:port).
693-
Smarthost serpent.HostPort `json:"smarthost" typescript:",notnull"`
697+
Smarthost serpent.String `json:"smarthost" typescript:",notnull"`
694698
// The hostname identifying the SMTP server.
695699
Hello serpent.String `json:"hello" typescript:",notnull"`
696700

@@ -1028,7 +1032,6 @@ when required by your organization's security policy.`,
10281032
Description: "The intermediary SMTP host through which emails are sent.",
10291033
Flag: "email-smarthost",
10301034
Env: "CODER_EMAIL_SMARTHOST",
1031-
Default: "localhost:587", // To pass validation.
10321035
Value: &c.Notifications.SMTP.Smarthost,
10331036
Group: &deploymentGroupEmail,
10341037
YAML: "smarthost",

‎codersdk/deployment_test.go

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,3 +568,69 @@ func TestPremiumSuperSet(t *testing.T) {
568568
require.NotContains(t, enterprise.Features(), "", "enterprise should not contain empty string")
569569
require.NotContains(t, premium.Features(), "", "premium should not contain empty string")
570570
}
571+
572+
func TestNotificationsCanBeDisabled(t *testing.T) {
573+
t.Parallel()
574+
575+
tests := []struct {
576+
name string
577+
expectNotificationsEnabled bool
578+
environment []serpent.EnvVar
579+
}{
580+
{
581+
name: "NoDeliveryMethodSet",
582+
environment: []serpent.EnvVar{},
583+
expectNotificationsEnabled: false,
584+
},
585+
{
586+
name: "SMTP_DeliveryMethodSet",
587+
environment: []serpent.EnvVar{
588+
{
589+
Name: "CODER_EMAIL_SMARTHOST",
590+
Value: "localhost:587",
591+
},
592+
},
593+
expectNotificationsEnabled: true,
594+
},
595+
{
596+
name: "Webhook_DeliveryMethodSet",
597+
environment: []serpent.EnvVar{
598+
{
599+
Name: "CODER_NOTIFICATIONS_WEBHOOK_ENDPOINT",
600+
Value: "https://example.com/webhook",
601+
},
602+
},
603+
expectNotificationsEnabled: true,
604+
},
605+
{
606+
name: "WebhookAndSMTP_DeliveryMethodSet",
607+
environment: []serpent.EnvVar{
608+
{
609+
Name: "CODER_NOTIFICATIONS_WEBHOOK_ENDPOINT",
610+
Value: "https://example.com/webhook",
611+
},
612+
{
613+
Name: "CODER_EMAIL_SMARTHOST",
614+
Value: "localhost:587",
615+
},
616+
},
617+
expectNotificationsEnabled: true,
618+
},
619+
}
620+
621+
for _, tt := range tests {
622+
tt := tt
623+
624+
t.Run(tt.name, func(t *testing.T) {
625+
t.Parallel()
626+
627+
dv := codersdk.DeploymentValues{}
628+
opts := dv.Options()
629+
630+
err := opts.ParseEnv(tt.environment)
631+
require.NoError(t, err)
632+
633+
require.Equal(t, tt.expectNotificationsEnabled, dv.Notifications.Enabled())
634+
})
635+
}
636+
}

‎docs/admin/monitoring/notifications/index.md

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@ flags.
7474
Notifications can currently be delivered by either SMTP or webhook. Each message
7575
can only be delivered to one method, and this method is configured globally with
7676
[`CODER_NOTIFICATIONS_METHOD`](../../../reference/cli/server.md#--notifications-method)
77-
(default: `smtp`).
77+
(default: `smtp`). When there are no delivery methods configured, notifications
78+
will be disabled.
7879

7980
Premium customers can configure which method to use for each of the supported
8081
[Events](#workspace-events); see the
@@ -89,11 +90,11 @@ existing one.
8990

9091
**Server Settings:**
9192

92-
| Required | CLI | Env | Type | Description | Default |
93-
| :------: | ------------------- | ----------------------- | ----------- | ----------------------------------------- | ------------- |
94-
| ✔️ | `--email-from` | `CODER_EMAIL_FROM` | `string` | The sender's address to use. | |
95-
| ✔️ | `--email-smarthost` | `CODER_EMAIL_SMARTHOST` | `host:port` | The SMTP relay to send messages through. | localhost:587 |
96-
| ✔️ | `--email-hello` | `CODER_EMAIL_HELLO` | `string` | The hostname identifying the SMTP server. | localhost |
93+
| Required | CLI | Env | Type | Description | Default |
94+
| :------: | ------------------- | ----------------------- | -------- | ----------------------------------------- | --------- |
95+
| ✔️ | `--email-from` | `CODER_EMAIL_FROM` | `string` | The sender's address to use. | |
96+
| ✔️ | `--email-smarthost` | `CODER_EMAIL_SMARTHOST` | `string` | The SMTP relay to send messages |
97+
| ✔️ | `--email-hello` | `CODER_EMAIL_HELLO` | `string` | The hostname identifying the SMTP server. | localhost |
9798

9899
**Authentication Settings:**
99100

‎docs/reference/api/general.md

Lines changed: 1 addition & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)