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

cass smtpd refactor #5005

Merged
merged 9 commits into from
Sep 3, 2024
Merged

Conversation

elliefm
Copy link
Contributor

@elliefm elliefm commented Aug 21, 2024

Refactors Cassandane's smtpd setup to address a few different issue. This probably makes most sense when reviewed commit by commit.

  1. Breaks the smtpd thing out into a standalone script (utils/fakesmtpd) that's fork-exec'd rather than just forked. This means:
    • the smtpd processes no longer share memory with Cassandane, and therefore no longer try to prematurely destruct Cassandane's state when they exit
    • Cassandane's verbose handling no longer conflicts with Net::Server mapping the client socket to stdin/stdout
  2. Cassandane now sets up its Cyrus instances with broken smtp configuration, because Cyrus's defaults would send mail to the world if the system's sendmail can do so, and that's a whoopsie we don't need. Now, if Cyrus tries to send mail from a test that hasn't specifically overridden the broken configuration, then Cyrus's smtpclient_open() will fail due to the bad config and no mail will go out.
  3. Adds a want parameter for tests to request it if they need it, and we don't configure or start it unless they request it. This shaves a decent amount of time vs starting it for every test like we used to.
  4. Marks up all the tests that actually need it so that they get it.
  5. Fixes a few other small bugs that were in the way

@elliefm elliefm force-pushed the v311/cass-smtpd-refactor branch 2 times, most recently from dd79a68 to 6542034 Compare August 23, 2024 02:07
Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

This looks great! But can we make the magic string be a little more verbose, e.g.: needs_smtp or needs_component_smtp?

@elliefm
Copy link
Contributor Author

elliefm commented Aug 25, 2024

@ksmurchison The :needs_foo stuff is a different kind of magic from this (automatic identification of Cyrus features based on cyr_buildinfo output).

The :SMTP magic is pretty difficult to hold correctly, regardless of what it's called. A test author needs to know whether Cyrus is going to try to make smtpclient_open calls during their test, which it does in some non-obvious places (e.g. JMAP Identity/get). If a test fails because Cyrus made an smtpclient_open call when the test wasn't set up for it, the error is pretty opaque too -- it's not really obvious that the solution is to turn on SMTP in the test unless you already know and are thinking about this problem. I would like to make it easier to hold, but the name isn't really the problem here.

@elliefm
Copy link
Contributor Author

elliefm commented Aug 26, 2024

@ksmurchison I started adding some comments explaining how to use :SMTP and how to detect when it was necessary, and in the process of trying to explain it I think I've figured out some things I can rename to make it all a bit clearer. So, thanks for the prompt about the name!

so we don't need to conditionally skip them anymore
spawns fewer fakesmtpd processes when they're not being used
* Reset it to zero on MAIL FROM and RSET, because these server
  processes can be reused for multiple emails or connections.  This
  effectively makes it a per-message recipient limit, where
  previously it was a per-process limit
* Explicitly initialise it to zero in new, so that it's obvious it
  exists
Cyrus will default to sending outgoing mail via sendmail if not
otherwise configured, which is potentially a problem when running
Cassandane on a machine configured to send mail to the world.

Make Cassandane set Cyrus up with an invalid smtp configuration by
default, so that if some test case hits smtpclient_open without
having been set up for it, the test just breaks instead of sending
mail.
@elliefm
Copy link
Contributor Author

elliefm commented Aug 26, 2024

Also remembered that we already have a general purpose :want_foo thing for adding stuff to want per-test, so we don't need a separate :SMTP magic word for it anyway

@elliefm elliefm marked this pull request as ready for review August 26, 2024 01:21
@elliefm
Copy link
Contributor Author

elliefm commented Aug 26, 2024

@rsto This might also fix your "Found some stray processes" failures we were talking about here, as those errors no longer occur for me on this branch with optimisations on

@elliefm elliefm requested a review from rjbs August 29, 2024 22:23
Copy link
Member

@rsto rsto left a comment

Choose a reason for hiding this comment

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

This looks real great to me. Looking forward to these fixes.

@elliefm elliefm merged commit f7256e9 into cyrusimap:master Sep 3, 2024
1 check passed
@elliefm elliefm deleted the v311/cass-smtpd-refactor branch September 3, 2024 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants