Opened 3 years ago

Closed 3 years ago

#5029 closed defect (fixed)

PXE boot clients can't boot because of multiple leases for the same hw-address in the database

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.2
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Apparently, Kea DHCPv4 server causes trouble for PXE boot clients by restricting the number of clients in the lease database, which use the same HW address. The issue has been raised here:
https://lists.isc.org/pipermail/kea-dev/2016-August/000703.html

The sequence of events seems to be: BIOS requests a lease using client-id0/hw-address. The installer requests a lease sign client-id1/hw-address. Now we have two leases in the database for the same host but using two different client-ids. The host comes up after installation and it uses client-id2/hw-address. The server checks that there are two leases for the same hw-address already and refuse to allocate a lease for the host.

Subtickets

Change History (8)

comment:1 Changed 3 years ago by marcin

  • Milestone changed from Kea-proposed to Kea1.2

Moving to 1.2 per Kea team call on 10/20/2016.

comment:2 Changed 3 years ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:3 Changed 3 years ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

I have implemented the fix. The lease backends will still report an error when getLease is called and there are multiple leases for the same HW address. We may need to think if we want to allow returning a first lease, for example, but in this case which lease is returned is unpredictable so I am iffy if we should really go down this path. Instead, I modified the allocation engine to retrieve all leases for the particular HW address and check which one belongs to us (if any). We could add a new function that returns all leases for the particular HW address and subnet id to improve performance.

In order to test that my change also works with Cassandra backend I finally decided to setup Cassandra on my local systems. It failed badly on BSD, with the version from FreeBSD ports. The reason was that I was unable to connect to the running Cassandra server. So, I downloaded cassandra 3.9 (latest version) and tried that. The server crashed on startup. Cassandra logs claimed that OpenJDK us not recommended. At the same time, Oracle doesn't provide java version for BSD systems. I gave up on this after a couple of hours. Perhaps we need to wait for the updated FreeBSD port of Cassandra. However, we may need to put something in the Kea documentation.

Then, I tried installing Cassandra on macOS sierra. This mostly worked but I had a crash in CqlLeaseMgr unit tests. This appeared to be due to the use of local variable in the CqlDataArray objects used to bind variables to the Cql queries. The local variable was modified by the query and then it went out of scope before the Cql structures did and resulted in a crash on deallocation.

This is the entire diff that I had to apply to successfully build with Cassandra support and run unit tests. Some things are unrelated but there are a few that should be resolved as a separate ticket:

  • configure.ac should specify location of the libuv, otherwise it won't be found if it is installed in non-standard location
  • boost headers should be included in the checks for GTEST - but this has been already resolved on master
  • local variables should not be used in CQL binding tables. In the diff below I made them static, which works ok, but maybe we want to put them in the class scope? The use of static seems to be ok for me, though.
  • The readlink -f $0 in the cql_config doesn't seem to be portable. Perhaps better to use: "$(cd $(dirname $0); pwd -P)"
diff --git a/configure.ac b/configure.ac
index ead7e9f..5b36970 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1061,7 +1061,7 @@ if test "$CQL_CONFIG" != "" ; then
 
     CQL_INCLUDEDIR=`$CQL_CONFIG --includedir`
     CQL_CPPFLAGS="`$CQL_CONFIG --cppflags` -I$CQL_INCLUDEDIR"
-    CQL_LIBS="-L`$CQL_CONFIG --libdir` -lcassandra_static -luv"
+    CQL_LIBS="-L`$CQL_CONFIG --libdir` -L$LIBUV_ROOT_DIR/lib -lcassandra_static -luv"
     CQL_VERSION=`$CQL_CONFIG --version`
 
     AC_SUBST(CQL_CPPFLAGS)
@@ -1278,7 +1278,7 @@ AC_SUBST(GTEST_SOURCE)
 if test $enable_gtest != "no"; then
    AC_MSG_CHECKING([if Google Test is compatible with the compiler])
    CPPFLAGS_SAVED=$CPPFLAGS
-   CPPFLAGS="$CPPFLAGS $GTEST_INCLUDES"
+   CPPFLAGS="$CPPFLAGS $BOOST_INCLUDES $GTEST_INCLUDES"
    AC_COMPILE_IFELSE(
        [AC_LANG_PROGRAM(
            [#include <boost/shared_ptr.hpp>
diff --git a/src/lib/dhcpsrv/cql_lease_mgr.cc b/src/lib/dhcpsrv/cql_lease_mgr.cc
index f7486c6..e6a6606 100644
--- a/src/lib/dhcpsrv/cql_lease_mgr.cc
+++ b/src/lib/dhcpsrv/cql_lease_mgr.cc
@@ -1239,12 +1239,13 @@ CqlLeaseMgr::addLeaseCommon(StatementIndex stindex,
     CassIterator* rows = cass_iterator_from_result(resultCollection);
     CqlDataArray appliedData;
     CqlDataArray appliedSize;
-    bool applied = false;
+    static bool applied = false;
     while (cass_iterator_next(rows)) {
         const CassRow* row = cass_iterator_get_row(rows);
         // [applied]: bool
         appliedData.add(reinterpret_cast<void*>(&applied));
         appliedSize.add(NULL);
+
         CqlLeaseMgr::getData(row, exchange.parameters_.size() - 1, appliedData,
                              appliedSize, 0, exchange);
     }
@@ -1255,6 +1256,7 @@ CqlLeaseMgr::addLeaseCommon(StatementIndex stindex,
     cass_future_free(future);
     cass_statement_free(statement);
 
+
     return applied;
 }
 
@@ -1673,7 +1675,7 @@ CqlLeaseMgr::updateLeaseCommon(StatementIndex stindex,
     CassIterator* rows = cass_iterator_from_result(resultCollection);
     CqlDataArray appliedData;
     CqlDataArray appliedSize;
-    bool applied = false;
+    static bool applied = false;
     while (cass_iterator_next(rows)) {
         const CassRow* row = cass_iterator_get_row(rows);
         // [applied]: bool
@@ -1777,7 +1779,7 @@ CqlLeaseMgr::deleteLeaseCommon(StatementIndex stindex,
     CassIterator* rows = cass_iterator_from_result(resultCollection);
     CqlDataArray appliedData;
     CqlDataArray appliedSize;
-    bool applied = false;
+    static bool applied = false;
     while (cass_iterator_next(rows)) {
         const CassRow* row = cass_iterator_get_row(rows);
         // [applied]: bool
diff --git a/src/lib/dhcpsrv/dhcp4o6_ipc.cc b/src/lib/dhcpsrv/dhcp4o6_ipc.cc
index 063e808..86dfafc 100644
--- a/src/lib/dhcpsrv/dhcp4o6_ipc.cc
+++ b/src/lib/dhcpsrv/dhcp4o6_ipc.cc
@@ -71,7 +71,7 @@ int Dhcp4o6IpcBase::open(uint16_t port, EndpointType endpoint_type) {
     }
     // We'll connect to the loopback address so bind to it too.
     local6.sin6_addr.s6_addr[15] = 1;
-    if (bind(sock, (struct sockaddr *)&local6, sizeof(local6)) < 0) {
+    if (::bind(sock, (struct sockaddr *)&local6, sizeof(local6)) < 0) {
         ::close(sock);
         isc_throw(Dhcp4o6IpcError, "Failed to bind DHCP4o6 socket.");
     }
diff --git a/tools/cql_config b/tools/cql_config
index 19a426a..1c9fdb5 100755
--- a/tools/cql_config
+++ b/tools/cql_config
@@ -1,5 +1,5 @@
 #!/bin/bash
-DIR=$(readlink -f $0 | xargs dirname)
+DIR=$(cd $(dirname $0); pwd -P)
 if ! [ -f ${DIR}/cql_config_defines.sh ] || ! [ -x ${DIR}/cql_config_defines.sh ]
 then
 	echo "missing path configuration file for DataStax Cassandra (cql_config_defines.h)"
diff --git a/tools/cql_config_defines.sh b/tools/cql_config_defines.sh
index bf3810d..a90fe61 100755
--- a/tools/cql_config_defines.sh
+++ b/tools/cql_config_defines.sh
@@ -6,5 +6,5 @@
 
 # This variable should point to the directory where cpp-driver is compiled.
 # You can download cpp-driver sources from https://github.com/datastax/cpp-driver
-CPP_DRIVER_PATH="/please/specify/path/to/cpp-driver/in/cql_config_defines.sh"
+CPP_DRIVER_PATH="/Users/marcin/devel/cpp-driver/"

Proposed ChangeLog entry:

11XX.	[bug]		marcin
	DHCPv4 server allows for allocating multiple leases for the
	same hardware address if a different client identifier is
	used for each lease. This facilitates the PXE boot use cases
	where a booted machine may request a lease multiple times.
	(Trac #5029, git abcd)

comment:4 follow-up: Changed 3 years ago by fdupont

  • Owner changed from UnAssigned to marcin

There are 2 parts:

  • the fix itself: it seems right on read, I am waiting for Jenkins to check it.
  • Cassandra stuff: IMHO this should be in its own ticket.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by marcin

Replying to fdupont:

There are 2 parts:

  • the fix itself: it seems right on read, I am waiting for Jenkins to check it.
  • Cassandra stuff: IMHO this should be in its own ticket.

Did it work?

BTW, there is a separate for the Cassandra issues.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by fdupont

Replying to marcin:

Replying to fdupont:

There are 2 parts:

  • the fix itself: it seems right on read, I am waiting for Jenkins to check it.
  • Cassandra stuff: IMHO this should be in its own ticket.

Did it work?

=> Jenkins failed but with unrelated reasons twice on MySQL. So my answer should be yes at 90%.

BTW, there is a separate for the Cassandra issues.

=> there should be a link to it (IMHO you refer to #5065).

comment:7 in reply to: ↑ 6 Changed 3 years ago by marcin

Replying to fdupont:

Replying to marcin:

Replying to fdupont:

There are 2 parts:

  • the fix itself: it seems right on read, I am waiting for Jenkins to check it.
  • Cassandra stuff: IMHO this should be in its own ticket.

Did it work?

=> Jenkins failed but with unrelated reasons twice on MySQL. So my answer should be yes at 90%.

BTW, there is a separate for the Cassandra issues.

=> there should be a link to it (IMHO you refer to #5065).

Yes, that is #5065.

comment:8 Changed 3 years ago by marcin

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged with commit 03defed4d8bb9a997d31dbfcf30ae3f866bd3353

Note: See TracTickets for help on using tickets.