From f4dd166c31640af5151a85fda94be11a6d907dea Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Thu, 17 Jul 2025 17:10:04 +0200 Subject: [PATCH] dnsdist: Explicitly update the configuration thread-local copy This commits ensures that all DNSdist threads are regularly checking if there is a new version of the runtime-modifiable configuration and update their local copy if necessary. Regular accesses to the copy are now fully read-only, meaning they do not invalidate the current copy. It prevents the case where a function is invalidating the copy that the caller is holding, and makes accessing the configuration cheaper. Signed-off-by: Remi Gacogne (cherry picked from commit c08ba9f1f2fe743c3e05313b2c329446e2e2d1b1) --- pdns/dnsdistdist/dnsdist-async.cc | 2 ++ pdns/dnsdistdist/dnsdist-carbon.cc | 2 ++ pdns/dnsdistdist/dnsdist-configuration.cc | 16 +++++++++++-- pdns/dnsdistdist/dnsdist-configuration.hh | 6 +++-- pdns/dnsdistdist/dnsdist-console.cc | 3 +++ pdns/dnsdistdist/dnsdist-discovery.cc | 1 + pdns/dnsdistdist/dnsdist-lua-network.cc | 1 + pdns/dnsdistdist/dnsdist-lua.cc | 1 + pdns/dnsdistdist/dnsdist-nghttp2.cc | 2 ++ pdns/dnsdistdist/dnsdist-resolver.cc | 2 ++ pdns/dnsdistdist/dnsdist-self-answers.cc | 1 - pdns/dnsdistdist/dnsdist-tcp.cc | 3 +++ pdns/dnsdistdist/dnsdist-web.cc | 1 + pdns/dnsdistdist/dnsdist-xsk.cc | 3 +++ pdns/dnsdistdist/dnsdist.cc | 28 +++++++++++++++-------- pdns/dnsdistdist/doh.cc | 4 ++++ pdns/dnsdistdist/doh3.cc | 2 ++ pdns/dnsdistdist/doq.cc | 2 ++ pdns/sholder.hh | 11 +++++++++ 19 files changed, 76 insertions(+), 15 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-async.cc b/pdns/dnsdistdist/dnsdist-async.cc index 776a8f011c..194077cdf7 100644 --- a/pdns/dnsdistdist/dnsdist-async.cc +++ b/pdns/dnsdistdist/dnsdist-async.cc @@ -96,6 +96,8 @@ void AsynchronousHolder::mainThread(std::shared_ptr data) while (true) { bool shouldWait = true; int timeout = -1; + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + { auto content = data->d_content.lock(); if (data->d_done) { diff --git a/pdns/dnsdistdist/dnsdist-carbon.cc b/pdns/dnsdistdist/dnsdist-carbon.cc index b14f5359c9..d005f9552a 100644 --- a/pdns/dnsdistdist/dnsdist-carbon.cc +++ b/pdns/dnsdistdist/dnsdist-carbon.cc @@ -309,6 +309,8 @@ static void carbonHandler(const Carbon::Endpoint& endpoint) try { uint8_t consecutiveFailures = 0; do { + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + DTime dtimer; dtimer.set(); if (doOneCarbonExport(endpoint)) { diff --git a/pdns/dnsdistdist/dnsdist-configuration.cc b/pdns/dnsdistdist/dnsdist-configuration.cc index 70875b8890..c4db8f3563 100644 --- a/pdns/dnsdistdist/dnsdist-configuration.cc +++ b/pdns/dnsdistdist/dnsdist-configuration.cc @@ -29,15 +29,27 @@ static GlobalStateHolder s_currentRuntimeConfiguration; static ImmutableConfiguration s_immutableConfiguration; static std::atomic s_immutableConfigurationDone{false}; -const RuntimeConfiguration& getCurrentRuntimeConfiguration() +static const RuntimeConfiguration& getCurrentRuntimeConfigurationInternal(bool refresh) { static thread_local auto t_threadLocalConfiguration = s_currentRuntimeConfiguration.getLocal(); - return *t_threadLocalConfiguration; + return t_threadLocalConfiguration.get(!refresh); +} + +const RuntimeConfiguration& getCurrentRuntimeConfiguration() +{ + return getCurrentRuntimeConfigurationInternal(false); +} + +const RuntimeConfiguration& refreshLocalRuntimeConfiguration() +{ + return getCurrentRuntimeConfigurationInternal(true); } void updateRuntimeConfiguration(const std::function& mutator) { s_currentRuntimeConfiguration.modify(mutator); + /* refresh the local "cache" right away */ + refreshLocalRuntimeConfiguration(); } void updateImmutableConfiguration(const std::function& mutator) diff --git a/pdns/dnsdistdist/dnsdist-configuration.hh b/pdns/dnsdistdist/dnsdist-configuration.hh index 063c69fdc6..a60a506867 100644 --- a/pdns/dnsdistdist/dnsdist-configuration.hh +++ b/pdns/dnsdistdist/dnsdist-configuration.hh @@ -173,12 +173,14 @@ struct RuntimeConfiguration }; /* Be careful not to hold on this for too long, it can be invalidated - by the next call to getCurrentRuntimeConfiguration() from the + by the next call to refreshLocalRuntimeConfiguration() from the same thread, so better be sure that any function you are not calling - while holding to this reference does not call getCurrentRuntimeConfiguration() + while holding to this reference does not call refreshLocalRuntimeConfiguration itself. When in doubt, better call getCurrentRuntimeConfiguration() twice. */ const RuntimeConfiguration& getCurrentRuntimeConfiguration(); +const RuntimeConfiguration& refreshLocalRuntimeConfiguration(); + /* Get the runtime-immutable configuration */ const ImmutableConfiguration& getImmutableConfiguration(); /* Update the runtime-immutable part of the configuration. This function can only be called diff --git a/pdns/dnsdistdist/dnsdist-console.cc b/pdns/dnsdistdist/dnsdist-console.cc index 626bcd3d25..db0e9735fb 100644 --- a/pdns/dnsdistdist/dnsdist-console.cc +++ b/pdns/dnsdistdist/dnsdist-console.cc @@ -528,6 +528,8 @@ static void controlClientThread(ConsoleConnection&& conn) line = dnsdist::crypto::authenticated::decryptSym(line, consoleKey, readingNonce); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + string response; try { bool withReturn = true; @@ -648,6 +650,7 @@ void controlThread(Socket&& acceptFD) infolog("Accepting control connections on %s", local.toStringWithPort()); while ((sock = SAccept(acceptFD.getHandle(), client)) >= 0) { + dnsdist::configuration::refreshLocalRuntimeConfiguration(); const auto& consoleKey = dnsdist::configuration::getCurrentRuntimeConfiguration().d_consoleKey; FDWrapper socket(sock); if (!dnsdist::crypto::authenticated::isValidKey(consoleKey)) { diff --git a/pdns/dnsdistdist/dnsdist-discovery.cc b/pdns/dnsdistdist/dnsdist-discovery.cc index e65d54ecb3..b3b9fff82e 100644 --- a/pdns/dnsdistdist/dnsdist-discovery.cc +++ b/pdns/dnsdistdist/dnsdist-discovery.cc @@ -500,6 +500,7 @@ void ServiceDiscovery::worker() { setThreadName("dnsdist/discove"); while (true) { + dnsdist::configuration::refreshLocalRuntimeConfiguration(); time_t now = time(nullptr); auto upgradeables = *(s_upgradeableBackends.lock()); diff --git a/pdns/dnsdistdist/dnsdist-lua-network.cc b/pdns/dnsdistdist/dnsdist-lua-network.cc index 58ad214ad5..e4e56e3082 100644 --- a/pdns/dnsdistdist/dnsdist-lua-network.cc +++ b/pdns/dnsdistdist/dnsdist-lua-network.cc @@ -144,6 +144,7 @@ void NetworkListener::runOnce(ListenerData& data, timeval& now, uint32_t timeout return; } + dnsdist::configuration::refreshLocalRuntimeConfiguration(); data.d_running = true; if (data.d_sockets.empty()) { throw runtime_error("NetworkListener started with no sockets"); diff --git a/pdns/dnsdistdist/dnsdist-lua.cc b/pdns/dnsdistdist/dnsdist-lua.cc index f330c2dae7..67fc230fc8 100644 --- a/pdns/dnsdistdist/dnsdist-lua.cc +++ b/pdns/dnsdistdist/dnsdist-lua.cc @@ -291,6 +291,7 @@ static void LuaThread(const std::string& code) for (;;) { try { + dnsdist::configuration::refreshLocalRuntimeConfiguration(); context.executeCode(code); errlog("Lua thread exited, restarting in 5 seconds"); } diff --git a/pdns/dnsdistdist/dnsdist-nghttp2.cc b/pdns/dnsdistdist/dnsdist-nghttp2.cc index 338858f913..5eea2a3429 100644 --- a/pdns/dnsdistdist/dnsdist-nghttp2.cc +++ b/pdns/dnsdistdist/dnsdist-nghttp2.cc @@ -878,6 +878,8 @@ static void dohClientThread(pdns::channel::Receiver&& receiv for (;;) { data.mplexer->run(&now, 1000); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + if (now.tv_sec > lastTimeoutScan) { lastTimeoutScan = now.tv_sec; diff --git a/pdns/dnsdistdist/dnsdist-resolver.cc b/pdns/dnsdistdist/dnsdist-resolver.cc index 8e3417dc8c..c9b1355e4e 100644 --- a/pdns/dnsdistdist/dnsdist-resolver.cc +++ b/pdns/dnsdistdist/dnsdist-resolver.cc @@ -23,11 +23,13 @@ #include "dnsdist-resolver.hh" #include "iputils.hh" +#include "threadname.hh" namespace dnsdist::resolver { void asynchronousResolver(const std::string& hostname, const std::function& ips)>& callback) { + setThreadName("dnsdist/resolve"); addrinfo hints{}; hints.ai_family = AF_UNSPEC; hints.ai_flags = AI_ADDRCONFIG; diff --git a/pdns/dnsdistdist/dnsdist-self-answers.cc b/pdns/dnsdistdist/dnsdist-self-answers.cc index 26beb80e34..ae87da5b31 100644 --- a/pdns/dnsdistdist/dnsdist-self-answers.cc +++ b/pdns/dnsdistdist/dnsdist-self-answers.cc @@ -252,7 +252,6 @@ bool generateAnswerFromRawPacket(DNSQuestion& dnsQuestion, const PacketBuffer& p bool removeRecordsAndSetRCode(DNSQuestion& dnsQuestion, uint8_t rcode) { auto [hadEDNS, dnssecOK] = getEDNSStatusInQuery(dnsQuestion); - dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsQuestion.getMutableData(), [rcode](dnsheader& header) { header.rcode = rcode; header.qr = true; diff --git a/pdns/dnsdistdist/dnsdist-tcp.cc b/pdns/dnsdistdist/dnsdist-tcp.cc index 7e12e67c23..d58cd24f36 100644 --- a/pdns/dnsdistdist/dnsdist-tcp.cc +++ b/pdns/dnsdistdist/dnsdist-tcp.cc @@ -1717,6 +1717,8 @@ static void tcpClientThread(pdns::channel::Receiver&& queryRecei for (;;) { data.mplexer->run(&now); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + try { t_downstreamTCPConnectionsManager.cleanupClosedConnections(now); dnsdist::IncomingConcurrentTCPConnectionsManager::cleanup(time(nullptr)); @@ -1880,6 +1882,7 @@ void tcpAcceptorThread(const std::vector& states) timeval now{}; while (true) { mplexer->run(&now, -1); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); } } } diff --git a/pdns/dnsdistdist/dnsdist-web.cc b/pdns/dnsdistdist/dnsdist-web.cc index de657f3ee0..42885f6b5d 100644 --- a/pdns/dnsdistdist/dnsdist-web.cc +++ b/pdns/dnsdistdist/dnsdist-web.cc @@ -1931,6 +1931,7 @@ void WebserverThread(ComboAddress listeningAddress, Socket sock) try { ComboAddress remote(listeningAddress); int fileDesc = SAccept(sock.getHandle(), remote); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); if (!isClientAllowedByACL(remote)) { vinfolog("Connection to webserver from client %s is not allowed, closing", remote.toStringWithPort()); diff --git a/pdns/dnsdistdist/dnsdist-xsk.cc b/pdns/dnsdistdist/dnsdist-xsk.cc index ff65b5c7cb..42e74bd966 100644 --- a/pdns/dnsdistdist/dnsdist-xsk.cc +++ b/pdns/dnsdistdist/dnsdist-xsk.cc @@ -42,6 +42,7 @@ void XskResponderThread(std::shared_ptr dss, std::shared_ptrisStopped()) { poll(pollfds.data(), pollfds.size(), -1); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); bool needNotify = false; if ((pollfds[0].revents & POLLIN) != 0) { needNotify = true; @@ -135,6 +136,7 @@ void XskRouter(std::shared_ptr xsk) while (true) { try { auto ready = xsk->wait(-1); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); // descriptor 0 gets incoming AF_XDP packets if ((fds.at(0).revents & POLLIN) != 0) { auto packets = xsk->recv(64, &failed); @@ -199,6 +201,7 @@ void XskClientThread(ClientState* clientState) while (!xskInfo->hasIncomingFrames()) { xskInfo->waitForXskSocket(); } + dnsdist::configuration::refreshLocalRuntimeConfiguration(); xskInfo->processIncomingFrames([&](XskPacket packet) { if (XskProcessQuery(*clientState, packet)) { packet.updatePacket(); diff --git a/pdns/dnsdistdist/dnsdist.cc b/pdns/dnsdistdist/dnsdist.cc index 81316cd685..3396f3c196 100644 --- a/pdns/dnsdistdist/dnsdist.cc +++ b/pdns/dnsdistdist/dnsdist.cc @@ -655,7 +655,7 @@ void handleResponseSent(const DNSName& qname, const QType& qtype, double udiff, static void handleResponseTC4UDPClient(DNSQuestion& dnsQuestion, uint16_t udpPayloadSize, PacketBuffer& response) { - if (udpPayloadSize > 0 && response.size() > udpPayloadSize) { + if (udpPayloadSize != 0 && response.size() > udpPayloadSize) { vinfolog("Got a response of size %d while the initial UDP payload size was %d, truncating", response.size(), udpPayloadSize); truncateTC(dnsQuestion.getMutableData(), dnsQuestion.getMaximumSize(), dnsQuestion.ids.qname.wirelength(), dnsdist::configuration::getCurrentRuntimeConfiguration().d_addEDNSToSelfGeneratedResponses); dnsdist::PacketMangling::editDNSHeaderFromPacket(dnsQuestion.getMutableData(), [](dnsheader& header) { @@ -663,7 +663,7 @@ static void handleResponseTC4UDPClient(DNSQuestion& dnsQuestion, uint16_t udpPay return true; }); } - else if (dnsQuestion.getHeader()->tc && dnsdist::configuration::getCurrentRuntimeConfiguration().d_truncateTC) { + else if (dnsdist::configuration::getCurrentRuntimeConfiguration().d_truncateTC && dnsQuestion.getHeader()->tc) { truncateTC(response, dnsQuestion.getMaximumSize(), dnsQuestion.ids.qname.wirelength(), dnsdist::configuration::getCurrentRuntimeConfiguration().d_addEDNSToSelfGeneratedResponses); } } @@ -792,6 +792,8 @@ void responderThread(std::shared_ptr dss) break; } + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + for (const auto& sockDesc : sockets) { /* allocate one more byte so we can detect truncation */ // NOLINTNEXTLINE(bugprone-use-after-move): resizing a vector has no preconditions so it is valid to do so after moving it @@ -2211,6 +2213,7 @@ static void udpClientThread(std::vector states) packet.resize(static_cast(got)); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); processUDPQuery(*param.cs, &msgh, remote, dest, packet, nullptr, nullptr, nullptr, nullptr); }; @@ -2292,6 +2295,7 @@ static void maintThread() for (;;) { std::this_thread::sleep_for(std::chrono::seconds(interval)); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); { auto lua = g_lua.lock(); try { @@ -2364,6 +2368,7 @@ static void dynBlockMaintenanceThread() { setThreadName("dnsdist/dynBloc"); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); DynBlockMaintenance::run(); } #endif @@ -2374,7 +2379,7 @@ static void secPollThread() setThreadName("dnsdist/secpoll"); for (;;) { - const auto& runtimeConfig = dnsdist::configuration::getCurrentRuntimeConfiguration(); + const auto& runtimeConfig = dnsdist::configuration::refreshLocalRuntimeConfiguration(); try { dnsdist::secpoll::doSecPoll(runtimeConfig.d_secPollSuffix); @@ -2421,9 +2426,11 @@ static void healthChecksThread() } std::unique_ptr mplexer{nullptr}; + const auto& runtimeConfig = dnsdist::configuration::refreshLocalRuntimeConfiguration(); + // this points to the actual shared_ptrs! // coverity[auto_causes_copy] - const auto servers = dnsdist::configuration::getCurrentRuntimeConfiguration().d_backends; + const auto servers = runtimeConfig.d_backends; for (const auto& dss : servers) { dss->updateStatisticsInfo(); @@ -3122,11 +3129,12 @@ static void parseParameters(int argc, char** argv, CommandLineParameters& cmdLin static void setupPools() { bool precompute = false; - if (dnsdist::configuration::getCurrentRuntimeConfiguration().d_lbPolicy->getName() == "chashed") { + const auto& currentConfig = dnsdist::configuration::getCurrentRuntimeConfiguration(); + if (currentConfig.d_lbPolicy->getName() == "chashed") { precompute = true; } else { - for (const auto& entry : dnsdist::configuration::getCurrentRuntimeConfiguration().d_pools) { + for (const auto& entry : currentConfig.d_pools) { if (entry.second->policy != nullptr && entry.second->policy->getName() == "chashed") { precompute = true; break; @@ -3136,7 +3144,7 @@ static void setupPools() if (precompute) { vinfolog("Pre-computing hashes for consistent hash load-balancing policy"); // pre compute hashes - for (const auto& backend : dnsdist::configuration::getCurrentRuntimeConfiguration().d_backends) { + for (const auto& backend : currentConfig.d_backends) { if (backend->d_config.d_weight < 100) { vinfolog("Warning, the backend '%s' has a very low weight (%d), which will not yield a good distribution of queries with the 'chashed' policy. Please consider raising it to at least '100'.", backend->getName(), backend->d_config.d_weight); } @@ -3262,11 +3270,11 @@ static void startFrontends() if (clientState->dohFrontend != nullptr && clientState->dohFrontend->d_library == "h2o") { #ifdef HAVE_DNS_OVER_HTTPS #ifdef HAVE_LIBH2OEVLOOP - std::thread dotThreadHandle(dohThread, clientState.get()); + std::thread dohThreadHandle(dohThread, clientState.get()); if (!clientState->cpus.empty()) { - mapThreadToCPUList(dotThreadHandle.native_handle(), clientState->cpus); + mapThreadToCPUList(dohThreadHandle.native_handle(), clientState->cpus); } - dotThreadHandle.detach(); + dohThreadHandle.detach(); #endif /* HAVE_LIBH2OEVLOOP */ #endif /* HAVE_DNS_OVER_HTTPS */ continue; diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc index 43f4cb3941..9dfaa5767a 100644 --- a/pdns/dnsdistdist/doh.cc +++ b/pdns/dnsdistdist/doh.cc @@ -701,6 +701,8 @@ static void processDOHQuery(DOHUnitUniquePtr&& unit, bool inMainThread = false) } }; + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + auto& ids = unit->ids; uint16_t queryId = 0; ComboAddress remote; @@ -1072,6 +1074,8 @@ static std::optional processForwardedForHeader(const h2o_req_t* re */ static int doh_handler(h2o_handler_t *self, h2o_req_t *req) { + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + try { if (req->conn->ctx->storage.size == 0) { return 0; // although we might was well crash on this diff --git a/pdns/dnsdistdist/doh3.cc b/pdns/dnsdistdist/doh3.cc index a027dd4173..7a8bc439f0 100644 --- a/pdns/dnsdistdist/doh3.cc +++ b/pdns/dnsdistdist/doh3.cc @@ -1019,6 +1019,8 @@ void doh3Thread(ClientState* clientState) readyFDs.clear(); mplexer->getAvailableFDs(readyFDs, 500); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + try { if (std::find(readyFDs.begin(), readyFDs.end(), sock.getHandle()) != readyFDs.end()) { handleSocketReadable(*frontend, *clientState, sock, buffer); diff --git a/pdns/dnsdistdist/doq.cc b/pdns/dnsdistdist/doq.cc index 46c9c9202b..d2aa389623 100644 --- a/pdns/dnsdistdist/doq.cc +++ b/pdns/dnsdistdist/doq.cc @@ -806,6 +806,8 @@ void doqThread(ClientState* clientState) readyFDs.clear(); mplexer->getAvailableFDs(readyFDs, 500); + dnsdist::configuration::refreshLocalRuntimeConfiguration(); + try { if (std::find(readyFDs.begin(), readyFDs.end(), sock.getHandle()) != readyFDs.end()) { handleSocketReadable(*frontend, *clientState, sock, buffer); diff --git a/pdns/sholder.hh b/pdns/sholder.hh index f26dd250bd..554a0f1b59 100644 --- a/pdns/sholder.hh +++ b/pdns/sholder.hh @@ -74,6 +74,17 @@ public: return *operator->(); } + // fast const-only access, see "read-only" above. + // if allowedOutdated is true, the current local version is returned + // without checking if there has been an update. + const T& get(bool allowedOutdated = false) + { + if (!allowedOutdated || !d_state) { + return *operator->(); + } + return *d_state; + } + void reset() { d_generation = 0; -- 2.47.3