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

health checker: add missing check for oversized mails #4727

Closed
wants to merge 1 commit into from

Conversation

sebix
Copy link

@sebix sebix commented Jul 20, 2023

the health checker endpoint only reported unprocessable mails, but not oversized mails that made custom monitoring solutions necessary, or - if not present - could lead to undetected operational issues this adds a check if (and how many) oversized mails are present in the OVERSIZED_MAIL directory

with mails present in the oversized mails directory, the health check output now looks like:

{"healthy":false,"message":"oversized mails: 4","issues":["oversized mails: 4"],"actions":[]}

see also https://community.zammad.org/t/new-unprocessable-mail-directory-does-not-exist-on-zammad-6/11787

the health checker endpoint only reported unprocessable mails, but not oversized mails
that made custom monitoring solutions necessary, or - if not present - could lead to undetected operational issues
this adds a check if (and how many) oversized mails are present in the OVERSIZED_MAIL directory

with mails present in the oversized mails directory, the health check output now looks like:

{"healthy":false,"message":"oversized mails: 4","issues":["oversized mails: 4"],"actions":[]}

see also https://community.zammad.org/t/new-unprocessable-mail-directory-does-not-exist-on-zammad-6/11787
@dominikklein
Copy link
Collaborator

@mgruner Maybe something you can check?

@martini
Copy link
Collaborator

martini commented Jul 25, 2023

Nice work! :) In addition: IMO the health check should only complain if postmaster_send_reject_if_mail_too_large is set to false (only then somebody need to interact).

@MrGeneration
Copy link
Member

Nice work! :) In addition: IMO the health check should only complain if postmaster_send_reject_if_mail_too_large is set to false (only then somebody need to interact).

The problem with that setting is that it doesn't seem to have an effect on the storage side.
If postmaster_send_reject_if_mail_too_large is set to true, Zammad will still store the affected mail within the filesystem.

I believe I pointed that out somewhen in the past without success.
This forces me to cleanup regulary in SaaS.

@mgruner
Copy link
Collaborator

mgruner commented Jul 25, 2023

IMO the health check should only complain if postmaster_send_reject_if_mail_too_large is set to false (only then somebody need to interact).

@martini why do you think so? The health check should not need to verify the setting, but looking at the directory should be enough. If something is there, it needs to complain, no matter what the setting was changed to.

@mgruner
Copy link
Collaborator

mgruner commented Jul 25, 2023

The problem with that setting is that it doesn't seem to have an effect on the storage side. If postmaster_send_reject_if_mail_too_large is set to true, Zammad will still store the affected mail within the filesystem.

I believe I pointed that out somewhen in the past without success. This forces me to cleanup regulary in SaaS.

@MrGeneration looking at the code, it appears to have this logic:

  • If the setting is false, mails are not processed at all (and thus stay on the mail server, waiting to be fetched). No reply, no file storage.
  • If the setting is true, mails are fetched, but not processed. Therefore a reply is sent, to indicate that the mail did not lead to a ticket, and also a file is written to the oversized mails folder to allow the admin to take action on it (like manually deleting it).

If that behaviour is not useful, we should open another issue to describe a better way. Let's not hijack this issue at hand, which is about monitoring (only).

@martini
Copy link
Collaborator

martini commented Jul 25, 2023

I think I have to go further and bring more light into the darkness.

Ahead: @sebix many thanks for the MR

On the functionality - historical background:

  1. Early Zammad: There was a limit in Zammad (postmaster_max_size) which defines the maximum size of an email that Zammad fetches from a mailbox.

Important: Emails larger than postmaster_max_size were "silently" left in the mailbox.

Problem: This had the problem that neither an admin nor a customer was notified about the email "left in the mailbox".

  1. Based on 1), Zammad was enhanced to send an auto-responder/postmaster email to the customer for oversized emails, thus notifying the customer if an oversized email did not arrive in Zammad (customer need to send a smaller email again).

Important: The oversized email will be downloaded/deleted from the mailbox. The customer will receive an auto-responder/postmaster email. And the oversized email is created as a copy in var/spool/oversized_mail/ (only as a backup - if there are emails here no action is necessary).

For downward compatibility reasons the setting postmaster_send_reject_if_mail_too_large was introduced:

postmaster_send_reject_if_mail_too_large=false: Old behavior as under 1)

postmaster_send_reject_if_mail_too_large=true: new behavior as under 2)

Summary: If emails are located under var/spool/oversized_mail/, then this is just a backup - no action by an administrator is required. Thus, no health checker message is needed either.

@sebix : I hope I could bring light into the darkness. My question now would be what are your backgrounds for the MR to better understand your problem. Thanks for response!

@sebix
Copy link
Author

sebix commented Jul 26, 2023

Thanks to everyone for the reactions!

Summary: If emails are located under var/spool/oversized_mail/, then this is just a backup - no action by an administrator is required. Thus, no health checker message is needed either.

@sebix : I hope I could bring light into the darkness. My question now would be what are your backgrounds for the MR to better understand your problem. Thanks for response!

Our motivation is the same as @mgruner's: Zammad dumps files somewhere and never cleans them up, and the operator never gets a notice. This could fill up the disk. Either we write our own script to remove those emails on a regular basis, or we get a notification from Zammad's health check to have a look (or both). In https://community.zammad.org/t/new-unprocessable-mail-directory-does-not-exist-on-zammad-6/11787 we discussed this matter, the result was that the health check misses the oversized mails and in contrast, covers the unprocessable mails
If Zammad never stored the oversized email, it would be - from an operation perspective - no issue at all.

(Storing the large - too large - mails could you actually make vulnerable, but that's very theoretical.)

@sebix
Copy link
Author

sebix commented Aug 29, 2023

Are there any open questions?

@MrGeneration
Copy link
Member

Summary: If emails are located under var/spool/oversized_mail/, then this is just a backup - no action by an administrator is required. Thus, no health checker message is needed either.

Cleaning up is definitively a must and so I strongly believe that Zammad should indicate there's messages in said folder.

@martini
Copy link
Collaborator

martini commented Sep 13, 2023

I forgot to submit my note. If postmaster_send_reject_if_mail_too_large is enabled we should not longer save oversized emails in var/spool/oversized_mail/ because it's not needed (sender got already informed).

@sebix
Copy link
Author

sebix commented Sep 13, 2023

I forgot to submit my note. If postmaster_send_reject_if_mail_too_large is enabled we should not longer save oversized emails in var/spool/oversized_mail/ because it's not needed (sender got already informed).

Is this a pre-condition for merging this PR? And if yes, do you prefer to put that change in here or a separate PR?

In addition: IMO the health check should only complain if postmaster_send_reject_if_mail_too_large is set to false (only then somebody need to interact).

If postmaster_send_reject_if_mail_too_large first is true, there are files in the directory and the user switches postmaster_send_reject_if_mail_too_large to false, the check should silence? Doesn't that hide the issue and potentially cause more confusion and issues?

@martini
Copy link
Collaborator

martini commented Sep 13, 2023

If postmaster_send_reject_if_mail_too_large larger emails are not downloaded (they still keep on server site / mailbox). :-D

So IMO we just need to remove one lines https://github.com/zammad/zammad/blob/develop/app/models/channel/email_parser.rb#L530C13-L530C13 to not save oversized emails under var/spool/oversized_mail/ anymore :-o

Just to be sure: @mgruner @MrGeneration anything against this approach (not storing oversized emails under var/spool/oversized_mail/ anymore if sender got already notified about it)?

@mgruner
Copy link
Collaborator

mgruner commented Sep 14, 2023

@martini that seems to be a valid choice. I can implement this small change.

@sebix
Copy link
Author

sebix commented Sep 14, 2023

Thank you for implementing this change. Looking forward for the next release containing this fix :)

@sebix sebix deleted the fix-health-oversized branch April 21, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants