#5425 closed enhancement (complete)

Add classes to pools

Reported by: fdupont Owned by: fdupont
Priority: high Milestone: Kea1.4
Component: classification Version: git
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

There is a consensus to add client-class to pools (similar to subnets but at the pool level) at a high priority.

Subtickets

Change History (27)

comment:1 Changed 15 months ago by fdupont

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

comment:2 Changed 15 months ago by fdupont

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

Almost done. There are a few tests I shall add and one which does not work but they are corner cases so IMHO the branch is ready for a review.
Two related questions:

  • there is no inheritance between the subnet and its pools. In fact it is useless as nothing is handled only at the pool level. Of course pools have the priority and about classification it can lead to funny cases...
  • the current subnet classification code I copied handles zero or one class. There is a comment about white and black lists (aka allow/deny) but it is not implemented. BTW it is more a question of syntax than API as the API itself is generic enough. IMHO we should provide an easy way to combine classes using boolean operators and give up on allow/deny stuff (cf classification tickets).

comment:3 Changed 15 months ago by fdupont

Completed and added new tests. Note there is still a bug in a corner case: renewal after a restriction change (it works for subnets but not (yet) for pools, and I have no idea about where to add the missing check).
Still to be reviewed.

comment:4 Changed 15 months ago by fdupont

Fixed the corner case issue. BTW if someone removes during a reconfig a DHCPv6 pool stats become wrong... IMHO it is not a problem but if you think it is please open a ticket.
Still to be reviewed.

comment:5 Changed 14 months ago by fdupont

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

Took the ticket to add support to know-clients (i.e., clients which have a host reservation) as it is heavily used in ISC DHCP (so a priori useful) and trivial to implement.
Last commit is 29c00734a010dc09b5d30aaa82b3b7926deace77

Last edited 14 months ago by fdupont (previous) (diff)

comment:6 Changed 14 months ago by fdupont

Added known-clients stuff. Ready again for review.

comment:7 Changed 14 months ago by fdupont

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

comment:8 Changed 14 months ago by marcin

  • Owner changed from UnAssigned to marcin

comment:9 follow-ups: Changed 14 months ago by marcin

  • Owner changed from marcin to fdupont

I reviewed the following commit 13a8becc1b8cc6751a43fbae75a37c61f4988b3f

I added a couple of little fixes, so please pull.

doc/examples/kea4/advanced.json
The reservations provided in this example only specify MAC addresses of the clients. The example doesn't explain well enough why only MAC addresses are specified. Should the administrator extend these reservations with some static values, such as IP address or hostname, or it is fine to keep the MAC address list and these clients will be allocated addresses from the "known-clients": "only" pool.

doc/examples/kea4/classify.json
The comments starting with "This one is for VoIP devices only" and the one below should be indented to the pools they describe.

Also, the "subnet" and "interface" indentation is wrong.

doc/examples/kea6/advanced.json

Same comment as for the v4 counterpart.

doc/examples/kea6/classify.json

doc/guide/classify.xml
I made some edits in this text, so please pull my changes.

The section describing client classification for pools lacks an example for DHCPv6 case. We have such example for client class specified for a subnet. Providing an example for DHCPv4 pool only creates an impression that it is not supported for v6.

doc/guide/dhcp4-srv.xml
doc/guide/dhcp6-srv.xml

I made some edits in the classification section for DHCPv4 and DHCPv6, so please pull.

The following new paragraph doesn't seem to fit into the rest of the text. I was very confused what was the primary thought conveyed by this new text. Is this describing a flaw in the client classification when the subnet belongs to a shared network? Or, it points me to the fact that classification can be used for pools?

      When subnets belong to a shared network the classification applies
      to subnet selection but not to pools, e.g., a pool in a subnet
      limited to a particular class can still be used by clients which do not
      belong to the class if the pool they are expected to use is exhausted.
      So the limit access based on class information is also available
      at the pool level, see <xref linkend="classification-pools"/>.

The text is confusing because it seems to refer to pool exhaustion, while it is in fact exhaustion of addresses in the subnet belonging to a shared network? But don't we protect against this by checking that the subnet in the shared network can't be used by the given client because it doesn't belong to a class? I think I am missing the point of this whole issue in the context of shared networks.

I'd rather start with saying something like:

      Client classification can also be used to restrict access to specific
      pools within a subnet. This is useful when to segregate clients belonging
      to the same subnet into different address ranges.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

Shouldn't there also be a test verifying that, for the client having a reservation, an address from the pool with known-clients=only is allocated?

Shouldn't these tests be moved to classify_unittest.cc? That would also be consistent with where v6 are implemented.

src/bin/dhcp4/tests/shared_network_unittest.cc

It might be good to have a test in which a client performs DORA without a hint. The server should locate appropriate pool for this client.

Plain subnet unit tests (configuration 18 and 19) should be moved elsewhere. Possibly to classify_unittest.cc?

src/bin/dhcp6/tests/classify_unittests.cc

Creation of the query object could be better moved to a function as there seem to be a lot of copy pasting in clientClassifyPool and clientKnownPool.

src/bin/dhcp6/tests/shared_network_unittest.cc

It might be good to have a test in which a client performs 4-way exchange without a hint. The server should locate appropriate pool for this client.

Plain subnet unit tests (configuration 21 and 22) should be moved elsewhere. Possibly to classify_unittest.cc?

src/lib/dhcpsrv/pool.h
I don't like the name of the new enum KnownClients, as it suggests that it is a container holding "known clients". I suggest renaming it to something like ServedClientsType?.

As a consequence, the getKnownClients and setKnownClients should be renamed.

There are the following doxygen errors reported:

/Users/marcin/devel/kea/src/lib/dhcpsrv/pool.h:252: warning: unable to resolve reference to `isc::dhcp::subnet::last_allocated_ia_' for \ref command
warning: Included by graph for 'exceptions/exceptions.h' not generated, too many nodes. Consider increasing DOT_GRAPH_MAX_NODES.
/Users/marcin/devel/kea/src/lib/dhcpsrv/alloc_engine.h:225: warning: The following parameters of isc::dhcp::AllocEngine::RandomAllocator::pickAddress(const SubnetPtr &subnet, const ClientClasses &client_classes, bool known_client, const DuidPtr &duid, const isc::asiolink::IOAddress &hint) are not documented:
  parameter 'client_classes'
  parameter 'known_client'
/Users/marcin/devel/kea/src/lib/dhcpsrv/pool.h:252: warning: unable to resolve reference to `isc::dhcp::subnet::last_allocated_ia_' for \ref command
/Users/marcin/devel/kea/src/lib/dhcpsrv/pool.h:252: warning: unable to resolve reference to `isc::dhcp::subnet::last_allocated_ia_' for \ref command
/Users/marcin/devel/kea/src/lib/dhcpsrv/pool.h:252: warning: unable to resolve reference to `isc::dhcp::subnet::last_allocated_ia_' for \ref command
/Users/marcin/devel/kea/src/lib/dhcpsrv/pool.h:252: warning: unable to resolve reference to `isc::dhcp::subnet::last_allocated_ia_' for \ref command

comment:10 follow-up: Changed 13 months ago by tomek

I'd like to review this change in more details, but for now I have two comments.

The first one is about known-clients. I don't think we need a separate mechanism to express that. It should be sufficient to add packets to "known-client" or "unknown-client" and then use normal class mechanism. This way, when the other change about improving class logic is in place, you could use this class the same way as anything else.

My second comment is about documentation. I rebuilt user's guide, but couldn't find any explanations there how to use classes on pool level.

comment:11 in reply to: ↑ 10 Changed 13 months ago by fdupont

Replying to tomek:

I'd like to review this change in more details, but for now I have two comments.

The first one is about known-clients. I don't think we need a separate mechanism to express that. It should be sufficient to add packets to "known-client" or "unknown-client" and then use normal class mechanism. This way, when the other change about improving class logic is in place, you could use this class the same way as anything else.

=> I can answer about this point: I believed a class should do the job but I failed to find a way to write the corresponding code so I made it an extension of the class mechanism instead...

comment:12 in reply to: ↑ 9 Changed 13 months ago by fdupont

Replying to marcin:

I reviewed the following commit 13a8becc1b8cc6751a43fbae75a37c61f4988b3f

I added a couple of little fixes, so please pull.

=> pull and reviewed.

doc/examples/kea4/advanced.json
The reservations provided in this example only specify MAC addresses of the clients. The example doesn't explain well enough why only MAC addresses are specified.

=> there is no other reasons than the hardware address is the most common identifier for DHCPv4 host reservations...

Should the administrator extend these reservations with some static values, such as IP address or hostname,

=> in fact I believe one of IP address or hostname is required so if the syntax is correct these host reservations will fail to be inserted to the system host map. I am fixing this (adding of course hostnames as I still want addresses from the pool).

or it is fine to keep the MAC address list and these clients will be allocated addresses from the "known-clients": "only" pool.

=> it is the goal so as you ask it must be clarified.

doc/examples/kea4/classify.json
The comments starting with "This one is for VoIP devices only" and the one below should be indented to the pools they describe.

Also, the "subnet" and "interface" indentation is wrong.

=> fixed

doc/examples/kea6/advanced.json

Same comment as for the v4 counterpart.

=> fixed too.

doc/examples/kea6/classify.json

doc/guide/classify.xml
I made some edits in this text, so please pull my changes.

=> pull and reviewed

The section describing client classification for pools lacks an example for DHCPv6 case. We have such example for client class specified for a subnet. Providing an example for DHCPv4 pool only creates an impression that it is not supported for v6.

=> I added a DHCPv6 example.

doc/guide/dhcp4-srv.xml
doc/guide/dhcp6-srv.xml

I made some edits in the classification section for DHCPv4 and DHCPv6, so please pull.

=> done

The following new paragraph doesn't seem to fit into the rest of the text. I was very confused what was the primary thought conveyed by this new text. Is this describing a flaw in the client classification when the subnet belongs to a shared network? Or, it points me to the fact that classification can be used for pools?

      When subnets belong to a shared network the classification applies
      to subnet selection but not to pools, e.g., a pool in a subnet
      limited to a particular class can still be used by clients which do not
      belong to the class if the pool they are expected to use is exhausted.
      So the limit access based on class information is also available
      at the pool level, see <xref linkend="classification-pools"/>.

The text is confusing because it seems to refer to pool exhaustion, while it is in fact exhaustion of addresses in the subnet belonging to a shared network? But don't we protect against this by checking that the subnet in the shared network can't be used by the given client because it doesn't belong to a class? I think I am missing the point of this whole issue in the context of shared networks.

I'd rather start with saying something like:

      Client classification can also be used to restrict access to specific
      pools within a subnet. This is useful when to segregate clients belonging
      to the same subnet into different address ranges.

=> in fact my text is wrong too. I replaced it by yours in the two guides.

Other comments are about the code so I'll put them in another reply.

comment:13 in reply to: ↑ 9 Changed 13 months ago by fdupont

Replying to marcin:

=> Second part about the code.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

Shouldn't there also be a test verifying that, for the client having a reservation, an address from the pool with known-clients=only is allocated?

=> do you mean the symmetric of clientPoolKnown? Added.

Shouldn't these tests be moved to classify_unittest.cc? That would also be consistent with where v6 are implemented.

=> the problem is that unit test files come with a lot of context including fixtures so it is not trivial to move a unit test between two files. In fact before adding a unit test I tried to find a file where it can be added with too many changes. Now I agree we should create a ticket to reorganize unit tests as we already did in the past.

src/bin/dhcp4/tests/shared_network_unittest.cc

It might be good to have a test in which a client performs DORA without a hint. The server should locate appropriate pool for this client.

=> only client1 uses a hint so I don't understand what you want.

Plain subnet unit tests (configuration 18 and 19) should be moved elsewhere. Possibly to classify_unittest.cc?

=> I already answered.

src/bin/dhcp6/tests/classify_unittests.cc

Creation of the query object could be better moved to a function as there seem to be a lot of copy pasting in clientClassifyPool and clientKnownPool.

=> done.

src/bin/dhcp6/tests/shared_network_unittest.cc

It might be good to have a test in which a client performs 4-way exchange without a hint. The server should locate appropriate pool for this client.

=> same answer than for DHCPv4 (the hint is not at the same place but again only client1 has a hint).

Plain subnet unit tests (configuration 21 and 22) should be moved elsewhere. Possibly to classify_unittest.cc?

=> same.

src/lib/dhcpsrv/pool.h
I don't like the name of the new enum KnownClients, as it suggests that it is a container holding "known clients". I suggest renaming it to something like ServedClientsType?.

As a consequence, the getKnownClients and setKnownClients should be renamed.

=> I'll move to ServedClientKind? (not yet as it is late).

There are the following doxygen errors reported:

=> I'll check a make devel too.

I committed the test changes I've done.

comment:14 Changed 13 months ago by fdupont

  • Owner changed from fdupont to marcin

comment:15 Changed 13 months ago by fdupont

Finished to address comments (at least what I understood :-). KnownClients was renamed and doxygen is happy.

comment:16 Changed 12 months ago by fdupont

BTW I remember at least a reason I did not simply use a class: there are 3 possible values (known clients only, unknown clients only, both known and unknown clients) and the branch which supports negative classes is at lower priority in the review queue and only applies to classification expression. So it is not possible to catch the idea with one class at this time. Note I still propose to revisit this when #5374 will be reviewed in the case we can find an easy optimization.

comment:17 Changed 12 months ago by tomek

  • Owner changed from marcin to tomek

As discussed with Marcin, taking over this ticket's review.

comment:18 Changed 12 months ago by fdupont

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

Decision taken: the know client code is removed so the new branch (trac5425a) will include only the addition of classes to pools as what is in the ticket description.

comment:19 Changed 12 months ago by tomek

The code in this ticket does two things: classes added to pools and known/unknown clients, which are two related, but independent features. The majority of concerns is related to known/unknown clients.

Here's the plan that Francis and I came up with (and discussed it with Marcin, who agrees):

  1. remove known-clients from trac5425, merge the remaining code;
  2. review/merge the #5374 that does class revamp
  3. once both are done, implement known-clients in a separate ticket. the storage for it should use classes. This ticket will be added to 1.4.

comment:20 Changed 12 months ago by fdupont

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

Ready for review (branch trac5425a).

comment:21 Changed 12 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:22 follow-up: Changed 12 months ago by tomek

  • Owner changed from tomek to fdupont

Thanks a lot for moving the code to a new branch. I have reviewed it and it looks good.

There is one thing that bothered me in the code. The structures for holding class information and all the logic are prepared to handle multiple classes. The only thing that doesn't are the parsers. So I decided to spend couple hours and tweak the code.

It is possible to define a list of classes that are allowed to use one specific pool. This will be super useful for several users who requested this.

Please pull and review. If you agree with my changes, this code is ready for merge.

This absolutely requires a changelog. Haven't seen anything proposed, so here's mine:

13xx.	[func]		fdupont
	It is now possible to specify client classification restrictions
	on per pool basis. This capability will be useful for grouping
	certain types of devices into specific address and/or prefix
	pools.
	(Trac #5425, git tbd)

comment:23 Changed 12 months ago by tomek

  • Component changed from Unclassified to classification

comment:24 in reply to: ↑ 22 Changed 12 months ago by fdupont

Replying to tomek:

There is one thing that bothered me in the code. The structures for holding class information and all the logic are prepared to handle multiple classes. The only thing that doesn't are the parsers. So I decided to spend couple hours and tweak the code.

=> Argh! I am afraid you wasted a couple hours because this issue is addressed in the other class ticket in a better way: instead of using a white list or white and black lists it allows boolean combination of classes. More powerful, IMHO more user friendly and clearly simpler to implement...
(BTW I decided to do things this way because the ISC DHCP allow/deny is not trivial to understand even after doc and code reading. So if I had a problem with it I needed to find a better solution for Kea).

comment:25 Changed 12 months ago by tomek

As discussed on jabber today, there is a consensus that the client-classes is not a preferred solution. Since I reviewed the earlier code, please merge all changes except the last two commits I made (i.e. merge all up to and including eb9efffcb0dcac06fb26afac719bf86cfc388e40).

comment:26 Changed 12 months ago by fdupont

I backtracked the 2 extra commits and reviewed them to include not client-classes fixes. I am checking it still builds and passes tests and after I'll merge. I committed the trac5425a branch in the case someone wants to look at them (surely faster than my iMac).
BTW even I have nothing against :
for (auto pool : pools) ...
I am afraid it won't be accepted by all compilers so if you really want to adopt this style please add a check in the m4 file for C++11.

comment:27 Changed 12 months ago by fdupont

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

Merged, closing!

Note: See TracTickets for help on using tickets.