BIND 10 #3274: Kea4/6: use client classification in subnet selection (classification, part 2)
BIND 10 Development
do-not-reply at isc.org
Thu Feb 6 19:39:44 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: 43 | Feature Depending on Ticket:
| Add Hours to Ticket: 4
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by tomek):
* hours: 7 => 4
* totalhours: 39 => 43
Comment:
Replying to [comment:8 marcin]:
> Reviewed commit b94b3c71b79baf0707e984fd97b11036a04bcc23
>
> '''General'''
> Please update copyright headers.
Done.
> There are some doxygen errors related to this ticket.
Fixed.
> '''doc/guide/bind10-guide.xml'''
> A few editorial nits that refer to both v4 and v6 part:
Done.
>
> '''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...
It is optional in the sense the user doesn't have to specify it. Updated.
> '''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?
Implemented one test. More unit-tests will be covered in #3322. Also note
that the classification and subnet selection logic is covered in CfgMgr
unit-tests.
> '''src/bin/dhcp6/dhcp6.dox'''
> You must have copy-pasted text from dhcp4.dox and you haven't changed 4
to 6.
Oops. Fixed. Please note that the text in one place refers to v4 calls
saying that there is no v6 equivalent.
> '''src/bin/dhcp6/dhcp6.spec'''
> The same comment as for dhcp4.spec.
Done.
> '''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?
Implemented one test. More unit-tests will be covered in #3322. Also note
that the classification and subnet selection logic is covered in CfgMgr
unit-tests.
> '''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.
There won't be any validation, as the classes can be arbitrary string.
> 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());
> }
> }}}
And miss the chance to use const_iterator without specifying its type?
Meh.
Ok, ok, updated as suggested :)
> 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.
Clarified the text a bit. Hopefully it is better readable now.
> Perhaps we should just say that !''!ClientClass is currently used by the
!Pkt4 and !Pkt6 classes which precludes move of this class to libdhcpsrv.
That is true, but that's not the core reason of it being stuck in dhcp for
now. The core reason is that the current Pkt{4,6} classes are really
ServerPkt{4,6} classes.
> This actually brings a question if classification should be purely a
server-side concept or it may be generic.
No. It is purely server-side. Client usually talks to only one server.
Added a text that tries to explain that.
> 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.
THere's a ticket for it somewhere and an agenda item during our F2F
meeting.
> 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 decided to not add such a @todo, because there are other methods that
will need to be moved (everything related to relays and classification).
> 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.
That is something we can do after doing BasePkt4->ServerPkt4 hierarchy.
> '''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.
Done.
> Nit: please update copyright headers.
Done.
>
> '''src/lib/dhcp/pkt6.h'''
> The same comments as for !Pkt4.
Done.
> '''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?
Yes. Right now the solution is only partially usable. It work right now as
a simple access control. You essentially can reject clients that do not
belong to specific classes.
> '''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.
Added extra comments.
> '''src/lib/dhcpsrv/dhcp_parsers.cc'''
> Line 814: It should be !RelayInfoParser, not !PoolParser.
Updated.
The remainder of the comments will be addressed tomorrow.
--
Ticket URL: <http://bind10.isc.org/ticket/3274#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list