Opened 4 years ago

Closed 4 years ago

#4212 closed enhancement (complete)

Implement missing IPv6 address/prefix in host reservations in MySQL

Reported by: tomek Owned by: marcin
Priority: medium Milestone: Kea1.1
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

Ticket #3682 introduced MySQL host data source implementation that stores hosts information in MySQL. It covers majority of the functionalities: add and several retrieve operations. Two things were skipped there:

  1. IPv6 address/prefix reservation (because IPv6 allows multiple addresses, this requires additional code to insert the host, retrieve its host_id and then insert necessary reservations. The same is true for retrieval. Additional query to get all IPv6 reservations for a given host_id are required.
  1. Classes information stored in MySQL.

This ticket covers functionality 1 described above and can be considered a direct follow-up for #3682.

Subtickets

Attachments (2)

0001-4212-Typo-and-compareReservations-function-fixes.patch (6.9 KB) - added by kalmus 4 years ago.
4212 fixes
mysql-host-exchange.svg (36.1 KB) - added by marcin 4 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by tomek

  • Component changed from dhcpdb to host-reservations

comment:2 Changed 4 years ago by kalmus

ChangeLog? entry proposal:

XXX. [func] kalmus
Implements IPv6 address/prefix reservations as separate ipv6_reservations 
MySQL table entries and associates them with proper host reservation.
(Trac #4212, git XXX)
Last edited 4 years ago by kalmus (previous) (diff)

comment:3 Changed 4 years ago by kalmus

  • Owner set to tomek
  • Status changed from new to reviewing

comment:4 Changed 4 years ago by tomek

  • Owner changed from tomek to kalmus

I did check the last patch I received over e-mail, applied it to trac4212 and pushed.
The patch applied cleanly, the code compiled fine and unit-tests have passed.

I do have couple comments, but they should be easy to address. In general,
good work!

host.h

Reserving a temporary address, that by its definition is temporary and
should change, is absurd. However, that never prevented people from
doing it. Sooner or later, we will be asked to support this.

mysql_host_data_source.h

Unless there's a good reason to keep them public, both
getAllReservations() and assignReservations() should be private.

mysql_host_data_source.cc

This include:

#define __STDC_FORMAT_MACROS
#include <inttypes.h>

can be replaced with simpler:

#include <stdint.h>

In theory, including C++ version (cstdint, rather than stdint.h) would
be even better, but sadly it creates warnings in many modern
compilers, because it's a C++11 feature. Using stdint.h is what we
are doing in all over the code, so it should be fine here as well.

Why is the reservation_id set manually when adding a reservation? I'm
not saying it's necessarily a bad thing, just asking. We include it
in the statement (INSERT INTO ipv6_reservations(reservation_id, ...),
but then set reservation_id to NULL. Wouldn't it be simpler if it was
not set at all? I think the schema would enforce it anyway (this
column is defined as: reservation_id INT NOT NULL AUTO_INCREMENT)
and the code would be simpler.

MySqlIPv6ReservationExchange class should have a class comment that
would explain its purpose. It doesn't have to be long.

Comment in line 855 is incorrect:

uint64_t    reservation_id_; /// Host unique identifier

MySqlHostDataSource::add(). Good approach with the mysql_insert_id.

There are couple places where the result is checked this way:

if (status == 1) {

mysql_stmt_fetch documentation uses explicit value of 1, not a
defined constant. Nevertheless, it would be useful to either define
a constant for it or add a comment that explains where this value of 1
came from.

generic_host_data_source_unittest.cc
The loop in the GenericHostDataSourceTest::compareReservations6 goes
through two IPv6 reservations. In most cases it will work ok, but what
would happen if you inserted 3 reservations into MySQL? Get IPv6
reservations would return them, but there is no guarantee that
they'll be retrieved in the same order as they were inserted.
True, in most cases MySQL will return them in that order, but that's
not guaranteed in any way. I think the algorithm here should be:

  1. check if the number of reservations is equal. If not, fail.
  2. for each reservation on the first list, find a reservation on the second list that is an exact match.

This approach would ensure that the lists are compared correctly, even
if one of the compared lists is in a different order.


Finally, there's a potential problem with duplicate reservations.
This is a complex problem, so I don't think we should solve it in
this ticket. However, it would be useful to think about the potential
conficts and add @todo comments in the code, possibly even ideas
for new tests. We do have testAddDuplicate that uses a host with IPv6
address. It checks if the second host with exactly the same DUID
can be added. That's fine, but similar test should be conducted for
MAC address and IPv4 host. Other scenarios I was thinking about were:

  1. add host A (with MAC = X) with reservation for IPv4 address Y. Try to add host B (with different MAC) with reservation for address Y. The identifiers are different, but the addresses reserved are the same, so the second addition should fail.
  1. the same as above, but for IPv6 addresses.
  1. add host A with 3 IPv6 reservations. Try to add host B with different duid, but one of the addresses matching an address reserved for host A.

I think this is a topic that should be handled in a separate ticket.
This is not something we initially planned, so don't worry. You won't
have to worry about it. One of ISC engineers will take care of it
one day.


I have not tested whether the reservation is actually working, but I
did set up the environment. There's one urgent task that will keep me
busy tomorrow, but I should get back to this on Monday.


The proposed ChangeLog? entry looks fine.

Please also add a note to AUTHORS file.


The code compiled and unit-tests passed on Ubuntu 15.10 x64.

Reassigning back to you, so you can tweak the code a bit. But expect
a report from testing early next week.

comment:5 Changed 4 years ago by kalmus

Sent fixes to issues addressed above via mail, reassigning for further review.

comment:6 Changed 4 years ago by kalmus

  • Owner changed from kalmus to tomek

comment:7 Changed 4 years ago by tomek

  • Owner changed from tomek to marcin

Adam sent me a patch over mail. I applied Adam's changes as
commit 51610a48a3c5ba49f6ffb164a569fe052a9933d8 and pushed them
to trac4212. Please make sure to pull before doing any further
changes.

mysql_host_data_source.cc
There are 3 instances where results of MySQL function are compared
against 0 and several others that compare against 1. MySQL does not
define those return codes. We should consider defining them ourselves.
Marcin, any thoughts on this? We do the same in

generic_host_data_source_unittest.cc

I'm not sure why compareReservations6 has the expect_match parameters.
Whatever the original intention was, it is not used now (and not
documented). It seems reasonable to get rid of this parameter.
This would make the implementations lightly easier.

The loops in compareReservations6 look slightly odd. It would
be easier if both loops were of the same type: either two
fors (which seem more readable) or two whiles.

Also, the if (ite == resrv2.second && expect_match) check could
be implemented outside of the second loop, it doesn't have to be
checked while the inner loop is still running, only after it
was completed.

There's a typo in several method names:
testMultipletReservations => testMultipleReservations (remove t)
testMultipletReservationsDifferentOrder =>
testMultipleReservationsDifferentOrder

Except those comments above, the changes you did are good.
Adam, pleasePlease address them. I'll also ask Marcin to review the code.

I did verify that the code compiles and unit-tests pass on
Ubuntu 15.10 x64.


Marcin, one thing that's not covered is a sanity check against
multiple reservations for the same address. The code as currently
written is able to detect attempts to add a new reservation with
the same DUID or MAC address. However, two hosts with different MACs
or DUIDs still could have the same address reserved. I think
that's ok for now, as we have the same deficiency in the
HostMgr? storage (see @todo in CfgHosts::add{4,6}). We should
probably add a separate ticket for this. Given that the code
in CfgHosts? is there and noboby is screaming at us, I presume
it may be somewhat low priority.


Note: The headers in Kea has changed since this code was branched.
I did not update copyright years, as it could cause unnecessary issues
when merging. Care should be taken to verify dates when the merge
is done.

Changed 4 years ago by kalmus

4212 fixes

comment:8 Changed 4 years ago by kalmus

Fixes according to Tomek's comments attached in patch file above.

Changed 4 years ago by marcin

comment:9 Changed 4 years ago by marcin

The code and the unit tests were useful.

But, the problem was that in order to obtain a host and its IPv6 reservations two queries were needed. The host reservation design calls for retrieving both host and reservations using a single query. To support this, I had to refactor the 'exchange' classes in the MySqlHostDataSource. The class diagram presenting new exchange classes and their relation to the MySqlHostDataSource is depicted below.


The MySqlHostExchange is a base class and it is used to retrieve !Host information without IPv6 reservations. It is used by the DHCPv4 server.

The !MySqlIPv6HostExchange extends the MySqlHostExchange class to also retrieve IPv6 reservations. It does both in a single query.

The !MySqlIPv6ReservationExchange is now solely used to insert new reservations into the ipv6_reservations table.

The MySqlHostDataSourceImpl is a container for all exchange classes and it basically separates API from the implementation, which is quite complex.

comment:10 Changed 4 years ago by marcin

  • Owner changed from marcin to UnAssigned

comment:11 Changed 4 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:12 Changed 4 years ago by tomek

This is part 1 of the review.

This review covers changes Marcin did in commits 24b7fb0e6f57fa808240675e556583b3ef91ecb4 through 7284e6874488e7e919c82ae2a0501345f3e9cfa7.

classify.cc
Copyright year should be 2014-2016.

mysql_host_data_source.h
Copyright year should be updated.

Comment for the destructor claims that the code releases prepared
MySQL statements. It does delete the actual implementation object.
I think the comment should be updated.

mysql_host_data_source.cc
Do you think it would be useful to add comments regarding the queries?
They could explain what is the expected returned information (a single
or multiple entries).

creaeBindForSend lines 273 through 288. What would happen if a host
was provided without duid or hw-address. We will soon have such hosts,
e.g. with remote-id reservations. I think the method should throw for
now, rather than leave bind_[1] empty.

line 682: why is ipv6_address_buffer_len_ initialized to 2?
ipv6_address_buffer_ size is ADDRESS6_TEXT_MAX_LEN, so it would make
sense to initalize it to 0 or ADDRESS6_TEXT_MAX_LEN.

line 736: This is a follow up to question about line 682. If the
ipv6_address_buffer_len_ is still set to 2, it would work, but what's
the point? If ipv6_address_buffer_len_ is set to the actual size
of ipv6_address_buffer_ field, then there's buffer overflow problem
in 736 (off by one).

Commnet for MySqlIPv6ReservationExchange class:
inserted to the database => inserted into the database

Line 928: You initialized the bind_ array with:

memset(bind_, 0, sizeof(bind_));

yet in other places bind_ is initialized with

memset(&bind_[0], 0, sizeof(MYSQL_BIND) * bind_.size());

while the second approach is correct, the first one works, too.
Since the bind_ definition known in the context the memset
command is used, sizeof(bind_) returns actual size
(i.e. sizeof(MYSQL_BIND) * bind_.size())


End of part 1. Part 2 of the review will resume from line 1100 of
mysql_host_data_source.cc and will cover unit-tests.

comment:13 Changed 4 years ago by tomek

  • Owner changed from tomek to marcin

This is part 2 of my review.

mysql_host_data_source.cc
Copyright year should be updated to 2016.

The current code uses if/else/if construct to determine whether
it's a DUID based or hwaddr based reservation. The logic should be
extended to cover cases where neither DUID nor hwaddr are specified.
It's ok to throw something for now, e.g. NotImplemented?. They all
should be extended with a final else clause that throws an exception.
Otherwise we could have empty columns representing identifier type and
identifier value when running a query. This will become essential to
cover all those cases when new reservation types will start to appear.
It's better to throw something rather than silenty lease the columns empty.

This is covered in MySqlHostDataSource::get4, but it's not in:

  • MySqlHostExchange::createBindForSend (lines 288 and 302)
  • MySqlHostDataSource::getAll (line 1320)
  • MySqlHostDataSource::get4 (line 1406)
  • MySqlHostDataSource::get6 (line 1506)

The last one may require a bit more work as the !hwaddr && !duid check
is done in line 1473. But since this method is dedicate to get by duid
or hwaddr, maybe it does not require any update after all?

MySqlHostIPv6Exchange::createBindForReceive() - there should be a
comment that the first line called base method that already set
offsets 0 through 8 of bind_.

retrieveHost(), line 480 - exception when handling default value: It
should use word "supported" rather than "allowed". This is a fine
distinction, but it will become important when someone running
Kea 1.0 will try to connect it to database with Kea 1.1 data (e.g.
remote-id reservations).

generic_host_data_source_unittest.h
Copyright year 2016 is missing.


I made small compilation fix, please pull before you work on this.

The code compiles (with -O0 to avoid known issue with gcc-5) on Ubuntu
15.10 x64 and all unit-tests pass.


Your comment:9 is very useful. Can you make sure that it's retained?
Maybe turn it into a comment in mysql_host_data_source.h?


If you don't manage to get this done before I disappear on vacation,
I'm ok with merging the code. Marcin, please make sure you credit
Adam appropriately in both ChangeLog? and AUTHORS. It may be very
useful for Adam to show those files during his master thesis defense.


Thanks for doing this work. I think this capability is sufficiently
important and useful (several people asked about it), so once it's
merged you can consider posting something about it to kea-dev or
kea-users.

comment:14 Changed 4 years ago by marcin

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

Merged with 79481043935789fc6898d4743bede1606f82eb75.

Note: See TracTickets for help on using tickets.