Opened 5 months ago

Closed 5 weeks ago

#5515 closed defect (fixed)

Fix outbound-interface use-routing

Reported by: fdupont Owned by: marcin
Priority: medium Milestone: Kea1.4
Component: dhcp 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 fdupont)

Two problems:

  • the DHCPv4 code resets the interface to "": I can't see how interface manager send() accepts this and BTW it is useless as the int packet filter uses only the interface index (reset to -1) and local address (reset to 0.0.0.0).
  • the DHCPv6 code is missing (fix lexer, parser, setting code and inet6 packet filter which BTW needs only the interface index).

Subtickets

Change History (14)

comment:1 Changed 5 months ago by tomek

  • Component changed from Unclassified to dhcp
  • Milestone changed from Kea-proposed to Kea1.4-final
  • Priority changed from medium to low

As discussed on 2018-01-25 call, moving this to 1.4-final as low. We are currently not aware of any users being affected by this problem.

comment:2 Changed 5 months ago by fdupont

Cf support 12308 too.

comment:3 Changed 5 months ago by tomek

  • Milestone changed from Kea1.4-final to Kea1.4
  • Priority changed from low to medium

This is related to a support issue, moving this to 1.4 then.

comment:4 Changed 5 months ago by fdupont

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

comment:5 Changed 5 months ago by fdupont

  • Description modified (diff)

comment:6 Changed 5 months ago by fdupont

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

Code done. For DHCPv4 the fix seems to work, for DHCPv6 it should but there is no system tests and the forge developer has a broken script for even no regression cannot be checked.

comment:7 Changed 4 months ago by marcin

  • Owner changed from UnAssigned to marcin

comment:8 follow-up: Changed 4 months ago by marcin

  • Owner changed from marcin to fdupont

I reviewed commits up to cf73109165c49093bc4c4a7f30f35b2d1772f463.

The code changes look good to me. The ticket certainly lacks ChangeLog so one has to be proposed.

The ticket description also doesn't mention what issue it solves (other than adding DHCPv6 support). Perhaps, clearing interface name confuses interface manager in some cases? I agree that the packet filter ignores interface name, so resetting the index to a negative value is good enough. But, what is the problem with clearing interface name?

I am also concerned that we don't have system tests for DHCPv6. Did we raise that with QA? Do you know of any plans that QA tests it?

comment:9 Changed 4 months ago by marcin

One additional comment. The following unit test now fails expecting an empty interface name:

[ RUN      ] Dhcpv4SrvTest.adjustIfaceDataUseRouting
dhcp4_srv_unittest.cc:269: Failure
Value of: resp->getIface().empty()
  Actual: false
Expected: true
[  FAILED  ] Dhcpv4SrvTest.adjustIfaceDataUseRouting (2 ms)

comment:10 in reply to: ↑ 8 Changed 4 months ago by fdupont

  • Owner changed from fdupont to marcin

Replying to marcin:

I reviewed commits up to cf73109165c49093bc4c4a7f30f35b2d1772f463.

The code changes look good to me. The ticket certainly lacks ChangeLog so one has to be proposed.

The ticket description also doesn't mention what issue it solves (other than adding DHCPv6 support).

=> there are 2 problems described in the ticket description, the second is DHCPv6 support and there is the first...

Perhaps, clearing interface name confuses interface manager in some cases?

=> more than being confused (note that getIface() returns a string):

IfaceMgr::send(const Pkt4Ptr& pkt) {

    IfacePtr iface = getIface(pkt->getIface());
    if (!iface) {
        isc_throw(BadValue, "Unable to send DHCPv4 message. Invalid interface ("
                  << pkt->getIface() << ") specified.");
    }

    // Assuming that packet filter is not NULL, because its modifier checks it.
    return (packet_filter_->send(*iface, getSocket(*pkt).sockfd_, pkt));
}

I agree that the packet filter ignores interface name, so resetting the index to a negative value is good enough. But, what is the problem with clearing interface name?

=> in IfaceMgr::send which is the caller of the packet filter code.

I am also concerned that we don't have system tests for DHCPv6. Did we raise that with QA? Do you know of any plans that QA tests it?

=> we can give the ticket to Wlodek?

Looking for the unit test. BTW do you have an idea for the ChangeLog entry?

comment:11 Changed 3 months ago by marcin

According to Wlodek, this change hasn't been properly system tested yet. Therefore, the ticket is stalled until testing is done.

comment:12 Changed 5 weeks ago by marcin

Per my discussion with Tomek earlier today, we're going to remove the DHCPv6 part from the docs for now. We primarily need this ticket to get in to fix the issues in DHCPv4.

comment:13 Changed 5 weeks ago by marcin

Created #5623 to deal with the DHCPv6 specific changes. This ticket gets limited to addressing the DHCPv4 problem.

comment:14 Changed 5 weeks ago by marcin

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

Merged DHCPv4 specific changes with commit 9d8d00f1f127ee606f09f7ff6006f0d142aac976

Note: See TracTickets for help on using tickets.