Merge branch '431-refactor-code-preparing-a-delegation-response' into 'master'

Refactor code preparing a delegation response

Closes #431

See merge request isc-projects/bind9!549

CHANGES

@@ -1,3 +1,8 @@
+5006.	[cleanup]	Code preparing a delegation response was extracted from
+			query_delegation() and query_zone_delegation() into a
+			separate function in order to decrease code
+			duplication. [GL #431]
+
 5005.	[bug]		dnssec-verify, and dnssec-signzone at the verification
 			step, failed on some validly signed zones. [GL #442]
 

bin/tests/system/mirror/ns2/named.conf.in

@@ -36,7 +36,7 @@ zone "example" {
 
 zone "sub.example" {
 	type master;
-	file "sub.example.db.in";
+	file "sub.example.db.signed";
 };
 
 zone "initially-unavailable" {

bin/tests/system/mirror/ns2/sign.sh

@@ -14,7 +14,7 @@ SYSTEMTESTTOP=../..
 
 keys_to_trust=""
 
-for zonename in example initially-unavailable; do
+for zonename in sub.example example initially-unavailable; do
 	zone=$zonename
 	infile=$zonename.db.in
 	zonefile=$zonename.db
@@ -24,7 +24,7 @@ for zonename in example initially-unavailable; do
 
 	cat $infile $keyname1.key $keyname2.key > $zonefile
 
-	$SIGNER -P -o $zone $zonefile > /dev/null
+	$SIGNER -P -g -o $zone $zonefile > /dev/null
 done
 
 # Only add the key for "initially-unavailable" to the list of keys trusted by

bin/tests/system/mirror/tests.sh

@@ -204,6 +204,21 @@ grep "${UPDATED_SERIAL_GOOD}.*; serial" dig.out.ns3.test$n > /dev/null || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 
+n=`expr $n + 1`
+echo_i "checking delegations sourced from a mirror zone ($n)"
+ret=0
+$DIG $DIGOPTS @10.53.0.3 foo.example A +norec > dig.out.ns3.test$n 2>&1 || ret=1
+# Check response code and flags in the answer.
+grep "NOERROR" dig.out.ns3.test$n > /dev/null || ret=1
+grep "flags:.* ad" dig.out.ns3.test$n > /dev/null && ret=1
+# Check that a delegation containing a DS RRset and glue is present.
+grep "ANSWER: 0" dig.out.ns3.test$n > /dev/null || ret=1
+grep "example.*IN.*NS" dig.out.ns3.test$n > /dev/null || ret=1
+grep "example.*IN.*DS" dig.out.ns3.test$n > /dev/null || ret=1
+grep "ns2.example.*A.*10.53.0.2" dig.out.ns3.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=`expr $status + $ret`
+
 n=`expr $n + 1`
 echo_i "checking that resolution involving a mirror zone works as expected ($n)"
 ret=0
@@ -238,14 +253,16 @@ ret=0
 $DIG $DIGOPTS @10.53.0.3 sub.example. NS > dig.out.ns3.test$n.1 2>&1 || ret=1
 # Ensure the child-side NS RRset is returned.
 grep "NOERROR" dig.out.ns3.test$n.1 > /dev/null || ret=1
-grep "ANSWER: 1" dig.out.ns3.test$n.1 > /dev/null || ret=1
+grep "ANSWER: 2" dig.out.ns3.test$n.1 > /dev/null || ret=1
 grep "sub.example.*IN.*NS" dig.out.ns3.test$n.1 > /dev/null || ret=1
 # Issue a non-recursive query for something below the cached zone cut.
 $DIG $DIGOPTS @10.53.0.3 +norec foo.sub.example. A > dig.out.ns3.test$n.2 2>&1 || ret=1
-# Ensure the cached NS RRset is returned in a delegation.
+# Ensure the cached NS RRset is returned in a delegation, along with the
+# parent-side DS RRset.
 grep "NOERROR" dig.out.ns3.test$n.2 > /dev/null || ret=1
 grep "ANSWER: 0" dig.out.ns3.test$n.2 > /dev/null || ret=1
 grep "sub.example.*IN.*NS" dig.out.ns3.test$n.2 > /dev/null || ret=1
+grep "sub.example.*IN.*DS" dig.out.ns3.test$n.2 > /dev/null || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=`expr $status + $ret`
 

lib/ns/include/ns/query.h

@@ -101,7 +101,6 @@ struct ns_query {
 #define NS_QUERYATTR_PARTIALANSWER	0x0004
 #define NS_QUERYATTR_NAMEBUFUSED	0x0008
 #define NS_QUERYATTR_RECURSING		0x0010
-#define NS_QUERYATTR_CACHEGLUEOK	0x0020
 #define NS_QUERYATTR_QUERYOKVALID	0x0040
 #define NS_QUERYATTR_QUERYOK		0x0080
 #define NS_QUERYATTR_WANTRECURSION	0x0100
@@ -153,8 +152,9 @@ typedef struct query_ctx {
 	dns_dbnode_t *node;			/* DB node */
 
 	dns_db_t *zdb;				/* zone DB values, saved */
-	dns_name_t *zfname;			/* while searching cache */
-	dns_dbversion_t *zversion;		/* for a better answer */
+	dns_dbnode_t *znode;			/* while searching cache */
+	dns_name_t *zfname;			/* for a better answer */
+	dns_dbversion_t *zversion;
 	dns_rdataset_t *zrdataset;
 	dns_rdataset_t *zsigrdataset;
 

lib/ns/query.c

@@ -112,9 +112,6 @@ do { \
 /*% Recursing? */
 #define RECURSING(c)		(((c)->query.attributes & \
 				  NS_QUERYATTR_RECURSING) != 0)
-/*% Cache glue ok? */
-#define CACHEGLUEOK(c)		(((c)->query.attributes & \
-				  NS_QUERYATTR_CACHEGLUEOK) != 0)
 /*% Want Recursion? */
 #define WANTRECURSION(c)	(((c)->query.attributes & \
 				  NS_QUERYATTR_WANTRECURSION) != 0)
@@ -5012,6 +5009,7 @@ qctx_init(ns_client_t *client, dns_fetchevent_t *event,
 	qctx->zsigrdataset = NULL;
 	qctx->zversion = NULL;
 	qctx->node = NULL;
+	qctx->znode = NULL;
 	qctx->db = NULL;
 	qctx->zdb = NULL;
 	qctx->version = NULL;
@@ -5077,11 +5075,10 @@ qctx_freedata(query_ctx_t *qctx) {
 	}
 
 	if (qctx->zdb != NULL) {
+		query_putrdataset(qctx->client, &qctx->zsigrdataset);
 		query_putrdataset(qctx->client, &qctx->zrdataset);
-		if (qctx->zsigrdataset != NULL)
-			query_putrdataset(qctx->client, &qctx->zsigrdataset);
-		if (qctx->zfname != NULL)
-			query_releasename(qctx->client, &qctx->zfname);
+		query_releasename(qctx->client, &qctx->zfname);
+		dns_db_detachnode(qctx->zdb, &qctx->znode);
 		dns_db_detach(&qctx->zdb);
 	}
 
@@ -7728,6 +7725,54 @@ query_notfound(query_ctx_t *qctx) {
 	return (query_delegation(qctx));
 }
 
+/*%
+ * We have a delegation but recursion is not allowed, so return the delegation
+ * to the client.
+ */
+static isc_result_t
+query_prepare_delegation_response(query_ctx_t *qctx) {
+	dns_rdataset_t **sigrdatasetp = NULL;
+	isc_boolean_t detach = ISC_FALSE;
+
+	/*
+	 * qctx->fname could be released in query_addrrset(), so save a copy of
+	 * it here in case we need it.
+	 */
+	dns_fixedname_init(&qctx->dsname);
+	dns_name_copy(qctx->fname, dns_fixedname_name(&qctx->dsname), NULL);
+
+	/*
+	 * This is the best answer.
+	 */
+	qctx->client->query.isreferral = ISC_TRUE;
+
+	if (!dns_db_iscache(qctx->db) && qctx->client->query.gluedb == NULL) {
+		dns_db_attach(qctx->db, &qctx->client->query.gluedb);
+		detach = ISC_TRUE;
+	}
+
+	/*
+	 * We must ensure NOADDITIONAL is off, because the generation of
+	 * additional data is required in delegations.
+	 */
+	qctx->client->query.attributes &= ~NS_QUERYATTR_NOADDITIONAL;
+	if (WANTDNSSEC(qctx->client) && qctx->sigrdataset != NULL) {
+		sigrdatasetp = &qctx->sigrdataset;
+	}
+	query_addrrset(qctx->client, &qctx->fname, &qctx->rdataset,
+		       sigrdatasetp, qctx->dbuf, DNS_SECTION_AUTHORITY);
+	if (detach) {
+		dns_db_detach(&qctx->client->query.gluedb);
+	}
+
+	/*
+	 * Add a DS if needed.
+	 */
+	query_addds(qctx);
+
+	return (query_done(qctx));
+}
+
 /*%
  * Handle a delegation response from an authoritative lookup. This
  * may trigger additional lookups, e.g. from the cache database to
@@ -7737,8 +7782,6 @@ query_notfound(query_ctx_t *qctx) {
 static isc_result_t
 query_zone_delegation(query_ctx_t *qctx) {
 	isc_result_t result;
-	dns_rdataset_t **sigrdatasetp = NULL;
-	isc_boolean_t detach = ISC_FALSE;
 
 	/*
 	 * If the query type is DS, look to see if we are
@@ -7803,8 +7846,8 @@ query_zone_delegation(query_ctx_t *qctx) {
 		 * we'll restore these values there.
 		 */
 		query_keepname(qctx->client, qctx->fname, qctx->dbuf);
-		dns_db_detachnode(qctx->db, &qctx->node);
 		SAVE(qctx->zdb, qctx->db);
+		SAVE(qctx->znode, qctx->node);
 		SAVE(qctx->zfname, qctx->fname);
 		SAVE(qctx->zversion, qctx->version);
 		SAVE(qctx->zrdataset, qctx->rdataset);
@@ -7815,57 +7858,7 @@ query_zone_delegation(query_ctx_t *qctx) {
 		return (query_lookup(qctx));
 	}
 
-	/*
-	 * qctx->fname could be released in
-	 * query_addrrset(), so save a copy of it
-	 * here in case we need it
-	 */
-	dns_fixedname_init(&qctx->dsname);
-	dns_name_copy(qctx->fname, dns_fixedname_name(&qctx->dsname), NULL);
-
-	/*
-	 * If we don't have a cache, this is the best
-	 * answer.
-	 *
-	 * If the client is making a nonrecursive
-	 * query we always give out the authoritative
-	 * delegation.  This way even if we get
-	 * junk in our cache, we won't fail in our
-	 * role as the delegating authority if another
-	 * nameserver asks us about a delegated
-	 * subzone.
-	 *
-	 * We enable the retrieval of glue for this
-	 * database by setting client->query.gluedb.
-	 */
-	if (qctx->db != NULL && qctx->client->query.gluedb == NULL) {
-		dns_db_attach(qctx->db, &qctx->client->query.gluedb);
-		detach = ISC_TRUE;
-	}
-	qctx->client->query.isreferral = ISC_TRUE;
-	/*
-	 * We must ensure NOADDITIONAL is off,
-	 * because the generation of
-	 * additional data is required in
-	 * delegations.
-	 */
-	qctx->client->query.attributes &=
-		~NS_QUERYATTR_NOADDITIONAL;
-	if (WANTDNSSEC(qctx->client) && qctx->sigrdataset != NULL)
-		sigrdatasetp = &qctx->sigrdataset;
-	query_addrrset(qctx->client, &qctx->fname,
-		       &qctx->rdataset, sigrdatasetp,
-		       qctx->dbuf, DNS_SECTION_AUTHORITY);
-	if (detach) {
-		dns_db_detach(&qctx->client->query.gluedb);
-	}
-
-	/*
-	 * Add a DS if needed
-	 */
-	query_addds(qctx);
-
-	return (query_done(qctx));
+	return (query_prepare_delegation_response(qctx));
 }
 
 /*%
@@ -7879,8 +7872,6 @@ query_zone_delegation(query_ctx_t *qctx) {
 static isc_result_t
 query_delegation(query_ctx_t *qctx) {
 	isc_result_t result;
-	dns_rdataset_t **sigrdatasetp = NULL;
-	isc_boolean_t detach = ISC_FALSE;
 
 	qctx->authoritative = ISC_FALSE;
 
@@ -7923,16 +7914,14 @@ query_delegation(query_ctx_t *qctx) {
 					  &qctx->sigrdataset);
 		qctx->version = NULL;
 
+		dns_db_detachnode(qctx->db, &qctx->node);
+		dns_db_detach(&qctx->db);
+		RESTORE(qctx->db, qctx->zdb);
+		RESTORE(qctx->node, qctx->znode);
 		RESTORE(qctx->fname, qctx->zfname);
 		RESTORE(qctx->version, qctx->zversion);
 		RESTORE(qctx->rdataset, qctx->zrdataset);
 		RESTORE(qctx->sigrdataset, qctx->zsigrdataset);
-
-		/*
-		 * We don't clean up zdb here because we
-		 * may still need it.  It will get cleaned
-		 * up by the main cleanup code in query_done().
-		 */
 	}
 
 	if (RECURSIONOK(qctx->client)) {
@@ -7993,54 +7982,7 @@ query_delegation(query_ctx_t *qctx) {
 		return (query_done(qctx));
 	}
 
-	/*
-	 * We have a delegation but recursion is not
-	 * allowed, so return the delegation to the client.
-	 *
-	 * qctx->fname could be released in
-	 * query_addrrset(), so save a copy of it
-	 * here in case we need it
-	 */
-	dns_fixedname_init(&qctx->dsname);
-	dns_name_copy(qctx->fname, dns_fixedname_name(&qctx->dsname), NULL);
-
-	/*
-	 * This is the best answer.
-	 */
-	qctx->client->query.attributes |= NS_QUERYATTR_CACHEGLUEOK;
-	qctx->client->query.isreferral = ISC_TRUE;
-
-	if (qctx->zdb != NULL && qctx->client->query.gluedb == NULL &&
-	    !(qctx->zone != NULL && dns_zone_ismirror(qctx->zone)))
-	{
-		dns_db_attach(qctx->zdb, &qctx->client->query.gluedb);
-		detach = ISC_TRUE;
-	}
-
-	/*
-	 * We must ensure NOADDITIONAL is off,
-	 * because the generation of
-	 * additional data is required in
-	 * delegations.
-	 */
-	qctx->client->query.attributes &= ~NS_QUERYATTR_NOADDITIONAL;
-	if (WANTDNSSEC(qctx->client) && qctx->sigrdataset != NULL) {
-		sigrdatasetp = &qctx->sigrdataset;
-	}
-	query_addrrset(qctx->client, &qctx->fname,
-		       &qctx->rdataset, sigrdatasetp,
-		       qctx->dbuf, DNS_SECTION_AUTHORITY);
-	qctx->client->query.attributes &= ~NS_QUERYATTR_CACHEGLUEOK;
-	if (detach) {
-		dns_db_detach(&qctx->client->query.gluedb);
-	}
-
-	/*
-	 * Add a DS if needed
-	 */
-	query_addds(qctx);
-
-	return (query_done(qctx));
+	return (query_prepare_delegation_response(qctx));
 }
 
 /*%