CC: "Enke Chen (enkechen)" <enkechen@cisco.com>
MIME-Version: 1.0
Content-ID: <9356C292BE204D48A8CC3B83EE73732E@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: AQHSkQNjOoLKAu11pUCdGnMBQLNnF6HkAA8A
Date: Wed, 3 May 2017 23:43:57 +0000
To: "dhcp-suggest@isc.org" <dhcp-suggest@isc.org>
Content-Transfer-Encoding: base64
From naiming@cisco.com  Wed May  3 23:44:03 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-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: <646422AD-300A-4686-BB29-BBE1266746C4@cisco.com>
X-Ironport-Anti-Spam-Filtered: true
X-MS-Tnef-Correlator: 
X-Ironport-Av: E=Sophos;i="5.38,284,1491264000";  d="scan'208";a="419371572"
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.89]
Accept-Language: en-US
X-MS-Exchange-Messagesentrepresentingtype: 1
From: "Naiming Shen (naiming)" <naiming@cisco.com>
RT-Message-ID: <rt-4.2.8-2611-1493855044-68.44535-0-0@isc.org>
Content-Length: 6664


Hi Thomas,

One thing I noticed when starting to make changes is that,
the per-interface defintion of relay-port may have some
issues. The current patch with this ‘-rp’ knob is modeling
after the existing ‘-p’ usage, which means if we are changing the
DHCP relay port number, it makes sense to change it for
the global context.

Since a relay agent device may have many interfaces. The DHCP
server may be multiple hops away. From a relay-agent admin point
of view (s)he does not exactly know which interface(s) it should
use. Even (s)he knows for now, the routing may change later, and the
device may also have too many interfaces to define each of
them explicitly.

The use case we explicitly target for this relay-port feature is
because multiple relay instances sharing the same physical device
with the same source IP address, so it can not use the same
UDP relay port among different relay-agent processes. For this
feature I doubt there is a need for one relay-agent trying to use
different UDP ports for different interfaces.

Best Regards,
- 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.
>