From de5d894c914d6ff707e30db37d999a2d00496573 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 17 Aug 2024 01:05:53 -0400 Subject: [PATCH] Fix handling of legacy `status` value When a legacy protocol implementation returns, move its `status` and `ip` results to the new `status-ipv4` and `ipv4` (or `status-ipv6` and `ipv6`) properties. Also remove the now-unused `status` variable definition, and remove `ip` from the recap. --- ChangeLog.md | 1 + ddclient.in | 128 +++++++++++++++++++++++++---------------------- t/update_nics.pl | 90 ++++++++++++++------------------- 3 files changed, 106 insertions(+), 113 deletions(-) diff --git a/ChangeLog.md b/ChangeLog.md index 68db4e9..69c2f76 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -133,6 +133,7 @@ repository history](https://github.com/ddclient/ddclient/commits/master). [#734](https://github.com/ddclient/ddclient/pull/734) * Fixed unnecessary repeated updates for some services. [#670](https://github.com/ddclient/ddclient/pull/670) + [#732](https://github.com/ddclient/ddclient/pull/732) * Fixed DNSExit provider when configured with a zone and non-identical hostname. [#674](https://github.com/ddclient/ddclient/pull/674) * `infomaniak`: Fixed frequent forced updates after 25 days (`max-interval`). diff --git a/ddclient.in b/ddclient.in index 280acce..c586a4a 100755 --- a/ddclient.in +++ b/ddclient.in @@ -158,7 +158,7 @@ my $saved_recap; my %saved_opt; my $daemon; # Control how many times warning message logged for invalid IP addresses -my (%warned_ip, %warned_ipv4, %warned_ipv6); +my (%warned_ipv4, %warned_ipv6); sub repr { my $vals = @_ % 2 ? [shift] : []; @@ -633,16 +633,20 @@ our %variables = ( 'max-interval' => setv(T_DELAY, 0, 0, interval('25d'), 0), 'min-error-interval' => setv(T_DELAY, 0, 0, interval('5m'), 0), - # As a recap value, this is the IP address (IPv4 or IPv6, but almost always IPv4) most - # recently saved at the DDNS service. As a setting, this is the desired IP address that - # should be saved at the DDNS service. Unfortunately, these two meanings are conflated, - # causing the bug "skipped: IP address was already set to a.b.c.d" when the IP was never - # set to a.b.c.d. - # TODO: Move the recap value elsewhere to fix the bug. - 'ip' => setv(T_IP, 0, 1, undef, undef), - # As `ip`, but only IPv4 addresses. + # The desired IP address (IPv4 or IPv6, but almost always IPv4) that should be saved at the + # DDNS service. + # TODO: Legacy protocol implementations write the IP address most recently saved at the + # DDNS service to this variable so that it can be saved in the recap (as `ipv4` or `ipv6`). + # Update the legacy implementations to use `ipv4` or `ipv6` instead, though see the TODO + # for those variables. + 'ip' => setv(T_IP, 0, 0, undef, undef), + # As a recap value, this is the IPv4 address most recently saved at the DDNS service. As a + # setting, this is the desired IPv4 address that should be saved at the DDNS service. + # Unfortunately, these two meanings are conflated, causing the bug "skipped: IP address was + # already set to a.b.c.d" when the IP was never set to a.b.c.d. + # TODO: Move the recap value elsewhere to fix the bug. 'ipv4' => setv(T_IPV4, 0, 1, undef, undef), - # As `ip`, but only IPv6 addresses. + # As `ipv4`, but for an IPv6 address. 'ipv6' => setv(T_IPV6, 0, 1, undef, undef), # Timestamp (seconds since epoch) indicating the earliest time the next update is # permitted. @@ -658,12 +662,10 @@ our %variables = ( # TODO: Create a timestamp type and change this to that type. 'atime' => setv(T_NUMBER,0, 1, 0, undef), # Disposition of the most recent (or currently in progress) attempt to update the DDNS - # service with the IP address in `wantip`. Anything other than `good`, including undef, is - # treated as a failure. - 'status' => setv(T_ANY, 0, 1, undef, undef), - # As `status`, but with `wantipv4`. + # service with the IP address in `wantipv4` (or `wantip`, if an IPv4 address). Anything + # other than `good`, including undef, is treated as a failure. 'status-ipv4' => setv(T_ANY, 0, 1, undef, undef), - # As `status`, but with `wantipv6`. + # As `status-ipv4`, but with `wantipv6` (or `wantip`, if an IPv6 address). 'status-ipv6' => setv(T_ANY, 0, 1, undef, undef), # Timestamp (seconds since epoch) of the most recent attempt that would have been made had # `min-interval` not inhibited the attempt. This is reset to 0 once an attempt is actually @@ -1440,15 +1442,51 @@ sub update_nics { delete($config{$h}{$_}) for qw(wantip wantipv4 wantipv6); } - # Backwards compatibility: - # The legacy '--use' parameter sets 'wantip' and the legacy providers process this and - # set 'ip', 'status' accordingly. - # The new '--usev*' parameters set 'wantipv*' and the new providers set 'ipv*' and 'status-ipv*'. - # To allow gradual transition, we make sure both the old 'status' and 'ip' are being set - # accordingly to what new providers returned in the new 'status-ipv*' and 'ipv*' fields respectively. + # Backwards compatibility: Legacy protocol implementations read `wantip` and set `ip` + # and `status`. Modern protocol implementations read `wantipv4` and `wantipv6` and set + # `ipv4`, `ipv6`, `status-ipv4`, and `status-ipv6`. Make legacy implementations look + # like modern implementations by moving `ip` and `status` to the modern + # version-specific equivalents. for my $h (@hosts) { - $config{$h}{'status'} //= $config{$h}{'status-ipv4'} // $config{$h}{'status-ipv6'}; - $config{$h}{'ip'} //= $config{$h}{'ipv4'} // $config{$h}{'ipv6'}; + local $_l = pushlogctx($h); + my $status = delete($config{$h}{'status'}) || next; + my $ip = $config{$h}{'ip'}; + my $ipv = is_ipv4($ip) ? '4' : is_ipv6($ip) ? '6' : undef; + if (!defined($ipv)) { + warning("ddclient bug: legacy protocol set 'status' but did not set 'ip' " . + "to an IPv4 or IPv6 address: " . ($ip // '')); + next; + } + # TODO: Currently $config{$h}{'ip'} is used for two distinct purposes: it holds the + # value of the --ip option, and it is updated by legacy protocols to hold the new + # IP address after an update. Fortunately, the --ip option is not used very often, + # and if it is, the values for the two use cases are usually (but not always) the + # same. This boolean is an imperfect attempt to identify whether 'ip' is being + # used for the --ip option, to avoid breaking some user's configuration. Protocols + # should be updated to not set $config{$h}{'ip'} because %config is for + # configuration, not update status (same goes for 'status', 'mtime', etc.). + my $ip_option = opt('use', $h) eq 'ip' || opt('usev6', $h) eq 'ip'; + delete($config{$h}{'ip'}) if !$ip_option; + debug("legacy protocol; moving status to status-ipv$ipv and ip to ipv$ipv"); + if (defined(my $vstatus = $config{$h}{"status-ipv$ipv"})) { + warning("ddclient bug: legacy protocol set both 'status' (to '$status') " . + "and 'status-ipv$ipv' (to '$vstatus')"); + } else { + $config{$h}{"status-ipv$ipv"} = $status; + } + # TODO: See above comment for $ip_option. This is the same situation, but for + # 'ipv4' and 'ipv6'. + my $vip_option = opt("usev$ipv", $h) eq "ipv$ipv"; + if (defined(my $vip = $config{$h}{"ipv$ipv"}) && $vip_option) { + debug("unable to update 'ipv$ipv' to '$ip' " . + "because it is already set to '$vip'") if $vip_option; + } else { + # A previous update will have set "ipv$ipv" if !$vip_option, so it's safe to + # overwrite it here because $vip_option was checked above. + debug("updating 'ipv$ipv' from '$vip' to '$ip'") + if defined($vip) && $vip ne $ip; + $config{$h}{"ipv$ipv"} = $ip; + } } runpostscript(join ' ', keys %ipsv4, keys %ipsv6); @@ -1510,7 +1548,7 @@ sub write_recap { $recap{$h}{$v} = opt($v, $h); } } else { - for my $v (qw(atime wtime status status-ipv4 status-ipv6)) { + for my $v (qw(atime wtime status-ipv4 status-ipv6)) { $recap{$h}{$v} = opt($v, $h); } } @@ -1572,7 +1610,7 @@ sub read_recap { next if !exists($config->{$h}); # TODO: Why is this limited to this set of variables? Why not copy every recap var # defined for the host's protocol? - for (qw(atime mtime wtime ip ipv4 ipv6 status status-ipv4 status-ipv6)) { + for (qw(atime mtime wtime ip ipv4 ipv6 status-ipv4 status-ipv6)) { # TODO: Isn't $config equal to \%recap here? If so, this is a no-op. What was the # original intention behind this? To copy %recap values into %config? If so, is # it better to just delete this and live with the current behavior (which doesn't @@ -3407,7 +3445,6 @@ sub nic_updateable { my ($host) = @_; my $force_update = $protocols{opt('protocol', $host)}{force_update}; my $update = 0; - my $ip = $config{$host}{'wantip'}; my $ipv4 = $config{$host}{'wantipv4'}; my $ipv6 = $config{$host}{'wantipv6'}; my $inv_ip_warn_count = opt('max-warn'); @@ -3419,7 +3456,6 @@ sub nic_updateable { my %prettyi = map({ ($_ => prettyinterval(opt($_, $host))); } qw(max-interval min-error-interval min-interval)); - $warned_ip{$host} = 0 if defined($ip); $warned_ipv4{$host} = 0 if defined($ipv4); $warned_ipv6{$host} = 0 if defined($ipv6); @@ -3432,42 +3468,13 @@ sub nic_updateable { $update = 1; } elsif ($recap{$host}{'wtime'} && $recap{$host}{'wtime'} > $now) { - warning("cannot update IP from $previp to $ip until after $prettyt{'wtime'}"); + warning("cannot update IP until after $prettyt{'wtime'}"); } elsif ($recap{$host}{'mtime'} && interval_expired($host, 'mtime', 'max-interval')) { info("update forced because it has been $prettyi{'max-interval'} since the previous update (on $prettyt{'mtime'})"); $update = 1; - } elsif (defined($ip) && $previp ne $ip) { - ## Check whether to update IP address for the "--use" method" - if (($recap{$host}{'status'} // '') eq 'good' && - !interval_expired($host, 'mtime', 'min-interval')) { - warning("skipped update from $previp to $ip because it has been less than $prettyi{'min-interval'} since the previous update (on $prettyt{'mtime'})") - if opt('verbose') || !($recap{$host}{'warned-min-interval'} // 0); - - $recap{$host}{'warned-min-interval'} = $now; - - } elsif (($recap{$host}{'status'} // '') ne 'good' && - !interval_expired($host, 'atime', 'min-error-interval')) { - - if (opt('verbose') || (!$recap{$host}{'warned-min-error-interval'} && - ($warned_ip{$host} // 0) < $inv_ip_warn_count)) { - warning("skipped update from $previp to $ip because it has been less than $prettyi{'min-error-interval'} since the previous update attempt (on $prettyt{'atime'}), which failed"); - if (!$ip && !opt('verbose')) { - $warned_ip{$host} = ($warned_ip{$host} // 0) + 1; - warning("IP address undefined. Warned $inv_ip_warn_count times, suppressing further warnings") - if ($warned_ip{$host} >= $inv_ip_warn_count); - } - } - - $recap{$host}{'warned-min-error-interval'} = $now; - - } else { - $update = 1; - } - } elsif (defined($ipv4) && $previpv4 ne $ipv4) { - ## Check whether to update IPv4 address for the "--usev4" method" if (($recap{$host}{'status-ipv4'} // '') eq 'good' && !interval_expired($host, 'mtime', 'min-interval')) { warning("skipped update from $previpv4 to $ipv4 because it has been less than $prettyi{'min-interval'} since the previous update (on $prettyt{'mtime'})") @@ -3495,7 +3502,6 @@ sub nic_updateable { } } elsif (defined($ipv6) && $previpv6 ne $ipv6) { - ## Check whether to update IPv6 address for the "--usev6" method" if (($recap{$host}{'status-ipv6'} // '') eq 'good' && !interval_expired($host, 'mtime', 'min-interval')) { warning("skipped update from $previpv6 to $ipv6 because it has been less than $prettyi{'min-interval'} since the previous update (on $prettyt{'mtime'})") @@ -3532,8 +3538,6 @@ sub nic_updateable { } else { if (opt('verbose')) { - success("skipped update because IP address is already set to $ip") - if defined($ip); success("skipped update because IPv4 address is already set to $ipv4") if defined($ipv4); success("skipped update because IPv6 address is already set to $ipv6") @@ -3541,6 +3545,8 @@ sub nic_updateable { } } + # TODO: `status` is set by legacy protocol implementations. Remove it from this list once all + # legacy protocol implementations have been upgraded. delete($config{$host}{$_}) for qw(status status-ipv4 status-ipv6 update); if ($update) { $config{$host}{'update'} = 1; @@ -3549,7 +3555,7 @@ sub nic_updateable { delete $recap{$host}{'warned-min-interval'}; delete $recap{$host}{'warned-min-error-interval'}; } else { - for (qw(status status-ipv4 status-ipv6)) { + for (qw(status-ipv4 status-ipv6)) { $config{$host}{$_} = $recap{$host}{$_} if defined($recap{$host}{$_}); } delete($config{$host}{$_}) for qw(wantip wantipv4 wantipv6); diff --git a/t/update_nics.pl b/t/update_nics.pl index 4b581a5..9807703 100644 --- a/t/update_nics.pl +++ b/t/update_nics.pl @@ -75,15 +75,15 @@ my @test_cases = ( want_update => 1, want_recap_changes => { 'atime' => $ddclient::now, - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv4' => 'good', }, want_cfg_changes => { 'atime' => $ddclient::now, - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv4' => 'good', }, %$_, }; @@ -99,15 +99,15 @@ my @test_cases = ( want_update => 1, want_recap_changes => { 'atime' => $ddclient::now, - 'ip' => '2001:db8::1', + 'ipv6' => '2001:db8::1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv6' => 'good', }, want_cfg_changes => { 'atime' => $ddclient::now, - 'ip' => '2001:db8::1', + 'ipv6' => '2001:db8::1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv6' => 'good', }, }, { @@ -120,15 +120,15 @@ my @test_cases = ( want_update => 1, want_recap_changes => { 'atime' => $ddclient::now, - 'ip' => '2001:db8::1', + 'ipv6' => '2001:db8::1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv6' => 'good', }, want_cfg_changes => { 'atime' => $ddclient::now, - 'ip' => '2001:db8::1', + 'ipv6' => '2001:db8::1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv6' => 'good', }, }, { @@ -142,15 +142,15 @@ my @test_cases = ( want_update => 1, want_recap_changes => { 'atime' => $ddclient::now, - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv4' => 'good', }, want_cfg_changes => { 'atime' => $ddclient::now, - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv4' => 'good', }, }, map({ @@ -160,9 +160,9 @@ my @test_cases = ( desc => "legacy, no change, not yet time, $desc", recap => { 'atime' => $ddclient::now - ddclient::opt('min-interval'), - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now - ddclient::opt('min-interval'), - 'status' => 'good', + 'status-ipv4' => 'good', }, cfg => { 'protocol' => 'legacy', @@ -170,10 +170,7 @@ my @test_cases = ( }, %$_, }; - } - {cfg => {use => 'web'}}, - {cfg => {usev4 => 'webv4'}, - want_recap_changes_TODO => 'usev4 and usev6 should check status from legacy protocols'}), + } {cfg => {use => 'web'}}, {cfg => {usev4 => 'webv4'}}), map({ my %cfg = %{delete($_->{cfg})}; my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); @@ -181,9 +178,9 @@ my @test_cases = ( desc => "legacy, min-interval elapsed but no change, $desc", recap => { 'atime' => $ddclient::now - ddclient::opt('min-interval') - 1, - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now - ddclient::opt('min-interval') - 1, - 'status' => 'good', + 'status-ipv4' => 'good', }, cfg => { 'protocol' => 'legacy', @@ -191,10 +188,7 @@ my @test_cases = ( }, %$_, }; - } - {cfg => {use => 'web'}}, - {cfg => {usev4 => 'webv4'}, - want_recap_changes_TODO => 'usev4 and usev6 should check status from legacy protocols'}), + } {cfg => {use => 'web'}}, {cfg => {usev4 => 'webv4'}}), map({ my %cfg = %{delete($_->{cfg})}; my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); @@ -202,9 +196,9 @@ my @test_cases = ( desc => "legacy, needs update, not yet time, $desc", recap => { 'atime' => $ddclient::now - ddclient::opt('min-interval'), - 'ip' => '192.0.2.2', + 'ipv4' => '192.0.2.2', 'mtime' => $ddclient::now - ddclient::opt('min-interval'), - 'status' => 'good', + 'status-ipv4' => 'good', }, cfg => { 'protocol' => 'legacy', @@ -215,10 +209,7 @@ my @test_cases = ( }, %$_, }; - } - {cfg => {use => 'web'}}, - {cfg => {usev4 => 'webv4'}, - want_recap_changes_TODO => 'usev4 and usev6 should check status from legacy protocols'}), + } {cfg => {use => 'web'}}, {cfg => {usev4 => 'webv4'}}), map({ my %cfg = %{delete($_->{cfg})}; my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); @@ -226,9 +217,9 @@ my @test_cases = ( desc => "legacy, min-interval elapsed, needs update, $desc", recap => { 'atime' => $ddclient::now - ddclient::opt('min-interval') - 1, - 'ip' => '192.0.2.2', + 'ipv4' => '192.0.2.2', 'mtime' => $ddclient::now - ddclient::opt('min-interval') - 1, - 'status' => 'good', + 'status-ipv4' => 'good', }, cfg => { 'protocol' => 'legacy', @@ -237,22 +228,17 @@ my @test_cases = ( want_update => 1, want_recap_changes => { 'atime' => $ddclient::now, - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now, }, want_cfg_changes => { 'atime' => $ddclient::now, - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now, }, %$_, }; - } - {cfg => {use => 'web'}}, - {cfg => {usev4 => 'webv4'}, - want_update_TODO => 'usev4 and usev6 should check status from legacy protocols', - want_cfg_changes_TODO => 'usev4 and usev6 should check status from legacy protocols', - want_recap_changes_TODO => 'usev4 and usev6 should check status from legacy protocols'}), + } {cfg => {use => 'web'}}, {cfg => {usev4 => 'webv4'}}), map({ my %cfg = %{delete($_->{cfg})}; my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); @@ -260,10 +246,10 @@ my @test_cases = ( desc => "legacy, previous failed update, not yet time to retry, $desc", recap => { 'atime' => $ddclient::now - ddclient::opt('min-error-interval'), - 'ip' => '192.0.2.2', + 'ipv4' => '192.0.2.2', 'mtime' => $ddclient::now - max(ddclient::opt('min-error-interval'), ddclient::opt('min-interval')) - 1, - 'status' => 'failed', + 'status-ipv4' => 'failed', }, cfg => { 'protocol' => 'legacy', @@ -282,9 +268,9 @@ my @test_cases = ( desc => "legacy, previous failed update, time to retry, $desc", recap => { 'atime' => $ddclient::now - ddclient::opt('min-error-interval') - 1, - 'ip' => '192.0.2.2', + 'ipv4' => '192.0.2.2', 'mtime' => $ddclient::now - ddclient::opt('min-error-interval') - 2, - 'status' => 'failed', + 'status-ipv4' => 'failed', }, cfg => { 'protocol' => 'legacy', @@ -293,15 +279,15 @@ my @test_cases = ( want_update => 1, want_recap_changes => { 'atime' => $ddclient::now, - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv4' => 'good', }, want_cfg_changes => { 'atime' => $ddclient::now, - 'ip' => '192.0.2.1', + 'ipv4' => '192.0.2.1', 'mtime' => $ddclient::now, - 'status' => 'good', + 'status-ipv4' => 'good', }, %$_, };