From: JINMEI Tatuya Date: Sat, 16 Jun 2012 00:22:07 +0000 (-0700) Subject: [1771-2] remove seen_ns&other check in getRRsets. X-Git-Tag: trac2351_base~213^2^2~1^2~4 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=ef571c297aabab0808ac3eecf3c457fdce55a73d;p=thirdparty%2Fkea.git [1771-2] remove seen_ns&other check in getRRsets. on a closer look, it just doesn't seem to be necessary if we are okay with just "hiding" records under a delegation NS for normal (ie. non GLUE_OK) lookups, instead of throwing an exception. Essentially only one existing test case (which expects an exception in that setup) should be updated (and the "brokenns2" case just seems to be redundant and so was removed), and the disabled tests in zone_finder_context_unittest now pass. --- diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc index 358dce8aad..f824acea95 100644 --- a/src/lib/datasrc/database.cc +++ b/src/lib/datasrc/database.cc @@ -206,7 +206,6 @@ DatabaseClient::Finder::getRRsets(const string& name, const WantedTypes& types, bool seen_cname(false); bool seen_ds(false); bool seen_other(false); - bool seen_ns(false); while (context->getNext(columns)) { // The domain is not empty @@ -249,8 +248,6 @@ DatabaseClient::Finder::getRRsets(const string& name, const WantedTypes& types, if (cur_type == RRType::CNAME()) { seen_cname = true; - } else if (cur_type == RRType::NS()) { - seen_ns = true; } else if (cur_type == RRType::DS()) { seen_ds = true; } else if (cur_type != RRType::RRSIG() && @@ -278,11 +275,11 @@ DatabaseClient::Finder::getRRsets(const string& name, const WantedTypes& types, RDATA_COLUMN]); } } - if (seen_cname && (seen_other || seen_ns || seen_ds)) { + if (seen_cname && (seen_other || seen_ds)) { isc_throw(DataSourceError, "CNAME shares domain " << name << " with something else"); } - if (check_ns && seen_ns && seen_other) { + if (check_ns && /*seen_ns*/ false && seen_other) { isc_throw(DataSourceError, "NS shares domain " << name << " with something else"); } diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc index 55d8052653..0302f0c253 100644 --- a/src/lib/datasrc/tests/database_unittest.cc +++ b/src/lib/datasrc/tests/database_unittest.cc @@ -168,13 +168,16 @@ const char* const TEST_RECORDS[][5] = { {"child.insecdelegation.example.org.", "DS", "3600", "", "DS 5 3 3600 " "20000101000000 20000201000000 12345 example.org. FAKEFAKEFAKE"}, - // Broken NS + // Delegation NS and other ordinary type of RR coexist at the same + // name. This is deviant (except for some special cases like the other + // RR could be used for addressing the NS name), but as long as the + // other records are hidden behind the delegation for normal queries + // it's not necessarily harmful. (so "broken" may be too strong, but we + // keep the name since it could be in a chain of sorted names for DNSSEC + // processing and renaming them may have other bad effects for tests). {"brokenns1.example.org.", "A", "3600", "", "192.0.2.1"}, {"brokenns1.example.org.", "NS", "3600", "", "ns.example.com."}, - {"brokenns2.example.org.", "NS", "3600", "", "ns.example.com."}, - {"brokenns2.example.org.", "A", "3600", "", "192.0.2.1"}, - // Now double DNAME, to test failure mode {"baddname.example.org.", "DNAME", "3600", "", "dname1.example.com."}, {"baddname.example.org.", "DNAME", "3600", "", "dname2.example.com."}, @@ -2202,15 +2205,23 @@ TYPED_TEST(DatabaseClientTest, findDelegation) { ZoneFinder::FIND_DEFAULT), DataSourceError); - // Broken NS - it lives together with something else - EXPECT_THROW(finder->find(isc::dns::Name("brokenns1.example.org."), - this->qtype_, - ZoneFinder::FIND_DEFAULT), - DataSourceError); - EXPECT_THROW(finder->find(isc::dns::Name("brokenns2.example.org."), - this->qtype_, - ZoneFinder::FIND_DEFAULT), - DataSourceError); + // NS and other type coexist: deviant and not necessarily harmful. + // It should normally just result in DELEGATION; if GLUE_OK is specified, + // the other RR should be visible. + this->expected_rdatas_.clear(); + this->expected_rdatas_.push_back("ns.example.com"); + doFindTest(*finder, Name("brokenns1.example.org"), this->qtype_, + RRType::NS(), this->rrttl_, ZoneFinder::DELEGATION, + this->expected_rdatas_, this->empty_rdatas_, + ZoneFinder::RESULT_DEFAULT); + + this->expected_rdatas_.clear(); + this->expected_rdatas_.push_back("192.0.2.1"); + doFindTest(*finder, Name("brokenns1.example.org"), this->qtype_, + this->qtype_, this->rrttl_, ZoneFinder::SUCCESS, + this->expected_rdatas_, this->empty_rdatas_, + ZoneFinder::RESULT_DEFAULT, Name("brokenns1.example.org"), + ZoneFinder::FIND_GLUE_OK); } TYPED_TEST(DatabaseClientTest, findDS) { diff --git a/src/lib/datasrc/tests/zone_finder_context_unittest.cc b/src/lib/datasrc/tests/zone_finder_context_unittest.cc index 50d409eb4c..b3c9b2d2fe 100644 --- a/src/lib/datasrc/tests/zone_finder_context_unittest.cc +++ b/src/lib/datasrc/tests/zone_finder_context_unittest.cc @@ -208,13 +208,6 @@ TEST_P(ZoneFinderContextTest, getAdditionalDelegation) { TEST_P(ZoneFinderContextTest, getAdditionalDelegationAtZoneCut) { // Similar to the previous case, but one of the NS addresses is at the // zone cut. - - // XXX: the current database-based data source incorrectly rejects this - // setup (see #1771) - if (GetParam() == createSQLite3Client) { - return; - } - ZoneFinderContextPtr ctx = finder_->find(Name("www.b.example.org"), RRType::SOA()); EXPECT_EQ(ZoneFinder::DELEGATION, ctx->code); @@ -316,12 +309,6 @@ TEST_P(ZoneFinderContextTest, getAdditionalMX) { } TEST_P(ZoneFinderContextTest, getAdditionalMXAtZoneCut) { - // XXX: the current database-based data source incorrectly rejects this - // setup (see #1771) - if (GetParam() == createSQLite3Client) { - return; - } - ZoneFinderContextPtr ctx = finder_->find(Name("mxatcut.example.org."), RRType::MX()); EXPECT_EQ(ZoneFinder::SUCCESS, ctx->code);