From 4864018bdb5a6f3f9ea9d7a9cc608521655233dd Mon Sep 17 00:00:00 2001 From: Jelte Jansen Date: Wed, 16 Feb 2011 16:14:39 +0100 Subject: [PATCH 1/8] [trac491] initial basic hookup of the cache It only stores the final result, and only looks into the cache at the start of a resolve() call right now, but it appears to work --- src/bin/auth/Makefile.am | 1 + src/bin/auth/benchmarks/Makefile.am | 1 + src/bin/auth/tests/Makefile.am | 1 + src/bin/resolver/Makefile.am | 2 + src/bin/resolver/resolver.cc | 11 ++--- src/bin/resolver/tests/Makefile.am | 2 + src/lib/asiolink/Makefile.am | 2 + src/lib/asiolink/recursive_query.cc | 67 +++++++++++++++++++++++------ src/lib/asiolink/recursive_query.h | 4 ++ src/lib/asiolink/tests/Makefile.am | 2 + src/lib/asiolink/udp_server.cc | 2 + src/lib/cache/message_cache.cc | 2 + src/lib/cache/resolver_cache.cc | 2 +- src/lib/cache/rrset_cache.h | 2 +- 14 files changed, 79 insertions(+), 22 deletions(-) diff --git a/src/bin/auth/Makefile.am b/src/bin/auth/Makefile.am index e9097f21df..36de53dc6e 100644 --- a/src/bin/auth/Makefile.am +++ b/src/bin/auth/Makefile.am @@ -50,6 +50,7 @@ b10_auth_LDADD += $(top_builddir)/src/lib/config/libcfgclient.la b10_auth_LDADD += $(top_builddir)/src/lib/cc/libcc.la b10_auth_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la b10_auth_LDADD += $(top_builddir)/src/lib/asiolink/libasiolink.la +b10_auth_LDADD += $(top_builddir)/src/lib/nsas/libnsas.la b10_auth_LDADD += $(top_builddir)/src/lib/xfr/libxfr.la b10_auth_LDADD += $(SQLITE_LIBS) diff --git a/src/bin/auth/benchmarks/Makefile.am b/src/bin/auth/benchmarks/Makefile.am index 05ab7540b1..653d5025f3 100644 --- a/src/bin/auth/benchmarks/Makefile.am +++ b/src/bin/auth/benchmarks/Makefile.am @@ -21,5 +21,6 @@ query_bench_LDADD += $(top_builddir)/src/lib/config/libcfgclient.la query_bench_LDADD += $(top_builddir)/src/lib/cc/libcc.la query_bench_LDADD += $(top_builddir)/src/lib/xfr/libxfr.la query_bench_LDADD += $(top_builddir)/src/lib/log/liblog.la +query_bench_LDADD += $(top_builddir)/src/lib/nsas/libnsas.la query_bench_LDADD += $(top_builddir)/src/lib/asiolink/libasiolink.la query_bench_LDADD += $(SQLITE_LIBS) diff --git a/src/bin/auth/tests/Makefile.am b/src/bin/auth/tests/Makefile.am index a1114e4694..def99b0564 100644 --- a/src/bin/auth/tests/Makefile.am +++ b/src/bin/auth/tests/Makefile.am @@ -45,6 +45,7 @@ run_unittests_LDADD += $(top_builddir)/src/lib/cc/libcc.la run_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la run_unittests_LDADD += $(top_builddir)/src/lib/xfr/libxfr.la run_unittests_LDADD += $(top_builddir)/src/lib/log/liblog.la +run_unittests_LDADD += $(top_builddir)/src/lib/nsas/libnsas.la endif noinst_PROGRAMS = $(TESTS) diff --git a/src/bin/resolver/Makefile.am b/src/bin/resolver/Makefile.am index 9e86ddde1e..75d6249ffc 100644 --- a/src/bin/resolver/Makefile.am +++ b/src/bin/resolver/Makefile.am @@ -48,6 +48,8 @@ b10_resolver_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la b10_resolver_LDADD += $(top_builddir)/src/lib/asiolink/libasiolink.la b10_resolver_LDADD += $(top_builddir)/src/lib/xfr/libxfr.la b10_resolver_LDADD += $(top_builddir)/src/lib/log/liblog.la +b10_resolver_LDADD += $(top_builddir)/src/lib/cache/libcache.la +b10_resolver_LDADD += $(top_builddir)/src/lib/nsas/libnsas.la b10_resolver_LDADD += $(top_builddir)/src/bin/auth/change_user.o b10_resolver_LDFLAGS = -pthread diff --git a/src/bin/resolver/resolver.cc b/src/bin/resolver/resolver.cc index dfd84a661a..c635fc9a16 100644 --- a/src/bin/resolver/resolver.cc +++ b/src/bin/resolver/resolver.cc @@ -256,11 +256,10 @@ public: const qid_t qid = query_message->getQid(); const bool rd = query_message->getHeaderFlag(Message::HEADERFLAG_RD); const bool cd = query_message->getHeaderFlag(Message::HEADERFLAG_CD); - const Opcode& opcode = query_message->getOpcode(); - - // Fill in the final details of the answer message + + // The opcode and question section should have already been set, + // fill in the final details of the answer message answer_message->setQid(qid); - answer_message->setOpcode(opcode); answer_message->setHeaderFlag(Message::HEADERFLAG_QR); answer_message->setHeaderFlag(Message::HEADERFLAG_RA); @@ -271,10 +270,6 @@ public: answer_message->setHeaderFlag(Message::HEADERFLAG_CD); } - vector questions; - questions.assign(query_message->beginQuestion(), query_message->endQuestion()); - for_each(questions.begin(), questions.end(), QuestionInserter(answer_message)); - // Now we can clear the buffer and render the new message into it buffer->clear(); MessageRenderer renderer(*buffer); diff --git a/src/bin/resolver/tests/Makefile.am b/src/bin/resolver/tests/Makefile.am index a03439c75f..3dc6b3b5a1 100644 --- a/src/bin/resolver/tests/Makefile.am +++ b/src/bin/resolver/tests/Makefile.am @@ -37,6 +37,8 @@ run_unittests_LDADD += $(top_builddir)/src/lib/cc/libcc.la run_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la run_unittests_LDADD += $(top_builddir)/src/lib/xfr/libxfr.la run_unittests_LDADD += $(top_builddir)/src/lib/log/liblog.la +run_unittests_LDADD += $(top_builddir)/src/lib/cache/libcache.la +run_unittests_LDADD += $(top_builddir)/src/lib/nsas/libnsas.la # Note the ordering matters: -Wno-... must follow -Wextra (defined in # B10_CXXFLAGS diff --git a/src/lib/asiolink/Makefile.am b/src/lib/asiolink/Makefile.am index 868fde526f..2c76fd9ecd 100644 --- a/src/lib/asiolink/Makefile.am +++ b/src/lib/asiolink/Makefile.am @@ -43,3 +43,5 @@ endif libasiolink_la_CPPFLAGS = $(AM_CPPFLAGS) libasiolink_la_LIBADD = $(top_builddir)/src/lib/log/liblog.la libasiolink_la_LIBADD += $(top_builddir)/src/lib/resolve/libresolve.la +libasiolink_la_LIBADD += $(top_builddir)/src/lib/cache/libcache.la +libasiolink_la_LIBADD += $(top_builddir)/src/lib/nsas/libnsas.la diff --git a/src/lib/asiolink/recursive_query.cc b/src/lib/asiolink/recursive_query.cc index 9f814b0c91..a5e3d79c73 100644 --- a/src/lib/asiolink/recursive_query.cc +++ b/src/lib/asiolink/recursive_query.cc @@ -32,9 +32,12 @@ #include #include +#include #include +#include + using isc::log::dlog; using namespace isc::dns; @@ -123,7 +126,7 @@ private: asio::deadline_timer lookup_timer; size_t queries_out_; - + // If we timed out ourselves (lookup timeout), stop issuing queries bool done_; @@ -132,6 +135,9 @@ private: // answer if we do find one later (or if we have a lookup_timeout) bool answer_sent_; + // Reference to our cache + isc::cache::ResolverCache& cache_; + // (re)send the query to the server. void send() { const int uc = upstream_->size(); @@ -291,7 +297,8 @@ public: OutputBufferPtr buffer, isc::resolve::ResolverInterface::CallbackPtr cb, int query_timeout, int client_timeout, int lookup_timeout, - unsigned retries) : + unsigned retries, + isc::cache::ResolverCache& cache) : io_(io), question_(question), answer_message_(answer_message), @@ -306,7 +313,8 @@ public: lookup_timer(io), queries_out_(0), done_(false), - answer_sent_(false) + answer_sent_(false), + cache_(cache) { // Setup the timer to stop trying (lookup_timeout) if (lookup_timeout >= 0) { @@ -366,6 +374,11 @@ public: // until that one comes back to us) done_ = true; if (resume && !answer_sent_) { + // Store the answer we found in our cache + std::cout << "[XX] caching our answer:" << std::endl; + std::cout << answer_message_->toText(); + cache_.update(*answer_message_); + std::cout << "[XX] done caching our answer" << std::endl; resolvercallback_->success(answer_message_); } else { resolvercallback_->failure(); @@ -424,11 +437,26 @@ RecursiveQuery::resolve(const QuestionPtr& question, MessagePtr answer_message(new Message(Message::RENDER)); OutputBufferPtr buffer(new OutputBuffer(0)); - - // It will delete itself when it is done - new RunningQuery(io, *question, answer_message, upstream_, - upstream_root_, buffer, callback, query_timeout_, - client_timeout_, lookup_timeout_, retries_); + + // TODO: general 'prepareinitialanswer' + answer_message->setOpcode(isc::dns::Opcode::QUERY()); + answer_message->addQuestion(question); + dlog("Try out cache first (direct call to resolve)"); + // First try to see if we have something cached in the messagecache + if (cache_.lookup(question->getName(), question->getType(), + question->getClass(), *answer_message)) { + dlog("Message found in cache, returning that"); + // TODO: err, should cache set rcode as well? + answer_message->setRcode(Rcode::NOERROR()); + callback->success(answer_message); + } else { + dlog("Message not found in cache, starting recursive query"); + // It will delete itself when it is done + new RunningQuery(io, *question, answer_message, upstream_, + upstream_root_, buffer, callback, query_timeout_, + client_timeout_, lookup_timeout_, retries_, + cache_); + } } void @@ -445,11 +473,26 @@ RecursiveQuery::resolve(const Question& question, isc::resolve::ResolverInterface::CallbackPtr crs( new isc::resolve::ResolverCallbackServer(server)); + + // TODO: general 'prepareinitialanswer' + answer_message->setOpcode(isc::dns::Opcode::QUERY()); + answer_message->addQuestion(question); - // It will delete itself when it is done - new RunningQuery(io, question, answer_message, upstream_, upstream_root_, - buffer, crs, query_timeout_, client_timeout_, - lookup_timeout_, retries_); + // First try to see if we have something cached in the messagecache + dlog("Try out cache first (started by incoming event)"); + if (cache_.lookup(question.getName(), question.getType(), + question.getClass(), *answer_message)) { + dlog("Message found in cache, returning that"); + // TODO: err, should cache set rcode as well? + answer_message->setRcode(Rcode::NOERROR()); + crs->success(answer_message); + } else { + dlog("Message not found in cache, starting recursive query"); + // It will delete itself when it is done + new RunningQuery(io, question, answer_message, upstream_, upstream_root_, + buffer, crs, query_timeout_, client_timeout_, + lookup_timeout_, retries_, cache_); + } } diff --git a/src/lib/asiolink/recursive_query.h b/src/lib/asiolink/recursive_query.h index 0660c0904a..6ef0069483 100644 --- a/src/lib/asiolink/recursive_query.h +++ b/src/lib/asiolink/recursive_query.h @@ -18,6 +18,7 @@ #include #include #include +#include namespace asiolink { /// \brief The \c RecursiveQuery class provides a layer of abstraction around @@ -107,6 +108,9 @@ private: int client_timeout_; int lookup_timeout_; unsigned retries_; + // Cache. TODO: I think we want this initialized in Resolver class, + // not here + isc::cache::ResolverCache cache_; }; } // namespace asiolink diff --git a/src/lib/asiolink/tests/Makefile.am b/src/lib/asiolink/tests/Makefile.am index 7fe268e5e9..619221c07c 100644 --- a/src/lib/asiolink/tests/Makefile.am +++ b/src/lib/asiolink/tests/Makefile.am @@ -33,6 +33,8 @@ run_unittests_LDADD += $(top_builddir)/src/lib/dns/libdns++.la run_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la run_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libasiolink.la run_unittests_LDADD += $(top_builddir)/src/lib/log/liblog.la +run_unittests_LDADD += $(top_builddir)/src/lib/cache/libcache.la +run_unittests_LDADD += $(top_builddir)/src/lib/nsas/libnsas.la # Note: the ordering matters: -Wno-... must follow -Wextra (defined in # B10_CXXFLAGS) run_unittests_CXXFLAGS = $(AM_CXXFLAGS) diff --git a/src/lib/asiolink/udp_server.cc b/src/lib/asiolink/udp_server.cc index 9a18d7611b..3331cd191d 100644 --- a/src/lib/asiolink/udp_server.cc +++ b/src/lib/asiolink/udp_server.cc @@ -28,6 +28,8 @@ #include +#include + using namespace asio; using asio::ip::udp; using asio::ip::tcp; diff --git a/src/lib/cache/message_cache.cc b/src/lib/cache/message_cache.cc index 65821994b6..5edf81c683 100644 --- a/src/lib/cache/message_cache.cc +++ b/src/lib/cache/message_cache.cc @@ -45,6 +45,7 @@ MessageCache::lookup(const isc::dns::Name& qname, isc::dns::Message& response) { std::string entry_name = genCacheEntryName(qname, qtype); + std::cout << "[XX] MESSAGECACHE LOOKUP: " << entry_name << std::endl; HashKey entry_key = HashKey(entry_name, RRClass(message_class_)); MessageEntryPtr msg_entry = message_table_.get(entry_key); if(msg_entry) { @@ -59,6 +60,7 @@ bool MessageCache::update(const Message& msg) { QuestionIterator iter = msg.beginQuestion(); std::string entry_name = genCacheEntryName((*iter)->getName(), (*iter)->getType()); + std::cout << "[XX] MESSAGECACHE UDPATE: " << entry_name << std::endl; HashKey entry_key = HashKey(entry_name, RRClass(message_class_)); // The simplest way to update is removing the old message entry directly. diff --git a/src/lib/cache/resolver_cache.cc b/src/lib/cache/resolver_cache.cc index 7ebdb4f570..c9183b069d 100644 --- a/src/lib/cache/resolver_cache.cc +++ b/src/lib/cache/resolver_cache.cc @@ -201,8 +201,8 @@ ResolverCache::lookupClosestRRset(const isc::dns::Name& qname, bool ResolverCache::update(const isc::dns::Message& msg) { - QuestionIterator iter = msg.beginQuestion(); + RRClass c = (*iter)->getClass(); ResolverClassCache* cc = getClassCache((*iter)->getClass()); if (cc) { return (cc->update(msg)); diff --git a/src/lib/cache/rrset_cache.h b/src/lib/cache/rrset_cache.h index d082b3c1a4..3dda239fad 100644 --- a/src/lib/cache/rrset_cache.h +++ b/src/lib/cache/rrset_cache.h @@ -17,7 +17,7 @@ #ifndef __RRSET_CACHE_H #define __RRSET_CACHE_H -#include +#include #include #include From f59af8201a12342a2779440a4c7afed1b51fef7f Mon Sep 17 00:00:00 2001 From: Jelte Jansen Date: Thu, 17 Feb 2011 10:05:11 +0100 Subject: [PATCH 2/8] [trac491] Also cache intermediate results Added some extensive dlog() calls for testing while we are building this --- src/lib/asiolink/recursive_query.cc | 37 ++++++++++++++++++++++++----- src/lib/cache/message_cache.cc | 4 +++- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/lib/asiolink/recursive_query.cc b/src/lib/asiolink/recursive_query.cc index a5e3d79c73..8cbfa49ec5 100644 --- a/src/lib/asiolink/recursive_query.cc +++ b/src/lib/asiolink/recursive_query.cc @@ -138,6 +138,24 @@ private: // Reference to our cache isc::cache::ResolverCache& cache_; + // perform a single lookup; first we check the cache to see + // if we have a response for our query stored already. if + // so, call handlerecursiveresponse(), if not, we call send() + void doLookup() { + dlog("doLookup: try cache"); + Message cached_message(Message::RENDER); + cached_message.addQuestion(question_); + cached_message.setOpcode(Opcode::QUERY()); + if (cache_.lookup(question_.getName(), question_.getType(), + question_.getClass(), cached_message)) { + dlog("Message found in cache, returning that"); + handleRecursiveAnswer(cached_message); + } else { + send(); + } + + } + // (re)send the query to the server. void send() { const int uc = upstream_->size(); @@ -219,7 +237,7 @@ private: question_.getType()); dlog("Following CNAME chain to " + question_.toText()); - send(); + doLookup(); return false; break; case isc::resolve::ResponseClassifier::NXDOMAIN: @@ -256,6 +274,10 @@ private: } if (found_ns_address) { // next resolver round + // we do NOT use doLookup() here, but send() (i.e. we + // skip the cache), since if we had the final answer + // instead of a delegation cached, we would have been + // there by now. send(); return false; } else { @@ -336,7 +358,7 @@ public: setZoneServersToRoot(); } - send(); + doLookup(); } void setZoneServersToRoot() { @@ -375,10 +397,10 @@ public: done_ = true; if (resume && !answer_sent_) { // Store the answer we found in our cache - std::cout << "[XX] caching our answer:" << std::endl; - std::cout << answer_message_->toText(); - cache_.update(*answer_message_); - std::cout << "[XX] done caching our answer" << std::endl; + //std::cout << "[XX] caching our answer:" << std::endl; + //std::cout << answer_message_->toText(); + //cache_.update(*answer_message_); + //std::cout << "[XX] done caching our answer" << std::endl; resolvercallback_->success(answer_message_); } else { resolvercallback_->failure(); @@ -405,6 +427,9 @@ public: InputBuffer ibuf(buffer_->getData(), buffer_->getLength()); incoming.fromWire(ibuf); + // let's first dunk it into our cache + cache_.update(incoming); + if (upstream_->size() == 0 && incoming.getRcode() == Rcode::NOERROR()) { done_ = handleRecursiveAnswer(incoming); diff --git a/src/lib/cache/message_cache.cc b/src/lib/cache/message_cache.cc index 5edf81c683..3e447a955e 100644 --- a/src/lib/cache/message_cache.cc +++ b/src/lib/cache/message_cache.cc @@ -60,7 +60,9 @@ bool MessageCache::update(const Message& msg) { QuestionIterator iter = msg.beginQuestion(); std::string entry_name = genCacheEntryName((*iter)->getName(), (*iter)->getType()); - std::cout << "[XX] MESSAGECACHE UDPATE: " << entry_name << std::endl; + std::cout << "[XX] MESSAGECACHE UPDATE: " << entry_name << std::endl; + std::cout << "[XX] FOR MESSAGE:" << std::endl; + std::cout << msg.toText(); HashKey entry_key = HashKey(entry_name, RRClass(message_class_)); // The simplest way to update is removing the old message entry directly. From 949f642c90ab8ad7f0f02c35e383b9eb9e65792a Mon Sep 17 00:00:00 2001 From: Jelte Jansen Date: Thu, 17 Feb 2011 10:31:09 +0100 Subject: [PATCH 3/8] [trac491] added isc::resolve::initResponseMessage() functions --- src/bin/resolver/resolver.cc | 2 ++ src/lib/asiolink/recursive_query.cc | 13 +++---------- src/lib/resolve/resolve.cc | 19 ++++++++++++++++++ src/lib/resolve/resolve.h | 30 +++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/bin/resolver/resolver.cc b/src/bin/resolver/resolver.cc index c635fc9a16..74eddfd44b 100644 --- a/src/bin/resolver/resolver.cc +++ b/src/bin/resolver/resolver.cc @@ -182,6 +182,8 @@ public: MessagePtr message_; }; + +// TODO: REMOVE, USE isc::resolve::MakeErrorMessage? void makeErrorMessage(MessagePtr message, OutputBufferPtr buffer, const Rcode& rcode) diff --git a/src/lib/asiolink/recursive_query.cc b/src/lib/asiolink/recursive_query.cc index 8cbfa49ec5..72f8a67d97 100644 --- a/src/lib/asiolink/recursive_query.cc +++ b/src/lib/asiolink/recursive_query.cc @@ -144,8 +144,7 @@ private: void doLookup() { dlog("doLookup: try cache"); Message cached_message(Message::RENDER); - cached_message.addQuestion(question_); - cached_message.setOpcode(Opcode::QUERY()); + isc::resolve::initResponseMessage(question_, cached_message); if (cache_.lookup(question_.getName(), question_.getType(), question_.getClass(), cached_message)) { dlog("Message found in cache, returning that"); @@ -396,11 +395,6 @@ public: // until that one comes back to us) done_ = true; if (resume && !answer_sent_) { - // Store the answer we found in our cache - //std::cout << "[XX] caching our answer:" << std::endl; - //std::cout << answer_message_->toText(); - //cache_.update(*answer_message_); - //std::cout << "[XX] done caching our answer" << std::endl; resolvercallback_->success(answer_message_); } else { resolvercallback_->failure(); @@ -461,11 +455,10 @@ RecursiveQuery::resolve(const QuestionPtr& question, asio::io_service& io = dns_service_.get_io_service(); MessagePtr answer_message(new Message(Message::RENDER)); + isc::resolve::initResponseMessage(*question, *answer_message); + OutputBufferPtr buffer(new OutputBuffer(0)); - // TODO: general 'prepareinitialanswer' - answer_message->setOpcode(isc::dns::Opcode::QUERY()); - answer_message->addQuestion(question); dlog("Try out cache first (direct call to resolve)"); // First try to see if we have something cached in the messagecache if (cache_.lookup(question->getName(), question->getType(), diff --git a/src/lib/resolve/resolve.cc b/src/lib/resolve/resolve.cc index 312c89d57b..c2bbe5ce7c 100644 --- a/src/lib/resolve/resolve.cc +++ b/src/lib/resolve/resolve.cc @@ -14,6 +14,9 @@ #include +#include +#include + using namespace isc::dns; namespace { @@ -44,6 +47,22 @@ makeErrorMessage(MessagePtr answer_message, answer_message->setRcode(error_code); } +void initResponseMessage(const isc::dns::Message& query_message, + isc::dns::Message& response_message) +{ + response_message.setOpcode(query_message.getOpcode()); + response_message.setQid(query_message.getQid()); + response_message.appendSection(Message::SECTION_QUESTION, + query_message); +} + +void initResponseMessage(const isc::dns::Question& question, + isc::dns::Message& response_message) +{ + response_message.setOpcode(isc::dns::Opcode::QUERY()); + response_message.addQuestion(question); +} + void copyResponseMessage(const Message& source, MessagePtr target) { target->setRcode(source.getRcode()); diff --git a/src/lib/resolve/resolve.h b/src/lib/resolve/resolve.h index b08c30c2ff..944691d95b 100644 --- a/src/lib/resolve/resolve.h +++ b/src/lib/resolve/resolve.h @@ -42,6 +42,36 @@ namespace resolve { void makeErrorMessage(isc::dns::MessagePtr answer_message, const isc::dns::Rcode& error_code); + +/// \brief Initialize a response message +/// +/// Based on the given query message, this fills in the very +/// first details of the response (i.e. the Question section and +/// the Opcode). This allows for direct usage of makeErrorMessage(), +/// as well as ResolveCache.lookup(). +/// +/// \param query_message The query message to take the Question, Qid, +/// and Opcode from. +/// \param response_message The fresh response message to initialize +/// (must be type Message::RENDER) +void initResponseMessage(const isc::dns::Message& query_message, + isc::dns::Message& response_message); + + +/// \brief Initialize a response message +/// +/// Based on the given question, this fills in the very +/// first details of the response (i.e. the Question section and the +/// Opcode Query). This allows for direct usage of makeErrorMessage(), +/// as well as ResolveCache.lookup(). +/// +/// \param question The question to place in the Question section +/// \param response_message The fresh response message to initialize +/// (must be type Message::RENDER) +void initResponseMessage(const isc::dns::Question& question, + isc::dns::Message& response_message); + + /// \brief Copies the parts relevant for a DNS response to the /// target message /// From cbc9f118a77f69878f4cebf383db39d984201bae Mon Sep 17 00:00:00 2001 From: Jelte Jansen Date: Thu, 17 Feb 2011 14:44:14 +0100 Subject: [PATCH 4/8] [trac491] update tests --- src/lib/resolve/resolve.h | 6 +++ src/lib/resolve/tests/Makefile.am | 1 + src/lib/resolve/tests/resolve_unittest.cc | 46 ++++++++++++++++++----- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/lib/resolve/resolve.h b/src/lib/resolve/resolve.h index 944691d95b..4544eb5a9f 100644 --- a/src/lib/resolve/resolve.h +++ b/src/lib/resolve/resolve.h @@ -50,6 +50,9 @@ void makeErrorMessage(isc::dns::MessagePtr answer_message, /// the Opcode). This allows for direct usage of makeErrorMessage(), /// as well as ResolveCache.lookup(). /// +/// Raises an isc::dns::InvalidMessageOperation if reponse_message is +/// not in RENDER mode. +/// /// \param query_message The query message to take the Question, Qid, /// and Opcode from. /// \param response_message The fresh response message to initialize @@ -65,6 +68,9 @@ void initResponseMessage(const isc::dns::Message& query_message, /// Opcode Query). This allows for direct usage of makeErrorMessage(), /// as well as ResolveCache.lookup(). /// +/// Raises an isc::dns::InvalidMessageOperation if reponse_message is +/// not in RENDER mode. +/// /// \param question The question to place in the Question section /// \param response_message The fresh response message to initialize /// (must be type Message::RENDER) diff --git a/src/lib/resolve/tests/Makefile.am b/src/lib/resolve/tests/Makefile.am index 3b5f41afbb..e669384f65 100644 --- a/src/lib/resolve/tests/Makefile.am +++ b/src/lib/resolve/tests/Makefile.am @@ -19,6 +19,7 @@ run_unittests_SOURCES += resolver_callback_unittest.cc run_unittests_SOURCES += response_classifier_unittest.cc run_unittests_LDADD = $(GTEST_LDADD) +run_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la run_unittests_LDADD += $(top_builddir)/src/lib/resolve/libresolve.la run_unittests_LDADD += $(top_builddir)/src/lib/dns/libdns++.la diff --git a/src/lib/resolve/tests/resolve_unittest.cc b/src/lib/resolve/tests/resolve_unittest.cc index fd68f16033..85d264dd12 100644 --- a/src/lib/resolve/tests/resolve_unittest.cc +++ b/src/lib/resolve/tests/resolve_unittest.cc @@ -122,21 +122,47 @@ TEST_F(ResolveHelperFunctionsTest, makeErrorMessageNonEmptyMessage) { } void -compareSections(const MessagePtr message_a, const MessagePtr message_b, +compareSections(const Message& message_a, const Message& message_b, Message::Section section) { - RRsetIterator rrs_a = message_a->beginSection(section); - RRsetIterator rrs_b = message_b->beginSection(section); - while (rrs_a != message_a->endSection(section) && - rrs_b != message_b->endSection(section) + RRsetIterator rrs_a = message_a.beginSection(section); + RRsetIterator rrs_b = message_b.beginSection(section); + while (rrs_a != message_a.endSection(section) && + rrs_b != message_b.endSection(section) ) { EXPECT_EQ(*rrs_a, *rrs_b); ++rrs_a; ++rrs_b; } // can't use EXPECT_EQ here, no eqHelper for endsection comparison - EXPECT_TRUE(rrs_a == message_a->endSection(section)); - EXPECT_TRUE(rrs_b == message_b->endSection(section)); + EXPECT_TRUE(rrs_a == message_a.endSection(section)); + EXPECT_TRUE(rrs_b == message_b.endSection(section)); +} + +TEST_F(ResolveHelperFunctionsTest, initResponseMessage) { + Message response_parse(Message::PARSE); + EXPECT_THROW(isc::resolve::initResponseMessage(*message_a_, + response_parse), + isc::dns::InvalidMessageOperation); + EXPECT_THROW(isc::resolve::initResponseMessage(*question_, + response_parse), + isc::dns::InvalidMessageOperation); + + Message response1(Message::RENDER); + isc::resolve::initResponseMessage(*message_a_, response1); + ASSERT_EQ(message_a_->getOpcode(), response1.getOpcode()); + ASSERT_EQ(message_a_->getQid(), response1.getQid()); + isc::dns::QuestionIterator qi = response1.beginQuestion(); + ASSERT_EQ(*question_, **qi); + ASSERT_TRUE(++qi == response1.endQuestion()); + + Message response2(Message::RENDER); + isc::resolve::initResponseMessage(*question_, response2); + ASSERT_EQ(Opcode::QUERY(), response2.getOpcode()); + ASSERT_EQ(0, response2.getQid()); + qi = response2.beginQuestion(); + ASSERT_EQ(*question_, **qi); + ASSERT_TRUE(++qi == response2.endQuestion()); } TEST_F(ResolveHelperFunctionsTest, copyAnswerMessage) { @@ -164,9 +190,9 @@ TEST_F(ResolveHelperFunctionsTest, copyAnswerMessage) { message_a_->getRRCount(Message::SECTION_ADDITIONAL)); - compareSections(message_a_, message_b_, Message::SECTION_ANSWER); - compareSections(message_a_, message_b_, Message::SECTION_AUTHORITY); - compareSections(message_a_, message_b_, Message::SECTION_ADDITIONAL); + compareSections(*message_a_, *message_b_, Message::SECTION_ANSWER); + compareSections(*message_a_, *message_b_, Message::SECTION_AUTHORITY); + compareSections(*message_a_, *message_b_, Message::SECTION_ADDITIONAL); } } // Anonymous namespace From 004816211d6b0f438713c1a4e167be8ac25a356f Mon Sep 17 00:00:00 2001 From: Jelte Jansen Date: Fri, 18 Feb 2011 14:34:22 +0100 Subject: [PATCH 5/8] [trac491] build cache and nsas before asiolink --- src/lib/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/Makefile.am b/src/lib/Makefile.am index 8ab93ba611..d5486a0f7d 100644 --- a/src/lib/Makefile.am +++ b/src/lib/Makefile.am @@ -1,2 +1,2 @@ SUBDIRS = exceptions dns cc config datasrc python xfr bench log \ - resolve asiolink testutils nsas cache + resolve nsas cache asiolink testutils From c9a06623e3e6041342046d52f502ce5e478ef29c Mon Sep 17 00:00:00 2001 From: Jelte Jansen Date: Fri, 18 Feb 2011 15:32:52 +0100 Subject: [PATCH 6/8] [trac491] also update cache with our full answer --- src/lib/asiolink/recursive_query.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/lib/asiolink/recursive_query.cc b/src/lib/asiolink/recursive_query.cc index 72f8a67d97..ed923a6d55 100644 --- a/src/lib/asiolink/recursive_query.cc +++ b/src/lib/asiolink/recursive_query.cc @@ -395,6 +395,13 @@ public: // until that one comes back to us) done_ = true; if (resume && !answer_sent_) { + // If we have a full successful answer, let's store that + // as well + // (note: we can either do this or only cache answers + // we receive, but in that case we'd need to re-do all + // answer processing, e.g. cname handling etc) + cache_.update(*answer_message_); + resolvercallback_->success(answer_message_); } else { resolvercallback_->failure(); From d22c96099aacc094be00c1fe51433d4b39b9656f Mon Sep 17 00:00:00 2001 From: Jelte Jansen Date: Tue, 22 Feb 2011 10:23:48 +0100 Subject: [PATCH 7/8] [trac491] review comments, the smaller issues --- src/bin/resolver/resolver.cc | 8 ++------ src/lib/asiolink/recursive_query.cc | 1 + src/lib/cache/message_cache.cc | 3 --- src/lib/cache/resolver_cache.cc | 1 - src/lib/resolve/resolve.cc | 1 + src/lib/resolve/resolve.h | 4 ++-- 6 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/bin/resolver/resolver.cc b/src/bin/resolver/resolver.cc index 74eddfd44b..95a417d285 100644 --- a/src/bin/resolver/resolver.cc +++ b/src/bin/resolver/resolver.cc @@ -265,12 +265,8 @@ public: answer_message->setHeaderFlag(Message::HEADERFLAG_QR); answer_message->setHeaderFlag(Message::HEADERFLAG_RA); - if (rd) { - answer_message->setHeaderFlag(Message::HEADERFLAG_RD); - } - if (cd) { - answer_message->setHeaderFlag(Message::HEADERFLAG_CD); - } + answer_message->setHeaderFlag(Message::HEADERFLAG_RD, rd); + answer_message->setHeaderFlag(Message::HEADERFLAG_CD, cd); // Now we can clear the buffer and render the new message into it buffer->clear(); diff --git a/src/lib/asiolink/recursive_query.cc b/src/lib/asiolink/recursive_query.cc index ed923a6d55..7381c88a3c 100644 --- a/src/lib/asiolink/recursive_query.cc +++ b/src/lib/asiolink/recursive_query.cc @@ -268,6 +268,7 @@ private: // TODO should use NSAS zone_servers_.push_back(addr_t(addr_str, 53)); found_ns_address = true; + break; } } } diff --git a/src/lib/cache/message_cache.cc b/src/lib/cache/message_cache.cc index 3e447a955e..f8e59102fd 100644 --- a/src/lib/cache/message_cache.cc +++ b/src/lib/cache/message_cache.cc @@ -45,7 +45,6 @@ MessageCache::lookup(const isc::dns::Name& qname, isc::dns::Message& response) { std::string entry_name = genCacheEntryName(qname, qtype); - std::cout << "[XX] MESSAGECACHE LOOKUP: " << entry_name << std::endl; HashKey entry_key = HashKey(entry_name, RRClass(message_class_)); MessageEntryPtr msg_entry = message_table_.get(entry_key); if(msg_entry) { @@ -60,8 +59,6 @@ bool MessageCache::update(const Message& msg) { QuestionIterator iter = msg.beginQuestion(); std::string entry_name = genCacheEntryName((*iter)->getName(), (*iter)->getType()); - std::cout << "[XX] MESSAGECACHE UPDATE: " << entry_name << std::endl; - std::cout << "[XX] FOR MESSAGE:" << std::endl; std::cout << msg.toText(); HashKey entry_key = HashKey(entry_name, RRClass(message_class_)); diff --git a/src/lib/cache/resolver_cache.cc b/src/lib/cache/resolver_cache.cc index c9183b069d..04b6346074 100644 --- a/src/lib/cache/resolver_cache.cc +++ b/src/lib/cache/resolver_cache.cc @@ -202,7 +202,6 @@ ResolverCache::lookupClosestRRset(const isc::dns::Name& qname, bool ResolverCache::update(const isc::dns::Message& msg) { QuestionIterator iter = msg.beginQuestion(); - RRClass c = (*iter)->getClass(); ResolverClassCache* cc = getClassCache((*iter)->getClass()); if (cc) { return (cc->update(msg)); diff --git a/src/lib/resolve/resolve.cc b/src/lib/resolve/resolve.cc index c2bbe5ce7c..f741121464 100644 --- a/src/lib/resolve/resolve.cc +++ b/src/lib/resolve/resolve.cc @@ -52,6 +52,7 @@ void initResponseMessage(const isc::dns::Message& query_message, { response_message.setOpcode(query_message.getOpcode()); response_message.setQid(query_message.getQid()); + assert(response_message.getRRCount(Message::SECTION_QUESTION) == 0); response_message.appendSection(Message::SECTION_QUESTION, query_message); } diff --git a/src/lib/resolve/resolve.h b/src/lib/resolve/resolve.h index 4544eb5a9f..550b6204e4 100644 --- a/src/lib/resolve/resolve.h +++ b/src/lib/resolve/resolve.h @@ -56,7 +56,7 @@ void makeErrorMessage(isc::dns::MessagePtr answer_message, /// \param query_message The query message to take the Question, Qid, /// and Opcode from. /// \param response_message The fresh response message to initialize -/// (must be type Message::RENDER) +/// (must be in RENDER mode) void initResponseMessage(const isc::dns::Message& query_message, isc::dns::Message& response_message); @@ -73,7 +73,7 @@ void initResponseMessage(const isc::dns::Message& query_message, /// /// \param question The question to place in the Question section /// \param response_message The fresh response message to initialize -/// (must be type Message::RENDER) +/// (must be in RENDER mode) void initResponseMessage(const isc::dns::Question& question, isc::dns::Message& response_message); From dee286dad1ba1b47d89659572fc2ce8fd1a94af6 Mon Sep 17 00:00:00 2001 From: Jelte Jansen Date: Tue, 22 Feb 2011 10:59:25 +0100 Subject: [PATCH 8/8] [trac491] rest of review comments updated a comment description, and moved caching of answers to after the response classifier has run --- src/lib/asiolink/recursive_query.cc | 30 +++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/lib/asiolink/recursive_query.cc b/src/lib/asiolink/recursive_query.cc index 7381c88a3c..33b1391ddd 100644 --- a/src/lib/asiolink/recursive_query.cc +++ b/src/lib/asiolink/recursive_query.cc @@ -207,6 +207,11 @@ private: question_, incoming, cname_target, cname_count_, true); bool found_ns_address = false; + + // If the packet is OK, store it in the cache + if (!isc::resolve::ResponseClassifier::error(category)) { + cache_.update(incoming); + } switch (category) { case isc::resolve::ResponseClassifier::ANSWER: @@ -396,11 +401,23 @@ public: // until that one comes back to us) done_ = true; if (resume && !answer_sent_) { - // If we have a full successful answer, let's store that - // as well - // (note: we can either do this or only cache answers - // we receive, but in that case we'd need to re-do all - // answer processing, e.g. cname handling etc) + // There are two types of messages we could store in the + // cache; + // 1. answers to our fetches from authoritative servers, + // exactly as we receive them, and + // 2. answers to queries we received from clients, which + // have received additional processing (following CNAME + // chains, for instance) + // + // Doing only the first would mean we would have to re-do + // processing when we get data from our cache, and doing + // only the second would miss out on the side-effect of + // having nameserver data in our cache. + // + // So right now we do both. Since the cache (currently) + // stores Messages on their question section only, this + // does mean that we overwrite the messages we stored in + // the previous iteration if we are following a delegation. cache_.update(*answer_message_); resolvercallback_->success(answer_message_); @@ -429,9 +446,6 @@ public: InputBuffer ibuf(buffer_->getData(), buffer_->getLength()); incoming.fromWire(ibuf); - // let's first dunk it into our cache - cache_.update(incoming); - if (upstream_->size() == 0 && incoming.getRcode() == Rcode::NOERROR()) { done_ = handleRecursiveAnswer(incoming);