#4269 closed enhancement (fixed)

Extract constant length fields from DHCPv6 packet

Reported by: tomek Owned by: sar
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: 2
Total Hours: 14 Internal?: no

Description

The ClientClassificationDesign calls for implementing methods to extract fixed length fields from the DHCPv6 packet. In particular, the following fields must be accessible: msg-type (for all packets), link-addr (for relay-forw and relay-reply), peer-addr (for relay-forw and relay-reply).

Subtickets

Change History (10)

comment:1 Changed 21 months ago by sar

As the link-addd and peer-addr fields are per relay I'm moving them to ticket 4265 so they can be handled in the same style as we use for per relay options. When that ticket is done they may be moved off to a new ticket depending on how much extra work they are.

comment:2 Changed 21 months ago by sar

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

comment:3 Changed 20 months ago by sar

  • Owner changed from sar to unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 8

Ready for review

Note that some of the text for classify.xml is commented out. 4268 adds another column to the table and the commented out line will fill it in.

comment:4 Changed 20 months ago by fdupont

  • Owner changed from unassigned to fdupont

comment:5 Changed 20 months ago by fdupont

  • Owner changed from fdupont to sar

I have a concern about the returned values: I believe(d) we agreed in #4268 to return the binary values of fields, not their representations as integers, i.e., pkt6.msgtype on a SOLICIT is 0x01, not '1'.

comment:6 Changed 20 months ago by sar

  • Owner changed from sar to unassigned
  • Total Hours changed from 8 to 10

I've updated the evaluation code and the tests to put a 4 byte integer on to the value stack for the message type and transaction id fields per the recent decision.

comment:7 Changed 20 months ago by tmark

  • Owner changed from unassigned to tmark

comment:8 follow-up: Changed 20 months ago by tmark

  • Add Hours to Ticket changed from 0 to 2
  • Owner changed from tmark to sar
  • Total Hours changed from 10 to 12

Seems like pretty straight forward stuff. A few things:

src/lib/eval/parser.yy

Pre coding guidelines, the local variable, pkt6field, should be pkt6_field.

+ | PKT6 "." pkt6_field
+ {
+ TokenPtr? pkt6field(new TokenPkt6($3));
+ ctx.expression.push_back(pkt6field);
+ }


src/lib/eval/tests/context_unittest.cc

Probably better not to use "type" for a parameter name and in keeping with
similiar methods, I would suggest expected_type.

+ / @brief checks if the given token is Pkt6 of specified type
+
/ @param token token to be checked
+ / @param type expected type of the Pkt6 field
+ void checkTokenPkt6(const TokenPtr?& token, TokenPkt6::FieldType? type) {
+ ASSERT_TRUE(token);


General:

Although it appears that none of the tests in context_unittest.cc do actually verify that expression evaluations are correct for a given packet, it is at least worth a mention in these tests, that they are only verifying that it parses correctly, not whether the extraction and comparison are checked. For instance, we know that this expression would never be true unless the message type value happened to be 0x36. My first reaction to "Hey, this test will fail?!"

+ Tests whether transaction id field in DHCPv6 can be accessed.
+TEST_F(EvalContextTest?, pkt6FieldTransid) {
+ testPkt6Field("pkt6.transid == '1'", TokenPkt6::TRANSID, 3);
+

It just seems like somewhere we should have the expression tested against an actual packet.


Obviously, 4268 has to be changed to conform to what you're doing here in terms of how the values are being extracted into 4 byte binary values stored as strings, as we can't very well have them being done differenty.


comment:9 Changed 20 months ago by tmark

I don't need to this again, btw.

comment:10 in reply to: ↑ 8 Changed 20 months ago by sar

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 12 to 14

Replying to tmark:

Seems like pretty straight forward stuff. A few things:

src/lib/eval/parser.yy

Pre coding guidelines, the local variable, pkt6field, should be pkt6_field.

Done


src/lib/eval/tests/context_unittest.cc

Probably better not to use "type" for a parameter name and in keeping with
similiar methods, I would suggest expected_type.

I changed it to exp_type to match other code in the same area.


General:

Although it appears that none of the tests in context_unittest.cc do actually verify that expression evaluations are correct for a given packet, it is at least worth a mention in these tests, that they are only verifying that it parses correctly, not whether the extraction and comparison are checked. For instance, we know that this expression would never be true unless the message type value happened to be 0x36. My first reaction to "Hey, this test will fail?!"

I'll create a new ticket for this type of testing.

I also made some typo level changes and needed to fix up some conflicts.

Note: See TracTickets for help on using tickets.