#4264 closed enhancement (complete)

Make options inserted by a v4 relay agent available for the classification

Reported by: tomek Owned by: fdupont
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: 0
Total Hours: 0 Internal?: no

Description

One of the requirements of the design is to provide an ability to access options inserted by a relay.

This ticket covers accessing options inserted by a relay agent, i.e. accessing sub-options of option 82 (RAI).

Subtickets

Change History (11)

comment:1 Changed 22 months ago by tomek

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

comment:2 Changed 22 months ago by tomek

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

The code is now ready for review. Proposed ChangeLog?:

10XX.	[func]		tomek
	Client classification in DHCPv4 has been enhanced. It is now
	possible to access relay sub-options using the expression
	relay[123].hex.
	(Trac #4264, git tbd)

comment:3 Changed 22 months ago by sar

  • Owner changed from Unassigned to sar

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

  • Owner changed from sar to tomek

I have fixed some typos and updated some copyrights please do a pull before continuing.

src/lib/eval/parser.yy

The error string "relay support for v6 is not implemented" will need to be updated when relay support for v6 is implemented. Would it be simpler to change it to the correct error now? Maybe something like "use <command> for relay support for v6"? In either case 4265 should have an update to describe what it needs to do - either use the correct command name or update the error string when it is implemented.

src/lib/eval/tests/token_unittest.cc

There should probably be a test to verify that only the RAI is searched for the requested sub option.

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

  • Owner changed from tomek to sar

Replying to sar:

I have fixed some typos and updated some copyrights please do a pull before continuing.

Thanks.

src/lib/eval/parser.yy

The error string "relay support for v6 is not implemented" will need to be updated when relay support for v6 is implemented. Would it be simpler to change it to the correct error now? Maybe something like "use <command> for relay support for v6"? In either case 4265 should have an update to describe what it needs to do - either use the correct command name or update the error string when it is implemented.

I thought about this a little bit more and now think that the way to go is to explicitly reference whether you talk about v4 relay or v6 relay. So now we have "relay4" and "relay6" clauses. There are a couple reasons. First, relays work fundamentally different, so there wouldn't be much code reuse. Second, the access patterns are different. For v4, you specify relay4[123] to get option 123. But for v6, there's another parameter needed that describes which relay it is. It may be an optional parameter, but that's a parameter that the option extraction code needs to implement. I think it will be something like relay6[X,Y], where X is option code and Y is optional parameter that can contain any value from 0 ... 31, any, or any value specified in Pkt6::RelaySearchOrder?: try the first relay (closest to the client) only, try the last relay (closest to the server) only, search from client side, search from server side. This complexity warrants a dedicated class.

src/lib/eval/tests/token_unittest.cc

There should probably be a test to verify that only the RAI is searched for the requested sub option.

Good point. Added TokenTest?.relayRAIOnly

comment:6 follow-up: Changed 22 months ago by sar

  • Owner changed from sar to tomek

src/lib/eval/parser.yy

Having two different commands for the v4 and v6 relay seems fine to me.

src/lib/eval/tests/token_unittest.cc

The new test creates opt70 but doesn't actually add it to the packet does it?

comment:7 in reply to: ↑ 6 Changed 22 months ago by tomek

Replying to sar:

src/lib/eval/tests/token_unittest.cc

The new test creates opt70 but doesn't actually add it to the packet does it?

Ooops. Fixed. The test continues to pass. This was a small issue, so I assumed it doesn't require another round of reviews. The code is now merged.

Thanks for the review. Closing ticket.

comment:8 Changed 21 months ago by tomek

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

comment:9 Changed 21 months ago by fdupont

  • Resolution complete deleted
  • Status changed from closed to reopened

Reopened as it was merged without taking #4231 into account, cf #4313.

comment:10 Changed 21 months ago by fdupont

  • Owner changed from tomek to fdupont
  • Status changed from reopened to reviewing

comment:11 Changed 21 months ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.