[pmg-devel] [PATCH] Close #2438: api: add lmtp support

[Stoiko Ivanov]

Hi,

I gave your patches a spin and found 3 small glitches and one (only partially
related) issue, which would need addressing:

On Mon,  9 Dec 2019 21:42:31 +0100
Julian Zehnter <pmg-devel at j-z.it> wrote:

> Signed-off-by: Julian Zehnter <pmg-devel at j-z.it>
> ---
>  src/PMG/API2/Transport.pm     | 21 ++++++++++++++++++---
>  src/PMG/Config.pm             |  9 ++++++++-
>  src/templates/main.cf.in      |  4 ++--
>  src/templates/main.cf.in.demo |  4 ++--
>  4 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/src/PMG/API2/Transport.pm b/src/PMG/API2/Transport.pm
> index 98ab414..36b2720 100644
> --- a/src/PMG/API2/Transport.pm
> +++ b/src/PMG/API2/Transport.pm
> @@ -33,6 +33,7 @@ __PACKAGE__->register_method ({
>  	    properties => {
>  		domain => { type => 'string' },
>  		host => { type => 'string' },
> +		protocol => { type => 'string' },
You could change the type to 'enum' here as well (as done for the relay_transport in Config.pm)?

>  		port => { type => 'integer' },
>  		use_mx => { type => 'boolean' },
>  		comment => { type => 'string'},
> @@ -73,8 +74,14 @@ __PACKAGE__->register_method ({
>  		description => "Target host (name or IP address).",
>  		type => 'string', format => 'address',
>  	    },
> +	    protocol => {
> +		description => "Transport protocol.",
> +		type => 'string',
> +		default => 'smtp',
> +		optional => 1,
> +	    },
>  	    port => {
> -		description => "SMTP port.",
> +		description => "Transport port.",
>  		type => 'integer',
>  		minimum => 1,
>  		maximum => 65535,
> @@ -108,6 +115,7 @@ __PACKAGE__->register_method ({
>  	    $tmap->{$param->{domain}} = {
>  		domain => $param->{domain},
>  		host => $param->{host},
> +		protocol => $param->{protocol} // smtp,
>  		port => $param->{port} // 25,
>  		use_mx => $param->{use_mx} // 1,
>  		comment => $param->{comment} // '',
> @@ -144,6 +152,7 @@ __PACKAGE__->register_method ({
>  	properties => {
>  	    domain => { type => 'string'},
>  	    host => { type => 'string'},
> +	    protocol => { type => 'string'},
>  	    port => { type => 'integer'},
>  	    use_mx => { type => 'boolean'},
>  	    comment => { type => 'string'},
> @@ -181,8 +190,14 @@ __PACKAGE__->register_method ({
>  		type => 'string', format => 'address',
>  		optional => 1,
>  	    },
> +	    protocol => {
> +		description => "Transport protocol.",
> +		type => 'string',
> +		default => 'smtp',
> +		optional => 1,
> +	    },
>  	    port => {
> -		description => "SMTP port.",
> +		description => "Transport port.",
>  		type => 'integer',
>  		minimum => 1,
>  		maximum => 65535,
> @@ -216,7 +231,7 @@ __PACKAGE__->register_method ({
>  
>  	    die "no options specified\n" if !scalar(keys %$param);
>  
> -	    for my $prop (qw(host port use_mx comment)) {
> +	    for my $prop (qw(host protocol port use_mx comment)) {
>  		$data->{$prop} = $param->{$prop} if defined($param->{$prop});
>  	    }
>  
> diff --git a/src/PMG/Config.pm b/src/PMG/Config.pm
> index 6e0a37c..7f3c2bd 100755
> --- a/src/PMG/Config.pm
> +++ b/src/PMG/Config.pm
> @@ -480,8 +480,14 @@ sub properties {
>  	    description => "The default mail delivery transport (incoming mails).",
>  	    type => 'string', format => 'address',
>  	},
> +	replayprotocol => {
a small typo: relayprotocol vs. replayprotocol

> +	    description => "Transport protocol for relay host.",
> +	    type => 'string',
> +	    enum => [qw(smtp lmtp)],
> +	    default => 'smtp',
> +	},
>  	relayport => {
> -	    description => "SMTP port number for relay host.",
> +	    description => "Transport port number for relay host.",
>  	    type => 'integer',
>  	    minimum => 1,
>  	    maximum => 65535,
> @@ -647,6 +653,7 @@ sub options {
>  	smarthost => { optional => 1 },
>  	smarthostport => { optional => 1 },
>  	relay => { optional => 1 },
> +	replayprotocol => { optional => 1 },
same here

>  	relayport => { optional => 1 },
>  	relaynomx => { optional => 1 },
>  	dwarning => { optional => 1 },

One thing which is missing in Config.pm is adapting 'read_transport_map'
and 'write_transport_map' (the subs, which parse and write '/etc/pmg/transports')
to actually return and write the protocol as well - e.g. the following should work
for read_transport_map:

if ($line =~ m/^(\S+)\s+([sl]mtp):(\S+):(\d+)\s*$/) {
    my ($domain, $protocol, $host, $port) = ($1, $2, $3, $4);


> diff --git a/src/templates/main.cf.in b/src/templates/main.cf.in
> index 7bf9afa..2bad66a 100644
> --- a/src/templates/main.cf.in
> +++ b/src/templates/main.cf.in
> @@ -35,9 +35,9 @@ transport_maps = hash:/etc/pmg/transport
>  
>  [% IF pmg.mail.relay %]
>  [% IF pmg.mail.relaynomx %]
> -relay_transport = smtp:[[% pmg.mail.relay %]]:[% pmg.mail.relayport %]
The lmtp transport does not recognize the no MX lookup option (see `man 8 smtp`)
or rather ignores it - however the setting of nomx loses it's meaning for lmtp
it would be great to make the setting mutually exclusive (but that would be on
the GUI side).

If I read the man-page right then the lmtp transport would need to start with
'inet:' - question is whether we would want the users to enter the complete
nexthop ('inet:192.0.2.1') or only the hostname/ip and then have the 'inet:' prefix
fixed in the template (for relay_transport) and write_transport_map (for the transports).
I think I would prefer the latter (otherwise the field should maybe not be named 'Host').


> +relay_transport = [% pmg.mail.protocol %]:[[% pmg.mail.relay %]]:[% pmg.mail.relayport %]
>  [% ELSE %]
> -relay_transport = smtp:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
> +relay_transport = [% pmg.mail.protocol %]:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
>  [% END %]
>  [% END %]
>  
> diff --git a/src/templates/main.cf.in.demo b/src/templates/main.cf.in.demo
> index 2c346ec..2c47785 100644
> --- a/src/templates/main.cf.in.demo
> +++ b/src/templates/main.cf.in.demo
> @@ -35,9 +35,9 @@ transport_maps = hash:/etc/pmg/transport
>  
>  [% IF pmg.mail.relay %]
>  [% IF pmg.mail.relaynomx %]
> -relay_transport = smtp:[[% pmg.mail.relay %]]:[% pmg.mail.relayport %]
> +relay_transport = [% pmg.mail.protocol %]:[[% pmg.mail.relay %]]:[% pmg.mail.relayport %]
>  [% ELSE %]
> -relay_transport = smtp:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
> +relay_transport = [% pmg.mail.protocol %]:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
>  [% END %]
>  [% END %]
>  

Thanks!
stoiko