From: Jelte Jansen Date: Thu, 8 Mar 2012 17:09:48 +0000 (+0100) Subject: [1747] split up 'target' vector to answer and auth X-Git-Tag: trac2351_base~226^2~116^2~117^2~7 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=4f93a3a8366f971078dc9c8fe4c846c447a7a45d;p=thirdparty%2Fkea.git [1747] split up 'target' vector to answer and auth and only fill in the actual response packet right before process() returns --- diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc index 912bfc2604..ae3184ff00 100644 --- a/src/bin/auth/auth_srv.cc +++ b/src/bin/auth/auth_srv.cc @@ -553,7 +553,8 @@ AuthSrvImpl::processNormalQuery(const IOMessage& io_message, Message& message, if (memory_client_ && memory_client_class_ == question->getClass()) { const RRType& qtype = question->getType(); const Name& qname = question->getName(); - query_.reset(memory_client_.get(), qname, qtype, &message, dnssec_ok); + query_.initialize(memory_client_.get(), qname, qtype, &message, + dnssec_ok); query_.process(); } else { datasrc::Query query(message, cache_, dnssec_ok); diff --git a/src/bin/auth/query.cc b/src/bin/auth/query.cc index 3b5635ed13..bb206efe17 100644 --- a/src/bin/auth/query.cc +++ b/src/bin/auth/query.cc @@ -103,8 +103,8 @@ Query::addSOA(ZoneFinder& finder) { * The const-cast is wrong, but the Message interface seems * to insist. */ - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast(soa_ctx->rrset), dnssec_); + authority_.push_back(boost::const_pointer_cast( + soa_ctx->rrset)); } } @@ -123,8 +123,7 @@ Query::addNXDOMAINProofByNSEC(ZoneFinder& finder, ConstRRsetPtr nsec) { } // Add the NSEC proving NXDOMAIN to the authority section. - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast(nsec), dnssec_); + authority_.push_back(boost::const_pointer_cast(nsec)); // Next, identify the best possible wildcard name that would match // the query name. It's the longer common suffix with the qname @@ -161,9 +160,8 @@ Query::addNXDOMAINProofByNSEC(ZoneFinder& finder, ConstRRsetPtr nsec) { // stage of performance optimization, we should consider optimizing this // for some optimized data source implementations. if (nsec->getName() != fcontext->rrset->getName()) { - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast(fcontext->rrset), - dnssec_); + authority_.push_back(boost::const_pointer_cast( + fcontext->rrset)); } } @@ -183,16 +181,12 @@ Query::addClosestEncloserProof(ZoneFinder& finder, const Name& name, } if (add_closest) { - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast( - result.closest_proof), - dnssec_); + authority_.push_back(boost::const_pointer_cast( + result.closest_proof)); } if (result.next_proof) { - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast( - result.next_proof), - dnssec_); + authority_.push_back(boost::const_pointer_cast( + result.next_proof)); } return (result.closest_labels); } @@ -208,10 +202,8 @@ Query::addNSEC3ForName(ZoneFinder& finder, const Name& name, bool match) { << (result.matched ? "matching" : "covering") << " NSEC3 found for " << name); } - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast( - result.closest_proof), - dnssec_); + authority_.push_back(boost::const_pointer_cast( + result.closest_proof)); } void @@ -246,10 +238,8 @@ Query::addWildcardProof(ZoneFinder& finder, isc_throw(BadNSEC, "Unexpected NSEC result for wildcard proof"); } - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast( - fcontext->rrset), - dnssec_); + authority_.push_back(boost::const_pointer_cast( + fcontext->rrset)); } else if (db_context.isNSEC3Signed()) { // Case for RFC 5155 Section 7.2.6. // @@ -279,9 +269,8 @@ Query::addWildcardNXRRSETProof(ZoneFinder& finder, ConstRRsetPtr nsec) { if (nsec->getName() != fcontext->rrset->getName()) { // one NSEC RR proves wildcard_nxrrset that no matched QNAME. - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast(fcontext->rrset), - dnssec_); + authority_.push_back(boost::const_pointer_cast( + fcontext->rrset)); } } @@ -290,10 +279,8 @@ Query::addDS(ZoneFinder& finder, const Name& dname) { ConstZoneFinderContextPtr ds_context = finder.find(dname, RRType::DS(), dnssec_opt_); if (ds_context->code == ZoneFinder::SUCCESS) { - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast( - ds_context->rrset), - dnssec_); + authority_.push_back(boost::const_pointer_cast( + ds_context->rrset)); } else if (ds_context->code == ZoneFinder::NXRRSET && ds_context->isNSECSigned()) { addNXRRsetProof(finder, *ds_context); @@ -312,10 +299,8 @@ Query::addNXRRsetProof(ZoneFinder& finder, const ZoneFinder::Context& db_context) { if (db_context.isNSECSigned() && db_context.rrset) { - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast( - db_context.rrset), - dnssec_); + authority_.push_back(boost::const_pointer_cast( + db_context.rrset)); if (db_context.isWildcard()) { addWildcardNXRRSETProof(finder, db_context.rrset); } @@ -354,9 +339,8 @@ Query::addAuthAdditional(ZoneFinder& finder, isc_throw(NoApexNS, "There's no apex NS records in zone " << finder.getOrigin().toText()); } - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast( - ns_context->rrset), dnssec_); + authority_.push_back(boost::const_pointer_cast( + ns_context->rrset)); getAdditional(qname_, qtype_, *ns_context, additionals); } @@ -380,7 +364,7 @@ void Query::process() { if (datasrc_client_ == NULL) { isc_throw(isc::InvalidOperation, - "Query::process called before setting data source"); + "Query::process() called before initialize()"); } // Found a zone which is the nearest ancestor to QNAME @@ -399,10 +383,12 @@ Query::process() { // has to be handled there. if (qtype_ == RRType::DS() && qname_.getLabelCount() > 1 && processDSAtChild()) { + createResponse(); return; } response_->setHeaderFlag(Message::HEADERFLAG_AA, false); response_->setRcode(Rcode::REFUSED()); + createResponse(); return; } ZoneFinder& zfinder = *result.zone_finder; @@ -415,7 +401,7 @@ Query::process() { const bool qtype_is_any = (qtype_ == RRType::ANY()); if (qtype_is_any) { find = boost::bind(&ZoneFinder::findAll, &zfinder, qname_, - boost::ref(target_), dnssec_opt_); + boost::ref(answer_), dnssec_opt_); } else { find = boost::bind(&ZoneFinder::find, &zfinder, qname_, qtype_, dnssec_opt_); @@ -424,9 +410,8 @@ Query::process() { switch (db_context->code) { case ZoneFinder::DNAME: { // First, put the dname into the answer - response_->addRRset(Message::SECTION_ANSWER, - boost::const_pointer_cast(db_context->rrset), - dnssec_); + answer_.push_back(boost::const_pointer_cast( + db_context->rrset)); /* * Empty DNAME should never get in, as it is impossible to * create one in master file. @@ -450,7 +435,7 @@ Query::process() { * of RFC 2672 mandates we return YXDOMAIN. */ response_->setRcode(Rcode::YXDOMAIN()); - return; + break; } // The new CNAME we are creating (it will be unsigned even // with DNSSEC, the DNAME is signed and it can be validated @@ -462,7 +447,7 @@ Query::process() { qname_.getLabelCount() - db_context->rrset->getName().getLabelCount()). concatenate(dname.getDname()))); - response_->addRRset(Message::SECTION_ANSWER, cname, dnssec_); + answer_.push_back(cname); break; } case ZoneFinder::CNAME: @@ -475,9 +460,8 @@ Query::process() { * * So, just put it there. */ - response_->addRRset(Message::SECTION_ANSWER, - boost::const_pointer_cast(db_context->rrset), - dnssec_); + answer_.push_back(boost::const_pointer_cast( + db_context->rrset)); // If the answer is a result of wildcard substitution, // add a proof that there's no closer name. @@ -489,14 +473,14 @@ Query::process() { if (qtype_is_any) { // If quety type is ANY, insert all RRs under the domain // into answer section. - BOOST_FOREACH(ConstRRsetPtr rrset, target_) { - response_->addRRset(Message::SECTION_ANSWER, - boost::const_pointer_cast(rrset), dnssec_); - } + // err THIS WILL FAIL + //BOOST_FOREACH(ConstRRsetPtr rrset, answer_) { + // answer_.push_back( + // boost::const_pointer_cast(rrset)); + //} } else { - response_->addRRset(Message::SECTION_ANSWER, - boost::const_pointer_cast(db_context->rrset), - dnssec_); + answer_.push_back(boost::const_pointer_cast( + db_context->rrset)); } // Retrieve additional records for the answer @@ -527,13 +511,12 @@ Query::process() { // complete the process in the child as specified in Section // 2.2.1.2. of RFC3658. if (qtype_ == RRType::DS() && processDSAtChild()) { - return; + break; } response_->setHeaderFlag(Message::HEADERFLAG_AA, false); - response_->addRRset(Message::SECTION_AUTHORITY, - boost::const_pointer_cast(db_context->rrset), - dnssec_); + authority_.push_back(boost::const_pointer_cast( + db_context->rrset)); // Retrieve additional records for the name servers db_context->getAdditional(A_AND_AAAA(), additionals_); @@ -568,9 +551,23 @@ Query::process() { break; } + createResponse(); +} + +void +Query::createResponse() { + for_each(answer_.begin(), answer_.end(), + RRsetInserter(*response_, Message::SECTION_ANSWER, + dnssec_)); + answer_.clear(); + for_each(authority_.begin(), authority_.end(), + RRsetInserter(*response_, Message::SECTION_AUTHORITY, + dnssec_)); + authority_.clear(); for_each(additionals_.begin(), additionals_.end(), RRsetInserter(*response_, Message::SECTION_ADDITIONAL, dnssec_)); + additionals_.clear(); } bool diff --git a/src/bin/auth/query.h b/src/bin/auth/query.h index f9aeb5f392..555577c738 100644 --- a/src/bin/auth/query.h +++ b/src/bin/auth/query.h @@ -226,6 +226,19 @@ private: void addNSEC3ForName(isc::datasrc::ZoneFinder& finder, const isc::dns::Name& name, bool match); + /// \brief Fill in the response sections + /// + /// This is the final step of the process() method, and within + /// that method, it should be called before it returns (if any + /// response data is to be added) + /// + /// This will take each RRset collected in answer_, authority_, and + /// additionals_, and add them to their corresponding sections in + /// the response packet. + /// + /// After they are added, the vectors are cleared. + void createResponse(); + public: /// Constructor from query parameters. /// @@ -348,8 +361,14 @@ public: {} }; + /// Set up the Query object for a new query lookup + /// + /// If the empty constructor, has been used to initialize the + /// query instance, of if the instance is reused, it should + /// be initialized with data to look up. + /// void - reset(datasrc::DataSourceClient* datasrc_client, + initialize(datasrc::DataSourceClient* datasrc_client, const isc::dns::Name qname, const isc::dns::RRType qtype, isc::dns::Message* response, bool dnssec = false) { datasrc_client_ = datasrc_client; @@ -359,8 +378,20 @@ public: dnssec_ = dnssec; dnssec_opt_ = (dnssec ? isc::datasrc::ZoneFinder::FIND_DNSSEC : isc::datasrc::ZoneFinder::FIND_DEFAULT); + // The call to reset() could in theory be ommitted, but + // seems prudent, just in case a previous process() left + // data in here. + reset(); + } - target_.clear(); + /// \brief Reset any partly built response data + /// + /// In theory, this is not necessary if the process() call finishes + /// successfully, but if it does not, reset() can be used to clean up. + void + reset() { + answer_.clear(); + authority_.clear(); additionals_.clear(); } @@ -372,7 +403,8 @@ private: bool dnssec_; isc::datasrc::ZoneFinder::FindOptions dnssec_opt_; - std::vector target_; + std::vector answer_; + std::vector authority_; std::vector additionals_; }; diff --git a/src/bin/auth/tests/query_unittest.cc b/src/bin/auth/tests/query_unittest.cc index b96127d139..2bcd22a1b2 100644 --- a/src/bin/auth/tests/query_unittest.cc +++ b/src/bin/auth/tests/query_unittest.cc @@ -965,10 +965,12 @@ TEST_F(QueryTest, exactMatchMultipleQueries) { // find match rrset responseCheck(response, Rcode::NOERROR(), AA_FLAG, 1, 3, 3, www_a_txt, zone_ns_txt, ns_addrs_txt); + + // clean up response for second query response.clear(isc::dns::Message::RENDER); response.setRcode(Rcode::NOERROR()); response.setOpcode(Opcode::QUERY()); - query.reset(&memory_client, qname, qtype, &response); + query.initialize(&memory_client, qname, qtype, &response); EXPECT_NO_THROW(query.process()); // find match rrset SCOPED_TRACE("Second query"); diff --git a/tests/lettuce/features/queries.feature b/tests/lettuce/features/queries.feature index 335300b116..b8f9b3d6f7 100644 --- a/tests/lettuce/features/queries.feature +++ b/tests/lettuce/features/queries.feature @@ -58,3 +58,24 @@ Feature: Querying feature """ example.org. 3600 IN SOA ns1.example.org. admin.example.org. 1234 3600 1800 2419200 7200 """ + + Scenario: ANY query + Given I have bind10 running with configuration example.org.inmem.config + A query for example.org type ANY should have rcode NOERROR + The last query response should have flags qr aa rd + The last query response should have ancount 4 + The last query response should have nscount 0 + The last query response should have adcount 3 + The answer section of the last query response should be + """ + example.org. 3600 IN NS ns1.example.org. + example.org. 3600 IN NS ns2.example.org. + example.org. 3600 IN SOA ns1.example.org. admin.example.org. 1234 3600 1800 2419200 7200 + example.org. 3600 IN MX 10 mail.example.org. + """ + The additional section of the last query response should be + """ + ns1.example.org. 3600 IN A 192.0.2.3 + ns2.example.org. 3600 IN A 192.0.2.4 + mail.example.org. 3600 IN A 192.0.2.10 + """