From 278403d319395c917360d6b9c84ef6c194dec920 Mon Sep 17 00:00:00 2001 From: Dan McCombs Date: Thu, 5 Apr 2018 09:53:34 -0400 Subject: [PATCH] Restrict value range for weight parameter, avoid overflowing and dropping queries if the sum of all weights is greater than the max value of int. --- pdns/dnsdist-lua.cc | 16 +- pdns/dnsdist.cc | 12 +- .../docs/guides/serverselection.rst | 2 +- pdns/dnsdistdist/docs/reference/config.rst | 3 +- regression-tests.dnsdist/test_Routing.py | 188 +++++++++++++++++- 5 files changed, 215 insertions(+), 6 deletions(-) diff --git a/pdns/dnsdist-lua.cc b/pdns/dnsdist-lua.cc index 1b1df5ab5..fb0d78378 100644 --- 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(vars["weight"])); + try { + int weightVal=std::stoi(boost::get(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::max()); + return ret; + } } if(vars.count("retries")) { diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc index be1049dd8..44ec64f42 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -669,10 +669,18 @@ shared_ptr leastOutstanding(const NumberedServerVector& servers shared_ptr valrandom(unsigned int val, const NumberedServerVector& servers, const DNSQuestion* dq) { vector>> poss; - int sum=0; + int sum = 0; + int max = std::numeric_limits::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 f1f0c6d88..de6316eda 100644 --- 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 f1989ec57..e552a4f01 100644 --- 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 48c81516f..a7c50e349 100644 --- 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")) -- 2.40.0