[pve-devel] [PATCH ha-manager] fix #1347: let postfix fill in FQDN in fence mails

[Thomas Lamprecht]

On 09/14/2017 11:15 AM, Philip Abernethy wrote:
> On Wed, 2017-09-13 at 08:11 +0200, Thomas Lamprecht wrote:
>> On 09/08/2017 02:56 PM, Philip Abernethy wrote:
>>> Using the nodename is not correct and can lead to mails not
>>> forwarding
>>> in restrictive mail server configurations.
>>> ---
>>>    src/PVE/HA/Env/PVE2.pm | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
>>> index fdfadd7..722d1b7 100644
>>> --- a/src/PVE/HA/Env/PVE2.pm
>>> +++ b/src/PVE/HA/Env/PVE2.pm
>>> @@ -204,8 +204,8 @@ sub log {
>>>    sub sendmail {
>>>        my ($self, $subject, $text) = @_;
>>>    
>>> -    my $mailfrom = 'root@' . $self->nodename();
>>> -    my $mailto = 'root at localhost';
>>> +    my $mailfrom = 'root';
>>> +    my $mailto = 'root';
>>
>> needs this to change? root at localhost worked as intended not for
>> "mailto"?
> 
> my $mailto = 'root at localhost';:
> X-original-to: root at localhost
> Delivered-to: root at localhost
> 
> my $mailto = 'root';:
> X-original-to: root
> Delivered-to: root at ceph2.proxmox.com
> 
> The result is the same, pvemailforward forwarding the mail to whatever
> address is configured in the datacenter's user config. But I think it's
>   more consistent that way.

OK, so if you want this then please write it in the commit message that
you touched and changed the second value just for consistence sake not
for fixing the bug.

>>
>> A comment would be nice too if we take this approach, so that the
>> unsuspecting reader may understand that while both values are root
>> they
>> mean something different when resolved by pvemailforward...
> 
> Fair. Can do. Although they don't really mean anything different. They
> aren't even handled differently. There's simply a forwarding for mail
> delivered to the root's mailbox.
>>
>> Besides that looks OK.
>>
>> But I'd still like to see the sendmail function a bit  adapted so
>> that this
>> is possible:
>>
>> sendmail($subject, text => $foo);  # send to root configured email a
>> plain text mail with roots or dc->email_from as from
>> sendmail($subject, text => $foo, html => $bar); # same but also a
>> HTML part
>> sendmail($subject, to => $backup_mail, text => $foo, html => $bar); #
>> same as above but a custom "to" (e.g. for backups)
>>
>> Seems like a nicer interface for the user to me, thoughts?
> 
> Originally I had edited PVE::Tools::sendmail to use named arguments,
> but that resulted in having to touch the backup file as well. So I'd be
> touching three files for barely any gain. The backup uses all fields
> anyway and the ha-manager only has a single undefined in the call.
> If you say you prefer named arguments, sure I can add it back in.
>>

FYI, theres another sendmail (the binary not our function) usage in:
pve-manager/PVE/API2/APT.pm


A nicer interface is never "barely any gain" ;) But yes you are
right here, the quick'n lazy (in the good sense) fix is this.
So if you give me a comment and improved commit message I'm totally
OK with this.


>>>    
>>>        PVE::Tools::sendmail($mailto, $subject, $text, undef,
>>> $mailfrom);
>>>    }
>>>
>>
>>