Make Redis optional

Previously the greylist plugin tried to use redis and fell back to DBM. This means that if a system already had an established DBM database, but happened to have Redis running, the existing DBM db would be abandoned for a new Redis DB. This would inevitably lead to more delays for legitimate mail, and possibly lost mail.

This adds a 'redis_server' argument which enables Redis and sets the location of the redis server; if it is not explicitly set, DBM is used instead.

If the redis server is unavailable, rather than failing to start QP, we instead fail to register the plugin.

plugins/greylisting

@@ -104,6 +104,13 @@ usable directory from the following list will be used:
 
 =back
 
+=head2 redis_server <host[:port]>
+
+Location of redis server where the greylisting DB will be stored.
+
+Redis can be used as a scalable and clusterable alternative
+to a simple DBM file.  For more information, see http://redis.io
+
 =head2 per_recipient <bool>
 
 Flag to indicate whether to use per-recipient configs.
@@ -172,7 +179,7 @@ my $VERSION = '0.12';
 
 my $DENYMSG        = "This mail is temporarily denied";
 my %PERMITTED_ARGS = map { $_ => 1 } qw(per_recipient remote_ip sender
-  recipient black_timeout white_timeout deny_late db_dir
+  recipient black_timeout white_timeout deny_late db_dir redis_server
   nfslock p0f reject loglevel geoip upgrade );
 $PERMITTED_ARGS{grey_timeout} = 1; # Legacy argument now ignored
 
@@ -204,13 +211,8 @@ sub register {
         $config->{reject} = $config->{mode} =~ /testonly|off/i ? 0 : 1;
     }
     $self->{_args} = $config;
-    unless ($config->{recipient} || $config->{per_recipient}) {
-        $self->register_hook('mail', 'mail_handler');
-    }
-    else {
-        $self->register_hook('rcpt', 'rcpt_handler');
-    }
-    $self->init_db();
+    $self->init_db() or return;
+    $self->register_hooks();
     $self->prune_db();
     if ($self->{_args}{upgrade}) {
         $self->convert_db();
@@ -218,23 +220,64 @@ sub register {
     $self->load_exclude_files();
 }
 
+sub register_hooks {
+    my ($self) = @_;
+    $self->register_hook('data', 'data_handler');
+    if ($self->{_args}{recipient} || $self->{_args}{per_recipient}) {
+        $self->register_hook('rcpt', 'rcpt_handler');
+    }
+    else {
+        $self->register_hook('mail', 'mail_handler');
+    }
+}
+
 sub init_db {
     my ($self) = @_;
-    $self->db( name => 'greylist' );
-    return if ! $self->db->can('path');
+    return $self->init_redis if $self->{_args}{redis_server};
+    return $self->init_dbm;
+}
+
+sub init_redis {
+    my ($self) = @_;
+    eval {
+        $self->db(
+            name   => 'greylist',
+            class  => 'Qpsmtpd::DB::Redis',
+            server => $self->parse_redis_server,
+        ) or die 'Unknown error';
+    };
+    return 1 if ! $@;
+    $self->log(LOGERROR, "Unable to connect to redis store: $@");
+    return 0;
+}
+
+sub parse_redis_server {
+    my ($self) = @_;
+    my $server = $self->{_args}{redis_server};
+    return $server if $server =~ /:/;
+    return "$server:6379";
+}
+
+sub init_dbm {
+    my ($self) = @_;
+    $self->db(
+        name  => 'greylist',
+        class => 'Qpsmtpd::DB::File::DBM'
+    ) or return 0;
     my $cdir = $self->{_args}{db_dir};
     $cdir = $1 if $cdir and $cdir =~ m{^([-a-zA-Z0-9./_]+)$};
     # greylisting-specific hints for where to store the greylist DB
     my $db_dir = $self->db->dir( $cdir, '/var/lib/qpsmtpd/greylisting' );
 
-    return if $self->db->file_extension ne '.dbm';
+    return 1 if $self->db->file_extension ne '.dbm';
     $self->db->nfs_locking( $self->{_args}{nfslock} );
     
     # Work around old DBM filename
-    return if -f "$db_dir/greylist.dbm";
+    return 1 if -f "$db_dir/greylist.dbm";
     my $oldname = 'denysoft_greylist';
-    return if ! -f "$db_dir/$oldname.dbm";
+    return 1 if ! -f "$db_dir/$oldname.dbm";
     $self->db->name($oldname);
+    return 1;
 }
 
 sub load_exclude_files {
@@ -319,7 +362,7 @@ sub rcpt_handler {
     return DECLINED;
 }
 
-sub hook_data {
+sub data_handler {
     my ($self, $transaction) = @_;
     return DECLINED unless $transaction->notes('greylist');
 

t/plugin_tests/greylisting

@@ -12,7 +12,7 @@ sub register_tests {
     my $self = shift;
 
     $self->register_test("test_load_exclude_files");
-    $self->register_test('test_hook_data');
+    $self->register_test('test_data_handler');
     $self->register_test('test_get_greylist_key');
     $self->register_test('test_exclude');
     $self->register_test("test_greylist_geoip");
@@ -22,6 +22,9 @@ sub register_tests {
     $self->register_test("test_greylist_p0f_uptime");
     $self->register_test('test_exclude_file_match');
     $self->register_test('test_greylist');
+    $self->register_test('test_init_redis');
+    $self->register_test('test_init_dbm');
+    $self->register_test('test_parse_redis_server');
 }
 
 sub test_load_exclude_files {
@@ -71,27 +74,27 @@ sub test_exclude_file_match {
     }
 }
 
-sub test_hook_data {
+sub test_data_handler {
     my $self = shift;
     my $transaction = $self->qp->transaction;
 
-    my ($code, $mess) = $self->hook_data( $transaction );
+    my ($code, $mess) = $self->data_handler( $transaction );
     cmp_ok( $code, '==', DECLINED, "no note" );
 
     $transaction->notes('greylist', 1);
 
-    ($code, $mess) = $self->hook_data( $transaction );
+    ($code, $mess) = $self->data_handler( $transaction );
     cmp_ok( $code, '==', DECLINED, "no recipients");
 
     my $address = Qpsmtpd::Address->new( "<$test_email>" );
     $transaction->recipients( $address );
 
     $transaction->notes('whitelistrcpt', 2);
-    ($code, $mess) = $self->hook_data( $transaction );
+    ($code, $mess) = $self->data_handler( $transaction );
     cmp_ok( $code, '==', DENYSOFT, "missing recipients");
 
     $transaction->notes('whitelistrcpt', 1);
-    ($code, $mess) = $self->hook_data( $transaction );
+    ($code, $mess) = $self->data_handler( $transaction );
     cmp_ok( $code, '==', DECLINED, "missing recipients");
 }
 
@@ -279,3 +282,31 @@ sub rc {
     return return_code($r) . ": $msg";
 }
 
+sub test_init_redis {
+    my ($self) = @_;
+    delete $self->{db};
+    $self->{_args}{redis_server} = 'bogusserverasdfqwerty.:6379';
+    ok( ! $self->init_redis, 'init_redis() fails on bogus server' );
+    eval { Qpsmtpd::DB::Redis->new };
+    return if $@;
+    $self->{_args}{redis_server} = 'localhost';
+    ok( $self->init_redis, 'init_redis() succeeds when redis is up' );
+}
+
+sub test_init_dbm {
+    my ($self) = @_;
+    delete $self->{db};
+    delete $self->{_args}{redis_server};
+    ok( $self->init_db, 'init_db() works for DBM' );
+}
+
+sub test_parse_redis_server {
+    my ($self) = @_;
+    $self->{_args}{redis_server} = 'asdf:1234';
+    is( $self->parse_redis_server, 'asdf:1234',
+        'parse_redis_server(): leave provided port alone' );
+    $self->{_args}{redis_server} = 'qwerty';
+    is( $self->parse_redis_server, 'qwerty:6379',
+        'parse_redis_server(): add default port' );
+}
+