Opened 4 years ago

Closed 4 years ago

#4626 closed enhancement (fixed)

Classification: next-server, filename and server-name must be supported

Reported by: tomek Owned by: tomek
Priority: high 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: 5 Internal?: no

Description

In some cases it is necessary to specify that certain fixed fields of the DHCP packet should be set to specific value for all hosts that belong to a class.

In particular, the following fields must be supported:

  • next-server (siaddr field, IPv4 address)
  • filename (file field, 128 bytes)
  • server-name (sname field, 64 bytes)

Subtickets

Change History (7)

comment:1 Changed 4 years ago by tomek

  • Summary changed from Constant fields in DHCPv4 must be settable for client classes to Classification: next-server, filename and server-name must be supported

comment:2 Changed 4 years ago by tomek

  • Owner set to Unassigned
  • Status changed from new to reviewing

This is not the most extensively tested code, but given the deadlines (1 day), I did what I could.

The code is now ready for review.

Proposed changelog:

11XX.	[func]		tomek
	It is now possible to specify fixed fields (next-server,
	server-hostname and boot-file-name parameters) for client classes.
	(Trac #4626, git tbd)

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

  • Owner changed from Unassigned to tomek

src/bin/dhcp4/dhcp4_srv.cc
General: there is no clear ordering in which fixed field values will be assigned to the client.
There are currently three places in the code where we set those fields: assignLease, setFixedFields and vendorClassSpecificProcessing. The assignLease function is really a poor choice for setting the value of siaddr because this function is meant for lease assignment, not seting fixed fields. So, I'd suggest that we move that out of assignLease to setFixedFields function and set the subnet specific value only if siaddr is not set using host reservations or client classification. Then... we have vendorClassSpecificProcessing, which can also set the siaddr and file name value if this is docsis modem. Having this function executed after setFixedFields means that anything we have set in setFixedFields can be overriden by DOCSIS specific stuff. I don't think this should be the case for at least host reservations. If client has reservation, the DOCSIS specific setting should be ignored.

What we should really consider (and perhaps do it after Beta) is to remove the vendorClassSpecificProcessing altogether. With the addition of fixed fields configuration we can configure the server to assign or not those fields without this special case we currently make for cable modem and eRouter class devices. Then, the order of processing for fixed fields should be: host reservations, client classification and subnet specific values.

processDiscover, processRequest, processInform: The following comment:

// See if the class mandates setting any fixed fields (siaddr, sname,
// filename).

is not accurate. This function is meant for setting the values based on reservations, classes or subnet specific configuration. As it stands, it suggests that this is only for classes.

src/bin/dhcp4/dhcp4_srv.h
The description should better already say that this class sets fields using host reservations or client classification or subnet specific configuration.

src/bin/dhcp4/tests/classify_unittest.cc
The following comment:

/// @brief Set of JSON configurations used throughout the DORA tests.

need to be updated to rather say "classification tests".

You don't seem to be using CONFIG[0] anywhere. So maybe it can be removed?

Also, the comment in array of configurations says: "Configuration 6". It should say "Configuration 1".

testFixedFields function is not documented.

testFixedFields: In #4552 I added support to !Dhcp4Client to extract values of sname and file into the string, which can be retrieved from the client. I suggest you use my approach. This should simplify the test.

src/lib/dhcpsrv/client_class_def.cc
Copyright date should be updated.

Please use:

asiolink::IOAddress::IPV4_ZERO_ADDRESS()

rather than

asiolink::IOAddress("0.0.0.0")

in this file.

src/lib/dhcpsrv/client_class_def.h
Copyright date should be updated.

getNextServer should return const reference, just like other accessors.

getSname() should probably be placed between setSname and setFilename.

Could setSname and setFilename have std::string argument instead of vector?

Please use:

asiolink::IOAddress::IPV4_ZERO_ADDRESS()

rather than

asiolink::IOAddress("0.0.0.0")

in this file.

addClass: next_server should be rather passed by reference.

src/lib/dhcpsrv/parsers/client_class_def_parser.cc
ClientClassDefParser::build the block of code which parses fixed fields throws exceptions but doesn't include line number. This whole block, as well as the preceding:

    try {
        name = string_values_->getParam("name");
    } catch (const std::exception& ex) {
        isc_throw(DhcpConfigError, ex.what() << " ("
                  << class_def_cfg->getPosition() << ")");
    }

and the

    try {
        // an OptionCollectionPtr
        class_dictionary_->addClass(name, match_expr_, options_, next_server,
                                    sname, filename);
    } catch (const std::exception& ex) {
        isc_throw(DhcpConfigError, ex.what()
                  << " (" << class_def_cfg->getPosition() << ")");
    }

should be included in a single (large) try-catch clause which appends line number (position) to the exception string.

I also think that the next_server should be checked for not being 0 or 255.255.255.255?

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
Copyright date to be updated.
Each test should come with some short description of what it is doing.

src/lib/dhcpsrv/tests/client_class_def_unittest.cc
Copyright date to be updated.
Again: better to use asiolink::IOAddress::IPV4_ZERO_ADDRESS();

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

  • Add Hours to Ticket changed from 0 to 3
  • Owner changed from tomek to marcin
  • Total Hours changed from 0 to 3

Replying to marcin:

src/bin/dhcp4/dhcp4_srv.cc
General: there is no clear ordering in which fixed field values will be assigned to the client.
There are currently three places in the code where we set those fields: assignLease, setFixedFields and vendorClassSpecificProcessing. The assignLease function is really a poor choice for setting the value of siaddr because this function is meant for lease assignment, not seting fixed fields. So, I'd suggest that we move that out of assignLease to setFixedFields function and set the subnet specific value only if siaddr is not set using host reservations or client classification. Then... we have vendorClassSpecificProcessing, which can also set the siaddr and file name value if this is docsis modem. Having this function executed after setFixedFields means that anything we have set in setFixedFields can be overriden by DOCSIS specific stuff. I don't think this should be the case for at least host reservations. If client has reservation, the DOCSIS specific setting should be ignored.

What we should really consider (and perhaps do it after Beta) is to remove the vendorClassSpecificProcessing altogether. With the addition of fixed fields configuration we can configure the server to assign or not those fields without this special case we currently make for cable modem and eRouter class devices. Then, the order of processing for fixed fields should be: host reservations, client classification and subnet specific values.

Removed vendorClassSpecificProcessing. It was actually quite quick. Added necessary note in the User's guide.

processDiscover, processRequest, processInform: The following comment:

// See if the class mandates setting any fixed fields (siaddr, sname,
// filename).

is not accurate. This function is meant for setting the values based on reservations, classes or subnet specific configuration. As it stands, it suggests that this is only for classes.

Updated as suggested.


src/bin/dhcp4/dhcp4_srv.h
The description should better already say that this class sets fields using host reservations or client classification or subnet specific configuration.

Updated.

src/bin/dhcp4/tests/classify_unittest.cc
The following comment:

/// @brief Set of JSON configurations used throughout the DORA tests.

need to be updated to rather say "classification tests".

Copy-paste error. Corrected.

You don't seem to be using CONFIG[0] anywhere. So maybe it can be removed?

Removed.

Also, the comment in array of configurations says: "Configuration 6". It should say "Configuration 1".

Updated.

testFixedFields function is not documented.

It is now.

testFixedFields: In #4552 I added support to !Dhcp4Client to extract values of sname and file into the string, which can be retrieved from the client. I suggest you use my approach. This should simplify the test.

Can't do it right now. You would have to merge the code, then I would have to rebase it and then take advantage of that code. It's not worth the effort. We can try doing the change when merging, though.


src/lib/dhcpsrv/client_class_def.cc
Copyright date should be updated.

Please use:

asiolink::IOAddress::IPV4_ZERO_ADDRESS()

rather than

asiolink::IOAddress("0.0.0.0")

in this file.

Done.

src/lib/dhcpsrv/client_class_def.h
Copyright date should be updated.

getNextServer should return const reference, just like other accessors.

getSname() should probably be placed between setSname and setFilename.

Reordered.

Could setSname and setFilename have std::string argument instead of vector?

Updated as suggested. Note that Pkt4 still uses vector<uint8_t>. I tried changing it, but it broke tons of unit-tests.

Please use:

asiolink::IOAddress::IPV4_ZERO_ADDRESS()

rather than

asiolink::IOAddress("0.0.0.0")

in this file.

Updated.

addClass: next_server should be rather passed by reference.

It is now.

src/lib/dhcpsrv/parsers/client_class_def_parser.cc
ClientClassDefParser::build the block of code which parses fixed fields throws exceptions but doesn't include line number. This whole block, as well as the preceding:
should be included in a single (large) try-catch clause which appends line number (position) to the exception string.

Updated as suggested.

I also think that the next_server should be checked for not being 0 or 255.255.255.255?

For broadcast yes, but not for 0.0.0.0. One day classes will allow to unset the value set for the whole subnet, so there's a usecase for the value being 0.0.0.0

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc
Copyright date to be updated.
Each test should come with some short description of what it is doing.

Description added, copyright update.

src/lib/dhcpsrv/tests/client_class_def_unittest.cc
Copyright date to be updated.
Again: better to use asiolink::IOAddress::IPV4_ZERO_ADDRESS();

Copyright updated, using IPV4_ZERO_ADDRESS now.

Last edited 4 years ago by tomek (previous) (diff)

comment:5 Changed 4 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit ada652bab5133d89db54e135797aa17f6c10d56b

Your changes look good. The only minor issues are to replace "live" with "life" in two commentaries. Also, remove the leftover commentary after removing the vendor specific classification, that includes "John".

I don't need to see this ticket again. Though, you'll have to merge with my changes obviously and also it would be good to use Dhcp4Client's functions to retrieve values of fixed fields, but it is not must have.

The code builds and unit tests pass on my OS X El Capitan.

comment:6 Changed 4 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.1

comment:7 Changed 4 years ago by tomek

  • Add Hours to Ticket changed from 3 to 2
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 3 to 5

Thanks for your review. Code merged. Closing ticket.

Note: See TracTickets for help on using tickets.