Skip to content

Commit f5b13df

Browse files
iliarossiliaross
committed
Fix to better handle PowerDNS database connection failures
ⓘ Preserve DBI errors, fail backup and restore cleanly when the database is unavailable, close database handles, add regression coverage. https://forum.virtualmin.com/t/partially-completed-backup/137626/12?u=ilia
1 parent 32047dd commit f5b13df

4 files changed

Lines changed: 174 additions & 12 deletions

File tree

‎module.info‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,5 @@ desc=Virtualmin PowerDNS
22
category=servers
33
perldepends=DBI DBD::mysql
44
depends=virtual-server/2.40 1.420
5-
version=1.13.1
5+
version=1.13.2
66
feedback=jcameron@webmin.com

‎t/database-errors.t‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
use strict;
2+
use warnings;
3+
use FindBin;
4+
use Test::More;
5+
use lib "$FindBin::Bin/..";
6+
7+
BEGIN {
8+
$INC{'DBI.pm'} = __FILE__;
9+
}
10+
11+
package DBI;
12+
13+
sub import
14+
{
15+
}
16+
17+
sub install_driver
18+
{
19+
die $DBI::install_error if ($DBI::install_error);
20+
return bless({ }, 'MockDriver');
21+
}
22+
23+
package MockDriver;
24+
25+
sub connect
26+
{
27+
my ($self, $dsn, $user, $pass, $attrs) = @_;
28+
$DBI::connect_attributes = { %$attrs };
29+
$_[0]->{'errstr'} = 'mock database connection failed';
30+
return undef;
31+
}
32+
33+
sub errstr
34+
{
35+
return $_[0]->{'errstr'};
36+
}
37+
38+
package main;
39+
40+
our (%config, %text, @first_messages, @second_messages,
41+
$module_config_directory, $module_root_directory);
42+
$module_config_directory = '/unused/config';
43+
$module_root_directory = '/unused/module';
44+
%config = (
45+
'db' => 'powerdns',
46+
'host' => 'localhost',
47+
'user' => 'powerdns',
48+
'pass' => 'secret',
49+
);
50+
%text = (
51+
'backup_dom' => 'Backing up PowerDNS database entries for domain ..',
52+
'feat_edb' => 'An error occurred connecting to the MySQL database : $1',
53+
'restore_dom' => 'Restoring PowerDNS database entries for domain ..',
54+
);
55+
56+
sub init_config
57+
{
58+
}
59+
60+
sub text
61+
{
62+
my ($key, @args) = @_;
63+
my $msg = $text{$key};
64+
for(my $i = 0; $i < @args; $i++) {
65+
my $n = $i + 1;
66+
$msg =~ s/\$$n/$args[$i]/g;
67+
}
68+
return $msg;
69+
}
70+
71+
package virtual_server;
72+
73+
our $first_print = sub { push(@main::first_messages, @_); };
74+
our $second_print = sub { push(@main::second_messages, @_); };
75+
our %text = ( 'setup_done' => '.. done' );
76+
77+
package main;
78+
79+
require "$FindBin::Bin/../virtual_feature.pl";
80+
81+
{
82+
local $DBI::install_error =
83+
"mock driver load failed at (eval 42) line 3.\ninternal detail\n";
84+
my ($missing_dbh, $missing_err) = connect_to_database();
85+
ok(!defined($missing_dbh), 'database driver failure returns no handle');
86+
is($missing_err, 'mock driver load failed',
87+
'database driver failure omits Perl internals');
88+
}
89+
90+
my ($dbh, $err) = connect_to_database();
91+
ok(!defined($dbh), 'database connection failure returns no handle');
92+
is($err, 'mock database connection failed',
93+
'database connection failure preserves the DBI error');
94+
is($DBI::connect_attributes->{'PrintError'}, 1,
95+
'connected handles keep DBI statement errors visible');
96+
97+
my $scalar_dbh = connect_to_database();
98+
ok(!defined($scalar_dbh),
99+
'scalar database connection failure returns no handle');
100+
101+
my $domain = { 'dom' => 'example.test' };
102+
my $backup_ok;
103+
my $backup_eval = eval {
104+
$backup_ok = feature_backup($domain, '/unused', { }, { });
105+
1;
106+
};
107+
ok($backup_eval, 'backup does not die when the database is unavailable');
108+
is($backup_ok, 0, 'backup reports failure when the database is unavailable');
109+
is_deeply(\@first_messages,
110+
[ 'Backing up PowerDNS database entries for domain ..' ],
111+
'backup prints its operation before the connection error');
112+
is_deeply(\@second_messages,
113+
[ 'An error occurred connecting to the MySQL database : '.
114+
'mock database connection failed' ],
115+
'backup prints the underlying database connection error');
116+
117+
@first_messages = ( );
118+
@second_messages = ( );
119+
my $restore_ok;
120+
my $restore_eval = eval {
121+
$restore_ok = feature_restore($domain, '/unused', { }, { });
122+
1;
123+
};
124+
ok($restore_eval, 'restore does not die when the database is unavailable');
125+
is($restore_ok, 0, 'restore reports failure when the database is unavailable');
126+
is_deeply(\@first_messages,
127+
[ 'Restoring PowerDNS database entries for domain ..' ],
128+
'restore prints its operation before the connection error');
129+
is_deeply(\@second_messages,
130+
[ 'An error occurred connecting to the MySQL database : '.
131+
'mock database connection failed' ],
132+
'restore prints the underlying database connection error');
133+
134+
done_testing();

‎virtual_feature.pl‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -273,9 +273,13 @@ sub feature_webmin
273273
sub feature_backup
274274
{
275275
my ($d, $file, $opts, $allopts) = @_;
276-
my $dbh = &connect_to_database();
277-
my $id = &domain_id($dbh, $_[0]->{'dom'});
278276
&$virtual_server::first_print($text{'backup_dom'});
277+
my ($dbh, $err) = &connect_to_database();
278+
if (!$dbh) {
279+
&$virtual_server::second_print(&text('feat_edb', $err));
280+
return 0;
281+
}
282+
my $id = &domain_id($dbh, $d->{'dom'});
279283
if ($id) {
280284
no strict "subs";
281285
&virtual_server::open_tempfile_as_domain_user($d, BFILE, ">$file");
@@ -287,10 +291,12 @@ sub feature_backup
287291
$cmd->finish();
288292
&virtual_server::close_tempfile_as_domain_user($d, BFILE);
289293
use strict "subs";
294+
$dbh->disconnect();
290295
&$virtual_server::second_print($virtual_server::text{'setup_done'});
291296
return 1;
292297
}
293298
else {
299+
$dbh->disconnect();
294300
&$virtual_server::second_print($text{'delete_missing'});
295301
return 0;
296302
}
@@ -302,9 +308,13 @@ sub feature_backup
302308
sub feature_restore
303309
{
304310
my ($d, $file, $opts, $allopts) = @_;
305-
my $dbh = &connect_to_database();
306-
my $id = &domain_id($dbh, $_[0]->{'dom'});
307311
&$virtual_server::first_print($text{'restore_dom'});
312+
my ($dbh, $err) = &connect_to_database();
313+
if (!$dbh) {
314+
&$virtual_server::second_print(&text('feat_edb', $err));
315+
return 0;
316+
}
317+
my $id = &domain_id($dbh, $d->{'dom'});
308318
if ($id) {
309319
# Remove all old records
310320
my $delreccmd = $dbh->prepare("delete from records where domain_id = ?");
@@ -322,10 +332,12 @@ sub feature_restore
322332
}
323333
close($BFILE);
324334
&increment_record_seq($dbh);
335+
$dbh->disconnect();
325336
&$virtual_server::second_print($virtual_server::text{'setup_done'});
326337
return 1;
327338
}
328339
else {
340+
$dbh->disconnect();
329341
&$virtual_server::second_print($text{'delete_missing'});
330342
return 0;
331343
}

‎virtualmin-powerdns-lib.pl‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,22 +8,38 @@
88
eval "use WebminCore;";
99
&init_config();
1010

11+
# clean_powerdns_database_error(error)
12+
# Remove Perl source locations from errors before displaying them to users.
13+
sub clean_powerdns_database_error
14+
{
15+
my ($err) = @_;
16+
$err ||= "Unknown error";
17+
$err =~ s/\s+at\s+(?:\(eval\s+\d+\)|\S+)\s+line\s+\d+.*$//s;
18+
return $err;
19+
}
20+
1121
# connect_to_database()
1222
sub connect_to_database
1323
{
1424
eval "use DBI;";
15-
return $@ if ($@);
16-
my ($dbh, $err);
25+
if ($@) {
26+
my $err = &clean_powerdns_database_error($@);
27+
return wantarray ? (undef, $err) : undef;
28+
}
29+
my ($dbh, $err, $drh);
1730
eval {
18-
my $drh = DBI->install_driver("mysql");
31+
$drh = DBI->install_driver("mysql");
1932
$dbh = $drh->connect("database=$config{'db'}".
2033
($config{'host'} ? ";host=$config{'host'}" : ""),
21-
$config{'user'}, $config{'pass'}, { });
34+
$config{'user'}, $config{'pass'},
35+
{ 'PrintError' => 1 });
2236
};
23-
if ($@ || !$dbh) {
24-
$err = $@ || "Unknown error";
37+
my $eval_err = $@;
38+
if ($eval_err || !$dbh) {
39+
my $driver_err = $drh ? $drh->errstr : undef;
40+
$err = $eval_err || $driver_err || "Unknown error";
2541
}
26-
$err =~ s/\s+at\s+.*//;
42+
$err = &clean_powerdns_database_error($err) if ($err);
2743
return wantarray ? ($dbh, $err) : $dbh;
2844
}
2945

0 commit comments

Comments
 (0)