49893 - (bz-smtp) Ability to use different SMTP server

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P3)

Description

[asric]

Right now, bugzilla requires that the machine it is running also be running an 
SMTP server. It would be nice if there were a configuration item to define 
which server and which port bugzilla should connect to, rather than trusting 
that the user has installed Sendmail correctly.

Comment 1

[Chris Yeh]

this is possible using any number of perl sendmail modules, but that would 
involve dragging in more dependencies into the main product

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

->Bugzilla product...

Comment 3

[Lee Chenhwa]

It may be convenient to group  the SMTP server setting with that server setting
of each account. 

Comment 4

[Dave Miller [:justdave]]

Reassigning all of my "future" targetted bugs to indicate that I'm not presently
working on them, and someone else could feel free to work on them.

Comment 5

[Dave Miller [:justdave]]

Reassigning all of my "future" targetted bugs to indicate that I'm not presently
working on them, and someone else could feel free to work on them. (sorry for
the spam if you got this twice, it didn't take right the first time)

Comment 6

[Erik Stambaugh]

Attachment: Moves Bugzilla's outbound mail to Net::SMTP

Net::SMTP is a good, server-agnostic abstraction of the SMTP protocol, and it
seems popular whenever the topic of moving away from sendmail dependency is
brought up.  So here it is.

I'm more intersted in seeing what everyone has to say first.  I tested this
patch briefly and it seems OK.	If it works for everyone, well, maybe we should
use it.  I'll poke at it a little more in the coming days.

Two possible issues with this patch that I will enumerate before someone else
finds them:

1. There is no more sendmailnow.  Deferred mail delivery is no longer available
if you use this.  I was talking to justdave and he mentioned the idea of
building an internal queue for bugmail, which would take out a big chunk of the
need for that.	If mail queues are a priority because of this, I can create a
new bug for that and work on it or, better, delegate it.

2. If multiple messages are sent, one session is opened for each one, in
sequence.  It goes against my instincts, but I decided to not have single
sessions in this on the off chance that one malformed address or message (or
who-knows-what) trashes the single session.  At least the rest of the mail gets
through.  Also, the gain for having one session per message should be marginal
on most installations.

I'm writing this with an ulterior motive.  I want to get inbound mail worked
into the main distribution.  I want to use the Bugzilla::Mail directory
(created here), and this seemed simple enough (famous last words) to throw in
while I'm at it.  And it might help give email reform a little nudge along its
way.

Comment 7

[Dave Miller [:justdave]]

Hmmm.... I like.

84876 is definitely trying to bite off too much at once.  Baby steps are the way
to do it.

Initiating a TCP connection actually seems to take a fair bit of time on most
sites, due to tcpwrappers checking and so forth.  What I'd suggest doing (long
term -- what you have on here now should be fine until we get the back-end
queueing in place) is expand on the error checking within the mail sending, and
as long as no errors occur, go ahead and keep sending on the same connection. 
Perhaps add a param for how many messages to attempt to send in the same
session, since a lot of servers have limits to prevent spamming.  Default should
be something sane line 20 (I hear most cut you off at 25).  Once you've sent
that many, drop the connection and reopen it.  If any error at all is
encountered during sending of the message, drop the connection after cleaning
up, then reopen for the next message.

As mentioned, that's not worth trying to get into this one.  Startup time for
sendmail on the command line is probably about as bad on a lot of systems right
now, and we do invoke that once per message already now.

Comment 8

[Erik Stambaugh]

Attachment: Net::SMTP patch

Whoops.  There was a typo in Flag.pm that caused it to not compile.

Also, an issue I've already found: checksetup isn't checking for Net::SMTP yet.
 I will add that soon.	Don't consider this a finished patch yet.  I'm just
looking for an answer as to whether this is the right way to go.

Comment 9

[Erik Stambaugh]

(In reply to comment #7)
> Hmmm.... I like.

Cool.

> Initiating a TCP connection actually seems to take a fair bit of time on most
> sites, due to tcpwrappers checking and so forth.  What I'd suggest doing (long
> term -- what you have on here now should be fine until we get the back-end
> queueing in place) is expand on the error checking within the mail sending, and
> as long as no errors occur, go ahead and keep sending on the same connection. 
> Perhaps add a param for how many messages to attempt to send in the same
> session, since a lot of servers have limits to prevent spamming.  Default should
> be something sane line 20 (I hear most cut you off at 25).  Once you've sent
> that many, drop the connection and reopen it.  If any error at all is
> encountered during sending of the message, drop the connection after cleaning
> up, then reopen for the next message.

You're right.  If it checks its errors more intelligently, it could pull that off.

Also, as much as it pains me to say it, I think I'd like to see an smtp object
with send_message as a method.  That should make checking for error messages
easier, since right now all we get is the return value of send_message.

> As mentioned, that's not worth trying to get into this one.  Startup time for
> sendmail on the command line is probably about as bad on a lot of systems right
> now, and we do invoke that once per message already now.

Well, um, actually, some of the invocations of sendmail ran once for a list of
cc's, and I broke those out into individual sessions.  I think I'd like to see
this accept an array of to: addresses to simplify sending to multiple addresses.

Again, though, this is just a baby step.

Comment 10

[Joel Peshkin]

What do we do about 4xx error codes??  

Comment 11

[Dave Miller [:justdave]]

bah.

[Fri 22:27:03] <joel> justdave: we will probably need to keep the "user last got
mail about this bug" entry if we start handling SMTP 4xx replies
[Fri 22:27:13] <joel> otherwise we wont know to try again
[Fri 22:27:35] <justdave> aw crap.
[Fri 22:27:46] <joel> queuing
[Fri 22:27:46] <justdave> forgot about that.  sendmail just queues it if it
can't get through :)
[Fri 22:28:03] <justdave> if we go to direct SMTP, we're going to need to do our
own queueing
[Fri 22:28:22] <joel> yup


The queuing stuff is either going to need to go in before this, or as part of this.

Comment 12

[Erik Stambaugh]

(In reply to comment #11)
> bah.
> 
> [Fri 22:27:03] <joel> justdave: we will probably need to keep the "user last got
> mail about this bug" entry if we start handling SMTP 4xx replies
> [Fri 22:27:13] <joel> otherwise we wont know to try again
> [Fri 22:27:35] <justdave> aw crap.
> [Fri 22:27:46] <joel> queuing
> [Fri 22:27:46] <justdave> forgot about that.  sendmail just queues it if it
> can't get through :)
> [Fri 22:28:03] <justdave> if we go to direct SMTP, we're going to need to do our
> own queueing
> [Fri 22:28:22] <joel> yup
> 
> 
> The queuing stuff is either going to need to go in before this, or as part of
this.


Bah is right.

OK, well, we can have a table with the pertinent information-- from, to,
message, and last-attempted-timestamp, and use that for generic queueing. 
Dealing with messages that wind up in the queue could either be a matter of
running a cron job, having a maintenance option for admins that sends pending
mail, or have each following smtp call try re-sending up to a certain number of
pending messages.  Have these all happen only after a certain interval, of
course (15 min. minimum?)-- then bounce anything that sat in the queue for too
long get bounced to the administrator.

And this was going to be *so* easy, too.

Actually, some of the above might be fairly simple.

Then we add a do-not-queue option for routines that handle it themselves, like
bugmail.

Comment 13

[Erik Stambaugh]

So we've been having discussions in IRC about queueing mail, and we almost have
a consensus or two about this.  Almost.  Here I go.

1. The current (local sendmail executable) implementation eats mail if something
happens to that local sendmail.

2. It is assumed in most cases, especially because history has required sendmail
to be installed locally, that the mail server people will be using will be the
local host

3. The effect of the local mailserver being down in this implementation is the
same as if the sendmail program became unavailable, if you squint your eyes and
tilt your head a little, that means a new problem isn't introduced.

Now, I'm not saying that an internal mail queue wouldn't be a feature that we
need, but does it hold this patch up, or should we make a new bug for that one
just to keep everything nice and bite-sized?

Comment 14

[Erik Stambaugh]

Attachment: Net::SMTP patch (with checksetup)

The previous patch didn't have checksetup looking for Net::SMTP.

Comment 15

[Erik Stambaugh]

Comment on attachment 151971 [details] [diff] [review]
Net::SMTP patch (with checksetup)

I think this might be ready for prime time.

Comment 16

[Cédric Caron]

In checksetup.pl the entry in the %ppm_modules table is mission for new module.

Comment 17

[:glob ✱]

(In reply to comment #16)
> In checksetup.pl the entry in the %ppm_modules table is missing for new module.

you need:

    'Net::SMTP'         => 'libnet',

Comment 18

[Joel Peshkin]

Comment on attachment 151971 [details] [diff] [review]
Net::SMTP patch (with checksetup)

If the return address it totally bogus and the smtp server rejects it, this
carps with an unhelpful error message

Comment 19

[Erik Stambaugh]

(In reply to comment #18)
> (From update of attachment 151971 [details] [diff] [review])
> If the return address it totally bogus and the smtp server rejects it, this
> carps with an unhelpful error message

I didn't alter any of the error conditions.  It's the same case as the local
sendmail executable rejecting.  I don't think there is any difference in behavior.

Comment 20

[Erik Stambaugh]

Also, kind of an afterthought:  It is possible that Net::SMTP is more sensitive
to bad addresses, but the helpfulness of the error message (or lack thereof) is
no different than it was before.

I'm going to test the old code to see if this is a new problem or not.

Comment 21

[Erik Stambaugh]

Attachment: Net::SMTP patch (with more checksetup and better error reporting)

This addresses the above.  Have at you.

Comment 22

[Joel Peshkin]

Comment on attachment 152037 [details] [diff] [review]
 Net::SMTP patch (with more checksetup and better error reporting)

Missed MailPassword in CGI.pm

Replace the die() call with ThrowCodeError and define the appropriate error
code. 

While we're at it, do we want to use example.com as the default or should we
try to populate it using Sys::Hostname which does seem to be standard.

Comment 23

[:glob ✱]

> While we're at it, do we want to use example.com as the default or should we
> try to populate it using Sys::Hostname which does seem to be standard.

we need the domain name, not the hostname, and there's no standard
cross-platform way to get the domain name.

i think that "localhost" is a better default.

Comment 24

[Cédric Caron]

If Net::SMTP suport SMTP server authentication why not adding a login and 
password parameters in the config file to use it ?

Comment 25

[Erik Stambaugh]

Attachment: Net::SMTP with (some) ThrowCodeError calls instead of die, and minor cleanup

(comment 22)

> Missed MailPassword in CGI.pm
Fixed.

> Replace the die() call with ThrowCodeError and define the appropriate error
> code. 

There are some places where that isn't appropriate, like whineatnews, and some
places where mail delivery errors are being ignored.  Otherwise, done.

> While we're at it, do we want to use example.com as the default or should
> we try to populate it using Sys::Hostname which does seem to be standard.
See below.

(comment 23)

> we need the domain name, not the hostname, and there's no standard
> cross-platform way to get the domain name.
> 
> i think that "localhost" is a better default.

That's what I have it using now.  It should be fairly adequate for the
frighteningly large number of un(der)-configured Bugzilla installations.

(comment 24)

> If Net::SMTP suport SMTP server authentication why not adding a login
> and password parameters in the config file to use it ?

Is this something that is needed, or something that would be a nice feature? 
Most (?) SMTP servers aren't configured to require authentication, or at least
can be configured to not require it from certain hosts.

If it's all the same, I'd like to add features like that at a later date,
especially after editparams gets its much-needed overhaul.

Comment 26

[Cédric Caron]

> > If Net::SMTP suport SMTP server authentication why not adding a login
> > and password parameters in the config file to use it ?
> Is this something that is needed, or something that would be a nice feature? 
> Most (?) SMTP servers aren't configured to require authentication, or at 
least

This is just a request but my ISP is now using authentication to protect 
against spam.

and it is probably not the only one.

Comment 27

[Erik Stambaugh]

> This is just a request but my ISP is now using authentication to protect 
> against spam.
> 
> and it is probably not the only one.

The current release version of Bugzilla still requires you to have a
locally-installed copy of Sendmail, so if you still have that, your upstream
ISP's SMTP requirements are probably not an issue since you can connect to
localhost.

I think this and other SMTP features should be included, just not in this patch.

Comment 28

[Erik Stambaugh]

Attachment: Cleaned a little bitrot

Comment 29

[Jouni Heikniemi]

Comment on attachment 154150 [details] [diff] [review]
Cleaned a little bitrot

>Index: defparams.pl
>===================================================================
>+   name => 'mailhost',
>+   desc => 'Host or IP address to use for outbound mail delivery',
>+   type => 't',
>+   default => 'localhost'

How about "smtphost" and "outbound SMTP mail delivery"? (just to be ready for
the next generation mail protocol)

>+   name    => 'mailtimeout',
>+   desc    => 'Number of seconds to wait on SMTP connections before giving up',

Also, "smtptimeout"

>+   name    => 'emaildelivery',

1) "outboundmailenabled"? (or something like that; booleans should sounds like
booleans)

>+   desc    => 'Is bug and other email from Bugzilla currently \'on\'?' .
>+              '(site-wide toggle)',

1) A space between the lines is missing. 
2) The desc is confusing... "Is outbound email from Bugzilla currently
enabled?" perhaps?


>Index: globals.pl
>===================================================================

>-                my $msg = PerformSubsts(Param("voteremovedmail"),
>+            my $msg = PerformSubsts(Param("voteremovedmail"),
>                                         \%substs);

Nit: Realign the \%substs under the Param.

>Index: Bugzilla/Flag.pm
>===================================================================

>+        Bugzilla::Mail::Outbound::send_message($mailparams) or
>+            ThrowCodeError("smtp_error",
>+                { "error" => $Bugzilla::Mail::Outbound::error },
>+            );

... I wonder, since we're repeating this ThrowCodeError in pretty many places,
shouldn't we have a send_message_or_throw_up version which would have the error
handling built in?

Also, since we're practically always constructing the mail hash with the
constant three params, how about if the wrapper method took three (or perhaps
even two - from value seems to be pretty constant here) scalar params instead
of that full hash? I have nothing against the current param structure, but
constructing the hash manually every time if unnecessarily verbose. I'd love it
if we could simplify this mail stuff once and for all.

See my comment about CreateMessage sub in bug 84876 comment 158.

>Index: Bugzilla/Token.pm
>===================================================================

>+    Bugzilla::Mail::Outbound::send_message($mailparams);

What about failure here? (repeat two more times for this file) 

>Index: Bugzilla/Mail/Outbound.pm
>===================================================================

>+# ...and it sends a single message in a single SMTP session

Nit: Space after "..."

>+# I like the idea of having a separate session for each outbound message
>+# because a single failure in a series of messages has a chance of messing
>+# things up for the whole connection, thereby poisoning the whole queue.

Nit: I agree, but don't write "I like" in source, because nobody knows who
wrote it. Just say "The idea here is to have a separate..."

>+# It is assumed that a nearby MTA is being used, so the performance gains for
>+# using a single connection are probably marginal.

Nit: "reusing the same connection" is way more clear. Also use "would be"
instead "are" so it's absolutely clear what you're talking about.

>+    unless (&::Param('emaildelivery')) {
>+        return 1;
>+    }

Err... This is a matter of opinion, but in a case like this I'd find "if
(!...)" much more readable than "unless (...)".

>+    # As for the validity of their content, that's going to be the MTA's
>+    # problem, since we have to check it for error messages no matter what,
>+    # and there are innumerable strange things in email that actually work at
>+    # some sites.

Eww. How about:

We don't validate the parameters but rather leave it up to the MTA to
avoid coding for site-wide specifics here. We'll check for SMTP errors
later on.

>+    for (qw(to from message)) {
>+        unless (length($params->{$_})) {
>+            $missing_params = join("", ($missing_params, $_));
>+        }
>+    }

Umm... This is pretty verbose (and besides, it causes warnings when length hits
an undef value). How about just

my @required_params = qw/to from message/;
my $missing_params = 
    join(", ", grep { !length($params->{$_} || '') } @required_params);

>+            $error = $smtp->code() . "  " . $smtp->message();

A single space should probably suffice (Internet protocols usually only have
that one space separating error codes and messages anyway).


You should also fix whine.pl.


As you can tell, those were pretty small problems. Looks very good indeed :-)

Comment 30

[:glob ✱]

+    unless ($smtp) {
+        $error = "Could not create a Net::SMTP Object";
+        return 0;
+    }

shouldn't the error message include $!

Comment 31

[Nick Barnes]

See also bug 270611 on the sendmail path problenm.  Not trying to preempt
anything here on on bug 84876, just trying to bite off a tiny piece to start with.

Comment 32

[Max Kanat-Alexander]

Now that bug 84876 is a meta-bug once again, does anybody have any code that
accomplishes this? It would be nice to use SMTP, although it's definitely more
difficult to deal with error conditions than with sendmail.

Comment 33

[Max Kanat-Alexander]

And you know, maybe I should stay out of this, bug doesn't it seem like bug
65101 would be an easier way to accomplish what the summary of this bug says the
goal is? (That is, if Mail::Sendmail is still a current, useful module from CPAN.)

Comment 34

[Max Kanat-Alexander]

OK, I just actually *read* all of bug 84876.

It looks like at one point we were going to use Mail::Mailer and Mail::Send
(both part of the MailTools package), which are modern and actively maintained
(last CPAN release on 2004-11-24). Those seem like good abstractions that also
handle interfacing with Net::SMTP if necessary.

I know that bbaetz had some issues with Mail::Send when he wrote it into his own
patch, but it's been a few *years* since then, and perhaps some of thos issues
have been resolved since then?

With the simplicity of Mail::Send and Mail::Mailer, I'd imagine this would be
all somewhat easier than having Bugzilla manually do the SMTP handling?

Comment 35

[Dave Miller [:justdave]]

*** Bug 277106 has been marked as a duplicate of this bug. ***

Comment 36

[Dave Miller [:justdave]]

OK, bug 277437 appears to be an exact duplicate of this bug, but went about
fixing it in a different way (and there is a "review+" patch there).  Should we
dupe this to that one?

Comment 37

[Max Kanat-Alexander]

Well, it would be nice to have this one still show up in the bug list when we're
doing release notes.

So let's just say that that one will fix this one.

Comment 38

[Erik Stambaugh]

(In reply to comment #37)
> Well, it would be nice to have this one still show up in the bug list when we're
> doing release notes.
> 
> So let's just say that that one will fix this one.

I'm not sure why that one wasn't resolved as a duplicate of this one and the
work wasn't done here.

Comment 39

[Max Kanat-Alexander]

Added to the release notes in bug 286274.