Opened 5 years ago

Last modified 4 years ago

#3496 reviewing enhancement

subnet4_preselect hook implementation (contributed patch)

Reported by: tomek Owned by: contributor
Priority: medium Milestone: Outstanding Tasks
Component: hooks 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 (last modified by tomek)

This is the sixth ticket created for processing patch by Shawn Lewis and David Gong.

Shawn:
Besides the available hooks (buffer4_recv, pkt4_recv, etc.), we think two additional hooks can be defined
to give the library developers more flexibility. The first one is subnet4_preselect. In the subnet4_select
hook, a subnet has already been selected for the DHCP message, which leaves the library developers little
space to custom the subnet. By introducing a subnet4_preselect hook, the library developers can choose
a specific subnet from a DHCP message based on various policies.
The second hook we would like to introduce is lease4_prerelease. When the DHCP server receives a
Release request, it first checks the existence of the lease, and the consistence of the hwaddr/client id
between the lease and the release message. It then calls the lease_release callouts and actually release
the lease. A lease4_prerelease hook should be introduced, so the library developers may conduct
customized lease query and attack detection.

Subtickets

Change History (9)

comment:1 Changed 5 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.0

As discussed on Kea meeting (2014-07-16) we not include this ticket in 0.9, due to lack of available processing time. Therefore moving to 1.0.

comment:2 Changed 5 years ago by tomek

  • Description modified (diff)
  • Status changed from new to reviewing

Here are my comments regarding new hooks implementation patch.

The subnet4_preselect hook may be useful, but I think we need to reach
a mutual understanding why it is needed. You said: "In the subnet4_select hook,
a subnet has already been selected for the DHCP message, which leaves
the library developers little space to custom the subnet.". This is not true.
The hook is really like the server asking "this is the subnet I plan to use.
Is it ok? If not, tell me which subnet to use". Note that the callout can change
server's proposal and return different value in subnet4 (it is both in *and* out
parameter). If the callout changes the value (i.e. the callout answers "no, use
this other subnet instead"), the server will honour it and will continue with
whatever subnet the callout returned.

Having said that, I think it is still useful to have subnet4_preselect call.
The reason for its existence would be performance. If you want to use
some complex criteria for selecting a subnet (like vlan must be in
range 1000 to 1024, relay's MAC address must match the one specified
in Subnet and client must not belong to cable modem class), then you
may develop some custom subnet selection routines in you callout that
would be faster than what Kea engine offers.

So if your only reason for implementing subnet4_preselect is to pick
a different subnet, it doesn't make sense. If you want to use custom
subnet selection algorithm and you are concerned about performance,
then it makes sense.

As for the lease4_prerelease, why can't you just use lease4_release?
It seems as if you want to provide an alternative storage for leases.
That's not the way to go. All leases should be stored in one location.
That's ok if you want to use Regis or other mem-only database for
storing leases, just keep all leases (context and "regular") in one place.
There are many, many places in the code that check if certain lease is
present or not and acts specifically, depending on that knowledge.

If we go with the data model I proposed in comments for 3491, then
we could just use lease4_release (it would fetch the lease, regardless
if it's regular or context).

src/bin/dhcp4/dhcp4_srv.cc|h
The changes in patch benu7.16.patch include only changes for
subnet_preselect. The changes for lease4_prerelease are missing.
You did include snippets from it in the PDF.


David or Shawn asked what would be the best way for callouts to signal
that a packet should be dropped. I created #3499 with a proposed solution.

comment:3 Changed 5 years ago by tomek

  • Version set to git

See ticket #3491 for the original patch.

comment:4 Changed 5 years ago by tomek

  • Milestone changed from Kea1.0 to Kea0.9.1

comment:5 Changed 5 years ago by tomek

  • Owner changed from UnAssigned to tomek
  • Summary changed from subnet4_preselect and lease4_preprelease hooks (contributed patch) to subnet4_preselect hook implementation (contributed patch)

As discussed with Shawn, lease4_prerelease is not needed if lease4_release would include subnet4 information. There's a ticket #3504 for that.

This limits this ticket down to subnet4_preselect. The rationale given for this hook is that it could replace normal subnet selection conducted by Kea code. The subnet selection will get more complex over time, so that rationale is valid.

Nota bene I would prefer working on improvements of the subnet selection code rather than reimplementing it in external library. But there will always be cases where external code is optimized for specific use case, so we need that call anyway.

I cut out part of the benu7.16.patch that is relevant to the
subnet4_preselect and committed it on trac3496 patch. When providing
updated patch, make sure you base it on trac3496.

The changes had trailing whitespaces. I removed them.

The code should end up at 80 lines. It is ok to occasionally get over that
limit by a couple characters if otherwise you'd need awkward wrapping, but
lines over 100 chars are definitely not acceptable.

There are no unit-tests for that new hook. See examples in
src/bin/dhcp4/tests/dhcp4_srv_unittest.cc, in particular tests that
are in the HooksDhcpv4SrvTest class.

Also, we discussed off-line and agreed that the best way for hooks
to report that a packet needs to be dropped would be to have one
unified result status. See #3499. It seems reasonable to implement
#3499 first, then modify this subnet4_preselect code to take advantage
of that new reporting mechanism.

src/bin/dhcp4/dhcp4_srv.cc

I don't like the assumption around line 256 that hooks are supposed
to set DROP argument. There are hook libraries developed by third
party that don't care about this drop. Why pushing them to implement
it? Quietly ingoring missing argument would be better. Also, after
#3499 is implemented, the code will go away anyway.

comment:6 Changed 5 years ago by contributor

  • Owner changed from tomek to contributor

comment:7 Changed 5 years ago by tomek

  • Milestone changed from Kea0.9.1 to Kea0.9.2

Moving to 0.9.2 as discussed on Kea weekly call today (2014-10-08).

If we hear back from the contributor and receive updated patch, we'll move it back to 0.9.1.

comment:8 Changed 5 years ago by hschempf

  • Milestone changed from Kea0.9.2 to DHCP Outstanding Tasks

comment:9 Changed 4 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

Note: See TracTickets for help on using tickets.