From: Marek Vavrusa <marek@vavrusa.com>
Date: Fri, 6 May 2016 06:40:28 +0000 (-0700)
Subject: lib: cleanup servfail soft-fails
X-Git-Tag: v1.0.0~24^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7b679ecf2a80b38bc7aabcf86a3ad7e67b4894a2;p=thirdparty%2Fknot-resolver.git

lib: cleanup servfail soft-fails

* simplified soft-fail per-ns limit to per-query
  limit, each query gets 4 tries at resolving
* instead of locking at single servfailing NS,
  penalise it and run reelection, this may or
  may not try other servers but avoids pathologic
  case when single NS is servfailing while others
  are good but never probed
* added new nsrep update mode (addition)
---

diff --git a/lib/defines.h b/lib/defines.h
index 895a1b66b..a5f653e4e 100644
--- a/lib/defines.h
+++ b/lib/defines.h
@@ -55,10 +55,7 @@ static inline int __attribute__((__cold__)) kr_error(int x) {
 #define KR_ITER_LIMIT 50     /* Built-in iterator limit */
 #define KR_CNAME_CHAIN_LIMIT 40 /* Built-in maximum CNAME chain length */
 #define KR_TIMEOUT_LIMIT 4   /* Maximum number of retries after timeout. */
-#define KR_QUERY_FAIL_LIMIT 40 /* Limit to the number of SERVFAIL\REFUSED
-                                * responses per query
-                                */
-#define KR_QUERY_NSRETRY_LIMIT 3 /* Maximum number of retries for single ns */
+#define KR_QUERY_NSRETRY_LIMIT 4 /* Maximum number of retries per query. */
 
 /*
  * Defines.
diff --git a/lib/layer/iterate.c b/lib/layer/iterate.c
index 92209af28..99abc5483 100644
--- a/lib/layer/iterate.c
+++ b/lib/layer/iterate.c
@@ -601,12 +601,10 @@ static int resolve(knot_layer_t *ctx, knot_pkt_t *pkt)
 	case KNOT_RCODE_SERVFAIL: {
 		DEBUG_MSG("<= rcode: %s\n", rcode ? rcode->name : "??");
 		query->fails += 1;
-		query->ns.fails += 1;
-		if (query->fails >= KR_QUERY_FAIL_LIMIT ||
-		    query->ns.fails >= KR_QUERY_NSRETRY_LIMIT) {
+		if (query->fails >= KR_QUERY_NSRETRY_LIMIT) {
+			query->fails = 0; /* Reset per-query counter. */
 			return resolve_error(pkt, req);
 		} else {
-			query->flags |= QUERY_SERVFAIL;
 			return KNOT_STATE_CONSUME;
 		}
 	}
diff --git a/lib/nsrep.c b/lib/nsrep.c
index 0fad08163..e5c918443 100644
--- a/lib/nsrep.c
+++ b/lib/nsrep.c
@@ -24,6 +24,7 @@
 #include "lib/resolve.h"
 #include "lib/defines.h"
 #include "lib/generic/pack.h"
+#include "contrib/ucw/lib.h"
 
 /** Some built-in unfairness ... */
 #define FAVOUR_IPV6 20 /* 20ms bonus for v6 */
@@ -176,7 +177,6 @@ int kr_nsrep_set(struct kr_query *qry, uint8_t *addr, size_t addr_len)
 	qry->ns.name = (const uint8_t *)"";
 	qry->ns.score = KR_NS_UNKNOWN;
 	qry->ns.reputation = 0;
-	qry->ns.fails = 0;
 	update_nsrep(&qry->ns, 0, addr, addr_len);
 	update_nsrep(&qry->ns, 1, NULL, 0);
 	return kr_ok();
@@ -186,7 +186,6 @@ int kr_nsrep_set(struct kr_query *qry, uint8_t *addr, size_t addr_len)
 	(ns)->ctx = (ctx_); \
 	(ns)->addr[0].ip.sa_family = AF_UNSPEC; \
 	(ns)->reputation = 0; \
-	(ns)->fails = 0; \
 	(ns)->score = KR_NS_MAX_SCORE + 1; \
 } while (0)
 
@@ -252,15 +251,17 @@ int kr_nsrep_update_rtt(struct kr_nsrep *ns, const struct sockaddr *addr,
 	if (score <= KR_NS_GLUED) {
 		score = KR_NS_GLUED + 1;
 	}
-
-	if ((*cur != 0) && (umode == KR_NS_UPDATE)) {
-	/* In KR_NS_UPDATE mode, calculate smooth over last two measurements */
-		*cur = (*cur + score) / 2;
-	} else {
-	/* First measurement or KR_NS_RESET mode, reset */
-		*cur = score;
+	/* First update is always set. */
+	if (*cur == 0) {
+		umode = KR_NS_RESET;
+	}
+	/* Update score, by default smooth over last two measurements. */
+	switch (umode) {
+	case KR_NS_UPDATE: *cur = (*cur + score) / 2; break;
+	case KR_NS_RESET:  *cur = score; break;
+	case KR_NS_ADD:    *cur = MIN(KR_NS_MAX_SCORE - 1, *cur + score); break;
+	default: break;
 	}
-
 	return kr_ok();
 }
 
diff --git a/lib/nsrep.h b/lib/nsrep.h
index 70a73aac3..9f2e0b784 100644
--- a/lib/nsrep.h
+++ b/lib/nsrep.h
@@ -35,6 +35,7 @@ enum kr_ns_score {
 	KR_NS_TIMEOUT   = (95 * KR_NS_MAX_SCORE) / 100,
 	KR_NS_LONG      = (3 * KR_NS_TIMEOUT) / 4,
 	KR_NS_UNKNOWN   = KR_NS_TIMEOUT / 2,
+	KR_NS_PENALTY   = 100,
 	KR_NS_GLUED     = 10
 };
 
@@ -51,8 +52,9 @@ enum kr_ns_rep {
  * NS RTT update modes.
  */
 enum kr_ns_update_mode {
-	KR_NS_UPDATE  = 0, /**< Update as smooth over last two measurements */
-	KR_NS_RESET	   /**< Set to given value */
+	KR_NS_UPDATE = 0, /**< Update as smooth over last two measurements */
+	KR_NS_RESET,      /**< Set to given value */
+	KR_NS_ADD         /**< Increment current value */
 };
 
 /**
@@ -72,7 +74,6 @@ struct kr_nsrep
 {
 	unsigned score;                  /**< NS score */
 	unsigned reputation;             /**< NS reputation */
-	unsigned fails;             	 /**< NS fail counter */
 	const knot_dname_t *name;        /**< NS name */
 	struct kr_context *ctx;          /**< Resolution context */
 	union {
@@ -126,7 +127,7 @@ int kr_nsrep_elect_addr(struct kr_query *qry, struct kr_context *ctx);
  * @param  addr         chosen address (NULL for first)
  * @param  score        new score (i.e. RTT), see enum kr_ns_score
  * @param  cache        LRU cache
- * @param  umode        update mode (KR_NS_UPDATE or KR_NS_RESET)
+ * @param  umode        update mode (KR_NS_UPDATE or KR_NS_RESET or KR_NS_ADD)
  * @return              0 on success, error code on failure
  */
 KR_EXPORT
diff --git a/lib/resolve.c b/lib/resolve.c
index dd70be6b7..3782fcd84 100644
--- a/lib/resolve.c
+++ b/lib/resolve.c
@@ -464,8 +464,12 @@ int kr_resolve_consume(struct kr_request *request, const struct sockaddr *src, k
 					DEBUG_MSG(qry, "<= server: '%s' rtt: %ld ms\n", addr_str, time_diff(&qry->timestamp, &now));
 				}
 			}
-			if (!(qry->flags & QUERY_SERVFAIL)) {
+			/* Do not complete NS address resolution on soft-fail. */
+			const int rcode = knot_wire_get_rcode(packet->wire);
+			if (rcode != KNOT_RCODE_SERVFAIL && rcode != KNOT_RCODE_REFUSED) {
 				qry->flags &= ~(QUERY_AWAIT_IPV6|QUERY_AWAIT_IPV4);
+			} else { /* Penalize SERVFAILs. */
+				kr_nsrep_update_rtt(&qry->ns, src, KR_NS_PENALTY, ctx->cache_rtt, KR_NS_ADD);
 			}
 		/* Do not penalize validation timeouts. */
 		} else if (!(qry->flags & QUERY_DNSSEC_BOGUS)) {
@@ -708,9 +712,7 @@ ns_election:
 		return KNOT_STATE_FAIL;
 	}
 
-	if (qry->flags & QUERY_SERVFAIL) {
-		qry->flags &= ~QUERY_SERVFAIL;
-	} else if (qry->flags & (QUERY_AWAIT_IPV4|QUERY_AWAIT_IPV6)) {
+	if (qry->flags & (QUERY_AWAIT_IPV4|QUERY_AWAIT_IPV6)) {
 		kr_nsrep_elect_addr(qry, request->ctx);
 	} else if (!qry->ns.name || !(qry->flags & (QUERY_TCP|QUERY_STUB))) { /* Keep NS when requerying/stub. */
 		/* Root DNSKEY must be fetched from the hints to avoid chicken and egg problem. */
diff --git a/lib/rplan.h b/lib/rplan.h
index 3b2d965b7..d0e8e2f3f 100644
--- a/lib/rplan.h
+++ b/lib/rplan.h
@@ -47,7 +47,6 @@
 	X(DNSSEC_WEXPAND,  1 << 19) /**< Query response has wildcard expansion. */ \
 	X(PERMISSIVE,      1 << 20) /**< Permissive resolver mode. */ \
 	X(STRICT,          1 << 21) /**< Strict resolver mode. */ \
-	X(SERVFAIL,        1 << 22) /**< Query response is SERVFAIL or REFUSED. */
 
 /** Query flags */
 enum kr_query_flag {