Skip to content

[WIP] Override smtp config variables with connection values specified by email_conn_id - #43413

Closed
scottlimmer wants to merge 3 commits into
apache:mainfrom
scottlimmer:34554
Closed

[WIP] Override smtp config variables with connection values specified by email_conn_id #43413
scottlimmer wants to merge 3 commits into
apache:mainfrom
scottlimmer:34554

Conversation

@scottlimmer

@scottlimmerscottlimmer commented Oct 27, 2024

Copy link
Copy Markdown
Contributor

closes: #34554
related: #41539

WIP This change is incomplete, requires discussion.

Mandatory configuration of smtp_user and smtp_pass via Connection object creates confusion as to source of truth for other SMTP params. This change proproses overriding all smtp_* vars with the Connection object equivalents when email.email_conn_id is specified. However, this creates some a questions that need resolving/consensus.

Issue 1

My preference would be to use the [smtp] config vars as default/fallback values. However, this isn't possible as the connection UI doesn't allow for an 'not-specified' state for certain fields.

E.g. the 'Disable SSL' checkbox defaults to False when not set, making it impossible to create a fallback condition. Attempting to implement a fallback leads to a state where the smtp_ssl config is ignored if the checkbox is checked, or the smtp_ssl config must be set to the desired value if the checkbox is unchecked.

Issue 2

[smtp] configuration variables are become usable only for connections not requiring authentication. This effectively reverses the confusion I'm trying to address. I.e. [smtp] config vars would have no effect if email_conn_id is in play.

Possible solutions

  • Remove all [smtp] config vars duplicated by connection object. Effectively mandate that all smtp configuration is done via a Connection object. This requires the most changes.
  • Split airflow.utils.email.send_email_smtp backend into send_email_smtp and smtp_email_smtp_via_conn. For this to work we'd need to revert smtp email user and password deprecated config removal #41539 and its deprecation, then auto-select backend based on presence of email_conn_id. Effectively states that if you want to use a Connection object to secure your SMTP params, you must configure the entirety of the SMTP connection using the object.
  • Introduct another config: smtp.smtp_config_source=config|connection and require the user to opt into desired behaviour. Allows the user to specify that all SMTP vars should be controlled by Connection object. Still leaves a confusing default state.
  • Modify the the connection UI to allow the user to clarify intent, thereby enabling the use of smtp config as fallback/default.
    • Change checkboxes to Default/Yes/No. This currently isn't possible as SelectField/RadioField types aren't allowed in Connection Forms
    • Provide No auth/user/pass checkbox to user to specify smtp doesn't require login.
  • Other ideas?

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@scottlimmer

Copy link
Copy Markdown
ContributorAuthor

@potiuk@hussein-awala Would be good to get your thoughts (or anyone else who might have insights) on this before progressing further.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 15, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

staleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

airflow/utils/email.py def send_mime_email doesn't take all values from connection

1 participant

@scottlimmer