Opened 4 months ago

Closed 6 days ago

#5549 closed enhancement (complete)

implement known/unknown class-like feature

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.4-final
Component: classification 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

Cf #5425. The last idea is to add a third (so depends on #5374) classification point between subnet selection and pool selection. Note this implies that for all messages hosts are looked for.

Subtickets

Change History (13)

comment:1 Changed 4 months ago by tomek

  • Milestone changed from Kea-proposed to Kea1.4
  • Priority changed from medium to low

As discussed on today's call.

comment:2 Changed 3 months ago by fdupont

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

Previous code is included in #5374 and should be removed. New code is based on the KNOWN builtin class and already described in #5374 so the question is more where to put the border between #5374 and #5549. I propose to not include in #5374 specific code but to leave shared code and shared comments so 5549 addition will be both specific and small.

comment:3 Changed 7 weeks ago by tomek

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

As discussed on 2018-04-26 call, moving low and some med priority tickets to 1.4-final.

comment:4 Changed 4 weeks ago by fdupont

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

Code updated in trac5540a (don't forget the 'a'). Ported doc and tests. Ready for review (IMHO there are opportunities for more tests).

comment:5 Changed 4 weeks ago by fdupont

  • Owner changed from UnAssigned to fdupont

Take it to add UNKNOWN class.

comment:6 Changed 4 weeks ago by fdupont

  • Owner changed from fdupont to UnAssigned

Done, ready again for review.

comment:7 Changed 12 days ago by tomek

  • Owner changed from UnAssigned to tomek

comment:8 Changed 12 days ago by tomek

  • Owner changed from tomek to fdupont
  • Priority changed from low to medium

The last commit on trac5549a is f079ed3f90f17d22f2d8c571a234ad610d0815b3, which is earlier than comment 5. Francis, did you forget to push or am I trying to review a wrong branch?

Also this ticket is more important than low, adjusting priority.

comment:9 Changed 12 days ago by fdupont

Forgot commit and merge. Fixed now. Note there should be some conflicts with the HA_ classes but I am prepared to solve them at merge time...

comment:10 Changed 8 days ago by fdupont

  • Owner changed from fdupont to tomek

comment:11 follow-up: Changed 7 days ago by tomek

  • Owner changed from tomek to fdupont

I have reviewed changes on trac5549a.

doc/guide/*.xml
I have updated the documentation a bit. Hopefully it is clearer
now. Please pull and review. One major missing point here was that
the docs must make it clear the known/unknown class is added when
host reservations are inspected. This happens after subnet was
selected, so you can't pick a subnet or shared network based
on this.

dhcp{4,6}/advanced.json
The changes should be moved or copied to classify.json.

dhcp4_srv.h
The comment for evaluateClasses doesn't explain what the second
part of the classification is and why it is needed. I think the
code is correct, but the description should be extended.

dhcp4_srv.cc, dhcp6_srv.cc
There should be a log a debug message that packet was added to
either KNOWN or UNKNOWN class.

dhcp4_srv_unittest.cc
The test you wrote clientPoolClassifyKnown actually tests only
unknown class. Either extend the test to add a matching reservation
and do second case or write separate test. In any case, make sure
the description (comment above test code) matches what the
test actually does.

dhcp6/classify_unittests.cc
This subnet definition is funny: 2001:db8:2::/40. That /40 ignores
that 2. While technically correct, it's equal to 2001:db8::/40 and
should be written that way.

client_class_def_parser.cc
Please don't move this block. It was further down the code:

    // Let's try to parse the only-if-required flag
    bool required = false;
    if (class_def_cfg->contains("only-if-required")) {
        required = getBoolean(class_def_cfg, "only-if-required");
    }

Thanks to Wlodek for testing this set of changes. I confirm Wlodek's
concern about the KNOWN/UNKNOWN classes not being usable to select
a network or subnet. This is impossible, because reservations are
defined for specific subnets, so subnet has to be selected first.
Updated the docs to be clearer about that.


Compiled on ubuntu 17.10 x64. unit-tests pass. Also tested this
with dibbler (v6) and dhclient (v4). This change indeed is working
for pools and obviously doesn't for subnets. THat's ok, as it is
now clearly documented.


Thanks to Wlodek for testing this and to Francis for writing the code.

Francis, please pull and review. Then address my comments above
and then you can merge it, unless you object to my comments. If you
do, let's continue discussion, but please keep in mind that the
code freeze is tomorrow.

comment:12 in reply to: ↑ 11 Changed 6 days ago by fdupont

Replying to tomek:

I have reviewed changes on trac5549a.

doc/guide/*.xml

=> pulled your changes

dhcp{4,6}/advanced.json
The changes should be moved or copied to classify.json.

=> moved to classify2.json

dhcp4_srv.h

=> added description (and in dhcp6_srv.h too).

dhcp4_srv.cc, dhcp6_srv.cc

=> added logging.

dhcp4_srv_unittest.cc

=> added clientPoolClassifyUnknown

dhcp6/classify_unittests.cc

=> fixed cut & paste.

client_class_def_parser.cc
Please don't move this block.

=> i don't understand why but I restore the block position.

Francis, please pull and review. Then address my comments above
and then you can merge it, unless you object to my comments.

=> no, I am checking against last typos and if god I am merging it.

I propose for the ChangeLog entry:
{{{Added KNOWN and UNKNOWN built-in client classes: after host lookup if
a matching host entry is found the incoming packet is added to the KNOWN
class, if none is found to the UNKNOWN class. Then expressions depending
directly or indirectly on these classes are evaluated. Note if these classes may
be used to select a pool they may not to select a subnet.}}}

comment:13 Changed 6 days ago by fdupont

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

Merged. Closing.

Note: See TracTickets for help on using tickets.