Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#4058 closed enhancement (complete)

DHCPv4 Subnet Selection Option support

Reported by: fdupont Owned by: fdupont
Priority: low Milestone: Kea1.0-beta
Component: Unclassified 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

DHCPv4 Subnet Selection Option (aka DHO_SUBNET_SELECTION) should be the second (after #4057) tested case in DHCPv4 (including DHCPv4-over-DHCPv6) subnet selection.

Subtickets

Change History (12)

comment:1 Changed 5 years ago by fdupont

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

Done (with in addition some missing comments). Depends on #4057 which depends on #4061.
Ready for review.

comment:2 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0
  • Owner changed from UnAssigned to stephen

per team meeting 7 oct, move to 1.0. Stephen will review.

comment:3 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commits e5efdff9a809ae7d93873a3250dfbc3f86ce0c63 through 27241065098a18c5fe7ebb1d92cb7665d2b7d1bb inclusive.

The code changes are OK for getting the appropriate address, but RFC 3011 states:

Servers configured to support this option MUST return an identical
copy of the option to any client that sends it, regardless of whether
or not the client requests the option in a parameter request list.
Clients using this option MUST discard DHCPOFFER or DHCPACK packets
that do not contain this option.

Nothing in the changes includes the recording of the option being sent and the appending of the option to the returned packet.

How this is implemented merits discussion on the kea-dev list. One possibility is to update (or create) the parameter request list option in the incoming packet to include the option so that when the options are added to the returned packet, the subnet selection option is included. However this suffers the drawback that the incoming packet has been modified, something that may have consequences elsewhere (e.g. during debugging problems).

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

Replying to stephen:

Reviewed commits e5efdff9a809ae7d93873a3250dfbc3f86ce0c63 through 27241065098a18c5fe7ebb1d92cb7665d2b7d1bb inclusive.

The code changes are OK for getting the appropriate address, but RFC 3011 states:

Servers configured to support this option MUST return an identical
copy of the option to any client that sends it, regardless of whether
or not the client requests the option in a parameter request list.
Clients using this option MUST discard DHCPOFFER or DHCPACK packets
that do not contain this option.

=> hum, this seems to be a mandatory echo back for this particular option.

Nothing in the changes includes the recording of the option being sent and the appending of the option to the returned packet.

=> all options are recorded by unpack so I don't understand the first part. For the second it should be addressed with the next comment.

How this is implemented merits discussion on the kea-dev list. One possibility is to update (or create) the parameter request list option in the incoming packet to include the option so that when the options are added to the returned packet, the subnet selection option is included. However this suffers the drawback that the incoming packet has been modified, something that may have consequences elsewhere (e.g. during debugging problems).

=> there are two methods where this can be put: appendRequestedOptions() (with the drawback you described) and appendBasicOptions(). IMHO none is suitable: we need something DHCPv6 copyClientOptions(), so I propose to add this function (add the subnet select option in response when presented in query in the copyDefaultFields() method. Note this method already copies the RAI option so its name already doesn't fully reflect its action...

Please comment and/or give back the ticket to me if you like my idea.

comment:5 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

comment:6 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Agreed. The the cleanest way would seem be to create a copyClientOptions() method and have it copy both the subnet select and the RAI option (which is removed from copyDefaultFields()). This leaves copyDefaultFields() copying the non-option fields of the incoming packet.

As far as I can see, copyDefaultFields() is only called once, in initResponse(). Following that with a call to copyClientOptions() should work.

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

  • Owner changed from fdupont to stephen

Replying to stephen:

Agreed. The the cleanest way would seem be to create a copyClientOptions() method and have it copy both the subnet select and the RAI option (which is removed from copyDefaultFields()). This leaves copyDefaultFields() copying the non-option fields of the incoming packet.

As far as I can see, copyDefaultFields() is only called once, in initResponse(). Following that with a call to copyClientOptions() should work.

=> Done. There is only one remaining point: should we provide a dedicated unit test? IMHO it should be integrated into a complete test where a response is generated and can be analyzed (so perhaps a system (vs unit) test).

comment:8 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commits c2c57b4941f2b5fcecdd414efebefbf7da4ff621 through 599964bb870908ea5c5fffc3bb46272ffd18cd63

Code looks OK.

Done. There is only one remaining point: should we provide a dedicated unit test? IMHO it should be integrated into a complete test where a response is generated and can be analyzed (so perhaps a system (vs unit) test).

We do need tests. As part of this ticket I think we need to add a unit test for the copyDefault{Fields,Options} methods. These are private, so I suggest writing a unit test for the public initResponse method (which does little more than call the two methods if the query is a DISCOVER, REQUEST or INFORM.

Create another ticket for a system test to check that the subnet selection and RAI options are copied across.

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

  • Owner changed from fdupont to stephen

Replying to stephen:

Done. There is only one remaining point: should we provide a dedicated unit test? IMHO it should be integrated into a complete test where a response is generated and can be analyzed (so perhaps a system (vs unit) test).

We do need tests. As part of this ticket I think we need to add a unit test for the copyDefault{Fields,Options} methods. These are private, so I suggest writing a unit test for the public initResponse method (which does little more than call the two methods if the query is a DISCOVER, REQUEST or INFORM.

=> done.

Create another ticket for a system test to check that the subnet selection and RAI options are copied across.

=> created #4083

comment:10 Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commits 1bbe5b67149930e43859df86fbf5325a6abe8fa8 through 5d02e9564887a3b1be6750499af4b2860cca82ea

All OK, please merge.

I think this needs a ChangeLog entry. Something like:

Add support for DHCPv4 subnet selection option.

... should be OK.

comment:11 Changed 4 years ago by fdupont

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

Merged. Closing.

comment:12 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.