[pmg-devel] [PATCH pmg-api v2 2/5] use postgres inet functions for greylist matching
Dominik Csapak
d.csapak at proxmox.com
Mon Apr 20 16:28:27 CEST 2020
nits inline
On 4/20/20 1:22 PM, Stoiko Ivanov wrote:
> In preparation for adding support for a configurable greylist netmask [0]
> and greylisting for ipv6 hosts the width of the IPNet column of the cgreylist
> table needs to be extended to 49 [1].
>
> Instead of comparing the first 3 octets of a ipv4 address we store the
> complete network definition (i.e. for 192.0.2.127/24 - 192.0.2.0/24)
> The last octet is not saved, but written as 0 (the information is not
> needed, and not used currently). The generation of the network is done
> with postgresql's functions for the inet and cidr datatypes [2,3].
>
> The change of the column width instead of using the inet datatype prevents
> errors while syncing or downgrading, although older nodes in a cluster (or
> downgraded nodes) will not match new records.
>
> When syncing from a node with old-style data the rows are inserted in the
> new format.
>
> Upon upgrade (`pmgdb init` in the postinst script) the data is changed to
> the new format and matched for duplicates (in case one node in the cluster
> got upgraded and it's contents were synced we should not edit the data
> again). This process does cause the Cgreylist table to be scanned, which
> takes time linear in the number of rows (e.g. with a test-dataset of
> ~ 1 million rows the upgrade is blocked for ~50 seconds on an
> average testinstallation).
> Changing only the column datatype does not lock the table and is almost
> instantenous [4].
>
> [0] defining from which neighbors a mail is accepted on the second attempt
> [1] INET6_ADDRSTRLEN is 46 + 4 for the netmask ('/128) - \0
> [2] https://www.postgresql.org/docs/11/datatype-net-types.html
> [3] https://www.postgresql.org/docs/11/functions-net.html
> [4] Notes section of https://www.postgresql.org/docs/11/sql-altertable.html
>
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
> src/PMG/Cluster.pm | 7 +++++--
> src/PMG/DBTools.pm | 43 +++++++++++++++++++++++++++++++++++++++---
> src/bin/pmgpolicy | 47 ++++++++++++++++++++++++++--------------------
> 3 files changed, 72 insertions(+), 25 deletions(-)
>
> diff --git a/src/PMG/Cluster.pm b/src/PMG/Cluster.pm
> index e890924..f99232d 100644
> --- a/src/PMG/Cluster.pm
> +++ b/src/PMG/Cluster.pm
> @@ -763,12 +763,15 @@ sub sync_greylist_db {
> "mtime >= $lastmt AND CID != 0";
> };
>
> - my $merge_sth = $dbh->prepare($PMG::DBTools::cgreylist_merge_sql);
> + # FIXME: drop Host column with PMG 7.0
> + my $merge_sth = $dbh->prepare(PMG::DBTools::cgreylist_merge_sql());
> my $mergefunc = sub {
> my ($ref) = @_;
>
> + my $ipnet = $ref->{ipnet};
> + $ipnet .= '.0/24' if $ipnet !~ /\/\d+$/;
> $merge_sth->execute(
> - $ref->{ipnet}, $ref->{host}, $ref->{sender}, $ref->{receiver},
> + $ipnet, 0, $ref->{sender}, $ref->{receiver},
> $ref->{instance}, $ref->{rctime}, $ref->{extime}, $ref->{delay},
> $ref->{blocked}, $ref->{passed}, 0, $ref->{cid});
> };
> diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
> index a7a18a4..269ff3b 100644
> --- a/src/PMG/DBTools.pm
> +++ b/src/PMG/DBTools.pm
> @@ -18,10 +18,16 @@ use PMG::Utils qw(postgres_admin_cmd);
>
> our $default_db_name = "Proxmox_ruledb";
>
> -our $cgreylist_merge_sql =
> +# FIXME: drop Host column with PMG 7.0
> +sub cgreylist_merge_sql {
> + my ($with_mask) = @_;
> +
> + my $network = $with_mask ? 'network(set_masklen(?, ?))' : '?';
> +
> + my $sql =
> 'INSERT INTO CGREYLIST (IPNet,Host,Sender,Receiver,Instance,RCTime,' .
> 'ExTime,Delay,Blocked,Passed,MTime,CID) ' .
> - 'VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ' .
> + "VALUES ($network, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) " .
> 'ON CONFLICT (IPNet,Sender,Receiver) DO UPDATE SET ' .
> 'Host = CASE WHEN CGREYLIST.MTime >= excluded.MTime THEN CGREYLIST.Host ELSE excluded.Host END,' .
> 'CID = GREATEST(CGREYLIST.CID, excluded.CID), RCTime = LEAST(CGREYLIST.RCTime, excluded.RCTime),' .
> @@ -30,6 +36,9 @@ our $cgreylist_merge_sql =
> 'Blocked = GREATEST(CGREYLIST.Blocked, excluded.Blocked),' .
> 'Passed = GREATEST(CGREYLIST.Passed, excluded.Passed)';
>
> + return $sql;
> +}
> +
> sub open_ruledb {
> my ($database, $host, $port) = @_;
>
> @@ -102,7 +111,7 @@ sub database_list {
>
> my $cgreylist_ctablecmd = <<__EOD;
> CREATE TABLE CGreylist
> - (IPNet VARCHAR(16) NOT NULL,
> + (IPNet VARCHAR(49) NOT NULL,
> Host INTEGER NOT NULL,
> Sender VARCHAR(255) NOT NULL,
> Receiver VARCHAR(255) NOT NULL,
> @@ -522,6 +531,34 @@ sub upgradedb {
> "AND value = 'content-type:application/x-java-vm';");
> };
>
> + # FIXME: drop Host column with PMG 7.0
> + # increase column size of cgreylist.ipnet for ipv6 support and transfer data
> + eval {
> + my $sth = $dbh->prepare("SELECT character_maximum_length ".
> + "FROM information_schema.columns ".
> + "WHERE table_name = 'cgreylist' AND column_name = 'ipnet'");
> + $sth->execute();
> + my $res = $sth->fetchrow_hashref();
> + if ($res->{character_maximum_length} == 16) {
> + $dbh->begin_work;
> + $dbh->do("ALTER TABLE CGreylist ALTER COLUMN " .
> + "IPNet TYPE varchar(49)");
> + eval {
> + $dbh->do("UPDATE CGreylist cg1 SET IPNet = IPNet || '.0/24' ".
> + "WHERE position('/' in IPNet) = 0 AND ".
> + "NOT EXISTS (SELECT 1 FROM CGreylist cg2 WHERE ".
> + "cg2.IPNet = cg1.IPNet || '.0/24' AND ".
> + "cg1.Receiver = cg2.Receiver AND cg1.Sender = cg2.Sender)");
> + };
> + #ignore errors here - legacy rows will eventually expire
> + $dbh->commit;
> + }
> + };
> + if (my $err = $@) {
> + $dbh->rollback;
> + die $err;
> + }
> +
> foreach my $table (keys %$tables) {
> eval { $dbh->do("ANALYZE $table"); };
> warn $@ if $@;
> diff --git a/src/bin/pmgpolicy b/src/bin/pmgpolicy
> index ebc24ce..b371525 100755
> --- a/src/bin/pmgpolicy
> +++ b/src/bin/pmgpolicy
> @@ -553,6 +553,7 @@ sub greylist_value {
>
> my ($net, $host) = $ip =~ m/(\d+\.\d+\.\d+)\.(\d+)/; # IPv4 for now
> return 'dunno' if !defined($net);
> + my $masklen = 24;
>
> my $spf_header;
>
> @@ -605,9 +606,10 @@ sub greylist_value {
> # check if there is already a record in the GL database
> my $sth = $dbh->prepare(
> "SELECT * FROM CGreylist " .
> - "where IPNet = ? AND Sender = ? AND Receiver = ?");
> + "WHERE IPNet::cidr = network(set_masklen(?, ?)) AND ".
> + "Sender = ? AND Receiver = ?");
>
> - $sth->execute($net, $sender, $rcpt);
> + $sth->execute($ip, $masklen, $sender, $rcpt);
> my $ref = $sth->fetchrow_hashref();
> $sth->finish();
>
> @@ -617,8 +619,9 @@ sub greylist_value {
>
> if (!defined($ref->{rctime})) {
>
> - $dbh->do($PMG::DBTools::cgreylist_merge_sql, undef,
> - $net, $host, $sender, $rcpt, $instance,
> + # FIXME: drop Host column with PMG 7.0
> + $dbh->do(PMG::DBTools::cgreylist_merge_sql(1), undef,
> + $ip, $masklen, 0, $sender, $rcpt, $instance,
> $ctime, $ctime + 10, 0, 100000, 0, $ctime, $self->{lcid});
> }
>
> @@ -659,9 +662,10 @@ sub greylist_value {
>
> my $sth = $dbh->prepare(
> "SELECT * FROM CGreylist " .
> - "where IPNet = ? AND Sender = ? AND Receiver = ?");
> + "WHERE IPNet::cidr = network(set_masklen(?, ?)) AND ".
> + "Sender = ? AND Receiver = ?");
>
> - $sth->execute($net, $sender, $rcpt);
> + $sth->execute($ip, $masklen, $sender, $rcpt);
>
> my $ref = $sth->fetchrow_hashref();
>
> @@ -669,9 +673,9 @@ sub greylist_value {
>
> if (!defined($ref->{rctime})) {
>
> - $dbh->do($PMG::DBTools::cgreylist_merge_sql, undef,
> - $net, $host, $sender, $rcpt, $instance,
> - $ctime, $ctime + $greylist_lifetime, 0, 1, 0, $ctime, $self->{lcid});
> + $dbh->do(PMG::DBTools::cgreylist_merge_sql(1), undef,
> + $ip, $masklen, 0, $sender, $rcpt, $instance,
> + $ctime, $ctime + $greylist_lifetime, 0, 1, 0, $ctime, $self->{lcid});
nit: the indentation is wrong, either align with the brackets or use one
extra level please
>
> $res = $defer_res;
> $self->log(3, "defer greylisted mail");
> @@ -683,27 +687,30 @@ sub greylist_value {
> $res = $defer_res;
> $self->log(3, "defer greylisted mail");
> $dbh->do("UPDATE CGreylist " .
> - "SET Blocked = Blocked + 1, Host = ?, MTime = ? " .
> - "WHERE IPNet = ? AND Sender = ? AND Receiver = ?", undef,
> - $host, $ctime, $net, $sender, $rcpt);
> + "SET Blocked = Blocked + 1, MTime = ? " .
> + "WHERE IPNet::cidr = network(set_masklen(?, ?)) AND ".
> + "Sender = ? AND Receiver = ?", undef,
> + $ctime, $ip, $masklen, $sender, $rcpt);
nit: indentation again
> } else {
> if ($ctime < $ref->{extime}) {
> # accept (not expired)
> my $lifetime = $sender eq "" ? 0 : $greylist_awlifetime;
> my $delay = $ref->{passed} ? "" : "Delay = $age, ";
> $dbh->do("UPDATE CGreylist " .
> - "SET Passed = Passed + 1, $delay Host = ?, ExTime = ?, MTime = ? " .
> - "WHERE IPNet = ? AND Sender = ? AND Receiver = ?", undef,
> - $host, $ctime + $lifetime, $ctime, $net, $sender, $rcpt);
> + "SET Passed = Passed + 1, $delay ExTime = ?, MTime = ? " .
> + "WHERE IPNet::cidr = network(set_masklen(?, ?)) AND ".
> + "Sender = ? AND Receiver = ?", undef,
> + $ctime + $lifetime, $ctime, $ip, $masklen, $sender, $rcpt);
also here
> } else {
> # defer (record is expired)
> $res = $defer_res;
> $dbh->do("UPDATE CGreylist " .
> - "SET Host = ?, RCTime = ?, ExTime = ?, MTime = ?, Instance = ?, " .
> - "Blocked = 1, Passed = 0 " .
> - "WHERE IPNet = ? AND Sender = ? AND Receiver = ?", undef,
> - $host, $ctime, $ctime + $greylist_lifetime, $ctime, $instance,
> - $net, $sender, $rcpt);
> + "SET RCTime = ?, ExTime = ?, MTime = ?, Instance = ?, " .
> + "Blocked = 1, Passed = 0 " .
> + "WHERE IPNet::cidr = network(set_masklen(?, ?)) AND ".
> + "Sender = ? AND Receiver = ?", undef,
> + $ctime, $ctime + $greylist_lifetime, $ctime, $instance,
> + $ip, $masklen, $sender, $rcpt);
and here
> }
> }
> }
>
More information about the pmg-devel
mailing list