[ovs-dev] [PATCH v5 2/3] ovn-controller: Add a new action - 'put_nd_ra_opts'

[Ben Pfaff]

On Fri, Aug 04, 2017 at 04:09:58PM +0530, Numan Siddique wrote:
> On Fri, Aug 4, 2017 at 3:02 AM, Ben Pfaff <blp at ovn.org> wrote:
> 
> > On Mon, Jul 31, 2017 at 06:11:35PM +0530, nusiddiq at redhat.com wrote:
> > > From: Numan Siddique <nusiddiq at redhat.com>
> > >
> > > This patch adds a new OVN action 'put_nd_ra_opts' to support native
> > > IPv6 Router Advertisement in OVN. This action can be used to respond
> > > to the IPv6 Router Solicitation requests.
> > >
> > > ovn-controller parses this action and adds a NXT_PACKET_IN2 OF flow
> > > with 'pause' flag set and the RA options stored in 'userdata' field.
> > > This action is similar to 'put_dhcp_opts' and 'put_dhcpv6_opts'.
> > >
> > > When a valid IPv6 RS packet is received by the pinctrl module of
> > > ovn-controller, it frames a new RA packet and sets the RA options
> > > from the 'userdata' field and resumes the packet storing 1 in the
> > > 1-bit result sub-field. If the packet is invalid, it resumes the
> > > packet without any modifications storing 0 in the 1-bit result
> > > sub-field.
> > >
> > > Eg. reg0[5] = put_nd_ra_opts(address_mode = "slaac", mtu = 1450,
> > >                              slla = 01:02:03:04:05:06, prefix =
> > aef0::/64)
> > >
> > > Note that unlike DHCPv4/v6, a new table to store the supported IPv6 ND RA
> > > options is not added in SB DB since there are only 3 ND RA options.
> > >
> > > Co-authored-by: Zongkai LI <zealokii at gmail.com>
> > > Signed-off-by: Zongkai LI <zealokii at gmail.com>
> > > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> > > Acked-by: Miguel Angel Ajo <majopela at redhat.com>
> >
> > Thanks for working on this.
> >
> > Looking at it, the treatment of some of the options strikes me as odd.
> > Some of the options (SLL) are actually required, and others could be
> > supplied as fixed data since there's a default value (mo_flags, mtu).
> > Only the prefixes seem to really be options in what I think of as the
> > usual sense.  It looks to me like those prefixes could be supplied
> > directly in the userdata as bytes to append to the packet.  It might be
> > cleaner to make these changes--then there would be less parsing of text
> > options, encoding them as binary options, decoding those binary options,
> > and then re-encoding them again in the packet.
> >
> >
> Thanks for the review Ben. I think what you are suggesting makes sense and
> simplies the code. Based on your comments, this is what I am planning to
> modify this patch.
> 
> "put_nd_ra_opts" action would be like
> 
> res_bit = put_nd_ra_opts(slla, mtu, mo_flags, ...)
> 
>  ovn-northd would include 0 or more prefixes following the mo_flags field
> depending the address mode defined by the user.
> 
> Eg.
> reg0[4] = put_nd_ra_opts(01:02:03:04:05:06, 1450, 0, aef0::/64)
> reg0[4] = put_nd_ra_opts(01:02:03:04:05:06, 1450, 64, aef0::/64, bef0::/64)
> reg0[4] = put_nd_ra_opts(01:02:03:04:05:06, 1450, 128)
> 
> 
> If this makes sense, I will update the patches accordingly. Please let me
> know.

That is one reasonable option, but it is not what I had in mind.  I like
put_nd_ra_opts() syntax because it is very clear, so my thought was to
retain that syntax and change only the format of the data sent to
ovn-controller.