From 104b67623541c94744e0bd4ab6e2c554f1e1dccf Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Mon, 26 Apr 2021 09:19:25 +0200
Subject: [PATCH] Serve-stale nit fixes

While working on the serve-stale backports, I noticed the following
oddities:

1. In the serve-stale system test, in one case we keep track of the
   time how long it took for dig to complete. In commit
   aaed7f9d8c2465790d769221dfe8378c7147f5eb, the code removed the
   exception to check for result == ISC_R_SUCCESS on stale found
   answers, and adjusted the test accordingly. This failed to update
   the time tracking accordingly. Move the t1/t2 time track variables
   back around the two dig commands to ensure the lookups resolved
   faster than the resolver-query-timeout.

2. We can remove the setting of NS_QUERYATTR_STALEOK and
   DNS_RDATASETATTR_STALE_ADDED on the "else if (stale_timeout)"
   code path, because they are added later when we know we have
   actually found a stale answer on a stale timeout lookup.

3. We should clear the NS_QUERYATTR_STALEOK flag from the client
   query attributes instead of DNS_RDATASETATTR_STALE_ADDED (that
   flag is set on the rdataset attributes).

4. In 'bin/named/config.c' we should set the configuration options
   in alpabetical order.

5. In the ARM, in the backports we have added "(stale)" between
   "cached" and "RRset" to make more clear a stale RRset may be
   returned in this scenario.
---
 bin/named/config.c                    | 2 +-
 bin/tests/system/serve-stale/tests.sh | 4 ++--
 doc/arm/reference.rst                 | 2 +-
 lib/ns/query.c                        | 5 +----
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/bin/named/config.c b/bin/named/config.c
index 76fc72eca9..b11e0c9df6 100644
--- a/bin/named/config.c
+++ b/bin/named/config.c
@@ -195,8 +195,8 @@ options {\n\
 	root-key-sentinel yes;\n\
 	servfail-ttl 1;\n\
 #	sortlist <none>\n\
-	stale-answer-enable false;\n\
 	stale-answer-client-timeout off;\n\
+	stale-answer-enable false;\n\
 	stale-answer-ttl 30; /* 30 seconds */\n\
 	stale-cache-enable false;\n\
 	stale-refresh-time 30; /* 30 seconds */\n\
diff --git a/bin/tests/system/serve-stale/tests.sh b/bin/tests/system/serve-stale/tests.sh
index 4a83e35ac6..b17d5363a4 100755
--- a/bin/tests/system/serve-stale/tests.sh
+++ b/bin/tests/system/serve-stale/tests.sh
@@ -1647,9 +1647,11 @@ status=$((status+ret))
 sleep 2
 
 echo_i "sending queries for tests $((n+1))-$((n+2))..."
+t1=`$PERL -e 'print time()'`
 $DIG -p ${PORT} +tries=1 +timeout=10  @10.53.0.3 data.example TXT > dig.out.test$((n+1)) &
 $DIG -p ${PORT} +tries=1 +timeout=10  @10.53.0.3 nodata.example TXT > dig.out.test$((n+2))
 wait
+t2=`$PERL -e 'print time()'`
 
 # We configured a long value of 30 seconds for resolver-query-timeout.
 # That should give us enough time to receive an stale answer from cache
@@ -1657,8 +1659,6 @@ wait
 n=$((n+1))
 echo_i "check stale data.example comes from cache (default stale-answer-client-timeout) ($n)"
 nextpart ns3/named.run > /dev/null
-t1=`$PERL -e 'print time()'`
-t2=`$PERL -e 'print time()'`
 wait_for_log 5 "data.example client timeout, stale answer used" ns3/named.run || ret=1
 ret=0
 grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst
index edd2d7337f..e3eb4b0ae5 100644
--- a/doc/arm/reference.rst
+++ b/doc/arm/reference.rst
@@ -1867,7 +1867,7 @@ Boolean Options
    is disabled.
 
    The maximum value for this option is ``resolver-query-timeout`` minus
-   one second. The minimum value, ``0``, causes a cached RRset to be
+   one second. The minimum value, ``0``, causes a cached (stale) RRset to be
    immediately returned if it is available while still attempting to
    refresh the data in cache. :rfc:`8767` recommends a value of ``1800``
    (milliseconds).
diff --git a/lib/ns/query.c b/lib/ns/query.c
index 7911012286..97b8a9d5a2 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -5917,9 +5917,6 @@ query_lookup(query_ctx_t *qctx) {
 			return (ns_query_done(qctx));
 		}
 	} else if (stale_timeout) {
-		qctx->client->query.attributes |= NS_QUERYATTR_STALEOK;
-		qctx->rdataset->attributes |= DNS_RDATASETATTR_STALE_ADDED;
-
 		if ((qctx->options & DNS_GETDB_STALEFIRST) != 0) {
 			if (!stale_found) {
 				/*
@@ -8033,7 +8030,7 @@ query_addanswer(query_ctx_t *qctx) {
 		 * We can clear the attribute to prevent redundant clearing
 		 * in subsequent lookups.
 		 */
-		qctx->client->query.attributes &= ~DNS_RDATASETATTR_STALE_ADDED;
+		qctx->client->query.attributes &= ~NS_QUERYATTR_STALEOK;
 	}
 
 	if (qctx->dns64) {