Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4121 closed defect (complete)

RFC 3046 (DHCPv4 RAI) conformance

Reported by: fdupont Owned by: tomek
Priority: low Milestone: Kea1.0-beta
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 (last modified by hschempf)

RFC 3046 "DHCP Relay Agent Information Option" section 2.2 with in particular

Servers SHOULD copy the Relay Agent Information option as the last DHCP option in the response.

Reported by Vadim Fedorenko <jun-junk@…>.

Subtickets

Change History (11)

comment:1 Changed 4 years ago by fdupont

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

comment:2 Changed 4 years ago by fdupont

Cloning LibDHCP::packOptions into 4 and 6. The only question is when the generic/legacy version should be removed.

comment:3 Changed 4 years ago by fdupont

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

Done. Ready for review.

comment:4 follow-up: Changed 4 years ago by hschempf

  • Description modified (diff)
  • Milestone changed from Kea-proposed to Kea1.0
  • Priority changed from medium to low

per team meeting Nov 18, accept in 1.0. could be done between beta/final. need to investigate what options require special treatment.

comment:5 in reply to: ↑ 4 Changed 4 years ago by fdupont

Replying to hschempf:

per team meeting Nov 18, accept in 1.0. could be done between beta/final. need to investigate what options require special treatment.

=> I added the 255 code too which must be the very last.

comment:6 Changed 4 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:7 Changed 4 years ago by tmark

  • Owner changed from tmark to tomek

I don't have an issue with having a packOptions4 and packOptions6, this is at least symetrical to the unpack functions. It is not unlikely that we'll find other protocol specific diffrences in the future. However, we should remove the "legacy" function now in 1.0, as it will only lead to confusion.

Removing the legacy method will mean reworking Option::packOptions() which calls it. I
would suggest simply replacing the body of Option::packOptions() with that of LIBDHCP::packOptions().

I would prefer to see packOptions4() structured similar to the
following to avoid performing option lookups:

void
LibDHCP::packOptions4(isc::util::OutputBuffer& buf,
                     const OptionCollection& options) {
    OptionPtr agent;
    OptionPtr end:
    OptionCollection::const_iterator it = options.begin();
    for (; it != options.end(); ++it) {
       // Some options must be last
        switch (it->first) {
            case DHO_DHCP_AGENT_OPTIONS:
                agent = it->second;
                break;
            case DHO_END:
                end = it->second;
                break;
            default:
                it->second->pack(buf);
                break;
        }
    }

    // Add the RAI option if it exists
    if (agent) {
       agent->pack(buf);
    }

    // And at the end the END option
    if (end)  {
       end->pack(buf);
    }
}

comment:8 Changed 4 years ago by tomek

  • Owner changed from tomek to tmark

Thanks for the review. Implemented packOptions4 as suggested (with only cosmetics changed). Added dedicated unit-test to verify that RAI option is indeed stored last.

Proposed ChangeLog?:

10XX.	[func]		fdupont,tomek
	The DHCPv4 server now stores Relay Agent Information option as
	the last one. packOptions() function in libdhcp++ has been
	replaced with packOptions4() and packOptions6().
	(Trac #4121, git tbd)

comment:9 Changed 4 years ago by tmark

  • Owner changed from tmark to tomek

Changes are ok, please merge.

comment:10 Changed 4 years ago by tomek

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

Thanks for the review. Code merged. Closing ticket.

comment:11 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.