From e430ce70390d181eb355c61f1f73c4c9d9943e06 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 12 Sep 2024 11:50:28 +0000 Subject: [PATCH 1/2] 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. --- 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 6643bdc233..f8d5cdc430 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -3417,7 +3417,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; @@ -3586,6 +3586,7 @@ normal_nses: } now = isc_stdtime_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 81b3c5d90821d14ee11bbdb68ccfa7319147d4ca Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 12 Sep 2024 12:17:28 +0000 Subject: [PATCH 2/2] 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. --- 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 c1b4ecd864..bf9cc7b577 100644 --- a/bin/tests/system/forward/clean.sh +++ b/bin/tests/system/forward/clean.sh @@ -19,6 +19,7 @@ rm -f ./*/named.memstats rm -f ./*/named.run ./*/named.run.prev ./*/ans.run rm -f ./*/named_dump.db rm -f ./ans*/query.log +rm -f ./statschannel.out.* rm -f ./dig.out.* rm -f ./ns*/managed-keys.bind* rm -f ./ns*/trusted.conf diff --git a/bin/tests/system/forward/ns4/named.conf.in b/bin/tests/system/forward/ns4/named.conf.in index e8d3bbb8a5..5777c0de51 100644 --- a/bin/tests/system/forward/ns4/named.conf.in +++ b/bin/tests/system/forward/ns4/named.conf.in @@ -29,6 +29,8 @@ options { trust-anchors { }; +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 2705db9a70..7a31febb25 100644 --- a/bin/tests/system/forward/tests.sh +++ b/bin/tests/system/forward/tests.sh @@ -164,6 +164,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