#4271 closed enhancement (complete)

Access to vendor options in v6 from classification expression

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea1.1
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: 3
Total Hours: 48 Internal?: no

Description

The ClientClassificationDesign call for implementing access to option specific fields for vendor-indenpendent vendor-specific option in v6 (e.g. option[17].enterprise-id) from the client classification expressions.

This ticket also covers accessing sub-options in vendor options.

This ticket should cover DHCPv6 options: Vendor Class (16) and Vendor-Specific Information (17) options.

Subtickets

Change History (9)

comment:1 Changed 16 months ago by tomek

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

comment:2 Changed 16 months ago by tomek

  • Add Hours to Ticket changed from 0 to 40
  • Owner tomek deleted
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 40

The code is now ready for review. The changes on trac4271 cover both DHCPv4 and DHCPv6. It was easier to do both of them at once, as both are very similar.

Proposed Changelog:

11XX.	[func]		tomek
	The vendor options (124, 125 in DHCPv4 and 16, 17 in DHCPv6) are
	now accessible from client classification.
	(Trac #4270, #4271, git tbd)

The #4270 ticket called to also address vendor class option (code 60), but that option is a simple string option. A sentence explaining how to access it has been added to the user's guide.

comment:3 Changed 16 months ago by sar

  • Owner set to sar

comment:4 follow-up: Changed 15 months ago by sar

  • Owner changed from sar to tomek

doc/guide/classify.xml

We appear to have some problems with "exist" vs "exists" in the docs.
From looking at the code I think it should always be "exists" and have
updated the documentation to match.

In 3925 the vendor options are allowed to have multiple enterprise
blocks. We should state if we do or don't support that.

In 3315 there can be multiple vendor-class options in a message,
it doesn't state either way for vendor option. We should be
clear what we do in the case of multiple options for things
like vendor.enterprise and vendor-class.enterprise.

vendor-class[4491].data has an example that is inconsistent with
the text 'true' vs data - I've updated it from true to docsis3.0.

It might be better to add an indication that the following is for
use with the vendor and vendor-class items, as we may add more wildcards
in the future.
+ <para>Asterisk (*) or 0 can be used to specify wildcard enterprise-id
+ value, i.e. it will match any enterprise-id value.</para>
Perhaps:
<para>In the vendor and vendor-class constructs Asterisk (*) or 0 can be
used to specify a wildcard enterprise-id value, i.e. it will match any
enterprise-id value.</para>

src/lib/eval/eval_messages.mes

I would remove the line "This message is moslty useful during
debugging of the client classificaiton expressions." As the
added messages are all labeled as DEBUG I don't think it is
necessary. They may have gotten it from EVAL_RESULT which
isn't clearly a debug item.

Shouldn't the messages be in alphabetical order?

src/lib/eval/parser.yy

The new clauses should be indented as the previous clauses are.

src/lib/eval/tests/token_unittest.cc

The comment in testVendorSuboption doesn't include a parameter
for repr.

In the test vendorClass4SpecificVendorData case 3 leads to
an odd result in the logged messages presumably due to the
odd nature of the v4 vendor class always having a data
block. There should be a comment mentioning this.

In the test vendorClass4DataIndex case 4 and 5 the
comments said 2 data tuples but the code used 1 & 3.
I've updated the comments but if you wanted other
numbers of tuples you should update the code (and
reset the comments).

Also in vendorClass4DataIndex there isn't a test
for incorrect vendor id with enough tuples. In
the v6 version case 4 uses 5 tuples but in the
v4 test there is only 1 tuple.

Should there be tests with multiple suboptions
in the vendor options tests?

src/lib/eval/token.cc

In the debug messages the values should be presented as they
would need to be written for a classify statement. So if
they are text they should have quotes (which appear to
be in the message) and if they are hex they should start
with 0x.

I'm unclear about why when extrating the ENTERPRISE_ID
it is being converted to network order, especially as
it doesn't seem to convert either side when comparing
the incoming vendor_id to the requested value.

src/lib/testutils/log_utils.h

Please elaborate a bit on the use of the verbose_ flag (yes the
routine spits out more output, but what is the output).

comment:5 in reply to: ↑ 4 Changed 15 months ago by tomek

  • Add Hours to Ticket changed from 40 to 5
  • Owner changed from tomek to sar
  • Total Hours changed from 40 to 45

Replying to sar:

doc/guide/classify.xml

We appear to have some problems with "exist" vs "exists" in the docs.
From looking at the code I think it should always be "exists" and have
updated the documentation to match.

Thanks for fixing them.

In 3925 the vendor options are allowed to have multiple enterprise
blocks. We should state if we do or don't support that.

In 3315 there can be multiple vendor-class options in a message,
it doesn't state either way for vendor option. We should be
clear what we do in the case of multiple options for things
like vendor.enterprise and vendor-class.enterprise.

We don't support that. The code calls pkt->getOption() that returns
a single pointer or NULL. Added a text that clarifies that. It also
suggests that if people want it, they should request it on trac.

I'm not against implementing support for multiple instances. It's just
too much work for the initial implementation in my opinion. And it would
significantly extend the number of unit-tests (many of them would have
to be repeated twice or even three times, to see if other instance
has an impact, especially if it's in the packet before or after the
option we're looking for).

vendor-class[4491].data has an example that is inconsistent with
the text 'true' vs data - I've updated it from true to docsis3.0.

Thanks.

It might be better to add an indication that the following is for
use with the vendor and vendor-class items, as we may add more wildcards
in the future.
+ <para>Asterisk (*) or 0 can be used to specify wildcard enterprise-id
+ value, i.e. it will match any enterprise-id value.</para>
Perhaps:
<para>In the vendor and vendor-class constructs Asterisk (*) or 0 can be
used to specify a wildcard enterprise-id value, i.e. it will match any
enterprise-id value.</para>

Updated as suggested.

src/lib/eval/eval_messages.mes

I would remove the line "This message is moslty useful during
debugging of the client classificaiton expressions." As the
added messages are all labeled as DEBUG I don't think it is
necessary. They may have gotten it from EVAL_RESULT which
isn't clearly a debug item.

Removed.

Shouldn't the messages be in alphabetical order?

Jeremy has a script that reorders all .msg files. I think he uses it as part of the release process, but I'm not 100% sure. Anyway, I've reordered those entries.

src/lib/eval/parser.yy

The new clauses should be indented as the previous clauses are.

Actually it should be the other way around. For some reason Francis wrote part of the parser with indentation of 10 spaces and people simply continued that way. This looks ugly on its own and furthermore causes excessive line wraps. But that's a problem that should not be fixed in this ticket, so I updated the next code as suggested. It will be cleaned up one day for the whole file.

src/lib/eval/tests/token_unittest.cc

The comment in testVendorSuboption doesn't include a parameter
for repr.

It does now.

In the test vendorClass4SpecificVendorData case 3 leads to
an odd result in the logged messages presumably due to the
odd nature of the v4 vendor class always having a data
block. There should be a comment mentioning this.

Added comment.

In the test vendorClass4DataIndex case 4 and 5 the
comments said 2 data tuples but the code used 1 & 3.
I've updated the comments

Thanks.

Also in vendorClass4DataIndex there isn't a test
for incorrect vendor id with enough tuples. In
the v6 version case 4 uses 5 tuples but in the
v4 test there is only 1 tuple.

Added.

Should there be tests with multiple suboptions
in the vendor options tests?

This would really be a test for Pkt::getOption() method. If you think there's much value in that, I can write such tests, but as you pointed out, the amount of testing here isn't exactly small.

src/lib/eval/token.cc

In the debug messages the values should be presented as they
would need to be written for a classify statement. So if
they are text they should have quotes (which appear to
be in the message) and if they are hex they should start
with 0x.

Excellent suggestion. Updated the code.

I'm unclear about why when extrating the ENTERPRISE_ID
it is being converted to network order, especially as
it doesn't seem to convert either side when comparing
the incoming vendor_id to the requested value.

The support for integer values is a bit sketchy, I admit. There's upcoming ticket (#4483) that hopes to address that. The reason to convert to network order was to be consistent with existing code. This is important, because we need to store integers somehow. It doesn't really matter how we do it as long as we do it exactly the same in every place. Take a look at the TokenPkt4::evaluate as example. What Francis did there for HLEN and HTYPE manually with pushing zeros followed by the actual value was explicit conversion to network order. It's also easier for us, because network order is more natural, because we can use things like 0x00000064 as they are, without any conversions.

There's also the matter of portability. The classification expressions should work the same way on both little and big endians.

src/lib/testutils/log_utils.h

Please elaborate a bit on the use of the verbose_ flag (yes the
routine spits out more output, but what is the output).

Added extra comments. Hopefully they're sufficient.

Ok, that's it. If my responses address all your comments, that's great. If not, let's continue. If you think that supporting multiple vendor option instances is important, I think this functionality should be pushed off to a separate ticket, because this one is large enough already.

Thanks for your review.

comment:6 Changed 15 months ago by fdupont

I am fixing lexer and parser files (mainly indentation but there are wrong comments and a missing ; (optional but required by guidelines)).

comment:7 Changed 15 months ago by fdupont

Done. Please pull. BTW there should be no need to regen lexer/parser files (but if you do this please try the last flex and bison I installed on docs in my home dir / bin).

comment:8 follow-up: Changed 15 months ago by sar

  • Owner changed from sar to tomek

The changes all look okay

they can be merged

comment:9 in reply to: ↑ 8 Changed 15 months ago by tomek

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

Replying to sar:

The changes all look okay

they can be merged

Thanks a lot for your review and comments.

Code merged. Closing ticket.

Note: See TracTickets for help on using tickets.