Opened 7 years ago

Closed 7 years ago

#2827 closed enhancement (fixed)

v6 relays support in Kea

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130411
Component: dhcp6 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: 0
Total Hours: 0 Internal?: no

Description

Current code do not support relays in DHCPv6 component. This must be implemented.

Subtickets

Change History (6)

comment:1 Changed 7 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130328

comment:2 Changed 7 years ago by tomek

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Pkt6 class is now able to parse and build relayed messages.

This does not mean that the server is able to handle relays yet. Support for this needs to be added in Dhcpv6Srv class, but that is a very easy thing to do once this ticket is merged.

Please review.

The traffic capture used in tests features a fancy message from a real life network. It has one odd remote-id option that have field that is empty. This tripped over our option parser, so it was modified slightly to allow empty strings and binary data. This is a separate issue and solving it properly should be done as a separate ticket.

comment:3 follow-up: Changed 7 years ago by marcin

  • Owner changed from UnAssigned to tomek

Reviewed commit 859e0b3e82d4cc5270d8fb557f0120dc35edd1a3

Please update the copyright dates in modified files.

ChangeLog
Changes are limited to ''libdhcp++'' so maybe reflect that in the changelog by replacing ''b10-dhcp6'' with ''libdhcp++.

pkt6.cc
Please update the copyright date.

Pkt6::len(): wouldn't it be better to call calculateRelaySizes() on packet reception - when unpackUDP() is called? The Pkt6::unpackUDP() is called once for the specific packet while Pkt6::len() can be called several times from different parts of the code. This will cause re-calculation.

Pkt6::calculateRelaySizes(): Proposing alternative to the loop in this function. However, this is really minor matter so you don't need to pay attention to this proposal. :-)

for (int relay_index = relay_info_.size(); relay_index > 0; --relay_index) {
    relay_info_[relay_index - 1].relay_msg_len_ = len;
    len += getRelayOverhead(relay_info_[relay_index - 1]);
}

Pkt6::packUDP():
Having some more comments would be useful in the part of the function that packs relay headers and options.

pkt6.h
getRelayOption(): this function is not documented.

directLen(): IMO, it could be better to say ''if received message was not relayed'' because ''directly'' may be ambiguous.
Also, this function can be declared const.

getRelayOverhead(): the ''relay'' parameter is not documented.
This function could be declared const.

pkt6_unittest.cc
It is probably not a good idea to...

using namespace boost;

On Solaris this will shade the uint8_t type which will result in compilation error.

capture2(): typo in the comment. s/related/relayed.

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

  • Owner changed from tomek to marcin

Replying to marcin:

Reviewed commit 859e0b3e82d4cc5270d8fb557f0120dc35edd1a3

Please update the copyright dates in modified files.

Updated.

ChangeLog
Changes are limited to ''libdhcp++'' so maybe reflect that in the changelog by replacing ''b10-dhcp6'' with ''libdhcp++.

Done as suggested.

pkt6.cc
Please update the copyright date.

Done.

Pkt6::len(): wouldn't it be better to call calculateRelaySizes() on packet reception - when unpackUDP() is called? The Pkt6::unpackUDP() is called once for the specific packet while Pkt6::len() can be called several times from different parts of the code. This will cause re-calculation.

No. This would return invalid values after each addOption() or delOption() call, so its validity would base on calling calculateRelaySizes() after each option manipulation. Our code should call Pkt6::len() only once or twice, so it's not that big deal. Also, the "relays" are typically just one relay, so there cost of calling calculateRelaySizes() is not that big. And for really complex multi-relay networks... well, there's a price to pay for complex design.

Pkt6::calculateRelaySizes(): Proposing alternative to the loop in this function. However, this is really minor matter so you don't need to pay attention to this proposal. :-)

for (int relay_index = relay_info_.size(); relay_index > 0; --relay_index) {
    relay_info_[relay_index - 1].relay_msg_len_ = len;
    len += getRelayOverhead(relay_info_[relay_index - 1]);
}

Done as requested.

Pkt6::packUDP():
Having some more comments would be useful in the part of the function that packs relay headers and options.

Added.

pkt6.h
getRelayOption(): this function is not documented.

It is now.

directLen(): IMO, it could be better to say ''if received message was not relayed'' because ''directly'' may be ambiguous.
Also, this function can be declared const.

Comment clarified, method is now const.

getRelayOverhead(): the ''relay'' parameter is not documented.
This function could be declared const.

Parameter documented, function is now const.


pkt6_unittest.cc
It is probably not a good idea to...

using namespace boost;

On Solaris this will shade the uint8_t type which will result in compilation error.

Aahhh, I forgot about that. using namespace boost removed.

capture2(): typo in the comment. s/related/relayed.

Fixed.

Thanks for the review.

comment:5 Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

Changes are ok and can be now merged.

comment:6 Changed 7 years ago by tomek

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

Code merged, closing ticket.

Note: See TracTickets for help on using tickets.