]> git.ipfire.org Git - thirdparty/kea.git/commitdiff
[3977] Addressed review comments.
authorMarcin Siodelski <marcin@isc.org>
Mon, 26 Oct 2015 19:50:30 +0000 (20:50 +0100)
committerMarcin Siodelski <marcin@isc.org>
Mon, 26 Oct 2015 19:50:30 +0000 (20:50 +0100)
12 files changed:
src/bin/dhcp4/dhcp4_srv.cc
src/bin/dhcp4/dhcp4_srv.h
src/bin/dhcp4/tests/dhcp4_test_utils.h
src/bin/dhcp4/tests/fqdn_unittest.cc
src/bin/dhcp6/tests/fqdn_unittest.cc
src/lib/dhcp/pkt6.h
src/lib/dhcp/tests/pkt4_unittest.cc
src/lib/dhcpsrv/alloc_engine_messages.mes
src/lib/dhcpsrv/dhcpsrv_messages.mes
src/lib/dhcpsrv/ncr_generator.cc
src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc
src/lib/dhcpsrv/tests/ncr_generator_unittest.cc

index 1b7279895cc407fb37dac4a8777db3dab2274b52..43b4cb7ff14f5ad2e67111fc5ed2cf529f6bebb7 100644 (file)
@@ -774,51 +774,6 @@ Dhcpv4Srv::srvidToString(const OptionPtr& srvid) {
     return (addrs[0].toText());
 }
 
-isc::dhcp_ddns::D2Dhcid
-Dhcpv4Srv::computeDhcid(const Lease4Ptr& lease) {
-    if (!lease) {
-        isc_throw(DhcidComputeError, "a pointer to the lease must be not"
-                  " NULL to compute DHCID");
-
-    } else if (lease->hostname_.empty()) {
-        isc_throw(DhcidComputeError, "unable to compute the DHCID for the"
-                  " lease which has empty hostname set");
-
-    }
-
-    // In order to compute DHCID the client's hostname must be encoded in
-    // canonical wire format. It is unlikely that the FQDN is malformed
-    // because it is validated by the classes which encapsulate options
-    // carrying client FQDN. However, if the client name was carried in the
-    // Hostname option it is more likely as it carries the hostname as a
-    // regular string.
-    std::vector<uint8_t> fqdn_wire;
-    try {
-        OptionDataTypeUtil::writeFqdn(lease->hostname_, fqdn_wire, true);
-
-    } catch (const Exception& ex) {
-        isc_throw(DhcidComputeError, "unable to compute DHCID because the"
-                  " hostname: " << lease->hostname_ << " is invalid");
-
-    }
-
-    // Prefer client id to HW address to compute DHCID. If Client Id is
-    // NULL, use HW address.
-    try {
-        if (lease->client_id_) {
-            return (D2Dhcid(lease->client_id_->getClientId(), fqdn_wire));
-
-        } else {
-            return (D2Dhcid(lease->hwaddr_, fqdn_wire));
-        }
-    } catch (const Exception& ex) {
-        isc_throw(DhcidComputeError, "unable to compute DHCID: "
-                  << ex.what());
-
-    }
-
-}
-
 void
 Dhcpv4Srv::appendServerID(Dhcpv4Exchange& ex) {
     // The source address for the outbound message should have been set already.
index 9cee662f1230e1585c6aea4ced2b2bf9efd8e6e9..72235e9c7c737172751f024b9667155419e9fdd3 100644 (file)
 namespace isc {
 namespace dhcp {
 
-/// @brief Exception thrown when DHCID computation failed.
-class DhcidComputeError : public isc::Exception {
-public:
-    DhcidComputeError(const char* file, size_t line, const char* what) :
-        isc::Exception(file, line, what) { };
-};
-
 /// @brief DHCPv4 message exchange.
 ///
 /// This class represents the DHCPv4 message exchange. The message exchange
@@ -666,28 +659,6 @@ protected:
     /// @return string representation
     static std::string srvidToString(const OptionPtr& opt);
 
-    /// @brief Computes DHCID from a lease.
-    ///
-    /// This method creates an object which represents DHCID. The DHCID value
-    /// is computed as described in RFC4701. The section 3.3. of RFC4701
-    /// specifies the DHCID RR Identifier Type codes:
-    /// - 0x0000 The 1 octet htype followed by glen octets of chaddr
-    /// - 0x0001 The data octets from the DHCPv4 client's Client Identifier
-    /// option.
-    /// - 0x0002 The client's DUID.
-    ///
-    /// Currently this function supports first two of these identifiers.
-    /// The 0x0001 is preferred over the 0x0000 - if the client identifier
-    /// option is present, the former is used. If the client identifier
-    /// is absent, the latter is used.
-    ///
-    /// @todo Implement support for 0x0002 DHCID identifier type.
-    ///
-    /// @param lease A pointer to the structure describing a lease.
-    /// @return An object encapsulating DHCID to be used for DNS updates.
-    /// @throw DhcidComputeError If the computation of the DHCID failed.
-    static isc::dhcp_ddns::D2Dhcid computeDhcid(const Lease4Ptr& lease);
-
     /// @brief Selects a subnet for a given client's packet.
     ///
     /// @param query client's message
index 13008b9f33939899d4ddf6611535846e7cb94fe4..333f700d20278d1dd5d55bd85f9a0a71cccc680c 100644 (file)
@@ -198,7 +198,6 @@ public:
     using Dhcpv4Srv::processDecline;
     using Dhcpv4Srv::processInform;
     using Dhcpv4Srv::processClientName;
-    using Dhcpv4Srv::computeDhcid;
     using Dhcpv4Srv::createNameChangeRequests;
     using Dhcpv4Srv::acceptServerId;
     using Dhcpv4Srv::sanityCheck;
index 59a345b61c636ec2c850800da590d6dadde48c34..13610c299bfcf9d6dbe53dd657ad082f5f540a47 100644 (file)
@@ -489,63 +489,6 @@ public:
     }
 };
 
-// Test that the exception is thrown if lease pointer specified as the argument
-// of computeDhcid function is NULL.
-TEST_F(NameDhcpv4SrvTest, dhcidNullLease) {
-    Lease4Ptr lease;
-    EXPECT_THROW(srv_->computeDhcid(lease), isc::dhcp::DhcidComputeError);
-
-}
-
-// Test that the appropriate exception is thrown if the lease object used
-// to compute DHCID comprises wrong hostname.
-TEST_F(NameDhcpv4SrvTest, dhcidWrongHostname) {
-    // First, make sure that the lease with the correct hostname is accepted.
-    Lease4Ptr lease = createLease(IOAddress("192.0.2.3"),
-                                  "myhost.example.com.", true, true);
-    ASSERT_NO_THROW(srv_->computeDhcid(lease));
-
-    // Now, use the wrong hostname. It should result in the exception.
-    lease->hostname_ = "myhost...example.com.";
-    EXPECT_THROW(srv_->computeDhcid(lease), isc::dhcp::DhcidComputeError);
-
-    // Also, empty hostname is wrong.
-    lease->hostname_ = "";
-    EXPECT_THROW(srv_->computeDhcid(lease), isc::dhcp::DhcidComputeError);
-}
-
-// Test that the DHCID is computed correctly, when the lease holds
-// correct hostname and non-NULL client id.
-TEST_F(NameDhcpv4SrvTest, dhcidComputeFromClientId) {
-    Lease4Ptr lease = createLease(IOAddress("192.0.2.3"),
-                                  "myhost.example.com.",
-                                  true, true);
-    isc::dhcp_ddns::D2Dhcid dhcid;
-    ASSERT_NO_THROW(dhcid = srv_->computeDhcid(lease));
-
-    // Make sure that the computed DHCID is valid.
-    std::string dhcid_ref = "00010132E91AA355CFBB753C0F0497A5A9404"
-        "36965B68B6D438D98E680BF10B09F3BCF";
-    EXPECT_EQ(dhcid_ref, dhcid.toStr());
-}
-
-// Test that the DHCID is computed correctly, when the lease holds correct
-// hostname and NULL client id.
-TEST_F(NameDhcpv4SrvTest, dhcidComputeFromHWAddr) {
-    Lease4Ptr lease = createLease(IOAddress("192.0.2.3"),
-                                  "myhost.example.com.",
-                                  true, true);
-    lease->client_id_.reset();
-
-    isc::dhcp_ddns::D2Dhcid dhcid;
-    ASSERT_NO_THROW(dhcid = srv_->computeDhcid(lease));
-
-    // Make sure that the computed DHCID is valid.
-    std::string dhcid_ref = "0000012247F6DC4423C3E8627434A9D6868609"
-        "D88948F78018B215EDCAA30C0C135035";
-    EXPECT_EQ(dhcid_ref, dhcid.toStr());
-}
-
 // Tests the following scenario:
 //  - Updates are enabled
 //  - All overrides are off
index ef449021ece0332dae27da2d8c86bd27e24f2821..bffa932c5ee2cb5e1077a6d74047d9603fa6530b 100644 (file)
@@ -738,8 +738,8 @@ TEST_F(FqdnDhcpv6SrvTest, createRemovalNameChangeRequestFwdRev) {
 
 }
 
-// Checks that NameChangeRequests to remove entries are not created
-// when ddns updates are disabled.
+// Checks that calling queueNCR would not result in error if DDNS updates are
+// disabled. 
 TEST_F(FqdnDhcpv6SrvTest, noRemovalsWhenDisabled) {
     // Disable DDNS updates.
     disableD2();
@@ -751,7 +751,10 @@ TEST_F(FqdnDhcpv6SrvTest, noRemovalsWhenDisabled) {
     // as if we typed domain-name in lower case.
     lease_->hostname_ = "MYHOST.example.com.";
 
-    // When DDNS is disabled an attempt to send a request will throw.
+    // When DDNS is disabled an attempt to send a request should not throw, but
+    // nothing is generated. Unfortunately, we can't see if anything get
+    // generated because getting anything from the queue when DDNS is disabled
+    // would result in exception.
     ASSERT_NO_THROW(queueNCR(CHG_REMOVE, lease_));
 }
 
index 32d960a0c60bfd9896dfeeb5a04c6d6af9d42b14..87dab71a67ddaeb8515fda1e25ee10ff2e29a849 100644 (file)
@@ -178,7 +178,6 @@ public:
     /// This variant of the method does not include transaction id.
     ///
     /// @param duid Pointer to the client identifier or NULL.
-    /// @param transid Numeric transaction id to include in the string.
     /// @param hwaddr Hardware address to include in the string or NULL.
     ///
     /// @return String with text representation of the packet identifiers.
index 6f0ce0ed3fa7f8fd7831bdd9ee6d19a1d3275883..36f2b353f69886f76dff7b28c15e32da2492d8df 100644 (file)
@@ -1035,10 +1035,14 @@ TEST_F(Pkt4Test, makeLabelWithoutTransactionId) {
     EXPECT_EQ("[hwtype=123 01:02:03:04:05:06], cid=[no info]",
               Pkt4::makeLabel(hwaddr, ClientIdPtr()));
 
-    // Test non-null client identifier.
+    // Test non-null client identifier and non-null hardware address.
     ClientIdPtr cid = ClientId::fromText("01:02:03:04");
     EXPECT_EQ("[hwtype=123 01:02:03:04:05:06], cid=[01:02:03:04]",
               Pkt4::makeLabel(hwaddr, cid));
+
+    // Test non-nnull client identifier and null hardware address.
+    EXPECT_EQ("[no hwaddr info], cid=[01:02:03:04]",
+              Pkt4::makeLabel(HWAddrPtr(), cid));
 }
 
 // Tests that the correct DHCPv4 message name is returned for various
index d3f03f4526e081795c3f96d4d1c3ec4d1baab7cf..10da5681ff5af024d402679975f6f7e98f9fecbd 100644 (file)
@@ -350,7 +350,7 @@ This debug message is issued when the server begins reclamation of the
 expired DHCPv6 lease. The reclaimed lease may either be an address lease
 or delegated prefix. The first argument provides the client identification
 information. The other arguments specify the prefix and the prefix length
-for the lease. The prefix length for address lease is equal to.
+for the lease. The prefix length for address lease is equal to 128.
 
 % ALLOC_ENGINE_V6_LEASE_RECLAMATION_FAILED failed to reclaim the lease %1: %2
 This error message is logged when the allocation engine fails to
index 1fc4956932c65f17608129dea7f81d7e920edcfb..1b37ad8020e248fc88d3819a2741d1a4952d9f66 100644 (file)
@@ -589,16 +589,24 @@ lease from the PostgreSQL database for the specified address.
 
 % DHCPSRV_QUEUE_NCR %1: name change request to %2 DNS entry queued: %3
 A debug message which is logged when the NameChangeRequest to add or remove
-a DNS entries for a particular lease has been queued. The first parameter
-includes the client identification information. The second parameter of
-indicates whether the DNS entry is to be added or removed. The second
-parameter carries the details of the NameChangeRequest.
+a DNS entries for a particular lease has been queued. The first argument
+includes the client identification information. The second argument
+indicates whether the DNS entry is to be added or removed. The third
+argument carries the details of the NameChangeRequest.
 
-% DHCPSRV_QUEUE_NCR_FAILED queueing %1 name change request failed for lease %2: %3
+% DHCPSRV_QUEUE_NCR_FAILED %1: queueing %2 name change request failed for lease %3: %4
 This error message is logged when sending a name change request
-to DHCP DDNS failed. The type of the name change request is specified
-as first argument. The remaining arguments provide the details of the
-lease and the reson for failure.
+to DHCP DDNS failed. The first argument includes the client identification
+information. The second argument indicates whether the DNS entry is to be
+added or removed. The third argument specifies the leased address. The
+last argument provides the reason for failure.
+
+% DHCPSRV_QUEUE_NCR_SKIP %1: skip queueing name change request for lease: %2
+This debug message is issued when the server decides to not queue the name
+change request because the lease doesn't include the FQDN, the forward and
+reverse update is disabled for this lease or the DNS updates are disabled
+in the configuration. The first argument includes the client identification
+information. The second argument includes the leased address.
 
 % DHCPSRV_TIMERMGR_CALLBACK_FAILED running handler for timer %1 caused exception: %2
 This error message is emitted when the timer elapsed and the
index d4cbfe3d1a2412302f2e0047645488e4fa0afd99..477567cf40a0c3897033258181d61896280fd1d0 100644 (file)
@@ -44,8 +44,13 @@ void queueNCRCommon(const NameChangeType& chg_type, const LeasePtrType& lease,
                     const IdentifierType& identifier, const std::string& label) {
 
     // Check if there is a need for update.
-    if (!lease || lease->hostname_.empty() || (!lease->fqdn_fwd_ && !lease->fqdn_rev_)
+    if (lease->hostname_.empty() || (!lease->fqdn_fwd_ && !lease->fqdn_rev_)
         || !CfgMgr::instance().getD2ClientMgr().ddnsEnabled()) {
+        LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL,
+                  DHCPSRV_QUEUE_NCR_SKIP)
+            .arg(label)
+            .arg(lease->addr_.toText());
+            
         return;
     }
 
@@ -72,6 +77,7 @@ void queueNCRCommon(const NameChangeType& chg_type, const LeasePtrType& lease,
 
     } catch (const std::exception& ex) {
         LOG_ERROR(dhcpsrv_logger, DHCPSRV_QUEUE_NCR_FAILED)
+            .arg(label)
             .arg(chg_type == CHG_ADD ? "add" : "remove")
             .arg(lease->addr_.toText())
             .arg(ex.what());
index d909d8dd3b824e883ce31f08e8160f3e4597c41d..1d3d80e4ed71bfc15a99d658803f9f192ae6f22d 100644 (file)
@@ -1216,7 +1216,10 @@ ExpirationAllocEngine6Test::createLeases() {
                                    20, SubnetID(1), true, true,
                                    generateHostnameForLeaseIndex(i)));
         leases_.push_back(lease);
-        LeaseMgrFactory::instance().addLease(lease);
+        // Copy the lease before adding it to the lease manager. We want to
+        // make sure that modifications to the leases held in the leases_
+        // container doesn't affect the leases in the lease manager.
+        LeaseMgrFactory::instance().addLease(Lease6Ptr(new Lease6(*lease)));
 
         // Note in the statistics that this lease has been added.
         StatsMgr& stats_mgr = StatsMgr::instance();
@@ -1324,7 +1327,7 @@ ExpirationAllocEngine6Test::testReclaimExpiredLeasesStats() {
 void
 ExpirationAllocEngine6Test::testReclaimReusedLeases(const uint16_t msg_type,
                                                     const bool use_reclaimed) {
-    BOOST_STATIC_ASSERT(TEST_LEASES_NUM < 0xFFFF);
+    BOOST_STATIC_ASSERT(TEST_LEASES_NUM < 1000);
 
     for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) {
         // Depending on the parameter, mark leases 'expired-reclaimed' or
@@ -1368,7 +1371,7 @@ ExpirationAllocEngine6Test::testReclaimReusedLeases(const uint16_t msg_type,
             ASSERT_NO_THROW(engine_->renewLeases6(ctx));
 
         } else {
-            ASSERT_NO_THROW(engine_->renewLeases6(ctx));
+            ASSERT_NO_THROW(engine_->allocateLeases6(ctx));
         }
     }
 
@@ -1632,7 +1635,8 @@ public:
     /// @param client_renews A boolean value which indicates if the test should
     /// simulate renewals of leases (if true) or reusing expired leases which
     /// belong to different clients (if false).
-    /// @param use_reclaimed Boolean parameter indicating if the leases
+    /// @param use_reclaimed Boolean parameter indicating if the leases being
+    /// reused should initially be reclaimed.
     void testReclaimReusedLeases(const uint8_t msg_type, const bool client_renews,
                                  const bool use_reclaimed);
 
@@ -1666,7 +1670,10 @@ ExpirationAllocEngine4Test::createLeases() {
                                    time(NULL), SubnetID(1), true, true,
                                    generateHostnameForLeaseIndex(i)));
         leases_.push_back(lease);
-        LeaseMgrFactory::instance().addLease(lease);
+        // Copy the lease before adding it to the lease manager. We want to
+        // make sure that modifications to the leases held in the leases_
+        // container doesn't affect the leases in the lease manager.
+        LeaseMgrFactory::instance().addLease(Lease4Ptr(new Lease4(*lease)));
 
         // Note in the statistics that this lease has been added.
         StatsMgr& stats_mgr = StatsMgr::instance();
@@ -1865,8 +1872,8 @@ void
 ExpirationAllocEngine4Test::testReclaimReusedLeases(const uint8_t msg_type,
                                                     const bool client_renews,
                                                     const bool use_reclaimed) {
-    // Let's restrict the number of leases to /16.
-    BOOST_STATIC_ASSERT(TEST_LEASES_NUM < 0xFFFF);
+    // Let's restrict the number of leases.
+    BOOST_STATIC_ASSERT(TEST_LEASES_NUM < 1000);
 
     for (unsigned int i = 0; i < TEST_LEASES_NUM; ++i) {
         // Depending on the parameter, mark leases 'expired-reclaimed' or
index 5ef1e3f94d45881846949c5ac8eb1b54dee3c068..5f73cea613c4d77d15e6f33cc3d293560b8c940f 100644 (file)
@@ -232,6 +232,16 @@ public:
                                 lease_->cltt_ + lease_->valid_lft_,
                                 lease_->valid_lft_);
     }
+
+    /// @brief Test that calling queueNCR for NULL lease doesn't cause
+    /// an exception.
+    ///
+    /// @param chg_type Name change type.
+    void testNullLease(const NameChangeType chg_type) {
+        lease_.reset();
+        ASSERT_NO_FATAL_FAILURE(queueNCR(chg_type, lease_));
+        EXPECT_EQ(0, d2_mgr_.getQueueSize());
+    }
 };
 
 /// @brief Test fixture class implementation for DHCPv6.
@@ -270,12 +280,29 @@ TEST_F(NCRGenerator6Test, fwdRev) {
                 "000201BE0D7A66F8AB6C4082E7F8B81E2656667A102E3"
                 "D0ECCEA5E0DD71730F392119A");
     }
+
+    // Now try the same test with all lower case.
+    {
+        SCOPED_TRACE("case CHG_REMOVE");
+        testNCR(CHG_REMOVE, true, true, "myhost.example.com.",
+                "000201BE0D7A66F8AB6C4082E7F8B81E2656667A102E3"
+                "D0ECCEA5E0DD71730F392119A");
+    }
+
     {
         SCOPED_TRACE("case CHG_ADD");
         testNCR(CHG_ADD, true, true, "MYHOST.example.com.",
                 "000201BE0D7A66F8AB6C4082E7F8B81E2656667A102E3"
                 "D0ECCEA5E0DD71730F392119A");
     }
+
+    {
+        SCOPED_TRACE("case CHG_ADD");
+        testNCR(CHG_ADD, true, true, "myhost.example.com.",
+                "000201BE0D7A66F8AB6C4082E7F8B81E2656667A102E3"
+                "D0ECCEA5E0DD71730F392119A");
+    }
+
 }
 
 // Checks that NameChangeRequests are not created when ddns updates are disabled.
@@ -292,7 +319,7 @@ TEST_F(NCRGenerator6Test, d2Disabled) {
 
 // Test creation of the NameChangeRequest for reverse mapping in the
 // given lease.
-TEST_F(NCRGenerator6Test, removeRev) {
+TEST_F(NCRGenerator6Test, revOnly) {
     {
         SCOPED_TRACE("case CHG_REMOVE");
         testNCR(CHG_REMOVE, false, true, "myhost.example.com.",
@@ -308,6 +335,25 @@ TEST_F(NCRGenerator6Test, removeRev) {
     }
 }
 
+// Test creation of the NameChangeRequest for forward mapping in the
+// given lease.
+TEST_F(NCRGenerator6Test, fwdOnly) {
+    {
+        SCOPED_TRACE("case CHG_REMOVE");
+        testNCR(CHG_REMOVE, true, false, "myhost.example.com.",
+                "000201BE0D7A66F8AB6C4082E7F8B81E2656667A102E3"
+                "D0ECCEA5E0DD71730F392119A");
+    }
+
+    {
+        SCOPED_TRACE("case CHG_ADD");
+        testNCR(CHG_ADD, true, false, "myhost.example.com.",
+                "000201BE0D7A66F8AB6C4082E7F8B81E2656667A102E3"
+                "D0ECCEA5E0DD71730F392119A");
+    }
+}
+
+
 // Test that NameChangeRequest is not generated when neither forward
 // nor reverse DNS update has been performed for a lease.
 TEST_F(NCRGenerator6Test, noFwdRevUpdate) {
@@ -340,11 +386,11 @@ TEST_F(NCRGenerator6Test, noHostname) {
 TEST_F(NCRGenerator6Test, wrongHostname) {
     {
         SCOPED_TRACE("case CHG_REMOVE");
-        testNoUpdate(CHG_REMOVE, false, false, "");
+        testNoUpdate(CHG_REMOVE, false, false, "myhost...example.com.");
     }
     {
         SCOPED_TRACE("case CHG_ADD");
-        testNoUpdate(CHG_ADD, false, false, "");
+        testNoUpdate(CHG_ADD, false, false, "myhost...example.com.");
     }
 }
 
@@ -364,6 +410,20 @@ TEST_F(NCRGenerator6Test, wrongLeaseType) {
     }
 }
 
+// Test that NameChangeRequest is not generated if the lease is NULL,
+// and that the call to queueNCR doesn't cause an exception or
+// assertion.
+TEST_F(NCRGenerator6Test, nullLease) {
+    {
+        SCOPED_TRACE("case CHG_REMOVE");
+        testNullLease(CHG_REMOVE);
+    }
+    {
+        SCOPED_TRACE("case CHG_ADD");
+        testNullLease(CHG_ADD);
+    }
+}
+
 /// @brief Test fixture class implementation for DHCPv4.
 class NCRGenerator4Test : public NCRGeneratorTest<Lease4Ptr> {
 public:
@@ -380,7 +440,7 @@ public:
 
     /// @brief Implementation of the method creating DHCPv6 lease instance.
     virtual void initLease() {
-        lease_.reset(new Lease4(IOAddress("2001:db8:1::1"), hwaddr_, ClientIdPtr(),
+        lease_.reset(new Lease4(IOAddress("192.0.2.1"), hwaddr_, ClientIdPtr(),
                                 100, 30, 60, time(NULL), 1));
     }
 };
@@ -397,12 +457,28 @@ TEST_F(NCRGenerator4Test, fwdRev) {
                 "000001E356D43E5F0A496D65BCA24D982D646140813E3"
                 "B03AB370BFF46BFA309AE7BFD");
     }
+
+    // Now try the same with all lower case.
+    {
+        SCOPED_TRACE("case CHG_REMOVE");
+        testNCR(CHG_REMOVE, true, true, "myhost.example.com.",
+                "000001E356D43E5F0A496D65BCA24D982D646140813E3"
+                "B03AB370BFF46BFA309AE7BFD");
+    }
+
     {
         SCOPED_TRACE("case CHG_ADD");
         testNCR(CHG_ADD, true, true, "MYHOST.example.com.",
                 "000001E356D43E5F0A496D65BCA24D982D646140813E3"
                 "B03AB370BFF46BFA309AE7BFD");
     }
+
+    {
+        SCOPED_TRACE("case CHG_ADD");
+        testNCR(CHG_ADD, true, true, "myhost.example.com.",
+                "000001E356D43E5F0A496D65BCA24D982D646140813E3"
+                "B03AB370BFF46BFA309AE7BFD");
+    }
 }
 
 // Checks that NameChangeRequests are not created when ddns updates are disabled.
@@ -419,7 +495,7 @@ TEST_F(NCRGenerator4Test, d2Disabled) {
 
 // Test creation of the NameChangeRequest for reverse mapping in the
 // given lease.
-TEST_F(NCRGenerator4Test, removeRev) {
+TEST_F(NCRGenerator4Test, revOnly) {
     {
         SCOPED_TRACE("case CHG_REMOVE");
         testNCR(CHG_REMOVE, false, true, "myhost.example.com.",
@@ -434,6 +510,23 @@ TEST_F(NCRGenerator4Test, removeRev) {
     }
 }
 
+// Test creation of the NameChangeRequest for forward mapping in the
+// given lease.
+TEST_F(NCRGenerator4Test, fwdOnly) {
+    {
+        SCOPED_TRACE("case CHG_REMOVE");
+        testNCR(CHG_REMOVE, true, false, "myhost.example.com.",
+                "000001E356D43E5F0A496D65BCA24D982D646140813E3B"
+                "03AB370BFF46BFA309AE7BFD");
+    }
+    {
+        SCOPED_TRACE("case CHG_ADD");
+        testNCR(CHG_ADD, true, false, "myhost.example.com.",
+                "000001E356D43E5F0A496D65BCA24D982D646140813E3B"
+                "03AB370BFF46BFA309AE7BFD");
+    }
+}
+
 // Test that NameChangeRequest is not generated when neither forward
 // nor reverse DNS update has been performed for a lease.
 TEST_F(NCRGenerator4Test, noFwdRevUpdate) {
@@ -466,11 +559,11 @@ TEST_F(NCRGenerator4Test, noHostname) {
 TEST_F(NCRGenerator4Test, wrongHostname) {
     {
         SCOPED_TRACE("case CHG_REMOVE");
-        testNoUpdate(CHG_REMOVE, false, false, "");
+        testNoUpdate(CHG_REMOVE, false, false, "myhost...example.org.");
     }
     {
         SCOPED_TRACE("case CHG_ADD");
-        testNoUpdate(CHG_ADD, false, false, "");
+        testNoUpdate(CHG_ADD, false, false, "myhost...example.org.");
     }
 }
 
@@ -483,7 +576,7 @@ TEST_F(NCRGenerator4Test, useClientId) {
     ASSERT_EQ(1, d2_mgr_.getQueueSize());
 
     verifyNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, true, true,
-                            "2001:db8:1::1",
+                            "192.0.2.1",
                             "000101C7AA5420483BDA99C437636EA7DA2FE18"
                             "31C9679FEB031C360CA571298F3D1FA",
                             lease_->cltt_ + lease_->valid_lft_, 100);
@@ -499,7 +592,20 @@ TEST_F(NCRGenerator4Test, useClientId) {
                 "000101C7AA5420483BDA99C437636EA7DA2FE1831C9679"
                 "FEB031C360CA571298F3D1FA");
     }
+}
 
+// Test that NameChangeRequest is not generated if the lease is NULL,
+// and that the call to queueNCR doesn't cause an exception or
+// assertion.
+TEST_F(NCRGenerator4Test, nullLease) {
+    {
+        SCOPED_TRACE("case CHG_REMOVE");
+        testNullLease(CHG_REMOVE);
+    }
+    {
+        SCOPED_TRACE("case CHG_ADD");
+        testNullLease(CHG_ADD);
+    }
 }
 
 } // end of anonymous namespace