[1607] adjusted auth::Query to use ZoneFinder::Context::getAdditional().

src/bin/auth/query.cc

@@ -12,82 +12,83 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#include <algorithm>            // for std::max
-#include <vector>
-#include <boost/foreach.hpp>
-#include <boost/bind.hpp>
-#include <boost/function.hpp>
-
 #include <dns/message.h>
 #include <dns/rcode.h>
+#include <dns/rrtype.h>
 #include <dns/rdataclass.h>
 
 #include <datasrc/client.h>
 
 #include <auth/query.h>
 
+#include <boost/foreach.hpp>
+#include <boost/bind.hpp>
+#include <boost/function.hpp>
+
+#include <algorithm>            // for std::max
+#include <vector>
+
+using namespace std;
 using namespace isc::dns;
 using namespace isc::datasrc;
 using namespace isc::dns::rdata;
 
-namespace isc {
-namespace auth {
+// Commonly used helper callback object for vector<ConstRRsetPtr> to
+// insert them to (the specified section of) a message.
+namespace {
+class RRsetInserter {
+public:
+    RRsetInserter(Message& msg, Message::Section section, bool dnssec) :
+        msg_(msg), section_(section), dnssec_(dnssec)
+    {}
+    void operator()(const ConstRRsetPtr& rrset) {
+        msg_.addRRset(section_,
+                      boost::const_pointer_cast<AbstractRRset>(rrset),
+                      dnssec_);
+    }
 
-void
-Query::addAdditional(ZoneFinder& zone, const AbstractRRset& rrset) {
-    RdataIteratorPtr rdata_iterator(rrset.getRdataIterator());
-    for (; !rdata_iterator->isLast(); rdata_iterator->next()) {
-        const Rdata& rdata(rdata_iterator->getCurrent());
-        if (rrset.getType() == RRType::NS()) {
-            // Need to perform the search in the "GLUE OK" mode.
-            const generic::NS& ns = dynamic_cast<const generic::NS&>(rdata);
-            addAdditionalAddrs(zone, ns.getNSName(), ZoneFinder::FIND_GLUE_OK);
-        } else if (rrset.getType() == RRType::MX()) {
-            const generic::MX& mx(dynamic_cast<const generic::MX&>(rdata));
-            addAdditionalAddrs(zone, mx.getMXName());
-        }
+private:
+    Message& msg_;
+    const Message::Section section_;
+    const bool dnssec_;
+};
+
+// This is a "constant" vector storing desired RR types for the additional
+// section.  The vector is filled first time it's used.
+const vector<RRType>&
+A_AND_AAAA() {
+    static vector<RRType> needed_types;
+    if (needed_types.empty()) {
+        needed_types.push_back(RRType::A());
+        needed_types.push_back(RRType::AAAA());
     }
+    return (needed_types);
 }
 
+// A wrapper for ZoneFinder::Context::getAdditional() so we don't include
+// duplicate RRs.  This is not efficient, and we should actually unify
+// this at the end of the process() method.  See also #1688.
 void
-Query::addAdditionalAddrs(ZoneFinder& zone, const Name& qname,
-                          const ZoneFinder::FindOptions options)
+getAdditional(const Name& qname, RRType qtype,
+              ZoneFinder::Context& ctx, vector<ConstRRsetPtr>& results)
 {
-    // Out of zone name
-    NameComparisonResult result = zone.getOrigin().compare(qname);
-    if ((result.getRelation() != NameComparisonResult::SUPERDOMAIN) &&
-        (result.getRelation() != NameComparisonResult::EQUAL))
-        return;
-
-    // Omit additional data which has already been provided in the answer
-    // section from the additional.
-    //
-    // All the address rrset with the owner name of qname have been inserted
-    // into ANSWER section.
-    if (qname_ == qname && qtype_ == RRType::ANY())
-        return;
-
-    // Find A rrset
-    if (qname_ != qname || qtype_ != RRType::A()) {
-        ZoneFinderContextPtr a_ctx = zone.find(qname, RRType::A(),
-                                               options | dnssec_opt_);
-        if (a_ctx->code == ZoneFinder::SUCCESS) {
-            response_.addRRset(Message::SECTION_ADDITIONAL,
-                    boost::const_pointer_cast<AbstractRRset>(a_ctx->rrset), dnssec_);
-        }
-    }
-
-    // Find AAAA rrset
-    if (qname_ != qname || qtype_ != RRType::AAAA()) {
-        ZoneFinderContextPtr aaaa_ctx = zone.find(qname, RRType::AAAA(),
-                                                  options | dnssec_opt_);
-        if (aaaa_ctx->code == ZoneFinder::SUCCESS) {
-            response_.addRRset(Message::SECTION_ADDITIONAL,
-                    boost::const_pointer_cast<AbstractRRset>(aaaa_ctx->rrset),
-                    dnssec_);
+    vector<ConstRRsetPtr> additionals;
+    ctx.getAdditional(A_AND_AAAA(), additionals);
+
+    vector<ConstRRsetPtr>::const_iterator it = additionals.begin();
+    vector<ConstRRsetPtr>::const_iterator it_end = additionals.end();
+    for (; it != it_end; ++it) {
+        if ((qtype == (*it)->getType() || qtype == RRType::ANY()) &&
+            qname == (*it)->getName()) {
+            continue;
         }
+        results.push_back(*it);
     }
 }
+}
+
+namespace isc {
+namespace auth {
 
 void
 Query::addSOA(ZoneFinder& finder) {
@@ -339,21 +340,24 @@ Query::addNXRRsetProof(ZoneFinder& finder,
 }
 
 void
-Query::addAuthAdditional(ZoneFinder& finder) {
+Query::addAuthAdditional(ZoneFinder& finder,
+                         vector<ConstRRsetPtr>& additionals)
+{
+    const Name& origin = finder.getOrigin();
+
     // Fill in authority and addtional sections.
-    ConstZoneFinderContextPtr ns_context =
-        finder.find(finder.getOrigin(), RRType::NS(), dnssec_opt_);
+    ConstZoneFinderContextPtr ns_context = finder.find(origin, RRType::NS(),
+                                                       dnssec_opt_);
 
     // zone origin name should have NS records
     if (ns_context->code != ZoneFinder::SUCCESS) {
         isc_throw(NoApexNS, "There's no apex NS records in zone " <<
-                finder.getOrigin().toText());
-    } else {
-        response_.addRRset(Message::SECTION_AUTHORITY,
-            boost::const_pointer_cast<AbstractRRset>(ns_context->rrset), dnssec_);
-        // Handle additional for authority section
-        addAdditional(finder, *ns_context->rrset);
+                  finder.getOrigin().toText());
     }
+    response_.addRRset(Message::SECTION_AUTHORITY,
+                       boost::const_pointer_cast<AbstractRRset>(
+                           ns_context->rrset), dnssec_);
+    getAdditional(qname_, qtype_, *ns_context, additionals);
 }
 
 namespace {
@@ -402,7 +406,8 @@ Query::process() {
     // indirectly via delegation).  Look into the zone.
     response_.setHeaderFlag(Message::HEADERFLAG_AA);
     response_.setRcode(Rcode::NOERROR());
-    std::vector<ConstRRsetPtr> target;
+    vector<ConstRRsetPtr> target;
+    vector<ConstRRsetPtr> additionals;
     boost::function0<ZoneFinderContextPtr> find;
     const bool qtype_is_any = (qtype_ == RRType::ANY());
     if (qtype_is_any) {
@@ -484,16 +489,16 @@ Query::process() {
                 BOOST_FOREACH(ConstRRsetPtr rrset, target) {
                     response_.addRRset(Message::SECTION_ANSWER,
                         boost::const_pointer_cast<AbstractRRset>(rrset), dnssec_);
-                    // Handle additional for answer section
-                    addAdditional(*result.zone_finder, *rrset.get());
                 }
             } else {
                 response_.addRRset(Message::SECTION_ANSWER,
                     boost::const_pointer_cast<AbstractRRset>(db_context->rrset),
                     dnssec_);
-                // Handle additional for answer section
-                addAdditional(*result.zone_finder, *db_context->rrset);
             }
+
+            // Retrieve additional records for the answer
+            getAdditional(qname_, qtype_, *db_context, additionals);
+
             // If apex NS records haven't been provided in the answer
             // section, insert apex NS records into the authority section
             // and AAAA/A RRS of each of the NS RDATA into the additional
@@ -502,7 +507,7 @@ Query::process() {
                 db_context->code != ZoneFinder::SUCCESS ||
                 (qtype_ != RRType::NS() && !qtype_is_any))
             {
-                addAuthAdditional(*result.zone_finder);
+                addAuthAdditional(*result.zone_finder, additionals);
             }
 
             // If the answer is a result of wildcard substitution,
@@ -524,12 +529,14 @@ Query::process() {
             response_.addRRset(Message::SECTION_AUTHORITY,
                 boost::const_pointer_cast<AbstractRRset>(db_context->rrset),
                 dnssec_);
+            // Retrieve additional records for the name servers
+            db_context->getAdditional(A_AND_AAAA(), additionals);
+
             // If DNSSEC is requested, see whether there is a DS
             // record for this delegation.
             if (dnssec_) {
                 addDS(*result.zone_finder, db_context->rrset->getName());
             }
-            addAdditional(*result.zone_finder, *db_context->rrset);
             break;
         case ZoneFinder::NXDOMAIN:
             response_.setRcode(Rcode::NXDOMAIN());
@@ -555,6 +562,10 @@ Query::process() {
             isc_throw(isc::NotImplemented, "Unknown result code");
             break;
     }
+
+    for_each(additionals.begin(), additionals.end(),
+             RRsetInserter(response_, Message::SECTION_ADDITIONAL,
+                           dnssec_));
 }
 
 bool

src/bin/auth/query.h

@@ -15,8 +15,11 @@
  */
 
 #include <exceptions/exceptions.h>
+#include <dns/rrset.h>
 #include <datasrc/zone.h>
 
+#include <vector>
+
 namespace isc {
 namespace dns {
 class Message;
@@ -129,44 +132,6 @@ private:
     void addWildcardNXRRSETProof(isc::datasrc::ZoneFinder& finder,
                                  isc::dns::ConstRRsetPtr nsec);
 
-    /// \brief Look up additional data (i.e., address records for the names
-    /// included in NS or MX records) and add them to the additional section.
-    ///
-    /// Note: Any additional data which has already been provided in the
-    /// answer section (i.e., if the original query happend to be for the
-    /// address of the DNS server), it should be omitted from the additional.
-    ///
-    /// This method may throw a exception because its underlying methods may
-    /// throw exceptions.
-    ///
-    /// \param zone The ZoneFinder through which the additional data for the
-    /// query is to be found.
-    /// \param rrset The RRset (i.e., NS or MX rrset) which require additional
-    /// processing.
-    void addAdditional(isc::datasrc::ZoneFinder& zone,
-                       const isc::dns::AbstractRRset& rrset);
-
-    /// \brief Find address records for a specified name.
-    ///
-    /// Search the specified zone for AAAA/A RRs of each of the NS/MX RDATA
-    /// (domain name), and insert the found ones into the additional section
-    /// if address records are available. By default the search will stop
-    /// once it encounters a zone cut.
-    ///
-    /// Note: we need to perform the search in the "GLUE OK" mode for NS RDATA,
-    /// which means that we should include A/AAAA RRs under a zone cut.
-    /// The glue records must exactly match the name in the NS RDATA, without
-    /// CNAME or wildcard processing.
-    ///
-    /// \param zone The \c ZoneFinder through which the address records is to
-    /// be found.
-    /// \param qname The name in rrset RDATA.
-    /// \param options The search options.
-    void addAdditionalAddrs(isc::datasrc::ZoneFinder& zone,
-                            const isc::dns::Name& qname,
-                            const isc::datasrc::ZoneFinder::FindOptions options
-                            = isc::datasrc::ZoneFinder::FIND_DEFAULT);
-
     /// \brief Look up a zone's NS RRset and their address records for an
     /// authoritative answer, and add them to the additional section.
     ///
@@ -185,7 +150,8 @@ private:
     ///
     /// \param finder The \c ZoneFinder through which the NS and additional
     /// data for the query are to be found.
-    void addAuthAdditional(isc::datasrc::ZoneFinder& finder);
+    void addAuthAdditional(isc::datasrc::ZoneFinder& finder,
+                           std::vector<isc::dns::ConstRRsetPtr>& additionals);
 
     /// \brief Process a DS query possible at the child side of zone cut.
     ///