[trac497] return actual error messages on resolver failures

client timeout and auth errors now result in SERVFAIL being sent back
added copySection and moved copyAnswerMessage to resolve.cc (no need to
have that in asiolink)

src/lib/asiolink/asiolink.cc

@@ -50,44 +50,6 @@ using namespace isc::dns;
 using isc::log::dlog;
 using namespace boost;
 
-// Is this something we can use in libdns++?
-namespace {
-    // TODO: remove
-    class SectionInserter {
-    public:
-        SectionInserter(MessagePtr message, const Message::Section sect) :
-            message_(message), section_(sect)
-        {}
-        void operator()(const RRsetPtr rrset) {
-            message_->addRRset(section_, rrset, true);
-        }
-        MessagePtr message_;
-        const Message::Section section_;
-    };
-
-
-    // TODO: move to resolve.cc
-    /// \brief Copies the parts relevant for a DNS answer to the
-    /// target message
-    ///
-    /// This adds all the RRsets in the answer, authority and
-    /// additional sections to the target, as well as the response
-    /// code
-    void copyAnswerMessage(const Message& source, MessagePtr target) {
-        target->setRcode(source.getRcode());
-
-        for_each(source.beginSection(Message::SECTION_ANSWER),
-                 source.endSection(Message::SECTION_ANSWER),
-                 SectionInserter(target, Message::SECTION_ANSWER));
-        for_each(source.beginSection(Message::SECTION_AUTHORITY),
-                 source.endSection(Message::SECTION_AUTHORITY),
-                 SectionInserter(target, Message::SECTION_AUTHORITY));
-        for_each(source.beginSection(Message::SECTION_ADDITIONAL),
-                 source.endSection(Message::SECTION_ADDITIONAL),
-                 SectionInserter(target, Message::SECTION_ADDITIONAL));
-    }
-}
-
 namespace asiolink {
 
 typedef pair<string, uint16_t> addr_t;
@@ -396,6 +358,11 @@ private:
     // If we timed out ourselves (lookup timeout), stop issuing queries
     bool done_;
 
+    // If we have a client timeout, we send back an answer, but don't
+    // stop. We use this variable to make sure we don't send another
+    // answer if we do find one later (or if we have a lookup_timeout)
+    bool answer_sent_;
+
     // (re)send the query to the server.
     void send() {
         const int uc = upstream_->size();
@@ -438,6 +405,9 @@ private:
     // returns false if we are not done
     bool handleRecursiveAnswer(const Message& incoming) {
         dlog("Handle response");
+        // In case we get a CNAME, we store the target
+        // here (classify() will set it when it walks through
+        // the cname chain to verify it).
         Name cname_target(question_.getName());
         
         isc::resolve::ResponseClassifier::Category category =
@@ -450,38 +420,41 @@ private:
         case isc::resolve::ResponseClassifier::ANSWER:
         case isc::resolve::ResponseClassifier::ANSWERCNAME:
             // Done. copy and return.
-            copyAnswerMessage(incoming, answer_message_);
+            isc::resolve::copyAnswerMessage(incoming, answer_message_);
             return true;
             break;
         case isc::resolve::ResponseClassifier::CNAME:
             dlog("Response is CNAME!");
-
+            // (unfinished) CNAME. We set our question_ to the CNAME
+            // target, then start over at the beginning (for now, that
+            // is, we reset our 'current servers' to the root servers).
             if (cname_count_ >= RESOLVER_MAX_CNAME_CHAIN) {
                 // just give up
-                // TODO: make SERVFAIL? clear currently read answers?
-                // just copy for now.
                 dlog("CNAME chain too long");
                 isc::resolve::makeErrorMessage(answer_message_,
                                                Rcode::SERVFAIL());
                 return true;
             }
 
-            for_each(incoming.beginSection(Message::SECTION_ANSWER),
-                     incoming.endSection(Message::SECTION_ANSWER),
-                     SectionInserter(answer_message_, Message::SECTION_ANSWER));
+            isc::resolve::copySection(incoming, answer_message_,
+                                      Message::SECTION_ANSWER);
             setZoneServersToRoot();
 
-            question_ = Question(cname_target, question_.getClass(), question_.getType());
+            question_ = Question(cname_target, question_.getClass(),
+                                 question_.getType());
 
             dlog("Following CNAME chain to " + question_.toText());
             send();
             return false;
             break;
         case isc::resolve::ResponseClassifier::NXDOMAIN:
-            copyAnswerMessage(incoming, answer_message_);
+            // NXDOMAIN, just copy and return.
+            isc::resolve::copyAnswerMessage(incoming, answer_message_);
             return true;
             break;
         case isc::resolve::ResponseClassifier::REFERRAL:
+            // Referral. For now we just take the first glue address
+            // we find and continue with that
             zone_servers_.clear();
 
             for (RRsetIterator rrsi = incoming.beginSection(Message::SECTION_ADDITIONAL);
@@ -513,7 +486,7 @@ private:
             } else {
                 dlog("[XX] no ready-made addresses in additional. need nsas.");
                 // this will result in answering with the delegation. oh well
-                copyAnswerMessage(incoming, answer_message_);
+                isc::resolve::copyAnswerMessage(incoming, answer_message_);
                 return true;
             }
             break;
@@ -529,70 +502,17 @@ private:
         case isc::resolve::ResponseClassifier::OPCODE:
         case isc::resolve::ResponseClassifier::RCODE:
         case isc::resolve::ResponseClassifier::TRUNCATED:
-            // TODO should create SERVFAIL?
+            // Should we try a different server rather than SERVFAIL?
+            isc::resolve::makeErrorMessage(answer_message_,
+                                           Rcode::SERVFAIL());
             return true;
             break;
         }
-        // should not be reached
+        // should not be reached. assert here?
         dlog("[FATAL] unreachable code");
         return true;
     }
     
-    bool oldhandleRecursiveAnswer(const Message& incoming) {
-        if (incoming.getRRCount(Message::SECTION_ANSWER) > 0) {
-            dlog("Got final result, copying answer.");
-            copyAnswerMessage(incoming, answer_message_);
-            return true;
-        } else {
-            dlog("Got delegation, continuing");
-            // ok we need to do some more processing.
-            // the ns list should contain all nameservers
-            // while the additional may contain addresses for
-            // them.
-            // this needs to tie into NSAS of course
-            // for this very first mockup, hope there is an
-            // address in additional and just use that
-
-            // send query to the addresses in the delegation
-            bool found_ns_address = false;
-            zone_servers_.clear();
-
-            for (RRsetIterator rrsi = incoming.beginSection(Message::SECTION_ADDITIONAL);
-                 rrsi != incoming.endSection(Message::SECTION_ADDITIONAL) && !found_ns_address;
-                 rrsi++) {
-                ConstRRsetPtr rrs = *rrsi;
-                if (rrs->getType() == RRType::A()) {
-                    // found address
-                    RdataIteratorPtr rdi = rrs->getRdataIterator();
-                    // just use the first for now
-                    if (!rdi->isLast()) {
-                        std::string addr_str = rdi->getCurrent().toText();
-                        dlog("[XX] first address found: " + addr_str);
-                        // now we have one address, simply
-                        // resend that exact same query
-                        // to that address and yield, when it
-                        // returns, loop again.
-                        
-                        // should use NSAS
-                        zone_servers_.push_back(addr_t(addr_str, 53));
-                        found_ns_address = true;
-                    }
-                }
-            }
-            if (found_ns_address) {
-                // next resolver round
-                send();
-                return false;
-            } else {
-                dlog("[XX] no ready-made addresses in additional. need nsas.");
-                // this will result in answering with the delegation. oh well
-                copyAnswerMessage(incoming, answer_message_);
-                return true;
-            }
-        }
-    }
-    
-
 public:
     RunningQuery(asio::io_service& io, const Question &question,
         MessagePtr answer_message, shared_ptr<AddressVector> upstream,
@@ -614,7 +534,8 @@ public:
         client_timer(io),
         lookup_timer(io),
         queries_out_(0),
-        done_(false)
+        done_(false),
+        answer_sent_(false)
     {
         // Setup the timer to stop trying (lookup_timeout)
         if (lookup_timeout >= 0) {
@@ -657,9 +578,12 @@ public:
         }
     }
     virtual void clientTimeout() {
-        // right now, just stop (should make SERVFAIL and send that
-        // back, but not stop)
-        stop(false);
+        // Return a SERVFAIL, but do not stop until
+        // we have an answer or timeout ourselves
+        isc::resolve::makeErrorMessage(answer_message_,
+                                       Rcode::SERVFAIL());
+        resolvercallback_->success(answer_message_);
+        answer_sent_ = true;
     }
 
     virtual void stop(bool resume) {
@@ -671,7 +595,7 @@ public:
         // same goes if we have an outstanding query (can't delete
         // until that one comes back to us)
         done_ = true;
-        if (resume) {
+        if (resume && !answer_sent_) {
             resolvercallback_->success(answer_message_);
         } else {
             resolvercallback_->failure();
@@ -702,7 +626,7 @@ public:
                 incoming.getRcode() == Rcode::NOERROR()) {
                 done_ = handleRecursiveAnswer(incoming);
             } else {
-                copyAnswerMessage(incoming, answer_message_);
+                isc::resolve::copyAnswerMessage(incoming, answer_message_);
                 done_ = true;
             }
             

src/lib/asiolink/tests/asiolink_unittest.cc

@@ -520,6 +520,39 @@ protected:
             bool* done_;
     };
 
+    // This version of mock server just stops the io_service when it is resumed
+    // the second time. (Used in the clientTimeout test, where resume
+    // is called initially with the error answer, and later when the
+    // lookup times out, it is called without an answer to send back)
+    class MockServerStop2 : public MockServer {
+        public:
+            explicit MockServerStop2(IOService& io_service,
+                                     bool* done1, bool* done2) :
+                MockServer(io_service),
+                done1_(done1),
+                done2_(done2),
+                stopped_once_(false)
+            {}
+
+            void resume(const bool done) {
+                if (stopped_once_) {
+                    *done2_ = done;
+                    io_.stop();
+                } else {
+                    *done1_ = done;
+                    stopped_once_ = true;
+                }
+            }
+
+            DNSServer* clone() {
+                return (new MockServerStop2(*this));
+            }
+        private:
+            bool* done1_;
+            bool* done2_;
+            bool stopped_once_;
+    };
+
 private:
     class ASIOCallBack : public SimpleCallback {
     public:
@@ -809,8 +842,9 @@ TEST_F(ASIOLinkTest, forwardClientTimeout) {
     sock_ = createTestSocket();
 
     // Prepare the server
-    bool done(true);
-    MockServerStop server(*io_service_, &done);
+    bool done1(true);
+    bool done2(true);
+    MockServerStop2 server(*io_service_, &done1, &done2);
 
     MessagePtr answer(new Message(Message::RENDER));
 
@@ -822,7 +856,7 @@ TEST_F(ASIOLinkTest, forwardClientTimeout) {
     RecursiveQuery query(*dns_service_,
                          singleAddress(TEST_IPV4_ADDR, port),
                          singleAddress(TEST_IPV4_ADDR, port),
-                         50, 120, 1000, 3);
+                         50, 120, 1000, 4);
     Question question(Name("example.net"), RRClass::IN(), RRType::A());
     OutputBufferPtr buffer(new OutputBuffer(0));
     query.resolve(question, answer, buffer, &server);
@@ -833,17 +867,15 @@ TEST_F(ASIOLinkTest, forwardClientTimeout) {
     // we know it'll fail, so make it a shorter timeout
     int recv_options = setSocketTimeout(sock_, 1, 0);
 
-    // Try to read 5 times, should stop after 3 reads
+    // Try to read 5 times
     int num = 0;
     bool read_success = tryRead(sock_, recv_options, 5, &num);
 
-    // The query should fail (for resolver it should send back servfail,
-    // but currently, and perhaps for forwarder in general, the effect
-    // will be the same as on a lookup timeout, i.e. no answer is sent
-    // back)
-    EXPECT_FALSE(done);
-    EXPECT_EQ(3, num);
-    EXPECT_FALSE(read_success);
+    // The query should fail, but we should have kept on trying
+    EXPECT_TRUE(done1);
+    EXPECT_FALSE(done2);
+    EXPECT_EQ(5, num);
+    EXPECT_TRUE(read_success);
 }
 
 // If we set lookup timeout to lower than querytimeout*retries, we should

src/lib/resolve/resolve.cc

@@ -44,6 +44,25 @@ makeErrorMessage(MessagePtr answer_message,
     answer_message->setRcode(error_code);
 }
 
+void
+copySection(const Message& source, MessagePtr target,
+            Message::Section section)
+{
+    for_each(source.beginSection(section),
+             source.endSection(section),
+             SectionInserter(target, section));
+}
+
+void copyAnswerMessage(const Message& source, MessagePtr target) {
+
+    target->setRcode(source.getRcode());
+
+    copySection(source, target, Message::SECTION_ANSWER);
+    copySection(source, target, Message::SECTION_AUTHORITY);
+    copySection(source, target, Message::SECTION_ADDITIONAL);
+}
+
+
 } // namespace resolve
 } // namespace isc
 

src/lib/resolve/resolve.h

@@ -37,9 +37,28 @@ namespace resolve {
 /// \param answer_message The message to clear and place the error in
 /// \param question The question to add to the
 /// \param error_code The error Rcode
-void
-makeErrorMessage(isc::dns::MessagePtr answer_message,
-                 const isc::dns::Rcode::Rcode& error_code);
+void makeErrorMessage(isc::dns::MessagePtr answer_message,
+                      const isc::dns::Rcode::Rcode& error_code);
+
+/// \brief Copies all rrsets in the given section from source
+/// to target
+///
+/// \param source The source Message
+/// \param target The target MessagePtr
+/// \param section the section to copy
+void copySection(const isc::dns::Message& source,
+                 isc::dns::MessagePtr target,
+                 isc::dns::Message::Section section);
+
+/// \brief Copies the parts relevant for a DNS answer to the
+/// target message
+///
+/// This adds all the RRsets in the answer, authority and
+/// additional sections to the target, as well as the response
+/// code
+void copyAnswerMessage(const isc::dns::Message& source,
+                       isc::dns::MessagePtr target);
+
 
 } // namespace resolve
 } // namespace isc