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

Add settings EMAIL_FQDN #13728

Closed
wants to merge 9 commits into from
Closed

Add settings EMAIL_FQDN #13728

wants to merge 9 commits into from

Conversation

jrief
Copy link
Contributor

@jrief jrief commented Nov 27, 2020

No description provided.

@felixxm
Copy link
Member

felixxm commented Nov 27, 2020

@jrief Thanks for this patch, however all new features require an accepted ticket. Also adding a new setting is always a bit controversial so I would recommend to start a discussion on DevelopersMailingList.

@felixxm
Copy link
Member

felixxm commented Nov 27, 2020

OK, there is a ticket-6989.

@jrief
Copy link
Contributor Author

jrief commented Nov 27, 2020

The triage state was set to Accepted.
Discussion is 13 years long, and the ticket was never closed.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jrief — Thanks for this!

Related is #13305 that @lsaavedr is working on. There adding three more settings to allow customising the SSLContext is suggested.

My thought on review was that that's too much… — I think the settings should control which backend is used, and maybe then the most used options (maybe just EMAIL_USE_SSL for the SMTP backend).

THEN I think we should provide a sane default (e.g. use the system CA chain) and ask people to subclass the email backend for more custom options.

(Make sense as an approach? — The idea is to hide unnecessary complexity for the common case.)

Then here I guess this would mean pulling the necessary functionality into the email backend, so that folks can set the FQDN there. 🤔

I'd like to address both these tickets in a unified way, so we're telling a single story — making a nice change for all — rather than pulling in opposite directions.

@jrief and @lsaavedr — What do you think?
Thanks both! 🥇

@jrief
Copy link
Contributor Author

jrief commented Dec 8, 2020

I see both pull requests as completely independent.

That one I fixed was accepted by Malcolm Tredinnick in 2010. Very sad that we can't ask him anymore.
Having to override the backend to add configuration directives, creates a bunch of other problems; imagine using the same project image for different implementations.

Anyway, how about moving all email related configuration directives into a dictionary? But that's a different story.

@MaicoTimmerman
Copy link

MaicoTimmerman commented Feb 12, 2021

I think the scope of this patch could be broadened a little more. Currently, it only improves the message-ID and names the setting accordingly. However, the EHLO/HELO SMTP command is still using the old FQDN behavior, which is also discussed in ticket-6989.

Some context: when a SMTP connection is made, often an EHLO or HELO command is send to define the connecting server. In our environment we run django in containers , meaning FQDN are more detailed then the domain from which e-mails are sent. The DNS_NAME.get_fqdn() would resolve to app1.cluster2.example.com, while we'd only want to EHLO using example.com.

In the ticket there exists a 10 year old patch, which covers this correctly.

Base automatically changed from master to main March 9, 2021 06:21
@jrief
Copy link
Contributor Author

jrief commented Mar 19, 2021

@MaicoTimmerman

I think the scope of this patch could be broadened a little more. Currently, it only improves the message-ID and names the setting accordingly. However, the EHLO/HELO SMTP command is still using the old FQDN behavior, which is also discussed in ticket-6989.

Some context: when a SMTP connection is made, often an EHLO or HELO command is send to define the connecting server. In our environment we run django in containers , meaning FQDN are more detailed then the domain from which e-mails are sent. The DNS_NAME.get_fqdn() would resolve to app1.cluster2.example.com, while we'd only want to EHLO using example.com.

This is a valid point! So my current proposal naming that setting EMAIL_MESSAGEID_FQDN wouldn't be appropriate anymore. EMAIL_LOCAL_HOSTNAME doesn't seem to fit into a clustered environment. How about EMAIL_FULL_QUALIFIED_DOMAIN_NAME or EMAIL_FQDN? @carltongibson Other proposals?

Anyway, those pull requests shall remain independent.

@carltongibson
Copy link
Member

@carltongibson Other proposals?

TBH I would (still) much prefer us to move towards pulling the numerous options here inside the email backend, rather than adding to an ever growing list of settings (with then matching parameters to the backend class...) — Configure your backend and then put that in settings…

I know that's a change in strategy, but my feeling is this is getting out of hand, and we'd be better served by a tidy-up.

It's open that you don't agree with that.

Assuming you're not totally opposed, would creating a modern backend, with all the right options, outside of Django, so we can iterate, and then proposing a migration to that once it was finished/stable be feasible?
(I'm reluctant to press on adding new settings only to move to something else...)

The alternative is to just stick with how we do it now, and keep adding to the settings.
(I'm less keen on that.)

@jrief
Copy link
Contributor Author

jrief commented Mar 20, 2021

Moving the Email backend out of Django (but as part of the ecosystem), certainly is an option we shall talk about. This however removes one battery, otherwise included into the Django project. How about this proposal:

For version 5, the Email backend shall be moved into a separate package.
This pull request won't make it into 3.2, so the next version would be 4. This means that we can make a bigger modification. How about putting all Email configuration directives into a Python dictionary, similar to DATABASES or TEMPLATES. We then could add a list of Email backends, rather than offering just one. Each of those backends then would offer the directive we have now.

For instance, EMAIL_BACKEND, EMAIL_HOST, EMAIL_PORT, EMAIL_HOST_USER, EMAIL_HOST_PASSWORD, etc. would translate into

EMAIL: {
    'default': {
        'BACKEND': ...,
        'SMTP_HOST': ...,
        'SMTP_PORT': ...,
        'HOST_USER': ...,
        'HOST_PASSWORD': ...,
        ...
    },
    'other': {
        ...
    },
}

When sending Email, one can choose the backend, or if unspecified, it falls back to default. This behaviour is well known for other parts of the Django project.

@carltongibson
Copy link
Member

I'm not suggesting we move it out of core permanently (although...🙂)

Rather, that we come up with a solution we're going to be happy with before pushing everyone down one path, and then immediately having them turn around and come back another way. Especially adding settings that's going to be painful.

I think the growth of the number of options being suggested for settings demonstrates that we need to rethink and modularise it in a better way.

My thought there is to have people configure these things internally to a backend, maybe leaving just one or two main options exposed (but arguably not.)

These issues are old, so I'm not sure too worried if they take an extra cycle to get right.

Does that make sense?

I'll have a look at this in the week, and think about the details of your suggestions here @jrief. (Don't have the bandwidth to consider them fully on the weekend.)

@carltongibson
Copy link
Member

carltongibson commented Mar 24, 2021

@jrief — just to update you — I'm thinking about this — trying to do the right thing for users…

There are only 5 email tickets in total — maybe we could (should) just add the setting here and those for ticket-31885 and then address a possible adjustment in a single step afterwards. 🤔

As I've said, I'm not particularly keen on adding the settings and then switching to something else, but the advantage of doing it this way would be to get those two tickets resolved for 4.0. We'd then buy the time needed (n versions likely) to come up with a more encapsulated solution.

Not sure how I feel about that, but your take is appreciated.

@jrief
Copy link
Contributor Author

jrief commented Mar 24, 2021

@carltongibson I'm completely with you. This probably is the beast approach!

As a side note, a more modular configuration system would be highly appreciated: Then each component gets its own prefix and all configuration settings would start with something like DJANGO.EMAIL.FQDN = '...' or DJANGO.DATABASE.DEFAULT... or CRISPYFORMS.TEMPLATE_PACK = '...'.

@carltongibson
Copy link
Member

@jrief @MaicoTimmerman — What's the conclusion of the discussion on using EMAIL_FQDN? — Do you want to update the patch to include the suggested change?

@jrief
Copy link
Contributor Author

jrief commented Mar 31, 2021

@carltongibson @MaicoTimmerman
I will update this pull request, if we can agree, that EMAIL_FQDN is the proper name for this configuration directive.

@carltongibson
Copy link
Member

@jrief It seems fine. We can always adjust on review, but I can't push this forwards unless it's actually in the "yes, this is the proposed change" state. From the discussion that's not the case.

@jrief
Copy link
Contributor Author

jrief commented Apr 12, 2021

@carltongibson

I can't push this forwards unless it's actually in the "yes, this is the proposed change" state. From the discussion that's not the case.

What can I do to fix this?

@jrief jrief changed the title Add settings EMAIL_MESSAGEID_FQDN Add settings EMAIL_FQDN Apr 19, 2021
@MaicoTimmerman
Copy link

@jrief @carltongibson I just ran into this thread again. I think the change is nearly complete, however we are still missing the fix for the EHLO / HELO.

My proposed addtion would be:

diff --git a/django/core/mail/backends/smtp.py b/django/core/mail/backends/smtp.py
index 13ed4a2798..218365c6d3 100644
--- a/django/core/mail/backends/smtp.py
+++ b/django/core/mail/backends/smtp.py
@@ -50,7 +50,11 @@ class EmailBackend(BaseEmailBackend):
 
         # If local_hostname is not specified, socket.getfqdn() gets used.
         # For performance, we use the cached FQDN for local_hostname.
-        connection_params = {'local_hostname': DNS_NAME.get_fqdn()}
+        connection_params = {}
+        if settings.EMAIL_FQDN:
+            connection_params['local_hostname'] = settings.EMAIL_FQDN
+        else:
+            connection_params['local_hostname'] = DNS_NAME.get_fqdn()
         if self.timeout is not None:
             connection_params['timeout'] = self.timeout
         if self.use_ssl:

@jrief
Copy link
Contributor Author

jrief commented Dec 6, 2021

@MaicoTimmerman @carltongibson I'm willing to push this forward, but the Django board must agree on details, for instance if EMAIL_FQDN is the right name for this extra settings variable. I asked for this on April 13th, but didn't get any response yet. Shall I retry through the Django developer mailing list?

This settings variable then would influence the right hand side of the Message-ID and the HELO/EHLO connection string. The implementation for the latter is currently missing.

@carltongibson
Copy link
Member

Hi @jrief — thanks for the work on this. I was hoping to return to it in time for 4.0, but time didn't allow. It's on my list to resolve for 4.1. What we want to avoid is rushing to add a partial solution only to deprecate that immediately. Measure twice… and so on.

Related to #13305, the issue is whether there's a better option than continuing to proliferate settings for every possible configuration option. i.e. encapsulate that better so that folks would just set EMAIL_BACKEND (as an example).

The issue/delay is just thinking through how an API for that would look — where exactly would the email configuration live… I looked into this a bit and am working on the thought that if get_connection() took a class as well as a import-path string then it might[^] be neat enough to do something like:

from functools import partial
from django.core.mail.backends.smtp import EmailBackend

EMAIL_BACKEND = partial(
   EmailBackend, 
   **my_many_custom_options
)

Rather than (having to) declare an actual subclass somewhere and then use the import path for the setting — but I'm yet to formalise that. Implementation apart, the goal would be to encapsulate the SMTP related email features inside django.core.mail, and the docs for that, reducing the spread in django.conf. Ultimately if something along those lines is not feasible then we can add ever more settings but...

[^]: Might — I'm very happy to take input here: What's the API you'd like to see?

@jrief
Copy link
Contributor Author

jrief commented Dec 6, 2021

@carltongibson wouldn't it then make sense, to allow multiple SMTP backends, similar to the database?

In the project's settings.py this then would look like:

EMAIL = {
    'default': {
        'BACKEND': 'django.core.mail.backends.smtp.EmailBackend',
        'HOST': 'smtp.example.org',
        'HOST_USER': 'smtp_user',
        'HOST_PASSWORD': 'smtp_secret',
        'PORT': 25,
        'FQDN': 'example.org',
        # and all other related directives
    }
}

Then instead of passing a class or import-path string to get_connection() we would pass in None (default) or an alternative name of the Email backend. This would allow users to configure more than one Email backend and let the application choose one of them, depending on their business logic.

It would also remove all but one (EMAIL) Email configuration directives from the global namespace.

Personally, I would prefer SMTP = {...} as the main configuration directive, but EMAIL seems to be more popular in Django.

@carltongibson
Copy link
Member

@carltongibson wouldn't it then make sense, to allow multiple SMTP backends, similar to the database?

Interesting. I hadn't considered that option — is it something you'd want? (Moving the config into the backend would allow multiple backends so it's certainly related.)

@jrief
Copy link
Contributor Author

jrief commented Dec 7, 2021

Interesting. I hadn't considered that option — is it something you'd want? (Moving the config into the backend would allow multiple backends so it's certainly related.)

For consistency it would make sense to handle the SMTP-backends similar to database and template-engine backends.

Currently I have no immediate need for different backends, but it the future this could be an issue. It also would make the configuration cleaner, if someone needs different backends when switching from a developer to a production environment.

BTW.: I am the second maintainer of https://github.com/ui/django-post_office and there we actually need a different Email backend.

Shall we move this discussion to the Django developer mailing list?

@carltongibson
Copy link
Member

carltongibson commented Dec 7, 2021

Shall we move this discussion to the Django developer mailing list?

If you wish, yes, super.

My list looks like: finish the SECRET_KEYS PR; process Andrew's Async ORM interface PR; look at these Email backend tickets, but if you'd like to pick it up happy to input and review!

Thanks!

@jrief
Copy link
Contributor Author

jrief commented Jan 30, 2022

I moved this issue to the discussion group:
https://groups.google.com/g/django-developers/c/R8ebGynQjK0
Please provide feedback, because as it seems currently we're stuck on this.

@nessita
Copy link
Contributor

nessita commented Jan 16, 2024

@jrief Hello! I have just caught up with this PR, the related ones, and the linked tickets (this is in the context of ticket #35118). Are you available to continue contributing as concluded in the django-developers email thread?

I'm keen to close this PR in favor of the new/future ticket for the multiple email backends work. Let me know! Thank you.

@jrief
Copy link
Contributor Author

jrief commented Jan 16, 2024

Hi Natalia,
I'd like to work on the ticket implementing multiple email backends. However, I currently have no resources to work on it. So please feel free to close it.
Jacob

@nessita
Copy link
Contributor

nessita commented Jan 16, 2024

Hi Natalia, I'd like to work on the ticket implementing multiple email backends. However, I currently have no resources to work on it. So please feel free to close it. Jacob

Thank you Jacob for your honest response. I will close this PR, hopefully you can go back to this contribution some time in the future. Looking forward to that! 💯

@nessita nessita closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants