Opened 4 years ago

Closed 4 years ago

#4523 closed defect (fixed)

Kea does not process vendor options in Renew and Rebind

Reported by: marcin Owned by: fdupont
Priority: medium Milestone: Kea1.1
Component: dhcp6 Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 1
Total Hours: 4 Internal?: no

Description

The !Dhcpv6Srv::appendRequestedVendorOptions is called for Solicit and Request case, but not for a Rebind case. For the DOCSIS3 case, where vendor options are requested via ORO within option 17, it means that only a client performing 4-way exchange can obtain vendor options from the server. For any client already having a lease this appears to be a problem because it will not get vendor options which may be configured on the server later on. In addition, if the server is configured to stop sending certain options the client having a lease will not notice that.

Subtickets

Change History (16)

comment:1 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.1

Per June 23 team meeting, accept 1.1, 1 day

comment:2 Changed 4 years ago by fdupont

Is there a good reason to not call it? If not we have just to call it...

comment:3 Changed 4 years ago by fdupont

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

comment:4 Changed 4 years ago by fdupont

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

Done. Ready for review.

comment:5 Changed 4 years ago by marcin

The most time consuming part of this ticket is about unit tests. From what I have seen, you haven't added any unit tests to verify that the vendor options are correctly processed for Renew/Rebind?.

comment:6 Changed 4 years ago by fdupont

In fact in the current unit tests only the SOLICIT case (from captureDocsisRelayedSolicit) is checked...
So we have a proof the appendRequestedVendorOptions() is correct but no per message type.

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

The point of unit-test to test a unit. In this particular case the unit is the code processing renew and rebind. Yes, the code for those tests will be very similar to the one that tests solicit, but that's perfectly ok.

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

Replying to tomek:

The point of unit-test to test a unit. In this particular case the unit is the code processing renew and rebind. Yes, the code for those tests will be very similar to the one that tests solicit, but that's perfectly ok.

=> this is not my point: my concern is this test exists only for solicit, not for request, so there are 2 solutions:

  • status quo, i.e., not add tests
  • create a new ticket to test this for all message types where it makes sense (but not solicit where it is already done).

The third way: add it only for renew and rebind is not consistent.

comment:9 follow-up: Changed 4 years ago by marcin

In fact there is one more solution, which is a hybrid of 2 and 3. That is, implement tests for Renew, Rebind and Request within this ticket.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by fdupont

Replying to marcin:

In fact there is one more solution, which is a hybrid of 2 and 3. That is, implement tests for Renew, Rebind and Request within this ticket.

=> IMHO it is the worst one: there are already enough missing tests to require a specific ticket to complete all tests (i.e., not only requested vendor options). Or with other words we should not put too much in this tickets and as we know there are many things missing do a more systematic work in a dedicated ticket.

comment:11 in reply to: ↑ 10 Changed 4 years ago by marcin

Replying to fdupont:

Replying to marcin:

In fact there is one more solution, which is a hybrid of 2 and 3. That is, implement tests for Renew, Rebind and Request within this ticket.

=> IMHO it is the worst one: there are already enough missing tests to require a specific ticket to complete all tests (i.e., not only requested vendor options). Or with other words we should not put too much in this tickets and as we know there are many things missing do a more systematic work in a dedicated ticket.

Our coding guidelines require us to implement unit tests for all code changes. Adding code change and no unit tests goes against the guidelines and worsens the situation as we have more code not covered by unit tests than we had before the code change. Adding a ticket to add tests to the existing code is also worse in that sense that it reverts the process of test driven development where you're expected to run a test before you apply the change (and expect it to fail) to make sure that the test really tests what it is supposed to. So, by all means, deferring the implementation of tests is the worse choice. If you don't want to fill the gap in testing the Request case, I suggest you at least cover the change you have made for Renew and Rebind. In your terms, this is option 3.

comment:12 Changed 4 years ago by fdupont

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

comment:13 Changed 4 years ago by fdupont

  • Estimated Difficulty changed from 1 to 4
  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 3

I added the DOCSIS vendor ORO unit tests fro renew and rebind. BTW now we have the framework to add this test for other cases so I still strongly suggest to open a ticket to add all missing tests (standard ORO processing is not checked for many message types so it is not only for the DOCIS vendor ORO).
Ready for review (BTW there was a spelling error in the middle of the example so I checked and fixed spelling of files before modifying them).

comment:14 Changed 4 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:15 Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 0 to 1
  • Owner changed from marcin to fdupont
  • Total Hours changed from 3 to 4

Reviewed commit cfe22c8dd7e14649b8878c3e83ec4ef4ccc1c46d

Important: please pull my changes.

Tests you have added are good. I found some nits and typos which I corrected:

  • Configuration 0 -> Configuration 7 (copy-paste error)
  • Invalid indentation of the vendor sub options data in the new configuration
  • No need for the config-file option definition, because this definition is pre-defined in docsis3_option_defs.h
  • OptionVentor -> OptionVendor
  • Rename RenewTest.requestDocsisORORenew -> RenewTest.docsisORO to be consistent with the same test for Rebind.
  • Copyright date updated in RebindTest

If you accept my changes, you're ready to merge.

comment:16 Changed 4 years ago by fdupont

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

Merged (resolving conflicts from multiple reservations). Closing.

Note: See TracTickets for help on using tickets.