Restrict value range for weight parameter, avoid overflowing and dropping queries...

diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc
index 1b1df5ab5c11ba148ae3a949b95d136c672d05ba..fb0d78378bead25d76e5c89c7b3b0e28ca1c07d5 100644 (file)
--- a/pdns/dnsdist-lua.cc
+++ b/pdns/dnsdist-lua.cc
@@ -228,7 +228,21 @@ void setupLuaConfig(bool client)
                        }
 
                        if(vars.count("weight")) {
-                         ret->weight=std::stoi(boost::get<string>(vars["weight"]));
+                         try {
+                           int weightVal=std::stoi(boost::get<string>(vars["weight"]));
+
+                           if(weightVal < 1) {
+                             errlog("Error creating new server: downstream weight value must be greater than 0.");
+                             return ret;
+                           }
+
+                           ret->weight=weightVal;
+                         }
+                         catch(std::exception& e) {
+                           // std::stoi will throw an exception if the string isn't in a value int range
+                           errlog("Error creating new server: downstream weight value must be between %s and %s", 1, std::numeric_limits<int>::max());
+                           return ret;
+                         }
                        }
 
                        if(vars.count("retries")) {

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index be1049dd80023e9d762bd95276c26023965d276b..44ec64f42c3b0ca4d3b64c41c99c6a93c5deb46d 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -669,10 +669,18 @@ shared_ptr<DownstreamState> leastOutstanding(const NumberedServerVector& servers
 shared_ptr<DownstreamState> valrandom(unsigned int val, const NumberedServerVector& servers, const DNSQuestion* dq)
 {
   vector<pair<int, shared_ptr<DownstreamState>>> poss;
-  int sum=0;
+  int sum = 0;
+  int max = std::numeric_limits<int>::max();
+
   for(auto& d : servers) {      // w=1, w=10 -> 1, 11
     if(d.second->isUp()) {
-      sum+=d.second->weight;
+      // Don't overflow sum when adding high weights
+      if(d.second->weight > max - sum) {
+        sum = max;
+      } else {
+        sum += d.second->weight;
+      }
+
       poss.push_back({sum, d.second});
     }
   }

diff --git a/pdns/dnsdistdist/docs/guides/serverselection.rst b/pdns/dnsdistdist/docs/guides/serverselection.rst
index f1f0c6d882d7ef7077db5d51fcdffda5c24a1cef..de6316edab5683a2832691d70debaedc10088d25 100644 (file)
--- a/pdns/dnsdistdist/docs/guides/serverselection.rst
+++ b/pdns/dnsdistdist/docs/guides/serverselection.rst
@@ -30,7 +30,7 @@ For now this is the only policy using the QPS limit.
 
 A further policy, ``wrandom`` assigns queries randomly, but based on the weight parameter passed to :func:`newServer`.
 
-For example, if two servers are available, the first one with a weigth of 2 and the second one with a weight of 1 (the default), the
+For example, if two servers are available, the first one with a weight of 2 and the second one with a weight of 1 (the default), the
 first one should get two thirds of the incoming queries and the second one the remaining third.
 
 ``whashed``

diff --git a/pdns/dnsdistdist/docs/reference/config.rst b/pdns/dnsdistdist/docs/reference/config.rst
index f1989ec576df0a9212f10e95225a0d7085cd1fdc..e552a4f01642b8af4b6a6a937b0dc10ee73bd5d0 100644 (file)
--- a/pdns/dnsdistdist/docs/reference/config.rst
+++ b/pdns/dnsdistdist/docs/reference/config.rst
@@ -294,7 +294,8 @@ Servers
       address="IP:PORT",     -- IP and PORT of the backend server (mandatory)
       qps=NUM,               -- Limit the number of queries per second to NUM, when using the `firstAvailable` policy
       order=NUM,             -- The order of this server, used by the `leastOustanding` and `firstAvailable` policies
-      weight=NUM,            -- The weight of this server, used by the `wrandom` and `whashed` policies
+      weight=NUM,            -- The weight of this server, used by the `wrandom` and `whashed` policies, default: 1
+                             -- Supported values are a minimum of 1, and a maximum of 2147483647.
       pool=STRING|{STRING},  -- The pools this server belongs to (unset or empty string means default pool) as a string or table of strings
       retries=NUM,           -- The number of TCP connection attempts to the backend, for a given query
       tcpConnectTimeout=NUM, -- The timeout (in seconds) of a TCP connection attempt

diff --git a/regression-tests.dnsdist/test_Routing.py b/regression-tests.dnsdist/test_Routing.py
index 48c81516f552d2b17a81898dac31693270b1cc74..a7c50e3497a29c0c78b3d0cd68a16163e0d11f34 100644 (file)
--- a/regression-tests.dnsdist/test_Routing.py
+++ b/regression-tests.dnsdist/test_Routing.py
@@ -1,4 +1,5 @@
 #!/usr/bin/env python
+import base64
 import threading
 import time
 import dns
@@ -139,7 +140,7 @@ class TestRoutingRoundRobinLB(DNSDistTest):
         """
         Routing: Round Robin
 
-        Send 100 A queries to "rr.routing.tests.powerdns.com.",
+        Send 10 A queries to "rr.routing.tests.powerdns.com.",
         check that dnsdist routes half of it to each backend.
         """
         numberOfQueries = 10
@@ -311,3 +312,188 @@ class TestRoutingNoServer(DNSDistTest):
 
         (_, receivedResponse) = self.sendTCPQuery(query, response=None, useQueue=False)
         self.assertEquals(receivedResponse, expectedResponse)
+
+
+class TestRoutingWRandom(DNSDistTest):
+
+    _testServer2Port = 5351
+    _config_params = ['_testServerPort', '_testServer2Port']
+    _config_template = """
+    setServerPolicy(wrandom)
+    s1 = newServer{address="127.0.0.1:%s", weight=1}
+    s1:setUp()
+    s2 = newServer{address="127.0.0.1:%s", weight=2}
+    s2:setUp()
+    """
+
+    @classmethod
+    def startResponders(cls):
+        print("Launching responders..")
+        cls._UDPResponder = threading.Thread(name='UDP Responder', target=cls.UDPResponder, args=[cls._testServerPort, cls._toResponderQueue, cls._fromResponderQueue])
+        cls._UDPResponder.setDaemon(True)
+        cls._UDPResponder.start()
+        cls._UDPResponder2 = threading.Thread(name='UDP Responder 2', target=cls.UDPResponder, args=[cls._testServer2Port, cls._toResponderQueue, cls._fromResponderQueue])
+        cls._UDPResponder2.setDaemon(True)
+        cls._UDPResponder2.start()
+
+        cls._TCPResponder = threading.Thread(name='TCP Responder', target=cls.TCPResponder, args=[cls._testServerPort, cls._toResponderQueue, cls._fromResponderQueue])
+        cls._TCPResponder.setDaemon(True)
+        cls._TCPResponder.start()
+
+        cls._TCPResponder2 = threading.Thread(name='TCP Responder 2', target=cls.TCPResponder, args=[cls._testServer2Port, cls._toResponderQueue, cls._fromResponderQueue])
+        cls._TCPResponder2.setDaemon(True)
+        cls._TCPResponder2.start()
+
+    def testWRandom(self):
+        """
+        Routing: WRandom
+
+        Send 100 A queries to "rr.routing.tests.powerdns.com.",
+        check that dnsdist routes less than half to one, more to the other.
+        """
+        numberOfQueries = 100
+        name = 'rr.routing.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+        response = dns.message.make_response(query)
+        rrset = dns.rrset.from_text(name,
+                                    60,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.A,
+                                    '192.0.2.1')
+        response.answer.append(rrset)
+
+        # the counter is shared for UDP and TCP,
+        # so we need to do UDP then TCP to have a clean count
+        for _ in range(numberOfQueries):
+            (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response)
+            receivedQuery.id = query.id
+            self.assertEquals(query, receivedQuery)
+            self.assertEquals(response, receivedResponse)
+
+        for _ in range(numberOfQueries):
+            (receivedQuery, receivedResponse) = self.sendTCPQuery(query, response)
+            receivedQuery.id = query.id
+            self.assertEquals(query, receivedQuery)
+            self.assertEquals(response, receivedResponse)
+
+        # The lower weight downstream should receive less than half the queries
+        self.assertTrue(self._responsesCounter['UDP Responder'] < numberOfQueries * 0.50)
+        self.assertTrue(self._responsesCounter['TCP Responder'] < numberOfQueries * 0.50)
+
+        # The higher weight downstream should receive more than half the queries
+        self.assertTrue(self._responsesCounter['UDP Responder 2'] > numberOfQueries * 0.50)
+        self.assertTrue(self._responsesCounter['TCP Responder 2'] > numberOfQueries * 0.50)
+
+
+class TestRoutingHighValueWRandom(DNSDistTest):
+
+    _testServer2Port = 5351
+    _consoleKey = DNSDistTest.generateConsoleKey()
+    _consoleKeyB64 = base64.b64encode(_consoleKey).decode('ascii')
+    _config_params = ['_consoleKeyB64', '_consolePort', '_testServerPort', '_testServer2Port']
+    _config_template = """
+    setKey("%s")
+    controlSocket("127.0.0.1:%s")
+    setServerPolicy(wrandom)
+    s1 = newServer{address="127.0.0.1:%s", weight=2000000000}
+    s1:setUp()
+    s2 = newServer{address="127.0.0.1:%s", weight=2000000000}
+    s2:setUp()
+    """
+
+    @classmethod
+    def startResponders(cls):
+        print("Launching responders..")
+        cls._UDPResponder = threading.Thread(name='UDP Responder', target=cls.UDPResponder, args=[cls._testServerPort, cls._toResponderQueue, cls._fromResponderQueue])
+        cls._UDPResponder.setDaemon(True)
+        cls._UDPResponder.start()
+        cls._UDPResponder2 = threading.Thread(name='UDP Responder 2', target=cls.UDPResponder, args=[cls._testServer2Port, cls._toResponderQueue, cls._fromResponderQueue])
+        cls._UDPResponder2.setDaemon(True)
+        cls._UDPResponder2.start()
+
+        cls._TCPResponder = threading.Thread(name='TCP Responder', target=cls.TCPResponder, args=[cls._testServerPort, cls._toResponderQueue, cls._fromResponderQueue])
+        cls._TCPResponder.setDaemon(True)
+        cls._TCPResponder.start()
+
+        cls._TCPResponder2 = threading.Thread(name='TCP Responder 2', target=cls.TCPResponder, args=[cls._testServer2Port, cls._toResponderQueue, cls._fromResponderQueue])
+        cls._TCPResponder2.setDaemon(True)
+        cls._TCPResponder2.start()
+
+    def testHighValueWRandom(self):
+        """
+        Routing: WRandom
+
+        Send 100 A queries to "rr.routing.tests.powerdns.com.",
+        check that dnsdist routes to each downstream, rather than failing with
+        no-policy.
+        """
+        numberOfQueries = 100
+        name = 'rr.routing.tests.powerdns.com.'
+        query = dns.message.make_query(name, 'A', 'IN')
+        response = dns.message.make_response(query)
+        rrset = dns.rrset.from_text(name,
+                                    60,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.A,
+                                    '192.0.2.1')
+        response.answer.append(rrset)
+
+        # the counter is shared for UDP and TCP,
+        # so we need to do UDP then TCP to have a clean count
+        for _ in range(numberOfQueries):
+            (receivedQuery, receivedResponse) = self.sendUDPQuery(query, response)
+            receivedQuery.id = query.id
+            self.assertEquals(query, receivedQuery)
+            self.assertEquals(response, receivedResponse)
+
+        for _ in range(numberOfQueries):
+            (receivedQuery, receivedResponse) = self.sendTCPQuery(query, response)
+            receivedQuery.id = query.id
+            self.assertEquals(query, receivedQuery)
+            self.assertEquals(response, receivedResponse)
+
+        stats = self.sendConsoleCommand("dumpStats()").split()
+        stats_dict = {}
+
+        # Map to a dict with every other element being the value to the previous one
+        for i, x in enumerate(stats):
+            if not i % 2:
+                stats_dict[x] = stats[i+1]
+
+        # There should be no queries getting "no-policy" responses
+        self.assertEquals(stats_dict['no-policy'], '0')
+
+        # Each downstream should receive some queries, but it will be unbalanced
+        # The first downstream will receive more than half the queries
+        self.assertTrue(self._responsesCounter['UDP Responder'] > numberOfQueries / 2)
+        self.assertTrue(self._responsesCounter['TCP Responder'] > numberOfQueries / 2)
+
+        # The second downstream will receive the remainder of the queries, more than 0
+        self.assertTrue(self._responsesCounter['UDP Responder 2'] > 0)
+        self.assertTrue(self._responsesCounter['TCP Responder 2'] > 0)
+        self.assertEquals(self._responsesCounter['UDP Responder 2'], numberOfQueries - self._responsesCounter['UDP Responder'])
+        self.assertEquals(self._responsesCounter['TCP Responder 2'], numberOfQueries - self._responsesCounter['TCP Responder'])
+
+
+class TestRoutingBadWeightWRandom(DNSDistTest):
+
+    _testServer2Port = 5351
+    _consoleKey = DNSDistTest.generateConsoleKey()
+    _consoleKeyB64 = base64.b64encode(_consoleKey).decode('ascii')
+    _config_params = ['_consoleKeyB64', '_consolePort', '_testServerPort', '_testServer2Port']
+    _config_template = """
+    setKey("%s")
+    controlSocket("127.0.0.1:%s")
+    setServerPolicy(wrandom)
+    s1 = newServer{address="127.0.0.1:%s", weight=-1}
+    s2 = newServer{address="127.0.0.1:%s", weight=2147483648}
+    """
+
+    def testBadWeightWRandom(self):
+        """
+        Routing: WRandom
+
+        Test that downstreams cannot be added with invalid weights.
+        """
+        # There should be no downstreams
+        self.assertTrue(self.sendConsoleCommand("getServer(0)").startswith("Error"))