Opened 6 years ago

Closed 6 years ago

#3203 closed enhancement (complete)

Minimalistic client classification is needed for docsis (classification, part 1)

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea0.8
Component: dhcp Version:
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: 3
Total Hours: 8 Internal?: no

Description (last modified by tomek)

We need a (minimalistic) client classification, so we can handle client belonging to different classes differently.

The support for v4 is required, for v6 it is nice to have (at least that's our understanding for now).

The 2 classes we need for now are docsis3.0 and eRouter1.0.

Follow up tasks: #3274 and #3275.

Subtickets

Change History (12)

comment:1 Changed 6 years ago by tomek

Also hops field in outgoing packets must be set to 0.

comment:2 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20131016 to Sprint-DHCP-20131204

comment:3 Changed 6 years ago by tomek

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

comment:4 Changed 6 years ago by tomek

ISC-DHCP allows definitions like the following:

class pxe {
  match if substring(option vendor-class-identifier, 0, 9) = "PXEClient";
}

Kea will not allow such things in any foreseeable future. The plan for now is to support so called minimalistic client classification. It is called minimalistic, because (compared to classes in ISC-DHCP) there is no way for the user to define its own classes or define what does it mean "a class". Kea will merely use content of user-class option.

Here's the plan for adding minimalistic client classification.

  1. add generic class support in v4/v6 packets. (this ticket, #3203)
  2. add client specific processing for v4 packets. This will include docsis specific processing that will be later extracted into separate library. (this ticket, #3203)
  3. use client classes in subnet selection (white list, black list for subnets) (ticket #3274)
  4. move docsis specific processing to external hook library. (ticket #3275)

Tasks 1-2 will be completed in this ticket. Tasks 3 and will be done in separate tickets.

Last edited 6 years ago by tomek (previous) (diff)

comment:5 Changed 6 years ago by tomek

  • Component changed from dhcp6 to dhcp
  • Description modified (diff)
  • Summary changed from Minimalistic client classification is needed for docsis to Minimalistic client classification is needed for docsis, pt.1

comment:6 Changed 6 years ago by tomek

  • Summary changed from Minimalistic client classification is needed for docsis, pt.1 to Minimalistic client classification is needed for docsis (classification, part 1)

comment:7 Changed 6 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from assigned to reviewing

Please review. Keep in mind that this is a first ticket in a planned series of at least 3 tickets. Some of the code written (specific to cable modems) should be moved to an external library, but the classifier itself is a generic mechanism that can be used in many scenarios, not necessarily cable-related. Ultimately, we may consider developing dedicated hook points for client classification.

Proposed ChangeLog? entry

719.	[func]		tomek
	b10-dhcp4, b10-dhcp6: Support for simplified client classification
        added. Incoming packets are now assigned to a client class based on
        the content of user class (DHCPv4) or vendor class (DHCPv6). Two classes
        (docsis3.0 and eRouter1.0) have class specific behavior in b10-dhcp4.
        See DHCPv4 Client Classification and DHCPv6 Client Classification in
        BIND10 Developer's Guide for details. This is a first ticket in a series
        of planned at least three tickets.
	(Trac #3203, git ABCD)

comment:8 Changed 6 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:9 follow-up: Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 0 to 4
  • Owner changed from tmark to tomek
  • Total Hours changed from 0 to 4

General:

Changes passed cppcheck and unit tests passed valgrind test.

  1. You are using hard-coded strings for class names. These should be defined as constants somewhere.
  1. Pkt4::classes_, inClass(), and addClass() are all identical to Pkt6. They

should be moved to the base class.


dhcp/Pkt4.h

Commentary for ::addClass is conceptually backwards. It describes adding
packets to classes when it fact it addes classes to a packet.
(Same in dhcp/Pkt6.h)


dhcp/tests/pkt4_unittests.cc

For the sake of thoroughess the test should verify that inClass("foo") returns
true after adding it. This ensures that no class value specific logic exists
that would interfere.
(Same in dhcp/tests/pk6_unittests.cc)


src/bin/dhcp4/dhcp4_srv.cc:

Dhcpv4Srv::classifyPacket()

There a few issues with the if-block beginning at line 1808.

  1. Why do you have this block at all? Regardless of the value in vendor_class

the net effect is the same:

  1. you add the value to the string "classes"
  2. you call pkt-addClass() passing in the value

Seems like you could just replace the whole block and subsequent test for logging with this:

    if (!classes.empty()) {
        classes += vendor_class->getValue();
        pkt->addClass(vendor_class->getValue());
        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_CLASS_ASSIGNED)
            .arg(classes);
    }
  1. If the block is useful, then I see the following:

2.1 Add todo commentary as to why the block should remain.

2.2 In the case of the two named append the class name plus a space to the string classes, but in the case where you simply add the value from the packet option there will be no trailing space. Shouldn't you do this?

        classes += vendor_class->getValue() + " ";

2.3 line 1815 spacing and bracket placement are wrong ;)

    }else
    {

should be:

    } else {

src/bin/dhcp4/dhcp4_srv.cc:

Dhcpv4Srv::classSpecificProcessing()

This method is called before the pkt_send call out right?
User_chk could overwrite the boot file option as it stands the two would not
necessarily be compatible. Really this is food for thought more than anything.


src/lib/dhcp/tests/libdhcp++_unittest.cc

  1. line 1142, this should be an ASSERT_NO_THROW. If this throws the remainder of test is moot.
  1. If all V6 vendor options are constructed of 3 fields: vendor id, data len,

and string at offsets 0, 1, and 2 respectively; shouldn't we at least create
constants for these index values? This test uses hard-coded numbers for
indexes and in Dhcp6Srv::classifyPacket() there are several calls to vclass->readString(2). All nice examples of "magic numbers".

One could even make a case for a derivation of OptionCustom called
VendorCustomOption which provided accessors for id, datalen, and data.


Regarding ChangeLog entry, I would recommend changing this:

Incoming packets are now assigned to a client class based on
the content of user class (DHCPv4) or vendor class (DHCPv6).

to this:

Incoming packets are now assigned to a client class based on 
the content of the packet's user class option (DHCPv4) or vendor
class option (DHCPv6).
Last edited 6 years ago by tmark (previous) (diff)

comment:10 in reply to: ↑ 9 Changed 6 years ago by tomek

  • Owner changed from tomek to tmark

Replying to tmark:

General:

Changes passed cppcheck and unit tests passed valgrind test.

  1. You are using hard-coded strings for class names. These should be defined as constants somewhere.

Added constants in docsis3_option_defs.h and libdhcp++.cc.

  1. Pkt4::classes_, inClass(), and addClass() are all identical to Pkt6. They

should be moved to the base class.

That's an excellent suggestion. The only slight problem is that they do not have
a base class. Added ticket #3296.


dhcp/Pkt4.h

Commentary for ::addClass is conceptually backwards. It describes adding
packets to classes when it fact it addes classes to a packet.
(Same in dhcp/Pkt6.h)

Added extra text. Let me know if it is satisfactory now.


dhcp/tests/pkt4_unittests.cc

For the sake of thoroughess the test should verify that inClass("foo") returns
true after adding it. This ensures that no class value specific logic exists
that would interfere.
(Same in dhcp/tests/pk6_unittests.cc)

Added check.



src/bin/dhcp4/dhcp4_srv.cc:

Dhcpv4Srv::classifyPacket()

There a few issues with the if-block beginning at line 1808.

  1. Why do you have this block at all? Regardless of the value in vendor_class

the net effect is the same:

Added explanation. In essence, this is simple now, but John B. talked about fancier
stuff there (this is a modem if and only if relay's subscriber-id option matches
client's MAC address). Also, we have one packet capture where the class name is
followed by a space.

Added explanation for that.

  1. If the block is useful, then I see the following:

2.1 Add todo commentary as to why the block should remain.

Added.

2.2 In the case of the two named append the class name plus a space to the string classes, but in the case where you simply add the value from the packet option there will be no trailing space. Shouldn't you do this?

This is for logging purposes only, so I don't really care if there's extra trailing space.

2.3 line 1815 spacing and bracket placement are wrong ;)

Fixed.


src/bin/dhcp4/dhcp4_srv.cc:

Dhcpv4Srv::classSpecificProcessing()

This method is called before the pkt_send call out right?
User_chk could overwrite the boot file option as it stands the two would not
necessarily be compatible. Really this is food for thought more than anything.

Yes. In fact, I'm considering whether we should make a separate hook point for this
(or two - one for doing classification and another one for acting based on the classes)
or not.

src/lib/dhcp/tests/libdhcp++_unittest.cc

  1. line 1142, this should be an ASSERT_NO_THROW. If this throws the remainder of test is moot.

Updated.

  1. If all V6 vendor options are constructed of 3 fields: vendor id, data len,

and string at offsets 0, 1, and 2 respectively; shouldn't we at least create
constants for these index values? This test uses hard-coded numbers for
indexes and in Dhcp6Srv::classifyPacket() there are several calls to vclass->readString(2). All nice examples of "magic numbers".

It's all magic. Ok, for less magically inclined I added constants in the option_vendor
class. There's no good place to add them (perhaps in std_option_defs.h, but that would
require including that header everywhere), but I think option_vendor class is somewhat
acceptable.

One could even make a case for a derivation of OptionCustom called
VendorCustomOption which provided accessors for id, datalen, and data.

Not really. The whole idea for OptionCustom? was to take care of all such odd
options.


Regarding ChangeLog entry, I would recommend changing this:

Incoming packets are now assigned to a client class based on
the content of user class (DHCPv4) or vendor class (DHCPv6).

to this:

Incoming packets are now assigned to a client class based on 
the content of the packet's user class option (DHCPv4) or vendor
class option (DHCPv6).

Agree. I will use the changelog as suggested.

Thanks for the review.

comment:11 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 4 to 1
  • Owner changed from tmark to tomek
  • Total Hours changed from 4 to 5

Your changes and responses are acceptable. Plus merge in your work

comment:12 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 1 to 3
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 5 to 8

Thanks for the review. Code merged and pushed. Closing ticket.

Note: See TracTickets for help on using tickets.