On May 7, 2017, at 10:45 PM, Naiming Shen (naiming) via RT <dhcp-suggest@isc.org> wrote:
Hi Thomas,
(1) as related to my below email. I tried to do a ‘-U interface[/port]’,
but I have problem at the place such as:
if (!(length = add_relay_agent_options(ip, packet, length,
ip->addresses[0])))
return;
…
for (sp = servers; sp; sp = sp->next) {
if (send_packet((fallback_interface
? fallback_interface : interfaces),
ULL, packet, length, ip->addresses[0],
&sp->to, NULL) < 0) {
Since the ‘relay-port’ is only related to upstream interface, while the
interface_info in that ‘add_relay_agent_options()’ is a downstream
interface. Although we can require users to put a port number at each
of the downstream interfaces, but that does not make sense and it
can be too many places.
We need to insert the RAI_RELAY_PORT sub-option when we receives
traffic from the client, and we have not got to the outbound (upstream)
interface yet.
This is again why I think this global ‘-rp’ makes sense for this relay-port
feature.
(2) on your comment of “.. the interface discovery code has been modified to
only register on upstream and one downstream interface.”
I have to apologize that I didn’t include much comments in this patch.
If you look at this code in discover_interfaces(), it has these lines:
#if defined(DHCPv6)
/* Only register the first interface for V6, since
* servers and relays all use the same socket.
* XXX: This has some messy side effects if we start
* dynamically adding and removing interfaces, but
* we're well beyond that point in terms of mess.
*/
if (((state == DISCOVER_SERVER) || (state == DISCOVER_RELAY)) &&
(local_family == AF_INET6))
break;
#endif
My understanding of the above code is that, for IPv6 only one socket is
needed so as long as we found only one interface, we breakout here.
This will break this 'relay-port' feature, since the 'relay-port' is
only related to the upstream devices (server or upstream relay agent),
if this code just happen to find a 'downstream' interface and breakout,
we have no place to do the 'relay-port' change and relay-port option for the
upstream socket. Thus in the patch if this 'relay port' is configured, we at least
need to have tow sockets, one to receive from DHCP client side with the
normal UDP port number, the other interfafce from the server side to receive
message with the user defined UDP port. Thus the code is added actually not to
break out until we at least find two intfs, one upstream and one downstream.
This is done the same way for IPv4 with the bpf filters, and we need
to make sure we receive packets from both up and down streams with
two different port numbers.
(3) on your comment of “relay/dhcrelay.c:process_up6(), Relocate the logic
to add the D60_RELAY_PORT to inside the upstream send loop….”
this is another place my patch needs to have good comments.
the purpose of the diff in process_up6() is not with finding and replacing
the existing option, this relay agent never replaces someone else’s
relay options because IPv6 relay options are encapsulated.
This is to figure out if THIS 'relay agent' needs to format and
send our own D60_RELAY_PORT option. There are two cases
we need to send:
(a) this relay-agent uses a non-dhcp standard UDP port for upstream,
this is the "relay_port" globally defined in dhcrelay.c; and
(b) if our downstream relay-agent uses a non-standard UDP port. For
that, we need to check if inbound port has the != 547 port in
packet->client_port, and record it in 'relay_client_port'.
If either one of the above condition is true, we'll put in this
D60_RELAY_PORT option in our relay option.
(4) if you agree to the patch using the global configuration as ‘-rp’,
I’ll break the patch into 5 smaller patches as you described, and make
good comments to the places the diff may not be obvious. Then
I’ll send to you and for further review.
Or any other suggestions you may have.
Best Regards,
- Naiming
On May 3, 2017, at 4:43 PM, Naiming Shen (naiming) <naiming@cisco.com<mailto:naiming@cisco.com>> wrote:
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<mailto: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.