Bug 49893 (bz-smtp)

Ability to use different SMTP server

RESOLVED FIXED in Bugzilla 2.20

Status

()

enhancement
P3
normal
RESOLVED FIXED
19 years ago
5 years ago

People

(Reporter: asric, Assigned: erik)

Tracking

unspecified
Bugzilla 2.20
Dependency tree / graph

Details

(Whiteboard: [fixed by blocker])

Attachments

(1 attachment, 5 obsolete attachments)

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.
this is possible using any number of perl sendmail modules, but that would 
involve dragging in more dependencies into the main product
Target Milestone: --- → Future
Whiteboard: MTAConfig
No longer depends on: 84876
Assignee: tara → justdave
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → unspecified
It may be convenient to group  the SMTP server setting with that server setting
of each account. 
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.
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)
Assignee: justdave → nobody
Hardware: PC → All
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.
Assignee: nobody → erik
Status: NEW → ASSIGNED
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.
Blocks: 84876
No longer depends on: 84876
Whiteboard: MTAConfig → MTAConfig [needed for bzWin32]
Posted patch Net::SMTP patch (obsolete) — Splinter Review
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.
Attachment #151741 - Attachment is obsolete: true
(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.
What do we do about 4xx error codes??  
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.
(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.
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?
The previous patch didn't have checksetup looking for Net::SMTP.
Attachment #151746 - Attachment is obsolete: true
Comment on attachment 151971 [details] [diff] [review]
Net::SMTP patch (with checksetup)

I think this might be ready for prime time.
Attachment #151971 - Flags: review?(bugreport)
Blocks: 249121
In checksetup.pl the entry in the %ppm_modules table is mission for new module.
(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 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
Attachment #151971 - Flags: review?(bugreport) → review-
(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.
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.
This addresses the above.  Have at you.
Attachment #151971 - Attachment is obsolete: true
Attachment #152037 - Flags: review?(bugreport)
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.
Attachment #152037 - Flags: review?(bugreport) → review-
> 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.
If Net::SMTP suport SMTP server authentication why not adding a login and 
password parameters in the config file to use it ?
(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.
Attachment #152037 - Attachment is obsolete: true
Attachment #153194 - Flags: review?(bugreport)
> > 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.
> 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.
Attachment #153194 - Attachment is obsolete: true
Attachment #154150 - Flags: review?
Attachment #153194 - Flags: review?(bugreport)
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 :-)
Attachment #154150 - Flags: review? → review-
+    unless ($smtp) {
+        $error = "Could not create a Net::SMTP Object";
+        return 0;
+    }

shouldn't the error message include $!


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.
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.
Alias: bz-smtp
No longer blocks: 178370
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.)
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?
*** Bug 277106 has been marked as a duplicate of this bug. ***
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?
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.
Depends on: 277437
Keywords: relnote
Whiteboard: MTAConfig [needed for bzWin32] → MTAConfig [needed for bzWin32] [blocker will fix]
(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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: MTAConfig [needed for bzWin32] [blocker will fix] → [fixed by blocker]
Target Milestone: Future → Bugzilla 2.20
Added to the release notes in bug 286274.
Keywords: relnote
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.