Opened 7 years ago

Closed 7 years ago

#2898 closed defect (fixed)

DHCPv6 server must handle relayed traffic

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130509
Component: dhcp6 Version:
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

#2827 added packet packing/unpacking. This information should now be used in dhcpv6 server.

  1. select subnet() should take the following cases into consideration
  • link-addr field from the relay closest to the client
  • interface-id option (if present) included in relay that is closest to the client
  1. When creating a response, relay info from the message we're answering to must be copied (especially interface-id)
  1. parser should be extended to allow (optional) interface-id option definition

Subtickets

Change History (8)

comment:1 Changed 7 years ago by tomek

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

comment:2 Changed 7 years ago by tomek

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

The unittests and the code has been written.
BIND10 Guide and BIND10 Developer's Guide has been updated.

Please review.

comment:3 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:4 follow-ups: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit df3ee4b9e04459e2ba6dbc50702b3c7f29c0ceb7

src/bin/dhcp6/config_parser.cc
Subnet6ConfigParser::createSubnet (line 1484): at this point the exception declaration is "const DhcpConfigError&", but a few lines later the same exception is caught via "DhcpConfigError". The catch clauses should be consistent.

Subnet6ConfigParser::createSubnet (line 1502): in the isc_throw statement when both the interface ID and interface are not empty, convert "len" to an int via the "static_cast<int>" construct.

Subnet6ConfigParser::createSubnet (line 1532): as the previous check has extablished that either iface is not empty or ifaceid is not empty, but both cannot contain something at the same time, this "if" could be an "else if". The effect is the same, but use of "else if" serves to document to the reader that it is "either-or". In fact, going beyond that, can't these two checks be combined into a single "if-else if-else if" block with the check that leads to the isc_throw statement:

if (!iface.empty() && !ifaceid.empty()) {
   :
} else if (!iface.empty()) {
   :
} else if (!ifaceid.empty()) {
   }
}

(Incidentally, before merging this, consult with Thomas - he has changed this area of the code.)

src/bin/dhcp6/dhcp6_srv.cc
dhcp6Srv::copyDefaultOptions: please start comments with a capital letter.

DhcpSrv::selectSubnet: the first block of the "if" can be simplified to:

Subnet6Ptr subnet = ...
if (!subnet) P
   // No subnet found, try to find it based on remote address.
   subnet = ...
}
return (subnet);

Similar logic can be used in the "else" clause to avoid the

if (subnet) {
   return (subnet);
}

Finally, both the "if" and "else" parts of the test end with "return (subnet)": this can be taken out of the "if" block and put as the last statement in the method.

src/bin/dhcp6/tests/config_parser_unittest.cc
class Dhcp6ParserTest: it would be helpful to comment the member variables: they are used in various tests and sometimes it is not clear what is being done.

Tests subnetInterfaceId, interfaceIdGlobal, subnetInterfaceAndInterfaceId: several points:

  • Please start comments with capital letters.
  • "config" can be declared as a "const string".
  • Is there a need to output the config to cout during the test?
  • Suggest that declaration of "status" (or "new_config" if renamed) be moved to after the declaration of "config" so that it is closer to the point of first use.
  • All tests contain a comment that says "return value should be..." after configureDhcp6Server is called. The comment is wrong, as the returned value is a pointer to a new configuration. (Incidentally, "status" seems the wrong name here, as a pointer to a new configuration is returned. Perhaps "new_config" is a better name?)

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Test selectSubnetRelayLinkaddr: please start comments with a capital letter (s/source of the packet should have/Souce of the packet should have/)

Test selectSubnetRelayInterfaceId: when adding to the relay options (in the code for case 1), use of std::make_pair() is generally clearer (and easier to type) than explicitly creating a "pair" object. Also, how often is this insert (where both the option and the type of option have to be specified) is used in the code? Rather than expose the options_ in the relay object, would it not be better to provide a method takes one argument (the option) and performs the insert?

src/lib/dhcp/libdhcp++.dox
The contents seems OK. I've not attempted to correct spelling or grammar, as I think that the whole of the programmer documentation needs editing.

src/lib/dhcp/pkt6.cc
Pkt6::getAnyRelayOption: please start comments with capital letters.

Pkt6::getAnyRelayOption: suggest putting the check whether relay_info_ is empty before the declaration of start/end/direction: this moves those declarations close to where they are first used.

Pkt6::getAnyRelayOption: in the loop, rather than have the additional check for i == end in the loop, use the STL iterator trick: define the end to be one element before/after the action end. For example, int the RELAY_SEARCH_FROM_CLIENT code, set end is set to -1. The "for" loop is thus modified to:

for (int i = start; i != end; i += direction) {

Pkt6:copyRelayInfo: please start comments with capital letters.

Pkt6:copyRelayInfo: would prefer that something like "info" instead of "x" is used for the temporary variable.

Pkt6:copyRelayInfo: what is "relay-repl info"?

Pkt6:copyRelayInfo: s/Is there interface-id option in this nesting level if there is,/Is there interface-id option in this nesting level? If there is,/

src/lib/dhcp/pkt6.h
It's OK, but why move the definition of Pkt6Ptr before the definition of Pkt6? It only necessitates the addition of a forward declaration.

The description of RELAY_SEARCH_{FIRST,LAST} is not completely clear. Does they mean that there is no search through the relays, that only the first or last relay is checked for the option?

Suggest slight rephrasing of the getAnyRelay method description:

/// @brief Return first instance of a specified option
///
/// When a client's packet traverses multiple relays, each passing relay may
/// insert extra options. This method allows the specific instance of a given
/// option to be obtained (e.g. closest to the client, closest to the server,
/// etc.) See @ref RelaySearchOrder for a detailed description.

Since methods have been added to search the relay information and to copy relay information, should relay_info_ still be a public field?

src/lib/dhcp/tests/pkt6_unittest.cc
getAnyRelayOption: the tests should be extended to check that searching for an option that does not exist (using all four search options) returns an empty pointer.

src/lib/dhcpsrv/cfgmgr.cc
CfgMgr::getSubnet6: inverting the logic of the initial test could remove one return statement:

if (iface_id_option) {
   <for loop goes here>
}

// Either the option wasn't given or we couldn't find a
// subnet with a matching ID

return (Subnet6Ptr());

CfgMgr::getSubnet6: the comment before the for loop is incorrect - if there are one or more subnets, we need to choose the proper one. And if more than one have a matching interface ID, only the first is returned.

src/lib/dhcpsrv/dhcpsrv_messages.mes
DHCPSRV_CFGMGR_SUBNET6_IFACE_ID: s/in subnet6 definition/in the subnet6 definition/

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
Line 51: s/paramaters/parameters/ (This was the line where a trailing space was removed, which is why I noticed it.)

Test subnet6Interface: with one subnet, should also check that looking for an incorrect interface ID will return an empty subnet pointer. (The check is made when multiple subnets are added. It could be that a single subnet is a special case and matches anything. IIRC there is somewhere else in the DHCP code that has similar logic.)

doc/guide/bind10-guide.xml
I have a number of corrections and suggestions to the new documentation. The suggested text is below:

<para>
    A DHCPv6 server with multiple subnets defined must select the
    appropriate subnet when it receives a request from client.  For clients
    connected via relays, two mechanisms are used.
</para>
<para>
    The first uses the linkaddr field in the RELAY_FORW message. The name
    of this field is somewhat misleading in that it does not contain link-layer
    address: instead, it holds an address (typically a global address) that is
    used to identify a link. The DHCPv6 server checks if the address belongs
    to a defined subnet and, if it does, that subnet is selected for the client's
    request.
</para>
<para>
    The second mechanism is based on interface-id options. While forwarding client's
    message, relays may insert an interface-id option into the message that
    identifies the interface on the relay that received client message. (Some
    relays allow configuration of that parameter, but it is sometimes
    hardcoded and may range from very simple (e.g. "vlan100") to very cryptic:
    one example seen on real hardware was "ISAM144|299|ipv6|nt:vp:1:110"). The
    server can use this information to select the appropriate subnet.
    The information is also returned to the relay which then knows which
    interface to use to transmit the response to the client. In order to
    use this successfully, the relay interface IDs must be unique within
    the network, and the server configuration must match those values.
</para>
<para>
    When configuring the DHCPv6 server, it should be noted that two
    similarly-named parameters can be configured for a subnet:
    <itemizedlist>
        <listitem><simpara>
            "interface" defines which local network interface can be used
            to access a given subnet.
        </simpara></listitem>
        <listitem><simpara>
            "interface-id" specifies the content of the interface-id option
            used by relays to identify the interface on the relay to which
            the response packet is sent.
        </simpara></listitem>
    </itemizedlist>
    The two are mutually exclusive: a subnet cannot be both reachable locally
    (direct traffic) and via relays (remote traffic). Specifying both is a
    configuration error and the DHCPv6 server will refuse such a configuration.
</para>

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

Replying to stephen:

Reviewed commit df3ee4b9e04459e2ba6dbc50702b3c7f29c0ceb7

src/bin/dhcp6/config_parser.cc
Subnet6ConfigParser::createSubnet (line 1484): at this point the exception declaration is "const DhcpConfigError&", but a few lines later the same exception is caught via "DhcpConfigError". The catch clauses should be consistent.

Done.

Subnet6ConfigParser::createSubnet (line 1502): in the isc_throw statement when both the interface ID and interface are not empty, convert "len" to an int via the "static_cast<int>" construct.

Done.

Subnet6ConfigParser::createSubnet (line 1532): as the previous check has extablished that either iface is not empty or ifaceid is not empty, but both cannot contain something at the same time, this "if" could be an "else if". The effect is the same, but use of "else if" serves to document to the reader that it is "either-or". In fact, going beyond that, can't these two checks be combined into a single "if-else if-else if" block with the check that leads to the isc_throw statement:

if (!iface.empty() && !ifaceid.empty()) {
   :
} else if (!iface.empty()) {
   :
} else if (!ifaceid.empty()) {
   }
}

I left the code as is for simplicity reasons. Both interface and interface-id are optional. Allowed combinations are: none, interface defined, interface-id defined. Illegal combination: both interface and interface-id defined.

We check if illegal conditions occur and throw exception. Once we check that this is not the case, then and only then we create subnet6 object. I don't want to create subnet object first and then throw it away if we detect later that invalid conditions occurred.

Sure, we could continue the line:

if (!iface.empty() && !ifaceid.empty()) {
  isc_throw(...)
}

with else clause and put the rest of the method in this clause, but it would give us little and only increase indentation, which would make the code more difficult to read.

x>

(Incidentally, before merging this, consult with Thomas - he has changed this area of the code.)

src/bin/dhcp6/dhcp6_srv.cc
dhcp6Srv::copyDefaultOptions: please start comments with a capital letter.

DhcpSrv::selectSubnet: the first block of the "if" can be simplified to:

Subnet6Ptr subnet = ...
if (!subnet) P
   // No subnet found, try to find it based on remote address.
   subnet = ...
}
return (subnet);

Similar logic can be used in the "else" clause to avoid the

if (subnet) {
   return (subnet);
}

I did as you requested. The code would is shorter by couple lines, but is marginally slower now. It is on a critical path, but the performance penalty is minimal (just one extra comparison).

src/bin/dhcp6/tests/config_parser_unittest.cc
class Dhcp6ParserTest: it would be helpful to comment the member variables: they are used in various tests and sometimes it is not clear what is being done.

Done.

Tests subnetInterfaceId, interfaceIdGlobal, subnetInterfaceAndInterfaceId: several points:

  • Please start comments with capital letters.
  • "config" can be declared as a "const string".
  • Is there a need to output the config to cout during the test?
  • Suggest that declaration of "status" (or "new_config" if renamed) be moved to after the declaration of "config" so that it is closer to the point of first use.

Done.

  • All tests contain a comment that says "return value should be..." after configureDhcp6Server is called. The comment is wrong, as the returned value is a pointer to a new configuration. (Incidentally, "status" seems the wrong name here, as a pointer to a new configuration is returned. Perhaps "new_config" is a better name?)

No. See documentation or implementation for configureDhcp6Server. It returns a pointer to very small element that contains rcode (integer value) and optionally a string that provides extra info (usually explains error condition is rcode!=0). Moved the status declaration closer to its usage, but kept its name.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Test selectSubnetRelayLinkaddr: please start comments with a capital letter (s/source of the packet should have/Souce of the packet should have/)

Done.

Changes pushed. The review will continue tomorrow.

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

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit df3ee4b9e04459e2ba6dbc50702b3c7f29c0ceb7

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Test selectSubnetRelayLinkaddr: please start comments with a capital letter (s/source of the packet should have/Souce of the packet should have/)

Test selectSubnetRelayInterfaceId: when adding to the relay options (in the code for case 1), use of std::make_pair() is generally clearer (and easier to type) than explicitly creating a "pair" object.

Done.

Also, how often is this insert (where both the option and the type of option have to be specified) is used in the code? Rather than expose the options_ in the relay object, would it not be better to provide a method takes one argument (the option) and performs the insert?

Because right now OptionCollection? is a std::multimap, a basic STL type. This data will soon be exported via hook to various languages. I have a hope that there are C++ to Python converters for standard STL types already defined and I hope to use them.

What you are proposing would give us really marginal benefic, but would bring in quite a lot of pain later. I very much prefer for the code to stay as it is.

src/lib/dhcp/pkt6.cc
Pkt6::getAnyRelayOption: please start comments with capital letters.

Pkt6::getAnyRelayOption: suggest putting the check whether relay_info_ is empty before the declaration of start/end/direction: this moves those declarations close to where they are first used.

Pkt6::getAnyRelayOption: in the loop, rather than have the additional check for i == end in the loop, use the STL iterator trick: define the end to be one element before/after the action end. For example, int the RELAY_SEARCH_FROM_CLIENT code, set end is set to -1. The "for" loop is thus modified to:

for (int i = start; i != end; i += direction) {

Your proposal wouldn't work, because in case of RELAY_SEARCH_LAST and RELAY_SEARCH_FIRST there is only one relay to check, so direction is 0 and i is not incremented. Furthermore, the code used to assume that after the loop i points to the last relay to be checked, so it would break down.

I've refactored the code a bit and it is now simpler and there are more comments around it. Please let me know if it is sufficient or should I tweak it more.

Pkt6:copyRelayInfo: please start comments with capital letters.

Done.

Pkt6:copyRelayInfo: would prefer that something like "info" instead of "x" is used for the temporary variable.

Done.

Pkt6:copyRelayInfo: what is "relay-repl info"?

The comment was confusing. Hopefully it is clearer now.

Pkt6:copyRelayInfo: s/Is there interface-id option in this nesting level if there is,/Is there interface-id option in this nesting level? If there is,/

Fixed.

src/lib/dhcp/pkt6.h
It's OK, but why move the definition of Pkt6Ptr before the definition of Pkt6? It only necessitates the addition of a forward declaration.

Please move it back and see what happens :)

(hint: see parameter type passed to copyRelayInfo())

The description of RELAY_SEARCH_{FIRST,LAST} is not completely clear. Does they mean that there is no search through the relays, that only the first or last relay is checked for the option?

It is difficult to convey the search patterns in a short name. I've renamed it to RELAY_GET_{FIRST,LAST}. Also updated the decriptions a bit. Hopefully it is more understandable now.

Suggest slight rephrasing of the getAnyRelay method description:

/// @brief Return first instance of a specified option
///
/// When a client's packet traverses multiple relays, each passing relay may
/// insert extra options. This method allows the specific instance of a given
/// option to be obtained (e.g. closest to the client, closest to the server,
/// etc.) See @ref RelaySearchOrder for a detailed description.

Since methods have been added to search the relay information and to copy relay information, should relay_info_ still be a public field?

Yes. Otherwise you get into the object lifetime mess. If you implement a method that hides it, will you return const references? You'll get into the problem that returned reference could be potentially used past the Pkt6 lifetime. Will you return a copy? That's inefficient and even useless when you want to modify something in the packet. It is much simpler to just keep it public. It will also make our life easier when implementing hooks.

I personally think about Pkt{4,6} not as a proper class, but rather a structure with some extra methods specified for convenience. The major argument for making members non-public is to have control over changes, so we maintain internal data consistency. However, internal consistency is our explicit non-goal: we want hooks to be able to modify anything, be it reasonable change or not.

src/lib/dhcp/tests/pkt6_unittest.cc
getAnyRelayOption: the tests should be extended to check that searching for an option that does not exist (using all four search options) returns an empty pointer.

Done. Also, expanded checks to see that the getAnyRelayOption will not return user options.

src/lib/dhcpsrv/cfgmgr.cc
CfgMgr::getSubnet6: inverting the logic of the initial test could remove one return statement:

if (iface_id_option) {
   <for loop goes here>
}

// Either the option wasn't given or we couldn't find a
// subnet with a matching ID

return (Subnet6Ptr());

This seemingly simple function would be then 5 indentation levels deep then. Simple return statement is not that much. Can we keep it as it is?

CfgMgr::getSubnet6: the comment before the for loop is incorrect - if there are one or more subnets, we need to choose the proper one. And if more than one have a matching interface ID, only the first is returned.

Copy-paste error. I have updated the comment.

src/lib/dhcpsrv/dhcpsrv_messages.mes
DHCPSRV_CFGMGR_SUBNET6_IFACE_ID: s/in subnet6 definition/in the subnet6 definition/

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc
Line 51: s/paramaters/parameters/ (This was the line where a trailing space was removed, which is why I noticed it.)

Fixed.

Test subnet6Interface: with one subnet, should also check that looking for an incorrect interface ID will return an empty subnet pointer. (The check is made when multiple subnets are added. It could be that a single subnet is a special case and matches anything. IIRC there is somewhere else in the DHCP code that has similar logic.)

Added. I think we'll need to revisit that special case logic once we have better per interface control implemented (right now we open up sockets on every interface present in the system).

doc/guide/bind10-guide.xml
I have a number of corrections and suggestions to the new documentation. The suggested text is below:

...

Text updated.

Also updated code in several places to use make_pair rather than longer pair<a_type,b_type>(a,b) definition.

The ticket is back with you.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 131a89175e97714ac87f14f95703cd76df7a965c

doc/guide/bind10-guide.xml
I've pushed an update correcting a few errors in the text I supplied.

src/bin/dhcp6/dhcp6_srv.cc

I did as you requested. The code would is shorter by couple lines, but is marginally slower now. It is on a critical path, but the performance penalty is minimal (just one extra comparison).

I'm not sure where the one extra comparison comes from. The number of "if" tests is unchanged for every path through the method. Besides which, the optimising compiler is likely to have optimised both versions into the same set of machine instructions.

src/bin/dhcp6/tests/config_parser_unittest.cc

All tests contain a comment that says "return value should be..." after configureDhcp6Server is called. The comment is wrong, as the returned value is a pointer to a new configuration. (Incidentally, "status" seems the wrong name here, as a pointer to a new configuration is returned. Perhaps "new_config" is a better name?)

No. See documentation or implementation for configureDhcp6Server. It returns a pointer to very small element that contains rcode (integer value) and optionally a string that provides extra info (usually explains error condition is rcode!=0). Moved the status declaration closer to its usage, but kept its name.

Fair enough, but the comment says:

Returned value should be 0 (configuration success)

...but when rcode is checked, it is checked against a value of 1. (Besides which, the description of the test is that it is checking that an invalid configuration is being rejected.)

src/lib/dhcp/pkt6.cc

Your proposal wouldn't work, because in case of RELAY_SEARCH_LAST and RELAY_SEARCH_FIRST there is only one relay to check, so direction is 0 and i is not incremented. Furthermore, the code used to assume that after the loop i points to the last relay to be checked, so it would break down.

Although I didn't state it in my comment, I had assumed that direction would be set to be non-zero.

I've refactored the code a bit and it is now simpler and there are more comments around it. Please let me know if it is sufficient or should I tweak it more.

It's fine and equivalent to my suggestion - rather than modify "end" and test against the modified value, you've included the modified value in the test itself by comparing against "end + direction".

src/lib/dhcp/pkt6.h

It's OK, but why move the definition of Pkt6Ptr before the definition of Pkt6? It only necessitates the addition of a forward declaration.

Please move it back and see what happens :)

OK, I missed that one.

Since methods have been added to search the relay information and to copy relay information, should relay_info_ still be a public field?

Yes. Otherwise you get into the object lifetime mess. If you implement a method that hides it, will you return const references?

You still have the lifetime problem whether the field is public or accessible via a method. In the former case, you may well retain a reference or a pointer to the field, in which case it becomes invalid as soon as the object is destroyed.

I personally think about Pkt{4,6} not as a proper class, but rather a structure with some extra methods specified for convenience.

That's a valid approach, in which case it should be declared as "struct". (No difference in C++ terms, but I think it would imply that it is a data carrier). Let's not change it now, but we should revisit this later: it is inconsistent to have the relay information public, whilst other data is private.

src/lib/dhcpsrv/cfgmgr.cc

This seemingly simple function would be then 5 indentation levels deep then. Simple return statement is not that much. Can we keep it as it is?

Yes, we can keep it as it is.

The only changes I'm suggesting in this review is the comments in config_parser_unittest.cc. After than is changed, the code can be merged - I don't need to see it again.

comment:8 Changed 7 years ago by tomek

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

config_parser_unittest.cc comments updated, also added Changelog (its entry was reviewed on dhcp chatroom).

Code merged to master, ticket closed.

Note: See TracTickets for help on using tickets.