Opened 11 months ago

Closed 7 months ago

#5102 closed defect (fixed)

Fix issues with static reservations based on client-identifier.

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

Description

After receiving the following email: https://lists.isc.org/pipermail/kea-users/2017-January/000730.html we investigated how we handle reservations based on the client identifier in DHCPv4. As a result we found several issues which I am listing below:

  1. If "host-reservation-identifiers" is not specified the server will only check reservations by: hw-address, duid and circuit-id. The reservations based on client-id aren't taken into account.
  2. host_identifier_type table in MySQL and PostgreSQL doesn't contain "client-id". This is non-blocking issue but requires an administrator to insert identifier type of 3 directly into the database, rather than using the user friendly name "client-id".
  3. Even if "host-reservation-identifiers" is specified and it includes "client-id" the client-id based reservations do not work for MySQL and Postgres. The MySQL and PostgreSQL host datasources complain that the identifier 3 (client-id) is invalid. As a result the client's packet is dropped.

The are workarounds for two first issues: always specify the "host-reservation-identifiers" with "client-id" or "auto" values. Secondly, use identifier value of 3 when inserting the host into the database.

There is no workaround for the 3rd issue and it has to be addressed with appropriate code changes:

  1. Unit tests for MySQL and PostgreSQL data source must be corrected to verify that the reservation is returned by client-id.
  2. The following code change should address the issue:
    diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc
    index b829748..f4429c0 100644
    --- a/src/lib/dhcpsrv/mysql_host_data_source.cc
    +++ b/src/lib/dhcpsrv/mysql_host_data_source.cc
    @@ -73,7 +73,7 @@ const size_t BOOT_FILE_NAME_MAX_LEN = 128;
     ///
     /// This value is used to validate whether the identifier type stored in
     /// a database is within bounds. of supported identifiers.
    -const uint8_t MAX_IDENTIFIER_TYPE = static_cast<uint8_t>(Host::IDENT_CIRCUIT_ID);
    +const uint8_t MAX_IDENTIFIER_TYPE = static_cast<uint8_t>(Host::LAST_IDENTIFIER_TYPE);
     
     /// @brief This class provides mechanisms for sending and retrieving
     /// information from the 'hosts' table.
    diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.cc b/src/lib/dhcpsrv/pgsql_host_data_source.cc
    index 12a3cc7..4085f6c 100644
    --- a/src/lib/dhcpsrv/pgsql_host_data_source.cc
    +++ b/src/lib/dhcpsrv/pgsql_host_data_source.cc
    @@ -44,7 +44,7 @@ const size_t OPTION_VALUE_MAX_LEN = 4096;
     ///
     /// This value is used to validate whether the identifier type stored in
     /// a database is within bounds. of supported identifiers.
    -const uint8_t MAX_IDENTIFIER_TYPE = static_cast<uint8_t>(Host::IDENT_CIRCUIT_ID);
    +const uint8_t MAX_IDENTIFIER_TYPE = static_cast<uint8_t>(Host::LAST_IDENTIFIER_TYPE);
     
     /// @brief Maximum length of DHCP identifier value.
     const size_t DHCP_IDENTIFIER_MAX_LEN = 128;
    
  3. System tests in forge must be written to test the client-id case.

Subtickets

Change History (9)

comment:1 Changed 11 months ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.2

Per Kea team meeting 5 Jan, accept 1.2 as High

comment:2 Changed 11 months ago by marcin

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

comment:3 Changed 8 months ago by tomek

We now also have flexible id identifier, so if we touch the DB backends, that should be reflected, too.

comment:4 Changed 8 months ago by tomek

  • Milestone changed from Kea1.2 to Kea1.2-final

discussed on 2017-04-06, moving to 1.2-final.

comment:5 Changed 7 months ago by marcin

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

I fixed all the issues mentioned in this ticket.

Proposed ChangeLog entry:

12XX.	[bug]		marcin
	Resolved issues with DHCPv4 host reservations by 'client-id'
	when MySQL or PostgreSQL host database backend is in use.
	Also, the 'client-id' is used together with other host
	identifier types when 'host-reservation-identifiers' parameter
	is not specified.
	(Trac #5102, git abcd)

comment:6 follow-up: Changed 7 months ago by tmark

  • Owner changed from UnAssigned to marcin

src/bin/dhcp4/tests/dora_unittest.cc

TEST_F(DORATest, reservationByClientId) {
+ Dhcp4Client client(Dhcp4Client::SELECTING);
+ Use relay agent so as the circuit-id can be inserted.
+ client.useRelay(true, IOAddress("10.0.0.20"), IOAddress("10.0.0.21"));

Do we need the relay agent for this test? If so, please add commentary as to
why, if not I'd suggest removing it.


src/share/database/scripts/mysql/dhcpdb_create.mysql

Are these lines specific to this ticket or because it is also done as 5206?

+# Add indexes for lease tables which will be used to perform searches
+# for all leases by subnet id.
+CREATE INDEX lease4_subnet_id ON lease4 (subnet_id ASC);
+CREATE INDEX lease6_subnet_id ON lease6 (subnet_id ASC);

I noticed you did not do this for Postgres, so I'd suggest removing them from this fix and let 5206 add them.


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

  • Owner changed from marcin to tmark

Replying to tmark:

src/bin/dhcp4/tests/dora_unittest.cc

TEST_F(DORATest, reservationByClientId) {
+ Dhcp4Client client(Dhcp4Client::SELECTING);
+ Use relay agent so as the circuit-id can be inserted.
+ client.useRelay(true, IOAddress("10.0.0.20"), IOAddress("10.0.0.21"));

Do we need the relay agent for this test? If so, please add commentary as to
why, if not I'd suggest removing it.

I updated the commentary.


src/share/database/scripts/mysql/dhcpdb_create.mysql

Are these lines specific to this ticket or because it is also done as 5206?

+# Add indexes for lease tables which will be used to perform searches
+# for all leases by subnet id.
+CREATE INDEX lease4_subnet_id ON lease4 (subnet_id ASC);
+CREATE INDEX lease6_subnet_id ON lease6 (subnet_id ASC);

I noticed you did not do this for Postgres, so I'd suggest removing them from this fix and let 5206 add them.

That was an oversight. I removed that from the MySQL script now.

comment:8 Changed 7 months ago by tmark

  • Owner changed from tmark to marcin

Changes are good. Please merge them.

comment:9 Changed 7 months ago by marcin

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

Merged with commit 390d687d0f61635f5562d13860ff6362eee67853

Note: See TracTickets for help on using tickets.