Opened 4 years ago

Closed 4 years ago

#4552 closed enhancement (complete)

Host Reservations: next-server and filename should be supported

Reported by: marcin 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: 40 Add Hours to Ticket: 3
Total Hours: 18 Internal?: no


The DHCPv4 message includes 'siaddr' and 'file' fields which can be configured on per-host basis. Our current implementation allows for assigning DHCP options for a host, but those fields are not options and thus need special handling. This will require updates in numerous places, such as Host object, MySQL and PostgreSQL schema, MySQL and Postgres host datasource (query updates), configuration parsers for in-memory reservations etc.


Change History (5)

comment:1 Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 0 to 12
  • Owner set to UnAssigned
  • Status changed from new to reviewing
  • Total Hours changed from 0 to 12

The code has been implemented and the User's Guide updated. One more thing is that we need to update to reflect the changes in schema. Though, this is not a part of the released code, so we can make this update in parallel with the release preparation.

ChangeLog? proposed:

11XX.	[func]		marcin
	Added support for static reservations for fixed fields in
	DHCPv4 messages: siaddr, sname and file.
	(Trac #4552, git abcd)

comment:2 Changed 4 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:3 Changed 4 years ago by tomek

  • Add Hours to Ticket changed from 12 to 3
  • Owner changed from tomek to marcin
  • Total Hours changed from 12 to 15
Dhcp4Client::applyConfiguration() The code that copies over sname_ and
boot_file_name_ assumes that the thing is null terminated. It usually
is, but for 64 bytes long sname it is not. The same is true for 128
bytes long filename. On a related note I think we should update the
length checks a bit. Since it's supposed to be null terminated, we
can allow strings up to 63 and 127 bytes, not 64 and 128. Oh well,
I'll update the unit-tests in #4626.

HostReservationParser4::build - The code doesn't check if next-server
is not IPv6 address.|h
The code in Host::setNextServer throws if the value is I
think this check should be removed. I can imagine a case where most
devices in your network have it set to some value, but then for some
selected hosts you want to actually unset it, e.g. because the devices
are buggy. The check against is probably ok. I don't
think using broadcast to discover the server dynamically makes sense.
On the other hand, how far are you willing to go to say the user what
is valid and what is not? What if the user wants to assign addresses
127.x.x.x. Is it valid or not? Should we even care?

Having said that, supporting unsetting next-server in hosts this is a
corner case we don't need to support on day 1. I can imagine some
tricky areas here, as we would have to differentiate between the
next-server not being set and being set to, schema scripts for MySQL/PgSQL
Adding columns without bumping up the schema revision is a bit shady,
but I see there's #4562 for that. We'll just need to make sure it
comes in right after #4552.

The <userinput> tag should start one line later (hw-address should
not be emphasized).

The code compiled and unit-tests passed on Ubuntu 16.04 x64 with
--with-dhcp-mysql and --with-dhcp-pgsql.

As discussed on jabber, it seems that #4626 should go in first
and then this (#4552) should tweak the code to call
Dhcp4Srv::setReservedMessageFields from Dhcp4Srv::setFixedFields.

The changelog looks ok, but I would explicitly mention names of
the parameters together with the field names in the packet.

comment:4 Changed 4 years ago by marcin

  • Milestone changed from Kea-proposed to Kea1.1

comment:5 Changed 4 years ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 15 to 18

Fixed issues from review and merged with commit 9b79fe005dd77328ea7c596fc6886f8fb838d1cf.

Note: See TracTickets for help on using tickets.