[3070] Addressed review comments.

- Minor changes in User Guide
- Reordering DHCPv6 log messages
- Improved comment for fake allocation
- Cleanup in the config parser test
- Added new test for Rapid Commit
- Default value of rapid_commit_ in subnet.

doc/guide/dhcp6-srv.xml

@@ -1329,7 +1329,7 @@ should include options from the isc option space:
 </screen>
         </para>
         <para>
-          This setting is solely effective for a subnet for which the
+          This setting only affects the subnet for which the
           <command>rapid-commit</command> is set to <command>true</command>.
           For clients connected to other subnets, the server will ignore the
           Rapid Commit option sent by the client and will follow the 4-way

src/bin/dhcp6/dhcp6_messages.mes

@@ -429,18 +429,18 @@ as a hint for possible requested prefix.
 % DHCP6_QUERY_DATA received packet length %1, data length %2, data is %3
 A debug message listing the data received from the client or relay.
 
-% DHCP6_RELEASE_MISSING_CLIENTID client (address=%1) sent RELEASE message without mandatory client-id
-This warning message indicates that client sent RELEASE message without
-mandatory client-id option. This is most likely caused by a buggy client
-(or a relay that malformed forwarded message). This request will not be
-processed and a response with error status code will be sent back.
-
 % DHCP6_RAPID_COMMIT %1: Rapid Commit option received, following 2-way exchange
 This debug messgage is issued when the server found Rapid Commit option
 in the client's message and the 2-way exchanges are supported by the
 server for the subnet, which the client is connected to. The argument
 specifies the client and transaction identification information.
 
+% DHCP6_RELEASE_MISSING_CLIENTID client (address=%1) sent RELEASE message without mandatory client-id
+This warning message indicates that client sent RELEASE message without
+mandatory client-id option. This is most likely caused by a buggy client
+(or a relay that malformed forwarded message). This request will not be
+processed and a response with error status code will be sent back.
+
 % DHCP6_RELEASE_NA address %1 belonging to client duid=%2, iaid=%3 was released properly
 This debug message indicates that an address was released properly. It
 is a normal operation during client shutdown.

src/bin/dhcp6/dhcp6_srv.cc

@@ -1312,12 +1312,17 @@ Dhcpv6Srv::assignIA_NA(const Pkt6Ptr& query, const Pkt6Ptr& answer,
         .arg(duid ? duid->toText() : "(no-duid)").arg(ia->getIAID())
         .arg(hint_opt ? hint.toText() : "(no hint)");
 
-    // "Fake" allocation is processing of SOLICIT message. We pretend to do an
-    // allocation, but we do not put the lease in the database. That is ok,
-    // because we do not guarantee that the user will get that exact lease. If
-    // the user selects this server to do actual allocation (i.e. sends REQUEST)
-    // it should include this hint. That will help us during the actual lease
-    // allocation.
+    // "Fake" allocation is the case when the server is processing the Solicit
+    // message without the Rapid Commit option and advertises a lease to
+    // the client, but doesn't commit this lease to the lease database. If
+    // the Solicit contains the Rapid Commit option and the server is
+    // configured to honor the Rapid Commit option, or the client has sent
+    // the Request message, the lease will be committed to the lease
+    // database. The type of the server's response may be used to determine
+    // if this is the fake allocation case or not. When the server sends
+    // Reply message it means that it is committing leases. Other message
+    // type (Advertise) means that server is not committing leases (fake
+    // allocation).
     bool fake_allocation = (answer->getType() != DHCPV6_REPLY);
 
     // Get DDNS update direction flags
@@ -1467,12 +1472,17 @@ Dhcpv6Srv::assignIA_PD(const Pkt6Ptr& query, const Pkt6Ptr& answer,
         .arg(duid ? duid->toText() : "(no-duid)").arg(ia->getIAID())
         .arg(hint_opt ? hint.toText() : "(no hint)");
 
-    // "Fake" allocation is processing of SOLICIT message. We pretend to do an
-    // allocation, but we do not put the lease in the database. That is ok,
-    // because we do not guarantee that the user will get that exact lease. If
-    // the user selects this server to do actual allocation (i.e. sends REQUEST)
-    // it should include this hint. That will help us during the actual lease
-    // allocation.
+    // "Fake" allocation is the case when the server is processing the Solicit
+    // message without the Rapid Commit option and advertises a lease to
+    // the client, but doesn't commit this lease to the lease database. If
+    // the Solicit contains the Rapid Commit option and the server is
+    // configured to honor the Rapid Commit option, or the client has sent
+    // the Request message, the lease will be committed to the lease
+    // database. The type of the server's response may be used to determine
+    // if this is the fake allocation case or not. When the server sends
+    // Reply message it means that it is committing leases. Other message
+    // type (Advertise) means that server is not committing leases (fake
+    // allocation).
     bool fake_allocation = (answer->getType() != DHCPV6_REPLY);
 
     // Use allocation engine to pick a lease for this client. Allocation engine

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -525,6 +525,9 @@ public:
 
         // Check the Rapid Commit flag for the subnet.
         EXPECT_EQ(exp_rapid_commit, subnet->getRapidCommit());
+
+        // Clear any existing configuration.
+        CfgMgr::instance().clear();
     }
 
     int rcode_;          ///< Return code (see @ref isc::config::parseAnswer)
@@ -1138,8 +1141,8 @@ TEST_F(Dhcp6ParserTest, subnetRapidCommit) {
     }
 
     {
-        SCOPED_TRACE("Enable Rapid Commit");
         // rapid-commit explicitly set to true.
+        SCOPED_TRACE("Enable Rapid Commit");
         testRapidCommit("{ \"preferred-lifetime\": 3000,"
                         "\"rebind-timer\": 2000, "
                         "\"renew-timer\": 1000, "
@@ -1153,8 +1156,8 @@ TEST_F(Dhcp6ParserTest, subnetRapidCommit) {
     }
 
     {
-        SCOPED_TRACE("Disable Rapid Commit");
         // rapid-commit explicitly set to false.
+        SCOPED_TRACE("Disable Rapid Commit");
         testRapidCommit("{ \"preferred-lifetime\": 3000,"
                         "\"rebind-timer\": 2000, "
                         "\"renew-timer\": 1000, "

src/bin/dhcp6/tests/dhcp6_client.cc

@@ -38,13 +38,14 @@ Dhcp6Client::Dhcp6Client() :
     dest_addr_(ALL_DHCP_RELAY_AGENTS_AND_SERVERS),
     duid_(generateDUID(DUID::DUID_LLT)),
     link_local_("fe80::3a60:77ff:fed5:cdef"),
+    iface_name_("eth0"),
     srv_(boost::shared_ptr<NakedDhcpv6Srv>(new NakedDhcpv6Srv(0))),
     use_na_(false),
     use_pd_(false),
     use_relay_(false),
     use_oro_(false),
     use_client_id_(true),
-    use_rapid_commit_(true),
+    use_rapid_commit_(false),
     prefix_hint_(),
     fqdn_() {
 }
@@ -55,13 +56,14 @@ Dhcp6Client::Dhcp6Client(boost::shared_ptr<NakedDhcpv6Srv>& srv) :
     dest_addr_(ALL_DHCP_RELAY_AGENTS_AND_SERVERS),
     duid_(generateDUID(DUID::DUID_LLT)),
     link_local_("fe80::3a60:77ff:fed5:cdef"),
+    iface_name_("eth0"),
     srv_(srv),
     use_na_(false),
     use_pd_(false),
     use_relay_(false),
     use_oro_(false),
     use_client_id_(true),
-    use_rapid_commit_(true),
+    use_rapid_commit_(false),
     prefix_hint_(),
     fqdn_() {
 }
@@ -532,7 +534,7 @@ Dhcp6Client::sendMsg(const Pkt6Ptr& msg) {
                               msg->getBuffer().getLength()));
     msg_copy->setRemoteAddr(link_local_);
     msg_copy->setLocalAddr(dest_addr_);
-    msg_copy->setIface("eth0");
+    msg_copy->setIface(iface_name_);
     srv_->fakeReceive(msg_copy);
     srv_->run();
 }

src/bin/dhcp6/tests/dhcp6_client.h

@@ -384,6 +384,13 @@ public:
         dest_addr_ = dest_addr;
     }
 
+    /// @brief Sets the interface to be used by the client.
+    ///
+    /// @param iface_name Interface name.
+    void setInterface(const std::string& iface_name) {
+        iface_name_ = iface_name;
+    }
+
     /// @brief Sets a prefix hint to be sent to a server.
     ///
     /// @param pref_lft Preferred lifetime.
@@ -575,7 +582,7 @@ private:
     /// @biref Current transaction id (altered on each send).
     uint32_t curr_transid_;
 
-    /// @brief Currently use destination address.
+    /// @brief Currently used destination address.
     asiolink::IOAddress dest_addr_;
 
     /// @brief Currently used DUID.
@@ -584,6 +591,9 @@ private:
     /// @brief Currently used link local address.
     asiolink::IOAddress link_local_;
 
+    /// @brief Currently used interface.
+    std::string iface_name_;
+
     /// @brief Pointer to the server that the client is communicating with.
     boost::shared_ptr<isc::test::NakedDhcpv6Srv> srv_;
 

src/bin/dhcp6/tests/sarr_unittest.cc

@@ -37,17 +37,12 @@ namespace {
 ///     match the subnet prefix
 ///
 /// - Configuration 1:
-///   - one subnet 2001:db8:1::/48 used on eth0 interface
-///   - one pool in a range of 2001:db8:1::1 - 2001:db8:1::10
-///   - enables Rapid Commit for the subnet and can be used for testing
-///     Rapid Commit option support
-///   - DNS updates enabled
-///
-/// - Configuration 2:
-///   - one subnet 2001:db8:1::/48 used on eth0 interface
-///   - one pool in a range of 2001:db8:1::1 - 2001:db8:1::10
-///   - disables Rapid Commit for the subnet and can be used for testing
-///     that server ignores Rapid Commit option from the client.
+///   - two subnets 2001:db8:1::/48 and 2001:db8:2::/48
+///   - first subnet assigned to interface eth0, another one assigned to eth1
+///   - one pool for subnet in a range of 2001:db8:X::1 - 2001:db8:X::10,
+///     where X is 1 or 2
+///   - enables Rapid Commit for the first subnet and disables for the second
+///     one
 ///   - DNS updates enabled
 ///
 const char* CONFIGS[] = {
@@ -82,6 +77,12 @@ const char* CONFIGS[] = {
         "    \"subnet\": \"2001:db8:1::/48\", "
         "    \"interface\": \"eth0\","
         "    \"rapid-commit\": True"
+        " },"
+        " {"
+        "    \"pools\": [ { \"pool\": \"2001:db8:2::1 - 2001:db8:2::10\" } ],"
+        "    \"subnet\": \"2001:db8:2::/48\", "
+        "    \"interface\": \"eth1\","
+        "    \"rapid-commit\": False"
         " } ],"
         "\"valid-lifetime\": 4000,"
         " \"dhcp-ddns\" : {"
@@ -101,6 +102,12 @@ const char* CONFIGS[] = {
         "    \"subnet\": \"2001:db8:1::/48\", "
         "    \"interface\": \"eth0\","
         "    \"rapid-commit\": False"
+        " },"
+        " {"
+        "    \"pools\": [ { \"pool\": \"2001:db8:2::1 - 2001:db8:2::10\" } ],"
+        "    \"subnet\": \"2001:db8:2::/48\", "
+        "    \"interface\": \"eth1\","
+        "    \"rapid-commit\": True"
         " } ],"
         "\"valid-lifetime\": 4000,"
         " \"dhcp-ddns\" : {"
@@ -202,7 +209,7 @@ TEST_F(SARRTest, rapidCommitEnable) {
     // Make sure we ended-up having expected number of subnets configured.
     const Subnet6Collection* subnets = CfgMgr::instance().getCurrentCfg()->
         getCfgSubnets6()->getAll();
-    ASSERT_EQ(1, subnets->size());
+    ASSERT_EQ(2, subnets->size());
     // Perform 2-way exchange.
     client.useRapidCommit(true);
     // Include FQDN to trigger generation of name change requests.
@@ -229,19 +236,51 @@ TEST_F(SARRTest, rapidCommitEnable) {
     EXPECT_EQ(1, CfgMgr::instance().getD2ClientMgr().getQueueSize());
 }
 
+// Check that the server responds with Advertise if the client hasn't
+// included the Rapid Commit option in the Solicit.
+TEST_F(SARRTest, rapidCommitNoOption) {
+    Dhcp6Client client;
+    // Configure client to request IA_NA
+    client.useNA();
+    configure(CONFIGS[1], *client.getServer());
+    ASSERT_NO_THROW(client.getServer()->startD2());
+    // Make sure we ended-up having expected number of subnets configured.
+    const Subnet6Collection* subnets = CfgMgr::instance().getCurrentCfg()->
+        getCfgSubnets6()->getAll();
+    ASSERT_EQ(2, subnets->size());
+    // Include FQDN to test that the server will not create name change
+    // requests when it sends Advertise (Rapid Commit disabled).
+    ASSERT_NO_THROW(client.useFQDN(Option6ClientFqdn::FLAG_S,
+                                   "client-name.example.org",
+                                   Option6ClientFqdn::FULL));
+    ASSERT_NO_THROW(client.doSolicit());
+    // There should be no lease because the server should have responded
+    // with Advertise.
+    ASSERT_EQ(0, client.getLeaseNum());
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    EXPECT_EQ(DHCPV6_ADVERTISE, client.getContext().response_->getType());
+    // Make sure that the Rapid Commit option is not included.
+    EXPECT_FALSE(client.getContext().response_->getOption(D6O_RAPID_COMMIT));
+    // There should be no name change request generated.
+    EXPECT_EQ(0, CfgMgr::instance().getD2ClientMgr().getQueueSize());
+}
+
 // Check that when the Rapid Commit support is disabled for the subnet
 // the server replies with an Advertise and ignores the Rapid Commit
 // option sent by the client.
 TEST_F(SARRTest, rapidCommitDisable) {
     Dhcp6Client client;
+    // The subnet assigned to eth1 has Rapid Commit disabled.
+    client.setInterface("eth1");
     // Configure client to request IA_NA
     client.useNA();
-    configure(CONFIGS[2], *client.getServer());
+    configure(CONFIGS[1], *client.getServer());
     ASSERT_NO_THROW(client.getServer()->startD2());
     // Make sure we ended-up having expected number of subnets configured.
     const Subnet6Collection* subnets = CfgMgr::instance().getCurrentCfg()->
         getCfgSubnets6()->getAll();
-    ASSERT_EQ(1, subnets->size());
+    ASSERT_EQ(2, subnets->size());
     // Send Rapid Commit option to the server.
     client.useRapidCommit(true);
     // Include FQDN to test that the server will not create name change

src/lib/dhcpsrv/subnet.h

@@ -679,6 +679,9 @@ private:
 
     /// @brief A flag indicating if Rapid Commit option is supported
     /// for this subnet.
+    ///
+    /// It's default value is false, which indicates that the Rapid
+    /// Commit is disabled for the subnet.
     bool rapid_commit_;
 };