From ef344cbd5ea46434587320b69785bdfd5a78a61f Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 12 Sep 2024 11:50:28 +0000 Subject: [PATCH 1/3] Fix a 'serverquota' counter calculation bug The 'all_spilled' local variable in resolver.c:fctx_getaddresses() is 'true' by default, and only becomes false when there is at least one successfully found NS address. However, when a 'forward only;' configuration is used, the code jumps over the part where it looks for NS addresses and doesn't reset the 'all_spilled' to false, which results in incorretly increased 'serverquota' statistics variable, and also in invalid return error code from the function. The result code error didn't make any differences, because all codes other than 'ISC_R_SUCCESS' or 'DNS_R_WAIT' were treated in the same way, and the result code was never logged anywhere. Set the default value of 'all_spilled' to 'false', and only make it 'true' before actually starting to look up NS addresses. (cherry picked from commit e430ce70390d181eb355c61f1f73c4c9d9943e06) --- lib/dns/resolver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index f8f53d2650..98137a514d 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -3575,7 +3575,7 @@ fctx_getaddresses(fetchctx_t *fctx, bool badcache) { bool all_bad; dns_rdata_ns_t ns; bool need_alternate = false; - bool all_spilled = true; + bool all_spilled = false; unsigned int no_addresses = 0; unsigned int ns_processed = 0; @@ -3735,6 +3735,7 @@ normal_nses: } isc_stdtime_get(&now); + all_spilled = true; /* resets to false below after the first success */ INSIST(ISC_LIST_EMPTY(fctx->finds)); INSIST(ISC_LIST_EMPTY(fctx->altfinds)); From 8f617d79719d295fe78211df9d19c478bc3fd219 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 12 Sep 2024 12:17:28 +0000 Subject: [PATCH 2/3] Add a statistics channel check in the forward system test Check that the fix in the previous commit works and that the 'ServerQuota' counter in the statistics channel is still unset after a SERVFAIL result in a 'forward only' zone. (cherry picked from commit 81b3c5d90821d14ee11bbdb68ccfa7319147d4ca) --- bin/tests/system/forward/clean.sh | 1 + bin/tests/system/forward/ns4/named.conf.in | 2 ++ bin/tests/system/forward/tests.sh | 9 +++++++++ 3 files changed, 12 insertions(+) diff --git a/bin/tests/system/forward/clean.sh b/bin/tests/system/forward/clean.sh index 6d76bb013c..8473402a13 100644 --- a/bin/tests/system/forward/clean.sh +++ b/bin/tests/system/forward/clean.sh @@ -14,6 +14,7 @@ # # Clean up after forward tests. # +rm -f ./statschannel.out.* rm -f ./dig.out.* rm -f ./*/named.conf rm -f ./*/named.memstats diff --git a/bin/tests/system/forward/ns4/named.conf.in b/bin/tests/system/forward/ns4/named.conf.in index c97823dee0..800b9831a7 100644 --- a/bin/tests/system/forward/ns4/named.conf.in +++ b/bin/tests/system/forward/ns4/named.conf.in @@ -24,6 +24,8 @@ options { minimal-responses yes; }; +statistics-channels { inet 10.53.0.4 port @EXTRAPORT1@ allow { localhost; }; }; + zone "." { type hint; file "root.db"; diff --git a/bin/tests/system/forward/tests.sh b/bin/tests/system/forward/tests.sh index b9426997cc..4304dab277 100644 --- a/bin/tests/system/forward/tests.sh +++ b/bin/tests/system/forward/tests.sh @@ -107,6 +107,15 @@ grep "SERVFAIL" dig.out.$n.f2 >/dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) +# GL#1793 +n=$((n + 1)) +echo_i "checking that the 'serverquota' counter isn't increased because of the SERVFAIL in the previous check ($n)" +ret=0 +"${CURL}" "http://10.53.0.4:${EXTRAPORT1}/json/v1" 2>/dev/null >statschannel.out.$n +grep -F "ServerQuota" statschannel.out.$n >/dev/null && ret=1 +if [ $ret != 0 ]; then echo_i "failed"; fi +status=$((status + ret)) + n=$((n + 1)) echo_i "checking for negative caching of forwarder response ($n)" # prime the cache, shutdown the forwarder then check that we can From 904940167c0030d292a3232ea21347609d379e0b Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 17 Sep 2024 14:18:23 +0000 Subject: [PATCH 3/3] Update the resolver system test The 'bin/tests/system/resolver.c' tool used in the resolver system test uses the 'dns_client_setservers()' function, which sets up a resolution in 'forward only' mode. Since a bug was just fixed in 'fctx_getaddresses()', two expected failures in the resolver system test now fail with a different failure message, because 'fctx_getaddresses()' returns 'ISC_R_FAILURE' instead of 'res->quotaresp[dns_quotatype_server]', which is 'DNS_R_SERVFAIL' by default. Change the expected failure message. --- bin/tests/system/resolver/tests.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index d74b1be6d5..982ff9761b 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -54,7 +54,7 @@ if [ -x "${RESOLVE}" ]; then echo_i "checking that local bound address can be set (Can't query from a denied address) ($n)" ret=0 resolve_with_opts -b 10.53.0.8 -t a -s 10.53.0.1 www.example.org 2>resolve.out.ns1.test${n} || ret=1 - grep "resolution failed: SERVFAIL" resolve.out.ns1.test${n} >/dev/null || ret=1 + grep "resolution failed: failure" resolve.out.ns1.test${n} >/dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) @@ -98,7 +98,7 @@ if [ -x "${RESOLVE}" ]; then echo_i "checking handling of bogus referrals using dns_client ($n)" ret=0 resolve_with_opts -t a -s 10.53.0.1 www.example.com 2>resolve.out.ns1.test${n} || ret=1 - grep "resolution failed: SERVFAIL" resolve.out.ns1.test${n} >/dev/null || ret=1 + grep "resolution failed: failure" resolve.out.ns1.test${n} >/dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) fi