Merge branch 'work/dname/memzone'

src/lib/datasrc/memory_datasrc.cc

@@ -35,13 +35,13 @@ namespace datasrc {
 struct MemoryZone::MemoryZoneImpl {
     // Constructor
     MemoryZoneImpl(const RRClass& zone_class, const Name& origin) :
-        zone_class_(zone_class), origin_(origin)
-    {}
-
-    // Information about the zone
-    RRClass zone_class_;
-    Name origin_;
-    string file_name_;
+        zone_class_(zone_class), origin_(origin), origin_data_(NULL)
+    {
+        // We create the node for origin (it needs to exist anyway in future)
+        domains_.insert(origin, &origin_data_);
+        DomainPtr origin_domain(new Domain);
+        origin_data_->setData(origin_domain);
+    }
 
     // Some type aliases
     /*
@@ -61,9 +61,58 @@ struct MemoryZone::MemoryZoneImpl {
     // The tree stores domains
     typedef RBTree<Domain> DomainTree;
     typedef RBNode<Domain> DomainNode;
+
+    // Information about the zone
+    RRClass zone_class_;
+    Name origin_;
+    DomainNode* origin_data_;
+    string file_name_;
+
     // The actual zone data
     DomainTree domains_;
 
+    /*
+     * Does some checks in context of the data that are already in the zone.
+     * Currently checks for forbidden combinations of RRsets in the same
+     * domain (CNAME+anything, DNAME+NS).
+     *
+     * If such condition is found, it throws AddError.
+     */
+    void contextCheck(const ConstRRsetPtr& rrset,
+                      const DomainPtr& domain) const {
+        // Ensure CNAME and other type of RR don't coexist for the same
+        // owner name.
+        if (rrset->getType() == RRType::CNAME()) {
+            // XXX: this check will become incorrect when we support DNSSEC
+            // (depending on how we support DNSSEC).  We should revisit it
+            // at that point.
+            if (!domain->empty()) {
+                isc_throw(AddError, "CNAME can't be added with other data for "
+                          << rrset->getName());
+            }
+        } else if (domain->find(RRType::CNAME()) != domain->end()) {
+            isc_throw(AddError, "CNAME and " << rrset->getType() <<
+                      " can't coexist for " << rrset->getName());
+        }
+
+        /*
+         * Similar with DNAME, but it must not coexist only with NS and only in
+         * non-apex domains.
+         * RFC 2672 section 3 mentions that it is implied from it and RFC 2181
+         */
+        if (rrset->getName() != origin_ &&
+            // Adding DNAME, NS already there
+            ((rrset->getType() == RRType::DNAME() &&
+            domain->find(RRType::NS()) != domain->end()) ||
+            // Adding NS, DNAME already there
+            (rrset->getType() == RRType::NS() &&
+            domain->find(RRType::DNAME()) != domain->end())))
+        {
+            isc_throw(AddError, "DNAME can't coexist with NS in non-apex "
+                "domain " << rrset->getName());
+        }
+    }
+
     /*
      * Implementation of longer methods. We put them here, because the
      * access is without the impl_-> and it will get inlined anyway.
@@ -74,10 +123,14 @@ struct MemoryZone::MemoryZoneImpl {
         if (!rrset) {
             isc_throw(NullRRset, "The rrset provided is NULL");
         }
-        if (rrset->getType() == RRType::CNAME() &&
-            rrset->getRdataCount() > 1) {
-            // XXX: this is not only for CNAME.  We should generalize this
-            // code for all other "singleton RR types" (such as SOA) in a
+        // Check for singleton RRs. It should probably handled at a different
+        // in future.
+        if ((rrset->getType() == RRType::CNAME() ||
+            rrset->getType() == RRType::DNAME()) &&
+            rrset->getRdataCount() > 1)
+        {
+            // XXX: this is not only for CNAME or DNAME. We should generalize
+            // this code for all other "singleton RR types" (such as SOA) in a
             // separate task.
             isc_throw(AddError, "multiple RRs of singleton type for "
                       << rrset->getName());
@@ -114,24 +167,12 @@ struct MemoryZone::MemoryZoneImpl {
             domain = node->getData();
         }
 
-        // Ensure CNAME and other type of RR don't coexist for the same
-        // owner name.
+        // Checks related to the surrounding data.
         // Note: when the check fails and the exception is thrown, it may
         // break strong exception guarantee.  At the moment we prefer
         // code simplicity and don't bother to introduce complicated
         // recovery code.
-        if (rrset->getType() == RRType::CNAME()) {
-            // XXX: this check will become incorrect when we support DNSSEC
-            // (depending on how we support DNSSEC).  We should revisit it
-            // at that point.
-            if (!domain->empty()) {
-                isc_throw(AddError, "CNAME can't be added with other data for "
-                          << rrset->getName());
-            }
-        } else if (domain->find(RRType::CNAME()) != domain->end()) {
-            isc_throw(AddError, "CNAME and " << rrset->getType() <<
-                      " can't coexist for " << rrset->getName());
-        }
+        contextCheck(rrset, domain);
 
         // Try inserting the rrset there
         if (domain->insert(DomainPair(rrset->getType(), rrset)).second) {
@@ -139,10 +180,12 @@ struct MemoryZone::MemoryZoneImpl {
 
             // If this RRset creates a zone cut at this node, mark the node
             // indicating the need for callback in find().
-            // TBD: handle DNAME, too
             if (rrset->getType() == RRType::NS() &&
                 rrset->getName() != origin_) {
                 node->enableCallback();
+            // If it is DNAME, we have a callback as well here
+            } else if (rrset->getType() == RRType::DNAME()) {
+                node->enableCallback();
             }
 
             return (result::SUCCESS);
@@ -174,31 +217,56 @@ struct MemoryZone::MemoryZoneImpl {
     /// It will be passed to \c zonecutCallback() and record a possible
     /// zone cut node and related RRset (normally NS or DNAME).
     struct FindState {
-        FindState(FindOptions options) : zonecut_node_(NULL),
-                                         options_(options)
+        FindState(FindOptions options) :
+            zonecut_node_(NULL),
+            dname_node_(NULL),
+            options_(options)
         {}
         const DomainNode* zonecut_node_;
+        const DomainNode* dname_node_;
         ConstRRsetPtr rrset_;
         const FindOptions options_;
     };
 
-    // A callback called from possible zone cut nodes.  This will be passed
-    // from the \c find() method to \c RBTree::find().
-    static bool zonecutCallback(const DomainNode& node, FindState* state) {
-        // We perform callback check only for the highest zone cut in the
-        // rare case of nested zone cuts.
-        if (state->zonecut_node_ != NULL) {
-            return (false);
+    // A callback called from possible zone cut nodes and nodes with DNAME.
+    // This will be passed from the \c find() method to \c RBTree::find().
+    static bool cutCallback(const DomainNode& node, FindState* state) {
+        // We need to look for DNAME first, there's allowed case where
+        // DNAME and NS coexist in the apex. DNAME is the one to notice,
+        // the NS is authoritative, not delegation (corner case explicitly
+        // allowed by section 3 of 2672)
+        const Domain::const_iterator foundDNAME(node.getData()->find(
+            RRType::DNAME()));
+        if (foundDNAME != node.getData()->end()) {
+            state->dname_node_ = &node;
+            state->rrset_ = foundDNAME->second;
+            // No more processing below the DNAME (RFC 2672, section 3
+            // forbids anything to exist below it, so there's no need
+            // to actually search for it). This is strictly speaking
+            // a different way than described in 4.1 of that RFC,
+            // but because of the assumption in section 3, it has the
+            // same behaviour.
+            return (true);
         }
 
-        const Domain::const_iterator found(node.getData()->find(RRType::NS()));
-        if (found != node.getData()->end()) {
-            // BIND 9 checks if this node is not the origin.  But it cannot
-            // be the origin because we don't enable the callback at the
-            // origin node (see MemoryZoneImpl::add()).  Or should we do a
-            // double check for it?
+        // Look for NS
+        const Domain::const_iterator foundNS(node.getData()->find(
+            RRType::NS()));
+        if (foundNS != node.getData()->end()) {
+            // We perform callback check only for the highest zone cut in the
+            // rare case of nested zone cuts.
+            if (state->zonecut_node_ != NULL) {
+                return (false);
+            }
+
+            // BIND 9 checks if this node is not the origin.  That's probably
+            // because it can support multiple versions for dynamic updates
+            // and IXFR, and it's possible that the callback is called at
+            // the apex and the DNAME doesn't exist for a particular version.
+            // It cannot happen for us (at least for now), so we don't do
+            // that check.
             state->zonecut_node_ = &node;
-            state->rrset_ = found->second;
+            state->rrset_ = foundNS->second;
 
             // Unless glue is allowed the search stops here, so we return
             // false; otherwise return true to continue the search.
@@ -222,8 +290,31 @@ struct MemoryZone::MemoryZoneImpl {
         // Get the node
         DomainNode* node(NULL);
         FindState state(options);
-        switch (domains_.find(name, &node, zonecutCallback, &state)) {
+        switch (domains_.find(name, &node, cutCallback, &state)) {
             case DomainTree::PARTIALMATCH:
+                /*
+                 * In fact, we could use a single variable instead of
+                 * dname_node_ and zonecut_node_. But then we would need
+                 * to distinquish these two cases by something else and
+                 * it seemed little more confusing to me when I wrote it.
+                 *
+                 * Usually at most one of them will be something else than
+                 * NULL (it might happen both are NULL, in which case we
+                 * consider it NOT FOUND). There's one corner case when
+                 * both might be something else than NULL and it is in case
+                 * there's a DNAME under a zone cut and we search in
+                 * glue OK mode ‒ in that case we don't stop on the domain
+                 * with NS and ignore it for the answer, but it gets set
+                 * anyway. Then we find the DNAME and we need to act by it,
+                 * therefore we first check for DNAME and then for NS. In
+                 * all other cases it doesn't matter, as at least one of them
+                 * is NULL.
+                 */
+                if (state.dname_node_ != NULL) {
+                    // We were traversing a DNAME node (and wanted to go
+                    // lower below it), so return the DNAME
+                    return (FindResult(DNAME, state.rrset_));
+                }
                 if (state.zonecut_node_ != NULL) {
                     return (FindResult(DELEGATION, state.rrset_));
                 }
@@ -244,8 +335,8 @@ struct MemoryZone::MemoryZoneImpl {
         Domain::const_iterator found;
 
         // If the node callback is enabled, this may be a zone cut.  If it
-        // has a NS RR, we should return a delegation.
-        if (node->isCallbackEnabled()) {
+        // has a NS RR, we should return a delegation, but not in the apex.
+        if (node->isCallbackEnabled() && node != origin_data_) {
             found = node->getData()->find(RRType::NS());
             if (found != node->getData()->end()) {
                 return (FindResult(DELEGATION, found->second));

src/lib/datasrc/tests/memory_datasrc_unittest.cc

 // Copyright (C) 2010  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011  CZ NIC
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -147,10 +148,12 @@ public:
         origin_("example.org"),
         ns_name_("ns.example.org"),
         cname_name_("cname.example.org"),
+        dname_name_("dname.example.org"),
         child_ns_name_("child.example.org"),
         child_glue_name_("ns.child.example.org"),
         grandchild_ns_name_("grand.child.example.org"),
         grandchild_glue_name_("ns.grand.child.example.org"),
+        child_dname_name_("dname.child.example.org"),
         zone_(class_, origin_),
         rr_out_(new RRset(Name("example.com"), class_, RRType::A(),
             RRTTL(300))),
@@ -160,6 +163,12 @@ public:
         rr_a_(new RRset(origin_, class_, RRType::A(), RRTTL(300))),
         rr_cname_(new RRset(cname_name_, class_, RRType::CNAME(), RRTTL(300))),
         rr_cname_a_(new RRset(cname_name_, class_, RRType::A(), RRTTL(300))),
+        rr_dname_(new RRset(dname_name_, class_, RRType::DNAME(), RRTTL(300))),
+        rr_dname_a_(new RRset(dname_name_, class_, RRType::A(),
+            RRTTL(300))),
+        rr_dname_ns_(new RRset(dname_name_, class_, RRType::NS(), RRTTL(300))),
+        rr_dname_apex_(new RRset(origin_, class_, RRType::DNAME(),
+            RRTTL(300))),
         rr_child_ns_(new RRset(child_ns_name_, class_, RRType::NS(),
                                RRTTL(300))),
         rr_child_glue_(new RRset(child_glue_name_, class_, RRType::A(),
@@ -167,13 +176,16 @@ public:
         rr_grandchild_ns_(new RRset(grandchild_ns_name_, class_, RRType::NS(),
                                     RRTTL(300))),
         rr_grandchild_glue_(new RRset(grandchild_glue_name_, class_,
-                                      RRType::AAAA(), RRTTL(300)))
+                                      RRType::AAAA(), RRTTL(300))),
+        rr_child_dname_(new RRset(child_dname_name_, class_, RRType::DNAME(),
+            RRTTL(300)))
     {
     }
     // Some data to test with
     const RRClass class_;
-    const Name origin_, ns_name_, cname_name_, child_ns_name_,
-        child_glue_name_, grandchild_ns_name_, grandchild_glue_name_;
+    const Name origin_, ns_name_, cname_name_, dname_name_, child_ns_name_,
+        child_glue_name_, grandchild_ns_name_, grandchild_glue_name_,
+        child_dname_name_;
     // The zone to torture by tests
     MemoryZone zone_;
 
@@ -196,10 +208,15 @@ public:
         rr_a_;
     RRsetPtr rr_cname_;         // CNAME in example.org (RDATA will be added)
     ConstRRsetPtr rr_cname_a_; // for mixed CNAME + A case
+    RRsetPtr rr_dname_;         // DNAME in example.org (RDATA will be added)
+    ConstRRsetPtr rr_dname_a_; // for mixed DNAME + A case
+    ConstRRsetPtr rr_dname_ns_; // for mixed DNAME + NS case
+    ConstRRsetPtr rr_dname_apex_; // for mixed DNAME + NS case in the apex
     ConstRRsetPtr rr_child_ns_; // NS of a child domain (for delegation)
     ConstRRsetPtr rr_child_glue_; // glue RR of the child domain
     ConstRRsetPtr rr_grandchild_ns_; // NS below a zone cut (unusual)
     ConstRRsetPtr rr_grandchild_glue_; // glue RR below a deeper zone cut
+    ConstRRsetPtr rr_child_dname_; // A DNAME under NS
 
     /**
      * \brief Test one find query to the zone.
@@ -318,6 +335,81 @@ TEST_F(MemoryZoneTest, findCNAMEUnderZoneCut) {
              Zone::FIND_GLUE_OK);
 }
 
+// Two DNAMEs at single domain are disallowed by RFC 2672, section 3)
+// Having a CNAME there is disallowed too, but it is tested by
+// addOtherThenCNAME and addCNAMEThenOther.
+TEST_F(MemoryZoneTest, addMultipleDNAMEs) {
+    rr_dname_->addRdata(generic::DNAME("dname1.example.org."));
+    rr_dname_->addRdata(generic::DNAME("dname2.example.org."));
+    EXPECT_THROW(zone_.add(rr_dname_), MemoryZone::AddError);
+}
+
+/*
+ * These two tests ensure that we can't have DNAME and NS at the same
+ * node with the exception of the apex of zone (forbidden by RFC 2672)
+ */
+TEST_F(MemoryZoneTest, addDNAMEThenNS) {
+    EXPECT_NO_THROW(EXPECT_EQ(SUCCESS, zone_.add(rr_dname_)));
+    EXPECT_THROW(zone_.add(rr_dname_ns_), MemoryZone::AddError);
+}
+
+TEST_F(MemoryZoneTest, addNSThenDNAME) {
+    EXPECT_NO_THROW(EXPECT_EQ(SUCCESS, zone_.add(rr_dname_ns_)));
+    EXPECT_THROW(zone_.add(rr_dname_), MemoryZone::AddError);
+}
+
+// It is allowed to have NS and DNAME at apex
+TEST_F(MemoryZoneTest, DNAMEAndNSAtApex) {
+    EXPECT_NO_THROW(EXPECT_EQ(SUCCESS, zone_.add(rr_dname_apex_)));
+    EXPECT_NO_THROW(EXPECT_EQ(SUCCESS, zone_.add(rr_ns_)));
+
+    // The NS should be possible to be found, below should be DNAME, not
+    // delegation
+    findTest(origin_, RRType::NS(), Zone::SUCCESS, true, rr_ns_);
+    findTest(child_ns_name_, RRType::A(), Zone::DNAME, true, rr_dname_apex_);
+}
+
+TEST_F(MemoryZoneTest, NSAndDNAMEAtApex) {
+    EXPECT_NO_THROW(EXPECT_EQ(SUCCESS, zone_.add(rr_ns_)));
+    EXPECT_NO_THROW(EXPECT_EQ(SUCCESS, zone_.add(rr_dname_apex_)));
+}
+
+// TODO: Test (and implement) adding data under DNAME. That is forbidden by
+// 2672 as well.
+
+// Search under a DNAME record. It should return the DNAME
+TEST_F(MemoryZoneTest, findBelowDNAME) {
+    rr_dname_->addRdata(generic::DNAME("target.example.org."));
+    EXPECT_NO_THROW(EXPECT_EQ(SUCCESS, zone_.add(rr_dname_)));
+    findTest(Name("below.dname.example.org"), RRType::A(), Zone::DNAME, true,
+        rr_dname_);
+}
+
+// Search at the domain with DNAME. It should act as DNAME isn't there, DNAME
+// influences only the data below (see RFC 2672, section 3)
+TEST_F(MemoryZoneTest, findAtDNAME) {
+    rr_dname_->addRdata(generic::DNAME("target.example.org."));
+    EXPECT_NO_THROW(EXPECT_EQ(SUCCESS, zone_.add(rr_dname_)));
+    EXPECT_NO_THROW(EXPECT_EQ(SUCCESS, zone_.add(rr_dname_a_)));
+
+    findTest(dname_name_, RRType::A(), Zone::SUCCESS, true, rr_dname_a_);
+    findTest(dname_name_, RRType::DNAME(), Zone::SUCCESS, true, rr_dname_);
+    findTest(dname_name_, RRType::TXT(), Zone::NXRRSET, true);
+}
+
+// Try searching something that is both under NS and DNAME, without and with
+// GLUE_OK mode (it should stop at the NS and DNAME respectively).
+TEST_F(MemoryZoneTest, DNAMEUnderNS) {
+    zone_.add(rr_child_ns_);
+    zone_.add(rr_child_dname_);
+
+    Name lowName("below.dname.child.example.org.");
+
+    findTest(lowName, RRType::A(), Zone::DELEGATION, true, rr_child_ns_);
+    findTest(lowName, RRType::A(), Zone::DNAME, true, rr_child_dname_, NULL,
+        NULL, Zone::FIND_GLUE_OK);
+}
+
 // Test adding child zones and zone cut handling
 TEST_F(MemoryZoneTest, delegationNS) {
     // add in-zone data
@@ -435,18 +527,6 @@ TEST_F(MemoryZoneTest, glue) {
              Zone::FIND_GLUE_OK);
 }
 
-// Test adding DNAMEs and resulting delegation handling
-// Listing ideas only for now
-TEST_F(MemoryZoneTest, delegationDNAME) {
-    // apex DNAME: allowed by spec.  No DNAME delegation at the apex;
-    // descendants are subject to delegation.
-
-    // Other cases of NS and DNAME mixture are prohibited.
-    // BIND 9 doesn't reject such cases at load time, however.
-
-    // DNAME and ordinary types (allowed by spec)
-}
-
 /**
  * \brief Test searching.
  *