From 9000b43d4694d53c71c8acc9f4a4f81d623702b0 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. (cherry picked from commit 89fa9a65927eb0542afaef55714d77953f09f0e0) --- 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 73f18ba78c..90b4be90f7 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -569,8 +569,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 bb9cc81dd49293649884610138fcfdcad3f9f803 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. (cherry picked from commit 863f51466e1d0ab8ab7410132d21fb0b0a4ba168) --- 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 5d26ebd0ff..655b6e01a2 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6252,7 +6252,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; } @@ -6314,7 +6314,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 b7149536eee868f1cb808d4fbe7fe31b92e094f7 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] (cherry picked from commit 041ffac0d7a443a5afb0843c7495bf39a4e39591) --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index 59297237ed..41928f2b81 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 6d64f9e4ec72ee63c5139818d86635a35002f55a 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. (cherry picked from commit ef344b1f52ff531a713a26aa7fbdb7715a7aadcf) --- 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 c1ddc0b06d..3a7669a23d 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -4601,7 +4601,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 840cad93c75fbaccba4aed8c5bfaef4374076142 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. (cherry picked from commit 0227565cf18e2c79b6bcc2710487a799545ac800) --- 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 d213c7a900..5f66233103 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -4063,6 +4063,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; } /*