dnsdist: Keep the TCP connection open on cache hit, generated answers

We used to close the TCP connection right away on cases where that did
not make sense:
- on a cache hit
- on a self-generated answer
- on a servfail answer caused by the lack of usable downstream servers

We still close the TCP connections on drops, dynamic blocks, lack of
usable downstream servers without `setServFailWhenNoServer()` set,
invalid queries, network errors..

diff --git a/pdns/dnsdist-tcp.cc b/pdns/dnsdist-tcp.cc
index e74ac4a398763d99b4fa14e85a6ca2993eaf7b3b..f37f4b9ea310b19a212718cb19c8453fc71554d4 100644 (file)
--- a/pdns/dnsdist-tcp.cc
+++ b/pdns/dnsdist-tcp.cc
@@ -272,8 +272,9 @@ void* tcpClientThread(int pipefd)
         ds = nullptr;
         outstanding = false;
 
-        if(!getNonBlockingMsgLen(ci.fd, &qlen, g_tcpRecvTimeout))
+        if(!getNonBlockingMsgLen(ci.fd, &qlen, g_tcpRecvTimeout)) {
           break;
+        }
 
         queriesCount++;
 
@@ -356,7 +357,7 @@ void* tcpClientThread(int pipefd)
 #endif
           sendResponseToClient(ci.fd, query, dq.len);
          g_stats.selfAnswered++;
-         goto drop;
+         continue;
        }
 
         std::shared_ptr<ServerPool> serverPool = getPool(*holders.pools, poolname);
@@ -401,7 +402,7 @@ void* tcpClientThread(int pipefd)
 #endif
             sendResponseToClient(ci.fd, cachedResponse, cachedResponseSize);
             g_stats.cacheHits++;
-            goto drop;
+            continue;
           }
           g_stats.cacheMisses++;
         }
@@ -420,6 +421,7 @@ void* tcpClientThread(int pipefd)
             }
 #endif
             sendResponseToClient(ci.fd, query, dq.len);
+            continue;
           }
 
           break;

diff --git a/regression-tests.dnsdist/test_TCPKeepAlive.py b/regression-tests.dnsdist/test_TCPKeepAlive.py
new file mode 100644 (file)
index 0000000..9f0f3fc
--- /dev/null
+++ b/regression-tests.dnsdist/test_TCPKeepAlive.py
@@ -0,0 +1,250 @@
+#!/usr/bin/env python
+import struct
+import time
+import dns
+from dnsdisttests import DNSDistTest
+
+class TestTCPKeepAlive(DNSDistTest):
+    """
+    These tests make sure that dnsdist keeps the TCP connection alive
+    in various cases, like cache hits, self-generated answer, and
+    that it doesn't in error cases (Drop, invalid queries...)
+    """
+
+    _tcpIdleTimeout = 20
+    _maxTCPQueriesPerConn = 99
+    _maxTCPConnsPerClient = 3
+    _maxTCPConnDuration = 99
+    _config_template = """
+    newServer{address="127.0.0.1:%s"}
+    setTCPRecvTimeout(%s)
+    setMaxTCPQueriesPerConnection(%s)
+    setMaxTCPConnectionsPerClient(%s)
+    setMaxTCPConnectionDuration(%s)
+    pc = newPacketCache(100, 86400, 1)
+    getPool(""):setCache(pc)
+    addAction("refused.tcpka.tests.powerdns.com.", RCodeAction(dnsdist.REFUSED))
+    addAction("dropped.tcpka.tests.powerdns.com.", DropAction())
+    addResponseAction("dropped-response.tcpka.tests.powerdns.com.", DropResponseAction())
+    -- create the pool named "nosuchpool"
+    getPool("nosuchpool")
+    addAction("nodownstream-servfail.tcpka.tests.powerdns.com.", PoolAction("nosuchpool"))
+    setServFailWhenNoServer(true)
+    """
+    _config_params = ['_testServerPort', '_tcpIdleTimeout', '_maxTCPQueriesPerConn', '_maxTCPConnsPerClient', '_maxTCPConnDuration']
+
+    def testTCPKaSelfGenerated(self):
+        """
+        TCP KeepAlive: Self-generated answer
+        """
+        name = 'refused.tcpka.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+        expectedResponse = dns.message.make_response(query)
+        expectedResponse.set_rcode(dns.rcode.REFUSED)
+
+        conn = self.openTCPConnection()
+
+        count = 0
+        for idx in xrange(5):
+            try:
+                self.sendTCPQueryOverConnection(conn, query)
+                response = self.recvTCPResponseOverConnection(conn)
+                if response is None:
+                    break
+                self.assertEquals(expectedResponse, response)
+                count = count + 1
+            except:
+                pass
+
+        conn.close()
+        self.assertEqual(count, 5)
+
+    def testTCPKaCacheHit(self):
+        """
+        TCP KeepAlive: Cache Hit
+        """
+        name = 'cachehit.tcpka.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+        expectedResponse = dns.message.make_response(query)
+        rrset = dns.rrset.from_text(name,
+                                    3600,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.A,
+                                    '192.0.2.1')
+        expectedResponse.answer.append(rrset)
+
+        # first query to fill the cache
+        (receivedQuery, receivedResponse) = self.sendTCPQuery(query, expectedResponse)
+        self.assertTrue(receivedQuery)
+        self.assertTrue(receivedResponse)
+        receivedQuery.id = query.id
+        self.assertEquals(query, receivedQuery)
+        self.assertEquals(receivedResponse, expectedResponse)
+
+        conn = self.openTCPConnection()
+
+        count = 0
+        for idx in xrange(5):
+            try:
+                self.sendTCPQueryOverConnection(conn, query)
+                response = self.recvTCPResponseOverConnection(conn)
+                if response is None:
+                    break
+                self.assertEquals(expectedResponse, response)
+                count = count + 1
+            except:
+                pass
+
+        conn.close()
+        self.assertEqual(count, 5)
+
+    def testTCPKaNoDownstreamServFail(self):
+        """
+        TCP KeepAlive: No downstream ServFail
+
+        The query is routed to a pool that has no server,
+        and dnsdist is configured to send a ServFail when
+        that happens. We should keep the TCP connection open.
+        """
+        name = 'nodownstream-servfail.tcpka.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+        expectedResponse = dns.message.make_response(query)
+        expectedResponse.set_rcode(dns.rcode.SERVFAIL)
+
+        conn = self.openTCPConnection()
+
+        count = 0
+        for idx in xrange(5):
+            try:
+                self.sendTCPQueryOverConnection(conn, query)
+                response = self.recvTCPResponseOverConnection(conn)
+                if response is None:
+                    break
+                self.assertEquals(expectedResponse, response)
+                count = count + 1
+            except:
+                pass
+
+        conn.close()
+        self.assertEqual(count, 5)
+
+    def testTCPKaQRBitSet(self):
+        """
+        TCP KeepAlive: QR bit set in question
+        """
+        name = 'qrset.tcpka.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+        query.flags |= dns.flags.QR
+
+        conn = self.openTCPConnection()
+
+        count = 0
+        for idx in xrange(5):
+            try:
+                self.sendTCPQueryOverConnection(conn, query)
+                response = self.recvTCPResponseOverConnection(conn)
+                if response is None:
+                    break
+                count = count + 1
+            except:
+                pass
+
+        conn.close()
+        self.assertEqual(count, 0)
+
+    def testTCPKaDrop(self):
+        """
+        TCP KeepAlive: Drop
+        """
+        name = 'dropped.tcpka.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+        query.flags |= dns.flags.QR
+
+        conn = self.openTCPConnection()
+
+        count = 0
+        for idx in xrange(5):
+            try:
+                self.sendTCPQueryOverConnection(conn, query)
+                response = self.recvTCPResponseOverConnection(conn)
+                if response is None:
+                    break
+                count = count + 1
+            except:
+                pass
+
+        conn.close()
+        self.assertEqual(count, 0)
+
+    def testTCPKaDropResponse(self):
+        """
+        TCP KeepAlive: Drop Response
+        """
+        name = 'dropped-response.tcpka.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+
+        conn = self.openTCPConnection()
+
+        count = 0
+        for idx in xrange(5):
+            try:
+                self.sendTCPQueryOverConnection(conn, query)
+                response = self.recvTCPResponseOverConnection(conn)
+                if response is None:
+                    break
+                count = count + 1
+            except:
+                pass
+
+        conn.close()
+        self.assertEqual(count, 0)
+
+class TestTCPKeepAliveNoDownstreamDrop(DNSDistTest):
+    """
+    This test makes sure that dnsdist drops the TCP connection
+    if no downstream server is available and setServFailWhenNoServer()
+    is not set.
+    """
+
+    _tcpIdleTimeout = 20
+    _maxTCPQueriesPerConn = 99
+    _maxTCPConnsPerClient = 3
+    _maxTCPConnDuration = 99
+    _config_template = """
+    newServer{address="127.0.0.1:%s"}
+    setTCPRecvTimeout(%s)
+    setMaxTCPQueriesPerConnection(%s)
+    setMaxTCPConnectionsPerClient(%s)
+    setMaxTCPConnectionDuration(%s)
+    -- create the pool named "nosuchpool"
+    getPool("nosuchpool")
+    addAction("nodownstream-drop.tcpka.tests.powerdns.com.", PoolAction("nosuchpool"))
+    """
+    _config_params = ['_testServerPort', '_tcpIdleTimeout', '_maxTCPQueriesPerConn', '_maxTCPConnsPerClient', '_maxTCPConnDuration']
+
+    def testTCPKaNoDownstreamDrop(self):
+        """
+        TCP KeepAlive: No downstream Drop
+
+        The query is routed to a pool that has no server,
+        and dnsdist is configured to drop the query when
+        that happens. We should close the TCP connection right away.
+        """
+        name = 'nodownstream-drop.tcpka.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+
+        conn = self.openTCPConnection()
+
+        count = 0
+        for idx in xrange(5):
+            try:
+                self.sendTCPQueryOverConnection(conn, query)
+                response = self.recvTCPResponseOverConnection(conn)
+                if response is None:
+                    break
+                count = count + 1
+            except:
+                pass
+
+        conn.close()
+        self.assertEqual(count, 0)