From: Stephen Morris Date: Fri, 16 Mar 2012 14:52:11 +0000 (+0000) Subject: [1688] Remove duplicate RRsets when producing output message X-Git-Tag: trac2351_base~226^2~116^2~66^2~10^2~17 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e0c470b595d05c8c075732bd090fcb02b2af996e;p=thirdparty%2Fkea.git [1688] Remove duplicate RRsets when producing output message When Query::createResponse() creates a message, this change ensures that any duplicate RRsets are removed. --- diff --git a/src/bin/auth/query.cc b/src/bin/auth/query.cc index db1223d746..6b01c6969d 100644 --- a/src/bin/auth/query.cc +++ b/src/bin/auth/query.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -26,6 +27,7 @@ #include #include // for std::max +#include #include using namespace std; @@ -35,27 +37,64 @@ using namespace isc::dns::rdata; // Commonly used helper callback object for vector to // insert them to (the specified section of) a message. +// +// One feature is that it maintains an internal set of raw pointers to the +// RRsets as they are added (this is safe - the object is only in scope in +// the createResponse() method and during this time, all RRsets referred to +// remain in existence due to the presence of the ConstRRsetPtr objects +// from which the raw objects were derived). The set is used to detect +// and discard duplicates. namespace { class RRsetInserter { +private: + // \brief RRset comparison functor. + struct RRsetLthan { + bool operator()(const AbstractRRset* r1, const AbstractRRset* r2) { + return (r1->lthan(*r2)); + } + }; + + typedef std::set AddedRRsets; + public: - RRsetInserter(Message& msg, Message::Section section, bool dnssec) : - msg_(msg), section_(section), dnssec_(dnssec) + RRsetInserter(Message& msg, const Message::Section section, + const 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(rrset), - dnssec_); + + // \brief Set Target Section + // + // Sets the section into which the information added by addRRset will be + // inserted. + // + // \param section New section number + void setSection(Message::Section section) { + section_ = section; + } + + // Insertion operation + // + // \param rrset Pointer to RRset to be added to the message + void addRRset(const ConstRRsetPtr& rrset) { + // Has the RRset already been added to this message? + std::pair result = + rrsets_added_.insert(rrset.get()); + if (result.second) { + // Were able to add the pointer to the RRset to rrsets_added_, so + // the RRset has not already been seen. Add it to the message. + // The const-cast is wrong, but the Message interface seems + // to insist. + msg_.addRRset(section_, + boost::const_pointer_cast(rrset), + dnssec_); + } } private: Message& msg_; - const Message::Section section_; + Message::Section section_; const bool dnssec_; + AddedRRsets rrsets_added_; }; // This is a "constant" vector storing desired RR types for the additional @@ -553,12 +592,26 @@ Query::initialize(datasrc::DataSourceClient& datasrc_client, 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_)); + // Add the RRsets to the message. The order of sections is important, + // as the RRsetInserter 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. + std::vector::const_iterator i; + RRsetInserter inserter(*response_, Message::SECTION_ANSWER, dnssec_); + for (i = answers_.begin(); i != answers_.end(); ++i) { + inserter.addRRset(*i); + } + + inserter.setSection(Message::SECTION_AUTHORITY); + for (i = authorities_.begin(); i != authorities_.end(); ++i) { + inserter.addRRset(*i); + } + + inserter.setSection(Message::SECTION_ADDITIONAL); + for (i = additionals_.begin(); i != additionals_.end(); ++i) { + inserter.addRRset(*i); + } } void diff --git a/src/bin/auth/query.h b/src/bin/auth/query.h index e295d914c6..5a112889f0 100644 --- a/src/bin/auth/query.h +++ b/src/bin/auth/query.h @@ -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,24 @@ private: isc::auth::Query& query_; }; +protected: + // Following methods declared protected so they can be accessed + // by unit tests. + + /// \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. The RRsets are filtered such that a + /// particular RRset appears only once in the message. + /// + /// After they are added, the vectors are cleared. + void createResponse(); + public: /// Default constructor. /// @@ -289,8 +294,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); @@ -406,10 +411,13 @@ 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_; +protected: + // Following members declared protected to allow them to be accessed + // by unit tests. + isc::dns::Message* response_; std::vector answers_; std::vector authorities_; std::vector additionals_; diff --git a/src/bin/auth/tests/query_unittest.cc b/src/bin/auth/tests/query_unittest.cc index b74ebcca70..e40854337d 100644 --- a/src/bin/auth/tests/query_unittest.cc +++ b/src/bin/auth/tests/query_unittest.cc @@ -12,12 +12,13 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. +#include #include #include -#include #include #include +#include #include @@ -2367,4 +2368,172 @@ TEST_F(QueryTest, emptyNameWithNSEC3) { EXPECT_TRUE(result->isNSEC3Signed()); EXPECT_FALSE(result->isWildcard()); } + +// Class to allow checking of duplication removal in messages resulting from. +// the query. This class allows the setting of the answers, authorities and +// additionals vector in the Query class, as well as the ability to call the +// createResponse() method. + +class DuplicateQuery : public isc::auth::Query { +public: + // \brief Constructor + // + // Fill in the parts of Query that we test in the DuplicateRemoval test. + DuplicateQuery(isc::dns::Message* message, + const vector& answers, + const vector& authorities, + const vector& additionals) : Query() { + response_ = message; + copy(answers.begin(), answers.end(), + back_inserter(answers_)); + copy(authorities.begin(), authorities.end(), + back_inserter(authorities_)); + copy(additionals.begin(), additionals.end(), + back_inserter(additionals_)); + } + + // \brief Create Response + // + // Public interface to the (protected) Query::createResponse() method. + void produceResponse() { + createResponse(); + } +}; + +// Vector of RRsets used for the test. Having this external to functions and +// classes used for the testing simplifies the code. +std::vector 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 authority 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 + // 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 answer; + copy(rrset_vector.begin() + 2, rrset_vector.begin() + 4, + back_inserter(answer)); + copy(rrset_vector.begin() + 0, rrset_vector.begin() + 2, + back_inserter(answer)); + + std::vector authority; + copy(rrset_vector.begin() + 3, rrset_vector.begin() + 8, + back_inserter(authority)); + authority.push_back(rrset_vector[2]); + + std::vector additional; + copy(rrset_vector.begin() + 7, rrset_vector.begin() + 9, + back_inserter(additional)); + 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. + DuplicateQuery query(&message, answer, authority, additional); + query.produceResponse(); + + // 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().toText(); + 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])); + } + } + } +} }