CC: "Enke Chen (enkechen)" <enkechen@cisco.com>
MIME-Version: 1.0
Content-ID: <FCA3CDE6D301D34E93767BD19A93E8D3@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/uA
Date: Mon, 27 Feb 2017 21:59:05 +0000
To: "dhcp-suggest@isc.org" <dhcp-suggest@isc.org>
Content-Transfer-Encoding: base64
From naiming@cisco.com  Mon Feb 27 21:59:11 2017
X-Spam-Status: No, score=-8.3 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-35031-1488204570-1581.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>
Message-ID: <1752E32F-724D-415E-B83C-649BC8487861@cisco.com>
X-Ironport-Anti-Spam-Filtered: true
X-MS-Tnef-Correlator: 
X-Ironport-Av: E=Sophos;i="5.35,215,1484006400";  d="scan'208";a="211639174"
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: [128.107.155.84]
Accept-Language: en-US
X-MS-Exchange-Messagesentrepresentingtype: 1
From: "Naiming Shen (naiming)" <naiming@cisco.com>
RT-Message-ID: <rt-4.2.8-35385-1488232752-1123.44535-0-0@isc.org>
Content-Length: 5690


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.
>