[#2485] wrong subnet-id with shared network - backport #2409 to 2.0.3

diff --git a/ChangeLog b/ChangeLog
index 5c8759b64971f43729f796d0f084b0da727c727f..de0814a7e531d3cb8fa1b6e3978a53ee321b04c1 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+1960.  [bug]           marcin
+       A bug in the allocation engine, which caused it to write an
+       allocated lease under the wrong subnet ID within a shared
+       network, has been corrected. This was occurring when multiple
+       clients matched the same fixed address reservation. The first
+       client is now assigned the fixed address, while a subsequent
+       client is then given a dynamically allocated address from a
+       different subnet in the shared network.
+       (Gitlab #2409, #2485)
+
+1959.  [bug]           razvan, marcin
+       Fixed a crash in HA+MT scenario caused by a race condition
+       between resetting the CalloutHandle state and accessing hook
+       point parameters from different threads when unparking packets.
+       (Gitlab #2473, #2477)
+
 1958.  [build]         andrei
        Froze sphinx dependency versions used to build documentation.
        Added the update-python-dependencies Makefile rule to bump the

diff --git a/src/bin/dhcp4/tests/shared_network_unittest.cc b/src/bin/dhcp4/tests/shared_network_unittest.cc
index 60eb4550e750802aa7f74dc646c1bbd1971e2488..cc1f908163e82f7f11278232268296b5949e39f2 100644 (file)
--- a/src/bin/dhcp4/tests/shared_network_unittest.cc
+++ b/src/bin/dhcp4/tests/shared_network_unittest.cc
@@ -1013,8 +1013,47 @@ const char* NETWORKS_CONFIG[] = {
     "            ]"
     "        }"
     "    ]"
-    "}"
+    "}",
 
+// Configuration #20
+// - a shared network with two subnets
+// - first subnet has a dynamic address pool
+// - second subnet has no address pool but it has a host reservation
+    "{"
+    "    \"interfaces-config\": {"
+    "        \"interfaces\": [ \"*\" ]"
+    "    },"
+    "    \"valid-lifetime\": 600,"
+    "    \"shared-networks\": ["
+    "        {"
+    "            \"name\": \"frog\","
+    "            \"relay\": {"
+    "                \"ip-address\": \"192.3.5.6\""
+    "            },"
+    "            \"subnet4\": ["
+    "                {"
+    "                    \"subnet\": \"10.0.0.0/24\","
+    "                    \"id\": 100,"
+    "                    \"pools\": ["
+    "                        {"
+    "                            \"pool\": \"10.0.0.1 - 10.0.0.1\""
+    "                        }"
+    "                    ]"
+    "                },"
+    "                {"
+    "                    \"subnet\": \"192.0.2.0/26\","
+    "                    \"id\": 10,"
+    "                    \"reservations\": ["
+    "                        {"
+    "                            \"circuit-id\": \"'charter950'\","
+    "                            \"ip-address\": \"192.0.2.28\""
+    "                        }"
+    "                    ]"
+    "                }"
+    "            ]"
+    "        }"
+    "    ]"
+    "}"
 };
 
 /// @Brief Test fixture class for DHCPv4 server using shared networks.
@@ -1681,6 +1720,56 @@ TEST_F(Dhcpv4SharedNetworkTest, reservationInSharedNetwork) {
     });
 }
 
+// Two clients use the same circuit ID and this circuit ID is used to assign a
+// host reservation. The clients compete for the reservation, and one of them
+// gets it and the other one gets an address from the dynamic pool. This test
+// checks that the assigned leases have appropriate subnet IDs. Previously, the
+// client getting the lease from the dynamic pool had a wrong subnet ID (not
+// matching the address from the dynamic pool).
+TEST_F(Dhcpv4SharedNetworkTest, reservationInSharedNetworkTwoClientsSameIdentifier) {
+    // Create client #1 which uses a circuit ID as a host identifier.
+    Dhcp4Client client1(Dhcp4Client::SELECTING);
+    client1.useRelay(true, IOAddress("192.3.5.6"));
+    client1.includeClientId("01:02:03:04");
+    client1.setCircuitId("charter950");
+
+    // Create server configuration with a shared network including two subnets.
+    // One of the subnets includes a reservation identified by the client's
+    // circuit ID.
+    configure(NETWORKS_CONFIG[20], *client1.getServer());
+
+    // Client #1 should get the reserved address.
+    testAssigned([this, &client1] {
+        doDORA(client1, "192.0.2.28", "");
+    });
+
+    // Create client #2 with the same circuit ID.
+    Dhcp4Client client2(client1.getServer(), Dhcp4Client::SELECTING);
+    client2.useRelay(true, IOAddress("192.3.5.6"));
+    client2.includeClientId("02:03:04:05");
+    client2.setCircuitId("charter950");
+
+    // Client #2 presents the same circuit ID but the reserved address has been
+    // already allocated. The client should get an address from the dynamic pool.
+    testAssigned([this, &client2] {
+        doDORA(client2, "10.0.0.1", "192.0.2.28");
+    });
+
+    // Client #1 renews the lease.
+    client1.setState(Dhcp4Client::RENEWING);
+    testAssigned([this, &client1]() {
+        doRequest(client1, "192.0.2.28");
+    });
+
+    // Client #2 renews the lease.
+    client2.setState(Dhcp4Client::RENEWING);
+    // Check if the client successfully renewed its address and that the
+    // subnet id in the renewed lease is not messed up.
+    testAssigned([this, &client2]() {
+        doRequest(client2, "10.0.0.1");
+    });
+}
+
 // Reserved address can't be assigned as long as access to a subnet is
 // restricted by classification.
 TEST_F(Dhcpv4SharedNetworkTest, reservationAccessRestrictedByClass) {

diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc
index 085da6c697e54b40be9030eb3abeb6621eba705f..ff68d37b5b131741ad811977ef5f818f1f646545 100644 (file)
--- a/src/lib/dhcpsrv/alloc_engine.cc
+++ b/src/lib/dhcpsrv/alloc_engine.cc
@@ -3816,12 +3816,14 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) {
     // If the client is requesting an address which is assigned to the client
     // let's just renew this address. Also, renew this address if the client
     // doesn't request any specific address.
-    // Added extra checks: the address is reserved or belongs to the dynamic
-    // pool for the case the pool class has changed before the request.
+    // Added extra checks: the address is reserved for this client or belongs
+    // to the dynamic pool for the case the pool class has changed before the
+    // request.
     if (client_lease) {
         if (((client_lease->addr_ == ctx.requested_address_) ||
              ctx.requested_address_.isV4Zero()) &&
-            (hasAddressReservation(ctx) ||
+            ((hasAddressReservation(ctx) &&
+              (ctx.currentHost()->getIPv4Reservation() == ctx.requested_address_)) ||
              inAllowedPool(ctx, client_lease->addr_))) {
 
             LOG_DEBUG(alloc_engine_logger, ALLOC_ENGINE_DBG_TRACE,