search

prometheus / alertmanager

Prometheus Alertmanager
https://prometheus.io
Apache License 2.0
6.69k stars 2.17k forks source link

SMTP notifier does not await submission completion #4005

Closed dannykopping closed 3 months ago

dannykopping commented 3 months ago

I'd like to backport a fix from https://github.com/coder/coder/pull/14495. We took heavy inspiration from notify/email/email.go for our SMTP dispatch implementation, but we discovered a fairly nasty bug which is also present in this code.

tl;dr: the email notifier does not correctly close an SMTP submission, which may lead to unsuccessful dispatches being marked as successful.

The details are documented in the above PR.

I'll raise a PR to fix this, but I wanted to first check if the maintainers would accept a new approach towards SMTP testing. In coder/coder I introduced emersion/go-smtp to enable SMTP server mocking (see here). I'd like to do the same here because it'll simplify the tests and does not require maildev+docker.

If there's a way to provoke this behaviour with maildev, please let me know as that would be simpler - but from my cursory scan of the docs it did not seem possible.

PS miss you folks at Grafana Labs ❤️

gotjosh commented 3 months ago

hi @dannykopping ❤️ , miss you too buddy - go for it, we'd be thrilled to have it assuming you can test the same functionality.

dannykopping commented 3 months ago

Sweet, thanks @gotjosh!