[pmg-devel] [PATCH v2 pmg-api] Feature #2438 add support for lmtp delivery to downstream servers
Stoiko Ivanov
s.ivanov at proxmox.com
Wed Jan 8 20:21:03 CET 2020
On Sun, 5 Jan 2020 15:53:07 +0100
Julian Zehnter <pmg-devel at j-z.it> wrote:
> new feature lmtp support for simplifying setups
> with lmtp capable downstream servers (e.g. dovecot)
> Postfix support lmtp out of the box and can now deliver
> mails directly to internal mailbox servers without
> one more smtp connection
>
> extending the api code for new lmtp option:
> Config.pm:
> Adding new variable "relayprotocol"
> Extending the read_transport_map & write_transport_map
> for parsing the /etc/pmg/transport
>
> Transport.pm:
> Add new protcol varialbe for smtp/lmtp setting
> Generalizing some "SMTP" keywords
>
> Templates:
> Adapting the main.cf templates for adding the lmtp keyword
>
> Signed-off-by: Julian Zehnter <pmg-devel at j-z.it>
> ---
> src/PMG/API2/Transport.pm | 27 ++++++++++++++++++++++-----
> src/PMG/Config.pm | 30 +++++++++++++++++++++++-------
> src/templates/main.cf.in | 8 ++++++--
> src/templates/main.cf.in.demo | 8 ++++++--
> 4 files changed, 57 insertions(+), 16 deletions(-)
>
> diff --git a/src/PMG/API2/Transport.pm b/src/PMG/API2/Transport.pm
> index 98ab414..d35ccc3 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' },
> port => { type => 'integer' },
> use_mx => { type => 'boolean' },
> comment => { type => 'string'},
> @@ -73,8 +74,15 @@ __PACKAGE__->register_method ({
> description => "Target host (name or IP address).",
> type => 'string', format => 'address',
> },
> + protocol => {
> + description => "Transport protocol.",
> + type => 'string',
> + enum => [qw(smtp lmtp)],
nit: wrong indentation level
> + default => 'smtp',
> + optional => 1,
> + },
> port => {
> - description => "SMTP port.",
> + description => "Transport port.",
> type => 'integer',
> minimum => 1,
> maximum => 65535,
> @@ -82,7 +90,7 @@ __PACKAGE__->register_method ({
> default => 25,
> },
> use_mx => {
> - description => "Enable MX lookups.",
> + description => "Enable MX lookups (SMTP).",
> type => 'boolean',
> optional => 1,
> default => 1,
> @@ -108,6 +116,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 +153,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,15 +191,22 @@ __PACKAGE__->register_method ({
> type => 'string', format => 'address',
> optional => 1,
> },
> + protocol => {
> + description => "Transport protocol.",
> + type => 'string',
> + enum => [qw(smtp lmtp)],
> + default => 'smtp',
> + optional => 1,
> + },
> port => {
> - description => "SMTP port.",
> + description => "Transport port.",
> type => 'integer',
> minimum => 1,
> maximum => 65535,
> optional => 1,
> },
> use_mx => {
> - description => "Enable MX lookups.",
> + description => "Enable MX lookups (SMTP).",
> type => 'boolean',
> optional => 1,
> },
> @@ -216,7 +233,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..fac87df 100755
> --- a/src/PMG/Config.pm
> +++ b/src/PMG/Config.pm
> @@ -480,15 +480,21 @@ sub properties {
> description => "The default mail delivery transport (incoming mails).",
> type => 'string', format => 'address',
> },
> + relayprotocol => {
> + 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,
> default => 25,
> },
might be more understandable (since it ends up in the docs) if we
write this explicitly as "SMTP/LMTP port number for relay host"
> relaynomx => {
> - description => "Disable MX lookups for default relay.",
> + description => "Disable MX lookups for default relay (SMTP).",
by rephrasing the parenthesis as: (SMTP only, ignored for LMTP)
we get the necessary information in the docs automatically.
> type => 'boolean',
> default => 0,
> },
> @@ -647,6 +653,7 @@ sub options {
> smarthost => { optional => 1 },
> smarthostport => { optional => 1 },
> relay => { optional => 1 },
> + relayprotocol => { optional => 1 },
> relayport => { optional => 1 },
> relaynomx => { optional => 1 },
> dwarning => { optional => 1 },
> @@ -1118,8 +1125,8 @@ sub read_transport_map {
> $comment = '';
> };
>
> - if ($line =~ m/^(\S+)\s+smtp:(\S+):(\d+)\s*$/) {
> - my ($domain, $host, $port) = ($1, $2, $3);
> + if ($line =~ m/^(\S+)\s+(?:(lmtp):inet|(smtp)):(\S+):(\d+)\s*$/) {
> + my ($domain, $protocol, $host, $port) = ($1, ($2 or $3), $4, $5);
cosmetic:
maybe
if ($line =~ m/^(\S+)\s+(lmtp:inet|smtp):(\S+):(\d+)\s*$/) {
my ($domain, $protocol, $host, $port) = ($1, $2, $3, $4);
$protocol =~ s/:inet$//;
would be a bit better readable?
>
> eval { pmg_verify_transport_domain_or_email($domain); };
> if (my $err = $@) {
> @@ -1131,6 +1138,7 @@ sub read_transport_map {
> $host = $1;
> $use_mx = 0;
> }
> + $use_mx = 0 if ($protocol eq "lmtp");
>
> eval { PVE::JSONSchema::pve_verify_address($host); };
> if (my $err = $@) {
> @@ -1140,6 +1148,7 @@ sub read_transport_map {
>
> my $data = {
> domain => $domain,
> + protocol => $protocol,
> host => $host,
> port => $port,
> use_mx => $use_mx,
> @@ -1170,12 +1179,19 @@ sub write_transport_map {
> my $use_mx = $data->{use_mx};
> $use_mx = 0 if $data->{host} =~ m/^(?:$IPV4RE|$IPV6RE)$/;
as an alternative to getting $is_lmtp below you could do
$use_mx = 0 if $data->{protocol} eq 'lmtp';
here and...
>
> - if ($use_mx) {
> + my $is_lmtp = 0;
> + $is_lmtp = 1 if $data->{protocol} eq "lmtp";
> +
> + if ($is_lmtp) {
> + $data->{protocol} = "lmtp:inet";
> + }
$data->{protocol} .= ':inet' if $data->{protocol} eq 'lmtp';
here (that's probably how I would have done that - but that's
not necessarily cleaner or better understandable...)
> +
> + if ($use_mx or $is_lmtp) {
> PVE::Tools::safe_print(
> - $filename, $fh, "$data->{domain} smtp:$data->{host}:$data->{port}\n");
> + $filename, $fh, "$data->{domain} $data->{protocol}:$data->{host}:$data->{port}\n");
> } else {
> PVE::Tools::safe_print(
> - $filename, $fh, "$data->{domain} smtp:[$data->{host}]:$data->{port}\n");
> + $filename, $fh, "$data->{domain} $data->{protocol}:[$data->{host}]:$data->{port}\n");
> }
> }
> }
> diff --git a/src/templates/main.cf.in b/src/templates/main.cf.in
> index 7bf9afa..aa52c08 100644
> --- a/src/templates/main.cf.in
> +++ b/src/templates/main.cf.in
> @@ -34,10 +34,14 @@ relay_domains = hash:/etc/pmg/domains
> transport_maps = hash:/etc/pmg/transport
>
> [% IF pmg.mail.relay %]
> +[% IF pmg.mail.relayprotocol == 'lmtp' %]
> +relay_transport = [% pmg.mail.relayprotocol %]:inet:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
> +[% ELSE %]
> [% IF pmg.mail.relaynomx %]
> -relay_transport = smtp:[[% pmg.mail.relay %]]:[% pmg.mail.relayport %]
> +relay_transport = [% pmg.mail.relayprotocol %]:[[% pmg.mail.relay %]]:[% pmg.mail.relayport %]
> [% ELSE %]
> -relay_transport = smtp:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
> +relay_transport = [% pmg.mail.relayprotocol %]:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
> +[% END %]
> [% END %]
> [% END %]
>
> diff --git a/src/templates/main.cf.in.demo b/src/templates/main.cf.in.demo
> index 2c346ec..61e2910 100644
> --- a/src/templates/main.cf.in.demo
> +++ b/src/templates/main.cf.in.demo
> @@ -34,10 +34,14 @@ relay_domains = hash:/etc/pmg/domains
> transport_maps = hash:/etc/pmg/transport
>
> [% IF pmg.mail.relay %]
> +[% IF pmg.mail.relayprotocol == 'lmtp' %]
> +relay_transport = [% pmg.mail.relayprotocol %]:inet:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
> +[% ELSE %]
> [% IF pmg.mail.relaynomx %]
> -relay_transport = smtp:[[% pmg.mail.relay %]]:[% pmg.mail.relayport %]
> +relay_transport = [% pmg.mail.relayprotocol %]:[[% pmg.mail.relay %]]:[% pmg.mail.relayport %]
> [% ELSE %]
> -relay_transport = smtp:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
> +relay_transport = [% pmg.mail.relayprotocol %]:[% pmg.mail.relay %]:[% pmg.mail.relayport %]
> +[% END %]
> [% END %]
> [% END %]
>
From my perspective the patch works as advertised and is clear (though
code-review by someone with more diligence/experience might be beneficial)
Short of the nits (which we can fixup with a follow-up commit):
Reviewed-By: Stoiko Ivanov <s.ivanov at proxmox.com>
Tested-By: Stoiko Ivanov <s.ivanov at proxmox.com>
More information about the pmg-devel
mailing list