From 89fa9a65927eb0542afaef55714d77953f09f0e0 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 18 Oct 2022 13:06:19 +0000 Subject: [PATCH 1/5] Add another prefetch check in the resolver system test The test triggers a prefetch, but fails to check if it acutally happened, which prevented it from catching a bug when the record's TTL value matches the configured prefetch eligibility value. Check that prefetch happened by comparing the TTL values. --- bin/tests/system/resolver/ns4/tld2.db | 4 ++-- bin/tests/system/resolver/tests.sh | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/bin/tests/system/resolver/ns4/tld2.db b/bin/tests/system/resolver/ns4/tld2.db index 2d8f428cb9..45dc6943a5 100644 --- a/bin/tests/system/resolver/ns4/tld2.db +++ b/bin/tests/system/resolver/ns4/tld2.db @@ -21,9 +21,9 @@ $TTL 300 ns A 10.53.0.4 fetch.tld. NS ns.fetch.tld. ns.fetch.tld. A 10.53.0.6 -fetchall 10 TXT A short ttl fetchall 10 A 1.2.3.4 -fetchall 10 AAAA ::1 +fetchall 10 AAAA ::1 +fetchall 10 TXT A short ttl no-edns-version.tld. NS ns.no-edns-version.tld. ns.no-edns-version.tld. A 10.53.0.6 edns-version.tld. NS ns.edns-version.tld. diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index 3862db6492..78fa92c617 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -444,8 +444,12 @@ sleep "${ttl1:-0}" dig_with_opts @10.53.0.5 fetchall.tld any > dig.out.2.${n} || ret=1 ttl2=$(awk '/"A" "short" "ttl"/ { print $2 }' dig.out.2.${n}) sleep 1 -# check that the nameserver is still alive +# check that prefetch occurred; +# note that only one record is prefetched, which is the TXT record in this case, +# because of the order of the records in the cache dig_with_opts @10.53.0.5 fetchall.tld any > dig.out.3.${n} || ret=1 +ttl3=$(awk '/"A" "short" "ttl"/ { print $2 }' dig.out.3.${n}) +test "${ttl3:-0}" -gt "${ttl2:-1}" || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) From 863f51466e1d0ab8ab7410132d21fb0b0a4ba168 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 18 Oct 2022 12:59:47 +0000 Subject: [PATCH 2/5] Match prefetch eligibility behavior with ARM ARM states that the "eligibility" TTL is the smallest original TTL value that is accepted for a record to be eligible for prefetching, but the code, which implements the condition doesn't behave in that manner for the edge case when the TTL is equal to the configured eligibility value. Fix the code to check that the TTL is greater than, or equal to the configured eligibility value, instead of just greater than it. --- lib/dns/resolver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index cc552ea33a..42048d9107 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6179,7 +6179,7 @@ cache_name(fetchctx_t *fctx, dns_name_t *name, dns_message_t *message, /* * Mark the rdataset as being prefetch eligible. */ - if (rdataset->ttl > fctx->res->view->prefetch_eligible) { + if (rdataset->ttl >= fctx->res->view->prefetch_eligible) { rdataset->attributes |= DNS_RDATASETATTR_PREFETCH; } @@ -6241,7 +6241,7 @@ cache_name(fetchctx_t *fctx, dns_name_t *name, dns_message_t *message, /* * Mark the rdataset as being prefetch eligible. */ - if (rdataset->ttl > fctx->res->view->prefetch_eligible) + if (rdataset->ttl >= fctx->res->view->prefetch_eligible) { rdataset->attributes |= DNS_RDATASETATTR_PREFETCH; From 041ffac0d7a443a5afb0843c7495bf39a4e39591 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 18 Oct 2022 13:10:04 +0000 Subject: [PATCH 3/5] Add a CHANGES note for [GL #3603] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index c0e6b5b0a8..fde089e4fd 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +6002. [bug] Fix a resolver prefetch bug when the record's TTL value + is equal to the configured prefetch eligibility value, + but the record was erroneously not treated as eligible + for prefetching. [GL #3603] + 6001. [bug] Always call dns_adb_endudpfetch() after calling dns_adb_beginudpfetch() for UDP queries in resolver.c, in order to adjust back the quota. [GL #3598] From ef344b1f52ff531a713a26aa7fbdb7715a7aadcf Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 18 Oct 2022 13:21:01 +0000 Subject: [PATCH 4/5] Fix prefetch "trigger" value's documentation in ARM For the prefetch "trigger" parameter ARM states that when a cache record with a lower TTL value is encountered during query processing, it is refreshed. But in reality, the record is refreshed when the TTL value is lower or equal to the configured "trigger" value. Fix the documentation to make it match with with the code. --- doc/arm/reference.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index ff36672cc4..7fbb2d4e5a 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -4598,7 +4598,7 @@ Tuning :any:`prefetch` specifies the "trigger" TTL value at which prefetch of the current query takes place; when a cache record with a - lower TTL value is encountered during query processing, it is + lower or equal TTL value is encountered during query processing, it is refreshed. Valid trigger TTL values are 1 to 10 seconds. Values larger than 10 seconds are silently reduced to 10. Setting a trigger TTL to zero causes prefetch to be disabled. The default From 0227565cf18e2c79b6bcc2710487a799545ac800 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 20 Oct 2022 10:38:50 +0000 Subject: [PATCH 5/5] Getting the "prefetch" setting from the configuration cannot fail The "prefetch" setting is in "defaultconf" so it cannot fail, use INSIST to confirm that. The 'trigger' and 'eligible' variables are now prefixed with 'prefetch_' and their declaration moved to an upper level, because there is no more additional code block after this change. --- bin/named/server.c | 48 ++++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 7f4bb94e67..e988d2c8d4 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -4071,6 +4071,8 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, const cfg_obj_t *zonelist; const cfg_obj_t *dlzlist; const cfg_obj_t *dlz; + const cfg_obj_t *prefetch_trigger; + const cfg_obj_t *prefetch_eligible; unsigned int dlzargc; char **dlzargv; const cfg_obj_t *dyndb_list, *plugin_list; @@ -5570,33 +5572,29 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, obj = NULL; result = named_config_get(maps, "prefetch", &obj); - if (result == ISC_R_SUCCESS) { - const cfg_obj_t *trigger, *eligible; - - trigger = cfg_tuple_get(obj, "trigger"); - view->prefetch_trigger = cfg_obj_asuint32(trigger); - if (view->prefetch_trigger > 10) { - view->prefetch_trigger = 10; - } - eligible = cfg_tuple_get(obj, "eligible"); - if (cfg_obj_isvoid(eligible)) { - int m; - for (m = 1; maps[m] != NULL; m++) { - obj = NULL; - result = named_config_get(&maps[m], "prefetch", - &obj); - INSIST(result == ISC_R_SUCCESS); - eligible = cfg_tuple_get(obj, "eligible"); - if (cfg_obj_isuint32(eligible)) { - break; - } + INSIST(result == ISC_R_SUCCESS); + prefetch_trigger = cfg_tuple_get(obj, "trigger"); + view->prefetch_trigger = cfg_obj_asuint32(prefetch_trigger); + if (view->prefetch_trigger > 10) { + view->prefetch_trigger = 10; + } + prefetch_eligible = cfg_tuple_get(obj, "eligible"); + if (cfg_obj_isvoid(prefetch_eligible)) { + int m; + for (m = 1; maps[m] != NULL; m++) { + obj = NULL; + result = named_config_get(&maps[m], "prefetch", &obj); + INSIST(result == ISC_R_SUCCESS); + prefetch_eligible = cfg_tuple_get(obj, "eligible"); + if (cfg_obj_isuint32(prefetch_eligible)) { + break; } - INSIST(cfg_obj_isuint32(eligible)); - } - view->prefetch_eligible = cfg_obj_asuint32(eligible); - if (view->prefetch_eligible < view->prefetch_trigger + 6) { - view->prefetch_eligible = view->prefetch_trigger + 6; } + INSIST(cfg_obj_isuint32(prefetch_eligible)); + } + view->prefetch_eligible = cfg_obj_asuint32(prefetch_eligible); + if (view->prefetch_eligible < view->prefetch_trigger + 6) { + view->prefetch_eligible = view->prefetch_trigger + 6; } /*