Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4097 closed enhancement (complete)

Evaluate v4 packets to classify the packet

Reported by: sar Owned by: fdupont
Priority: high Milestone: Kea1.0-beta
Component: dhcp Version: git
Keywords: classification 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: 24 Internal?: no

Description (last modified by sar)

This task is to evaluate a packet using the code from 4094 and the class information from the classes in 4095 and any required information from the packet to determine which classes the packet belongs to and from that which options should be use with the packet.

  • walk list of candidate classes
    • evaluate each class using 4094 and 4095
    • tag packet with included classes (I think this will be necessary later
    • add options from included classes to packet

Subtickets

Change History (24)

comment:1 Changed 4 years ago by fdupont

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

comment:2 Changed 4 years ago by fdupont

Adding global classes in dhcpsrc/srv_config.h (common code with #4098).

comment:3 Changed 4 years ago by fdupont

BTW this ticket requires #4094 which is still under review at the time I am writing this...

comment:4 Changed 4 years ago by fdupont

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

Done. Note I added a temporary common code in srv_config (SrvConfig? class) where should come the configuration API.
Ready for review.

comment:5 Changed 4 years ago by sar

  • Description modified (diff)
  • Owner changed from UnAssigned to sar

comment:6 Changed 4 years ago by fdupont

#4096 is almost ready, #4088 merged so I'll rebase #4097 on them. Ideas should stay anyway. BTW should I add a specific logger (I use the option logger because it was the one in the code I copied). Another point: I'll move the docsis stuff before the class/eval (unconditionally and simplest first, this matches possible future dependency too).

comment:7 follow-up: Changed 4 years ago by sar

  • Owner changed from sar to fdupont

I fixed a minor typo, please do a pull.

src/bin/dhcp4/dhcp4_srv.h

The description of classifyPacket() needs to be updated.

src/bin/dhcp4/dhcp4_srv.cc

Some comments would be nice.

classifyPacket()

Is it legal for the class map to not have a client class definition (it->second to be NULL)? Should this be logged?

Is it legal for a class to have no expression (getMatchExpr() returns NULL)? This seems more valid than no client class but perhaps it should also be logged?

Can you swap lines 2281 and 2281 to make the final else clause in the vendor checks the same order as the if and else if clauses?

classSpecificProcessing()

I find iterating over all classes and seeing which, if any, are in the list of classes for this packet to be somewhat inefficient but I suppose it won't make any real difference.

The additional check at 2390 would appear to be an issue. The requirements call for the options to be
global -> class global -> subnet -> class subnet
where we haven't done class subnet yet. I think the current code will prefer globals over class globals. Perhaps we should switch to class global over subnet?

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

There should be tests for:

  • negative class (I think with the necessary options with incorrect values (for example host name == bar) and without a required option (so no host name at all)
  • overriding a global option (see above on the ordering of options)
  • not over riding a subnet option (see above on the ordering of options)

Running the tests

I'm currently unable to run the tests as the evaluate code isn't in this branch.

comment:8 in reply to: ↑ 7 Changed 4 years ago by fdupont

Replying to sar:
=> replying to questions.

classifyPacket()

Is it legal for the class map to not have a client class definition (it->second to be NULL)?

=> no, it is clearly illegal but we can discussed about the proper action (skip as I proposed, log, or raise an exception).

Should this be logged?

=> if it is logged it will be at each incoming packet. If it is not it wont' be noticed. Perhaps it should be logged and repaired?

Is it legal for a class to have no expression (getMatchExpr() returns NULL)?

=> yes, the match expression is optional: this means a packet is put in a class by another way, e.g., a hook.

This seems more valid than no client class but perhaps it should also be logged?

=> IMHO this case should only be skipped.

Can you swap lines 2281 and 2281 to make the final else clause in the vendor checks the same order as the if and else if clauses?

=> I believe I understood what you want: commit be614d0..2446577

classSpecificProcessing()

I find iterating over all classes and seeing which, if any, are in the list of classes for this packet to be somewhat inefficient but I suppose it won't make any real difference.

=> it is inefficient but I didn't find a clear better way.

The additional check at 2390 would appear to be an issue. The requirements call for the options to be
global -> class global -> subnet -> class subnet
where we haven't done class subnet yet. I think the current code will prefer globals over class globals. Perhaps we should switch to class global over subnet?

=> this depends when classSpecificProcessing() is called (currently it is called last). BTW it is the right time to discuss this as we have the same issue for DHCPv6 aka #4098 where this method doesn't yet exist.

Running the tests

I'm currently unable to run the tests as the evaluate code isn't in this branch.

=> it is merged in the master so you can patch the diff (or simply wait for the rebase).

comment:9 Changed 4 years ago by fdupont

Rebased and updated. There are still comments to address but the code already works so one can stack something over it.
New branch is trac4097a.

comment:10 Changed 4 years ago by fdupont

Currently there is no way to know if an option is global or per subnet as subnet->getCfgOption() returns both. I open a ticket to address this when we'll implement subnet classes.
Cf #4205

comment:11 Changed 4 years ago by fdupont

  • Owner changed from fdupont to sar

I believed I addressed all comments at the exception of the add option priority/order which is beyond easy repair.
When it will be merged I port the same code to DHCPv6 in #4098.

comment:12 Changed 4 years ago by tmark

We do not use gotos and labels in Kea. It would be much more modular and in keeping with C++ to simply put the whole vendor option/class logic into it's own method:

void Dhcp4::classifyByVendor(Pkt4ptr packet) {

    // Built-in vendor class processing
    boost::shared_ptr<OptionString> vendor_class =
        boost::dynamic_pointer_cast<OptionString>(pkt->getOption(DHO_VENDOR_CLASS_IDENTIFIER));

    if (vendor_class) {
        // DOCSIS specific section
            :
        if (vendor_class->getValue().find(DOCSIS3_CLASS_MODEM) != std::string::n
pos) {
            :
    }
}

and then replace all of this:

    if (!vendor_class) {
        goto vendor_class_done;
    }

    // DOCSIS specific section
         :

vendor_class_done:

    // Run match expressions

with a call to the new method:

     classifyByVendor(pkt);

    // Run match expressions

comment:13 Changed 4 years ago by fdupont

  • Owner changed from sar to UnAssigned

I adopted your idea (even IMHO the goto question is more religious than technical) because it will make writing of the subnet classify routine easier.
Does it needs a ChangeLog? If yes it should be common with #4098.

comment:14 follow-up: Changed 4 years ago by sar

  • Owner changed from UnAssigned to fdupont

I tided up some comments please do a pull before continuing.

We are still discussing keeping the vendor specific code. At this point I think the proper path is to leave it in and we can create a ticket to remove it if we make that decision after this ticket is merged.

As this is now a user visible change I think it does need a change log entry.

src/bin/dhcp4/dhcp4_srv.cc

classSpecificProcessing()

Line 2384 checks to see if we found a class for the given class name. We should emit a log message. Either we previously matched a class and managed to lose it which is weird or some hook added a class name and it doesn't exist, possibly because of inconsistent naming. In both cases some sort of log message, possibly at debug level, would be useful.

Line 2398 adding the option - as discussed in jabber I think we want the option to only be added if it is in the PRL or a required option.

comment:15 in reply to: ↑ 14 Changed 4 years ago by fdupont

  • Owner changed from fdupont to sar

Replying to sar:

I tided up some comments please do a pull before continuing.

=> unfortunately I addressed the last point raised on Jabber before reading this. I believe your comment fixes were correctly merged by git.

We are still discussing keeping the vendor specific code. At this point I think the proper path is to leave it in and we can create a ticket to remove it if we make that decision after this ticket is merged.

=> the vendor specific classify code is its own private method so this should be easy.

As this is now a user visible change I think it does need a change log entry.

=> what about "DHCP servers classify incoming packets evaluating match expressions and add requested options when configured in classes".

src/bin/dhcp4/dhcp4_srv.cc

classSpecificProcessing()

Line 2384 checks to see if we found a class for the given class name.

=> I am not sure what line it was: I think it is when findClass() returns null, isn't it?

=> We should emit a log message. Either we previously matched a class and managed to lose it which is weird or some hook added a class name and it doesn't exist, possibly because of inconsistent naming. In both cases some sort of log message, possibly at debug level, would be useful.

=> IMHO the second case is not a problem (BTW it is the way a packet was classified before match expressions so at least legacy hooks will do exactly this: put a packet in a class which is not in the configured dictionary). But a log debug doesn't

Line 2398 adding the option - as discussed in jabber I think we want the option to only be added if it is in the PRL or a required option.

=> done. Do we need requested vendor options too? (I believe we want them too for at least 1.1)

comment:16 Changed 4 years ago by fdupont

Getting it back to apply new ideas, for instance move the option addition code to appendRequestedOptions. This could fix #4205 too.

comment:17 Changed 4 years ago by fdupont

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

comment:18 Changed 4 years ago by fdupont

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

Done. The code builds a list of configured options (CfgOptionList) and iterates on it in append* methods.
For the ChangeLog entry I propose: Classify match expressions are evaluated on incoming packets and requested options are appended when configured by the subnet, a class or globally.
Ready for final review?

comment:19 Changed 4 years ago by fdupont

Should not log a not configured class when the class name begins by VENDOR_CLASS_PREFIX.

comment:20 Changed 4 years ago by sar

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

comment:21 follow-up: Changed 4 years ago by sar

  • Owner changed from sar to fdupont

Once the following are addressed this can be merged, the Change log entry is sufficient.

src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.c

In appendRequestedOptions() we immediately return if there is no subnet. Shouldn't we do the same for buildCfgOptionList()?

src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h

Shouldn't there be a mention that the second getCfgOptionList (line 120) is for const use?

The note about the configured option list around line 440 should also be included in the description of the cfg_option_list around line 156

src/bin/dhcp4/tests/config_parser_unittest.cc

In the v6 version of optionDataDefaultsGlobal() there is a check to see if options that weren't configured are returned (the last part of the routine). Should a similar check be added in the v4 version?

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

There is a lot of duplicated code between subnetClassPriority() and classGlobalPriority(). This should probably be combined into a test routine. In addition we should be complete and add subnetGlobalPriority() to verify that subnet is chosen before global.

src/bin/dhcp6/dhcp6_messages.mes

Do we not have a label for the packet that we can use in this message to be similar to the dhcp4 version?

src/bin/dhcp6/dhcp6_srv.cc

The v4 classifyPacket() uses options4_logger for debug statements while v6 classifyPacket() is using dhcp6_logger. They should be using similar loggers.

src/bin/dhcp6/tests/dhcp6_srv_unittests.cc

In the matchClassification test query2 and query3 are using DHCPDISCOVER instead of DHCPV6_SOLICIT

We should be complete and add subnetGlobalPriority() to verify that subnet is chosen before global.

comment:22 in reply to: ↑ 21 Changed 4 years ago by fdupont

Replying to sar:

Once the following are addressed this can be merged, the Change log entry is sufficient.

src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.c

In appendRequestedOptions() we immediately return if there is no subnet. Shouldn't we do the same for buildCfgOptionList()?

=> I agree: as all methods using the configured option list return soon when there is no subnet this should be added to buildCfgOptionList(). Done 95fd371..d40a3b4.

src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h

Shouldn't there be a mention that the second getCfgOptionList (line 120) is for const use?

=> I added a src_config.h like comment. Done in d40a3b4..c6bced0.

The note about the configured option list around line 440 should also be included in the description of the cfg_option_list around line 156

=> done in c6bced0..525a66e

src/bin/dhcp4/tests/config_parser_unittest.cc

In the v6 version of optionDataDefaultsGlobal() there is a check to see if options that weren't configured are returned (the last part of the routine). Should a similar check be added in the v4 version?

=> Done in 525a66e..52887c0.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

There is a lot of duplicated code between subnetClassPriority() and classGlobalPriority(). This should probably be combined into a test routine.

=> I disagree because it is new code and it is better to get accurate errors if a test fails (with a test routine the error message will refer to a line in its body). I believe this idea should be postponed to 1.1.

In addition we should be complete and add subnetGlobalPriority() to verify that subnet is chosen before global.

=> Done in 52887c0..e11158f.

src/bin/dhcp6/dhcp6_messages.mes

Do we not have a label for the packet that we can use in this message to be similar to the dhcp4 version?

=> Aligned DHCPv6 code on the DHCPv4 one in e11158f..8aa346b.

src/bin/dhcp6/dhcp6_srv.cc

The v4 classifyPacket() uses options4_logger for debug statements while v6 classifyPacket() is using dhcp6_logger. They should be using similar loggers.

=> Aligned to dhcp?_logger in 8aa346b..2de93ab.

src/bin/dhcp6/tests/dhcp6_srv_unittests.cc

In the matchClassification test query2 and query3 are using DHCPDISCOVER instead of DHCPV6_SOLICIT

=> oops. Fixed.

We should be complete and add subnetGlobalPriority() to verify that subnet is chosen before global.

=> done in 2de93ab..0b33e52.

No remaining points, I'll merge.

comment:23 Changed 4 years ago by fdupont

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

comment:24 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.