feat: Add multi-channel notification system for IP bans #174
Conversation
dkhalizov
changed the title
tomMoulard
left a comment
tomMoulard
left a comment
There was a problem hiding this comment.
Hello @gulihuligagaga,
Thanks for your interest in this Traefik plugin !
I just have a few nitpicks as a review. I am also a bit scared of the notifying system: I would prefer to have actual clients instead of doing the request “by hand”. Do you think you could incorporate clients to do the request instead of here directly ?
Besides, I still need to test this behaviour in a real life experiment when I'll do a proper review !
Hi @tomMoulard, Thank you for the review! I appreciate you taking the time to look at this PR. Regarding the notification implementation, I considered using SDKs but ultimately opted for direct HTTP requests for these reasons: Minimal Dependencies: The current approach only uses Go's standard net/http package, keeping dependencies lean (0 new dependencies added) Simplicity: For basic webhook interactions and SMTP, the implementation is straightforward (avg 50-75 lines per notifier) |
|
Indeed, for code simplicity sake, it is better to use direct clients. But for a maintaining standpoint, using libraries is better ! It allows dependabot to update each library independently to have the correct version of things, as it deports the complexity of being up-to-date with the API to the foreign library. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a multi-channel notification system to the Fail2Ban middleware, allowing ban and unban events to be sent via Telegram, Discord, Email, or custom webhooks with customizable templates.
- Adds a new
notificationspackage housing aServiceabstraction and multiple notifier implementations. - Wires the notification service into the core
Fail2Banlogic and middleware initialization. - Updates documentation and tests to cover the new notification features.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/notifications/webhook.go | Implements WebhookNotifier with HTTP delivery logic |
| pkg/notifications/templates.go | Adds TemplateHandler for managing Go templates |
| pkg/fail2ban/fail2ban.go | Triggers notifications on ban/unban within ShouldAllow |
| fail2ban.go | Initializes and starts the notification service |
| README.md | Documents notification channels, configuration, and templates |
Comments suppressed due to low confidence (1)
README.md:275
- [nitpick] This heading is empty and duplicates the later '### Templates' section. Remove or populate it to avoid confusion.
### Default Templates
- Implement notification support for Telegram, Discord, Email and custom webhooks - Add configuration options for each notification channel - Support customizable message templates using Go template syntax - Update fail2ban core to trigger notifications on ban/unban events - Add tests and documentation for notification features (cherry picked from commit 4efa22e)
- Implement notification support for Telegram, Discord, Email and custom webhooks - Add configuration options for each notification channel - Support customizable message templates using Go template syntax - Update fail2ban core to trigger notifications on ban/unban events - Add tests and documentation for notification features (cherry picked from commit 4efa22e)
…ng and reliability - Add rich Discord embeds with color-coded events - Implement persistent SMTP connections with TLS - Add template customization for all channels - Update README.md
related to tomMoulard#120
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
tomMoulard
force-pushed
the
notifications
branch
from
6c4c797 to
26378aa
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a notifications subsystem (events, templates, notifiers, service) with Telegram, Discord, Email, and Webhook implementations, integrates service initialization into the fail2ban middleware, updates constructor signatures and tests, and expands README with Notifications documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Middleware as Fail2Ban Middleware
participant Service as Notifications Service
participant Template as Template Handler
participant Notifiers as Notifiers
participant External as External APIs/SMTP
Middleware->>Service: Notify(BanEvent)
activate Service
Service->>Service: If event type allowed -> enqueue
Service-->>Middleware: return
deactivate Service
Service->>Service: Run() drains channel
activate Service
loop for each event from channel
Service->>Template: RenderTemplate(event)
activate Template
Template-->>Service: rendered payload
deactivate Template
Service->>Notifiers: for each notifier Send(ctx,event)
activate Notifiers
Notifiers->>External: HTTP POST / SMTP send
activate External
External-->>Notifiers: response / status
deactivate External
Notifiers-->>Service: success or error
deactivate Notifiers
end
Service->>Service: log results
deactivate Service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @.golangci.yml:
- Around line 91-94: The exclude-rules entry uses the wrong key name so the mnd
rule for the "Magic number: 2, in <condition> detected" pattern is not applied;
open the .golangci.yml exclude-rules block, find the rule that currently uses
file: and replace that key with path:, ensuring the rule targets the same
pattern and linter (mnd) so the exclusion takes effect.
In `@fail2ban.go`:
- Around line 141-144: Replace the use of context.WithoutCancel when starting
the notifications service: call notifSrvc.Run with the original ctx (not
context.WithoutCancel(ctx)) so cancellation propagates; then update
notifications.Service.Run (or the concrete Run implementation referenced by
notifications.NewService) to observe ctx.Done() and return/close its event
channel when cancelled to avoid goroutine/channel leaks.
In `@pkg/fail2ban/fail2ban.go`:
- Around line 16-23: The Fail2Ban struct's notifSrv field is never used—update
the decision points in methods ShouldAllow and IsNotBanned to call
notifSrv.Notify(...) when a ban or unban is decided: on detecting a ban in
ShouldAllow call f.notifSrv.Notify(BanEvent{IP: ip, Rule: ruleName, Reason:
...}) (or equivalent fields used by your Event types), and on lifting a ban in
IsNotBanned call f.notifSrv.Notify(UnbanEvent{IP: ip, Rule: ruleName}); ensure
you nil-check f.notifSrv before calling Notify to preserve existing behavior if
notifications are not configured and use the existing event types
(BanEvent/UnbanEvent) provided by the notifications package to pass IP, rule
identifier and any context needed.
In `@pkg/notifications/discord_test.go`:
- Around line 39-54: The test currently overwrites DiscordConfig.WebhookURL
unconditionally with server.URL (in the test loop), so the "invalid webhook URL"
and "empty webhook URL" cases never exercise malformed/empty inputs; modify the
table-driven tests to add a boolean flag (e.g., overrideWebhook or useServerURL)
to each case and only assign tc.config.WebhookURL = server.URL inside the loop
when that flag is true; update the failing cases to leave their WebhookURL as-is
(invalid or empty) and set overrideWebhook=true for cases that should use the
test server, so the notifier's validation logic in the tests for invalid/empty
URLs actually runs.
In `@pkg/notifications/email.go`:
- Around line 126-155: createSMTPClient leaks the underlying TCP connection when
smtp.NewClient fails and leaks the SMTP client when StartTLS or Auth fail;
ensure you close resources on those error paths: if smtp.NewClient returns an
error call conn.Close(); after successfully creating client, on any subsequent
error (StartTLS or Auth) call client.Close() (and if necessary conn.Close())
before returning the error; alternatively establish a short-lived deferred
cleanup (e.g., defer func(){ if err != nil { client.Close(); conn.Close() } }())
that cancels on success so the returned client/connection are not leaked.
In `@pkg/notifications/events_test.go`:
- Around line 45-48: The subtests call t.Parallel() while closing over the loop
variable `test`, causing all parallel subtests to see the final iteration;
rebind the loop value before the closure (e.g., assign `tc := test`) and use
that `tc` inside the t.Run anonymous func (the block that calls t.Parallel()) so
each subtest uses its own copy of the test case when running in parallel.
In `@pkg/notifications/service_test.go`:
- Around line 3-10: The test has a data race on the boolean variable `notified`
which is written by the notifier goroutine and read by the test goroutine;
replace the unsynchronized access with a concurrency-safe mechanism (e.g., use
an atomic boolean via the sync/atomic package or synchronize reads/writes by
sending a signal on a channel) so all writes in the notifier (the goroutine
created in the test) use atomic.StoreUint32/atomic.StoreInt32 (or channel sends)
and all reads in the test use atomic.LoadUint32/atomic.LoadInt32 (or channel
receives); update occurrences of `notified` (and any checks at lines around
162-167 and 197) to use the chosen atomic or channel-based pattern.
In `@pkg/notifications/service.go`:
- Around line 34-42: The Run method currently blocks reading from s.ch and
ignores cancellation; update Service.Run to be context-aware by replacing the
plain range with a select loop that listens on both ctx.Done() and s.ch: in the
loop select on case <-ctx.Done(): return to stop the goroutine on cancellation,
and case event, ok := <-s.ch: to handle normal events (return if !ok). Keep the
existing notifier iteration (for _, n := range s.notifiers { if err :=
n.Send(ctx, event) ... }) unchanged so Send calls still occur when events are
received.
- Around line 24-32: Notify can block when s.ch is full; update Service.Notify
to perform a non-blocking send instead of a plain blocking send to avoid
stalling callers. Inside Service.Notify (which iterates s.allowedTypes and
compares string(event.Type)), replace the direct send to s.ch with a select that
attempts to send event on s.ch and falls through to a default case that drops
the event and logs or increments a dropped counter (use your logger or add a
dropped metric) so overflow events are observed; keep the existing early return
behavior on successful send.
In `@pkg/notifications/telegram_test.go`:
- Around line 32-35: The subtests use t.Run with t.Parallel() but capture the
loop variable "test", causing all subtests to see the last iteration; rebind the
loop variable before the closure (e.g., create a new local variable like "tc :=
test") and then use that "tc" inside the t.Run anonymous func and its t.Parallel
call so each subtest gets its own copy of the test case.
♻️ Duplicate comments (3)
README.md (1)
275-279: Fill or remove the empty “Default Templates” section.Line 275 introduces the heading but no content follows; it’s also overlapping with the later “Templates” defaults. Consider either adding the defaults here or removing the header.
pkg/notifications/discord.go (1)
12-15: Discord avatar URL field is misnamedDiscord webhooks expect
avatar_url. The current JSON tag usesavatarUrl, so the avatar will likely be ignored.✅ Suggested fix
type DiscordWebhookPayload struct { Username string `json:"username"` - AvatarURL string `json:"avatarUrl"` + AvatarURL string `json:"avatar_url"` Content string `json:"content"` Embeds []DiscordEmbed `json:"embeds"` }Discord webhook payload avatar_url field namepkg/notifications/templates.go (1)
61-65: Returning nil can cause nil-pointer panic in RenderTemplate.If the default template fails to parse (unlikely but possible),
makeTmplOrDefaultreturnsnil, which will cause a panic at line 78 whentmpl.Executeis called. Consider returning an error fromNewTemplateHandleror using a hardcoded fallback that cannot fail.🐛 Suggested nil check in RenderTemplate
func (th *TemplateHandler) RenderTemplate(event Event) (string, error) { tmpl, ok := th.templates[event.Type] - if !ok { - return "", fmt.Errorf("no template found for %s", event.Type) + if !ok || tmpl == nil { + return "", fmt.Errorf("no valid template found for %s", event.Type) } var buf bytes.Buffer
🧹 Nitpick comments (6)
pkg/notifications/email_test.go (1)
91-123: Consider skipping the main Send call for the concurrent test case.The "concurrent connections" test case sends an email in the main test loop (line 164) before the
validatefunction runs its 3 concurrent sends. If the intent is purely to test concurrent behavior, consider either:
- Adding
expectError: falsecheck only invalidate, or- Skipping the main
Sendcall whenvalidateis presentThis is a minor clarity improvement—the current behavior still correctly tests concurrency.
Also applies to: 162-164
pkg/fail2ban/fail2ban.go (1)
91-93: Redundant Printf pattern.The
fmt.Printf("%s", msg)call on line 93 can be simplified. Ifmsgis only used for logging here, consider usingfmt.Print(msg)or reverting to the direct format string. If the variable is prepared for future notification dispatch, add a TODO comment to clarify intent.Suggested simplification
- msg := fmt.Sprintf("%q is banned for %d>=%d request", - remoteIP, ip.Count+1, u.rules.MaxRetry) - fmt.Printf("%s", msg) + fmt.Printf("%q is banned for %d>=%d request", + remoteIP, ip.Count+1, u.rules.MaxRetry)pkg/notifications/webhook_test.go (1)
58-67: Test name "timeout" is misleading.This test case validates that a 504 Gateway Timeout response results in an error, but it doesn't test actual client-side timeout behavior (e.g., context deadline exceeded). The
Headers: map[string]string{"Timeout": "1"}isn't used by the mock server. Consider renaming to "server timeout response" or "504 status code" for clarity.pkg/notifications/webhook.go (2)
18-26: Consider validating method and URL in constructor.The constructor does not validate
cfg.Methodorcfg.URL. An empty or invalid HTTP method will causehttp.NewRequestWithContextto fail at runtime, and an empty URL will produce confusing errors.♻️ Suggested validation
func NewWebhookNotifier(cfg WebhookConfig, templates *TemplateHandler, httpCli *http.Client) *WebhookNotifier { + method := cfg.Method + if method == "" { + method = http.MethodPost + } return &WebhookNotifier{ url: cfg.URL, - method: cfg.Method, + method: method, headers: cfg.Headers, httpCli: httpCli, templates: templates, } }
39-44: User headers can override Content-Type.User-provided headers are applied after the defaults, allowing override of
Content-Type. If the template always produces JSON, this could lead to mismatches. If this is intentional (to support non-JSON payloads), consider documenting it; otherwise, apply user headers first or excludeContent-Typefrom override.pkg/notifications/templates.go (1)
50-54: Logging on every missing template may be noisy.When users don't provide custom templates (the common case), this logs for each event type during initialization. Consider using debug-level logging or only logging when a custom template was provided but failed to parse.
2 participants
Closes #50
Overview
This PR implements a notification system for the fail2ban middleware that sends alerts when IPs are banned or unbanned. The implementation supports multiple notification channels and allows for customizable message templates.
Features
Configuration Example
Implementation Details
notificationspackage with support for multiple notification channelsTesting
Documentation
Checklist
Related Issues
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.