dnsdist: Add unit tests for server consistency

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>

diff --git a/pdns/dnsdistdist/Makefile.am b/pdns/dnsdistdist/Makefile.am
index a493f453a22bc892087a3da9539f6d8b7c2b4822..918dfa98d7774306642cf0d563d9bc5770e4de79 100644 (file)
--- a/pdns/dnsdistdist/Makefile.am
+++ b/pdns/dnsdistdist/Makefile.am
@@ -411,6 +411,7 @@ testrunner_SOURCES = \
        test-dnsdistpacketcache_cc.cc \
        test-dnsdistrings_cc.cc \
        test-dnsdistrules_cc.cc \
+       test-dnsdistserverpool.cc \
        test-dnsdistsvc_cc.cc \
        test-dnsdisttcp_cc.cc \
        test-dnsparser_cc.cc \

diff --git a/pdns/dnsdistdist/dnsdist-backend.cc b/pdns/dnsdistdist/dnsdist-backend.cc
index a04b36e3eb3b4f46147eab5ca7e02013f9751c45..9bd23b966e25f2fc0a6df32a6ee37d20dfe4efab 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-backend.cc
+++ b/pdns/dnsdistdist/dnsdist-backend.cc
@@ -1099,10 +1099,11 @@ void ServerPool::removeServer(shared_ptr<DownstreamState>& server)
 
 void ServerPool::updateConsistency()
 {
+  bool consistent{true};
   bool first{true};
   bool useECS{false};
   bool tcpOnly{false};
-  bool zeroScope{false};
+  bool zeroScope{true};
 
   for (const auto& serverPair : d_servers) {
     const auto& server = serverPair.second;
@@ -1113,23 +1114,30 @@ void ServerPool::updateConsistency()
       zeroScope = !server->d_config.disableZeroScope;
     }
     else {
-      if (server->d_config.useECS != useECS ||
-          server->isTCPOnly() != tcpOnly ||
-          server->d_config.disableZeroScope == zeroScope) {
-        d_tcpOnly = false;
-        d_isConsistent = false;
-        return;
+      if (consistent) {
+        if (server->d_config.useECS != useECS) {
+          consistent = false;
+        }
+        if (server->d_config.disableZeroScope == zeroScope) {
+          consistent = false;
+        }
+      }
+      if (server->isTCPOnly() != tcpOnly) {
+        consistent = false;
+        tcpOnly = false;
       }
     }
   }
 
   d_tcpOnly = tcpOnly;
-  /* at this point we know that all servers agree
-     on these settings, so let's just use the same
-     values for the pool itself */
-  d_useECS = useECS;
-  d_zeroScope = zeroScope;
-  d_isConsistent = true;
+  if (consistent) {
+    /* at this point we know that all servers agree
+       on these settings, so let's just use the same
+       values for the pool itself */
+    d_useECS = useECS;
+    d_zeroScope = zeroScope;
+  }
+  d_isConsistent = consistent;
 }
 
 void ServerPool::setZeroScope(bool enabled)

diff --git a/pdns/dnsdistdist/dnsdist-server-pool.hh b/pdns/dnsdistdist/dnsdist-server-pool.hh
index 2336e8e9bcb2760cf17f143e0d3afcdb24d40f73..78ba591aa90643410e4166a6f33163e17c2e0c78 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-server-pool.hh
+++ b/pdns/dnsdistdist/dnsdist-server-pool.hh
@@ -38,6 +38,10 @@ struct ServerPool
     return d_useECS;
   }
 
+  /* Note that the pool will do a consistency check,
+     and might decide to override the supplied value
+     if all backends in the pool have the same ECS
+     value and the value differs from the supplied one */
   void setECS(bool useECS);
 
   bool getZeroScope() const
@@ -45,6 +49,10 @@ struct ServerPool
     return d_zeroScope;
   }
 
+  /* Note that the pool will do a consistency check,
+     and might decide to override the supplied value
+     if all backends in the pool have the same disable zero scope setting
+     value and the value differs from the supplied one */
   void setZeroScope(bool enabled);
 
   bool isConsistent() const
@@ -52,6 +60,7 @@ struct ServerPool
     return d_isConsistent;
   }
 
+  /* sum of outstanding queries for all servers in this pool */
   size_t poolLoad() const;
   size_t countServers(bool upOnly) const;
   bool hasAtLeastOneServerAvailable() const;
@@ -60,6 +69,7 @@ struct ServerPool
   void removeServer(std::shared_ptr<DownstreamState>& server);
   bool isTCPOnly() const
   {
+    // coverity[missing_lock]
     return d_tcpOnly;
   }
 

diff --git a/pdns/dnsdistdist/meson.build b/pdns/dnsdistdist/meson.build
index 75ff2e05994b2294c7ba2dd59f07b27ca27bc483..9f13bec05db8b5a2246ecd35c8138e3d4b035d9c 100644 (file)
--- a/pdns/dnsdistdist/meson.build
+++ b/pdns/dnsdistdist/meson.build
@@ -537,6 +537,7 @@ test_sources += files(
   src_dir / 'test-dnsdistrings_cc.cc',
   src_dir / 'test-dnsdistrules_cc.cc',
   src_dir / 'test-dnsdistsvc_cc.cc',
+  src_dir / 'test-dnsdistserverpool.cc',
   src_dir / 'test-dnsdisttcp_cc.cc',
   src_dir / 'test-dnsparser_cc.cc',
   src_dir / 'test-iputils_hh.cc',

diff --git a/pdns/dnsdistdist/test-dnsdistserverpool.cc b/pdns/dnsdistdist/test-dnsdistserverpool.cc
new file mode 100644 (file)
index 0000000..07c4ad4
--- /dev/null
+++ b/pdns/dnsdistdist/test-dnsdistserverpool.cc
@@ -0,0 +1,191 @@
+/*
+ * This file is part of PowerDNS or dnsdist.
+ * Copyright -- PowerDNS.COM B.V. and its contributors
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * In addition, for the avoidance of any doubt, permission is granted to
+ * link this program with OpenSSL and to (re)distribute the binaries
+ * produced as the result of such linking.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef BOOST_TEST_DYN_LINK
+#define BOOST_TEST_DYN_LINK
+#endif
+#define BOOST_TEST_NO_MAIN
+
+#include <boost/test/unit_test.hpp>
+
+#include "dnsdist-server-pool.hh"
+#include "dnsdist.hh"
+
+BOOST_AUTO_TEST_SUITE(dnsdistserverpool)
+
+BOOST_AUTO_TEST_CASE(test_ServerPoolBasics)
+{
+  ServerPool pool{};
+  BOOST_CHECK(!pool.getCache());
+
+  BOOST_CHECK(!pool.getECS());
+  /* will be ignored because there is no server with ECS
+     in the pool at this point */
+  pool.setECS(true);
+  BOOST_CHECK(!pool.getECS());
+  pool.setECS(false);
+  BOOST_CHECK(!pool.getECS());
+
+  BOOST_CHECK(pool.getZeroScope());
+  /* will be ignored because there is no server with the
+     zero scope feature disabled in the pool at this point */
+  pool.setZeroScope(false);
+  BOOST_CHECK(pool.getZeroScope());
+  pool.setZeroScope(true);
+  BOOST_CHECK(pool.getZeroScope());
+
+  BOOST_CHECK(!pool.isTCPOnly());
+  BOOST_CHECK(pool.isConsistent());
+  BOOST_CHECK(!pool.hasAtLeastOneServerAvailable());
+  BOOST_CHECK_EQUAL(pool.countServers(true), 0U);
+  BOOST_CHECK_EQUAL(pool.countServers(false), 0U);
+  BOOST_CHECK_EQUAL(pool.poolLoad(), 0U);
+
+  {
+    const auto& servers = pool.getServers();
+    BOOST_CHECK(servers.empty());
+  }
+
+  {
+    /* add one server */
+    DownstreamState::Config config;
+    auto ds = std::make_shared<DownstreamState>(std::move(config), nullptr, false);
+    pool.addServer(ds);
+    BOOST_CHECK(!pool.isTCPOnly());
+    BOOST_CHECK(pool.isConsistent());
+    /* the server is unavailable by default */
+    BOOST_CHECK(!pool.hasAtLeastOneServerAvailable());
+    BOOST_CHECK_EQUAL(pool.countServers(true), 0U);
+    BOOST_CHECK_EQUAL(pool.countServers(false), 1U);
+
+    BOOST_CHECK_EQUAL(pool.poolLoad(), 0U);
+    {
+      const auto& servers = pool.getServers();
+      BOOST_CHECK(!servers.empty());
+    }
+
+    /* let's make it available */
+    ds->setUp();
+    BOOST_CHECK(pool.hasAtLeastOneServerAvailable());
+    BOOST_CHECK_EQUAL(pool.countServers(true), 1U);
+    BOOST_CHECK_EQUAL(pool.countServers(false), 1U);
+
+    /* now remove it */
+    pool.removeServer(ds);
+    BOOST_CHECK_EQUAL(pool.countServers(true), 0U);
+    BOOST_CHECK_EQUAL(pool.countServers(false), 0U);
+    {
+      const auto& servers = pool.getServers();
+      BOOST_CHECK(servers.empty());
+    }
+  }
+}
+
+BOOST_AUTO_TEST_CASE(test_ServerPoolECSConsistency)
+{
+  ServerPool pool{};
+
+  /* one server with ECS */
+  DownstreamState::Config config1;
+  config1.useECS = true;
+  auto ds1 = std::make_shared<DownstreamState>(std::move(config1), nullptr, false);
+  pool.addServer(ds1);
+  BOOST_CHECK_EQUAL(pool.countServers(false), 1U);
+  BOOST_CHECK(!pool.isTCPOnly());
+  BOOST_CHECK(pool.isConsistent());
+  BOOST_CHECK(pool.getECS());
+  /* should be ignored */
+  pool.setECS(false);
+  BOOST_CHECK(pool.getECS());
+
+  /* and now one without ECS */
+  DownstreamState::Config config2;
+  config2.useECS = false;
+  auto ds2 = std::make_shared<DownstreamState>(std::move(config2), nullptr, false);
+  pool.addServer(ds2);
+  BOOST_CHECK_EQUAL(pool.countServers(false), 2U);
+  BOOST_CHECK(!pool.isTCPOnly());
+  BOOST_CHECK(!pool.isConsistent());
+  /* the first backend has ECS so the pool automatically picked up
+     ECS-enabled, and after that the inconsistency meant the state
+     was not longer automatically updated */
+  BOOST_CHECK(pool.getECS());
+  /* should NOT be ignored */
+  pool.setECS(false);
+  BOOST_CHECK(!pool.getECS());
+  /* should NOT be ignored */
+  pool.setECS(true);
+  BOOST_CHECK(pool.getECS());
+
+  /* remove the server with ECS */
+  pool.removeServer(ds1);
+  BOOST_CHECK_EQUAL(pool.countServers(false), 1U);
+  BOOST_CHECK(!pool.isTCPOnly());
+  BOOST_CHECK(pool.isConsistent());
+  BOOST_CHECK(!pool.getECS());
+
+  /* re-add the server with ECS */
+  pool.addServer(ds1);
+  BOOST_CHECK_EQUAL(pool.countServers(false), 2U);
+  BOOST_CHECK(!pool.isTCPOnly());
+  BOOST_CHECK(!pool.isConsistent());
+  /* the pool was in a consistent state without ECS,
+     and now is in a inconsistent state so it not automatically
+     updated */
+  BOOST_CHECK(!pool.getECS());
+}
+
+BOOST_AUTO_TEST_CASE(test_ServerPoolTCPOnlyConsistency)
+{
+  ServerPool pool{};
+
+  DownstreamState::Config config1;
+  config1.d_tcpOnly = true;
+  auto ds1 = std::make_shared<DownstreamState>(std::move(config1), nullptr, false);
+  pool.addServer(ds1);
+  BOOST_CHECK_EQUAL(pool.countServers(false), 1U);
+  BOOST_CHECK(pool.isTCPOnly());
+  BOOST_CHECK(pool.isConsistent());
+  BOOST_CHECK(!pool.getECS());
+
+  DownstreamState::Config config2;
+  config2.d_tcpOnly = false;
+  auto ds2 = std::make_shared<DownstreamState>(std::move(config2), nullptr, false);
+  pool.addServer(ds2);
+  BOOST_CHECK_EQUAL(pool.countServers(false), 2U);
+  BOOST_CHECK(!pool.isTCPOnly());
+  BOOST_CHECK(!pool.isConsistent());
+
+  /* remove the TCP-only server */
+  pool.removeServer(ds1);
+  BOOST_CHECK_EQUAL(pool.countServers(false), 1U);
+  BOOST_CHECK(!pool.isTCPOnly());
+  BOOST_CHECK(pool.isConsistent());
+
+  /* re-add the server with TCP-only */
+  pool.addServer(ds1);
+  BOOST_CHECK_EQUAL(pool.countServers(false), 2U);
+  BOOST_CHECK(!pool.isTCPOnly());
+  BOOST_CHECK(!pool.isConsistent());
+}
+
+BOOST_AUTO_TEST_SUITE_END()