From 555359dc984b562044694ce89b0011912005df11 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 19 Aug 2024 17:41:45 -0400 Subject: [PATCH 01/11] tests: Add unit tests for legacy protocol handling in `nic_updateable` --- Makefile.am | 1 + configure.ac | 1 + t/update_nics.pl | 463 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 465 insertions(+) create mode 100644 t/update_nics.pl diff --git a/Makefile.am b/Makefile.am index a9d782b..4f0f916 100644 --- a/Makefile.am +++ b/Makefile.am @@ -80,6 +80,7 @@ handwritten_tests = \ t/protocol_dyndns2.pl \ t/skip.pl \ t/ssl-validate.pl \ + t/update_nics.pl \ t/use_web.pl \ t/variable_defaults.pl \ t/write_recap.pl diff --git a/configure.ac b/configure.ac index 5ca6da7..ab9ef75 100644 --- a/configure.ac +++ b/configure.ac @@ -77,6 +77,7 @@ m4_foreach_w([_m], [ B File::Spec::Functions File::Temp + List::Util ], [AX_PROG_PERL_MODULES([_m], [], [AC_MSG_WARN([some tests will fail due to missing module _m])])]) diff --git a/t/update_nics.pl b/t/update_nics.pl new file mode 100644 index 0000000..e5d0ee0 --- /dev/null +++ b/t/update_nics.pl @@ -0,0 +1,463 @@ +use Test::More; +use File::Temp; +use List::Util qw(max); +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 $ipv6_supported = eval { + require IO::Socket::IP; + my $ipv6_socket = IO::Socket::IP->new( + Domain => 'PF_INET6', + LocalHost => '::1', + Listen => 1, + ); + defined($ipv6_socket); +}; +my $http_daemon_supports_ipv6 = eval { + require HTTP::Daemon; + HTTP::Daemon->VERSION(6.12); +}; + +sub run_httpd { + my ($ipv) = @_; + return undef if $ipv eq '6' && (!$ipv6_supported || !$http_daemon_supports_ipv6); + my $httpd = ddclient::Test::Fake::HTTPD->new( + host => $ipv eq '4' ? '127.0.0.1' : '::1', + daemon_args => {V6Only => 1}, + ); + my $ip = $ipv eq '4' ? '192.0.2.1' : '2001:db8::1'; + $httpd->run(sub { return [200, ['content-type' => 'text/plain; charset=utf-8'], [$ip]]; }); + diag("started IPv$ipv HTTP server running at " . $httpd->endpoint()); + return $httpd; +} +my %httpd = ( + '4' => run_httpd('4'), + '6' => run_httpd('6'), +); +local %ddclient::builtinweb = ( + v4 => {url => "" . $httpd{'4'}->endpoint()}, + defined($httpd{'6'}) ? (v6 => {url => "" . $httpd{'6'}->endpoint()}) : (), +); + +local $ddclient::globals{debug} = 1; +local $ddclient::globals{verbose} = 1; +local $ddclient::now = 1000; +our @updates; +local %ddclient::protocols = ( + # The `legacy` protocol reads the legacy `wantip` property and sets the legacy `ip` and `status` + # properties. (Modern protocol implementations read `wantipv4` and `wantipv6` and set `ipv4`, + # `ipv6`, `status-ipv4`, and `status-ipv6`.) It always succeeds. + legacy => { + update => sub { + for my $h (@_) { + push(@updates, [@_]); + $ddclient::config{$h}{status} = 'good'; + $ddclient::config{$h}{ip} = delete($ddclient::config{$h}{wantip}); + $ddclient::config{$h}{mtime} = $ddclient::now; + } + }, + variables => { + %{$ddclient::variables{'protocol-common-defaults'}}, + }, + }, +); + +my @test_cases = ( + map({ + my %cfg = %{delete($_->{cfg})}; + my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); + { + desc => "legacy, fresh, $desc", + cfg => { + 'protocol' => 'legacy', + 'use' => 'disabled', + %cfg, + }, + want_update => 1, + want_recap_changes => { + 'atime' => $ddclient::now, + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + want_cfg_changes => { + 'atime' => $ddclient::now, + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantipv4' => '192.0.2.1', + 'wantipv6' => undef, + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + %$_, + }; + } {cfg => {use => 'web'}}, {cfg => {usev4 => 'webv4'}}), + { + desc => 'legacy, fresh, use=web (IPv6)', + ipv6 => 1, + cfg => { + 'protocol' => 'legacy', + 'use' => 'web', + 'web' => 'v6', + }, + want_update => 1, + want_recap_changes => { + 'atime' => $ddclient::now, + 'ip' => '2001:db8::1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + want_cfg_changes => { + 'atime' => $ddclient::now, + 'ip' => '2001:db8::1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantipv4' => undef, + 'wantipv6' => '2001:db8::1', + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + }, + { + desc => 'legacy, fresh, usev6=webv6', + ipv6 => 1, + cfg => { + 'protocol' => 'legacy', + 'use' => 'disabled', + 'usev6' => 'webv6', + }, + want_update => 1, + want_recap_changes => { + 'atime' => $ddclient::now, + 'ip' => '2001:db8::1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + want_cfg_changes => { + 'atime' => $ddclient::now, + 'ip' => '2001:db8::1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantipv4' => undef, + 'wantipv6' => '2001:db8::1', + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + }, + { + desc => 'legacy, fresh, usev4=webv4 usev6=webv6', + ipv6 => 1, + cfg => { + 'protocol' => 'legacy', + 'use' => 'disabled', + 'usev4' => 'webv4', + 'usev6' => 'webv6', + }, + want_update => 1, + want_recap_changes => { + 'atime' => $ddclient::now, + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + want_cfg_changes => { + 'atime' => $ddclient::now, + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantipv4' => '192.0.2.1', + 'wantipv6' => '2001:db8::1', + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + }, + map({ + my %cfg = %{delete($_->{cfg})}; + my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); + { + desc => "legacy, no change, not yet time, $desc", + recap => { + 'atime' => $ddclient::now - ddclient::opt('min-interval'), + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now - ddclient::opt('min-interval'), + 'status' => 'good', + }, + cfg => { + 'protocol' => 'legacy', + 'use' => 'disabled', + %cfg, + }, + want_cfg_changes => { + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantip' => '192.0.2.1', + 'wantipv4' => '192.0.2.1', + 'wantipv6' => undef, + }, + %$_, + }; + } + {cfg => {use => 'web'}}, + {cfg => {usev4 => 'webv4'}, + want_recap_changes_TODO => 'usev4 and usev6 should check status from legacy protocols'}), + map({ + my %cfg = %{delete($_->{cfg})}; + my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); + { + desc => "legacy, min-interval elapsed but no change, $desc", + recap => { + 'atime' => $ddclient::now - ddclient::opt('min-interval') - 1, + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now - ddclient::opt('min-interval') - 1, + 'status' => 'good', + }, + cfg => { + 'protocol' => 'legacy', + 'use' => 'disabled', + %cfg, + }, + want_cfg_changes => { + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantip' => '192.0.2.1', + 'wantipv4' => '192.0.2.1', + 'wantipv6' => undef, + }, + %$_, + }; + } + {cfg => {use => 'web'}}, + {cfg => {usev4 => 'webv4'}, + want_recap_changes_TODO => 'usev4 and usev6 should check status from legacy protocols'}), + map({ + my %cfg = %{delete($_->{cfg})}; + my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); + { + desc => "legacy, needs update, not yet time, $desc", + recap => { + 'atime' => $ddclient::now - ddclient::opt('min-interval'), + 'ip' => '192.0.2.2', + 'mtime' => $ddclient::now - ddclient::opt('min-interval'), + 'status' => 'good', + }, + cfg => { + 'protocol' => 'legacy', + 'use' => 'disabled', + %cfg, + }, + want_recap_changes => { + 'warned-min-interval' => $ddclient::now, + }, + want_cfg_changes => { + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantip' => '192.0.2.1', + 'wantipv4' => '192.0.2.1', + 'wantipv6' => undef, + }, + %$_, + }; + } + {cfg => {use => 'web'}}, + {cfg => {usev4 => 'webv4'}, + want_recap_changes_TODO => 'usev4 and usev6 should check status from legacy protocols'}), + map({ + my %cfg = %{delete($_->{cfg})}; + my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); + { + desc => "legacy, min-interval elapsed, needs update, $desc", + recap => { + 'atime' => $ddclient::now - ddclient::opt('min-interval') - 1, + 'ip' => '192.0.2.2', + 'mtime' => $ddclient::now - ddclient::opt('min-interval') - 1, + 'status' => 'good', + }, + cfg => { + 'protocol' => 'legacy', + 'use' => 'disabled', + %cfg, + }, + want_update => 1, + want_recap_changes => { + 'atime' => $ddclient::now, + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now, + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + want_cfg_changes => { + 'atime' => $ddclient::now, + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now, + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantipv4' => '192.0.2.1', + 'wantipv6' => undef, + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + %$_, + }; + } + {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'}), + map({ + my %cfg = %{delete($_->{cfg})}; + my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); + { + 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', + 'mtime' => $ddclient::now - max(ddclient::opt('min-error-interval'), + ddclient::opt('min-interval')) - 1, + 'status' => 'failed', + }, + cfg => { + 'protocol' => 'legacy', + 'use' => 'disabled', + %cfg, + }, + want_recap_changes => { + 'warned-min-error-interval' => $ddclient::now, + }, + want_cfg_changes => { + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantip' => '192.0.2.1', + 'wantipv4' => '192.0.2.1', + 'wantipv6' => undef, + }, + %$_, + }; + } {cfg => {use => 'web'}}, {cfg => {usev4 => 'webv4'}}), + map({ + my %cfg = %{delete($_->{cfg})}; + my $desc = join(' ', map("$_=$cfg{$_}", keys(%cfg))); + { + desc => "legacy, previous failed update, time to retry, $desc", + recap => { + 'atime' => $ddclient::now - ddclient::opt('min-error-interval') - 1, + 'ip' => '192.0.2.2', + 'mtime' => $ddclient::now - ddclient::opt('min-error-interval') - 2, + 'status' => 'failed', + }, + cfg => { + 'protocol' => 'legacy', + 'use' => 'disabled', + %cfg, + }, + want_update => 1, + want_recap_changes => { + 'atime' => $ddclient::now, + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + want_cfg_changes => { + 'atime' => $ddclient::now, + 'ip' => '192.0.2.1', + 'mtime' => $ddclient::now, + 'status' => 'good', + 'status-ipv4' => undef, + 'status-ipv6' => undef, + 'wantipv4' => '192.0.2.1', + 'wantipv6' => undef, + 'warned-min-error-interval' => 0, + 'warned-min-interval' => 0, + 'wtime' => 0, + }, + %$_, + }; + } {cfg => {use => 'web'}}, {cfg => {usev4 => 'webv4'}}), +); + +for my $tc (@test_cases) { + SKIP: { + skip("IPv6 not supported on this system", 1) if $tc->{ipv6} && !$ipv6_supported; + skip("HTTP::Daemon too old for IPv6 support", 1) + if $tc->{ipv6} && !$http_daemon_supports_ipv6; + subtest($tc->{desc} => sub { + local $ddclient::_l = ddclient::pushlogctx($tc->{desc}); + # Copy %{$tc->{recap}} so that updates to $recap{$h} don't update %{$tc->{recap}}. + local %ddclient::recap = (host => {%{$tc->{recap} // {}}}); + my $cachef = File::Temp->new(); + # $cachef is an object that stringifies to a filename. + local $ddclient::globals{cache} = "$cachef"; + my %cfg = ( + %{$tc->{recap} // {}}, # Simulate a previous update. + web => 'v4', + webv4 => 'v4', + webv6 => 'v6', + %{$tc->{cfg} // {}}, + ); + # Copy %cfg so that updates to $config{$h} don't update %cfg. + local %ddclient::config = (host => {%cfg}); + local @updates; + + ddclient::update_nics(); + + TODO: { + local $TODO = $tc->{want_update_TODO}; + is_deeply(\@updates, [(['host']) x ($tc->{want_update} ? 1 : 0)], + 'got expected update'); + } + my %want_recap = (host => { + %{$tc->{recap} // {}}, + %{$tc->{want_recap_changes} // {}}, + }); + TODO: { + local $TODO = $tc->{want_recap_changes_TODO}; + is_deeply(\%ddclient::recap, \%want_recap, 'recap matches') + or diag(ddclient::repr(Values => [\%ddclient::recap, \%want_recap], + Names => ['*got', '*want'])); + } + my %want_cfg = (host => { + update => $tc->{want_update} ? 1 : 0, + %cfg, + %{$tc->{want_cfg_changes} // {}}, + }); + TODO: { + local $TODO = $tc->{want_cfg_changes_TODO}; + is_deeply(\%ddclient::config, \%want_cfg, 'config matches') + or diag(ddclient::repr(Values => [\%ddclient::config, \%want_cfg], + Names => ['*got', '*want'])); + } + }); + } +} + +done_testing(); From b9ec2d42a301255af22990c9509bb9462482eded Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 17 Aug 2024 00:50:45 -0400 Subject: [PATCH 02/11] Remove the (broken and unused?) `--retry` option --- ChangeLog.md | 2 ++ ddclient.in | 14 -------------- sample-etc_cron.d_ddclient | 4 ---- 3 files changed, 2 insertions(+), 18 deletions(-) diff --git a/ChangeLog.md b/ChangeLog.md index b8cf192..68db4e9 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -48,6 +48,8 @@ repository history](https://github.com/ddclient/ddclient/commits/master). removed. [#716](https://github.com/ddclient/ddclient/pull/716) * `googledomains`: Support was removed because the service shut down. [#716](https://github.com/ddclient/ddclient/pull/716) + * The `--retry` option was removed. + [#732](https://github.com/ddclient/ddclient/pull/732) ### New features diff --git a/ddclient.in b/ddclient.in index 88e4c5d..6236c94 100755 --- a/ddclient.in +++ b/ddclient.in @@ -599,7 +599,6 @@ our %variables = ( 'cmdv6' => setv(T_PROG, 0, 0, undef, undef), 'timeout' => setv(T_DELAY, 0, 0, interval('120s'), interval('120s')), - 'retry' => setv(T_BOOL, 0, 0, 0, undef), 'force' => setv(T_BOOL, 0, 0, 0, undef), 'ssl' => setv(T_BOOL, 0, 0, 1, undef), 'syslog' => setv(T_BOOL, 0, 0, 0, undef), @@ -1221,7 +1220,6 @@ my @opt = ( ["ssl_ca_file", "=s", "--ssl_ca_file= : look at for certificates of trusted certificate authorities (default: auto-detect)"], ["fw-ssl-validate", "!", "--{no}fw-ssl-validate : Validate SSL certificate when retrieving IP address from firewall"], ["web-ssl-validate", "!", "--{no}web-ssl-validate : Validate SSL certificate when retrieving IP address from web"], - ["retry", "!", "--{no}retry : Initiate a one-time update attempt for hosts that have not been successfully updated (according to the cache). Incompatible with '--daemon'"], ["force", "!", "--{no}force : force an update even if the update may be unnecessary"], ["timeout", "=i", "--timeout= : when fetching a URL, wait at most seconds for a response"], ["syslog", "!", "--{no}syslog : log messages to syslog"], @@ -1937,23 +1935,11 @@ sub init_config { # if they are not associated with a host via `--host=` or `--options=host=`)? } - ## sanity check - if (defined(opt('host')) && opt('retry')) { - fatal("options --retry and --host (or --option host=..) are mutually exclusive"); - } - fatal("options --retry and --daemon cannot be used together") if (opt('retry') && opt('daemon')); - ## determine hosts to update (those on the cmd-line, config-file, or failed in recap) my @hosts = keys %config; if (opt('host')) { @hosts = split_by_comma($opt{'host'}); } - # TODO: The first two times init_config() is called the cache file has not been read yet, so - # this will not filter out any hosts and thus updates will not be limited to non-good hosts as - # intended. - if (opt('retry')) { - @hosts = grep(($recap{$_}{'status'} // '') ne 'good', keys(%recap)); - } ## remove any other hosts my %hosts; diff --git a/sample-etc_cron.d_ddclient b/sample-etc_cron.d_ddclient index b081832..6019805 100644 --- a/sample-etc_cron.d_ddclient +++ b/sample-etc_cron.d_ddclient @@ -10,7 +10,3 @@ ## force an update twice a month (only if you are not using daemon-mode) ## ## 30 23 1,15 * * root /usr/bin/ddclient -daemon=0 -syslog -quiet -force -###################################################################### -## retry failed updates every hour (only if you are not using daemon-mode) -## -## 0 * * * * root /usr/bin/ddclient -daemon=0 -syslog -quiet retry From 533e4735cd2d08890e8893325b2b5a483132eaca Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 18 Aug 2024 02:44:40 -0400 Subject: [PATCH 03/11] init_config: Support any variable as a command-line arg This doesn't add any new command-line arguments, but it does mean that a new command-line argument can be added for any variable, not just those in `$variables{'global-defaults'}`, and its value will be copied to `%globals`. My main motivation for this commit is to make it possible to remove the redundant variable declarations between `$variables{'global-defaults'}` and `$variables{'protocol-common-defaults'}`. --- ddclient.in | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ddclient.in b/ddclient.in index 6236c94..24e4c52 100755 --- a/ddclient.in +++ b/ddclient.in @@ -1923,9 +1923,7 @@ sub init_config { ## override global options with those on the command-line. for my $o (keys %opt) { - # TODO: Why is this limited to $variables{'global-defaults'}? Why not - # $variables{'merged'}? - if (defined $opt{$o} && exists $variables{'global-defaults'}{$o}) { + if (defined $opt{$o} && exists $variables{'merged'}{$o}) { # TODO: What's the point of this? The opt() function will fall back to %globals if # %opt doesn't have a value, so this shouldn't be necessary. $globals{$o} = $opt{$o}; From 603a59ffe39b7e64e881dc57faa4ca08c6cf0e6f Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 18 Aug 2024 02:52:43 -0400 Subject: [PATCH 04/11] Delete redundant variable declarations from `global-defaults` --- ddclient.in | 28 ---------------------------- 1 file changed, 28 deletions(-) diff --git a/ddclient.in b/ddclient.in index 24e4c52..f9d2839 100755 --- a/ddclient.in +++ b/ddclient.in @@ -570,34 +570,6 @@ our %variables = ( 'proxy' => setv(T_FQDNP, 0, 0, undef, undef), 'protocol' => setv(T_PROTO, 0, 0, 'dyndns2', undef), - 'use' => setv(T_USE, 0, 0, 'ip', undef), - 'usev4' => setv(T_USEV4, 0, 0, 'disabled', undef), - 'usev6' => setv(T_USEV6, 0, 0, 'disabled', undef), - 'ip' => setv(T_IP, 0, 0, undef, undef), - 'ipv4' => setv(T_IPV4, 0, 0, undef, undef), - 'ipv6' => setv(T_IPV6, 0, 0, undef, undef), - 'if' => setv(T_IF, 0, 0, 'ppp0', undef), - 'ifv4' => setv(T_IF, 0, 0, 'default', undef), - 'ifv6' => setv(T_IF, 0, 0, 'default', undef), - 'web' => setv(T_STRING,0, 0, 'dyndns', undef), - 'web-skip' => setv(T_STRING,0, 0, undef, undef), - 'webv4' => setv(T_STRING,0, 0, 'ipify-ipv4', undef), - 'webv4-skip' => setv(T_STRING,0, 0, undef, undef), - 'webv6' => setv(T_STRING,0, 0, 'ipify-ipv6', undef), - 'webv6-skip' => setv(T_STRING,0, 0, undef, undef), - 'fw' => setv(T_ANY, 0, 0, undef, undef), - 'fw-skip' => setv(T_STRING,0, 0, undef, undef), - 'fwv4' => setv(T_ANY, 0, 0, undef, undef), - 'fwv4-skip' => setv(T_STRING,0, 0, undef, undef), - 'fwv6' => setv(T_ANY, 0, 0, undef, undef), - 'fwv6-skip' => setv(T_STRING,0, 0, undef, undef), - 'fw-login' => setv(T_LOGIN, 0, 0, undef, undef), - 'fw-password' => setv(T_PASSWD,0, 0, undef, undef), - 'cmd' => setv(T_PROG, 0, 0, undef, undef), - 'cmd-skip' => setv(T_STRING,0, 0, undef, undef), - 'cmdv4' => setv(T_PROG, 0, 0, undef, undef), - 'cmdv6' => setv(T_PROG, 0, 0, undef, undef), - 'timeout' => setv(T_DELAY, 0, 0, interval('120s'), interval('120s')), 'force' => setv(T_BOOL, 0, 0, 0, undef), 'ssl' => setv(T_BOOL, 0, 0, 1, undef), From f23070a114a250cf9adbf13e8efa229da2f29c71 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 18 Aug 2024 02:56:22 -0400 Subject: [PATCH 05/11] Change defaults for `warned-min{,-error}-interval` from 0 to undef The code already treats `undef` and 0 the same, and `undef` omits them from the recap which simplifies testing. --- ddclient.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ddclient.in b/ddclient.in index f9d2839..a645032 100755 --- a/ddclient.in +++ b/ddclient.in @@ -664,13 +664,13 @@ our %variables = ( # made. This is used as a boolean to suppress repeated warnings to the user that indicate # that `min-interval` has inhibited an update attempt. # TODO: Change to a boolean and rename to improve readability. - 'warned-min-interval' => setv(T_ANY, 0, 1, 0, undef), + 'warned-min-interval' => setv(T_ANY, 0, 1, undef, undef), # Timestamp (seconds since epoch) of the most recent attempt that would have been made had # `min-error-interval` not inhibited the attempt. This is reset to 0 once an attempt is # actually made. This is used as a boolean to suppress repeated warnings to the user that # indicate that `min-error-interval` has inhibited an update attempt. # TODO: Change to a boolean and rename to improve readability. - 'warned-min-error-interval' => setv(T_ANY, 0, 1, 0, undef), + 'warned-min-error-interval' => setv(T_ANY, 0, 1, undef, undef), }, 'dyndns-common-defaults' => { 'backupmx' => setv(T_BOOL, 0, 1, 0, undef), From 46bd2f177120b885179afb77949e3483848f4eb9 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 21 Aug 2024 19:54:28 -0400 Subject: [PATCH 06/11] tests: Also test `default()` return value --- t/variable_defaults.pl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/t/variable_defaults.pl b/t/variable_defaults.pl index 09dc92c..b1ac203 100644 --- a/t/variable_defaults.pl +++ b/t/variable_defaults.pl @@ -23,10 +23,14 @@ for my $tc (@test_cases) { if ($tc->{def}{required}) { is($tc->{def}{default}, undef, "'$tc->{desc}' (required) has no default"); } else { + local %ddclient::variables = (merged => {var => $tc->{def}}); my $norm; - my $valid = eval { $norm = ddclient::check_value($tc->{def}{default}, $tc->{def}); 1; }; + my $default = ddclient::default('var'); + diag("'$tc->{desc}' default: " . ($default // '')); + is($default, $tc->{def}{default}, "'$tc->{desc}' default() return value matches default"); + my $valid = eval { $norm = ddclient::check_value($default, $tc->{def}); 1; } or diag($@); ok($valid, "'$tc->{desc}' (optional) has a valid default"); - is($norm, $tc->{def}{default}, "'$tc->{desc}' default normalizes to itself") if $valid; + is($norm, $default, "'$tc->{desc}' default normalizes to itself") if $valid; } } done_testing(); From f024bcce345cf598e6738fbf1b61c54fa365a603 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 18 Aug 2024 03:10:07 -0400 Subject: [PATCH 07/11] Dynamically compute default for `use` based on `usev4`, `usev6` This is mostly to simplify tests, but it also improves readability. The infrastructure changes in this commit also make it possible to introduce a new `url` variable that defaults to `opt('server', $h)` concatenated with `opt('script', $h)` so that we can start migrating away from those user-unfriendly variables. --- configure.ac | 1 + ddclient.in | 22 ++++++++----- t/update_nics.pl | 9 ------ t/variable_defaults.pl | 71 ++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 84 insertions(+), 19 deletions(-) diff --git a/configure.ac b/configure.ac index ab9ef75..75edda5 100644 --- a/configure.ac +++ b/configure.ac @@ -78,6 +78,7 @@ m4_foreach_w([_m], [ File::Spec::Functions File::Temp List::Util + re ], [AX_PROG_PERL_MODULES([_m], [], [AC_MSG_WARN([some tests will fail due to missing module _m])])]) diff --git a/ddclient.in b/ddclient.in index a645032..4a2c91b 100755 --- a/ddclient.in +++ b/ddclient.in @@ -597,7 +597,13 @@ our %variables = ( 'password' => setv(T_PASSWD,1, 0, undef, undef), 'host' => setv(T_STRING,1, 1, undef, undef), - 'use' => setv(T_USE, 0, 0, 'ip', undef), + 'use' => setv(T_USE, 0, 0, sub { + my ($h) = @_; + return "'disabled' if '--usev4' or '--usev6' is enabled, otherwise 'ip'" + if ($h // '') eq ''; + return 'disabled' if opt('usev4', $h) ne 'disabled' || opt('usev6', $h) ne 'disabled'; + return 'ip'; + }, undef), 'usev4' => setv(T_USEV4, 0, 0, 'disabled', undef), 'usev6' => setv(T_USEV6, 0, 0, 'disabled', undef), 'if' => setv(T_IF, 0, 0, 'ppp0', undef), @@ -1092,7 +1098,7 @@ $variables{'merged'} = { }; # This will hold the processed args. -my %opt = (); +our %opt; my $deprecated_handler = sub { warning("'-$_[0]' is deprecated and does nothing"); }; $opt{'fw-banlocal'} = $deprecated_handler; $opt{'if-skip'} = $deprecated_handler; @@ -1937,7 +1943,7 @@ sub usage { for (@_) { if (ref $_) { my ($key, $specifier, $arg_usage) = @$_; - my $value = default($key); + my $value = default($key, ''); next unless $arg_usage; $usage .= " $arg_usage"; if (defined($value) && $value ne '') { @@ -2220,15 +2226,17 @@ sub split_by_comma { } sub default { my ($v, $h) = @_; + my $var; if (defined($h) && $config{$h}) { my $proto = $protocols{opt('protocol', $v eq 'protocol' ? undef : $h)}; - my $var = $proto->{variables}{$v} if $proto; - return $var->{default} if $var; + $var = $proto->{variables}{$v} if $proto; } - return undef if !defined($variables{'merged'}{$v}); # TODO: This might grab an arbitrary protocol-specific variable definition, which could cause # surprising behavior. - return $variables{'merged'}{$v}{'default'}; + $var //= $variables{'merged'}{$v}; + return undef if !defined($var); + return $var->{'default'}($h) if ref($var->{default}) eq 'CODE'; + return $var->{'default'}; } sub opt { my $v = shift; diff --git a/t/update_nics.pl b/t/update_nics.pl index e5d0ee0..5b48061 100644 --- a/t/update_nics.pl +++ b/t/update_nics.pl @@ -70,7 +70,6 @@ my @test_cases = ( desc => "legacy, fresh, $desc", cfg => { 'protocol' => 'legacy', - 'use' => 'disabled', %cfg, }, want_update => 1, @@ -136,7 +135,6 @@ my @test_cases = ( ipv6 => 1, cfg => { 'protocol' => 'legacy', - 'use' => 'disabled', 'usev6' => 'webv6', }, want_update => 1, @@ -168,7 +166,6 @@ my @test_cases = ( ipv6 => 1, cfg => { 'protocol' => 'legacy', - 'use' => 'disabled', 'usev4' => 'webv4', 'usev6' => 'webv6', }, @@ -209,7 +206,6 @@ my @test_cases = ( }, cfg => { 'protocol' => 'legacy', - 'use' => 'disabled', %cfg, }, want_cfg_changes => { @@ -238,7 +234,6 @@ my @test_cases = ( }, cfg => { 'protocol' => 'legacy', - 'use' => 'disabled', %cfg, }, want_cfg_changes => { @@ -267,7 +262,6 @@ my @test_cases = ( }, cfg => { 'protocol' => 'legacy', - 'use' => 'disabled', %cfg, }, want_recap_changes => { @@ -299,7 +293,6 @@ my @test_cases = ( }, cfg => { 'protocol' => 'legacy', - 'use' => 'disabled', %cfg, }, want_update => 1, @@ -345,7 +338,6 @@ my @test_cases = ( }, cfg => { 'protocol' => 'legacy', - 'use' => 'disabled', %cfg, }, want_recap_changes => { @@ -374,7 +366,6 @@ my @test_cases = ( }, cfg => { 'protocol' => 'legacy', - 'use' => 'disabled', %cfg, }, want_update => 1, diff --git a/t/variable_defaults.pl b/t/variable_defaults.pl index b1ac203..8d82347 100644 --- a/t/variable_defaults.pl +++ b/t/variable_defaults.pl @@ -1,4 +1,5 @@ use Test::More; +use re qw(is_regexp); SKIP: { eval { require Test::Warnings; } or skip($@, 1); } eval { require 'ddclient'; } or BAIL_OUT($@); @@ -23,14 +24,78 @@ for my $tc (@test_cases) { if ($tc->{def}{required}) { is($tc->{def}{default}, undef, "'$tc->{desc}' (required) has no default"); } else { - local %ddclient::variables = (merged => {var => $tc->{def}}); + # Preserve all existing variables in $variables{merged} so that variables with dynamic + # defaults can reference them. + local %ddclient::variables = (merged => { + %{$ddclient::variables{merged}}, + 'var for test' => $tc->{def}, + }); + # Variables with dynamic defaults will need their own unit tests, but we can still check the + # clean-slate hostless default. + local %ddclient::config; + local %ddclient::opt; + local %ddclient::globals; my $norm; - my $default = ddclient::default('var'); + my $default = ddclient::default('var for test'); diag("'$tc->{desc}' default: " . ($default // '')); - is($default, $tc->{def}{default}, "'$tc->{desc}' default() return value matches default"); + is($default, $tc->{def}{default}, "'$tc->{desc}' default() return value matches default") + if ref($tc->{def}{default}) ne 'CODE'; my $valid = eval { $norm = ddclient::check_value($default, $tc->{def}); 1; } or diag($@); ok($valid, "'$tc->{desc}' (optional) has a valid default"); is($norm, $default, "'$tc->{desc}' default normalizes to itself") if $valid; } } + +my @use_test_cases = ( + { + desc => 'clean slate hostless default', + want => 'ip', + }, + { + desc => 'usage string', + host => '', + want => qr/disabled.*ip|ip.*disabled/, + }, + { + desc => 'usev4 disables use by default', + host => 'host', + cfg => {usev4 => 'webv4'}, + want => 'disabled', + }, + { + desc => 'usev6 disables use by default', + host => 'host', + cfg => {usev4 => 'webv4'}, + want => 'disabled', + }, + { + desc => 'explicitly setting use re-enables it', + host => 'host', + cfg => {use => 'web', usev4 => 'webv4'}, + want => 'web', + }, +); +for my $tc (@use_test_cases) { + my $desc = "'use' dynamic default: $tc->{desc}"; + local %ddclient::protocols = + (protocol => {variables => $ddclient::variables{'protocol-common-defaults'}}); + local %ddclient::variables = (merged => { + 'protocol' => $ddclient::variables{'merged'}{'protocol'}, + 'use' => $ddclient::variables{'protocol-common-defaults'}{'use'}, + 'usev4' => $ddclient::variables{'merged'}{'usev4'}, + 'usev6' => $ddclient::variables{'merged'}{'usev6'}, + }); + local %ddclient::config = (host => {protocol => 'protocol', %{$tc->{cfg} // {}}}); + local %ddclient::opt; + local %ddclient::globals; + + my $got = ddclient::opt('use', $tc->{host}); + + if (is_regexp($tc->{want})) { + like($got, $tc->{want}, $desc); + } else { + is($got, $tc->{want}, $desc); + } +} + done_testing(); From acd8dfe47fd4fe29bdaaa769148f92a3f2406326 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 18 Aug 2024 03:18:15 -0400 Subject: [PATCH 08/11] Don't force `use` to `disabled` if `usev4` or `usev6` is enabled Now that the default changes depending on `usev4` and `usev6`, this is no longer necessary. Removing it simplifies the code a bit and makes the behavior of unit tests match the overall behavior a bit better. --- ddclient.in | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ddclient.in b/ddclient.in index 4a2c91b..144bd2b 100755 --- a/ddclient.in +++ b/ddclient.in @@ -1143,7 +1143,7 @@ my @opt = ( ["cache", "=s", "--cache= : record address used in "], ["pid", "=s", "--pid= : record process id in if daemonized"], "", - ["use", "=s", "--use= : deprecated, see '--usev4' and '--usev6' (forced to 'disabled' if either '--usev4' or '--usev6' is enabled)"], + ["use", "=s", "--use= : deprecated, see '--usev4' and '--usev6'"], &ip_strategies_usage(), ["usev4", "=s", "--usev4= : how the IPv4 address should be obtained"], &ipv4_strategies_usage(), @@ -1925,10 +1925,6 @@ sub init_config { # TODO: Why aren't the hosts specified by --host added to %config except when --options is also # given? - for my $h (keys %config) { - $config{$h}{use} = 'disabled' - if opt('usev4', $h) ne 'disabled' || opt('usev6', $h) ne 'disabled'; - } my @protos = map(opt('protocol', $_), keys(%config)); my @needs_sha1 = grep({ my $p = $_; grep($_ eq $p, @protos); } qw(freedns nfsn)); load_sha1_support(join(', ', @needs_sha1)) if @needs_sha1; From a21e215ada6e2a84a71550dc3c9741bc07bdeafa Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 18 Aug 2024 03:26:35 -0400 Subject: [PATCH 09/11] Reduce unnecessary values in `%config` and `%recap` * Delete values from `$config{$h}` and `$recap{$h}` when resetting values (as opposed to setting a falsy value). * Delete values from `$config{$h}` and `$recap{$h}` when they are no longer needed. This is mostly done to improve the tests in `t/update_nics.pl`. --- ddclient.in | 24 ++++++------- t/update_nics.pl | 90 +----------------------------------------------- 2 files changed, 13 insertions(+), 101 deletions(-) diff --git a/ddclient.in b/ddclient.in index 144bd2b..119026b 100755 --- a/ddclient.in +++ b/ddclient.in @@ -1436,6 +1436,9 @@ sub update_nics { $0 = sprintf("%s - updating %s", $program, join(',', @hosts)); local $_l = pushlogctx($p); &$update(@hosts); + for my $h (@hosts) { + delete($config{$h}{$_}) for qw(wantip wantipv4 wantipv6); + } # Backwards compatibility: # The legacy '--use' parameter sets 'wantip' and the legacy providers process this and @@ -3541,21 +3544,18 @@ sub nic_updateable { } } - $config{$host}{'status'} = $recap{$host}{'status'}; - $config{$host}{'status-ipv4'} = $recap{$host}{'status-ipv4'}; - $config{$host}{'status-ipv6'} = $recap{$host}{'status-ipv6'}; - $config{$host}{'update'} = $update; + delete($config{$host}{$_}) for qw(status status-ipv4 status-ipv6 update); if ($update) { - $config{$host}{'status'} = undef; - $config{$host}{'status-ipv4'} = undef; - $config{$host}{'status-ipv6'} = undef; - $config{$host}{'atime'} = $now; - $config{$host}{'wtime'} = 0; - $config{$host}{'warned-min-interval'} = 0; - $config{$host}{'warned-min-error-interval'} = 0; - + $config{$host}{'update'} = 1; + $config{$host}{'atime'} = $now; + delete($config{$host}{$_}) for qw(wtime warned-min-interval warned-min-error-interval); delete $recap{$host}{'warned-min-interval'}; delete $recap{$host}{'warned-min-error-interval'}; + } else { + for (qw(status status-ipv4 status-ipv6)) { + $config{$host}{$_} = $recap{$host}{$_} if defined($recap{$host}{$_}); + } + delete($config{$host}{$_}) for qw(wantip wantipv4 wantipv6); } return $update; diff --git a/t/update_nics.pl b/t/update_nics.pl index 5b48061..4b581a5 100644 --- a/t/update_nics.pl +++ b/t/update_nics.pl @@ -78,22 +78,12 @@ my @test_cases = ( 'ip' => '192.0.2.1', 'mtime' => $ddclient::now, 'status' => 'good', - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, want_cfg_changes => { 'atime' => $ddclient::now, 'ip' => '192.0.2.1', 'mtime' => $ddclient::now, 'status' => 'good', - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantipv4' => '192.0.2.1', - 'wantipv6' => undef, - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, %$_, }; @@ -112,22 +102,12 @@ my @test_cases = ( 'ip' => '2001:db8::1', 'mtime' => $ddclient::now, 'status' => 'good', - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, want_cfg_changes => { 'atime' => $ddclient::now, 'ip' => '2001:db8::1', 'mtime' => $ddclient::now, 'status' => 'good', - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantipv4' => undef, - 'wantipv6' => '2001:db8::1', - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, }, { @@ -143,22 +123,12 @@ my @test_cases = ( 'ip' => '2001:db8::1', 'mtime' => $ddclient::now, 'status' => 'good', - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, want_cfg_changes => { 'atime' => $ddclient::now, 'ip' => '2001:db8::1', 'mtime' => $ddclient::now, 'status' => 'good', - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantipv4' => undef, - 'wantipv6' => '2001:db8::1', - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, }, { @@ -175,22 +145,12 @@ my @test_cases = ( 'ip' => '192.0.2.1', 'mtime' => $ddclient::now, 'status' => 'good', - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, want_cfg_changes => { 'atime' => $ddclient::now, 'ip' => '192.0.2.1', 'mtime' => $ddclient::now, 'status' => 'good', - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantipv4' => '192.0.2.1', - 'wantipv6' => '2001:db8::1', - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, }, map({ @@ -208,13 +168,6 @@ my @test_cases = ( 'protocol' => 'legacy', %cfg, }, - want_cfg_changes => { - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantip' => '192.0.2.1', - 'wantipv4' => '192.0.2.1', - 'wantipv6' => undef, - }, %$_, }; } @@ -236,13 +189,6 @@ my @test_cases = ( 'protocol' => 'legacy', %cfg, }, - want_cfg_changes => { - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantip' => '192.0.2.1', - 'wantipv4' => '192.0.2.1', - 'wantipv6' => undef, - }, %$_, }; } @@ -267,13 +213,6 @@ my @test_cases = ( want_recap_changes => { 'warned-min-interval' => $ddclient::now, }, - want_cfg_changes => { - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantip' => '192.0.2.1', - 'wantipv4' => '192.0.2.1', - 'wantipv6' => undef, - }, %$_, }; } @@ -300,21 +239,11 @@ my @test_cases = ( 'atime' => $ddclient::now, 'ip' => '192.0.2.1', 'mtime' => $ddclient::now, - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, want_cfg_changes => { 'atime' => $ddclient::now, 'ip' => '192.0.2.1', 'mtime' => $ddclient::now, - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantipv4' => '192.0.2.1', - 'wantipv6' => undef, - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, %$_, }; @@ -343,13 +272,6 @@ my @test_cases = ( want_recap_changes => { 'warned-min-error-interval' => $ddclient::now, }, - want_cfg_changes => { - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantip' => '192.0.2.1', - 'wantipv4' => '192.0.2.1', - 'wantipv6' => undef, - }, %$_, }; } {cfg => {use => 'web'}}, {cfg => {usev4 => 'webv4'}}), @@ -374,22 +296,12 @@ my @test_cases = ( 'ip' => '192.0.2.1', 'mtime' => $ddclient::now, 'status' => 'good', - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, want_cfg_changes => { 'atime' => $ddclient::now, 'ip' => '192.0.2.1', 'mtime' => $ddclient::now, 'status' => 'good', - 'status-ipv4' => undef, - 'status-ipv6' => undef, - 'wantipv4' => '192.0.2.1', - 'wantipv6' => undef, - 'warned-min-error-interval' => 0, - 'warned-min-interval' => 0, - 'wtime' => 0, }, %$_, }; @@ -437,7 +349,7 @@ for my $tc (@test_cases) { Names => ['*got', '*want'])); } my %want_cfg = (host => { - update => $tc->{want_update} ? 1 : 0, + $tc->{want_update} ? (update => 1) : (), %cfg, %{$tc->{want_cfg_changes} // {}}, }); From 4f89492dc0c1e52eb4adc93c0a9f6ce593d6e3ea Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 17 Aug 2024 01:01:24 -0400 Subject: [PATCH 10/11] nic_updateable: Use `wantip` definedness, not `use` enabledness Rather than check whether `use`, `usev4`, or `usev6` is a non-disabled value, check that `wantip`, `wantipv4`, or `wantipv6` is a defined value. This is preparation for removing the `status` variable in a future commit. --- ddclient.in | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/ddclient.in b/ddclient.in index 119026b..280acce 100755 --- a/ddclient.in +++ b/ddclient.in @@ -3410,9 +3410,6 @@ sub nic_updateable { my $ip = $config{$host}{'wantip'}; my $ipv4 = $config{$host}{'wantipv4'}; my $ipv6 = $config{$host}{'wantipv6'}; - my $use = opt('use', $host); - my $usev4 = opt('usev4', $host); - my $usev6 = opt('usev6', $host); my $inv_ip_warn_count = opt('max-warn'); my $previp = $recap{$host}{'ip'} || ''; my $previpv4 = $recap{$host}{'ipv4'} || ''; @@ -3422,9 +3419,9 @@ sub nic_updateable { my %prettyi = map({ ($_ => prettyinterval(opt($_, $host))); } qw(max-interval min-error-interval min-interval)); - $warned_ip{$host} = 0 if $use ne 'disabled' && $ip; - $warned_ipv4{$host} = 0 if $usev4 ne 'disabled' && $ipv4; - $warned_ipv6{$host} = 0 if $usev6 ne 'disabled' && $ipv6; + $warned_ip{$host} = 0 if defined($ip); + $warned_ipv4{$host} = 0 if defined($ipv4); + $warned_ipv6{$host} = 0 if defined($ipv6); if (opt('force')) { info("update forced via 'force' option"); @@ -3441,7 +3438,7 @@ sub nic_updateable { info("update forced because it has been $prettyi{'max-interval'} since the previous update (on $prettyt{'mtime'})"); $update = 1; - } elsif ($use ne 'disabled' && $previp ne $ip) { + } 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')) { @@ -3469,7 +3466,7 @@ sub nic_updateable { $update = 1; } - } elsif ($usev4 ne 'disabled' && $previpv4 ne $ipv4) { + } 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')) { @@ -3497,7 +3494,7 @@ sub nic_updateable { $update = 1; } - } elsif ($usev6 ne 'disabled' && $previpv6 ne $ipv6) { + } 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')) { @@ -3536,11 +3533,11 @@ sub nic_updateable { } else { if (opt('verbose')) { success("skipped update because IP address is already set to $ip") - if $use ne 'disabled'; + if defined($ip); success("skipped update because IPv4 address is already set to $ipv4") - if $usev4 ne 'disabled'; + if defined($ipv4); success("skipped update because IPv6 address is already set to $ipv6") - if $usev6 ne 'disabled'; + if defined($ipv6); } } From de5d894c914d6ff707e30db37d999a2d00496573 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 17 Aug 2024 01:05:53 -0400 Subject: [PATCH 11/11] 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', }, %$_, };