Opened 6 years ago

Closed 6 years ago

#3274 closed enhancement (complete)

Kea4/6: use client classification in subnet selection (classification, part 2)

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea0.8
Component: dhcp 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: 2.5
Total Hours: 56 Internal?: no

Description (last modified by tomek)

Once #3203 is implemented, the packets will be segregated to classes. This ticket is about using that class information when choosing subnets.

One specific use case is to allow cable modems (docsis class) and eRouters to be in separate subnets.

Note: Due to high conflict likelihood with #3242, this ticket covers both client classification and relay configuration in subnets, but only classification is currently used. The relay information is ignored. It will be implemented in a follow up ticket: #3322.

Note: There is a follow up ticket (#3275) to extract cable/docsis specific operation to external hook library.

Subtickets

Change History (18)

comment:1 Changed 6 years ago by tomek

  • Description modified (diff)

comment:2 Changed 6 years ago by tomek

  • Summary changed from Kea4/6: use client classification in subnet selection to Kea4/6: use client classification in subnet selection (classification, part 2)

comment:3 Changed 6 years ago by tomek

  • Milestone changed from DHCP-Kea1.0-proposed to DHCP-Kea1.0-alpha

Moving to DHCP-Kea1.0-alpha as agreed on Kea status meeting (2014-01-08)

comment:4 Changed 6 years ago by tomek

  • Owner set to tomek
  • Priority changed from medium to high
  • Status changed from new to assigned

comment:5 Changed 6 years ago by tomek

  • Description modified (diff)

comment:6 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 0 to 32
  • Owner changed from tomek to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 32

The following features has been implemented:

  • client class configuration in subnets
  • relay information configuration in subnets
  • client classification logic
  • several cosmetic fixes in BIND10 User's Guide (which were too small for a stand alone ticket)

What was not implemented (moved to #3322):

  • usage of relay information configuration

Please review. This ticket implements the results of a discussion
we had recently about client classification. I was considering
whether the conclusion of that discussion should be converted into
a design document, but we already have #3307 that is deferred
until after 0.9-beta.

comment:7 Changed 6 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:8 follow-ups: Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 32 to 7
  • Owner changed from marcin to tomek
  • Total Hours changed from 32 to 39

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.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 7 to 4
  • Total Hours changed from 39 to 43

Replying to 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.

comment:10 in reply to: ↑ 8 ; follow-up: Changed 6 years ago by tomek

  • Owner changed from tomek to marcin

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.

Added validation. I decided to follow the same path as ParserContext? - pass Option::Universe and then make decisions based on 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.

Implemented. Extended tests. They now check if specifying address of the wrong type will throw exception. Also check if bogus address will throw.

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

Added.

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.

You are right regarding the setters, but I'm more concerned with getters that are much more frequently used. What do you think the getter should return? Shared pointer? Then you're complicating both the structure access method and making it slower.

I think the parameter verification is sufficiently checked at the parsing time. If you disagree, I can then turn RelayInfo? into a class, make the addr_field protected, and implement getters/setters for it.

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?

Yes. This can be the same structure with the same parser. Even if we decide to extend it, we still can have a single RelayInfo? object. That's was the huge parser refactor was about, isn't it? So let's try to keep things unified as long as we can.

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.

Because for now it is a single class, but it will grow into two lists (list of allowed classes,
list of forbidden classes) eventually. So I decided to keep the interface forward compatible,
only the implementation will have to change. But I agree that it looks confusing, so added
an explanatory note to white_list_ and a reference to it in allowClientClass().

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.

Updated.

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.

We can create a ticket for it, but I don't expect anyone would look at it anytime soon. Excessive number of such tickets is one of the reasons that doomed recursive resolver project.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
There should be a test for invalid ip address too.

Added.

Thanks for thorough review.
Back to you, Marcin.

comment:11 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 4 to 6
  • Total Hours changed from 43 to 49

comment:12 in reply to: ↑ 9 Changed 6 years ago by marcin

Replying to tomek:

src/lib/dhcp/pkt6.h
The same comments as for !Pkt4.

Done.

The functions inClass and addClass still use the std::string instead of the ClientClass type.

comment:13 in reply to: ↑ 10 ; follow-up: Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 6 to 1
  • Total Hours changed from 49 to 50

Replying to tomek:

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.

Added validation. I decided to follow the same path as ParserContext? - pass Option::Universe and then make decisions based on that.

Although, normally I am picky about use of consts. Adding a const in front of Option::Universe may cost you cppcheck warnings on some systems. The cppcheck for some reason wants const enums be passed by reference, like any other class.

I added a space in one of the exception strings to make exception string more readable.

src/lib/dhcpsrv/dhcp_parsers.h
A couple nits:

  • RelayInfoParser::build: typo in''strcture''.

This was not addressed, so I now fixed it myself.

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.

You are right regarding the setters, but I'm more concerned with getters that are much more frequently used. What do you think the getter should return? Shared pointer? Then you're complicating both the structure access method and making it slower.

Why not return the const reference to relay_? It is sufficiently fast and precludes modification of the object. And I don't really see a big problem in the fact that the reference invalidates when the object is destroyed. We do it in many places and it has never been a real problem. It's been rather theoretical.

I think the parameter verification is sufficiently checked at the parsing time. If you disagree, I can then turn RelayInfo? into a class, make the addr_field protected, and implement getters/setters for it.

It is checked, I agree. However, if we start adding new members to this structure it would be better to do validation on the structure side, on the parser's side. The parser's code is already overly complex and any more sophisticated validation would make it more complex. This is opposed to calling a single method of RelayInfo? like: RelayInfo::validateData(). So, I think that turning this structure into a class that has some validation capability makes sense but I don't consider it critical now.

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.

Because for now it is a single class, but it will grow into two lists (list of allowed classes,
list of forbidden classes) eventually. So I decided to keep the interface forward compatible,
only the implementation will have to change. But I agree that it looks confusing, so added
an explanatory note to white_list_ and a reference to it in allowClientClass().

I still don't get that. Per documentation of ClientClass, it represents a single class. There is a ClientClasses type which holds a list of classes. So, are you saying that ClientClass? will evolve to multiple classes and ClientClasses will hold a list of list of classes? If ClientClass is going to evolve to collection of client's classes, we may need to rename it now to indicate that it will be a list and add a note that even though it is a list, there may be just one class on the list at the moment and the rest is ignored, or whatever.

comment:14 Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

I fixed two little typos in the code, so please pull from the repo.

comment:15 in reply to: ↑ 13 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 1 to 3
  • Owner changed from tomek to marcin
  • Total Hours changed from 50 to 53

Replying to marcin:

Replying to tomek:

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.

Added validation. I decided to follow the same path as ParserContext? - pass Option::Universe and then make decisions based on that.

Although, normally I am picky about use of consts. Adding a const in front of Option::Universe may cost you cppcheck warnings on some systems. The cppcheck for some reason wants const enums be passed by reference, like any other class.

Changed to reference.

I added a space in one of the exception strings to make exception string more readable.

Thanks.

src/lib/dhcpsrv/dhcp_parsers.h
A couple nits:

  • RelayInfoParser::build: typo in''strcture''.

This was not addressed, so I now fixed it myself.

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.

You are right regarding the setters, but I'm more concerned with getters that are much more frequently used. What do you think the getter should return? Shared pointer? Then you're complicating both the structure access method and making it slower.

Why not return the const reference to relay_? It is sufficiently fast and precludes modification of the object. And I don't really see a big problem in the fact that the reference invalidates when the object is destroyed. We do it in many places and it has never been a real problem. It's been rather theoretical.

Ok. Added getRelayInfo() that returns const reference and the relay_ member is now protected. Also renamed getRelay to getRelayInfo().

It is checked, I agree. However, if we start adding new members to this structure it would be better to do validation on the structure side, on the parser's side. The parser's code is already overly complex and any more sophisticated validation would make it more complex. This is opposed to calling a single method of RelayInfo? like: RelayInfo::validateData(). So, I think that turning this structure into a class that has some validation capability makes sense but I don't consider it critical now.

We will turn it into a class when more parameters appear in it.

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.

Because for now it is a single class, but it will grow into two lists (list of allowed classes,
list of forbidden classes) eventually. So I decided to keep the interface forward compatible,
only the implementation will have to change. But I agree that it looks confusing, so added
an explanatory note to white_list_ and a reference to it in allowClientClass().

I still don't get that. Per documentation of ClientClass, it represents a single class. There is a ClientClasses type which holds a list of classes. So, are you saying that ClientClass? will evolve to multiple classes and ClientClasses will hold a list of list of classes? If ClientClass is going to evolve to collection of client's classes, we may need to rename it now to indicate that it will be a list and add a note that even though it is a list, there may be just one class on the list at the moment and the rest is ignored, or whatever.

Further clarified the @todo text. ClientClass? is and will remain a single class. ClientClasses? is a collection of classes with possibly extra quantifiers added later.

The intention here is to eventually change white_list_ into ClientClasses?. There is a reason to avoid it for now, though. Right now the algorithm checks whether a collection of classes (that client's message belongs to) meet a single class. This is simple: iterate over classes and find a match. If we promote white_list_ to ClientClasses?, we will have to implement a inclusion relationship check of two sets. (To check whether each element from set A also belongs to set B). This is not overly complex, but has some corner cases (what if the expected classes list is empty? It means accept for black_list, but means reject for white_list, unless there's nothing on black_list, which means that no access control is configured, hence means accept). So this starts to be a bigger task, like a separate ticket that can stand on its own. Hence white_list_ is a ClientClass? now, but will be promoted to ClientClasses? eventually.

I hope that explanation is satisfactory.

comment:16 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 3 to 0.5
  • Owner changed from marcin to tomek
  • Total Hours changed from 53 to 53.5

Reviewed commit 0c74cff26aa147bd1f8b807b441d86a064a90f60

Changed are acceptable. Please merge.

comment:17 Changed 6 years ago by marcin

Reviewed commit 1791d19899b92a6ee411199f664bdfc690ec08b2

Please merge.

comment:18 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 0.5 to 2.5
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 53.5 to 56

Thanks for the review.

It was a bit tricky merge. After #3242, I needed to update couple files. Then after the #3242 logic change one of the tests started to fail. When I finally made it, #3299 was merged in the means time. Another quick merge and here it is. #3274 pushed on master.

Closing ticket.

Note: See TracTickets for help on using tickets.