CC: "Enke Chen (enkechen)" <enkechen@cisco.com>
MIME-Version: 1.0
Content-ID: <1141998B89EEDE4ABDF69782A9B31687@emea.cisco.com>
content-type: text/plain; charset="utf-8"
Subject: Re: [ISC-Bugs #44535] Patch: DHCP relay port feature code for review (from Cisco - Support ticket #11062)
Thread-Index: AQHSkQNjOoLKAu11pUCdGnMBQLNnF6F9y/uAgGO0MS+AAGckgA==
Date: Tue, 2 May 2017 15:42:54 +0000
To: "dhcp-suggest@isc.org" <dhcp-suggest@isc.org>
Content-Transfer-Encoding: base64
From naiming@cisco.com  Tue May  2 15:42:59 2017
X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,RP_MATCHES_RCVD autolearn=ham autolearn_force=no version=3.4.0
In-Reply-To: <rt-4.2.8-78708-1493735502-550.44535-5-0@isc.org>
X-RT-Interface: API
Content-Language: en-US
References: <RT-Ticket-44535@isc.org> <rt-4.2.8-62213-1485427934-1350.44535-5-0@isc.org> <rt-4.2.8-35031-1488204570-1581.44535-5-0@isc.org> <1752E32F-724D-415E-B83C-649BC8487861@cisco.com> <rt-4.2.8-35385-1488232752-1123.44535-5-0@isc.org> <rt-4.2.8-78708-1493735502-550.44535-5-0@isc.org>
Message-ID: <70CFC3FB-9813-43B6-A3FA-0AEB505F0459@cisco.com>
X-Ironport-Anti-Spam-Filtered: true
X-MS-Tnef-Correlator: 
X-Ironport-Av: E=Sophos;i="5.38,280,1491264000";  d="scan'208";a="420642118"
Return-Path: <naiming@cisco.com>
X-Original-To: dhcp-suggest@bugs.isc.org
X-MS-Has-Attach: 
Thread-Topic: [ISC-Bugs #44535] Patch: DHCP relay port feature code for review (from Cisco - Support ticket #11062)
X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on mx.pao1.isc.org
X-Originating-Ip: [10.24.102.224]
Accept-Language: en-US
X-MS-Exchange-Messagesentrepresentingtype: 1
From: "Naiming Shen (naiming)" <naiming@cisco.com>
RT-Message-ID: <rt-4.2.8-78629-1493739780-1108.44535-0-0@isc.org>
Content-Length: 6920


Hi Thomas,

I’ll modified the patch of the ‘relay port’ feature in these two weeks
based on your comments. Sorry for the delay.
The working group has passed the Last Call on this draft, and
I was waiting to see if we could get IANA official assigned numbers.
But I think I’ll modify the patch first. 

Best Regards,
- Naiming

> On May 2, 2017, at 7:31 AM, Thomas Markwalder via RT <dhcp-suggest@isc.org> wrote:
> 
> Hello Naiming:
> 
> I am following up on the review we provided for you back in February. We are looking at feature content for our next feature release of ISC DHCP, 4.4.0.
> 
> Can you tell the status of your efforts there?  Do you have follow up questions or feedback?
> 
> Regards,
> 
> Thomas Markwalder
> ISC Software Engineering
> 
> 
> 
> On Mon Feb 27 21:59:12 2017, naiming@cisco.com wrote:
>> 
>> Hi Thomas,
>> 
>> Thanks for the detailed review. and We’ll look over this and get
>> back to you. thanks.
>> 
>> - Naiming
>> 
>>> On Feb 27, 2017, at 6:09 AM, Thomas Markwalder via RT <dhcp-
>>> suggest@isc.org> wrote:
>>> 
>>> Hello Naiming:
>>> 
>>> First, I apologize for the delay in getting our comments back to you
>>> regarding the patch. In order to preserve formatting I have attached
>>> our
>>> review as a text file.
>>> 
>>> In general, the patch mechanics are fine, but we would rather see the
>>> the relay port specification done per interface rather than as global
>>> parameter.  Details on this are included in the attachment.
>>> 
>>> To set expectations, these changes will not be included in our
>>> upcoming maintenance release, 4.3.6, due out in May '17. Rather they
>>> would go into our feature release, 4.4.0, which is due out later in
>>> the year, date is TBD.
>>> 
>>> Thank you for your efforts thus far.  If you have any comments or
>>> questions please let us know.
>>> 
>>> Sincerely,
>>> 
>>> Thomas Markwalder
>>> ISC Software Engineering
>>> 
>>> 
>>> 
>>> We would like to the patch split as follows:
>>> 
>>> 1. V4 Server support
>>> 2. V4 Relay support
>>> 3. V6 Server support
>>> 4. V6 Relay
>>> 5. 4o6 Support
>>> 
>>> 2. The BPF filter changes seem fine. They allows an interface to
>>> accept
>>> messages either from the standard port or the alternate port  which
>>> matches
>>> the RFC draft. For interfaces that are both up and down, though it
>>> would
>>> mean that the upstream could only receive on the alternate port as
>>> the first
>>> port in the filter would be the standard downstream port. This is
>>> likely
>>> acceptable versus more complicated filter logic.
>>> 
>>> 3. The patch implements a global command line parameter, "-rp", used
>>> by
>>> dhcrelay to specify an alternate port value. When this parameter is
>>> specified
>>> the interface discovery code has been modified to only register one
>>> upstream
>>> and one downstream interface.
>>> 
>>> It is unclear if this was done for simplicity or based upon a
>>> perceived most
>>> likely use-case. There is a more flexible approach, that supports
>>> multiple
>>> interfaces, and has the additional benefit of being more seamless:
>>> 
>>> Add an optional port value to the v4's "-i,-iu,-U" and v6's "-u"
>>> command arguments:
>>> 
>>> -iu interface[/port]
>>> 
>>> -u [address%]interface[/port]
>>> 
>>> This makes the alternate port use specific to an upstream interface.
>>> If users want only a single upstream or downstream interfaces they
>>> explicitly configure it that way, if they want more than one they can
>>> still do that.
>>> 
>>> The steps to implement this are outlined below:
>>> 
>>> Rather than a single global relay_port variable, add a member for it
>>> to struct interface_info:
>>> 
>>> struct interface_info {
>>>    :
>>>    unit16_t relay_port;
>>> }
>>> 
>>> 
>>> relay/dhcrealy.c:request_v4_interface()
>>>   Modify to look for the port value and populate it in the
>>> structure, only
>>>   allowed if the flag parameter includes INTERFACE_UPSTREAM.
>>> 
>>> relay/dhcrealy.c:parse_upstream()
>>>   Modify to look for the port value and populate it in the
>>> structure.
>>> 
>>> discover.c::discover_interfaces()
>>>   Would not require the changes made by the patch, it would revert
>>> to as is.
>>> 
>>> 
>>> Several places would need look at the structure port value, instead
>>> of the global value:
>>> 
>>> common/discover.c::lpf_gen_filter_setup():
>>> 
>>> if ((udp.uh_dport != local_port) &&
>>>    ((interface->relay_port == 0) || (udp.uh_dport != interface-
>>>> relay_port)))
>>> 
>>> 
>>> common/packet.c:assemble_udp_ip_header():
>>> 
>>> udp.uh_sport = (interface->relay_port ? interface->relay_port :
>>> local_port);
>>> 
>>> common/packet.c:decode_udp_ip_header():
>>> 
>>> if ((udp.uh_dport != local_port) &&
>>>    ((interface->relay_port == 0) || (udp.uh_dport != interface-
>>>> relay_port)))
>>> 
>>> common/raw.c:if_register_send():
>>> 
>>> name.sin_port = (info->relay_port ? info->relay_port: local_port;
>>> 
>>> common/socket.c:if_register_socket():
>>> 
>>> addr6->sin6_port = (info->relay_port ? info->relay_port: local_port;
>>> 
>>> The rest of the changes the patch makes to this function are
>>> unnecessary.
>>> 
>>> relay/dhcrelay.c:process_up6()
>>>   Relocate the logic to add the D6O_RELAY_PORT to inside the
>>> upstream send
>>>   loop. Here the logic would need to be a little smarter:
>>> 
>>> /* Send it to all upstreams. */
>>> int has_relay_option = 0;
>>> for (up = upstreams; up; up = up->next) {
>>> if (has_relay_option) {
>>>    /* Previous upstream added it, this either shouldn't
>>>     * have it or needs to replace it, so let's remove it */
>>>    delete_option(&dhcpv6_universe, opts, D6O_RELAY_PORT);
>>>    has_relay_option = 0;
>>> }
>>> 
>>> if (upstream->relay_port || relay_client_port) {
>>>    if (!save_option_buffer(&dhcpv6_universe, opts, NULL,
>>>        (unsigned char *) &relay_client_port, sizeof(u_int16_t),
>>>         D6O_RELAY_PORT, 0)) {
>>>        log_error("Can't save relay-port.");
>>>        option_state_dereference(&opts, MDL);
>>>        return;
>>>    }
>>> 
>>> has_relay_option = 1;
>>> }
>>> 
>>> send_packet6(up->ifp, (unsigned char *) forw_data,
>>>             (size_t) cursor, &up->link);
>>> }
>>> 
>>> Note these changes may not have covered everything necessary, and of
>>> course
>>> have not been been compile so there may syntax errors. However they
>>> should
>>> suffice to guide the actual code work.
>>> 
>>> 
>>> 4. The 4o6 patch seems fine. It adds room in the IPC header to
>>> accommodate the
>>> alternate port the V6 server received the message from so the v4
>>> server can
>>> echo it back. This allows the V6 server to send the response back on
>>> the
>>> alternate port.
>>> 
>> 
> 
> 
>