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

Allows to choose SSL context for SMTP provider #33075

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Aug 3, 2023

This change add two options to choose from when SSL SMTP connection is created:

  • default - for balance between compatibility and security
  • none - in case compatibility with existing infrastructure is   preferred

The fallback is:

  • The Airflow "email", "ssl_context"
  • "default"

^ 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.

airflow/providers/smtp/CHANGELOG.rst Show resolved Hide resolved
airflow/providers/smtp/hooks/smtp.py Show resolved Hide resolved
airflow/providers/smtp/hooks/smtp.py Outdated Show resolved Hide resolved
airflow/providers/smtp/provider.yaml Show resolved Hide resolved
@potiuk potiuk force-pushed the use-default-context-for-ssl-for-smtp-provider branch 3 times, most recently from 6adc0fc to 4078fa6 Compare August 3, 2023 15:51
@potiuk
Copy link
Member Author

potiuk commented Aug 3, 2023

Also for @hussein-awala -> I am not sure if this approach is best and how it will work with #30531 - I just figured that we have to use "smtp_provider" section rather than "smtp" from the core - and I think we should follow similar approach like here - where "smtp" will go to "pre_2_7_0" defaults, but the provider will use the "smtp_provider" section and fall back to the "smtp" one, similarly as I am falling back now to "email" / "ssl_context" - would love to hear from you on that one.

As explained above, I am trying to make chain of defaults this way and added unit tests covering the behaviour

airflow/providers/smtp/CHANGELOG.rst Outdated Show resolved Hide resolved
potiuk and others added 2 commits August 4, 2023 12:03
This change add two options to choose from when SSL SMTP connection
is created:

* default - for balance between compatibility and security
* none - in case compatibility with existing infrastructure is
  preferred

The fallback is:

* The Airflow "email", "ssl_context"
* "default"
Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com>
@potiuk potiuk force-pushed the use-default-context-for-ssl-for-smtp-provider branch from 6482eaa to dba5b23 Compare August 4, 2023 10:03
@potiuk
Copy link
Member Author

potiuk commented Aug 4, 2023

@hussein-awala -> would you like to comment on that one, re #30531 - or you are ok for now. I'd love to merge that one and implement IMAP change as well so that we can release new providers today :)

@potiuk
Copy link
Member Author

potiuk commented Aug 4, 2023

Let me merge that one now, I think we can always change it in the future.

@potiuk potiuk merged commit e20325d into apache:main Aug 4, 2023
42 checks passed
@potiuk potiuk deleted the use-default-context-for-ssl-for-smtp-provider branch August 4, 2023 10:30
@hussein-awala
Copy link
Member

I'm late to the party.

I just figured that we have to use "smtp_provider" section rather than "smtp" from the core - and I think we should follow similar approach like here - where "smtp" will go to "pre_2_7_0" defaults, but the provider will use the "smtp_provider" section and fall back to the "smtp" one, similarly as I am falling back now to "email" / "ssl_context" - would love to hear from you on that one.

For smtp_provider, I think it would be better to use the connection extras to configure ssl_context (as we do with the other configurations), instead of using Airlfow config (or even provider config).
https://github.com/potiuk/airflow/blob/ca2f3013bcb123c4b3973a5b85de77094bf2c459/airflow/providers/smtp/hooks/smtp.py#L321-L355

Since we cannot use the operator/hook without a connection, IMO it's better if we provide a single way to configure them.

WDYT? could we move this conf to the connection before releasing the provider?

@potiuk
Copy link
Member Author

potiuk commented Aug 4, 2023

Discussion in Slack follows

@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Aug 8, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone Aug 8, 2023
ephraimbuddy pushed a commit that referenced this pull request Aug 8, 2023
* Allows to choose SSL context for SMTP provider

This change add two options to choose from when SSL SMTP connection
is created:

* default - for balance between compatibility and security
* none - in case compatibility with existing infrastructure is
  preferred

The fallback is:

* The Airflow "email", "ssl_context"
* "default"

* Update airflow/providers/smtp/CHANGELOG.rst

Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com>
(cherry picked from commit e20325d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) kind:documentation provider:smtp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants