[5364] The same value of rapid-commit is now enforced on all subnets

src/bin/dhcp6/json_config_parser.cc

@@ -251,8 +251,33 @@ public:
 
             const Subnet6Collection* subnets = (*net)->getAllSubnets();
             if (subnets) {
+
+                bool rapid_commit;
+
                 // For each subnet, add it to a list of regular subnets.
                 for (auto subnet = subnets->begin(); subnet != subnets->end(); ++subnet) {
+
+                    // Rapid commit must either be enabled or disabled in all subnets
+                    // in the shared network.
+                    if (subnet == subnets->begin()) {
+                        // If this is the first subnet, remember the value.
+                        rapid_commit = (*subnet)->getRapidCommit();
+                    } else {
+                        // Ok, this is the second or following subnets. The value
+                        // must match what was set in the first subnet.
+                        if (rapid_commit != (*subnet)->getRapidCommit()) {
+                            isc_throw(DhcpConfigError, "All subnets in a shared network "
+                                      "must have the same rapid-commit value. Subnet "
+                                      << (*subnet)->toText()
+                                      << " has specified rapid-commit "
+                                      << ( (*subnet)->getRapidCommit() ? "true" : "false")
+                                      << ", but earlier subnet in the same shared-network"
+                                      << " or the shared-network itself used rapid-commit "
+                                      << (rapid_commit ? "true" : "false"));
+                        }
+                    }
+
+
                     if (iface.empty()) {
                         iface = (*subnet)->getIface();
                         continue;
@@ -290,8 +315,8 @@ public:
 
         }
     }
-    
-    
+
+
 };
 
 } // anonymous namespace
@@ -568,7 +593,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set,
         // it checks that there is no conflict between plain subnets and those
         // defined as part of shared networks.
         global_parser.sanityChecks(srv_config, mutable_cfg);
-        
+
     } catch (const isc::Exception& ex) {
         LOG_ERROR(dhcp6_logger, DHCP6_PARSER_FAIL)
                   .arg(config_pair.first).arg(ex.what());
@@ -603,7 +628,7 @@ configureDhcp6Server(Dhcpv6Srv&, isc::data::ConstElementPtr config_set,
 
             // Setup the command channel.
             configureCommandChannel();
-            
+
             // No need to commit interface names as this is handled by the
             // CfgMgr::commit() function.
 

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -5617,7 +5617,7 @@ TEST_F(Dhcp6ParserTest, sharedNetworksDerive) {
         "        },\n"
         "        \"valid-lifetime\": 400, \n"
         "        \"interface-id\": \"twotwo\",\n"
-        "        \"rapid-commit\": false,\n"
+        "        \"rapid-commit\": true,\n"
         "        \"reservation-mode\": \"out-of-pool\"\n"
         "    }\n"
         "    ]\n"
@@ -5673,7 +5673,7 @@ TEST_F(Dhcp6ParserTest, sharedNetworksDerive) {
     ASSERT_TRUE(s->getInterfaceId());
     EXPECT_TRUE(iface_id2.equals(s->getInterfaceId()));
     EXPECT_EQ(IOAddress("2222::2"), s->getRelayInfo().addr_);
-    EXPECT_EQ(false, s->getRapidCommit());
+    EXPECT_EQ(true, s->getRapidCommit());
     EXPECT_EQ(Network::HR_OUT_OF_POOL, s->getHostReservationMode());
 
     // Ok, now check the second shared subnet.
@@ -5778,6 +5778,38 @@ TEST_F(Dhcp6ParserTest, sharedNetworksDeriveInterfaces) {
     EXPECT_EQ("", s->getIface());
 }
 
+
+// It is not allowed to have different values for interfaces names is subnets
+// in the same shared network.
+TEST_F(Dhcp6ParserTest, sharedNetworksInterfacesMixed) {
+
+    // We need to fake the interfaces present, because we want to test
+    // interface names inheritance. However, there are sanity checks
+    // on subnet level that would refuse the value if the interface
+    // is not present.
+    IfaceMgrTestConfig iface_config(true);
+
+    string config = "{\n"
+        "\"shared-networks\": [ {\n"
+        "    \"name\": \"foo\"\n,"
+        "    \"subnet6\": [\n"
+        "    { \n"
+        "        \"subnet\": \"2001:db1::/48\",\n"
+        "        \"interface\": \"eth0\"\n"
+        "    },\n"
+        "    { \n"
+        "        \"subnet\": \"2001:db2::/48\",\n"
+        "        \"interface\": \"eth1\"\n"
+        "    }\n"
+        "    ]\n"
+        " } ]\n"
+        "} \n";
+
+    configure(config, CONTROL_RESULT_ERROR, "Subnet 2001:db2::/48 has specified "
+              "interface eth1, but earlier subnet in the same shared-network "
+              "or the shared-network itself used eth0");
+}
+
 // This test checks if client-class is derived properly.
 TEST_F(Dhcp6ParserTest, sharedNetworksDeriveClientClass) {
 
@@ -5862,4 +5894,100 @@ TEST_F(Dhcp6ParserTest, sharedNetworksDeriveClientClass) {
     EXPECT_TRUE(classes.empty());
 }
 
+// Tests if rapid-commit is derived properly.
+TEST_F(Dhcp6ParserTest, sharedNetworksRapidCommit) {
+
+    string config = "{\n"
+        "\"shared-networks\": [ {\n"
+        "    \"name\": \"frog\"\n,"
+        "    \"rapid-commit\": true,\n"
+        "    \"subnet6\": [\n"
+        "    { \n"
+        "        \"subnet\": \"2001:db1::/48\",\n"
+        "        \"pools\": [ { \"pool\": \"2001:db1::/64\" } ]\n"
+        "    },\n"
+        "    { \n"
+        "        \"subnet\": \"2001:db2::/48\",\n"
+        "        \"pools\": [ { \"pool\": \"2001:db2::/64\" } ],\n"
+        "        \"client-class\": \"beta\"\n"
+        "    }\n"
+        "    ]\n"
+        " },\n"
+        "{ // second shared-network starts here\n"
+        "    \"name\": \"bar\",\n"
+        "    \"rapid-commit\": false,\n"
+        "    \"subnet6\": [\n"
+        "    {\n"
+        "        \"subnet\": \"2001:db3::/48\",\n"
+        "        \"pools\": [ { \"pool\": \"2001:db3::/64\" } ]\n"
+        "    }\n"
+        "    ]\n"
+        "} ]\n"
+        "} \n";
+
+    configure(config, CONTROL_RESULT_SUCCESS, "");
+
+    // Now verify that the shared network was indeed configured.
+    CfgSharedNetworks6Ptr cfg_net = CfgMgr::instance().getStagingCfg()
+        ->getCfgSharedNetworks6();
+
+    // Two shared networks are expeced.
+    ASSERT_TRUE(cfg_net);
+    const SharedNetwork6Collection* nets = cfg_net->getAll();
+    ASSERT_TRUE(nets);
+    ASSERT_EQ(2, nets->size());
+
+    // Let's check the first one.
+    SharedNetwork6Ptr net = nets->at(0);
+    ASSERT_TRUE(net);
+
+    const Subnet6Collection * subs = net->getAllSubnets();
+    ASSERT_TRUE(subs);
+    ASSERT_EQ(2, subs->size());
+    EXPECT_TRUE(subs->at(0)->getRapidCommit());
+    EXPECT_TRUE(subs->at(1)->getRapidCommit());
+
+    // Ok, now check the second shared network. It doesn't have
+    // anything defined on shared-network or subnet level, so
+    // everything should have default values.
+    net = nets->at(1);
+    ASSERT_TRUE(net);
+
+    subs = net->getAllSubnets();
+    ASSERT_TRUE(subs);
+    EXPECT_EQ(1, subs->size());
+
+    // This subnet should derive its renew-timer from global scope.
+    EXPECT_FALSE(subs->at(0)->getRapidCommit());
+}
+
+// Tests if rapid-commit is derived properly.
+TEST_F(Dhcp6ParserTest, sharedNetworksRapidCommitMix) {
+
+    string config = "{\n"
+        "\"shared-networks\": [ {\n"
+        "    \"name\": \"frog\"\n,"
+        "    \"subnet6\": [\n"
+        "    { \n"
+        "        \"subnet\": \"2001:db1::/48\",\n"
+        "        \"rapid-commit\": true,\n"
+        "        \"pools\": [ { \"pool\": \"2001:db1::/64\" } ]\n"
+        "    },\n"
+        "    { \n"
+        "        \"subnet\": \"2001:db2::/48\",\n"
+        "        \"rapid-commit\": false,\n"
+        "        \"pools\": [ { \"pool\": \"2001:db2::/64\" } ],\n"
+        "        \"client-class\": \"beta\"\n"
+        "    }\n"
+        "    ]\n"
+        " } ]\n"
+        "} \n";
+
+    configure(config, CONTROL_RESULT_ERROR, "All subnets in a shared network "
+              "must have the same rapid-commit value. Subnet 2001:db2::/48 has "
+              "specified rapid-commit false, but earlier subnet in the same "
+              "shared-network or the shared-network itself used rapid-commit true");
+}
+
+
 };

src/bin/dhcp6/tests/dhcp6_client.cc

@@ -964,6 +964,22 @@ Dhcp6Client::clearExtraOptions() {
     extra_options_.clear();
 }
 
+void
+Dhcp6Client::printConfiguration() const {
+
+    // Print DUID
+    std::cout << "Client " << (duid_ ? duid_->toText() : "(without duid)")
+              << " got " << getLeaseNum() << " lease(s): ";
+
+    // Print leases
+    for (int i = 0; i < getLeaseNum(); i++) {
+        Lease6 lease = getLease(i);
+        std::cout << lease.addr_.toText() << " ";
+    }
+
+    /// @todo: Print many other parameters.
+    std::cout << std::endl;
+}
 
 } // end of namespace isc::dhcp::test
 } // end of namespace isc::dhcp

src/bin/dhcp6/tests/dhcp6_client.h

@@ -749,6 +749,12 @@ public:
     /// @brief Configures the client to not send any extra options.
     void clearExtraOptions();
 
+    /// @brief Debugging method the prints currently held configuration
+    ///
+    /// @todo: This is mostly useful when debugging tests. This method
+    /// is incomplete. Please extend it when needed.
+    void printConfiguration() const;
+
 private:
 
     /// @brief Applies the new leases for the client.

src/bin/dhcp6/tests/shared_network_unittest.cc

@@ -832,9 +832,7 @@ const char* NETWORKS_CONFIG[] = {
     "}",
 
 // Configuration #16
-// - one shared network with two subnets
-//   - first subnet has the rapid commit enabled
-//   - second subnet has the rapid commit disabled
+// - one shared network with two subnets, both have rapid-commit enabled
     "{"
     "    \"shared-networks\": ["
     "        {"
@@ -854,7 +852,41 @@ const char* NETWORKS_CONFIG[] = {
     "                {"
     "                    \"subnet\": \"2001:db8:2::/64\","
     "                    \"id\": 100,"
-    "                    \"rapid-commit\": false,"
+    "                    \"rapid-commit\": true,"
+    "                    \"pools\": ["
+    "                        {"
+    "                            \"pool\": \"2001:db8:2::20 - 2001:db8:2::20\""
+    "                        }"
+    "                    ]"
+    "                }"
+    "            ]"
+    "        }"
+    "    ]"
+    "}",
+
+
+// Configuration #17:
+// - one shared network with rapid-commit enabled
+// - two subnets (which should derive the rapid-commit setting)
+    "{"
+    "    \"shared-networks\": ["
+    "        {"
+    "            \"name\": \"frog\","
+    "            \"interface\": \"eth1\","
+    "            \"rapid-commit\": true,"
+    "            \"subnet6\": ["
+    "                {"
+    "                    \"subnet\": \"2001:db8:1::/64\","
+    "                    \"id\": 10,"
+    "                    \"pools\": ["
+    "                        {"
+    "                            \"pool\": \"2001:db8:1::20 - 2001:db8:1::20\""
+    "                        }"
+    "                    ]"
+    "                },"
+    "                {"
+    "                    \"subnet\": \"2001:db8:2::/64\","
+    "                    \"id\": 100,"
     "                    \"pools\": ["
     "                        {"
     "                            \"pool\": \"2001:db8:2::20 - 2001:db8:2::20\""
@@ -1120,6 +1152,98 @@ public:
 
     }
 
+    /// @brief Tests that for a given configuration the rapid-commit works (or not)
+    ///
+    /// The provided configuration is expected to be able to handle two clients.
+    /// The second parameter governs whether rapid-commit is expected to be enabled
+    /// or disabled. Third and fourth parameters are text representations of expected
+    /// leases to be assigned (if rapid-commit is enabled)
+    ///
+    /// @param config - text version of the configuration to be tested
+    /// @param enabled - true = rapid-commit is expected to work
+    /// @param exp_addr1 - an eddress the first client is expected to get (if
+    ///                    rapid-commit is enabled).
+    /// @param exp_addr2 - an eddress the second client is expected to get (if
+    ///                    rapid-commit is enabled).
+    void testRapidCommit(const std::string& config, bool enabled,
+                         const std::string& exp_addr1,
+                         const std::string& exp_addr2) {
+
+        // Create client #1. This clients wants to use rapid-commit.
+        Dhcp6Client client1;
+        client1.setInterface("eth1");
+        client1.useRapidCommit(true);
+
+        Dhcp6Client client2;
+        client2.setInterface("eth1");
+        client2.useRapidCommit(true);
+
+        // Configure the server with a shared network.
+        ASSERT_NO_FATAL_FAILURE(configure(config, *client1.getServer()));
+
+        // Ok, client should have one
+        EXPECT_EQ(0, client1.getLeaseNum());
+
+        // Client #1 should be assigned an address from shared network. The first
+        // subnet has rapid-commit enabled, so the address should be assigned.
+        ASSERT_NO_THROW(client1.requestAddress(0xabca0));
+        testAssigned([this, &client1] {
+                ASSERT_NO_THROW(client1.doSolicit());
+            });
+
+        // Make sure something was sent back.
+        ASSERT_TRUE(client1.getContext().response_);
+
+        if (enabled) {
+            // rapid-commit enabled.
+
+            // Make sure that REPLY was sent back.
+            EXPECT_EQ(DHCPV6_REPLY, client1.getContext().response_->getType());
+
+            // Just make sure the client didn't get an address.
+            EXPECT_TRUE(hasLeaseForAddress(client1, IOAddress(exp_addr1),
+                                           LeaseOnServer::MUST_EXIST));
+        } else {
+            // rapid-commit disabled.
+
+            // Make sure that ADVERTISE was sent back.
+            EXPECT_EQ(DHCPV6_ADVERTISE, client1.getContext().response_->getType());
+
+            // And that it doesn't have any leases.
+            EXPECT_EQ(0, client1.getLeaseNum());
+        }
+
+        // Create client #2. This client behaves the same as the first one, but the
+        // first subnet is already full (it's a really small subnet) and the second
+        // subnet does not allow rapid-commit.
+        ASSERT_NO_THROW(client2.requestAddress(0xabca0));
+        testAssigned([this, &client2] {
+                ASSERT_NO_THROW(client2.doSolicit());
+            });
+
+        // Make sure something was sent back.
+        ASSERT_TRUE(client2.getContext().response_);
+
+        if (enabled) {
+            // rapid-commit enabled.
+
+            // Make sure that REPLY was sent back.
+            EXPECT_EQ(DHCPV6_REPLY, client2.getContext().response_->getType());
+
+            // Just make sure the client didn't get an address.
+            EXPECT_TRUE(hasLeaseForAddress(client2, IOAddress(exp_addr2),
+                                           LeaseOnServer::MUST_EXIST));
+        } else {
+            // rapid-commit disabled.
+
+            // Make sure that ADVERTISE was sent back.
+            EXPECT_EQ(DHCPV6_ADVERTISE, client1.getContext().response_->getType());
+
+            // And that it doesn't have any leases.
+            EXPECT_EQ(0, client1.getLeaseNum());
+        }
+    }
+
     /// @brief Destructor.
     virtual ~Dhcpv6SharedNetworkTest() {
         StatsMgr::instance().removeAll();
@@ -1895,67 +2019,22 @@ TEST_F(Dhcpv6SharedNetworkTest, sharedNetworkSelectedByInterfaceIdInSubnet) {
     ASSERT_TRUE(hasLeaseForAddress(client2, IOAddress("2001:db8:2::20")));
 }
 
-// Check that the rapid-commit works with shared networks:
-// - that it can be defined on per subnet basis
-// - that its value can be mixed (some subnets have enabled, others disabled)
-TEST_F(Dhcpv6SharedNetworkTest, sharedNetworkRapidCommit) {
-
-    // Create client #1. This clients wants to use rapid-commit.
-    Dhcp6Client client1;
-    client1.setInterface("eth1");
-    client1.useRapidCommit(true);
-
-    Dhcp6Client client2;
-    client2.setInterface("eth1");
-    client2.useRapidCommit(true);
-
-    // Configure the server with a shared network.
-    ASSERT_NO_FATAL_FAILURE(configure(NETWORKS_CONFIG[16], *client1.getServer()));
-
-    // Ok, client should have one
-    EXPECT_EQ(0, client1.getLeaseNum());
-
-    std::cout << "Client 1 got " << client1.getLeaseNum() << " lease(s): ";
-    for (int i = 0; i < client1.getLeaseNum(); i++) {
-        Lease6 lease = client1.getLease(i);
-        std::cout << lease.addr_.toText() << " ";
-    }
-    std::cout << std::endl;
-
-    // Client #1 should be assigned an address from shared network. The first
-    // subnet has rapid-commit enabled, so the address should be assigned.
-    ASSERT_NO_THROW(client1.requestAddress(0xabca0));
-    testAssigned([this, &client1] {
-        ASSERT_NO_THROW(client1.doSolicit());
-    });
-
-    std::cout << "Client 1 got " << client1.getLeaseNum() << " lease(s): ";
-    for (int i = 0; i < client1.getLeaseNum(); i++) {
-        Lease6 lease = client1.getLease(i);
-        std::cout << lease.addr_.toText() << " ";
-    }
-    std::cout << std::endl;
-
-    // Make sure that REPLY was sent back.
-    ASSERT_TRUE(client1.getContext().response_);
-    EXPECT_EQ(DHCPV6_REPLY, client1.getContext().response_->getType());
-
-    // Create client #2. This client behaves the same as the first one, but the
-    // first subnet is already full (it's a really small subnet) and the second
-    // subnet does not allow rapid-commit.
-    testAssigned([this, &client2] {
-        ASSERT_NO_THROW(client2.doSolicit());
-    });
-
-    EXPECT_EQ(0, client2.getLeaseNum());
+// Check that the rapid-commit works with shared networks. Rapid-commit
+// enabled on each subnet separately.
+TEST_F(Dhcpv6SharedNetworkTest, sharedNetworkRapidCommit1) {
+    testRapidCommit(NETWORKS_CONFIG[16], true, "2001:db8:1::20", "2001:db8:2::20");
+}
 
-    // Make sure that ADVERTISE was sent back.
-    ASSERT_TRUE(client2.getContext().response_);
-    EXPECT_EQ(DHCPV6_ADVERTISE, client2.getContext().response_->getType());
+// Check that the rapid-commit works with shared networks. Rapid-commit
+// enabled for the whole shared network. This should be applied to both
+// subnets.
+TEST_F(Dhcpv6SharedNetworkTest, sharedNetworkRapidCommit2) {
+    testRapidCommit(NETWORKS_CONFIG[17], true, "2001:db8:1::20", "2001:db8:2::20");
+}
 
-    // Just make sure the client didn't get an address.
-    EXPECT_FALSE(hasLeaseForAddress(client2, IOAddress("2001:db8:2::20"),
-                                    LeaseOnServer::MUST_NOT_EXIST));
+// Check that the rapid-commit is disabled by default.
+TEST_F(Dhcpv6SharedNetworkTest, sharedNetworkRapidCommit3) {
+    testRapidCommit(NETWORKS_CONFIG[1], false, "", "");
 }
 
 } // end of anonymous namespace