[master] Merge branch 'trac491'

Conflicts:
	src/lib/asiolink/recursive_query.cc
	src/lib/asiolink/tests/Makefile.am

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)
 

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)

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)

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
 

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)
@@ -256,25 +258,16 @@ 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);
-        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);
 
-        vector<QuestionPtr> 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);

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

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

src/lib/asiolink/Makefile.am

@@ -47,3 +47,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

src/lib/asiolink/recursive_query.cc

@@ -26,8 +26,10 @@
 
 #include <dns/question.h>
 #include <dns/message.h>
+#include <dns/opcode.h>
 
 #include <resolve/resolve.h>
+#include <cache/resolver_cache.h>
 
 #include <asio.hpp>
 #include <asiolink/dns_service.h>
@@ -123,7 +125,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 +134,26 @@ 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_;
+
+    // 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);
+        isc::resolve::initResponseMessage(question_, cached_message);
+        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();
@@ -184,6 +206,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:
@@ -213,7 +240,7 @@ private:
                                  question_.getType());
 
             dlog("Following CNAME chain to " + question_.toText());
-            send();
+            doLookup();
             return false;
             break;
         case isc::resolve::ResponseClassifier::NXDOMAIN:
@@ -245,11 +272,16 @@ private:
                         // TODO should use NSAS
                         zone_servers_.push_back(addr_t(addr_str, 53));
                         found_ns_address = true;
+                        break;
                     }
                 }
             }
             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 {
@@ -291,7 +323,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 +339,8 @@ public:
         lookup_timer(io.get_io_service()),
         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) {
@@ -328,7 +362,7 @@ public:
             setZoneServersToRoot();
         }
 
-        send();
+        doLookup();
     }
 
     void setZoneServersToRoot() {
@@ -369,6 +403,26 @@ public:
         done_ = true;
         if (resume && !answer_sent_) {
             answer_sent_ = true;
+
+            // 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_);
         } else {
             resolvercallback_->failure();
@@ -426,12 +480,26 @@ RecursiveQuery::resolve(const QuestionPtr& question,
     IOService& io = dns_service_.getIOService();
 
     MessagePtr answer_message(new Message(Message::RENDER));
+    isc::resolve::initResponseMessage(*question, *answer_message);
+
     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_);
+
+    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
@@ -448,11 +516,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_);
+    }
 }
 
 

src/lib/asiolink/recursive_query.h

@@ -18,6 +18,7 @@
 #include <asiolink/dns_service.h>
 #include <asiolink/dns_server.h>
 #include <dns/buffer.h>
+#include <cache/resolver_cache.h>
 
 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

src/lib/asiolink/tests/Makefile.am

@@ -36,6 +36,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
 
 run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS) $(LOG4CXX_LDFLAGS)
 

src/lib/asiolink/udp_server.cc

@@ -28,6 +28,8 @@
 #include <asiolink/udp_server.h>
 #include <asiolink/udp_socket.h>
 
+#include <dns/opcode.h>
+
 using namespace asio;
 using asio::ip::udp;
 using isc::log::dlog;

src/lib/cache/message_cache.cc

@@ -57,6 +57,7 @@ bool
 MessageCache::update(const Message& msg) {
     QuestionIterator iter = msg.beginQuestion();
     std::string entry_name = genCacheEntryName((*iter)->getName(), (*iter)->getType());
+    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.

src/lib/cache/rrset_cache.h

@@ -15,7 +15,7 @@
 #ifndef __RRSET_CACHE_H
 #define __RRSET_CACHE_H
 
-#include <rrset_entry.h>
+#include <cache/rrset_entry.h>
 #include <nsas/hash_table.h>
 #include <nsas/lru_list.h>
 

src/lib/resolve/resolve.cc

@@ -14,6 +14,9 @@
 
 #include <resolve/resolve.h>
 
+#include <dns/message.h>
+#include <dns/opcode.h>
+
 using namespace isc::dns;
 
 namespace {
@@ -44,6 +47,23 @@ 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());
+    assert(response_message.getRRCount(Message::SECTION_QUESTION) == 0);
+    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());
 

src/lib/resolve/resolve.h

@@ -42,6 +42,42 @@ 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().
+///
+/// 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
+///                         (must be in RENDER mode)
+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().
+///
+/// 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 in RENDER mode)
+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
 ///

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
 

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