Opened 3 years ago

Closed 18 months ago

#3572 closed enhancement (complete)

Use options defined for a given host when responding to a DHCPv4 client

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: 24 Add Hours to Ticket: 2
Total Hours: 17 Internal?: no

Description (last modified by marcin)

Once #3571 is done, we will need to update appendRequestedOptions() method (or other part of the DHCPv4) to include those options.

Several cases will need to be covered here:

  1. send options from subnet if there are no options in host scope (or there is no host definition)
  2. send options from host if present, even if there are options with the same code specified in subnet

In other words, options defined in host scope should override options from subnet.

Subtickets

Change History (24)

comment:1 Changed 3 years ago by hschempf

  • Priority changed from medium to low

comment:2 Changed 3 years ago by tomek

  • Milestone changed from Kea0.9.1beta to Kea0.9.1

Moving from 0.9.1beta to 0.9.1.

comment:3 Changed 3 years ago by hschempf

  • Priority changed from low to very low

comment:4 Changed 3 years ago by hschempf

  • Milestone changed from Kea0.9.1 to Kea0.9.2

comment:5 Changed 3 years ago by hschempf

  • Milestone changed from Kea0.9.2 to Kea1.0
  • Priority changed from very low to medium

comment:6 Changed 2 years ago by hschempf

  • Priority changed from medium to high

Per decision during ticket estimates, changed to High

comment:7 Changed 2 years ago by stephen

  • Milestone changed from Kea1.0 to DHCP Outstanding Tasks

Per the Kea planning meeting in October, remove from 1.0.

comment:8 Changed 2 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

comment:9 Changed 2 years ago by tomek

  • Milestone changed from Outstanding Tasks to Kea1.1

comment:10 Changed 22 months ago by tomek

  • Component changed from dhcp4 to host-reservations

comment:11 Changed 22 months ago by tomek

  • Priority changed from high to medium

comment:12 Changed 18 months ago by marcin

  • Description modified (diff)
  • Owner set to marcin
  • Status changed from new to assigned

comment:13 Changed 18 months ago by marcin

  • Status changed from assigned to accepted

comment:14 Changed 18 months ago by marcin

  • Estimated Difficulty changed from 0 to 3

comment:15 Changed 18 months ago by marcin

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

This ticket is now ready for review.

Proposed ChangeLog entry:

11XX.	[func]		marcin
	DHCPv4 server assigns DHCP options specified for hosts. Host
	specific options take precedence over class specific options,
	subnet specific options and global options.
	(Trac #3572, git abcd)

comment:16 Changed 18 months ago by marcin

  • Add Hours to Ticket changed from 0 to 12
  • Estimated Difficulty changed from 3 to 24
  • Total Hours changed from 0 to 12

comment:17 Changed 18 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:18 Changed 18 months ago by tmark

  • Owner changed from tmark to Unassigned

comment:19 Changed 18 months ago by tomek

  • Owner changed from Unassigned to tomek

comment:20 follow-up: Changed 18 months ago by tomek

  • Owner changed from tomek to marcin

std_option_defs.h
Why did you change vivso-suboptions defintion from binary to
uint32? I'm looking at Section 4 of RFC3925 and the option
doesn't look like uint32. Yes, it starts with 32 bit enterprise-number
field, but other than that, there's not much in common with
32 bit integers. I looked at the configs you added in
host_options_unittest.cc and agree that usage seems convenient now.

However, this header should be updated. If you want to keep the change
as is, please please at least explain why uint32 is better than
binary. And update the comment in lines 206-210, which has
"until that happens let's thread them as 'binary' options".

dhcp4_srv.cc
I took a closer look at buildCfgOptionList and I think, while it is
working as written, there's a room for improvement. If you have an
option foo defined globally, for a subnet and for a host, the option
foo will appear on the list 3 times, with 2 of them being useless.
Then we iterate over that list to find a response. Note that
the length of that list depends on total combined number of
all options defined globally, in the subnet and for this
specific hosts. This list may not be as short as you would think.

It's also quite inefficient. Imagine a case where 20 different
options are specified and client asks for just one option. All 20
options will get on the list anyway.

I think we should consider doing the checks whether an option
will be sent back or not in the Dhcpv4Srv::buildCfgOptionList,
not in Dhcpv4Srv::appendRequestedOptions(). That's probably way
too big of a change, so if you agree with this optimization proposal,
let's create a ticket for it. If not, let's continue the discussion.

HostOptionsTest::testOverrideVendorOptions - in line 499 the code
deferentiates opt without ever checking if it's null or not.

host_options_unittest.cc
Please update different values for routers and log-servers. Also,
values for domain-name-servers and cookie-servers should be different.

Constructor and destructor of HostOptionsTest? wipe all statistics.
Do we really need it? As far as I can see, there's no test that ever
uses statatics here. Nor there is any statistic that is related
to host reservations.

Thanks for writing those tests. They look simple and are easy to
understand, yet they test quite complex scenarios.


Please add an example config that defines per host options.

I presume full documentation will be added in separate ticket. I
haven't checked if such ticket currently exists in 1.1 milestone.

Your proposed changelog looks ok.


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

comment:21 Changed 18 months ago by tomek

  • Add Hours to Ticket changed from 12 to 3
  • Total Hours changed from 12 to 15

comment:22 in reply to: ↑ 20 Changed 18 months ago by marcin

  • Add Hours to Ticket changed from 3 to 2
  • Owner changed from marcin to tomek
  • Total Hours changed from 15 to 17

Replying to tomek:

std_option_defs.h
Why did you change vivso-suboptions defintion from binary to
uint32? I'm looking at Section 4 of RFC3925 and the option
doesn't look like uint32. Yes, it starts with 32 bit enterprise-number
field, but other than that, there's not much in common with
32 bit integers. I looked at the configs you added in
host_options_unittest.cc and agree that usage seems convenient now.

However, this header should be updated. If you want to keep the change
as is, please please at least explain why uint32 is better than
binary. And update the comment in lines 206-210, which has
"until that happens let's thread them as 'binary' options".

I added commentary. I think there is still some room for improvement for this particular option, but what we have now should be good enough.

dhcp4_srv.cc
I took a closer look at buildCfgOptionList and I think, while it is
working as written, there's a room for improvement. If you have an
option foo defined globally, for a subnet and for a host, the option
foo will appear on the list 3 times, with 2 of them being useless.
Then we iterate over that list to find a response. Note that
the length of that list depends on total combined number of
all options defined globally, in the subnet and for this
specific hosts. This list may not be as short as you would think.

It's also quite inefficient. Imagine a case where 20 different
options are specified and client asks for just one option. All 20
options will get on the list anyway.

I think we should consider doing the checks whether an option
will be sent back or not in the Dhcpv4Srv::buildCfgOptionList,
not in Dhcpv4Srv::appendRequestedOptions(). That's probably way
too big of a change, so if you agree with this optimization proposal,
let's create a ticket for it. If not, let's continue the discussion.

I am not an author of this part of the code. I merely updated the existing function. However, I also don't see the issue you're seeing with the current approach. The list holds pointers to the CfgOption objects, so there is no copy construction involved. Only pointers copying. The information on the list is not really redundant because for any processed packet the server needs class specific, host specific, subnet specific and global CfgOption and only this information is stored on the list. The pointers do point to some of the options which will not be included, but those have to be eliminated at some point, and where they are eliminated doesn't really make any difference from the performance standpoint.

If you really see an issue with the current approach, we should better address it in a separate ticket, because this is going to be essential change and it is not related to this ticket.

HostOptionsTest::testOverrideVendorOptions - in line 499 the code
deferentiates opt without ever checking if it's null or not.

Added ASSERT_TRUE

host_options_unittest.cc
Please update different values for routers and log-servers. Also,
values for domain-name-servers and cookie-servers should be different.

They are now.

Constructor and destructor of HostOptionsTest? wipe all statistics.
Do we really need it? As far as I can see, there's no test that ever
uses statatics here. Nor there is any statistic that is related
to host reservations.

Copy paste error.

Thanks for writing those tests. They look simple and are easy to
understand, yet they test quite complex scenarios.

Thanks.


Please add an example config that defines per host options.

This will be added as part of the documentation effort.

I presume full documentation will be added in separate ticket. I
haven't checked if such ticket currently exists in 1.1 milestone.

This is weird. It seems that the ticket got lost somewhere. Not sure how that happened. There seem to be only a ticket for Postgresql, which we may want to turn into the documentation ticket covering the whole thing.

Your proposed changelog looks ok.


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

comment:23 Changed 18 months ago by tomek

  • Owner changed from tomek to marcin

My issue with how buildCfgOptionList works is not with option copying, but the fact that we're creating a list and iterate over it. But I agree that it's neither super important or that relevant to this particular ticket. We will fix it one day, but not today.

Thanks for the fixes and extra comments.

The code is ready to go.

comment:24 Changed 18 months ago by marcin

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

Merged with commit 49f67aaf36dab38b4fcbf59dcad97e4309903b2f

Note: See TracTickets for help on using tickets.