Skip to content

feat: implement thin vertical slice of system-generated notifications #13537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 30 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
53c9cbb
feat: system-generated notifications
dannykopping Jun 11, 2024
4856aed
Fixing lint errors & minor tests
dannykopping Jun 11, 2024
cda6efb
Fixing dbauthz test
dannykopping Jun 11, 2024
86f937a
TestBufferedUpdates does not need a real db, altering test details sl…
dannykopping Jun 11, 2024
e8f1af2
Correct TestBufferedUpdates to count updated entries, use real db again
dannykopping Jun 12, 2024
a056f54
Use UUID for notifier IDs
dannykopping Jun 27, 2024
8c64d30
Small improvements from review suggestions
dannykopping Jun 27, 2024
ac149ec
Protect notifiers from modification during Stop()
dannykopping Jun 27, 2024
884fadf
Split out enqueuer as separate responsibility, get rid of singleton
dannykopping Jun 28, 2024
4e362e7
Remove unnecessary handler registry
dannykopping Jun 28, 2024
8097290
Remove unused context
dannykopping Jun 28, 2024
1b841ad
Centralise markdown rendering
dannykopping Jun 28, 2024
61f5bd6
Appease the linter
dannykopping Jun 28, 2024
3c8e33b
Only enqueue notification when not initiated by self
dannykopping Jul 1, 2024
757327c
Hide config flags which are unlikely to be modified by operators
dannykopping Jul 1, 2024
6f909ae
Remove unnecessary Labels struct
dannykopping Jul 1, 2024
36698c5
Enable experiment as safe
dannykopping Jul 1, 2024
c5701a6
Correcting bad refactor
dannykopping Jul 1, 2024
9d4c312
Initialize Enqueuer on API startup
dannykopping Jul 1, 2024
9380d8e
Only start one notifier since all dispatches are concurrent anyway
dannykopping Jul 1, 2024
4b7214d
Fix docs
dannykopping Jul 1, 2024
6679ef1
Fix lint error
dannykopping Jul 1, 2024
337997d
Merge branch 'main' of github.com:/coder/coder into dk/system-notific…
dannykopping Jul 2, 2024
ba5f7c6
Merge branch 'main' of github.com:/coder/coder into dk/system-notific…
dannykopping Jul 3, 2024
0f29293
Review feedback
dannykopping Jul 3, 2024
7c6c486
Merge branch 'main' of github.com:/coder/coder into dk/system-notific…
dannykopping Jul 4, 2024
c6e75c2
Fix lint failures
dannykopping Jul 4, 2024
aff9e6c
Review comments
dannykopping Jul 4, 2024
613e074
Avoid race by exposing number of pending updates
dannykopping Jul 4, 2024
faea7fc
Merge branch 'main' of github.com:/coder/coder into dk/system-notific…
dannykopping Jul 8, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ linters-settings:
- name: var-naming
- name: waitgroup-by-value

# irrelevant as of Go v1.22: https://go.dev/blog/loopvar-preview
govet:
disable:
- loopclosure

issues:
# Rules listed here: https://github.com/securego/gosec#available-rules
exclude-rules:
Expand Down
70 changes: 64 additions & 6 deletions cli/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ import (

"cdr.dev/slog"
"cdr.dev/slog/sloggers/sloghuman"
"github.com/coder/pretty"
"github.com/coder/retry"
"github.com/coder/serpent"
"github.com/coder/wgtunnel/tunnelsdk"

"github.com/coder/coder/v2/buildinfo"
"github.com/coder/coder/v2/cli/clilog"
"github.com/coder/coder/v2/cli/cliui"
Expand All @@ -64,6 +69,7 @@ import (
"github.com/coder/coder/v2/coderd/autobuild"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/awsiamrds"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbmem"
"github.com/coder/coder/v2/coderd/database/dbmetrics"
"github.com/coder/coder/v2/coderd/database/dbpurge"
Expand All @@ -73,6 +79,7 @@ import (
"github.com/coder/coder/v2/coderd/externalauth"
"github.com/coder/coder/v2/coderd/gitsshkey"
"github.com/coder/coder/v2/coderd/httpmw"
"github.com/coder/coder/v2/coderd/notifications"
"github.com/coder/coder/v2/coderd/oauthpki"
"github.com/coder/coder/v2/coderd/prometheusmetrics"
"github.com/coder/coder/v2/coderd/prometheusmetrics/insights"
Expand All @@ -97,10 +104,6 @@ import (
"github.com/coder/coder/v2/provisionersdk"
sdkproto "github.com/coder/coder/v2/provisionersdk/proto"
"github.com/coder/coder/v2/tailnet"
"github.com/coder/pretty"
"github.com/coder/retry"
"github.com/coder/serpent"
"github.com/coder/wgtunnel/tunnelsdk"
)

func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) {
Expand Down Expand Up @@ -592,6 +595,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
SSHConfigOptions: configSSHOptions,
},
AllowWorkspaceRenames: vals.AllowWorkspaceRenames.Value(),
NotificationsEnqueuer: notifications.NewNoopEnqueuer(), // Changed further down if notifications enabled.
}
if httpServers.TLSConfig != nil {
options.TLSCertificates = httpServers.TLSConfig.Certificates
Expand Down Expand Up @@ -660,6 +664,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
options.OIDCConfig = oc
}

experiments := coderd.ReadExperiments(
options.Logger, options.DeploymentValues.Experiments.Value(),
)

// We'll read from this channel in the select below that tracks shutdown. If it remains
// nil, that case of the select will just never fire, but it's important not to have a
// "bare" read on this channel.
Expand Down Expand Up @@ -969,6 +977,32 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
options.WorkspaceUsageTracker = tracker
defer tracker.Close()

// Manage notifications.
var (
notificationsManager *notifications.Manager
)
if experiments.Enabled(codersdk.ExperimentNotifications) {
cfg := options.DeploymentValues.Notifications

// The enqueuer is responsible for enqueueing notifications to the given store.
enqueuer, err := notifications.NewStoreEnqueuer(cfg, options.Database, templateHelpers(options), logger.Named("notifications.enqueuer"))
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(cfg, options.Database, logger.Named("notifications.manager"))
if err != nil {
return xerrors.Errorf("failed to instantiate notification manager: %w", err)
}

// nolint:gocritic // TODO: create own role.
notificationsManager.Run(dbauthz.AsSystemRestricted(ctx))
}

// Wrap the server in middleware that redirects to the access URL if
// the request is not to a local IP.
var handler http.Handler = coderAPI.RootHandler
Expand Down Expand Up @@ -1049,10 +1083,10 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
case <-stopCtx.Done():
exitErr = stopCtx.Err()
waitForProvisionerJobs = true
_, _ = io.WriteString(inv.Stdout, cliui.Bold("Stop caught, waiting for provisioner jobs to complete and gracefully exiting. Use ctrl+\\ to force quit"))
_, _ = io.WriteString(inv.Stdout, cliui.Bold("Stop caught, waiting for provisioner jobs to complete and gracefully exiting. Use ctrl+\\ to force quit\n"))
case <-interruptCtx.Done():
exitErr = interruptCtx.Err()
_, _ = io.WriteString(inv.Stdout, cliui.Bold("Interrupt caught, gracefully exiting. Use ctrl+\\ to force quit"))
_, _ = io.WriteString(inv.Stdout, cliui.Bold("Interrupt caught, gracefully exiting. Use ctrl+\\ to force quit\n"))
case <-tunnelDone:
exitErr = xerrors.New("dev tunnel closed unexpectedly")
case <-pubsubWatchdogTimeout:
Expand Down Expand Up @@ -1088,6 +1122,21 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
// Cancel any remaining in-flight requests.
shutdownConns()

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
// connections to close.
var wg sync.WaitGroup
Expand Down Expand Up @@ -1227,6 +1276,15 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
return serverCmd
}

// templateHelpers builds a set of functions which can be called in templates.
// We build them here to avoid an import cycle by using coderd.Options in notifications.Manager.
// We can later use this to inject whitelabel fields when app name / logo URL are overridden.
func templateHelpers(options *coderd.Options) map[string]any {
return map[string]any{
"base_url": func() string { return options.AccessURL.String() },
}
}

// printDeprecatedOptions loops through all command options, and prints
// a warning for usage of deprecated options.
func PrintDeprecatedOptions() serpent.MiddlewareFunc {
Expand Down
24 changes: 24 additions & 0 deletions cli/testdata/coder_server_--help.golden
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,30 @@ can safely ignore these settings.
Minimum supported version of TLS. Accepted values are "tls10",
"tls11", "tls12" or "tls13".

NOTIFICATIONS OPTIONS:
--notifications-dispatch-timeout duration, $CODER_NOTIFICATIONS_DISPATCH_TIMEOUT (default: 1m0s)
How long to wait while a notification is being sent before giving up.

--notifications-max-send-attempts int, $CODER_NOTIFICATIONS_MAX_SEND_ATTEMPTS (default: 5)
The upper limit of attempts to send a notification.

--notifications-method string, $CODER_NOTIFICATIONS_METHOD (default: smtp)
Which delivery method to use (available options: 'smtp', 'webhook').

NOTIFICATIONS / EMAIL OPTIONS:
--notifications-email-from string, $CODER_NOTIFICATIONS_EMAIL_FROM
The sender's address to use.

--notifications-email-hello string, $CODER_NOTIFICATIONS_EMAIL_HELLO (default: localhost)
The hostname identifying the SMTP server.

--notifications-email-smarthost host:port, $CODER_NOTIFICATIONS_EMAIL_SMARTHOST (default: localhost:587)
The intermediary SMTP host through which emails are sent.

NOTIFICATIONS / WEBHOOK OPTIONS:
--notifications-webhook-endpoint url, $CODER_NOTIFICATIONS_WEBHOOK_ENDPOINT
The endpoint to which to send webhooks.

OAUTH2 / GITHUB OPTIONS:
--oauth2-github-allow-everyone bool, $CODER_OAUTH2_GITHUB_ALLOW_EVERYONE
Allow all logins, setting this option means allowed orgs and teams
Expand Down
55 changes: 55 additions & 0 deletions cli/testdata/server-config.yaml.golden
Original file line number Diff line number Diff line change
Expand Up @@ -493,3 +493,58 @@ userQuietHoursSchedule:
# compatibility reasons, this will be removed in a future release.
# (default: false, type: bool)
allowWorkspaceRenames: false
notifications:
# Which delivery method to use (available options: 'smtp', 'webhook').
# (default: smtp, type: string)
method: smtp
# How long to wait while a notification is being sent before giving up.
# (default: 1m0s, type: duration)
dispatch-timeout: 1m0s
email:
# The sender's address to use.
# (default: <unset>, type: string)
from: ""
# The intermediary SMTP host through which emails are sent.
# (default: localhost:587, type: host:port)
smarthost: localhost:587
# The hostname identifying the SMTP server.
# (default: localhost, type: string)
hello: localhost
webhook:
# The endpoint to which to send webhooks.
# (default: <unset>, type: url)
hello:
# The upper limit of attempts to send a notification.
# (default: 5, type: int)
max-send-attempts: 5
# The minimum time between retries.
# (default: 5m0s, type: duration)
retry-interval: 5m0s
# The notifications system buffers message updates in memory to ease pressure on
# the database. This option controls how often it synchronizes its state with the
# database. The shorter this value the lower the change of state inconsistency in
# a non-graceful shutdown - but it also increases load on the database. It is
# recommended to keep this option at its default value.
# (default: 2s, type: duration)
store-sync-interval: 2s
# The notifications system buffers message updates in memory to ease pressure on
# the database. This option controls how many updates are kept in memory. The
# lower this value the lower the change of state inconsistency in a non-graceful
# shutdown - but it also increases load on the database. It is recommended to keep
# this option at its default value.
# (default: 50, type: int)
store-sync-buffer-size: 50
# How long a notifier should lease a message. This is effectively how long a
# notification is 'owned' by a notifier, and once this period expires it will be
# available for lease by another notifier. Leasing is important in order for
# multiple running notifiers to not pick the same messages to deliver
# concurrently. This lease period will only expire if a notifier shuts down
# ungracefully; a dispatch of the notification releases the lease.
# (default: 2m0s, type: duration)
lease-period: 2m0s
# How many notifications a notifier should lease per fetch interval.
# (default: 20, type: int)
lease-count: 20
# How often to query the database for queued notifications.
# (default: 15s, type: duration)
fetch-interval: 15s
101 changes: 99 additions & 2 deletions coderd/apidoc/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading