Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#5241 closed enhancement (complete)

missing feature: enforce an option in response

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

Description

ISC DHCP configs use this trick (from Dan's samples):

option wpad code 252 = text;

....
        # Windows doesn't put WPAD on its PRL, but does consume it.
        option dhcp-parameter-request-list =
                        concat(option dhcp-parameter-request-list, fc);

The dhcp-parameter-request-list (aka PRL) option in received packets is patched to add the code fc (252) so responses will include it.
As far as I know there is currently no way (other than by a hook library) to do the same in Kea. The code has the persistent flag is OptionDescriptor cfg_option.h but it is set to false in all cases by the option data parser.

Subtickets

Change History (11)

comment:1 Changed 3 years ago by tomek

  • Milestone changed from ISC DHCP migration to Kea1.3
  • Priority changed from high to medium
  • Type changed from defect to enhancement

Francis suggested on 2017-04-13 call that this ticket should be looked at. It's a feature that is very useful for migration. It's present in ISC DHCP and we almost have it in Kea. Almost, because the underlying logic and persistent flag is there, there's just no way to enforce it from a config file.

Based on that, I decided to move it to 1.3.

comment:2 Changed 3 years ago by tomek

  • Component changed from Unclassified to configuration

comment:3 Changed 2 years ago by fdupont

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

comment:4 Changed 2 years ago by fdupont

  • Add Hours to Ticket changed from 0 to 4
  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 4

Done. I don't know if more is needed about the OR semantic (cf jabber and/or classGlobalPersistency new unit tests), in particular in documentation.
There were some requests for this feature so it should be hard to find testers (both for the feature and its doc)!

comment:5 Changed 2 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:6 follow-up: Changed 2 years ago by tomek

I have reviewed the changes. They're mostly good, but have some comments that require fixing before this could be merged:

Even though we internally named the field "persistent", I don't think
it's the best name to be exposed to users. Perhaps better keyword
would by "always-send" or "always"?

dhcp4_srv_unittest.cc

There should be tests that verifies only persistent options are
added. In all scenarios option A is configured (persistent=false) and
option B is configured (persistent=true). The use cases are:

  • Case 1: client sends PRL requesting C, result: only B is added
  • Case 2: client sends PRL requesting A, A and B are added
  • Case 3: client doesn't send PRL, only B is added

The case 3 is almost covered by Dhcpv4SrvTest.classGlobalPersistency.

The documentation should discuss what happens if the persistent flag
is inherited or not. I'm ok with any answer as long as it is
documented. I'm talking about this scenario:

there's option A with a value X, persistent: true specified on global
level, and option A with value Y specified on subnet level. The client
does not send PRL. Which value will he get: X or Y? My undstanding
it's X. Is that correct? Whatever the answer is, it should be explained
in the user's guide.

dhcp_parsers.cc
I don't think you need to use OptionalValue<bool>, but can use
SimpleParser::getBoolean() instead. The JSON is supposed to have all
the values filled in. If it's not specified, the default values
should be used. But if this change causes many unit-test failures,
we can keep the code as is.


The code build and unit-tests pass on Ubuntu 16.04 x64.

comment:7 Changed 2 years ago by tomek

  • Add Hours to Ticket changed from 4 to 2
  • Owner changed from tomek to fdupont
  • Total Hours changed from 4 to 6

A ChangeLog entry is required for this. If you don't have any better proposal,
here's mine:

12xx.	[func]		fdupont
	A 'always' parameter has been added to options configuration.
	It allows on option to be always sent, even if a client
	didn't request it.
	(Trac #5241, git ...)

comment:8 in reply to: ↑ 6 Changed 2 years ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

I have reviewed the changes. They're mostly good, but have some comments that require fixing before this could be merged:

Even though we internally named the field "persistent", I don't think
it's the best name to be exposed to users. Perhaps better keyword
would by "always-send" or "always"?

=> I go for "always-send"

dhcp4_srv_unittest.cc

There should be tests that verifies only persistent options are
added. In all scenarios option A is configured (persistent=false) and
option B is configured (persistent=true). The use cases are:

  • Case 1: client sends PRL requesting C, result: only B is added
  • Case 2: client sends PRL requesting A, A and B are added
  • Case 3: client doesn't send PRL, only B is added

The case 3 is almost covered by Dhcpv4SrvTest.classGlobalPersistency.

=> I added the other cases for both DHCPv4 and DHCPv6.

The documentation should discuss what happens if the persistent flag
is inherited or not. I'm ok with any answer as long as it is
documented.

=> the flag has the same effect than adding the option in the PRL (or ORO).
BTW it is the way it is implemented in ISC DHCP. I updated guides but
I can add more if you think it is needed.

I'm talking about this scenario:

there's option A with a value X, persistent: true specified on global
level, and option A with value Y specified on subnet level. The client
does not send PRL. Which value will he get: X or Y? My undstanding
it's X. Is that correct? Whatever the answer is, it should be explained
in the user's guide.

=> Y as the client gets when it adds A in the PRL. Note this is explicitly
tested in the classGlobalPersistency with a comment about "OR" semantics.

dhcp_parsers.cc
I don't think you need to use OptionalValue<bool>, but can use
SimpleParser::getBoolean() instead. The JSON is supposed to have all
the values filled in. If it's not specified, the default values
should be used. But if this change causes many unit-test failures,
we can keep the code as is.

=> I copied from cvs-format and BTW the same argument applies to.
So either I change both or none. What do you prefer?

Ready for another round... BTW I like the ChangeLog proposal.

comment:9 Changed 2 years ago by tomek

  • Add Hours to Ticket changed from 2 to 1
  • Owner changed from tomek to fdupont
  • Total Hours changed from 6 to 7

Thanks for doing the changes. I have reviewed them and they look good, but I improved couple minor things. Please pull and review. If you're ok with them, please merge.

Oh, the ChangeLog? could be improved a bit as well:

   An 'always-send' parameter has been added to options configuration.
   It allows an option to be always sent, even if a client
   didn't request it.

comment:10 Changed 2 years ago by fdupont

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

Merged. Closing.

comment:11 Changed 2 years ago by vicky

  • Milestone changed from Kea1.3 to Kea1.3 beta

Milestone renamed

Note: See TracTickets for help on using tickets.