From 7ed656989413f09f74b9c4ebb231f2f903890b71 Mon Sep 17 00:00:00 2001 From: Adam Chalkley Date: Thu, 9 Jul 2020 18:04:13 -0500 Subject: [PATCH] Retry delay: Update flag, vars and docs to clarify BACKSTORY As I went to start work towards implementing email notification support per GH-3, I was struck by how unclear the arguments to the `emailNotifier` function (and similarly, the `teamsNotifier` function) were. The difference between `config.NotifyMgrEmailNotificationDelay` and `cfg.EmailNotificationDelay()` were not immediately clear and it took longer than I'd like to admit to fully backtrack and confirm not only the original intent, but the current behavior surrounding those settings. This commit attempts to clarify not only the original intent, but the purpose behind each related portion of code surrounding what I now understand to be handling the rate limit and notification retry delay. CHANGES - documentation - README.md - update "feature" entry to emphasize configurable retries, but hard-coded notifications rate limit (hopefully soon to change) - configure.md - updated description - updated command-line flag name - updated environment variable name - updated help text/description for flag - doc.go - explicitly note that it is the retry delay that is configurable - configuration file - rename `delay` to `retry_delay` - add missing doc comments for each msteams section setting - code - search/replace across codebase to replace `Delay` with `RetryDelay` - this includes renaming the `Delay` field for the `MSTeams` type, and its associated flag, environment variable and TOML config file setting - replace `sendDelay` parameter with `sendRateLimit` to better communicate what the incoming value to `teamsNotifier` and `emailNotifier` is intended for - doc comments - fixes to correct wrong information - fixes to better communicate the intent and real behavior REFERENCES - refs GH-85 --- README.md | 4 +-- cmd/brick/notify.go | 28 ++++++++++--------- config/config.go | 23 ++++++++++------ config/constants.go | 46 +++++++++++++++---------------- config/getters.go | 24 ++++++++-------- config/types.go | 6 ++-- config/validate.go | 8 +++--- contrib/brick/config.example.toml | 17 +++++++++++- doc.go | 2 +- docs/configure.md | 6 ++-- 10 files changed, 93 insertions(+), 71 deletions(-) diff --git a/README.md b/README.md index 6f62f91c..3cc16dac 100644 --- a/README.md +++ b/README.md @@ -114,8 +114,8 @@ See also: - ignored user - ignored IP Address - error occurred - - configurable retries - - configurable notifications delay in order to respect remote API limits + - configurable retries and delay between retries + - hard-coded notifications rate limit in order to respect remote API limits - Logging - Payload receipt from monitoring system diff --git a/cmd/brick/notify.go b/cmd/brick/notify.go index f3e6b0d3..c559a871 100644 --- a/cmd/brick/notify.go +++ b/cmd/brick/notify.go @@ -341,7 +341,7 @@ func teamsNotifier( ctx context.Context, webhookURL string, sendTimeout time.Duration, - sendDelay time.Duration, + sendRateLimit time.Duration, retries int, retriesDelay int, incoming <-chan events.Record, @@ -355,9 +355,10 @@ func teamsNotifier( ourResultQueue := make(chan NotifyResult) // Setup new scheduler that we can use to add an intentional delay between - // Microsoft Teams notification attempts + // Microsoft Teams notification attempts. This delay is added in order to + // rate limit our outgoing messages to comply with remote API limits. // https://docs.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/how-to/connectors-using - notifyScheduler := newNotifyScheduler(sendDelay) + notifyScheduler := newNotifyScheduler(sendRateLimit) for { @@ -388,7 +389,6 @@ func teamsNotifier( time.Now(), record) log.Debug("Calculating next scheduled notification") - nextScheduledNotification := notifyScheduler() log.Debugf("Now: %v, Next scheduled notification: %v", @@ -468,7 +468,7 @@ func teamsNotifier( func emailNotifier( ctx context.Context, sendTimeout time.Duration, - sendDelay time.Duration, + sendRateLimit time.Duration, retries int, retriesDelay int, incoming <-chan events.Record, @@ -482,8 +482,10 @@ func emailNotifier( ourResultQueue := make(chan NotifyResult) // Setup new scheduler that we can use to add an intentional delay between - // email notification attempts - notifyScheduler := newNotifyScheduler(sendDelay) + // email notification attempts. This delay is added in order to rate limit + // our outgoing messages to comply with any destination email server + // limits. + notifyScheduler := newNotifyScheduler(sendRateLimit) for { @@ -627,10 +629,10 @@ func NotifyMgr(ctx context.Context, cfg *config.Config, notifyWorkQueue <-chan e go teamsNotifier( ctx, cfg.TeamsWebhookURL(), - config.NotifyMgrTeamsTimeout, - config.NotifyMgrTeamsNotificationDelay, + config.NotifyMgrTeamsNotificationTimeout, + config.NotifyMgrTeamsNotificationRateLimit, cfg.TeamsNotificationRetries(), - cfg.TeamsNotificationDelay(), + cfg.TeamsNotificationRetryDelay(), teamsNotifyWorkQueue, teamsNotifyResultQueue, teamsNotifyDone, @@ -644,10 +646,10 @@ func NotifyMgr(ctx context.Context, cfg *config.Config, notifyWorkQueue <-chan e log.Debug("NotifyMgr: Starting up emailNotifier") go emailNotifier( ctx, - config.NotifyMgrEmailTimeout, - config.NotifyMgrEmailNotificationDelay, + config.NotifyMgrEmailNotificationTimeout, + config.NotifyMgrEmailNotificationRateLimit, cfg.EmailNotificationRetries(), - cfg.EmailNotificationDelay(), + cfg.EmailNotificationRetryDelay(), emailNotifyWorkQueue, emailNotifyResultQueue, emailNotifyDone, diff --git a/config/config.go b/config/config.go index 8142d2ab..2cdc0846 100644 --- a/config/config.go +++ b/config/config.go @@ -50,7 +50,7 @@ func (c *Config) String() string { "IgnoredIPAddressesFile: [%q %t],"+ "TeamsWebhookURL: %q,"+ "TeamsNotificationRetries: %q,"+ - "TeamsNotificationDelay: %q,"+ + "TeamsNotificationRetryDelay: %q,"+ "ConfigFile: %q}", c.LocalTCPPort(), c.LocalIPAddress(), @@ -67,7 +67,7 @@ func (c *Config) String() string { c.IsSetIgnoredIPAddressesFile(), c.TeamsWebhookURL(), c.TeamsNotificationRetries(), - c.TeamsNotificationDelay(), + c.TeamsNotificationRetryDelay(), c.ConfigFile(), ) } @@ -87,17 +87,24 @@ func (c configTemplate) Description() string { return MyAppDescription } -// GetNotificationTimeout accepts the next scheduled notification, the number -// of message submission retries and the delay between each attempt and -// returns the timeout value for the entire message submission process, -// including the initial attempt and all retry attempts. +// GetNotificationTimeout calculates the timeout value for the entire message +// submission process, including the initial attempt and all retry attempts. // // This overall timeout value is computed using multiple values; (1) the base // timeout value for a single message submission attempt, (2) the next // scheduled notification (which was created using the configured delay we // wish to force between message submission attempts), (3) the total number of -// retries allowed, (4) the delay between retry attempts -func GetNotificationTimeout(baseTimeout time.Duration, schedule time.Time, retries int, retriesDelay int) time.Duration { +// retries allowed, (4) the delay between each retry attempt. +// +// This computed timeout value is intended to be used to cancel a notification +// attempt once it reaches this timeout threshold. +// +func GetNotificationTimeout( + baseTimeout time.Duration, + schedule time.Time, + retries int, + retriesDelay int, +) time.Duration { timeoutValue := (baseTimeout + time.Until(schedule)) + (time.Duration(retriesDelay) * time.Duration(retries)) diff --git a/config/constants.go b/config/constants.go index 7b742f4c..f6800d62 100644 --- a/config/constants.go +++ b/config/constants.go @@ -69,7 +69,7 @@ const ( // the number of seconds to wait between retry attempts; applies to // Microsoft Teams notifications only - defaultMSTeamsDelay int = 5 + defaultMSTeamsRetryDelay int = 5 // this is based on the official installation instructions defaultEZproxyExecutablePath string = "/usr/local/ezproxy/ezproxy" @@ -119,21 +119,21 @@ const NotifyMgrServicesShutdownTimeout time.Duration = 2 * time.Second // and child goroutines to concurrently process notification requests. const ( - // NotifyMgrTeamsTimeout is the timeout setting applied to each Microsoft - // Teams notification attempt. This value does NOT take into account the - // number of configured retries and retry delays. The final value timeout - // applied to each notification attempt should be based on those - // calculations. The GetTimeout method does just that. - NotifyMgrTeamsTimeout time.Duration = 10 * time.Second - - // NotifyMgrTeamsSendAttemptTimeout - - // NotifyMgrEmailTimeout is the timeout setting applied to each email - // notification attempt. This value does NOT take into account the number - // of configured retries and retry delays. The final value timeout applied - // to each notification attempt should be based on those calculations. The - // GetTimeout method does just that. - NotifyMgrEmailTimeout time.Duration = 30 * time.Second + // NotifyMgrTeamsNotificationTimeout is the timeout setting applied to + // each Microsoft Teams notification attempt. This value does NOT take + // into account the number of configured retries, retry delays or overall + // rate limit for Teams notifications. The final timeout value applied to + // each notification attempt should be based on those calculations and + // this value. The GetNotificationTimeout method does just that. + NotifyMgrTeamsNotificationTimeout time.Duration = 10 * time.Second + + // NotifyMgrEmailNotificationTimeout is the timeout setting applied to + // each email notification attempt. This value does NOT take into account + // the number of configured retries, retry delays or overall rate limit + // for email notifications. The final timeout value applied to each + // notification attempt should be based on those calculations and this + // value. The GetNotificationTimeout method does just that. + NotifyMgrEmailNotificationTimeout time.Duration = 30 * time.Second // NotifyStatsMonitorDelay limits notification stats logging to no more // often than this duration. This limiter is to keep from logging the @@ -145,15 +145,15 @@ const ( // details so often that the information simply becomes noise. NotifyQueueMonitorDelay time.Duration = 15 * time.Second - // NotifyMgrTeamsNotificationDelay is the delay between Microsoft Teams - // notification attempts. This delay is intended to help prevent + // NotifyMgrTeamsNotificationRateLimit is the rate limit between Microsoft + // Teams notification attempts. This limit is intended to help prevent // unintentional abuse of remote services. - NotifyMgrTeamsNotificationDelay time.Duration = 5 * time.Second + NotifyMgrTeamsNotificationRateLimit time.Duration = 5 * time.Second - // NotifyMgrEmailNotificationDelay is the delay between email notification - // attempts. This delay is intended to help prevent unintentional abuse of - // remote services. - NotifyMgrEmailNotificationDelay time.Duration = 5 * time.Second + // NotifyMgrEmailNotificationRateLimit is the rate limit between email + // notification attempts. This limit is intended to help prevent + // unintentional abuse of remote services. + NotifyMgrEmailNotificationRateLimit time.Duration = 5 * time.Second ) // NotifyMgrQueueDepth is the number of items allowed into the queue/channel diff --git a/config/getters.go b/config/getters.go index f60b30e3..ce04643e 100644 --- a/config/getters.go +++ b/config/getters.go @@ -301,19 +301,18 @@ func (c Config) TeamsNotificationRetries() int { } } -// TeamsNotificationDelay returns the user-provided delay for Microsoft Teams -// notifications or the default value if not provided. CLI flag values take -// precedence if provided. This delay is added regardless of whether a -// previous notification delivery attempt has been made. -func (c Config) TeamsNotificationDelay() int { +// TeamsNotificationRetryDelay returns the user-provided delay between retry +// attempts for Microsoft Teams notifications or the default value if not +// provided. CLI flag values take precedence if provided. +func (c Config) TeamsNotificationRetryDelay() int { switch { - case c.cliConfig.MSTeams.Delay != nil: - return *c.cliConfig.MSTeams.Delay - case c.fileConfig.MSTeams.Delay != nil: - return *c.fileConfig.MSTeams.Delay + case c.cliConfig.MSTeams.RetryDelay != nil: + return *c.cliConfig.MSTeams.RetryDelay + case c.fileConfig.MSTeams.RetryDelay != nil: + return *c.fileConfig.MSTeams.RetryDelay default: - return defaultMSTeamsDelay + return defaultMSTeamsRetryDelay } } @@ -356,9 +355,8 @@ func (c Config) EmailNotificationRetries() int { // EmailNotificationRetries returns the user-provided delay for email // notifications or the default value if not provided. CLI flag values take -// precedence if provided. This delay is added regardless of whether a -// previous notification delivery attempt has been made. -func (c Config) EmailNotificationDelay() int { +// precedence if provided. +func (c Config) EmailNotificationRetryDelay() int { log.Warn("Implement this as part of GH-3") diff --git a/config/types.go b/config/types.go index ac1399fb..1a4c78ad 100644 --- a/config/types.go +++ b/config/types.go @@ -138,9 +138,9 @@ type MSTeams struct { // application. WebhookURL *string `toml:"webhook_url" arg:"--teams-webhook-url,env:BRICK_MSTEAMS_WEBHOOK_URL" help:"The Webhook URL provided by a preconfigured Connector. If specified, this application will attempt to send client request details to the Microsoft Teams channel associated with the webhook URL."` - // Delay is the number of seconds to wait between Microsoft Teams message - // delivery attempts. - Delay *int `toml:"delay" arg:"--teams-notify-delay,env:BRICK_MSTEAMS_WEBHOOK_DELAY" help:"The number of seconds to wait between Microsoft Teams message delivery attempts."` + // RetryDelay is the number of seconds to wait between Microsoft Teams + // message delivery retry attempts. + RetryDelay *int `toml:"retry_delay" arg:"--teams-notify-retry-delay,env:BRICK_MSTEAMS_WEBHOOK_RETRY_DELAY" help:"The number of seconds to wait between Microsoft Teams message delivery retry attempts."` // Retries is the number of attempts that this application will make to // deliver Microsoft Teams messages before giving up. diff --git a/config/validate.go b/config/validate.go index 7748ef14..c9530b69 100644 --- a/config/validate.go +++ b/config/validate.go @@ -153,11 +153,11 @@ func validate(c Config) error { } - if c.TeamsNotificationDelay() < 0 { - log.Debugf("unsupported delay specified for MS Teams notifications: %d ", c.TeamsNotificationDelay()) + if c.TeamsNotificationRetryDelay() < 0 { + log.Debugf("unsupported retry delay specified for MS Teams notifications: %d ", c.TeamsNotificationRetryDelay()) return fmt.Errorf( - "invalid delay specified for MS Teams notifications: %d", - c.TeamsNotificationDelay(), + "invalid retry delay specified for MS Teams notifications: %d", + c.TeamsNotificationRetryDelay(), ) } diff --git a/contrib/brick/config.example.toml b/contrib/brick/config.example.toml index 62fd76d1..a94c66e3 100644 --- a/contrib/brick/config.example.toml +++ b/contrib/brick/config.example.toml @@ -104,9 +104,24 @@ file_path = "/usr/local/etc/brick/ips.brick-ignored.txt" [msteams] +# The full URL used to submit messages to the Teams channel. This URL is in +# the form of +# +# https://outlook.office.com/webhook/xxx or +# https://outlook.office365.com/webhook/xxx. +# +# This URL needs to be created in advance by adding/configuring a Webhook +# Connector in a Microsoft Teams channel that you wish to submit messages to +# using this application. webhook_url = "" + +# The number of attempts that this application will make to deliver Microsoft +# Teams messages before giving up. retries = 2 -delay = 5 + +# The number of seconds to wait between Microsoft Teams message delivery retry +# attempts. +retry_delay = 5 # TODO: Add this in later diff --git a/doc.go b/doc.go index 53645fde..da9dccf4 100644 --- a/doc.go +++ b/doc.go @@ -54,7 +54,7 @@ FEATURES • User configurable support for ignoring specific IP Addresses (i.e., prevent disabling associated account) -• Microsoft Teams notifications generated for multiple events with configurable retries and notification delay +• Microsoft Teams notifications generated for multiple events with configurable retries and notification retry delay • Logging of all events (e.g., payload receipt, action taken due to payload) diff --git a/docs/configure.md b/docs/configure.md index eb0a6537..1f730faa 100644 --- a/docs/configure.md +++ b/docs/configure.md @@ -51,7 +51,7 @@ environment variables) and use the configuration file for the other settings. | `ignored-users-file` | No | `/usr/local/etc/brick/users.brick-ignored.txt` | No | *valid path to a file* | Fully-qualified path to the file containing a list of user accounts which should not be disabled and whose IP Address reported in the same alert should not be banned by this application. Leading and trailing whitespace per line is ignored. | | `ignored-ips-file` | No | `/usr/local/etc/brick/ips.brick-ignored.txt` | No | *valid path to a file* | Fully-qualified path to the file containing a list of individual IP Addresses which should not be disabled and whose user account reported in the same alert should not be disabled by this application. Leading and trailing whitespace per line is ignored. | | `teams-webhook-url` | No | *empty string* | No | [*valid webhook url*](#worth-noting) | The Webhook URL provided by a preconfigured Connector. If specified, this application will attempt to send client request details to the Microsoft Teams channel associated with the webhook URL. | -| `teams-notify-delay` | No | `5` | No | *valid whole number* | The number of seconds to wait between Microsoft Teams message delivery attempts. | +| `teams-notify-retry-delay` | No | `5` | No | *valid whole number* | The number of seconds to wait between Microsoft Teams message retry delivery attempts. | | `teams-notify-retries` | No | `2` | No | *valid whole number* | The number of attempts that this application will make to deliver Microsoft Teams messages before giving up. | | `ezproxy-executable-path` | No | `/usr/local/ezproxy/ezproxy` | No | *valid path to a file* | The fully-qualified path to the EZproxy executable/binary. This executable is usually named 'ezproxy' and is set to start at system boot. The fully-qualified path to this executable is required for session termination. | | `ezproxy-active-file-path` | No | `/usr/local/ezproxy/ezproxy.hst` | No | *valid path to a file* | The fully-qualified path to the Active Users and Hosts 'state' file used by EZproxy (and this application) to track current sessions and hosts managed by EZproxy. | @@ -84,7 +84,7 @@ Arguments](#command-line-arguments) table for more information. | `ignored-users-file` | `BRICK_IGNORED_USERS_FILE` | | `BRICK_IGNORED_USERS_FILE="/usr/local/etc/brick/users.brick-ignored.txt"` | | `ignored-ips-file` | `BRICK_IGNORED_IP_ADDRESSES_FILE` | | `BRICK_IGNORED_IP_ADDRESSES_FILE="/usr/local/etc/brick/ips.brick-ignored.txt"` | | `teams-webhook-url` | `BRICK_MSTEAMS_WEBHOOK_URL` | | `BRICK_MSTEAMS_WEBHOOK_URL="https://outlook.office.com/webhook/a1269812-6d10-44b1-abc5-b84f93580ba0@9e7b80c7-d1eb-4b52-8582-76f921e416d9/IncomingWebhook/3fdd6767bae44ac58e5995547d66a4e4/f332c8d9-3397-4ac5-957b-b8e3fc465a8c"` | -| `teams-notify-delay` | `BRICK_MSTEAMS_WEBHOOK_DELAY` | | `BRICK_MSTEAMS_WEBHOOK_DELAY="2"` | +| `teams-notify-retry-delay` | `BRICK_MSTEAMS_WEBHOOK_RETRY_DELAY` | | `BRICK_MSTEAMS_WEBHOOK_RETRY_DELAY="2"` | | `teams-notify-retries` | `BRICK_MSTEAMS_WEBHOOK_RETRIES` | | `BRICK_MSTEAMS_WEBHOOK_RETRIES="5"` | | `ezproxy-executable-path` | `BRICK_EZPROXY_EXECUTABLE_PATH` | | `BRICK_EZPROXY_EXECUTABLE_PATH="/usr/local/ezproxy/ezproxy"` | | `ezproxy-active-file-path` | `BRICK_EZPROXY_ACTIVE_FILE_PATH` | | `BRICK_EZPROXY_ACTIVE_FILE_PATH="/usr/local/ezproxy/ezproxy.hst"` | @@ -117,7 +117,7 @@ settings. | `ignored-users-file` | `file_path` | `ignoredusers` | | | `ignored-ips-file` | `file_path` | `ignoredipaddresses` | | | `teams-webhook-url` | `webhook_url` | `msteams` | | -| `teams-notify-delay` | `delay` | `msteams` | | +| `teams-notify-retry-delay` | `retry_delay` | `msteams` | | | `teams-notify-retries` | `retries` | `msteams` | | | `ezproxy-executable-path` | `executable_path` | `ezproxy` | | | `ezproxy-active-file-path` | `active_file_path` | `ezproxy` | |