Opened 6 years ago

Closed 6 years ago

#3184 closed defect (fixed)

Kea4: does not echo back relay agent info option

Reported by: tomek Owned by: tomek
Priority: very high Milestone: Sprint-DHCP-20131016
Component: dhcp4 Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Low
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Responses to relayed traffic in DHCPv4 must echo back option 82 (relay agent info).

Set to high as this blocks upcoming demo.

Subtickets

Change History (8)

comment:1 Changed 6 years ago by tomek

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

comment:2 Changed 6 years ago by tomek

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

Fix and test verifying it implemented.

When doing review, keep in mind that majority of the changes is a move of class Dhcpv4SrvTest from dhcp4_srv_unittest.cc to dhcp4_test_utils.h. See commit-id 708a8dd0257f40c076f3a3db58502dacf26ff020.

Besides that commit, the changes are really minor.

comment:3 Changed 6 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:4 Changed 6 years ago by tmark

  • Owner changed from stephen to tmark

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

  • Owner changed from tmark to tomek

dhcp4_test_utils.h -

Closing endif is tagged incorrectly.

 // DHCP6_TEST_UTILS_H

dhcp4_test_utils.cc -

You left all of the method briefs in the .cc file. Convention is that they are
in the .h file, not both.


wireshark.cc -

Extraneous spaces at the end of lines. HA! I finally caught someone with this! Marcin and Stephen always manage to get me on this one.


wireshark.cc -

You might consider a bit of restructure here. There is an underlying mechanism for building packets from captured packet hex strings that should be extracted from the captureRelayedDiscover() into a generic method, something like:

    Pkt4Ptr Dhcpv4SrvTest::packetFromCapture(const std::string& hex_string) {
        std::vector<uint8_t> bin;

        // Decode the hex string and store it in bin (which happens
        // to be OptionBuffer format)
        isc::util::encode::decodeHex(hex_string, bin);

        Pkt4Ptr pkt(new Pkt4(&bin[0], bin.size()));
        captureSetDefaultFields(pkt);

        return (pkt);
    }

You could then start a static array of captured hex strings with numeric const indexes to look them up, or maybe a map of captures with string names for keys, or number of other things; rather than a new method for each new captured packet. Down the road you could load packet inventory from file etc.

For now, simply the basic method as above and alter captureRelayDiscover to delcare the hex_string and just do this:

    return (packetFromCapture(hex_string));

would suffice.


dhcp4_srv_unittest.cc -

In TEST_F(Dhcpv4SrvTest, relayAgentInfoEcho), you should probably put an
ASSERT_NO_THROW around the call to captureRelayedDiscover method. That method uses a Pkt4 constructor that can throw.


cppcheck, clang build, and valgrind unit tests all pass

Last edited 6 years ago by tmark (previous) (diff)

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

  • Owner changed from tomek to tmark

Replying to tmark:

dhcp4_test_utils.h -

Closing endif is tagged incorrectly.

 // DHCP6_TEST_UTILS_H

Fixed.

dhcp4_test_utils.cc -

You left all of the method briefs in the .cc file. Convention is that they are
in the .h file, not both.

I had troubles addressing this one, because there was no such file in my repo. But I thought it would be rude to throw such a fine comment, so I added dhcp4_test_utils.cc.

wireshark.cc -

Extraneous spaces at the end of lines. HA! I finally caught someone with this! Marcin and Stephen always manage to get me on this one.

Ha! I forgot that one time to call whitespace-cleanup and you caught me. I should be more sneaky next time.

wireshark.cc -

You might consider a bit of restructure here. There is an underlying mechanism for building packets from captured packet hex strings that should be extracted from the captureRelayedDiscover() into a generic method, something like:

    Pkt4Ptr Dhcpv4SrvTest::packetFromCapture(const std::string& hex_string) {
        std::vector<uint8_t> bin;

        // Decode the hex string and store it in bin (which happens
        // to be OptionBuffer format)
        isc::util::encode::decodeHex(hex_string, bin);

        Pkt4Ptr pkt(new Pkt4(&bin[0], bin.size()));
        captureSetDefaultFields(pkt);

        return (pkt);
    }

You could then start a static array of captured hex strings with numeric const indexes to look them up, or maybe a map of captures with string names for keys, or number of other things; rather than a new method for each new captured packet. Down the road you could load packet inventory from file etc.

For now, simply the basic method as above and alter captureRelayDiscover to delcare the hex_string and just do this:

    return (packetFromCapture(hex_string));

would suffice.

Done as suggested.

dhcp4_srv_unittest.cc -

In TEST_F(Dhcpv4SrvTest, relayAgentInfoEcho), you should probably put an
ASSERT_NO_THROW around the call to captureRelayedDiscover method. That method uses a Pkt4 constructor that can throw.

Done.

cppcheck, clang build, and valgrind unit tests all pass

Excellent.

I think I addressed all issues. There's new file dhcp4_test_utils.cc that contains code moved from dhcp4_srv_unittest.cc. Several classes were moved to dhcp4_test_utils.h as well.

comment:7 Changed 6 years ago by tmark

  • Owner changed from tmark to tomek

Changes are acceptable. Please merge.

comment:8 Changed 6 years ago by tomek

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

Merged. Thanks for your review.

Note: See TracTickets for help on using tickets.