From 3c194549a61e68ed38bbcd90e69f1f954ff286f4 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Fri, 22 Apr 2011 16:09:15 +0200 Subject: [PATCH 1/7] [trac859] Remove leftover forward declaration --- src/lib/nsas/nameserver_address_store.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lib/nsas/nameserver_address_store.h b/src/lib/nsas/nameserver_address_store.h index 148052ddc8..87845c932d 100644 --- a/src/lib/nsas/nameserver_address_store.h +++ b/src/lib/nsas/nameserver_address_store.h @@ -38,7 +38,6 @@ template class LruList; namespace nsas { -class ResolverInterface; template class HashTable; class ZoneEntry; class NameserverEntry; From ffbe8a099bece23efb94e3adbe8fdfff8cfb379e Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Fri, 22 Apr 2011 18:21:21 +0200 Subject: [PATCH 2/7] [trac859] Make TestResolver reusable by other libs --- .../nsas/tests/nameserver_address_unittest.cc | 3 +- .../nsas/tests/nameserver_entry_unittest.cc | 3 +- src/lib/nsas/tests/nsas_test.h | 148 +------------- .../resolve/tests/recursive_query_unittest.cc | 2 + src/lib/util/unittests/Makefile.am | 2 +- src/lib/util/unittests/resolver.h | 193 ++++++++++++++++++ 6 files changed, 202 insertions(+), 149 deletions(-) create mode 100644 src/lib/util/unittests/resolver.h diff --git a/src/lib/nsas/tests/nameserver_address_unittest.cc b/src/lib/nsas/tests/nameserver_address_unittest.cc index e3bc5de8e2..1b211e9e3e 100644 --- a/src/lib/nsas/tests/nameserver_address_unittest.cc +++ b/src/lib/nsas/tests/nameserver_address_unittest.cc @@ -51,7 +51,8 @@ public: ns_.reset(new NameserverEntry(name_.toText(), RRClass::IN())); ns_->askIP(resolver_.get(), boost::shared_ptr(new Callback), ANY_OK); resolver_->asksIPs(name_, 0, 1); - resolver_->requests[0].second->success(createResponseMessage(rrv4_)); + resolver_->requests[0].second->success( + isc::util::unittests::createResponseMessage(rrv4_)); } // Return the sample NameserverEntry diff --git a/src/lib/nsas/tests/nameserver_entry_unittest.cc b/src/lib/nsas/tests/nameserver_entry_unittest.cc index 7e9f6750bf..3435d26b77 100644 --- a/src/lib/nsas/tests/nameserver_entry_unittest.cc +++ b/src/lib/nsas/tests/nameserver_entry_unittest.cc @@ -72,7 +72,8 @@ private: RRsetPtr set) { if (set) { - resolver->requests[index].second->success(createResponseMessage(set)); + resolver->requests[index].second->success( + isc::util::unittests::createResponseMessage(set)); } else { resolver->requests[index].second->failure(); } diff --git a/src/lib/nsas/tests/nsas_test.h b/src/lib/nsas/tests/nsas_test.h index 033df8101b..2dd95effd5 100644 --- a/src/lib/nsas/tests/nsas_test.h +++ b/src/lib/nsas/tests/nsas_test.h @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -35,24 +36,12 @@ #include #include #include -#include #include "../nsas_entry.h" using namespace isc::dns::rdata; using namespace isc::dns; using namespace isc::util; - -namespace { - MessagePtr - createResponseMessage(RRsetPtr answer_rrset) - { - MessagePtr response(new Message(Message::RENDER)); - response->setOpcode(Opcode::QUERY()); - response->setRcode(Rcode::NOERROR()); - response->addRRset(Message::SECTION_ANSWER, answer_rrset); - return response; - } -} +using isc::util::unittests::TestResolver; namespace isc { namespace dns { @@ -223,139 +212,6 @@ private: static const uint32_t HASHTABLE_DEFAULT_SIZE = 1009; ///< First prime above 1000 -using namespace std; - -/* - * This pretends to be a resolver. It stores the queries and - * they can be answered. - */ -class TestResolver : public isc::resolve::ResolverInterface { - private: - bool checkIndex(size_t index) { - return (requests.size() > index); - } - - typedef std::map - PresetAnswers; - PresetAnswers answers_; - public: - typedef pair Request; - vector requests; - - /// \brief Destructor - /// - /// This is important. All callbacks in the requests vector must be - /// called to remove them from internal loops. Without this, destroying - /// the NSAS object will leave memory assigned. - ~TestResolver() { - for (size_t i = 0; i < requests.size(); ++i) { - requests[i].second->failure(); - } - } - - virtual void resolve(const QuestionPtr& q, const CallbackPtr& c) { - PresetAnswers::iterator it(answers_.find(*q)); - if (it == answers_.end()) { - requests.push_back(Request(q, c)); - } else { - if (it->second) { - c->success(createResponseMessage(it->second)); - } else { - c->failure(); - } - } - } - - /* - * Add a preset answer. If shared_ptr() is passed (eg. NULL), - * it will generate failure. If the question is not preset, - * it goes to requests and you can answer later. - */ - void addPresetAnswer(const isc::dns::Question& question, - RRsetPtr answer) - { - answers_[question] = answer; - } - - // Thrown if the query at the given index does not exist. - class NoSuchRequest : public std::exception { }; - - // Thrown if the answer does not match the query - class DifferentRequest : public std::exception { }; - - QuestionPtr operator[](size_t index) { - if (index >= requests.size()) { - throw NoSuchRequest(); - } - return (requests[index].first); - } - /* - * Looks if the two provided requests in resolver are A and AAAA. - * Sorts them so index1 is A. - * - * Returns false if there aren't enough elements - */ - bool asksIPs(const Name& name, size_t index1, size_t index2) { - size_t max = (index1 < index2) ? index2 : index1; - if (!checkIndex(max)) { - return false; - } - EXPECT_EQ(name, (*this)[index1]->getName()); - EXPECT_EQ(name, (*this)[index2]->getName()); - EXPECT_EQ(RRClass::IN(), (*this)[index1]->getClass()); - EXPECT_EQ(RRClass::IN(), (*this)[index2]->getClass()); - // If they are the other way around, swap - if ((*this)[index1]->getType() == RRType::AAAA() && - (*this)[index2]->getType() == RRType::A()) - { - TestResolver::Request tmp((*this).requests[index1]); - (*this).requests[index1] = - (*this).requests[index2]; - (*this).requests[index2] = tmp; - } - // Check the correct addresses - EXPECT_EQ(RRType::A(), (*this)[index1]->getType()); - EXPECT_EQ(RRType::AAAA(), (*this)[index2]->getType()); - return (true); - } - - /* - * Sends a simple answer to a query. - * 1) Provide index of a query and the address(es) to pass. - * 2) Provide index of query and components of address to pass. - */ - void answer(size_t index, RRsetPtr& set) { - if (index >= requests.size()) { - throw NoSuchRequest(); - } - requests[index].second->success(createResponseMessage(set)); - } - - void answer(size_t index, const Name& name, const RRType& type, - const rdata::Rdata& rdata, size_t TTL = 100) - { - RRsetPtr set(new RRset(name, RRClass::IN(), - type, RRTTL(TTL))); - set->addRdata(rdata); - answer(index, set); - } - - - void provideNS(size_t index, - RRsetPtr nameservers) - { - if (index >= requests.size()) { - throw NoSuchRequest(); - } - if (requests[index].first->getName() != nameservers->getName() || - requests[index].first->getType() != RRType::NS()) - { - throw DifferentRequest(); - } - requests[index].second->success(createResponseMessage(nameservers)); - } -}; - // String constants. These should end in a dot. static const std::string EXAMPLE_CO_UK("example.co.uk."); static const std::string EXAMPLE_NET("example.net."); diff --git a/src/lib/resolve/tests/recursive_query_unittest.cc b/src/lib/resolve/tests/recursive_query_unittest.cc index 3338893651..22cc974427 100644 --- a/src/lib/resolve/tests/recursive_query_unittest.cc +++ b/src/lib/resolve/tests/recursive_query_unittest.cc @@ -860,4 +860,6 @@ TEST_F(RecursiveQueryTest, DISABLED_recursiveSendNXDOMAIN) { // TODO: add tests that check whether the cache is updated on succesfull // responses, and not updated on failures. + + } diff --git a/src/lib/util/unittests/Makefile.am b/src/lib/util/unittests/Makefile.am index b7026478ff..4d5896f8ad 100644 --- a/src/lib/util/unittests/Makefile.am +++ b/src/lib/util/unittests/Makefile.am @@ -2,6 +2,6 @@ AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib AM_CXXFLAGS = $(B10_CXXFLAGS) lib_LTLIBRARIES = libutil_unittests.la -libutil_unittests_la_SOURCES = fork.h fork.cc +libutil_unittests_la_SOURCES = fork.h fork.cc resolver.h CLEANFILES = *.gcno *.gcda diff --git a/src/lib/util/unittests/resolver.h b/src/lib/util/unittests/resolver.h new file mode 100644 index 0000000000..560a8922ba --- /dev/null +++ b/src/lib/util/unittests/resolver.h @@ -0,0 +1,193 @@ +// Copyright (C) 2011 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#ifndef UTIL_UNITTEST_RESOLVER_H +#define UTIL_UNITTEST_RESOLVER_H + +/// \file resolver.h +/// \brief Fake resolver + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace isc { +namespace util { +namespace unittests { + +/// \brief Put rrset into a message as an answer +inline static isc::dns::MessagePtr +createResponseMessage(isc::dns::RRsetPtr answer_rrset) +{ + isc::dns::MessagePtr response(new isc::dns::Message( + isc::dns::Message::RENDER)); + response->setOpcode(isc::dns::Opcode::QUERY()); + response->setRcode(isc::dns::Rcode::NOERROR()); + response->addRRset(isc::dns::Message::SECTION_ANSWER, answer_rrset); + return response; +} + +/// \brief Mock resolver +/// +/// This class pretends to be a resolver. However, it only stores the +/// requests and can answer them right away by prepared answers. It doesn't +/// do any real work and is intended for testing purposes. +class TestResolver : public isc::resolve::ResolverInterface { + private: + bool checkIndex(size_t index) { + return (requests.size() > index); + } + + typedef std::map + PresetAnswers; + PresetAnswers answers_; + public: + typedef std::pair Request; + /// \brief List of requests the tested class sent trough resolve + std::vector requests; + + /// \brief Destructor + /// + /// This is important. All callbacks in the requests vector must be + /// called to remove them from internal loops. Without this, destroying + /// the NSAS object will leave memory assigned. + ~TestResolver() { + for (size_t i = 0; i < requests.size(); ++i) { + requests[i].second->failure(); + } + } + + /// \brief Testing version of resolve + /// + /// If there's a prepared answer (provided by addPresetAnswer), it + /// answers it right away. Otherwise it just stores the request in + /// the requests member so it can be examined later. + virtual void resolve(const isc::dns::QuestionPtr& q, + const CallbackPtr& c) + { + PresetAnswers::iterator it(answers_.find(*q)); + if (it == answers_.end()) { + requests.push_back(Request(q, c)); + } else { + if (it->second) { + c->success(createResponseMessage(it->second)); + } else { + c->failure(); + } + } + } + + /// \brief Add a preset answer. + /// + /// Add a preset answer. If shared_ptr() is passed (eg. NULL), + /// it will generate failure. If the question is not preset, + /// it goes to requests and you can answer later. + void addPresetAnswer(const isc::dns::Question& question, + isc::dns::RRsetPtr answer) + { + answers_[question] = answer; + } + + /// \brief Thrown if the query at the given index does not exist. + class NoSuchRequest : public std::exception { }; + + /// \brief Thrown if the answer does not match the query + class DifferentRequest : public std::exception { }; + + /// \brief Provides the question of request on given answer + isc::dns::QuestionPtr operator[](size_t index) { + if (index >= requests.size()) { + throw NoSuchRequest(); + } + return (requests[index].first); + } + /// \brief Test it asks for IP addresses + /// Looks if the two provided requests in resolver are A and AAAA. + /// Sorts them so index1 is A. + /// + /// Returns false if there aren't enough elements + bool asksIPs(const isc::dns::Name& name, size_t index1, + size_t index2) + { + size_t max = (index1 < index2) ? index2 : index1; + if (!checkIndex(max)) { + return false; + } + EXPECT_EQ(name, (*this)[index1]->getName()); + EXPECT_EQ(name, (*this)[index2]->getName()); + EXPECT_EQ(isc::dns::RRClass::IN(), (*this)[index1]->getClass()); + EXPECT_EQ(isc::dns::RRClass::IN(), (*this)[index2]->getClass()); + // If they are the other way around, swap + if ((*this)[index1]->getType() == isc::dns::RRType::AAAA() && + (*this)[index2]->getType() == isc::dns::RRType::A()) + { + TestResolver::Request tmp((*this).requests[index1]); + (*this).requests[index1] = + (*this).requests[index2]; + (*this).requests[index2] = tmp; + } + // Check the correct addresses + EXPECT_EQ(isc::dns::RRType::A(), (*this)[index1]->getType()); + EXPECT_EQ(isc::dns::RRType::AAAA(), (*this)[index2]->getType()); + return (true); + } + + /// \brief Answer a request + /// Sends a simple answer to a query. + /// 1) Provide index of a query and the address(es) to pass. + /// 2) Provide index of query and components of address to pass. + void answer(size_t index, isc::dns::RRsetPtr& set) { + if (index >= requests.size()) { + throw NoSuchRequest(); + } + requests[index].second->success(createResponseMessage(set)); + } + + void answer(size_t index, const isc::dns::Name& name, + const isc::dns::RRType& type, + const isc::dns::rdata::Rdata& rdata, size_t TTL = 100) + { + isc::dns::RRsetPtr set(new isc::dns::RRset(name, + isc::dns::RRClass::IN(), + type, + isc::dns::RRTTL(TTL))); + set->addRdata(rdata); + answer(index, set); + } + /// \Answer the query at index by list of nameservers + void provideNS(size_t index, isc::dns::RRsetPtr nameservers) + { + if (index >= requests.size()) { + throw NoSuchRequest(); + } + if (requests[index].first->getName() != nameservers->getName() || + requests[index].first->getType() != isc::dns::RRType::NS()) + { + throw DifferentRequest(); + } + requests[index].second->success(createResponseMessage(nameservers)); + } +}; + +} +} +} + +#endif From ea46adf9f6bbdd78a06ae748d979986a75b9b8d2 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Fri, 22 Apr 2011 21:13:12 +0200 Subject: [PATCH 3/7] [trac859] Test for correct resolving start --- .../resolve/tests/recursive_query_unittest.cc | 78 ++++++++++++++++++- 1 file changed, 75 insertions(+), 3 deletions(-) diff --git a/src/lib/resolve/tests/recursive_query_unittest.cc b/src/lib/resolve/tests/recursive_query_unittest.cc index 22cc974427..aca1102766 100644 --- a/src/lib/resolve/tests/recursive_query_unittest.cc +++ b/src/lib/resolve/tests/recursive_query_unittest.cc @@ -31,7 +31,9 @@ #include #include +#include #include +#include #include #include @@ -110,6 +112,9 @@ class RecursiveQueryTest : public ::testing::Test { protected: RecursiveQueryTest(); ~RecursiveQueryTest() { + // It would delete itself, but after the io_service_, which could + // segfailt in case there were unhandled requests + resolver_.reset(); if (res_ != NULL) { freeaddrinfo(res_); } @@ -423,16 +428,17 @@ protected: vector callback_data_; int sock_; struct addrinfo* res_; + boost::shared_ptr resolver_; }; RecursiveQueryTest::RecursiveQueryTest() : dns_service_(NULL), callback_(NULL), callback_protocol_(0), - callback_native_(-1), sock_(-1), res_(NULL) + callback_native_(-1), sock_(-1), res_(NULL), + resolver_(new isc::util::unittests::TestResolver()) { io_service_ = new IOService(); setDNSService(true, true); - boost::shared_ptrmock_resolver(new MockResolver()); - nsas_ = new isc::nsas::NameserverAddressStore(mock_resolver); + nsas_ = new isc::nsas::NameserverAddressStore(resolver_); } TEST_F(RecursiveQueryTest, v6UDPSend) { @@ -857,6 +863,72 @@ TEST_F(RecursiveQueryTest, DISABLED_recursiveSendNXDOMAIN) { EXPECT_EQ(0, answer->getRRCount(Message::SECTION_ANSWER)); } +// Test that we don't start at root when we have a lower NS cached. +TEST_F(RecursiveQueryTest, CachedNS) { + setDNSService(true, true); + + // Prefill the cache. There's a zone with a NS and IP address for one + // of them (to see that one is enough) and another deeper one, with NS, + // but without IP. + RRsetPtr nsUpper(new RRset(Name("example.org"), RRClass::IN(), + RRType::NS(), RRTTL(300))); + nsUpper->addRdata(rdata::generic::NS(Name("ns.example.org"))); + nsUpper->addRdata(rdata::generic::NS(Name("ns2.example.org"))); + + RRsetPtr nsLower(new RRset(Name("somewhere.deep.example.org"), + RRClass::IN(), RRType::NS(), RRTTL(300))); + nsLower->addRdata(rdata::generic::NS(Name("ns.somewhere.deep.example.org")) + ); + + RRsetPtr nsIp(new RRset(Name("ns2.example.org"), RRClass::IN(), + RRType::A(), RRTTL(300))); + nsIp->addRdata(rdata::in::A("192.0.2.1")); + + // Make sure the test runs in the correct environment (we don't test + // the cache, but we need it to unswer this way for the test, so we + // just make sure) + ASSERT_TRUE(cache_.update(nsUpper)); + ASSERT_TRUE(cache_.update(nsLower)); + ASSERT_TRUE(cache_.update(nsIp)); + RRsetPtr deepest(cache_.lookupDeepestNS(Name( + "www.somewhere.deep.example.org"), RRClass::IN())); + ASSERT_NE(RRsetPtr(), deepest); + ASSERT_EQ(nsLower->getName(), deepest->getName()); + + // Prepare the recursive query + vector > roots; + roots.push_back(pair("192.0.2.2", 53)); + + RecursiveQuery rq(*dns_service_, *nsas_, cache_, + vector >(), roots); + // Ask a question at the bottom. It should not use the lower NS, because + // it would lead to a loop in NS. But it can use the nsUpper one, it has + // an IP address and we can avoid asking root. + Question q(Name("www.somewhere.deep.example.org"), RRClass::IN(), + RRType::A()); + OutputBufferPtr buffer(new OutputBuffer(0)); + MessagePtr answer(new Message(Message::RENDER)); + bool done; + // The server is here so we have something to pass there + MockServer server(*io_service_); + rq.resolve(q, answer, buffer, &server); + // We don't need to run the service in this test. We are interested only + // in the place it starts resolving at + + // Now, it should not start at the nsLower, because we don't have IP + // address for it and that could cause a loop. But it can be the nsUpper, + // we don't need to go to root. + // + // We use the trick that NSAS asks the resolver for the NS record, because + // it doesn't know it. This isn't the best way to write a unittest, but + // it isn't easy to replace the NSAS with something else, so this is the + // least painfull way. + EXPECT_NO_THROW(EXPECT_EQ(nsUpper->getName(), + (*resolver_)[0]->getName()) << + "It starts resolving at the wrong place") << + "It does not ask NSAS anything, how does it know where to send?"; +} + // TODO: add tests that check whether the cache is updated on succesfull // responses, and not updated on failures. From b963854734ff8ba78ccb14f2e5a481c9426881c6 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Fri, 22 Apr 2011 21:59:49 +0200 Subject: [PATCH 4/7] [trac859] Start delegating as deep as possible --- src/lib/resolve/recursive_query.cc | 54 +++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/src/lib/resolve/recursive_query.cc b/src/lib/resolve/recursive_query.cc index c0d5ee64fb..1e80c376d5 100644 --- a/src/lib/resolve/recursive_query.cc +++ b/src/lib/resolve/recursive_query.cc @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -48,6 +49,23 @@ using namespace isc::asiolink; namespace isc { namespace asiodns { +namespace { +// Function to check if the given name/class has any address in the cache +bool +hasAddress(const Name& name, const RRClass& rrClass, + const isc::cache::ResolverCache& cache) +{ + // FIXME: If we are single-stack and we get only the other type of + // address, what should we do? In that case, it will be considered + // unreachable, which is most probably true, because A and AAAA will + // usually have the same RTT, so we should have both or none from the + // glue. + return (cache.lookup(name, RRType::A(), rrClass) != RRsetPtr() || + cache.lookup(name, RRType::AAAA(), rrClass) != RRsetPtr()); +} + +} + typedef std::vector > AddressVector; // Here we do not use the typedef above, as the SunStudio compiler @@ -239,7 +257,6 @@ private: // if we have a response for our query stored already. if // so, call handlerecursiveresponse(), if not, we call send() void doLookup() { - cur_zone_ = "."; dlog("doLookup: try cache"); Message cached_message(Message::RENDER); isc::resolve::initResponseMessage(question_, cached_message); @@ -255,6 +272,41 @@ private: stop(); } } else { + dlog("doLookup: get lowest usable delegation from cache"); + cur_zone_ = "."; + bool foundAddress(false); + RRsetPtr cachedNS; + Name tryName(question_.getName()); + // Look for delegation point from bottom, until we find one with + // IP address or get to root. + // + // We need delegation with IP address so we can ask it right away. + // If we don't have the IP address, we would need to ask above it + // anyway in the best case, and the NS could be inside the zone, + // and we could get all loopy with the NSAS in the worst case. + while (!foundAddress && tryName.getLabelCount() > 1 && + (cachedNS = cache_.lookupDeepestNS(tryName, + question_.getClass())) != + RRsetPtr()) { + // Look if we have an IP address for the NS + for (RdataIteratorPtr ns(cachedNS->getRdataIterator()); + !ns->isLast(); ns->next()) { + // Do we have IP for this specific NS? + if (hasAddress(dynamic_cast( + ns->getCurrent()).getNSName(), + question_.getClass(), cache_)) { + // Found one, stop checking and use this zone + // (there may be more addresses, that's only better) + foundAddress = true; + cur_zone_ = cachedNS->getName().toText(); + break; + } + } + // We don't have anything for this one, so try something higher + if (!foundAddress && tryName.getLabelCount() > 1) { + tryName = tryName.split(1); + } + } send(); } From abdb0ff5e8cdd781d7c4960bb51d8538f4e3b44e Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 26 Apr 2011 13:38:25 +0200 Subject: [PATCH 5/7] [trac859] Remove unused class --- src/lib/resolve/recursive_query.cc | 2 +- src/lib/resolve/tests/recursive_query_unittest.cc | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/lib/resolve/recursive_query.cc b/src/lib/resolve/recursive_query.cc index 1e80c376d5..7f2846223c 100644 --- a/src/lib/resolve/recursive_query.cc +++ b/src/lib/resolve/recursive_query.cc @@ -309,7 +309,7 @@ private: } send(); } - + } // Send the current question to the given nameserver address diff --git a/src/lib/resolve/tests/recursive_query_unittest.cc b/src/lib/resolve/tests/recursive_query_unittest.cc index aca1102766..85f78baa6f 100644 --- a/src/lib/resolve/tests/recursive_query_unittest.cc +++ b/src/lib/resolve/tests/recursive_query_unittest.cc @@ -353,12 +353,6 @@ protected: private: bool* done_; }; - - class MockResolver : public isc::resolve::ResolverInterface { - void resolve(const QuestionPtr& question, - const ResolverInterface::CallbackPtr& callback) { - } - }; // This version of mock server just stops the io_service when it is resumed // the second time. (Used in the clientTimeout test, where resume From 309bc10ee624106cf75165efd4251312deed3212 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 26 Apr 2011 20:59:59 +0200 Subject: [PATCH 6/7] [trac859] Split the logic to separate function --- src/lib/resolve/recursive_query.cc | 78 +++++++++++-------- .../resolve/tests/recursive_query_unittest.cc | 42 ++++++++-- 2 files changed, 78 insertions(+), 42 deletions(-) diff --git a/src/lib/resolve/recursive_query.cc b/src/lib/resolve/recursive_query.cc index 7f2846223c..34411ee0e2 100644 --- a/src/lib/resolve/recursive_query.cc +++ b/src/lib/resolve/recursive_query.cc @@ -66,6 +66,48 @@ hasAddress(const Name& name, const RRClass& rrClass, } +/// \brief Find deepest usable delegation in the cache +/// +/// This finds the deepest delegation we have in cache and is safe to use. +/// It is not public function, therefore it's not in header. But it's not +/// in anonymous namespace, so we can call it from unittests. +/// \param name The name we want to delegate to. +/// \param cache The place too look for known delegations. +std::string +deepestDelegation(Name name, RRClass rrclass, + isc::cache::ResolverCache& cache) +{ + RRsetPtr cachedNS; + // Look for delegation point from bottom, until we find one with + // IP address or get to root. + // + // We need delegation with IP address so we can ask it right away. + // If we don't have the IP address, we would need to ask above it + // anyway in the best case, and the NS could be inside the zone, + // and we could get all loopy with the NSAS in the worst case. + while (name.getLabelCount() > 1 && + (cachedNS = cache.lookupDeepestNS(name, rrclass)) != RRsetPtr()) { + // Look if we have an IP address for the NS + for (RdataIteratorPtr ns(cachedNS->getRdataIterator()); + !ns->isLast(); ns->next()) { + // Do we have IP for this specific NS? + if (hasAddress(dynamic_cast( + ns->getCurrent()).getNSName(), rrclass, + cache)) { + // Found one, stop checking and use this zone + // (there may be more addresses, that's only better) + return (cachedNS->getName().toText()); + } + } + // We don't have anything for this one, so try something higher + if (name.getLabelCount() > 1) { + name = name.split(1); + } + } + // Fallback, nothing found, start at root + return ("."); +} + typedef std::vector > AddressVector; // Here we do not use the typedef above, as the SunStudio compiler @@ -273,40 +315,8 @@ private: } } else { dlog("doLookup: get lowest usable delegation from cache"); - cur_zone_ = "."; - bool foundAddress(false); - RRsetPtr cachedNS; - Name tryName(question_.getName()); - // Look for delegation point from bottom, until we find one with - // IP address or get to root. - // - // We need delegation with IP address so we can ask it right away. - // If we don't have the IP address, we would need to ask above it - // anyway in the best case, and the NS could be inside the zone, - // and we could get all loopy with the NSAS in the worst case. - while (!foundAddress && tryName.getLabelCount() > 1 && - (cachedNS = cache_.lookupDeepestNS(tryName, - question_.getClass())) != - RRsetPtr()) { - // Look if we have an IP address for the NS - for (RdataIteratorPtr ns(cachedNS->getRdataIterator()); - !ns->isLast(); ns->next()) { - // Do we have IP for this specific NS? - if (hasAddress(dynamic_cast( - ns->getCurrent()).getNSName(), - question_.getClass(), cache_)) { - // Found one, stop checking and use this zone - // (there may be more addresses, that's only better) - foundAddress = true; - cur_zone_ = cachedNS->getName().toText(); - break; - } - } - // We don't have anything for this one, so try something higher - if (!foundAddress && tryName.getLabelCount() > 1) { - tryName = tryName.split(1); - } - } + cur_zone_ = deepestDelegation(question_.getName(), + question_.getClass(), cache_); send(); } diff --git a/src/lib/resolve/tests/recursive_query_unittest.cc b/src/lib/resolve/tests/recursive_query_unittest.cc index 85f78baa6f..2520994a9d 100644 --- a/src/lib/resolve/tests/recursive_query_unittest.cc +++ b/src/lib/resolve/tests/recursive_query_unittest.cc @@ -61,6 +61,18 @@ using namespace isc::asiolink; using namespace isc::dns; using namespace isc::util; +namespace isc { +namespace asiodns { + +// This is defined in recursive_query.cc, but not in header (it's not public +// function). So bring it in to be tested. +std::string +deepestDelegation(Name name, RRClass rrclass, + isc::cache::ResolverCache& cache); + +} +} + namespace { const char* const TEST_SERVER_PORT = "53535"; const char* const TEST_CLIENT_PORT = "53536"; @@ -861,6 +873,11 @@ TEST_F(RecursiveQueryTest, DISABLED_recursiveSendNXDOMAIN) { TEST_F(RecursiveQueryTest, CachedNS) { setDNSService(true, true); + // Check we have a reasonable fallback - if there's nothing of interest + // in the cache, start at root. + EXPECT_EQ(".", deepestDelegation(Name("www.somewhere.deep.example.org"), + RRClass::IN(), cache_)); + // Prefill the cache. There's a zone with a NS and IP address for one // of them (to see that one is enough) and another deeper one, with NS, // but without IP. @@ -889,6 +906,22 @@ TEST_F(RecursiveQueryTest, CachedNS) { ASSERT_NE(RRsetPtr(), deepest); ASSERT_EQ(nsLower->getName(), deepest->getName()); + // Direct check of the function that chooses the delegation point + // It should not use nsLower, because we don't have IP address for + // that one. But it can choose nsUpper. + EXPECT_EQ("example.org.", + deepestDelegation(Name("www.somewhere.deep.example.org"), + RRClass::IN(), cache_)); + + // Now more complex and indirect test: + // We ask it to resolve the name for us. It will pick up a delegation + // point and ask NSAS for it. NSAS will in turn ask resolver for NS record + // of the delegation point. We then pick it up from the fake resolver + // and check it is the correct one. This checks the delegation point + // travels safely trough the whole path there (it would be enough to check + // it up to NSAS, but replacing NSAS is more complicated, so we just + // include in the test as well for simplicity). + // Prepare the recursive query vector > roots; roots.push_back(pair("192.0.2.2", 53)); @@ -909,14 +942,7 @@ TEST_F(RecursiveQueryTest, CachedNS) { // We don't need to run the service in this test. We are interested only // in the place it starts resolving at - // Now, it should not start at the nsLower, because we don't have IP - // address for it and that could cause a loop. But it can be the nsUpper, - // we don't need to go to root. - // - // We use the trick that NSAS asks the resolver for the NS record, because - // it doesn't know it. This isn't the best way to write a unittest, but - // it isn't easy to replace the NSAS with something else, so this is the - // least painfull way. + // Look what is asked by NSAS - it should be our delegation point. EXPECT_NO_THROW(EXPECT_EQ(nsUpper->getName(), (*resolver_)[0]->getName()) << "It starts resolving at the wrong place") << From 87e4b9874ac16ccc5d236a2fcb8221942713e086 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 27 Apr 2011 08:58:20 -0700 Subject: [PATCH 7/7] [master] corrected a cppcheck error (unused variable) --- src/lib/resolve/tests/recursive_query_unittest.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lib/resolve/tests/recursive_query_unittest.cc b/src/lib/resolve/tests/recursive_query_unittest.cc index 2520994a9d..04a78032b5 100644 --- a/src/lib/resolve/tests/recursive_query_unittest.cc +++ b/src/lib/resolve/tests/recursive_query_unittest.cc @@ -935,7 +935,6 @@ TEST_F(RecursiveQueryTest, CachedNS) { RRType::A()); OutputBufferPtr buffer(new OutputBuffer(0)); MessagePtr answer(new Message(Message::RENDER)); - bool done; // The server is here so we have something to pass there MockServer server(*io_service_); rq.resolve(q, answer, buffer, &server);