Fix SMTP connection from_email being ignored by email_on_failure and …#69271
Fix SMTP connection from_email being ignored by email_on_failure and …#69271bramhanandlingala wants to merge 4 commits into
Conversation
SameerMesiah97
left a comment
There was a problem hiding this comment.
Only one clarification. Looks good otherwise.
| @@ -2058,50 +1951,13 @@ def _send_error_email_notification( | |||
| to=to_emails, | |||
| subject=subject, | |||
| html_content=html_content, | |||
| from_email=conf.get("email", "from_email", fallback="airflow@airflow"), | |||
| from_email=None, | |||
There was a problem hiding this comment.
Are there any other callers that still resolve from_email themselves before constructing the notifier? Please check.
There was a problem hiding this comment.
Good question — I checked. SmtpNotifier is only constructed in one production call site outside its own module, which is _send_error_email_notification in task_runner.py, and that's already updated in this PR to pass from_email=None. I didn't find any other operator or utility in the repo that resolves from_email from config before building the notifier, so this should cover all callers.
|
Hello @bramhanandlingala! Thanks for creating a PR related to my issue. I think your suggested changes make sense overall. I have one suggestion though: I also have some wording suggestions for the test comments, but these are maybe more personal preferences. |
| @@ -211,6 +211,70 @@ def test_notifier_with_nondefault_connection_extra( | |||
| **DEFAULT_EMAIL_PARAMS, | |||
| ) | |||
|
|
|||
| @mock.patch("airflow.providers.smtp.notifications.smtp.SmtpHook") | |||
| def test_notifier_from_email_falls_back_to_config(self, mock_smtphook_hook, create_dag_without_db): | |||
| """When from_email is unset and the connection defines none, `[email] from_email` config is used.""" | |||
There was a problem hiding this comment.
| """When from_email is unset and the connection defines none, `[email] from_email` config is used.""" | |
| """When config [email] from_email is set and the SMTP connection extra `from_email` is NOT set, `[email] from_email` config is used.""" |
I assume unset was a typing mistake as this is the test case where the config is set and the connection extra field is not set.
|
Hi @stephsi Good catch, thanks — that was indeed a typo, "unset" should've said the connection extra is not set. Fixed in the latest commit using your suggested wording exactly. I've also added a matching docstring to test_notifier_with_nondefault_connection_extra so all four tests (connection-only, config-only, connection-over-config, neither-set-default) are now consistent in style, per your other comment. Happy to take any further wording suggestions if you have them. |
|
Thanks @potiuk @stephsi, @bugraoz93, @choo121600, @ephraimbuddy, @henry3260, @jason810496, @rawwar, @kaxil, @ashb, @amoghrajesh, @Lee-W |
Summary
_send_error_email_notification(used foremail_on_failure/email_on_retry) resolved[email] from_emailbefore constructingSmtpNotifier, soSmtpNotifier's own connection-extra fallback branch never ran. This meant the SMTP connection'sfrom_emailextra was silently ignored whenever[email] from_emailwas set — or even when it wasn't, since a hardcodedairflow@airflowfallback was passed in regardless.This PR moves
from_emailprecedence resolution intoSmtpNotifieritself:from_emailargument (if passed directly toSmtpNotifier)from_email[email] from_emailconfigairflow@airflowdefaulttask_runner.pynow passesfrom_email=NonetoSmtpNotifierand lets it resolve precedence, instead of resolving[email] from_emailupfront and locking out the connection-extra check.Root cause
Since this was never
None,SmtpNotifier._build_email_content'sif self.from_email is None:branch — which checks the SMTP connection'sfrom_emailextra — could never execute.Fix
providers/smtp/src/airflow/providers/smtp/notifications/smtp.py:_build_email_contentnow falls back through connection extra →[email] from_emailconfig → hardcoded default, instead of raising when the connection extra is unset.task-sdk/src/airflow/sdk/execution_time/task_runner.py: passesfrom_email=Noneso the notifier's own resolution logic runs.Testing
test_smtp.pycovering all precedence combinations (connection-only, config-only, connection-over-config, neither-set-default).test_task_runner.pyto reflect thatfrom_emailresolution now happens insideSmtpNotifier, nottask_runner.py.Fixes #69262