Opened 3 years ago

Closed 3 years ago

#4765 closed enhancement (complete)

Client Classification: servers should utilize client classes read from host reservations database.

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.1-final
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: 1
Total Hours: 20 Internal?: no

Description

Kea 1.1.0-beta version is able to read statically assigned classes from host reservation database. Though, the servers currently don't use those classes during processing. We should the servers' logic to iterate over the classes dynamically assigned to the client and the classes read from the host reservations database. Presumably, the classes from the HR should take precedence over any other classes.

Subtickets

Change History (8)

comment:1 Changed 3 years ago by marcin

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

comment:2 Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 0 to 8
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 8

This ticket is now ready for review. Apart from the new logic in the servers, I had to also add support for client-classes parameters in the configuration parsers, as it seems we didn't support that too.

I also moved classification specific tests into new file.

Proposed ChangeLog entry:

11XX.	[func]		marcin
	DHCP servers utilize client classes defined in host reservations.
	(Trac #4765, git abcd)

comment:3 Changed 3 years ago by marcin

  • Total Hours changed from 8 to 16

comment:4 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:5 follow-up: Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 8 to 3
  • Owner changed from tomek to marcin
  • Total Hours changed from 16 to 19

I have reviewed the code and found only minor issues.

dhcp6-srv.xml
The new example has extra " at the end of the example (3 lines).

src/bin/dhcp4/tests/classify_unittest.cc
Configuration 1 should be described.

The comment in line 322 says "matches 'pxe' class, but there's
only 'reserved-class1'. The same is true for lines 330 and 336.

src/bin/dhcp6/tests/classify_unittests.cc
matchClassification, subnetClassPriority, subnetGlobalPriority,
classGlobalPriority, clientClassifySubnet, relayOverrideAndClientClass
tests have their own configs, which should be part
of the global CONFIGS array.

dhcp{4,6}.spec files
If you think it makes sense to keep updating those files, please
add item_description to the new entries.

src/bin/dhcp6/dhcp6_srv.cc
I think the setReservedClientClasses() call should be made from
initContext(). That would be one call instead of many.


The code builds and unit-tests pass on Ubuntu 16.04 x64.

With those changes applied, this ticket is ready to go. I don't
need to see it again.

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

  • Owner changed from marcin to tomek

Replying to tomek:

I have reviewed the code and found only minor issues.

dhcp6-srv.xml
The new example has extra " at the end of the example (3 lines).

Removed.

src/bin/dhcp4/tests/classify_unittest.cc
Configuration 1 should be described.

It is described now.

The comment in line 322 says "matches 'pxe' class, but there's
only 'reserved-class1'. The same is true for lines 330 and 336.

Configuration 1 does include 'pxe' class besides reserved classes. Am I missing something?

src/bin/dhcp6/tests/classify_unittests.cc
matchClassification, subnetClassPriority, subnetGlobalPriority,
classGlobalPriority, clientClassifySubnet, relayOverrideAndClientClass
tests have their own configs, which should be part
of the global CONFIGS array.

True, but I merely moved those unit tests as they are from the other file. I don't want to spend time on refactoring existing unit tests within this ticket. This is endless story.

dhcp{4,6}.spec files
If you think it makes sense to keep updating those files, please
add item_description to the new entries.

Updated.

src/bin/dhcp6/dhcp6_srv.cc
I think the setReservedClientClasses() call should be made from
initContext(). That would be one call instead of many.

I actually prefer leaving it as it is now. initContext should do what it name says: initialize the context. Classification of a packet is a different thing. Would you agree?


The code builds and unit-tests pass on Ubuntu 16.04 x64.

With those changes applied, this ticket is ready to go. I don't
need to see it again.

Thanks. Because we don't fully agree on everything, I am passing it back to you in case you want to comment further.

comment:7 in reply to: ↑ 6 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 3 to 1
  • Owner changed from tomek to marcin
  • Total Hours changed from 19 to 20

Replying to marcin:

Replying to tomek:

I have reviewed the code and found only minor issues.

dhcp6-srv.xml
The new example has extra " at the end of the example (3 lines).

Removed.

Sort of. You removed only one, with the two remaining. I removed them. Please pull.

The comment in line 322 says "matches 'pxe' class, but there's
only 'reserved-class1'. The same is true for lines 330 and 336.

Configuration 1 does include 'pxe' class besides reserved classes. Am I missing something?

Nope. It was me who was missing something :) The comment is good after all.

src/bin/dhcp6/tests/classify_unittests.cc
matchClassification, subnetClassPriority, subnetGlobalPriority,
classGlobalPriority, clientClassifySubnet, relayOverrideAndClientClass
tests have their own configs, which should be part
of the global CONFIGS array.

True, but I merely moved those unit tests as they are from the other file. I don't want to spend time on refactoring existing unit tests within this ticket. This is endless story.

Fair enough.

src/bin/dhcp6/dhcp6_srv.cc
I think the setReservedClientClasses() call should be made from
initContext(). That would be one call instead of many.

I actually prefer leaving it as it is now. initContext should do what it name says: initialize the context. Classification of a packet is a different thing. Would you agree?

This comment came from the discrepancy between v4 that calls the new code from just one place (Dhcpv4Exchange constructor) vs multiple places (in v6). I suppose one day we should unify how we handle the code. But that day is not today. :)

Thanks. Because we don't fully agree on everything

We do now. Please pull, then merge. Unless you disagree with the quotes removal.

Tomek

comment:8 Changed 3 years ago by marcin

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

Merged with commit 242fbc47b31da404e57be458ac065f811084cca9

Note: See TracTickets for help on using tickets.