Merge pull request #728 from rhansen/dyndns2

dyndns2: Fix handling of multi-host response, add tests

ChangeLog.md

@@ -148,6 +148,8 @@ repository history](https://github.com/ddclient/ddclient/commits/master).
     [#719](https://github.com/ddclient/ddclient/pull/719)
   * `gandi`: Fixed handling of error responses.
     [#721](https://github.com/ddclient/ddclient/pull/721)
+  * `dyndns2`: Fixed handling of responses for multi-host updates.
+    [#728](https://github.com/ddclient/ddclient/pull/728)
 
 ## 2023-11-23 v3.11.2
 

Makefile.am

@@ -63,7 +63,6 @@ AM_PL_LOG_FLAGS = -Mstrict -w \
 	-MDevel::Autoflush
 handwritten_tests = \
 	t/builtinfw_query.pl \
-	t/dnsexit2.pl \
 	t/get_ip_from_if.pl \
 	t/geturl_connectivity.pl \
 	t/geturl_response.pl \
@@ -75,6 +74,8 @@ handwritten_tests = \
 	t/is-and-extract-ipv6-global.pl \
 	t/logmsg.pl \
 	t/parse_assignments.pl \
+	t/protocol_dnsexit2.pl \
+	t/protocol_dyndns2.pl \
 	t/skip.pl \
 	t/ssl-validate.pl \
 	t/use_web.pl \

ddclient.in

@@ -131,6 +131,7 @@ my $daemon_default = ($programd =~ /d$/) ? interval('5m') : undef;
 # Current Logger instance.  To push a context prefix onto the context stack:
 #    local _l = pushlogctx('additional context goes here');
 our $_l = ddclient::Logger->new();
+our @_test_headers;
 
 $ENV{'PATH'} = (exists($ENV{PATH}) ? "$ENV{PATH}:" : "") . "/sbin:/usr/sbin:/bin:/usr/bin:/etc:/usr/lib:";
 
@@ -159,6 +160,14 @@ my $daemon;
 # Control how many times warning message logged for invalid IP addresses
 my (%warned_ip, %warned_ipv4, %warned_ipv6);
 
+sub repr {
+    my $vals = @_ % 2 ? [shift] : [];
+    my %opts = @_;
+    my $d = Data::Dumper->new($vals)->Sortkeys(1)->Terse(!exists($opts{Names}))->Useqq(1);
+    $d->$_($opts{$_}) for keys(%opts);
+    return $d->Dump();
+}
+
 sub T_ANY    { 'any' }
 sub T_STRING { 'string' }
 sub T_EMAIL  { 'e-mail address' }
@@ -2674,7 +2683,6 @@ sub geturl {
     my $timeout  = opt('timeout');
     my $redirect  = opt('redirect');
     my @curlopt = ();
-    my @header_lines = ();
 
     ## canonify use_ssl, proxy and url
     if ($url =~ /^https:/) {
@@ -2721,11 +2729,8 @@ sub geturl {
         push(@curlopt, "user=\"".escape_curl_param("${login}:${password}").'"') if (defined($login) && defined($password));
         push(@curlopt, "proxy=\"".escape_curl_param("${protocol}://${proxy}").'"') if defined($proxy);
         push(@curlopt, "url=\"".escape_curl_param("${protocol}://${server}/${url}").'"');
-
+        push(@curlopt, map('header="' . escape_curl_param($_) . '"', @_test_headers,
-        # Each header line is added individually
+                           ref($headers) eq 'ARRAY' ? @$headers : split('\n', $headers)));
-        @header_lines = ref($headers) eq 'ARRAY' ? @$headers : split('\n', $headers);
-        $_ = "header=\"".escape_curl_param($_).'"' for (@header_lines);
-        push(@curlopt, @header_lines);
 
         # Add in the data if any was provided (for POST/PATCH)
         push(@curlopt, "data=\"".escape_curl_param(${data}).'"') if ($data);
@@ -3363,10 +3368,9 @@ sub group_hosts_by {
     @attrs = sort(keys(%attrs));
     my %groups;
     my %cfgs;
-    my $d = Data::Dumper->new([])->Indent(0)->Sortkeys(1)->Terse(1)->Useqq(1);
     for my $h (@$hosts) {
         my %cfg = map({ ($_ => $config{$h}{$_}); } grep(exists($config{$h}{$_}), @attrs));
-        my $sig = $d->Reset()->Values([\%cfg])->Dump();
+        my $sig = repr(\%cfg, Indent => 0);
         push(@{$groups{$sig}}, $h);
         $cfgs{$sig} = \%cfg;
     }
@@ -3826,7 +3830,7 @@ sub nic_dyndns2_update {
         '!yours' => 'The hostname specified exists, but not under the username currently being used',
         '!donator' => 'The offline setting was set, when the user is not a donator',
         '!active' => 'The hostname specified is in a Custom DNS domain which has not yet been activated.',
-       'abuse', => 'The hostname specified is blocked for abuse; you should receive an email notification which provides an unblock request link.  More info can be found on https://www.dyndns.com/support/abuse.html',
+        'abuse' => 'The hostname specified is blocked for abuse; you should receive an email notification which provides an unblock request link.  More info can be found on https://www.dyndns.com/support/abuse.html',
         'numhost' => 'System error: Too many or too few hosts found. Contact support@dyndns.org',
         'dnserr' => 'System error: DNS error encountered. Contact support@dyndns.org',
         'nochg' => 'No update required; unnecessary attempts to change to the current address are considered abusive',
@@ -3876,56 +3880,69 @@ sub nic_dyndns2_update {
         # updates too frequent) so the body of the response must also be checked.
         (my $body = $reply) =~ s/^.*?\n\n//s;
         my @reply = split(qr/\n/, $body);
-        if (!@reply) {
-            failed("Could not connect to $groupcfg{'server'}");
-            next;
-        }
         # From <https://help.dyn.com/remote-access-api/return-codes/>:
         #
         #   If updating multiple hostnames, hostname-specific return codes are given one per line,
         #   in the same order as the hostnames were specified.  Return codes indicating a failure
         #   with the account or the system are given only once.
         #
-        # TODO: There is no mention of what happens if multiple IP addresses are supplied (e.g.,
+        # If there is only one result for multiple hosts, this function assumes the one result
-        # IPv4 and IPv6) for a host.  If one address fails to update and the other doesn't, is that
+        # applies to all hosts.  According to the documentation quoted above this should only
-        # one error status line?  An error status line and a success status line?  Or is an update
+        # happen if the result is a failure.  In case there is a single successful result, this
-        # considered to be all-or-nothing and the status applies to the operation as a whole?  If
+        # code applies the success to all hosts (with a warning) to maximize potential
-        # the IPv4 address changes but not the IPv6 address does that result in a status of "good"
+        # compatibility with all DynDNS-like services.  If there are zero results, or two or more
-        # because the set of addresses for a host changed even if a subset did not?
+        # results, any host without a corresponding result line is treated as a failure.
         #
-        # TODO: The logic below applies the last line's status to all hosts.  Change it to apply
+        # TODO: The DynDNS documentation does not mention what happens if multiple IP addresses are
-        # each status to its corresponding host.
+        # supplied (e.g., IPv4 and IPv6) for a host.  If one address fails to update and the other
-        for my $line (@reply) {
+        # doesn't, is that one error status line?  An error status line and a success status line?
+        # Or is an update considered to be all-or-nothing and the status applies to the collection
+        # of addresses as a whole?  If the IPv4 address changes but not the IPv6 address does that
+        # result in a status of "good" because the set of addresses for a host changed even if a
+        # subset did not?
+        my @statuses = map({ (my $l = $_) =~ s/ .*$//; $l; } @reply);
+        if (@statuses < @hosts && @statuses == 1) {
+            warning("service returned one successful result for multiple hosts; " .
+                    "assuming the one success is intended to apply to all hosts")
+                if $statuses[0] =~ qr/^(?:good|nochg)$/;
+            @statuses = ($statuses[0]) x @hosts;
+        }
+        for (my $i = 0; $i < @hosts; ++$i) {
+            my $h = $hosts[$i];
+            local $_l = $_l->{parent}; $_l = pushlogctx($h);
+            my $status = $statuses[$i] // 'unknown';
+            if ($status eq 'nochg') {
+                warning("$status: $errors{$status}");
+                $status = 'good';
+            }
+            $config{$h}{'status-ipv4'} = $status if $ipv4;
+            $config{$h}{'status-ipv6'} = $status if $ipv6;
+            if ($status ne 'good') {
+                if (exists($errors{$status})) {
+                    failed("$status: $errors{$status}");
+                } elsif ($status eq 'unknown') {
+                    failed('server did not return a success/fail result; assuming failure');
+                } else {
+                    # This case can only happen if there is a corresponding status line for this
+                    # host or there was only one status line for all hosts.
+                    failed("unexpected status: " . ($reply[$i] // $reply[0]));
+                }
+                next;
+            }
             # The IP address normally comes after the status, but we ignore it.  We could compare
             # it with the expected address and mark the update as failed if it differs, but (1)
             # some services do not return the IP; and (2) comparison is brittle (e.g.,
             # 192.000.002.001 vs. 192.0.2.1) and false errors could cause high load on the service
             # (an update attempt every min-error-interval instead of every max-interval).
-            (my $status = $line) =~ s/ .*$//;
-            if ($status eq 'nochg') {
-                warning("$status: $errors{$status}");
-                $status = 'good';
-            }
-            for my $h (@hosts) {
-                $config{$h}{'status-ipv4'} = $status if $ipv4;
-                $config{$h}{'status-ipv6'} = $status if $ipv6;
-            }
-            if ($status ne 'good') {
-                if (exists($errors{$status})) {
-                    failed("$status: $errors{$status}");
-                } else {
-                    failed("unexpected status: $line");
-                }
-                next;
-            }
-            for my $h (@hosts) {
             $config{$h}{'ipv4'} = $ipv4 if $ipv4;
             $config{$h}{'ipv6'} = $ipv6 if $ipv6;
             $config{$h}{'mtime'} = $now;
-            }
             success("IPv4 address set to $ipv4") if $ipv4;
             success("IPv6 address set to $ipv6") if $ipv6;
         }
+        warning("unexpected extra lines after per-host update status lines:\n" .
+                join("\n", @reply[@hosts..$#reply]))
+            if (@reply > @hosts);
     }
 }
 

t/protocol_dyndns2.pl

@@ -0,0 +1,303 @@
+use Test::More;
+use Scalar::Util qw(blessed);
+use MIME::Base64;
+eval { require ddclient::Test::Fake::HTTPD; } or plan(skip_all => $@);
+SKIP: { eval { require Test::Warnings; } or skip($@, 1); }
+eval { require 'ddclient'; } or BAIL_OUT($@);
+
+my $httpd = ddclient::Test::Fake::HTTPD->new();
+$httpd->run(sub {
+    my ($req) = @_;
+    diag('==============================================================================');
+    diag("Test server received request:\n" . $req->as_string());
+    my $headers = ['content-type' => 'text/plain; charset=utf-8'];
+    my $wantauthn = 'Basic ' . encode_base64('username:password', '');
+    return [401, [@$headers, 'www-authenticate' => 'Basic realm="realm", charset="UTF-8"'],
+            ['authentication required']] if ($req->header('authorization') // '') ne $wantauthn;
+    return [400, $headers, ['invalid method: ' . $req->method()]] if $req->method() ne 'GET';
+    return [400, $headers, ['unexpected request: ' . $req->uri() . "\n",
+                            'want: ' . $req->header('want-req')]]
+        if $req->uri() ne $req->header('want-req');
+    return [200, $headers, [map("$_\n", $req->header('line'))]];
+});
+diag("started IPv4 HTTP server running at " . $httpd->endpoint());
+
+{
+    package Logger;
+    BEGIN { push(our @ISA, qw(ddclient::Logger)); }
+    sub new {
+        my ($class, $parent) = @_;
+        my $self = $class->SUPER::new(undef, $parent);
+        $self->{logs} = [];
+        return $self;
+    }
+    sub _log {
+        my ($self, $args) = @_;
+        push(@{$self->{logs}}, $args)
+            if ($args->{label} // '') =~ qr/^(?:WARNING|FATAL|SUCCESS|FAILED)$/;
+        return $self->SUPER::_log($args);
+    }
+}
+
+my @test_cases = (
+    {
+        desc => 'IPv4, single host, good',
+        cfg => {h1 => {wantipv4 => '192.0.2.1'}},
+        resp => ['good'],
+        wantquery => 'hostname=h1&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+        },
+        wantlogs => [
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv4/},
+        ],
+    },
+    {
+        desc => 'IPv4, single host, nochg',
+        cfg => {h1 => {wantipv4 => '192.0.2.1'}},
+        resp => ['nochg'],
+        wantquery => 'hostname=h1&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+        },
+        wantlogs => [
+            {label => 'WARNING', ctx => ['h1'], msg => qr/nochg/},
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv4/},
+        ],
+    },
+    {
+        desc => 'IPv4, single host, bad',
+        cfg => {h1 => {wantipv4 => '192.0.2.1'}},
+        resp => ['nohost'],
+        wantquery => 'hostname=h1&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'nohost'},
+        },
+        wantlogs => [
+            {label => 'FAILED', ctx => ['h1'], msg => qr/nohost/},
+        ],
+    },
+    {
+        desc => 'IPv4, single host, unexpected',
+        cfg => {h1 => {wantipv4 => '192.0.2.1'}},
+        resp => ['WAT'],
+        wantquery => 'hostname=h1&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'WAT'},
+        },
+        wantlogs => [
+            {label => 'FAILED', ctx => ['h1'], msg => qr/unexpected.*WAT/},
+        ],
+    },
+    {
+        desc => 'IPv4, multiple hosts, multiple good',
+        cfg => {
+            h1 => {wantipv4 => '192.0.2.1'},
+            h2 => {wantipv4 => '192.0.2.1'},
+        },
+        resp => [
+            'good 192.0.2.1',
+            'good',
+        ],
+        wantquery => 'hostname=h1,h2&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+            h2 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+        },
+        wantlogs => [
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv4/},
+            {label => 'SUCCESS', ctx => ['h2'], msg => qr/IPv4/},
+        ],
+    },
+    {
+        desc => 'IPv4, multiple hosts, mixed success',
+        cfg => {
+            h1 => {wantipv4 => '192.0.2.1'},
+            h2 => {wantipv4 => '192.0.2.1'},
+            h3 => {wantipv4 => '192.0.2.1'},
+        },
+        resp => [
+            'good',
+            'nochg',
+            'dnserr',
+        ],
+        wantquery => 'hostname=h1,h2,h3&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+            h2 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+            h3 => {'status-ipv4' => 'dnserr'},
+        },
+        wantlogs => [
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv4/},
+            {label => 'WARNING', ctx => ['h2'], msg => qr/nochg/},
+            {label => 'SUCCESS', ctx => ['h2'], msg => qr/IPv4/},
+            {label => 'FAILED', ctx => ['h3'], msg => qr/dnserr/},
+        ],
+    },
+    {
+        desc => 'IPv6, single host, good',
+        cfg => {h1 => {wantipv6 => '2001:db8::1'}},
+        resp => ['good'],
+        wantquery => 'hostname=h1&myip=2001:db8::1',
+        wantstatus => {
+            h1 => {'status-ipv6' => 'good', 'ipv6' => '2001:db8::1', 'mtime' => $ddclient::now},
+        },
+        wantlogs => [
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv6/},
+        ],
+    },
+    {
+        desc => 'IPv4 and IPv6, single host, good',
+        cfg => {h1 => {wantipv4 => '192.0.2.1', wantipv6 => '2001:db8::1'}},
+        resp => ['good'],
+        wantquery => 'hostname=h1&myip=192.0.2.1,2001:db8::1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1',
+                   'status-ipv6' => 'good', 'ipv6' => '2001:db8::1',
+                   'mtime' => $ddclient::now},
+        },
+        wantlogs => [
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv4/},
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv6/},
+        ],
+    },
+    {
+        desc => 'excess status line',
+        cfg => {
+            h1 => {wantipv4 => '192.0.2.1'},
+            h2 => {wantipv4 => '192.0.2.1'},
+        },
+        resp => [
+            'good',
+            'good',
+            'WAT',
+        ],
+        wantquery => 'hostname=h1,h2&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+            h2 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+        },
+        wantlogs => [
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv4/},
+            {label => 'SUCCESS', ctx => ['h2'], msg => qr/IPv4/},
+            {label => 'WARNING', ctx => ['h1,h2'], msg => qr/unexpected.*\nWAT$/},
+        ],
+    },
+    {
+        desc => 'multiple hosts, single failure',
+        cfg => {
+            h1 => {wantipv4 => '192.0.2.1'},
+            h2 => {wantipv4 => '192.0.2.1'},
+        },
+        resp => ['abuse'],
+        wantquery => 'hostname=h1,h2&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'abuse'},
+            h2 => {'status-ipv4' => 'abuse'},
+        },
+        wantlogs => [
+            {label => 'FAILED', ctx => ['h1'], msg => qr/abuse/},
+            {label => 'FAILED', ctx => ['h2'], msg => qr/abuse/},
+        ],
+    },
+    {
+        desc => 'multiple hosts, single success',
+        cfg => {
+            h1 => {wantipv4 => '192.0.2.1'},
+            h2 => {wantipv4 => '192.0.2.1'},
+        },
+        resp => ['good'],
+        wantquery => 'hostname=h1,h2&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+            h2 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+        },
+        wantlogs => [
+            {label => 'WARNING', ctx => ['h1,h2'], msg => qr//},
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv4/},
+            {label => 'SUCCESS', ctx => ['h2'], msg => qr/IPv4/},
+        ],
+    },
+    {
+        desc => 'multiple hosts, fewer results',
+        cfg => {
+            h1 => {wantipv4 => '192.0.2.1'},
+            h2 => {wantipv4 => '192.0.2.1'},
+            h3 => {wantipv4 => '192.0.2.1'},
+        },
+        resp => [
+            'good',
+            'nochg',
+        ],
+        wantquery => 'hostname=h1,h2,h3&myip=192.0.2.1',
+        wantstatus => {
+            h1 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+            h2 => {'status-ipv4' => 'good', 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now},
+            h3 => {'status-ipv4' => 'unknown'},
+        },
+        wantlogs => [
+            {label => 'SUCCESS', ctx => ['h1'], msg => qr/IPv4/},
+            {label => 'WARNING', ctx => ['h2'], msg => qr/nochg/},
+            {label => 'SUCCESS', ctx => ['h2'], msg => qr/IPv4/},
+            {label => 'FAILED', ctx => ['h3'], msg => qr/assuming failure/},
+        ],
+    },
+);
+
+for my $tc (@test_cases) {
+    diag('==============================================================================');
+    diag("Starting test: $tc->{desc}");
+    diag('==============================================================================');
+    local $ddclient::globals{debug} = 1;
+    local $ddclient::globals{verbose} = 1;
+    my $l = Logger->new($ddclient::_l);
+    local %ddclient::config;
+    $ddclient::config{$_} = {
+        login => 'username',
+        password => 'password',
+        server => $httpd->endpoint(),
+        script => '/nic/update',
+        %{$tc->{cfg}{$_}},
+    } for keys(%{$tc->{cfg}});
+    {
+        local @ddclient::_test_headers = (
+            "want-req: /nic/update?$tc->{wantquery}",
+            map("line: $_", @{$tc->{resp}}),
+        );
+        local $ddclient::_l = $l;
+        ddclient::nic_dyndns2_update(sort(keys(%{$tc->{cfg}})));
+    }
+    # These are the properties in %ddclient::config to check against $tc->{wantstatus}.
+    my %statuskeys = map(($_ => undef), qw(atime ip ipv4 ipv6 mtime status status-ipv4 status-ipv6
+                                           wantip wantipv4 wantipv6 wtime));
+    my %gotstatus;
+    for my $h (keys(%ddclient::config)) {
+        $gotstatus{$h} = {map(($_ => $ddclient::config{$h}{$_}),
+                              grep(exists($statuskeys{$_}), keys(%{$ddclient::config{$h}})))};
+    }
+    is_deeply(\%gotstatus, $tc->{wantstatus}, "$tc->{desc}: status")
+        or diag(ddclient::repr(\%ddclient::config, Names => ['*ddclient::config']));
+    $tc->{wantlogs} //= [];
+    subtest("$tc->{desc}: logs" => sub {
+        my @got = @{$l->{logs}};
+        my @want = @{$tc->{wantlogs}};
+        for my $i (0..$#want) {
+            last if $i >= @got;
+            my $got = $got[$i];
+            my $want = $want[$i];
+            subtest("log $i" => sub {
+                is($got->{label}, $want->{label}, "label matches");
+                is_deeply($got->{ctx}, $want->{ctx}, "context matches");
+                like($got->{msg}, $want->{msg}, "message matches");
+            }) or diag(ddclient::repr(Values => [$got, $want], Names => ['*got', '*want']));
+        }
+        my @unexpected = @got[@want..$#got];
+        ok(@unexpected == 0, "no unexpected logs")
+            or diag(ddclient::repr(\@unexpected, Names => ['*unexpected']));
+        my @missing = @want[@got..$#want];
+        ok(@missing == 0, "no missing logs")
+            or diag(ddclient::repr(\@missing, Names => ['*missing']));
+    });
+}
+
+done_testing();