BIND 10 #3274: Kea4/6: use client classification in subnet selection (classification, part 2)
BIND 10 Development
do-not-reply at isc.org
Wed Feb 5 17:09:33 UTC 2014
#3274: Kea4/6: use client classification in subnet selection (classification, part
2)
-------------------------------------+-------------------------------------
Reporter: tomek | Owner: tomek
Type: enhancement | Status:
Priority: high | reviewing
Component: dhcp | Milestone: DHCP-
Keywords: | Kea0.9-alpha
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 39 | Feature Depending on Ticket:
| Add Hours to Ticket: 7
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* hours: 32 => 7
* owner: marcin => tomek
* totalhours: 32 => 39
Comment:
Reviewed commit b94b3c71b79baf0707e984fd97b11036a04bcc23
'''General'''
Please update copyright headers.
There are some doxygen errors related to this ticket.
'''doc/guide/bind10-guide.xml'''
A few editorial nits that refer to both v4 and v6 part:
OLD:
The DHCPv4 component is colloquially referred to as Kea4 and its DHCPv6 is
called Kea6.
NEW:
The DHCPv4 component is colloquially referred to as Kea4 and its DHCPv6
'''counterpart''' is called Kea6.
OLD:
''It is envisaged that the client classification will be used for changing
behavior of almost any part of the DHCP engine processing, including
assigning leases from different pools, assigning different option (or
different values of the same options) etc''
NEW:
It is envisaged that the client classification will be used for changing
behavior of almost any part of the DHCP '''message''' processing,
including assigning leases from different pools, assigning different
option (or different values of the same options) etc
OLD:
The primary use case for such a scenario are cable networks
NEW:
The primary use case for such a scenario '''is''' cable networks
OLD:
In certain cases it beneficial to restrict access to certains subnets only
to clients that belong to a given subnet
NEW:
In certain cases it beneficial to restrict access to '''certain''' subnets
only to clients that belong to a given subnet
'''src/bin/dhcp4/dhcp4.spec'''
It looks a little odd, that !''ip-address!'' item is not optional (as
indicated by !''item_optional!'' field) but the description says it is
optional. The description could be modified to say that !''... relay
address defaults to 0.0.0.0 which causes server to ignore this
parameter!''. Or so...
'''src/bin/dhcp4/dhcp4_srv.cc'''
The change in the server logic is not covered by unit test. Is it TBD
after #3322 is done?
'''src/bin/dhcp6/dhcp6.dox'''
You must have copy-pasted text from dhcp4.dox and you haven't changed 4 to
6.
'''src/bin/dhcp6/dhcp6.spec'''
The same comment as for dhcp4.spec.
'''src/bin/dhcp6/dhcp6_srv.cc'''
The change in the server logic is not covered by unit test. Is it TBD
after #3322 is done?
'''src/lib/dhcp/classify.h'''
It is smart to derive from std::set and I suppose there is no issue with
this concept. However, we may need some more clarity in the doxygen
documentation how this new class is supposed to be used. In particular:
- It should be crystal clear that this class inherits from std::set and
its accessors and modifiers should be used to access and modify elements.
Doxygen makes a poor attempt to provide this documentation and it gives a
link to STL documenentation on my local computer which leads nowhere (see
!''Additional Inherited Members!'' section in class documentation). If you
think it is a bad idea to provide a link like this:
http://www.cplusplus.com/reference/set/set/, maybe at least there should
be a sentence saying: !''see STL documentation for std::set members which
you have to use to access elements in this container!''. Or so....
- I suppose it is possible to add an empty string to a std::set. I am not
sure if there are any validation criteria for a class name already (or a
class in general) but it may be stated in the class description that the
caller is responsible for validating correctness of the class. In fact,
this problem could be sorted out on the class object level at some point
if we changed this:
{{{
typedef std::string ClientClass;
}}}
to
{{{
class ClientClass {
public:
ClientClass(const std::string& class_name) {
validateStuff(class_name);
}
}
}}}
but for the time being, I am not sure if this is the plan.
Some nits:
- missing @brief in the constructor's description
- The !''X!'' in the should be lower case as it refers to the function
argument which is lower case
- There are no unit tests for the new class. I realize that this class has
trivial implementation but as stated in the comment: !''It is simple for
now, but the complexity involved in client classification is expected to
grow significantly!''. It would be good to at least have unit test
template with one simple test, testing contains function as a base for
extending the class in the future.
- The contains could be simplified
{{{
bool
contains(const ClientClass& x) const {
return (find(x) != end());
}
}}}
Whether you fix the last one is completely up to you.
There is one point about the todo comment I have. The term !''server-
side!'' implementation is confusing as it is unclear what server-side
implementation in libdhcp++ you're referring to. Perhaps we should just
say that !''!ClientClass is currently used by the !Pkt4 and !Pkt6 classes
which precludes move of this class to libdhcpsrv.
This actually brings a question if classification should be purely a
server-side concept or it may be generic. You seem to be suggesting that
classification is server-side only, which means that at some point we will
have to move !Pkt4::addClass and alike out of !Pkt4. This in turn means
that we will have to implement something along the lines like !ServerPkt4
deriving from !Pkt4. In such case it would be probably rational to put a
todo in the !Pkt4 class or even better: a ticket.
The other possible solution would be to keep client classification code in
!Pkt4 and assume that it may be used for clients and relays or simply
unused. In this case, the todo in classify.h becomes irrelevant.
I don't have any strong opinion which way to go, but I guess we either
need a ticket for moving out the classification from !Pkt4 class or change
the todo in classify.h.
'''src/lib/dhcp/pkt4.h'''
As you're migrating from simple set of strings to the !ClientClasses, I
suggest that you change the type of the argument passed to addClass and
inClass to !ClientClass.
Nit: please update copyright headers.
'''src/lib/dhcp/pkt6.h'''
The same comments as for !Pkt4.
'''src/lib/dhcpsrv/cfgmgr.cc'''
Question: I spent a while trying to figure out how the classification will
work for single relay serving cable modems and routers that belong to two
different subnets. The getSubnet4 will currently return empty pointer if
the relay address is not in range. But I guess, this is the thing that
will be implemented as #3322?
'''src/lib/dhcpsrv/cfgmgr.h'''
None of the new ''classes'' parameters passed to getSubnet4 and getSubnet6
are documented. Also, the descriptions of these functions must be extended
to give an idea of classification.
'''src/lib/dhcpsrv/dhcp_parsers.cc'''
Line 814: It should be !RelayInfoParser, not !PoolParser.
The parser should validate whether the IP address is v4 or v6. If I
understand correctly, the v6 relay address doesn't make sense for DHCPv4
server and vice versa. The global_context_->universe carries necessary
information to check whether we are in the v4 or v6 context. In the
future, the data structure may be extended so as more sophisticated
validation may be required and we should probably have some methods in
Subnet to handle that.
Nit: When the parser is given a bogus IP address the IOAddress constructor
will throw an exception. Typically, parsers throw Dhcp4ConfigError
exception but the IOAddress constructor will throw an exception of a
different type. This shouldn't be a problem because (I think) all
exceptions are caught. But for consistency the !RelayInfoParser could
throw an exception. It is up to you.
'''src/lib/dhcpsrv/dhcp_parsers.h'''
A couple nits:
- RelayInfoParser::build: typo in!''strcture!''.
- instead of !''dummy!'' you could name the unused parameter !''unused!''
or !''ignored!'' - up to you.
- createConfigParser has no documentation
'''src/lib/dhcpsrv/subnet.h'''
The member which holds relay information is public and I believe it should
be private or protected. One can argue that it is simpler to expose a
public field instead of providing getters and setters for each member.
But, I think there is a lot of sense in providing a setter if a parameter
being set is a structure which doesn't validate its fields. Currently the
!RelayInfo structure holds an IP address but it is already questionable
whether it should be possible to set the IPv6 address as a relay address
for IPv4 subnet. It is possible with the current code because neither
!RelayInfo nor setRelay validate it. If we extend !RelayInfo structure
there will be more cases to validate. So, if we want to stick to having
!RelayInfo as a structure, we probably need to extend setRelay to validate
that the structure is acceptable. But, if we have setRelay function it
doesn't make much sense to expose relay_ in a public scope because
everybody can bypass the validation checks.
On the related matter... should !RelayInfo be the same for !Subnet4 and
!Subnet6? Even if it should, maybe Subnet4 and Subnet6 should provide
different implementations of setRelay function to validate that the relay
information is valid for a particular subnet type?
allowClientClass: The documentation says that this function '''adds''' a
class to the list of supported classes. It doesn't add. It replaces the
current class name with a new class name.
The following member:
{{{
ClientClass white_list_;
}}}
has confusing name, i.e. everything that is called a list sounds like it
contains multiple elements in it. In fact, it merely contains a single
name.
A few nits:
- brief description of setRelay: Currently it is true, but if you extend
the structure I doubt that it will be updated to say: !''Sets relay
information!'', instead of !''Sets address of the relay!''.
- The same comment as above relates to @params tag.
- It should be @param, not @params.
'''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''
Not really related to this work because there is a lot of those in the
code:
{{{
EXPECT_EQ(subnet2, cfg_mgr.getSubnet6(ifaceid2, classify_));
}}}
This will check that the pointers match. I wonder if we should rather have
a == operator for subnet and check that subnets are equal, not necessarily
that pointers are the same.
'''src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc'''
There should be a test for invalid ip address too.
--
Ticket URL: <https://bind10.isc.org/ticket/3274#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list