Opened 4 months ago

Closed 8 weeks ago

#5535 closed enhancement (fixed)

Multiple relay addresses per subnet

Reported by: tomek Owned by: tmark
Priority: medium Milestone: Kea1.4
Component: dhcp4 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

As of 1.3, Kea supports subnet selection based on relay IP address. For a single subnet only one IP address can be specified. However, it was requested by a user that in his network there are multiple relays (that use different IP addresses) that can lead to the same subnet.

This ticket requires an extension to the ip-address mechanism in relay structure. While the ip-address will remain to be used in most deployments, there will be some more advanced deployments that'll need to handle multiple addresses. The recommended way to deal with this is to keep ip-address (a single ip defined as string), but also add ip-addresses that will handle a list of strings, each defining a single address. Internally, both parameters should have its content kept in joint list of addresses.

While the original request was for IPv4 only, similar mechanism should be implemented for IPv6 as well.

Subtickets

Change History (4)

comment:1 Changed 8 weeks ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:2 Changed 8 weeks ago by tmark

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

Tomek claimed this ticket wasn't complicated when he dumped it on me. I have to call BS on that, as the reviewer will soon see.

Both servers will now support the use of either "ip-address" or "ip-addresses" in "relay" elements.
In addition, shared networks now actually parse "relay" elements. This was an oversight that is
mentioned in #5441.

ChangeLog?:

13xx.   [func]      tmark
    For both kea-dhcp4 and kea-dhcp6, the "ip-address" parameter
    in the "relay" element for both subnets and shared networks,
    has been replaced with a list form, "ip-addresses".  Configuration
    parsing will continue to honor the singular form, but it should
    be considered deprecated.  In addition, an omission in 1.3 that
    caused shared network parsing to ignore the "relay' element has
    been corrected.
    (Trac #5535, git TBD)
Last edited 8 weeks ago by tmark (previous) (diff)

comment:3 follow-up: Changed 8 weeks ago by tomek

  • Owner changed from UnAssigned to tmark

I was not aware of the shared network bug, so it complicated things a fair bit.

Don't commit *.hh files

dhcp6/tests/config_parser_unittest.cc

Dhcp6ParserTest.subnetRelayInfoList test:

The relay IPs should be out of the subnet. This is uncommon (but valid) setup.
The problem here is if the address passed to selectSubnet is within the subnet,
you don't know whether the subnet was chosen because of the relay info or
because it's in the subnet.

The same applies to v4, although in a slightly different way. The way
to go there is to use out of subnet addresses, but pass the relay address
as giaddr_ field in the subnet selector.

I've updated both tests. Please pull and review.

network.cc

hasAddresses() called size() and then checked for being greater than 0.
This actually requires the implementation to count the items first.
It's better to simply call !empty(). Code updated.

shared_network_parser.cc
Hat off to your thoroughness, sir! This one could easily slip by
unnoticed. Make sure you put something about it being fixed in the
ChangeLog?. Something like "Thanks to Britney Spears inspiration,
the relay information specified on shared network level now actually works.".
You may consider dropping the BS part, though.

shared_network_parser_unittest.cc
You removed the description of SharedNetwork4ParserTest.clientClassMatchClientId.
Why? If there's no good reason, please pull. I've reverted it.

That's fine set of new unit-tests. Impressive work. Thank you for doing that.


The code builds and unit-tests pass on mac os 10.12.6.

Please pull and review. If you're ok with my changes, go ahead and merge.
If you're not ok with them, discard them and merge anyway.

Thanks a lot for doing this work. Much appreciated.

Last edited 8 weeks ago by tomek (previous) (diff)

comment:4 in reply to: ↑ 3 Changed 8 weeks ago by tmark

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

Replying to tomek:

I was not aware of the shared network bug, so it complicated things a fair bit.

Don't commit *.hh files

Oops. I already did. Will check in the future.

dhcp6/tests/config_parser_unittest.cc

Dhcp6ParserTest.subnetRelayInfoList test:

The relay IPs should be out of the subnet. This is uncommon (but valid) setup.
The problem here is if the address passed to selectSubnet is within the subnet,
you don't know whether the subnet was chosen because of the relay info or
because it's in the subnet.

The same applies to v4, although in a slightly different way. The way
to go there is to use out of subnet addresses, but pass the relay address
as giaddr_ field in the subnet selector.

I've updated both tests. Please pull and review.

Ah. Hadn't occurred to me but that's a good point. Thanks.

network.cc

hasAddresses() called size() and then checked for being greater than 0.
This actually requires the implementation to count the items first.
It's better to simply call !empty(). Code updated.

Quite right. I should know better. Thanks again.

shared_network_parser.cc
Hat off to your thoroughness, sir! This one could easily slip by
unnoticed. Make sure you put something about it being fixed in the
ChangeLog?. Something like "Thanks to Britney Spears inspiration,
the relay information specified on shared network level now actually works.".
You may consider dropping the BS part, though.

ChangeLog? mentions the correction, though Britney will be disappointed that I left her out of it.

shared_network_parser_unittest.cc
You removed the description of SharedNetwork4ParserTest.clientClassMatchClientId.
Why? If there's no good reason, please pull. I've reverted it.

Oops. Thanks.

That's fine set of new unit-tests. Impressive work. Thank you for doing that.


The code builds and unit-tests pass on mac os 10.12.6.

Please pull and review. If you're ok with my changes, go ahead and merge.
If you're not ok with them, discard them and merge anyway.

Thanks a lot for doing this work. Much appreciated.

Changes merged with git #f4601abdb657122a8ba5d7784eded773ec01d171
Added ChangeLog? entry #1391

Ticket is complete.

Note: See TracTickets for help on using tickets.