[trac497] use CNAME following code from response classifier

This adds two arguments to classify(), cname_target and cname_count;
which are modified if a CNAME chain is found (so we don't have to jump
through the packet twice with exactly the same code)

src/lib/asiolink/asiolink.cc

@@ -435,12 +435,13 @@ private:
     // returns false if we are not done
     bool handleRecursiveAnswer(const Message& incoming) {
         dlog("Handle response");
+        Name cname_target(question_.getName());
+        
         isc::resolve::ResponseClassifier::Category category =
-            isc::resolve::ResponseClassifier::classify(question_, incoming, true);
+            isc::resolve::ResponseClassifier::classify(
+                question_, incoming, cname_target, cname_count_, true);
 
         bool found_ns_address = false;
-        ConstRRsetPtr cname_rrs;
-        RdataIteratorPtr cname_rdatas;
 
         switch (category) {
         case isc::resolve::ResponseClassifier::ANSWER:
@@ -451,24 +452,6 @@ private:
             break;
         case isc::resolve::ResponseClassifier::CNAME:
             dlog("Response is CNAME!");
-            // add the current answer to our response,
-            // set the question to the cname target,
-            // and restart from the top
-            for_each(incoming.beginSection(Message::SECTION_ANSWER),
-                     incoming.endSection(Message::SECTION_ANSWER),
-                     SectionInserter(answer_message_, Message::SECTION_ANSWER));
-            setZoneServersToRoot();
-            // we need the target of the last CNAME
-            // assuming the cnames are in order (are they?)
-            dlog("[XX] walking through answers");
-            for (RRsetIterator rrsi = incoming.beginSection(Message::SECTION_ANSWER);
-                 rrsi != incoming.endSection(Message::SECTION_ANSWER) && !found_ns_address;
-                 rrsi++) {
-                if (*rrsi && (*rrsi)->getType() == isc::dns::RRType::CNAME()) {
-                    ++cname_count_;
-                    cname_rrs = *rrsi;
-                }
-            }
 
             if (cname_count_ >= RESOLVER_MAX_CNAME_CHAIN) {
                 // just give up
@@ -479,18 +462,12 @@ private:
                 return true;
             }
 
-            cname_rdatas = cname_rrs->getRdataIterator();
-            while (!cname_rdatas->isLast()) {
-                if (!cname_rdatas->isLast()) {
-                    // hmz, why does getCurrent() return a reference? can't use
-                    // ->rrtypeSpecific()
-                    question_ = isc::dns::Question(Name(
-                        cname_rdatas->getCurrent().toText()),
-                        question_.getClass(),
-                        question_.getType());
-                }
-                cname_rdatas->next();
-            }
+            for_each(incoming.beginSection(Message::SECTION_ANSWER),
+                     incoming.endSection(Message::SECTION_ANSWER),
+                     SectionInserter(answer_message_, Message::SECTION_ANSWER));
+            setZoneServersToRoot();
+
+            question_ = Question(cname_target, question_.getClass(), question_.getType());
 
             dlog("Following CNAME chain to " + question_.toText());
             send();

src/lib/resolve/response_classifier.cc

@@ -32,7 +32,9 @@ namespace resolve {
 // Classify the response in the "message" object.
 
 ResponseClassifier::Category ResponseClassifier::classify(
-    const Question& question, const Message& message, bool tcignore)
+    const Question& question, const Message& message, 
+    Name& cname_target, unsigned int& cname_count, bool tcignore
+    )
 {
     // Check header bits
     if (!message.getHeaderFlag(Message::HEADERFLAG_QR)) {
@@ -137,6 +139,9 @@ ResponseClassifier::Category ResponseClassifier::classify(
                 (question.getType() == RRType::ANY())) {
                 return (ANSWER);
             } else if (answer[0]->getType() == RRType::CNAME()) {
+                RdataIteratorPtr it = answer[0]->getRdataIterator();
+                cname_target = Name(it->getCurrent().toText());
+                ++cname_count;
                 return (CNAME);
             } else {
                 return (INVTYPE);
@@ -189,14 +194,16 @@ ResponseClassifier::Category ResponseClassifier::classify(
 
     vector<RRsetPtr> ansrrset(answer);
     vector<int> present(ansrrset.size(), 1);
-    return cnameChase(question.getName(), question.getType(), ansrrset, present,
-        ansrrset.size());
+    return cnameChase(question.getName(), question.getType(),
+        cname_target, cname_count,
+        ansrrset, present, ansrrset.size());
 }
 
 // Search the CNAME chain.
 ResponseClassifier::Category ResponseClassifier::cnameChase(
-    const Name& qname, const RRType& qtype, vector<RRsetPtr>& ansrrset,
-    vector<int>& present, size_t size)
+    const Name& qname, const RRType& qtype,
+    Name& cname_target, unsigned int& cname_count,
+    vector<RRsetPtr>& ansrrset, vector<int>& present, size_t size)
 {
     // Search through the vector of RRset pointers until we find one with the
     // right QNAME.
@@ -218,9 +225,10 @@ ResponseClassifier::Category ResponseClassifier::cnameChase(
                     present[i] = 0;
                     --size;
                     if (size == 0) {
+                        RdataIteratorPtr it = ansrrset[i]->getRdataIterator();
+                        cname_target = Name(it->getCurrent().toText());
                         return (CNAME);
-                    }
-                    else {
+                    } else {
                         if (ansrrset[i]->getRdataCount() != 1) {
 
                             // Multiple RDATA for a CNAME?  This is invalid.
@@ -230,8 +238,9 @@ ResponseClassifier::Category ResponseClassifier::cnameChase(
                         RdataIteratorPtr it = ansrrset[i]->getRdataIterator();
                         Name newname(it->getCurrent().toText());
 
-                        return cnameChase(newname, qtype, ansrrset, present,
-                            size);
+                        // Increase CNAME count, and continue
+                        return cnameChase(newname, qtype, cname_target,
+                            ++cname_count, ansrrset, present, size);
                     }
 
                 } else {

src/lib/resolve/response_classifier.h

@@ -94,13 +94,20 @@ public:
     ///
     /// \param question Question that was sent to the server
     /// \param message Pointer to the associated response from the server.
+    /// \param cname_target If the message contains an (unfinished) CNAME
+    ///                     chain, this Name will be replaced by the
+    ///                     target of the last CNAME in the chain
+    /// \param cname_count This unsigned int will be incremented with
+    ///                    the number of CNAMEs followed
     /// \param tcignore If set, the TC bit in a response packet is
     /// ignored.  Otherwise the error code TRUNCATED will be returned.  The
     /// only time this is likely to be used is in development where we are not
     /// going to fail over to TCP and will want to use what is returned, even
     /// if some of the response was lost.
     static Category classify(const isc::dns::Question& question,
-            const isc::dns::Message& message, bool tcignore = false);
+            const isc::dns::Message& message, 
+            isc::dns::Name& cname_target, unsigned int& cname_count,
+            bool tcignore = false);
 
 private:
     /// \brief Follow CNAMEs
@@ -132,10 +139,16 @@ private:
     /// management seemed high.  This solution imposes some additional loop
     /// cycles, but that should be minimal compared with the overhead of the
     /// memory management.
+    /// \param cname_target If the message contains an (unfinished) CNAME
+    ///                     chain, this Name will be replaced by the
+    ///                     target of the last CNAME in the chain
+    /// \param cname_count This unsigned int will be incremented with
+    ///                    the number of CNAMEs followed
     /// \param size Number of elements to check.  See description of \c present
     /// for details.
     static Category cnameChase(const isc::dns::Name& qname,
         const isc::dns::RRType& qtype,
+        isc::dns::Name& cname_target, unsigned int& cname_count,
         std::vector<isc::dns::RRsetPtr>& ansrrset, std::vector<int>& present,
         size_t size);
 };

src/lib/resolve/tests/response_classifier_unittest.cc

@@ -81,7 +81,9 @@ public:
         rrs_in_ns_(new RRset(Name("example.com"), RRClass::IN(),
             RRType::NS(), RRTTL(300))),
         rrs_in_txt_www(new RRset(Name("www.example.com"), RRClass::IN(),
-            RRType::TXT(), RRTTL(300)))
+            RRType::TXT(), RRTTL(300))),
+        cname_target("."),
+        cname_count(0)
     {
         // Set up the message to indicate a successful response to the question
         // "www.example.com A", but don't add in any response sections.
@@ -145,6 +147,10 @@ public:
     RRsetPtr    rrs_in_cname_www2;  // www2.example.com IN CNAME
     RRsetPtr    rrs_in_ns_;         // example.com IN NS
     RRsetPtr    rrs_in_txt_www;     // www.example.com IN TXT
+    Name        cname_target;       // Used in response classifier to
+                                    // store the target of a possible
+                                    // CNAME chain
+    unsigned int cname_count;       // Used to count cnames in a chain
 };
 
 // Test that the error() function categorises the codes correctly.
@@ -180,7 +186,8 @@ TEST_F(ResponseClassifierTest, Query) {
 
     // Should be rejected as it is a query, not a response
     EXPECT_EQ(ResponseClassifier::NOTRESPONSE,
-        ResponseClassifier::classify(qu_in_a_www, msg_a));
+        ResponseClassifier::classify(qu_in_a_www, msg_a,
+                                     cname_target, cname_count));
 }
 
 // Check that we get an OPCODE error on all but QUERY opcodes.
@@ -193,10 +200,12 @@ TEST_F(ResponseClassifierTest, Opcode) {
         msg_a.setOpcode(Opcode(i));
         if (i == query) {
             EXPECT_NE(ResponseClassifier::OPCODE,
-                ResponseClassifier::classify(qu_in_a_www, msg_a));
+                ResponseClassifier::classify(qu_in_a_www, msg_a,
+                                             cname_target, cname_count));
         } else {
             EXPECT_EQ(ResponseClassifier::OPCODE,
-                ResponseClassifier::classify(qu_in_a_www, msg_a));
+                ResponseClassifier::classify(qu_in_a_www, msg_a,
+                                             cname_target, cname_count));
         }
     }
 }
@@ -214,22 +223,26 @@ TEST_F(ResponseClassifierTest, MultipleQuestions) {
 
     // Zero questions
     EXPECT_EQ(ResponseClassifier::NOTONEQUEST,
-        ResponseClassifier::classify(qu_in_a_www, message));
+        ResponseClassifier::classify(qu_in_a_www, message,
+                                     cname_target, cname_count));
 
     // One question
     message.addQuestion(qu_in_a_www);
     EXPECT_NE(ResponseClassifier::NOTONEQUEST,
-        ResponseClassifier::classify(qu_in_a_www, message));
+        ResponseClassifier::classify(qu_in_a_www, message,
+                                     cname_target, cname_count));
 
     // Two questions
     message.addQuestion(qu_in_ns_);
     EXPECT_EQ(ResponseClassifier::NOTONEQUEST,
-        ResponseClassifier::classify(qu_in_a_www, message));
+        ResponseClassifier::classify(qu_in_a_www, message,
+                                     cname_target, cname_count));
 
     // And finish the check with three questions
     message.addQuestion(qu_in_txt_www);
     EXPECT_EQ(ResponseClassifier::NOTONEQUEST,
-        ResponseClassifier::classify(qu_in_a_www, message));
+        ResponseClassifier::classify(qu_in_a_www, message,
+                                     cname_target, cname_count));
 }
 
 // Test that the question in the question section in the message response
@@ -238,9 +251,11 @@ TEST_F(ResponseClassifierTest, MultipleQuestions) {
 TEST_F(ResponseClassifierTest, SameQuestion) {
 
     EXPECT_EQ(ResponseClassifier::MISMATQUEST,
-        ResponseClassifier::classify(qu_in_ns_, msg_a));
+        ResponseClassifier::classify(qu_in_ns_, msg_a,
+                                     cname_target, cname_count));
     EXPECT_NE(ResponseClassifier::MISMATQUEST,
-        ResponseClassifier::classify(qu_in_a_www, msg_a));
+        ResponseClassifier::classify(qu_in_a_www, msg_a,
+                                     cname_target, cname_count));
 }
 
 // Should get an NXDOMAIN response only on an NXDOMAIN RCODE.
@@ -253,10 +268,12 @@ TEST_F(ResponseClassifierTest, NXDOMAIN) {
         msg_a.setRcode(Rcode(i));
         if (i == nxdomain) {
             EXPECT_EQ(ResponseClassifier::NXDOMAIN,
-                ResponseClassifier::classify(qu_in_a_www, msg_a));
+                ResponseClassifier::classify(qu_in_a_www, msg_a,
+                                             cname_target, cname_count));
         } else {
             EXPECT_NE(ResponseClassifier::NXDOMAIN,
-                ResponseClassifier::classify(qu_in_a_www, msg_a));
+                ResponseClassifier::classify(qu_in_a_www, msg_a,
+                                             cname_target, cname_count));
         }
     }
 }
@@ -272,10 +289,12 @@ TEST_F(ResponseClassifierTest, RCODE) {
         msg_a.setRcode(Rcode(i));
         if ((i == nxdomain) || (i == noerror)) {
             EXPECT_NE(ResponseClassifier::RCODE,
-                ResponseClassifier::classify(qu_in_a_www, msg_a));
+                ResponseClassifier::classify(qu_in_a_www, msg_a,
+                                             cname_target, cname_count));
         } else {
             EXPECT_EQ(ResponseClassifier::RCODE,
-                ResponseClassifier::classify(qu_in_a_www, msg_a));
+                ResponseClassifier::classify(qu_in_a_www, msg_a,
+                                             cname_target, cname_count));
         }
     }
 }
@@ -291,17 +310,21 @@ TEST_F(ResponseClassifierTest, Truncated) {
     // bit is not set.
     msg_a.setHeaderFlag(Message::HEADERFLAG_TC, false);
     EXPECT_NE(ResponseClassifier::TRUNCATED,
-        ResponseClassifier::classify(qu_in_a_www, msg_a, true));
+        ResponseClassifier::classify(qu_in_a_www, msg_a, cname_target,
+                                     cname_count, true));
     EXPECT_NE(ResponseClassifier::TRUNCATED,
-        ResponseClassifier::classify(qu_in_a_www, msg_a, false));
+        ResponseClassifier::classify(qu_in_a_www, msg_a, cname_target,
+                                     cname_count, false));
 
     // Expect the truncated code if the TC bit is set, only if we don't ignore
     // it.
     msg_a.setHeaderFlag(Message::HEADERFLAG_TC, true);
     EXPECT_NE(ResponseClassifier::TRUNCATED,
-        ResponseClassifier::classify(qu_in_a_www, msg_a, true));
+        ResponseClassifier::classify(qu_in_a_www, msg_a, cname_target,
+                                     cname_count, true));
     EXPECT_EQ(ResponseClassifier::TRUNCATED,
-        ResponseClassifier::classify(qu_in_a_www, msg_a, false));
+        ResponseClassifier::classify(qu_in_a_www, msg_a, cname_target,
+                                     cname_count, false));
 }
 
 // Check for an empty packet (i.e. no error, but with the answer and additional
@@ -310,7 +333,8 @@ TEST_F(ResponseClassifierTest, Truncated) {
 TEST_F(ResponseClassifierTest, Empty) {
 
     EXPECT_EQ(ResponseClassifier::EMPTY,
-        ResponseClassifier::classify(qu_in_a_www, msg_a));
+        ResponseClassifier::classify(qu_in_a_www, msg_a, cname_target,
+                                     cname_count));
 }
 
 // Anything where we have an empty answer section but something in the
@@ -320,7 +344,8 @@ TEST_F(ResponseClassifierTest, EmptyAnswerReferral) {
 
     msg_a.addRRset(Message::SECTION_AUTHORITY, rrs_in_ns_);
     EXPECT_EQ(ResponseClassifier::REFERRAL,
-        ResponseClassifier::classify(qu_in_a_www, msg_a));
+        ResponseClassifier::classify(qu_in_a_www, msg_a, cname_target,
+                                     cname_count));
 
 }
 
@@ -334,12 +359,14 @@ TEST_F(ResponseClassifierTest, SingleAnswer) {
     // Check a question that matches the answer
     msg_a.addRRset(Message::SECTION_ANSWER, rrs_in_a_www);
     EXPECT_EQ(ResponseClassifier::ANSWER,
-        ResponseClassifier::classify(qu_in_a_www, msg_a));
+        ResponseClassifier::classify(qu_in_a_www, msg_a, cname_target,
+                                     cname_count));
 
     // Check an ANY question that matches the answer
     msg_any.addRRset(Message::SECTION_ANSWER, rrs_in_a_www);
     EXPECT_EQ(ResponseClassifier::ANSWER,
-        ResponseClassifier::classify(qu_in_any_www, msg_any));
+        ResponseClassifier::classify(qu_in_any_www, msg_any, cname_target,
+                                     cname_count));
 
     // Check a CNAME response that matches the QNAME.
     Message message_a(Message::RENDER);
@@ -349,7 +376,8 @@ TEST_F(ResponseClassifierTest, SingleAnswer) {
     message_a.addQuestion(qu_in_cname_www1);
     message_a.addRRset(Message::SECTION_ANSWER, rrs_in_cname_www1);
     EXPECT_EQ(ResponseClassifier::CNAME,
-        ResponseClassifier::classify(qu_in_cname_www1, message_a));
+        ResponseClassifier::classify(qu_in_cname_www1, message_a,
+                                     cname_target, cname_count));
 
     // Check if the answer QNAME does not match the question
     // Q: www.example.com  IN A
@@ -361,7 +389,8 @@ TEST_F(ResponseClassifierTest, SingleAnswer) {
     message_b.addQuestion(qu_in_a_www);
     message_b.addRRset(Message::SECTION_ANSWER, rrs_in_a_mail);
     EXPECT_EQ(ResponseClassifier::INVNAMCLASS,
-        ResponseClassifier::classify(qu_in_a_www, message_b));
+        ResponseClassifier::classify(qu_in_a_www, message_b,
+                                     cname_target, cname_count));
 
     // Check if the answer class does not match the question
     // Q: www.example.com CH A
@@ -373,7 +402,8 @@ TEST_F(ResponseClassifierTest, SingleAnswer) {
     message_c.addQuestion(qu_ch_a_www);
     message_c.addRRset(Message::SECTION_ANSWER, rrs_in_a_www);
     EXPECT_EQ(ResponseClassifier::INVNAMCLASS,
-        ResponseClassifier::classify(qu_ch_a_www, message_c));
+        ResponseClassifier::classify(qu_ch_a_www, message_c,
+                                     cname_target, cname_count));
 
     // Check if the answer type does not match the question
     // Q: www.example.com IN A
@@ -385,7 +415,8 @@ TEST_F(ResponseClassifierTest, SingleAnswer) {
     message_d.addQuestion(qu_in_a_www);
     message_d.addRRset(Message::SECTION_ANSWER, rrs_in_txt_www);
     EXPECT_EQ(ResponseClassifier::INVTYPE,
-        ResponseClassifier::classify(qu_in_a_www, message_d));
+        ResponseClassifier::classify(qu_in_a_www, message_d,
+                                     cname_target, cname_count));
 }
 
 // Check what happens if we have multiple RRsets in the answer.
@@ -401,7 +432,8 @@ TEST_F(ResponseClassifierTest, MultipleAnswerRRsets) {
     message_a.addRRset(Message::SECTION_ANSWER, rrs_in_a_www);
     message_a.addRRset(Message::SECTION_ANSWER, rrs_in_txt_www);
     EXPECT_EQ(ResponseClassifier::ANSWER,
-        ResponseClassifier::classify(qu_in_any_www, message_a));
+        ResponseClassifier::classify(qu_in_any_www, message_a,
+                                     cname_target, cname_count));
 
     // On another type of query, it results in an EXTRADATA error
     Message message_b(Message::RENDER);
@@ -412,7 +444,8 @@ TEST_F(ResponseClassifierTest, MultipleAnswerRRsets) {
     message_b.addRRset(Message::SECTION_ANSWER, rrs_in_a_www);
     message_b.addRRset(Message::SECTION_ANSWER, rrs_in_txt_www);
     EXPECT_EQ(ResponseClassifier::EXTRADATA,
-        ResponseClassifier::classify(qu_in_a_www, message_b));
+        ResponseClassifier::classify(qu_in_a_www, message_b,
+                                     cname_target, cname_count));
 
     // Same QNAME on an ANY query is not valid with mixed classes
     Message message_c(Message::RENDER);
@@ -423,7 +456,8 @@ TEST_F(ResponseClassifierTest, MultipleAnswerRRsets) {
     message_c.addRRset(Message::SECTION_ANSWER, rrs_in_a_www);
     message_c.addRRset(Message::SECTION_ANSWER, rrs_hs_txt_www);
     EXPECT_EQ(ResponseClassifier::MULTICLASS,
-        ResponseClassifier::classify(qu_in_any_www, message_c));
+        ResponseClassifier::classify(qu_in_any_www, message_c,
+                                     cname_target, cname_count));
 
     // Mixed QNAME is not valid unless QNAME requested is a CNAME.
     Message message_d(Message::RENDER);
@@ -434,7 +468,8 @@ TEST_F(ResponseClassifierTest, MultipleAnswerRRsets) {
     message_d.addRRset(Message::SECTION_ANSWER, rrs_in_a_www);
     message_d.addRRset(Message::SECTION_ANSWER, rrs_in_a_mail);
     EXPECT_EQ(ResponseClassifier::EXTRADATA,
-        ResponseClassifier::classify(qu_in_a_www, message_d));
+        ResponseClassifier::classify(qu_in_a_www, message_d,
+                                     cname_target, cname_count));
 
     // Mixed QNAME is not valid when the query is an ANY.
     Message message_e(Message::RENDER);
@@ -446,7 +481,8 @@ TEST_F(ResponseClassifierTest, MultipleAnswerRRsets) {
     message_e.addRRset(Message::SECTION_ANSWER, rrs_in_txt_www);
     message_e.addRRset(Message::SECTION_ANSWER, rrs_in_a_mail);
     EXPECT_EQ(ResponseClassifier::EXTRADATA,
-        ResponseClassifier::classify(qu_in_any_www, message_e));
+        ResponseClassifier::classify(qu_in_any_www, message_e,
+                                     cname_target, cname_count));
 }
 
 // CNAME chain is CNAME if it terminates in a CNAME, answer if it
@@ -461,22 +497,26 @@ TEST_F(ResponseClassifierTest, CNAMEChain) {
     message_a.addQuestion(qu_in_a_www2);
     message_a.addRRset(Message::SECTION_ANSWER, rrs_in_cname_www2);
     EXPECT_EQ(ResponseClassifier::CNAME,
-        ResponseClassifier::classify(qu_in_a_www2, message_a));
+        ResponseClassifier::classify(qu_in_a_www2, message_a,
+                                     cname_target, cname_count));
 
     // Add a CNAME for www1, and it should still return a CNAME
     message_a.addRRset(Message::SECTION_ANSWER, rrs_in_cname_www1);
     EXPECT_EQ(ResponseClassifier::CNAME,
-        ResponseClassifier::classify(qu_in_a_www2, message_a));
+        ResponseClassifier::classify(qu_in_a_www2, message_a,
+                                     cname_target, cname_count));
 
     // Add the A record for www and it should be an answer
     message_a.addRRset(Message::SECTION_ANSWER, rrs_in_a_www);
     EXPECT_EQ(ResponseClassifier::ANSWERCNAME,
-        ResponseClassifier::classify(qu_in_a_www2, message_a));
+        ResponseClassifier::classify(qu_in_a_www2, message_a,
+                                     cname_target, cname_count));
 
     // Adding an unrelated TXT record should result in EXTRADATA
     message_a.addRRset(Message::SECTION_ANSWER, rrs_in_txt_www);
     EXPECT_EQ(ResponseClassifier::EXTRADATA,
-        ResponseClassifier::classify(qu_in_a_www2, message_a));
+        ResponseClassifier::classify(qu_in_a_www2, message_a,
+                                     cname_target, cname_count));
 
     // Recreate the chain, but this time end with a TXT RR and not the A
     // record.  This should return INVTYPE.
@@ -490,7 +530,8 @@ TEST_F(ResponseClassifierTest, CNAMEChain) {
     message_b.addRRset(Message::SECTION_ANSWER, rrs_in_txt_www);
 
     EXPECT_EQ(ResponseClassifier::INVTYPE,
-        ResponseClassifier::classify(qu_in_a_www2, message_b));
+        ResponseClassifier::classify(qu_in_a_www2, message_b,
+                                     cname_target, cname_count));
 }
 
 } // Anonymous namespace