Opened 6 years ago

Closed 6 years ago

#3322 closed enhancement (complete)

Client classification: relay address override

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea0.8
Component: dhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 3
Total Hours: 16 Internal?: no

Description

This is a follow up to the ticket #3274.

#3274 implemented configuration for relay information, but that information was never used. This ticket is about implementing its use.

Subtickets

Change History (9)

comment:1 Changed 6 years ago by tomek

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

comment:2 Changed 6 years ago by tomek

  • Status changed from accepted to assigned

comment:3 Changed 6 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from assigned to reviewing

The code builds upon #3274. It is rather small ticket (diff is 671 lines). Two sections has been added to each DHCP component guides. Please review.

comment:4 Changed 6 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:5 follow-up: Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit: a3db6f08b0321dcfbd7539e07764f0c124ac19af

ChangeLog

Some minor editorial changes:

7XX.	[func]		tomek
	b10-dhcp4, b10-dhcp6: IP address of the relay agent can now be specified
	for both IPv4 and IPv6 subnets. That information allows the server to
	properly handle a case where relay agent address does not match	*the* subnet.
	This is mostly useful *for* shared subnets and cable networks.
	(Trac #3322, git abcd)

doc/guide/bind10-guide.xml
Along with this ticket the note in section 17.6 became only partly true. The subnet selection mechanism not only is altered by the classification but also by the relay override mechanism. That should be updated in the note.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
Please use the !Dhcp4SrvTest::configure method to create configuration for the new tests.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Typo in the relayOverride: should be ''subnet4'', not ''subnet6''.

src/lib/dhcpsrv/cfgmgr.cc
Please consider adding a debug message in getSubnet4 and getSubnet6 to inform that the configured relay info is used to select subnet when relay argument is true.

Nit: there is a commented default value for relay in getSubnet4, but there isn't one for getSubnet6. Personally, I never add the default parameter in .cc files, partly because it may be hard to maintain consistency between the default value in cc file and h file. But, if you want to keep it, you could add the default for getSubnet4 for consistency. But, it is up to you.

src/lib/dhcpsrv/cfgmgr.h
Typo in the getSubnet6 description: ''Dhcp4/subnet6[X]/relay/ip-address'' - should be Dhcp6.

Also, irrelevant comment ''They are for giaddr only''. There is no giaddr in DHCPv6.

Nit: technically relay could be const.

When I was going through the documentation of getSubnet6 I realized that the function description deserves some updates with respect to relay information and classification which have been introduced lately. Specifically, in this part:

    /// If there are any classes specified in a subnet, that subnet
    /// will be selected only if the client belongs to appropriate class.
    ///
    /// If relay is true then relay info overrides (i.e. value the sysadmin
    /// can configure in Dhcp4/subnet6[X]/relay/ip-address) can be used.
    /// That is true only for relays. Those overrides must not be used
    /// for client address or for client hints. They are for giaddr only.

it should be mentioned that it is based on the assumption that there is more than one subnet configured. For a single subnet the code may never get to classification nor to relay overriding. This is explained in the function body, but should be first of all explained in the function description. I can imagine that someone sets client class for the sole subnet and expects that some clients will be rejected, some accepted. In fact, he will end up accepting all clients because if there is one subnet and the client (or relay?) sends message from link local addres, this sole subnet is returned regardless of classification and such. So, that makes me doubt that we should return the sole subnet, regardless of client class and relay info?

I also think that some considerations regarding this issue should be included in the bind10-guide.

There should probably be a unit test that covers getSubnet4 and getSubnet6 behavior when the relay parameter is true.

comment:6 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 0 to 3
  • Total Hours changed from 0 to 3

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

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

Replying to marcin:

Reviewed commit: a3db6f08b0321dcfbd7539e07764f0c124ac19af

ChangeLog

Some minor editorial changes:

7XX.	[func]		tomek
	b10-dhcp4, b10-dhcp6: IP address of the relay agent can now be specified
	for both IPv4 and IPv6 subnets. That information allows the server to
	properly handle a case where relay agent address does not match	*the* subnet.
	This is mostly useful *for* shared subnets and cable networks.
	(Trac #3322, git abcd)

Thanks.

doc/guide/bind10-guide.xml
Along with this ticket the note in section 17.6 became only partly true. The subnet selection mechanism not only is altered by the classification but also by the relay override mechanism. That should be updated in the note.

Updated section 17.6 and some v6 sections.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
Please use the !Dhcp4SrvTest::configure method to create configuration for the new tests.

Done. Cool method. Implemented similar one for v6.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Typo in the relayOverride: should be ''subnet4'', not ''subnet6''.

Fixed in dhcp6_srv_unittest.cc

src/lib/dhcpsrv/cfgmgr.cc
Please consider adding a debug message in getSubnet4 and getSubnet6 to inform that the configured relay info is used to select subnet when relay argument is true.

Added.

Nit: there is a commented default value for relay in getSubnet4, but there isn't one for getSubnet6. Personally, I never add the default parameter in .cc files, partly because it may be hard to maintain consistency between the default value in cc file and h file. But, if you want to keep it, you could add the default for getSubnet4 for consistency. But, it is up to you.

Good point. I haven't thought about the cc/h consistency over time. Removed comment.

src/lib/dhcpsrv/cfgmgr.h
Typo in the getSubnet6 description: ''Dhcp4/subnet6[X]/relay/ip-address'' - should be Dhcp6.

Fixed.

Also, irrelevant comment ''They are for giaddr only''. There is no giaddr in DHCPv6.

Fixed.

Nit: technically relay could be const.

Added.

When I was going through the documentation of getSubnet6 I realized that the function description deserves some updates with respect to relay information and classification which have been introduced lately. Specifically, in this part:

    /// If there are any classes specified in a subnet, that subnet
    /// will be selected only if the client belongs to appropriate class.
    ///
    /// If relay is true then relay info overrides (i.e. value the sysadmin
    /// can configure in Dhcp4/subnet6[X]/relay/ip-address) can be used.
    /// That is true only for relays. Those overrides must not be used
    /// for client address or for client hints. They are for giaddr only.

it should be mentioned that it is based on the assumption that there is more than one subnet configured. For a single subnet the code may never get to classification nor to relay overriding. This is explained in the function body, but should be first of all explained in the function description. I can imagine that someone sets client class for the sole subnet and expects that some clients will be rejected, some accepted. In fact, he will end up accepting all clients because if there is one subnet and the client (or relay?) sends message from link local addres, this sole subnet is returned regardless of classification and such. So, that makes me doubt that we should return the sole subnet, regardless of client class and relay info?

This comment is no longer relevant. After our discussion on Jabber I decided to get rid of the single subnet exception. There were surprisingly wide repercussions of that step:

  • over 20 tests started to fail
  • they all worked under the assumption that the subnet will be selected if there's only one and source is link-local
  • I adjusted them to use interface name instead, but that brought the issue of having known interface configuration
  • So PktFilter6TestStub was implemented.
  • Dhcpv6SrvTest was refactored slightly, so there is no configure() method similar to the one in Dhcpv4SrvTest
  • Many tests updated to match mentioned changes

I also think that some considerations regarding this issue should be included in the bind10-guide.

Section updatd.

There should probably be a unit test that covers getSubnet4 and getSubnet6 behavior when the relay parameter is true.

There are two new tests for this now.

Thanks for your review, but I'm afraid this is not over yet...

comment:8 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 8 to 2
  • Owner changed from marcin to tomek
  • Total Hours changed from 11 to 13

Reviewed commit 97f4d442ff4afde00db47f4108212d17c89752a5

I fixed one typo in the header file, so please pull this change before merge.

lib/dhcp/IfaceMgrTestConfig.cc
In the constructor you select the stub packet filter which remains set as long as the unit test is being run. I think you should set a default packet filter in the class destructor (it is being done for v4 stub) to make sure that we don't use the stub in the subsequent tests if they don't need it, or if they must use real sockets and interfaces.

Apart from this, your changed are good. When you fix the issue mentioned above, please merge.

comment:9 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 2 to 3
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 13 to 16

Thanks for the review. IfaceMgrTestConfig? class now reverts PktFilter6 to the default in its constructor.

Code merged. Closing ticket.

Note: See TracTickets for help on using tickets.