BIND 10 #2898: DHCPv6 server must handle relayed traffic
BIND 10 Development
do-not-reply at isc.org
Fri May 3 13:13:59 UTC 2013
#2898: DHCPv6 server must handle relayed traffic
-------------------------------------+-------------------------------------
Reporter: tomek | Owner: tomek
Type: defect | Status:
Priority: medium | reviewing
Component: dhcp6 | Milestone:
Keywords: | Sprint-DHCP-20130509
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by stephen):
* owner: stephen => tomek
Comment:
Reviewed commit df3ee4b9e04459e2ba6dbc50702b3c7f29c0ceb7
'''src/bin/dhcp6/config_parser.cc'''[[BR]]
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'''[[BR]]
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'''[[BR]]
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'''[[BR]]
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'''[[BR]]
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'''[[BR]]
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'''[[BR]]
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'''[[BR]]
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'''[[BR]]
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'''[[BR]]
DHCPSRV_CFGMGR_SUBNET6_IFACE_ID: s/in subnet6 definition/in the subnet6
definition/
'''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''[[BR]]
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'''[[BR]]
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>
}}}
--
Ticket URL: <http://bind10.isc.org/ticket/2898#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list