[2325] Changes after review.

src/bin/dhcp6/dhcp6_messages.mes

@@ -22,6 +22,11 @@ successfully established a session with the BIND 10 control channel.
 This debug message is issued just before the IPv6 DHCP server attempts
 to establish a session with the BIND 10 control channel.
 
+% DHCP6_CLIENTID_MISSING mandatory client-id option is missing, message from %1 dropped
+This error message indicates that received message does not include
+mandatory client-id option that is necessary for address assignment,
+so the message is being dropped. This likely indicates a buggy client.
+
 % DHCP6_COMMAND_RECEIVED received command %1, arguments: %2
 A debug message listing the command (and possible arguments) received
 from the BIND 10 control system by the IPv6 DHCP server.
@@ -82,10 +87,11 @@ received REQUEST) a lease for a given client. There may be many reasons for
 such failure. Each specific failure is logged in a separate log entry.
 
 % DHCP6_REQUIRED_OPTIONS_CHECK_FAIL %1 message received from %2 failed the following check: %3
-This message indicates that received message has invalid number of options:
+This message indicates that received DHCPv6 packet is invalid.  This may be due
-mandatory client-id option is missing, server-id forbidden in that particular
+to a number of reasons, e.g. the mandatory client-id option is missing,
-type of message is present, there is more than one instance of client-id
+the server-id forbidden in that particular type of message is present,
-or server-id etc. Exact reason for rejecting the packed is printed.
+there is more than one instance of client-id or server-id present,
+etc. The exact reason for rejecting the packet is included in the message.
 
 % DHCP6_NOT_RUNNING IPv6 DHCP server is not running
 A warning message is issued when an attempt is made to shut down the

src/bin/dhcp6/dhcp6_srv.cc

@@ -433,7 +433,7 @@ void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer) {
 
         // perhaps this should be logged on some higher level? This is most likely
         // configuration bug.
-        LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_SUBNET_SELECTION_FAILED);
+        LOG_ERROR(dhcp6_logger, DHCP6_SUBNET_SELECTION_FAILED);
     } else {
         LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA, DHCP6_SUBNET_SELECTED)
             .arg(subnet->toText());
@@ -451,6 +451,7 @@ void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer) {
     if (opt_duid) {
         duid = DuidPtr(new DUID(opt_duid->getData()));
     } else {
+        LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_CLIENTID_MISSING);
         // Let's drop the message. This client is not sane.
         isc_throw(RFCViolation, "Mandatory client-id is missing in received message");
     }
@@ -624,7 +625,7 @@ void Dhcpv6Srv::renewLeases(const Pkt6Ptr& renew, Pkt6Ptr& reply) {
 
         // perhaps this should be logged on some higher level? This is most likely
         // configuration bug.
-        LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_SUBNET_SELECTION_FAILED);
+        LOG_ERROR(dhcp6_logger, DHCP6_SUBNET_SELECTION_FAILED);
     } else {
         LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA, DHCP6_SUBNET_SELECTED)
             .arg(subnet->toText());

src/bin/dhcp6/dhcp6_srv.h

@@ -213,8 +213,8 @@ protected:
     /// @brief Renews specific IA_NA option
     ///
     /// Generates response to IA_NA. This typically includes finding a lease that
-    /// corresponds to the received address. If no such lease is found, IA_NA
+    /// corresponds to the received address. If no such lease is found, an IA_NA
-    /// response is generates with appropriate status code.
+    /// response is generated with an appropriate status code.
     ///
     /// @param subnet subnet the sender belongs to
     /// @param duid client's duid
@@ -255,6 +255,7 @@ protected:
     ///
     /// It supports addresses (IA_NA) only. It does NOT support temporary
     /// addresses (IA_TA) nor prefixes (IA_PD).
+    /// @todo: Extend this method once TA and PD becomes supported
     ///
     /// @param question client's message (with requested IA_NA)
     /// @param answer server's message (IA_NA options will be added here)

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -796,12 +796,15 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) {
     boost::scoped_ptr<NakedDhcpv6Srv> srv;
     ASSERT_NO_THROW( srv.reset(new NakedDhcpv6Srv(0)) );
 
-    IOAddress addr("2001:db8:1:1::cafe:babe");
+    const IOAddress addr("2001:db8:1:1::cafe:babe");
     const uint32_t iaid = 234;
 
-    // generate client-id also duid_
+    // Generate client-id also duid_
     OptionPtr clientid = generateClientId();
 
+    // Check that the address we are about to use is indeed in pool
+    ASSERT_TRUE(subnet_->inPool(addr));
+
     // Note that preferred, valid, T1 and T2 timers and CLTT are set to invalid
     // value on purpose. They should be updated during RENEW.
     Lease6Ptr lease(new Lease6(Lease6::LEASE_IA_NA, addr, duid_, iaid,
@@ -809,11 +812,12 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) {
     lease->cltt_ = 1234;
     ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease));
 
-    // check that the lease is really in the database
+    // Check that the lease is really in the database
     Lease6Ptr l = LeaseMgrFactory::instance().getLease6(addr);
     ASSERT_TRUE(l);
 
-    // Check that T1, T2, preferred, valid and cltt really
+    // Check that T1, T2, preferred, valid and cltt really set and not using
+    // previous (500, 501, etc.) values
     EXPECT_NE(l->t1_, subnet_->getT1());
     EXPECT_NE(l->t2_, subnet_->getT2());
     EXPECT_NE(l->preferred_lft_, subnet_->getPreferred());
@@ -825,38 +829,37 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) {
     req->setRemoteAddr(IOAddress("fe80::abcd"));
     boost::shared_ptr<Option6IA> ia = generateIA(iaid, 1500, 3000);
 
-    ASSERT_TRUE(subnet_->inPool(addr));
     OptionPtr renewed_addr_opt(new Option6IAAddr(D6O_IAADDR, addr, 300, 500));
     ia->addOption(renewed_addr_opt);
     req->addOption(ia);
     req->addOption(clientid);
 
-    // server-id is mandatory in RENEW
+    // Server-id is mandatory in RENEW
     req->addOption(srv->getServerID());
 
     // Pass it to the server and hope for a REPLY
     Pkt6Ptr reply = srv->processRenew(req);
 
-    // check if we get response at all
+    // Check if we get response at all
     checkResponse(reply, DHCPV6_REPLY, 1234);
 
     OptionPtr tmp = reply->getOption(D6O_IA_NA);
     ASSERT_TRUE(tmp);
 
-    // check that IA_NA was returned and that there's an address included
+    // Check that IA_NA was returned and that there's an address included
     boost::shared_ptr<Option6IAAddr> addr_opt = checkIA_NA(reply, 234, subnet_->getT1(),
                                                            subnet_->getT2());
 
-    // check that we've got the address we requested
+    // Check that we've got the address we requested
     checkIAAddr(addr_opt, addr, subnet_->getPreferred(), subnet_->getValid());
 
-    // check DUIDs
+    // Check DUIDs
     checkServerId(reply, srv->getServerID());
     checkClientId(reply, clientid);
 
-    // check that the lease is really in the database
+    // Check that the lease is really in the database
     l = checkLease(duid_, reply->getOption(D6O_IA_NA), addr_opt);
-    EXPECT_TRUE(l);
+    ASSERT_TRUE(l);
 
     // Check that T1, T2, preferred, valid and cltt were really updated
     EXPECT_EQ(l->t1_, subnet_->getT1());
@@ -864,10 +867,10 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) {
     EXPECT_EQ(l->preferred_lft_, subnet_->getPreferred());
     EXPECT_EQ(l->valid_lft_, subnet_->getValid());
 
-    // checking for CLTT is a bit tricky if we want to avoid off by 1 errors
+    // Checking for CLTT is a bit tricky if we want to avoid off by 1 errors
     int32_t cltt = static_cast<int32_t>(l->cltt_);
     int32_t expected = static_cast<int32_t>(time(NULL));
-    // 1 >= difference between cltt and expected
+    // equality or difference by 1 between cltt and expected is ok.
     EXPECT_GE(1, abs(cltt - expected));
 
     EXPECT_TRUE(LeaseMgrFactory::instance().deleteLease6(addr_opt->getAddress()));
@@ -886,18 +889,22 @@ TEST_F(Dhcpv6SrvTest, RenewBasic) {
 // - returned REPLY message has IA that includes STATUS-CODE
 // - No lease in LeaseMgr
 TEST_F(Dhcpv6SrvTest, RenewReject) {
+
     boost::scoped_ptr<NakedDhcpv6Srv> srv;
     ASSERT_NO_THROW( srv.reset(new NakedDhcpv6Srv(0)) );
 
-    IOAddress addr("2001:db8:1:1::dead");
+    const IOAddress addr("2001:db8:1:1::dead");
     const uint32_t transid = 1234;
     const uint32_t valid_iaid = 234;
     const uint32_t bogus_iaid = 456;
 
-    // generate client-id also duid_
+    // Quick sanity check that the address we're about to use is ok
+    ASSERT_TRUE(subnet_->inPool(addr));
+
+    // GenerateClientId() also sets duid_
     OptionPtr clientid = generateClientId();
 
-    // check that the lease is really in the database
+    // Check that the lease is NOT in the database
     Lease6Ptr l = LeaseMgrFactory::instance().getLease6(addr);
     ASSERT_FALSE(l);
 
@@ -906,13 +913,12 @@ TEST_F(Dhcpv6SrvTest, RenewReject) {
     req->setRemoteAddr(IOAddress("fe80::abcd"));
     boost::shared_ptr<Option6IA> ia = generateIA(bogus_iaid, 1500, 3000);
 
-    ASSERT_TRUE(subnet_->inPool(addr));
     OptionPtr renewed_addr_opt(new Option6IAAddr(D6O_IAADDR, addr, 300, 500));
     ia->addOption(renewed_addr_opt);
     req->addOption(ia);
     req->addOption(clientid);
 
-    // server-id is mandatory in RENEW
+    // Server-id is mandatory in RENEW
     req->addOption(srv->getServerID());
 
     // Case 1: No lease known to server
@@ -972,7 +978,6 @@ TEST_F(Dhcpv6SrvTest, RenewReject) {
     ASSERT_TRUE(ia);
     checkRejectedIA_NA(ia, STATUS_NoAddrsAvail);
 
-
     lease = LeaseMgrFactory::instance().getLease6(addr);
     ASSERT_TRUE(lease);
     // Verify that the lease was not updated.