Commit b77baca5 authored by JINMEI Tatuya's avatar JINMEI Tatuya
Browse files

[master] Merge branch 'trac1688'

parents b41d5a89 953d4bcf
......@@ -15,6 +15,7 @@
#include <dns/message.h>
#include <dns/rcode.h>
#include <dns/rrtype.h>
#include <dns/rrset.h>
#include <dns/rdataclass.h>
#include <datasrc/client.h>
......@@ -25,7 +26,9 @@
#include <boost/bind.hpp>
#include <boost/function.hpp>
#include <cassert>
#include <algorithm> // for std::max
#include <functional>
#include <vector>
using namespace std;
......@@ -33,33 +36,9 @@ using namespace isc::dns;
using namespace isc::datasrc;
using namespace isc::dns::rdata;
// 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) {
/*
* FIXME:
* The const-cast is wrong, but the Message interface seems
* to insist.
*/
msg_.addRRset(section_,
boost::const_pointer_cast<AbstractRRset>(rrset),
dnssec_);
}
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.
namespace {
const vector<RRType>&
A_AND_AAAA() {
static vector<RRType> needed_types;
......@@ -69,33 +48,58 @@ A_AND_AAAA() {
}
return (needed_types);
}
}
namespace isc {
namespace auth {
// 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
getAdditional(const Name& qname, RRType qtype,
ZoneFinder::Context& ctx, vector<ConstRRsetPtr>& results)
Query::ResponseCreator::addRRset(isc::dns::Message& message,
const isc::dns::Message::Section section,
const ConstRRsetPtr& rrset, const bool 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);
/// Is this RRset already in the list of RRsets added to the message?
const std::vector<const AbstractRRset*>::const_iterator i =
std::find_if(added_.begin(), added_.end(),
std::bind1st(Query::ResponseCreator::IsSameKind(),
rrset.get()));
if (i == added_.end()) {
// No - add it to both the message and the list of RRsets processed.
// The const-cast is wrong, but the message interface seems to insist.
message.addRRset(section,
boost::const_pointer_cast<AbstractRRset>(rrset),
dnssec);
added_.push_back(rrset.get());
}
}
void
Query::ResponseCreator::create(Message& response,
const vector<ConstRRsetPtr>& answers,
const vector<ConstRRsetPtr>& authorities,
const vector<ConstRRsetPtr>& additionals,
const bool dnssec)
{
// Inserter should be reset each time the query is reset, so should be
// empty at this point.
assert(added_.empty());
// Add the RRsets to the message. The order of sections is important,
// as the ResponseCreator remembers RRsets added and will not add
// duplicates. Adding in the order answer, authory, additional will
// guarantee that if there are duplicates, the single RRset added will
// appear in the most important section.
BOOST_FOREACH(const ConstRRsetPtr& rrset, answers) {
addRRset(response, Message::SECTION_ANSWER, rrset, dnssec);
}
BOOST_FOREACH(const ConstRRsetPtr& rrset, authorities) {
addRRset(response, Message::SECTION_AUTHORITY, rrset, dnssec);
}
BOOST_FOREACH(const ConstRRsetPtr& rrset, additionals) {
addRRset(response, Message::SECTION_ADDITIONAL, rrset, dnssec);
}
}
namespace isc {
namespace auth {
void
Query::addSOA(ZoneFinder& finder) {
ZoneFinderContextPtr soa_ctx = finder.find(finder.getOrigin(),
......@@ -154,14 +158,10 @@ Query::addNXDOMAINProofByNSEC(ZoneFinder& finder, ConstRRsetPtr nsec) {
isc_throw(BadNSEC, "Unexpected result for wildcard NXDOMAIN proof");
}
// Add the (no-) wildcard proof only when it's different from the NSEC
// that proves NXDOMAIN; sometimes they can be the same.
// Note: name comparison is relatively expensive. When we are at the
// stage of performance optimization, we should consider optimizing this
// for some optimized data source implementations.
if (nsec->getName() != fcontext->rrset->getName()) {
authorities_.push_back(fcontext->rrset);
}
// Add the (no-) wildcard proof. This can be the same NSEC we already
// added, but we'd add it here anyway; duplicate checks will take place
// later in a unified manner.
authorities_.push_back(fcontext->rrset);
}
uint8_t
......@@ -261,11 +261,8 @@ Query::addWildcardNXRRSETProof(ZoneFinder& finder, ConstRRsetPtr nsec) {
fcontext->rrset->getRdataCount() == 0) {
isc_throw(BadNSEC, "Unexpected result for no match QNAME proof");
}
if (nsec->getName() != fcontext->rrset->getName()) {
// one NSEC RR proves wildcard_nxrrset that no matched QNAME.
authorities_.push_back(fcontext->rrset);
}
authorities_.push_back(fcontext->rrset);
}
void
......@@ -332,7 +329,7 @@ Query::addAuthAdditional(ZoneFinder& finder,
finder.getOrigin().toText());
}
authorities_.push_back(ns_context->rrset);
getAdditional(*qname_, *qtype_, *ns_context, additionals);
ns_context->getAdditional(A_AND_AAAA(), additionals);
}
namespace {
......@@ -468,7 +465,7 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
}
// Retrieve additional records for the answer
getAdditional(*qname_, *qtype_, *db_context, additionals_);
db_context->getAdditional(A_AND_AAAA(), additionals_);
// If apex NS records haven't been provided in the answer
// section, insert apex NS records into the authority section
......@@ -534,7 +531,8 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
break;
}
createResponse();
response_creator_.create(*response_, answers_, authorities_, additionals_,
dnssec_);
}
void
......@@ -551,16 +549,6 @@ Query::initialize(datasrc::DataSourceClient& datasrc_client,
isc::datasrc::ZoneFinder::FIND_DEFAULT);
}
void
Query::createResponse() {
for_each(answers_.begin(), answers_.end(),
RRsetInserter(*response_, Message::SECTION_ANSWER, dnssec_));
for_each(authorities_.begin(), authorities_.end(),
RRsetInserter(*response_, Message::SECTION_AUTHORITY, dnssec_));
for_each(additionals_.begin(), additionals_.end(),
RRsetInserter(*response_, Message::SECTION_ADDITIONAL, dnssec_));
}
void
Query::reset() {
datasrc_client_ = NULL;
......@@ -570,6 +558,7 @@ Query::reset() {
answers_.clear();
authorities_.clear();
additionals_.clear();
response_creator_.clear();
}
bool
......@@ -601,7 +590,8 @@ Query::processDSAtChild() {
}
}
createResponse();
response_creator_.create(*response_, answers_, authorities_, additionals_,
dnssec_);
return (true);
}
......
......@@ -20,6 +20,7 @@
#include <boost/noncopyable.hpp>
#include <functional>
#include <vector>
namespace isc {
......@@ -36,13 +37,6 @@ class DataSourceClient;
namespace auth {
/// \brief Initial reserved size for the vectors in Query
///
/// The value is larger than we expect the vectors to even become, and
/// has been chosen arbitrarily. The reason to set them quite high is
/// to prevent reallocation on addition.
const size_t RESERVE_RRSETS = 64;
/// The \c Query class represents a standard DNS query that encapsulates
/// processing logic to answer the query.
///
......@@ -75,6 +69,12 @@ const size_t RESERVE_RRSETS = 64;
/// we keep this name at the moment.
class Query : boost::noncopyable {
private:
/// \brief Initial reserved size for the vectors in Query
///
/// The value is larger than we expect the vectors to even become, and
/// has been chosen arbitrarily. The reason to set them quite high is
/// to prevent reallocation on addition.
static const size_t RESERVE_RRSETS = 64;
/// \brief Adds a SOA.
///
......@@ -251,19 +251,6 @@ private:
const isc::dns::Name& qname, const isc::dns::RRType& qtype,
isc::dns::Message& response, bool dnssec = false);
/// \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 answers_, authorities_, and
/// additionals_, and add them to their corresponding sections in
/// the response message.
///
/// After they are added, the vectors are cleared.
void createResponse();
/// \brief Resets any partly built response data, and internal pointers
///
/// Called by the QueryCleaner object upon its destruction
......@@ -282,6 +269,12 @@ private:
isc::auth::Query& query_;
};
protected:
// Following methods declared protected so they can be accessed
// by unit tests.
void createResponse();
public:
/// Default constructor.
///
......@@ -289,8 +282,8 @@ public:
///
Query() :
datasrc_client_(NULL), qname_(NULL), qtype_(NULL),
response_(NULL), dnssec_(false),
dnssec_opt_(isc::datasrc::ZoneFinder::FIND_DEFAULT)
dnssec_(false), dnssec_opt_(isc::datasrc::ZoneFinder::FIND_DEFAULT),
response_(NULL)
{
answers_.reserve(RESERVE_RRSETS);
authorities_.reserve(RESERVE_RRSETS);
......@@ -402,14 +395,102 @@ public:
{}
};
/// \brief Response Creator Class
///
/// This is a helper class of Query, and is expected to be used during the
/// construction of the response message. This class performs the
/// duplicate RRset detection check. It keeps a list of RRsets added
/// to the message and does not add an RRset if it is the same as one
/// already added.
///
/// This class is essentially private to Query, but is visible to public
/// for testing purposes. It's not expected to be used from a normal
/// application.
class ResponseCreator {
public:
/// \brief Constructor
///
/// Reserves space for the list of RRsets. Although the
/// ResponseCreator will be used to create a message from the
/// contents of the Query object's answers_, authorities_ and
/// additionals_ elements, and each of these are sized to
/// RESERVE_RRSETS, it is _extremely_ unlikely that all three will be
/// filled to capacity. So we reserve more elements than in each of
/// these components, but not three times the amount.
///
/// As with the answers_, authorities_ and additionals_ elements, the
/// reservation is made in the constructor to avoid dynamic allocation
/// of memory. The ResponseCreator is a member variable of the Query
/// object so is constructed once and lasts as long as that object.
/// Internal state is cleared through the clear() method.
ResponseCreator() {
added_.reserve(2 * RESERVE_RRSETS);
}
/// \brief Reset internal state
void clear() {
added_.clear();
}
/// \brief Complete the response message with filling in the
/// response sections.
///
/// This is the final step of the Query::process() method, and within
/// that method, it should be called before it returns (if any
/// response data is to be added)
///
/// This will take a message to build and each RRsets for the answer,
/// authority, and additional sections, and add them to their
/// corresponding sections in the given message. The RRsets are
/// filtered such that a particular RRset appears only once in the
/// message.
///
/// If \c dnssec is true, it tells the message to include any RRSIGs
/// attached to the RRsets.
void create(
isc::dns::Message& message,
const std::vector<isc::dns::ConstRRsetPtr>& answers_,
const std::vector<isc::dns::ConstRRsetPtr>& authorities_,
const std::vector<isc::dns::ConstRRsetPtr>& additionals_,
const bool dnssec);
private:
// \brief RRset comparison functor.
struct IsSameKind : public std::binary_function<
const isc::dns::AbstractRRset*,
const isc::dns::AbstractRRset*,
bool> {
bool operator()(const isc::dns::AbstractRRset* r1,
const isc::dns::AbstractRRset* r2) const {
return (r1->isSameKind(*r2));
}
};
/// Insertion operation
///
/// \param message Message to which the RRset is to be added
/// \param section Section of the message in which the RRset is put
/// \param rrset Pointer to RRset to be added to the message
/// \param dnssec Whether RRSIG records should be added as well
void addRRset(isc::dns::Message& message,
const isc::dns::Message::Section section,
const isc::dns::ConstRRsetPtr& rrset, const bool dnssec);
private:
/// List of RRsets already added to the message
std::vector<const isc::dns::AbstractRRset*> added_;
};
private:
const isc::datasrc::DataSourceClient* datasrc_client_;
const isc::dns::Name* qname_;
const isc::dns::RRType* qtype_;
isc::dns::Message* response_;
bool dnssec_;
isc::datasrc::ZoneFinder::FindOptions dnssec_opt_;
ResponseCreator response_creator_;
isc::dns::Message* response_;
std::vector<isc::dns::ConstRRsetPtr> answers_;
std::vector<isc::dns::ConstRRsetPtr> authorities_;
std::vector<isc::dns::ConstRRsetPtr> additionals_;
......
......@@ -12,12 +12,13 @@
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
#include <map>
#include <sstream>
#include <vector>
#include <map>
#include <boost/bind.hpp>
#include <boost/scoped_ptr.hpp>
#include <boost/static_assert.hpp>
#include <exceptions/exceptions.h>
......@@ -2367,4 +2368,145 @@ TEST_F(QueryTest, emptyNameWithNSEC3) {
EXPECT_TRUE(result->isNSEC3Signed());
EXPECT_FALSE(result->isWildcard());
}
// Vector of RRsets used for the test. Having this external to functions and
// classes used for the testing simplifies the code.
std::vector<RRsetPtr> rrset_vector;
// Callback function for masterLoad.
void
loadRRsetVectorCallback(RRsetPtr rrsetptr) {
rrset_vector.push_back(rrsetptr);
}
// Load a set of RRsets into a vector for use in the duplicate RRset test.
// They don't make a lot of sense as a zone, they are just different. The
// variables used in the stringstream input have been chosen so that each
// represents just one RRset.
void
loadRRsetVector() {
stringstream ss;
// Comments indicate offset in the rrset_vector (when loaded) and the
// number of RRs in that RRset.
ss << soa_txt // 0(1)
<< zone_ns_txt // 1(3)
<< delegation_txt // 2(4)
<< delegation_ds_txt // 3(1)
<< mx_txt // 4(3)
<< www_a_txt // 5(1)
<< cname_txt // 6(1)
<< cname_nxdom_txt // 7(1)
<< cname_out_txt; // 8(1)
rrset_vector.clear();
masterLoad(ss, Name("example.com."), RRClass::IN(),
loadRRsetVectorCallback);
}
TEST_F(QueryTest, DuplicateNameRemoval) {
// Load some RRsets into the master vector.
loadRRsetVector();
EXPECT_EQ(9, rrset_vector.size());
// Create an answer, authority and additional vector with some overlapping
// entries. The following indicate which elements from rrset_vector
// go into each section vector. (The values have been separated to show
// the overlap.)
//
// answer = 0 1 2 3
// authority = 2 3 4 5 6 7...
// ...5 (duplicate in the same section)
// additional = 0 3 7 8
//
// If the duplicate removal works, we should end up with the following in
// the message created from the three vectors:
//
// answer = 0 1 2 3
// authority = 4 5 6 7
// additional = 8
//
// The expected section into which each RRset is placed is indicated in the
// array below.
const Message::Section expected_section[] = {
Message::SECTION_ANSWER,
Message::SECTION_ANSWER,
Message::SECTION_ANSWER,
Message::SECTION_ANSWER,
Message::SECTION_AUTHORITY,
Message::SECTION_AUTHORITY,
Message::SECTION_AUTHORITY,
Message::SECTION_AUTHORITY,
Message::SECTION_ADDITIONAL
};
EXPECT_EQ(rrset_vector.size(),
(sizeof(expected_section) / sizeof(Message::Section)));
// Create the vectors of RRsets (with the RRsets in a semi-random order).
std::vector<ConstRRsetPtr> answer;
answer.insert(answer.end(), rrset_vector.begin() + 2,
rrset_vector.begin() + 4);
answer.insert(answer.end(), rrset_vector.begin() + 0,
rrset_vector.begin() + 2);
std::vector<ConstRRsetPtr> authority;
authority.insert(authority.end(), rrset_vector.begin() + 3,
rrset_vector.begin() + 8);
authority.push_back(rrset_vector[2]);
authority.push_back(rrset_vector[5]);
std::vector<ConstRRsetPtr> additional;
additional.insert(additional.end(), rrset_vector.begin() + 7,
rrset_vector.end());
additional.push_back(rrset_vector[3]);
additional.push_back(rrset_vector[0]);
// Create the message object into which the RRsets are put
Message message(Message::RENDER);
EXPECT_EQ(0, message.getRRCount(Message::SECTION_ANSWER));
EXPECT_EQ(0, message.getRRCount(Message::SECTION_AUTHORITY));
EXPECT_EQ(0, message.getRRCount(Message::SECTION_ADDITIONAL));
// ... and fill it.
Query::ResponseCreator().create(message, answer, authority, additional,
false);
// Check counts in each section. Note that these are RR counts,
// not RRset counts.
EXPECT_EQ(9, message.getRRCount(Message::SECTION_ANSWER));
EXPECT_EQ(6, message.getRRCount(Message::SECTION_AUTHORITY));
EXPECT_EQ(1, message.getRRCount(Message::SECTION_ADDITIONAL));
// ... and check that the RRsets are in the correct section
BOOST_STATIC_ASSERT(Message::SECTION_QUESTION == 0);
BOOST_STATIC_ASSERT(Message::SECTION_ANSWER == 1);
BOOST_STATIC_ASSERT(Message::SECTION_AUTHORITY == 2);
BOOST_STATIC_ASSERT(Message::SECTION_ADDITIONAL == 3);
Message::Section sections[] = {
Message::SECTION_QUESTION,
Message::SECTION_ANSWER,
Message::SECTION_AUTHORITY,
Message::SECTION_ADDITIONAL
};
for (int section = 1; section <= 3; ++section) {
for (int vecindex = 0; vecindex < rrset_vector.size(); ++vecindex) {
// Prepare error message in case an assertion fails (as the default
// message will only refer to the loop indexes).
stringstream ss;
ss << "section " << section << ", name "
<< rrset_vector[vecindex]->getName();
SCOPED_TRACE(ss.str());
// Check RRset is in the right section and not in the wrong
// section.
if (sections[section] == expected_section[vecindex]) {
EXPECT_TRUE(message.hasRRset(sections[section],
rrset_vector[vecindex]));
} else {
EXPECT_FALSE(message.hasRRset(sections[section],
rrset_vector[vecindex]));
}
}
}
}
}
......@@ -12,16 +12,17 @@
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
#include <stdexcept>
#include <exceptions/exceptions.h>
#include <dns/rdataclass.h>
#include <datasrc/rbnode_rrset.h>
#include <testutils/dnsmessage_test.h>
#include <dns/tests/unittest_util.h>
#include <gtest/gtest.h>
#include <dns/tests/unittest_util.h>
#include <sstream>
#include <stdexcept>
using isc::UnitTestUtil;
......
......@@ -482,8 +482,8 @@ public:
/// \param other Pointer to another AbstractRRset to compare
/// against.
virtual bool isSameKind(const AbstractRRset& other) const;
//@}
};
/// \brief The \c RdataIterator class is an abstract base class that
......
......@@ -12,8 +12,6 @@
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
#include <stdexcept>
#include <util/buffer.h>
#include <dns/messagerenderer.h>
#include <dns/name.h>
......@@ -24,9 +22,12 @@
#include <dns/rrttl.h>
#include <dns/rrset.h>
#include <dns/tests/unittest_util.h>
#include <gtest/gtest.h>
#include <dns/tests/unittest_util.h>
#include <stdexcept>
#include <sstream>
using isc::UnitTestUtil;
......
......@@ -160,45 +160,41 @@ Feature: NSEC3 Authoritative service
# Below are additional tests, not explicitely stated in RFC5155
#
# THIS TEST CURRENTLY FAILS: An NSEC3 record is added twice
# See ticket #1688
#Scenario: 7.2.2 other; Name Error where one NSEC3 covers multiple parts of proof (closest encloser)