From 0bac9c7c5ca73a5a81c6d28ad9e6160591517ced Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 19 Aug 2021 12:14:21 +0200
Subject: [PATCH 1/7] Add stats unit test

Add a simple stats unit test that tests the existing library functions
isc_stats_ncounters, isc_stats_increment, isc_stats_decrement,
isc_stats_set, and isc_stats_update_if_greater.
---
 doc/dev/copyrights         |  10 +--
 lib/isc/tests/Makefile.am  |   1 +
 lib/isc/tests/stats_test.c | 122 +++++++++++++++++++++++++++++++++++++
 util/copyrights            |   1 +
 4 files changed, 129 insertions(+), 5 deletions(-)
 create mode 100644 lib/isc/tests/stats_test.c

diff --git a/doc/dev/copyrights b/doc/dev/copyrights
index 33b7151e9b..e18d4a45ba 100644
--- a/doc/dev/copyrights
+++ b/doc/dev/copyrights
@@ -5,10 +5,10 @@ perl util/merge_copyrights
 diff util/copyrights util/newcopyrights
 ... examine output, particularly any files with the "?" type, and
 ... then edit util/newcopyrights if necessary
-$ mv util/newcopyrights util/copyrights
-$ perl util/update_copyrights < util/copyrights
-$ git diff
+mv util/newcopyrights util/copyrights
+perl util/update_copyrights < util/copyrights
+git diff
 ... examine output, edit as necessary.  mail me about anything that
 ... the script should have been able to do itself. :-)
-$ git add util/copyrights
-$ git commit -m 'update_copyrights'
+git add util/copyrights
+git commit -m 'update_copyrights'
diff --git a/lib/isc/tests/Makefile.am b/lib/isc/tests/Makefile.am
index a8b81fc256..de488951ab 100644
--- a/lib/isc/tests/Makefile.am
+++ b/lib/isc/tests/Makefile.am
@@ -41,6 +41,7 @@ check_PROGRAMS =	\
 	siphash_test	\
 	sockaddr_test	\
 	socket_test	\
+	stats_test	\
 	symtab_test	\
 	task_test	\
 	taskpool_test	\
diff --git a/lib/isc/tests/stats_test.c b/lib/isc/tests/stats_test.c
new file mode 100644
index 0000000000..a212613143
--- /dev/null
+++ b/lib/isc/tests/stats_test.c
@@ -0,0 +1,122 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at https://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+#if HAVE_CMOCKA
+
+#include <sched.h> /* IWYU pragma: keep */
+#include <setjmp.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define UNIT_TESTING
+#include <cmocka.h>
+
+#include <isc/mem.h>
+#include <isc/result.h>
+#include <isc/stats.h>
+#include <isc/util.h>
+
+#include "isctest.h"
+
+static int
+_setup(void **state) {
+	isc_result_t result;
+
+	UNUSED(state);
+
+	result = isc_test_begin(NULL, true, 0);
+	assert_int_equal(result, ISC_R_SUCCESS);
+
+	return (0);
+}
+
+static int
+_teardown(void **state) {
+	UNUSED(state);
+
+	isc_test_end();
+
+	return (0);
+}
+
+/* test stats */
+static void
+isc_stats_basic_test(void **state) {
+	isc_stats_t *stats = NULL;
+	isc_result_t result;
+
+	UNUSED(state);
+
+	result = isc_stats_create(test_mctx, &stats, 4);
+	assert_int_equal(result, ISC_R_SUCCESS);
+	assert_int_equal(isc_stats_ncounters(stats), 4);
+
+	/* Default all 0. */
+	for (int i = 0; i < isc_stats_ncounters(stats); i++) {
+		assert_int_equal(isc_stats_get_counter(stats, i), 0);
+	}
+
+	/* Test increment. */
+	for (int i = 0; i < isc_stats_ncounters(stats); i++) {
+		isc_stats_increment(stats, i);
+		assert_int_equal(isc_stats_get_counter(stats, i), 1);
+		isc_stats_increment(stats, i);
+		assert_int_equal(isc_stats_get_counter(stats, i), 2);
+	}
+
+	/* Test decrement. */
+	for (int i = 0; i < isc_stats_ncounters(stats); i++) {
+		isc_stats_decrement(stats, i);
+		assert_int_equal(isc_stats_get_counter(stats, i), 1);
+		isc_stats_decrement(stats, i);
+		assert_int_equal(isc_stats_get_counter(stats, i), 0);
+	}
+
+	/* Test set. */
+	for (int i = 0; i < isc_stats_ncounters(stats); i++) {
+		isc_stats_set(stats, i, i);
+		assert_int_equal(isc_stats_get_counter(stats, i), i);
+	}
+
+	/* Test update if greater. */
+	for (int i = 0; i < isc_stats_ncounters(stats); i++) {
+		isc_stats_update_if_greater(stats, i, i);
+		assert_int_equal(isc_stats_get_counter(stats, i), i);
+		isc_stats_update_if_greater(stats, i, i + 1);
+		assert_int_equal(isc_stats_get_counter(stats, i), i + 1);
+	}
+
+	isc_stats_detach(&stats);
+}
+
+int
+main(void) {
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test_setup_teardown(isc_stats_basic_test, _setup,
+						_teardown),
+	};
+
+	return (cmocka_run_group_tests(tests, NULL, NULL));
+}
+
+#else /* HAVE_CMOCKA */
+
+#include <stdio.h>
+
+int
+main(void) {
+	printf("1..0 # Skipped: cmocka not available\n");
+	return (SKIPPED_TEST_EXIT_CODE);
+}
+
+#endif /* if HAVE_CMOCKA */
diff --git a/util/copyrights b/util/copyrights
index 0f4dab432d..490b788e8a 100644
--- a/util/copyrights
+++ b/util/copyrights
@@ -1882,6 +1882,7 @@
 ./lib/isc/tests/siphash_test.c			C	2019,2020,2021
 ./lib/isc/tests/sockaddr_test.c			C	2012,2015,2016,2017,2018,2019,2020,2021
 ./lib/isc/tests/socket_test.c			C	2011,2012,2013,2014,2015,2016,2017,2018,2019,2020,2021
+./lib/isc/tests/stats_test.c			C	2021
 ./lib/isc/tests/symtab_test.c			C	2011,2012,2013,2016,2018,2019,2020,2021
 ./lib/isc/tests/task_test.c			C	2011,2012,2016,2017,2018,2019,2020,2021
 ./lib/isc/tests/taskpool_test.c			C	2011,2012,2016,2018,2019,2020,2021

From 9acce8a82abec4b88630c1d05a18a44ebb051ecb Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Thu, 19 Aug 2021 13:38:51 +0200
Subject: [PATCH 2/7] Add a function isc_stats_resize

Add a new function to resize the number of counters in a statistics
counter structure. This will be needed when we keep track of DNSSEC
sign statistics and new keys are introduced due to a rollover.
---
 lib/isc/include/isc/stats.h | 11 +++++++++++
 lib/isc/stats.c             | 32 ++++++++++++++++++++++++++++++++
 lib/isc/tests/stats_test.c  | 17 +++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/lib/isc/include/isc/stats.h b/lib/isc/include/isc/stats.h
index c2ab4e6c03..f57aa1a96f 100644
--- a/lib/isc/include/isc/stats.h
+++ b/lib/isc/include/isc/stats.h
@@ -238,6 +238,17 @@ isc_stats_get_counter(isc_stats_t *stats, isc_statscounter_t counter);
  *	on creation.
  */
 
+void
+isc_stats_resize(isc_stats_t **stats, int ncounters);
+/*%<
+ * Resize a statistics counter structure of general type. The new set of
+ * counters are indexed by an ID between 0 and ncounters -1.
+ *
+ * Requires:
+ *\li	'stats' is a valid isc_stats_t.
+ *\li	'ncounters' is a non-zero positive number.
+ */
+
 ISC_LANG_ENDDECLS
 
 #endif /* ISC_STATS_H */
diff --git a/lib/isc/stats.c b/lib/isc/stats.c
index ab599b18af..6297946cb2 100644
--- a/lib/isc/stats.c
+++ b/lib/isc/stats.c
@@ -162,3 +162,35 @@ isc_stats_get_counter(isc_stats_t *stats, isc_statscounter_t counter) {
 
 	return (atomic_load_acquire(&stats->counters[counter]));
 }
+
+void
+isc_stats_resize(isc_stats_t **statsp, int ncounters) {
+	isc_stats_t *stats;
+	size_t counters_alloc_size;
+	isc__atomic_statcounter_t *newcounters;
+
+	REQUIRE(statsp != NULL && *statsp != NULL);
+	REQUIRE(ISC_STATS_VALID(*statsp));
+	REQUIRE(ncounters > 0);
+
+	stats = *statsp;
+	if (stats->ncounters >= ncounters) {
+		/* We already have enough counters. */
+		return;
+	}
+
+	/* Grow number of counters. */
+	counters_alloc_size = sizeof(isc__atomic_statcounter_t) * ncounters;
+	newcounters = isc_mem_get(stats->mctx, counters_alloc_size);
+	for (int i = 0; i < ncounters; i++) {
+		atomic_init(&newcounters[i], 0);
+	}
+	for (int i = 0; i < stats->ncounters; i++) {
+		uint32_t counter = atomic_load_acquire(&stats->counters[i]);
+		atomic_store_release(&newcounters[i], counter);
+	}
+	isc_mem_put(stats->mctx, stats->counters,
+		    sizeof(isc__atomic_statcounter_t) * stats->ncounters);
+	stats->counters = newcounters;
+	stats->ncounters = ncounters;
+}
diff --git a/lib/isc/tests/stats_test.c b/lib/isc/tests/stats_test.c
index a212613143..7b7476d3ad 100644
--- a/lib/isc/tests/stats_test.c
+++ b/lib/isc/tests/stats_test.c
@@ -96,6 +96,23 @@ isc_stats_basic_test(void **state) {
 		assert_int_equal(isc_stats_get_counter(stats, i), i + 1);
 	}
 
+	/* Test resize. */
+	isc_stats_resize(&stats, 3);
+	assert_int_equal(isc_stats_ncounters(stats), 4);
+	isc_stats_resize(&stats, 4);
+	assert_int_equal(isc_stats_ncounters(stats), 4);
+	isc_stats_resize(&stats, 5);
+	assert_int_equal(isc_stats_ncounters(stats), 5);
+
+	/* Existing counters are retained */
+	for (int i = 0; i < isc_stats_ncounters(stats); i++) {
+		uint32_t expect = i + 1;
+		if (i == 4) {
+			expect = 0;
+		}
+		assert_int_equal(isc_stats_get_counter(stats, i), expect);
+	}
+
 	isc_stats_detach(&stats);
 }
 

From d9cca81d508ae0f84974b89ce48af17d5096186d Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 20 Aug 2021 11:14:49 +0200
Subject: [PATCH 3/7] Grow dnssec-sign statistics instead of rotating

We have introduced dnssec-sign statistics to the zone statistics. This
introduced an operational issue because when using zone-statistics
full, the memory usage was going through the roof. We fixed this by
by allocating just four key slots per zone. If a zone exceeds the
number of keys for example through a key rollover, the keys will be
rotated out on a FIFO basis.

This works for most cases, and fixes the immediate problem of high
memory usage, but if you sign your zone with many, many keys, or are
sign with a ZSK/KSK double algorithm strategy you may experience weird
statistics. A better strategy is to grow the number of key slots per
zone on key rollover events.

That is what this commit is doing: instead of rotating the four slots
to track sign statistics, named now grows the number of key slots
during a key rollover (or via some other method that introduces new
keys).
---
 lib/dns/stats.c | 39 ++++++++++++++-------------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/lib/dns/stats.c b/lib/dns/stats.c
index 702b690b83..40127fd3c1 100644
--- a/lib/dns/stats.c
+++ b/lib/dns/stats.c
@@ -103,7 +103,7 @@ typedef enum {
  */
 
 /* Maximum number of keys to keep track of for DNSSEC signing statistics. */
-static int dnssecsign_max_keys = 4;
+static int dnssecsign_num_keys = 4;
 static int dnssecsign_block_size = 3;
 /* Key id mask */
 #define DNSSECSIGNSTATS_KEY_ID_MASK 0x0000FFFF
@@ -245,7 +245,8 @@ dns_dnssecsignstats_create(isc_mem_t *mctx, dns_stats_t **statsp) {
 	 * the actual counters for creating and refreshing signatures.
 	 */
 	return (create_stats(mctx, dns_statstype_dnssec,
-			     dnssecsign_max_keys * 3, statsp));
+			     dnssecsign_num_keys * dnssecsign_block_size,
+			     statsp));
 }
 
 /*%
@@ -361,6 +362,8 @@ void
 dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg,
 			      dnssecsignstats_type_t operation) {
 	uint32_t kval;
+	int num_keys = isc_stats_ncounters(stats->counters) /
+		       dnssecsign_block_size;
 
 	REQUIRE(DNS_STATS_VALID(stats) && stats->type == dns_statstype_dnssec);
 
@@ -368,7 +371,7 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg,
 	kval = (uint32_t)(alg << 16 | id);
 
 	/* Look up correct counter. */
-	for (int i = 0; i < dnssecsign_max_keys; i++) {
+	for (int i = 0; i < num_keys; i++) {
 		int idx = i * dnssecsign_block_size;
 		uint32_t counter = isc_stats_get_counter(stats->counters, idx);
 		if (counter == kval) {
@@ -379,7 +382,7 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg,
 	}
 
 	/* No match found. Store key in unused slot. */
-	for (int i = 0; i < dnssecsign_max_keys; i++) {
+	for (int i = 0; i < num_keys; i++) {
 		int idx = i * dnssecsign_block_size;
 		uint32_t counter = isc_stats_get_counter(stats->counters, idx);
 		if (counter == 0) {
@@ -389,27 +392,12 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg,
 		}
 	}
 
-	/* No room, rotate keys. */
-	for (int i = 1; i < dnssecsign_max_keys; i++) {
-		int gidx = i * dnssecsign_block_size; /* Get key (get index,
-							 gidx) */
-		uint32_t keyv = isc_stats_get_counter(stats->counters, gidx);
-		uint32_t sign = isc_stats_get_counter(
-			stats->counters, (gidx + dns_dnssecsignstats_sign));
-		uint32_t refr = isc_stats_get_counter(
-			stats->counters, (gidx + dns_dnssecsignstats_refresh));
-
-		int sidx = (i - 1) * dnssecsign_block_size; /* Set key, (set
-							       index, sidx) */
-		isc_stats_set(stats->counters, keyv, sidx);
-		isc_stats_set(stats->counters, sign,
-			      (sidx + dns_dnssecsignstats_sign));
-		isc_stats_set(stats->counters, refr,
-			      (sidx + dns_dnssecsignstats_refresh));
-	}
+	/* No room, grow stats storage. */
+	isc_stats_resize(&stats->counters,
+			 (num_keys * dnssecsign_block_size * 2));
 
 	/* Reset counters for new key (new index, nidx). */
-	int nidx = (dnssecsign_max_keys - 1) * dnssecsign_block_size;
+	int nidx = num_keys * dnssecsign_block_size;
 	isc_stats_set(stats->counters, kval, nidx);
 	isc_stats_set(stats->counters, 0, (nidx + dns_dnssecsignstats_sign));
 	isc_stats_set(stats->counters, 0, (nidx + dns_dnssecsignstats_refresh));
@@ -525,9 +513,10 @@ dnssec_dumpcb(isc_statscounter_t counter, uint64_t value, void *arg) {
 static void
 dnssec_statsdump(isc_stats_t *stats, dnssecsignstats_type_t operation,
 		 isc_stats_dumper_t dump_fn, void *arg, unsigned int options) {
-	int i;
+	int i, num_keys;
 
-	for (i = 0; i < dnssecsign_max_keys; i++) {
+	num_keys = isc_stats_ncounters(stats) / dnssecsign_block_size;
+	for (i = 0; i < num_keys; i++) {
 		int idx = dnssecsign_block_size * i;
 		uint32_t kval, val;
 		dns_keytag_t id;

From 019a52a1844c843b9e5dff2f7a2f5adea8400c2f Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 20 Aug 2021 11:19:28 +0200
Subject: [PATCH 4/7] Add back the statschannel manykeys test case

Add a test case that has more than four keys (the initial number of
key slots that are created for dnssec-sign statistics). We shouldn't
be expecting weird values.

This fixes some errors in the manykeys zone configuration (keys
were created for algorithm RSASHA256, but the policy expected RSASHA1,
and the zone was not allowing dynamic updates).

This also fixes an error in the calls to 'zones-json.pl': The perl
script excepts an index number where the zone can be found, rather
than the zone name.
---
 .../system/statschannel/ns2/named.conf.in     |  9 +-
 bin/tests/system/statschannel/tests.sh        | 96 ++++++++++++++++---
 bin/tests/system/statschannel/zones-json.pl   |  1 -
 3 files changed, 89 insertions(+), 17 deletions(-)

diff --git a/bin/tests/system/statschannel/ns2/named.conf.in b/bin/tests/system/statschannel/ns2/named.conf.in
index 68367989cc..c4ec68db3f 100644
--- a/bin/tests/system/statschannel/ns2/named.conf.in
+++ b/bin/tests/system/statschannel/ns2/named.conf.in
@@ -36,8 +36,8 @@ controls {
 
 dnssec-policy "manykeys" {
 	keys {
-		ksk lifetime unlimited algorithm 5;
-		zsk lifetime unlimited algorithm 5;
+		ksk lifetime unlimited algorithm 8;
+		zsk lifetime unlimited algorithm 8;
 		ksk lifetime unlimited algorithm 13;
 		zsk lifetime unlimited algorithm 13;
 		ksk lifetime unlimited algorithm 14;
@@ -62,8 +62,9 @@ zone "dnssec" {
 };
 
 zone "manykeys" {
-        type primary;
-        file "manykeys.db.signed";
+	type primary;
+	file "manykeys.db.signed";
+	allow-update { any; };
 	zone-statistics full;
 	dnssec-policy "manykeys";
 };
diff --git a/bin/tests/system/statschannel/tests.sh b/bin/tests/system/statschannel/tests.sh
index ad2b6152ce..552ac0b738 100644
--- a/bin/tests/system/statschannel/tests.sh
+++ b/bin/tests/system/statschannel/tests.sh
@@ -184,14 +184,11 @@ refresh_prefix="dnssec-refresh operations"
 ksk_id=`cat ns2/$zone.ksk.id`
 zsk_id=`cat ns2/$zone.zsk.id`
 
-# 1. Test sign operations for scheduled resigning.
+# Test sign operations for scheduled resigning.
 ret=0
 # The dnssec zone has 10 RRsets to sign (including NSEC) with the ZSK and one
 # RRset (DNSKEY) with the KSK. So starting named with signatures that expire
 # almost right away, this should trigger 10 zsk and 1 ksk sign operations.
-# However, the DNSSEC maintenance assumes when we see the SOA record we have
-# walked the whole zone, since the SOA record should always have the most
-# recent signature.
 echo "${refresh_prefix} ${zsk_id}: 10" > zones.expect
 echo "${refresh_prefix} ${ksk_id}: 1" >> zones.expect
 echo "${sign_prefix} ${zsk_id}: 10" >> zones.expect
@@ -199,20 +196,20 @@ echo "${sign_prefix} ${ksk_id}: 1" >> zones.expect
 cat zones.expect | sort > zones.expect.$n
 rm -f zones.expect
 # Fetch and check the dnssec sign statistics.
-echo_i "fetching zone stats data after zone maintenance at startup ($n)"
+echo_i "fetching zone '$zone' stats data after zone maintenance at startup ($n)"
 if [ $PERL_XML ]; then
     getzones xml $zone x$n || ret=1
     cmp zones.out.x$n zones.expect.$n || ret=1
 fi
 if [ $PERL_JSON ]; then
-    getzones json $zone j$n || ret=1
+    getzones json 0 j$n || ret=1
     cmp zones.out.j$n zones.expect.$n || ret=1
 fi
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 n=`expr $n + 1`
 
-# 2. Test sign operations after dynamic update.
+# Test sign operations after dynamic update.
 ret=0
 (
 # Update dnssec zone to trigger signature creation.
@@ -229,22 +226,22 @@ echo "${sign_prefix} ${ksk_id}: 1" >> zones.expect
 cat zones.expect | sort > zones.expect.$n
 rm -f zones.expect
 # Fetch and check the dnssec sign statistics.
-echo_i "fetching zone stats data after dynamic update ($n)"
+echo_i "fetching zone '$zone' stats data after dynamic update ($n)"
 if [ $PERL_XML ]; then
     getzones xml $zone x$n || ret=1
     cmp zones.out.x$n zones.expect.$n || ret=1
 fi
 if [ $PERL_JSON ]; then
-    getzones json $zone j$n || ret=1
+    getzones json 0 j$n || ret=1
     cmp zones.out.j$n zones.expect.$n || ret=1
 fi
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 n=`expr $n + 1`
 
-# 3. Test sign operations of KSK.
+# Test sign operations of KSK.
 ret=0
-echo_i "fetch zone stats data after updating DNSKEY RRset ($n)"
+echo_i "fetch zone '$zone' stats data after updating DNSKEY RRset ($n)"
 # Add a standby DNSKEY, this triggers resigning the DNSKEY RRset.
 zsk=$("$KEYGEN" -K ns2 -q -a "$DEFAULT_ALGORITHM" -b "$DEFAULT_BITS" "$zone")
 $SETTIME -K ns2 -P now -A never $zsk.key > /dev/null
@@ -262,13 +259,88 @@ if [ $PERL_XML ]; then
     cmp zones.out.x$n zones.expect.$n || ret=1
 fi
 if [ $PERL_JSON ]; then
-    getzones json $zone j$n || ret=1
+    getzones json 0 j$n || ret=1
     cmp zones.out.j$n zones.expect.$n || ret=1
 fi
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 n=`expr $n + 1`
 
+# Test sign operations for scheduled resigning (many keys).
+ret=0
+zone="manykeys"
+ksk8_id=`cat ns2/$zone.ksk8.id`
+zsk8_id=`cat ns2/$zone.zsk8.id`
+ksk13_id=`cat ns2/$zone.ksk13.id`
+zsk13_id=`cat ns2/$zone.zsk13.id`
+ksk14_id=`cat ns2/$zone.ksk14.id`
+zsk14_id=`cat ns2/$zone.zsk14.id`
+# The dnssec zone has 10 RRsets to sign (including NSEC) with the ZSKs and one
+# RRset (DNSKEY) with the KSKs. So starting named with signatures that expire
+# almost right away, this should trigger 10 zsk and 1 ksk sign operations per
+# key.
+echo "${refresh_prefix} ${zsk8_id}: 10" > zones.expect
+echo "${refresh_prefix} ${zsk13_id}: 10" >> zones.expect
+echo "${refresh_prefix} ${zsk14_id}: 10" >> zones.expect
+echo "${refresh_prefix} ${ksk8_id}: 1" >> zones.expect
+echo "${refresh_prefix} ${ksk13_id}: 1" >> zones.expect
+echo "${refresh_prefix} ${ksk14_id}: 1" >> zones.expect
+echo "${sign_prefix} ${zsk8_id}: 10" >> zones.expect
+echo "${sign_prefix} ${zsk13_id}: 10" >> zones.expect
+echo "${sign_prefix} ${zsk14_id}: 10" >> zones.expect
+echo "${sign_prefix} ${ksk8_id}: 1" >> zones.expect
+echo "${sign_prefix} ${ksk13_id}: 1" >> zones.expect
+echo "${sign_prefix} ${ksk14_id}: 1" >> zones.expect
+cat zones.expect | sort > zones.expect.$n
+rm -f zones.expect
+# Fetch and check the dnssec sign statistics.
+echo_i "fetching zone '$zone' stats data after zone maintenance at startup ($n)"
+if [ $PERL_XML ]; then
+    getzones xml $zone x$n || ret=1
+    cmp zones.out.x$n zones.expect.$n || ret=1
+fi
+if [ $PERL_JSON ]; then
+    getzones json 2 j$n || ret=1
+    cmp zones.out.j$n zones.expect.$n || ret=1
+fi
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=`expr $status + $ret`
+n=`expr $n + 1`
+
+# Test sign operations after dynamic update (many keys).
+ret=0
+(
+# Update dnssec zone to trigger signature creation.
+echo zone $zone
+echo server 10.53.0.2 "$PORT"
+echo update add $zone. 300 in txt "nsupdate added me"
+echo send
+) | $NSUPDATE
+# This should trigger the resign of SOA, TXT and NSEC (+3 zsk).
+echo "${refresh_prefix} ${zsk8_id}: 10" > zones.expect
+echo "${refresh_prefix} ${zsk13_id}: 10" >> zones.expect
+echo "${refresh_prefix} ${zsk14_id}: 10" >> zones.expect
+echo "${refresh_prefix} ${ksk8_id}: 1" >> zones.expect
+echo "${refresh_prefix} ${ksk13_id}: 1" >> zones.expect
+echo "${refresh_prefix} ${ksk14_id}: 1" >> zones.expect
+echo "${sign_prefix} ${zsk8_id}: 13" >> zones.expect
+echo "${sign_prefix} ${zsk13_id}: 13" >> zones.expect
+echo "${sign_prefix} ${zsk14_id}: 13" >> zones.expect
+echo "${sign_prefix} ${ksk8_id}: 1" >> zones.expect
+echo "${sign_prefix} ${ksk13_id}: 1" >> zones.expect
+echo "${sign_prefix} ${ksk14_id}: 1" >> zones.expect
+cat zones.expect | sort > zones.expect.$n
+rm -f zones.expect
+# Fetch and check the dnssec sign statistics.
+echo_i "fetching zone '$zone' stats data after dynamic update ($n)"
+if [ $PERL_XML ]; then
+    getzones xml $zone x$n || ret=1
+    cmp zones.out.x$n zones.expect.$n || ret=1
+fi
+if [ $PERL_JSON ]; then
+    getzones json 2 j$n || ret=1
+    cmp zones.out.j$n zones.expect.$n || ret=1
+fi
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 n=`expr $n + 1`
diff --git a/bin/tests/system/statschannel/zones-json.pl b/bin/tests/system/statschannel/zones-json.pl
index 9ccaf0eaf8..5da65324a2 100644
--- a/bin/tests/system/statschannel/zones-json.pl
+++ b/bin/tests/system/statschannel/zones-json.pl
@@ -23,7 +23,6 @@ close(INPUT);
 
 my $ref = decode_json($text);
 
-
 my $dnssecsign = $ref->{views}->{_default}->{zones}[$zone]->{"dnssec-sign"};
 my $type = "dnssec-sign operations ";
 foreach $key (keys %{$dnssecsign}) {

From de15e07800f695ac2a344463cbe56fe9c189f95d Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 20 Aug 2021 15:06:13 +0200
Subject: [PATCH 5/7] Clear dnssec-sign stats for removed keys

Clear the key slots for dnssec-sign statistics for keys that are
removed. This way, the number of slots will stabilize to the maximum
key usage in a zone and will not grow every time a key rollover is
triggered.
---
 lib/dns/include/dns/stats.h | 13 +++++++++++--
 lib/dns/stats.c             | 27 +++++++++++++++++++++++++++
 lib/dns/zone.c              | 18 ++++++++++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/lib/dns/include/dns/stats.h b/lib/dns/include/dns/stats.h
index 8a2833a7f1..8c973fe5e0 100644
--- a/lib/dns/include/dns/stats.h
+++ b/lib/dns/include/dns/stats.h
@@ -698,8 +698,17 @@ void
 dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg,
 			      dnssecsignstats_type_t operation);
 /*%<
- * Increment the statistics counter for the DNSKEY 'id'. The 'operation'
- * determines what counter is incremented.
+ * Increment the statistics counter for the DNSKEY 'id' with algorithm 'alg'.
+ * The 'operation' determines what counter is incremented.
+ *
+ * Requires:
+ *\li	'stats' is a valid dns_stats_t created by dns_dnssecsignstats_create().
+ */
+
+void
+dns_dnssecsignstats_clear(dns_stats_t *stats, dns_keytag_t id, uint8_t alg);
+/*%<
+ * Clear the statistics counter for the DNSKEY 'id' with algorithm 'alg'.
  *
  * Requires:
  *\li	'stats' is a valid dns_stats_t created by dns_dnssecsignstats_create().
diff --git a/lib/dns/stats.c b/lib/dns/stats.c
index 40127fd3c1..b6d0e0c188 100644
--- a/lib/dns/stats.c
+++ b/lib/dns/stats.c
@@ -406,6 +406,33 @@ dns_dnssecsignstats_increment(dns_stats_t *stats, dns_keytag_t id, uint8_t alg,
 	isc_stats_increment(stats->counters, (nidx + operation));
 }
 
+void
+dns_dnssecsignstats_clear(dns_stats_t *stats, dns_keytag_t id, uint8_t alg) {
+	uint32_t kval;
+	int num_keys = isc_stats_ncounters(stats->counters) /
+		       dnssecsign_block_size;
+
+	REQUIRE(DNS_STATS_VALID(stats) && stats->type == dns_statstype_dnssec);
+
+	/* Shift algorithm in front of key tag, which is 16 bits */
+	kval = (uint32_t)(alg << 16 | id);
+
+	/* Look up correct counter. */
+	for (int i = 0; i < num_keys; i++) {
+		int idx = i * dnssecsign_block_size;
+		uint32_t counter = isc_stats_get_counter(stats->counters, idx);
+		if (counter == kval) {
+			/* Match */
+			isc_stats_set(stats->counters, 0, idx);
+			isc_stats_set(stats->counters, 0,
+				      (idx + dns_dnssecsignstats_sign));
+			isc_stats_set(stats->counters, 0,
+				      (idx + dns_dnssecsignstats_refresh));
+			return;
+		}
+	}
+}
+
 /*%
  * Dump methods
  */
diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index 77432951eb..bca0f4dce4 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -21775,6 +21775,8 @@ zone_rekey(dns_zone_t *zone) {
 
 	if (commit) {
 		dns_difftuple_t *tuple;
+		dns_stats_t *dnssecsignstats =
+			dns_zone_getdnssecsignstats(zone);
 
 		DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_NEEDNOTIFY);
 
@@ -21795,6 +21797,22 @@ zone_rekey(dns_zone_t *zone) {
 						   "%s",
 						   dns_result_totext(result));
 				}
+
+				/* Clear DNSSEC sign statistics. */
+				if (dnssecsignstats != NULL) {
+					dns_dnssecsignstats_clear(
+						dnssecsignstats,
+						dst_key_id(key->key),
+						dst_key_alg(key->key));
+					/*
+					 * Also clear the dnssec-sign
+					 * statistics of the revoked key id.
+					 */
+					dns_dnssecsignstats_clear(
+						dnssecsignstats,
+						dst_key_rid(key->key),
+						dst_key_alg(key->key));
+				}
 			}
 		}
 

From 1a3c82f765f622ed96f0b1e09923b94aed56b3d0 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 20 Aug 2021 15:08:29 +0200
Subject: [PATCH 6/7] Add statschannel test case for key removal

Add a statschannel test case to confirm that when keys are removed
(in this case because of a dnssec-policy change), the corresponding
dnssec-sign stats are cleared and are no longer shown in the
statistics.
---
 .../system/statschannel/ns2/named2.conf.in    | 66 +++++++++++++++++++
 bin/tests/system/statschannel/tests.sh        | 29 ++++++++
 2 files changed, 95 insertions(+)
 create mode 100644 bin/tests/system/statschannel/ns2/named2.conf.in

diff --git a/bin/tests/system/statschannel/ns2/named2.conf.in b/bin/tests/system/statschannel/ns2/named2.conf.in
new file mode 100644
index 0000000000..15c52d4972
--- /dev/null
+++ b/bin/tests/system/statschannel/ns2/named2.conf.in
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+options {
+	query-source address 10.53.0.2;
+	notify-source 10.53.0.2;
+	transfer-source 10.53.0.2;
+	port @PORT@;
+	pid-file "named.pid";
+	listen-on { 10.53.0.2; };
+	listen-on-v6 { none; };
+	recursion no;
+	notify no;
+	minimal-responses no;
+	version none;  // make statistics independent of the version number
+};
+
+statistics-channels { inet 10.53.0.2 port @EXTRAPORT1@ allow { localhost; }; };
+
+key rndc_key {
+	secret "1234abcd8765";
+	algorithm hmac-sha256;
+};
+
+controls {
+	inet 10.53.0.2 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
+};
+
+dnssec-policy "manykeys" {
+	keys {
+		ksk lifetime unlimited algorithm 8;
+		zsk lifetime unlimited algorithm 8;
+	};
+};
+
+zone "example" {
+	type primary;
+	file "example.db";
+	allow-transfer { any; };
+};
+
+zone "dnssec" {
+	type primary;
+	file "dnssec.db.signed";
+	auto-dnssec maintain;
+	allow-update { any; };
+	zone-statistics full;
+	dnssec-dnskey-kskonly yes;
+	update-check-ksk yes;
+};
+
+zone "manykeys" {
+	type primary;
+	file "manykeys.db.signed";
+	allow-update { any; };
+	zone-statistics full;
+	dnssec-policy "manykeys";
+};
diff --git a/bin/tests/system/statschannel/tests.sh b/bin/tests/system/statschannel/tests.sh
index 552ac0b738..bbbc8a0c97 100644
--- a/bin/tests/system/statschannel/tests.sh
+++ b/bin/tests/system/statschannel/tests.sh
@@ -345,5 +345,34 @@ if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 n=`expr $n + 1`
 
+# Test sign operations after dnssec-policy change (removing keys).
+ret=0
+copy_setports ns2/named2.conf.in ns2/named.conf
+$RNDCCMD 10.53.0.2 reload 2>&1 | sed 's/^/I:ns2 /'
+# This should trigger the resign of DNSKEY (+1 ksk), and SOA, NSEC,
+# TYPE65534 (+3 zsk). The dnssec-sign statistics for the removed keys should
+# be cleared and thus no longer visible. But NSEC and SOA are (mistakenly)
+# counted double, one time because of zone_resigninc and one time because of
+# zone_nsec3chain. So +5 zsk in total.
+echo "${refresh_prefix} ${zsk8_id}: 15" > zones.expect
+echo "${refresh_prefix} ${ksk8_id}: 2" >> zones.expect
+echo "${sign_prefix} ${zsk8_id}: 18" >> zones.expect
+echo "${sign_prefix} ${ksk8_id}: 2" >> zones.expect
+cat zones.expect | sort > zones.expect.$n
+rm -f zones.expect
+# Fetch and check the dnssec sign statistics.
+echo_i "fetching zone '$zone' stats data after dnssec-policy change ($n)"
+if [ $PERL_XML ]; then
+    getzones xml $zone x$n || ret=1
+    cmp zones.out.x$n zones.expect.$n || ret=1
+fi
+if [ $PERL_JSON ]; then
+    getzones json 2 j$n || ret=1
+    cmp zones.out.j$n zones.expect.$n || ret=1
+fi
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=`expr $status + $ret`
+n=`expr $n + 1`
+
 echo_i "exit status: $status"
 [ $status -eq 0 ] || exit 1

From 8224dc8e351b6d1b36eca66498402d822b986565 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 20 Aug 2021 15:10:42 +0200
Subject: [PATCH 7/7] Add CHANGES for [GL #1721]

---
 CHANGES | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/CHANGES b/CHANGES
index 469d17b45f..7f6b4d6821 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,6 @@
+5699.	[func]		Grow and shrink dnssec-sign statistics on key rollover
+			events. [GL #1721]
+
 5698.	[bug]		Migrate a single key to CSK when reconfiguring a zone
 			to use 'dnssec-policy'. [GL #2857]