Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

scottlimmer
Copy link
Contributor

@scottlimmer scottlimmer commented Oct 27, 2024

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
Contributor Author

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

Copy link

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-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 15, 2024
@github-actions github-actions bot closed this Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale 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