Skip to content

Conversation

@BYK
Copy link
Member

@BYK BYK commented Aug 25, 2021

Fixes #1045.

@BYK BYK requested a review from chadwhitacre August 25, 2021 08:38
@BYK
Copy link
Member Author

BYK commented Aug 25, 2021

Will also update docs on https://develop.sentry.dev/self-hosted/email/

Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

LGTM
With such a name, in docs, you need to make it very clear this is not to be filled by an email but rather than with the server name (FQDN) that is the one filled in the reverse DNS PTR.

Example:

  • To find out the IP dig sentry-server.company.tld +short > IP
  • Use the IP to see to what it reverse resolves dig -x $IP +short > result

Adding a disclaimer that people NEED to add the server to their SPF field would be great, nobody seems to care enough about it..

Use the $result to fill the value

This will do that the chain is resolvable
email>smtp>helo $result>spf check>name resolves the ip that we are receiving mail from>spf ok (I over simplified)

Co-authored-by: William Desportes <williamdes@wdes.fr>
@BYK
Copy link
Member Author

BYK commented Aug 25, 2021

@williamdes thanks a lot for the review! I'm okay renaming the variable, any suggestions? Should we add _HOST or _FQDN at the end?

Regarding docs, would you like to give it a shot yourself as you seem to be more knowledgeable than myself regarding mailing? 😄

@williamdes
Copy link
Contributor

@williamdes thanks a lot for the review! I'm okay renaming the variable, any suggestions? Should we add _HOST or _FQDN at the end?

Regarding docs, would you like to give it a shot yourself as you seem to be more knowledgeable than myself regarding mailing? 😄

Sure, I would love to get all Sentry installations perfectly email compatible, let me know where I can contribute

_HOST would be perfect a I am not sure everyone knows what a FQDN is

Maybe _HOSTNAME but I am not sure where the python setting is actually used
Could you post me links on where is the source code the setting is finally used to evaluate the impacts?

BYK and others added 2 commits August 25, 2021 23:04
Co-authored-by: Chad Whitacre <chadwhitacre@sentry.io>
@BYK BYK enabled auto-merge (squash) August 25, 2021 20:30
Comment on lines +235 to +236
SENTRY_OPTIONS["mail.list-namespace"] = env('SENTRY_MAIL_HOST', 'localhost')
SENTRY_OPTIONS["mail.from"] = f"sentry@{SENTRY_OPTIONS['mail.list-namespace']}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks right and wrong to me

I was thinking SENTRY_MAIL_HOST was used for the HELO command, but it seems like not

The value of the HELO command can be first.server.company.tld but send mails as domain2.tld

This is what did happen at phpMyAdmin, we send Sentry reports as xxx@phpmyadmin.net and use a HELO value of server.hostingcompany.tld

Could you please link me the actual uses of this two variables ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The value of the HELO command can be first.server.company.tld but send mails as domain2.tld

This is fine, they don't need to be in sync. We are just setting sensible defaults for a good out-of-the-box experience. One can change these as freely as they can.

Could you please link me the actual uses of this two variables ?

This is the code that uses them: https://github.com/getsentry/sentry/blob/2103815733832c1e463142f225f6bd2652b7e87e/src/sentry/utils/email.py#L277-L283

SENTRY_MAIL_HOST is not used, it is something I just made up for easy configuration in self-hosted. The mail server settings are here: https://github.com/getsentry/onpremise/blob/674a600770acad37ab8560e323e38ead49b0cf97/sentry/config.example.yml#L10-L16

By default we use the smtp service based on exim4 but it is also fully configurable.

@BYK BYK merged commit b32de84 into master Aug 26, 2021
@BYK BYK deleted the byk/feat/mail_host branch August 26, 2021 14:40
@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a hostname to the container to avoid emails getting rejected

4 participants