CC: "Enke Chen (enkechen)" <enkechen@cisco.com>
MIME-Version: 1.0
Content-ID: <44EE718B25AD0541B5CC00384A3DB104@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+AAGckgP//rtN+gABalAA=
Date: Tue, 2 May 2017 16:16:33 +0000
To: "dhcp-suggest@isc.org" <dhcp-suggest@isc.org>
Content-Transfer-Encoding: base64
From naiming@cisco.com  Tue May  2 16:16:38 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-78629-1493740329-353.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> <70CFC3FB-9813-43B6-A3FA-0AEB505F0459@cisco.com> <rt-4.2.8-78629-1493739780-1108.44535-5-0@isc.org> <rt-4.2.8-78629-1493740329-353.44535-5-0@isc.org>
Message-ID: <2B708E0F-C0A8-4FAB-8002-DF669C7A1131@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="238199535"
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.155.252.192]
Accept-Language: en-US
X-MS-Exchange-Messagesentrepresentingtype: 1
From: "Naiming Shen (naiming)" <naiming@cisco.com>
RT-Message-ID: <rt-4.2.8-78708-1493741799-916.44535-0-0@isc.org>
Content-Length: 8219


Hi Thomas,

Thanks and I agree. What is the approacimate time of 4.4.0 of DHCP?

Best Regards,
- Naiming

> On May 2, 2017, at 8:52 AM, Thomas Markwalder via RT <dhcp-suggest@isc.org> wrote:
> 
> Hi Naiming:
> 
> Thank you for your prompt reply.  I can understand you wanting to wait.  Since we've passed the last call it should be safe enough to develop the patch and just use place-holder values until there are official values rom IANA. Hopefully, we'll have them well before we release time.
> 
> In the meantime, if you run into any issues or questions just let me know.
> 
> Cheers,
> 
> Thomas
> 
> If you have any questions or run into issues please let me know.  
> On Tue May 02 15:43:00 2017, naiming@cisco.com wrote:
>> 
>> 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.
>>>>> 
>>>> 
>>> 
>>> 
>>> 
>> 
> 
> 
>