Re: [net-next,v2,4/5] seg6: add support for the SRv6 End.DT4 behavior

[Jakub Kicinski]

On Sat, 14 Nov 2020 02:50:58 +0100 Andrea Mayer wrote:
> Hi Jakub,
> Please see my responses inline:
> 
> On Fri, 13 Nov 2020 15:54:37 -0800
> Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> 
> > On Sat, 14 Nov 2020 00:00:24 +0100 Andrea Mayer wrote:  
> > > On Fri, 13 Nov 2020 13:40:10 -0800
> > > Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > > 
> > > I can tackle the v6 version but how do we face the compatibility issue raised
> > > by Stefano in his message?
> > > 
> > > if it is ok to implement a uAPI that breaks the existing scripts, it is relatively
> > > easy to replicate the VRF-based approach also in v6.  
> > 
> > We need to keep existing End.DT6 as is, and add a separate
> > implementation.  
> 
> ok
> 
> >
> > The way to distinguish between the two could be either by  
> 
> > 1) passing via
> > netlink a flag attribute (which would request use of VRF in both
> > cases);  
> 
> yes, feasible... see UAPI solution 1
> 
> > 2) using a different attribute than SEG6_LOCAL_TABLE for the
> > table id (or perhaps passing VRF's ifindex instead), e.g.
> > SEG6_LOCAL_TABLE_VRF;  
> 
> yes, feasible... see UAPI solution 2
> 
> > 3) or adding a new command
> > (SEG6_LOCAL_ACTION_END_DT6_VRF) which would behave like End.DT4.  
> 
> no, we prefer not to add a new command, because it is better to keep a 
> semantic one-to-one relationship between these commands and the SRv6 
> behaviors defined in the draft.
> 
> 
> UAPI solution 1
> 
> we add a new parameter "vrfmode". DT4 can only be used with the 
> vrfmode parameter (hence it is a required parameter for DT4).
> DT6 can be used with "vrfmode" (new vrf based mode) or without "vrfmode" 
> (legacy mode)(hence "vrfmode" is an optional parameter for DT6)
> 
> UAPI solution 1 examples:
> 
> ip -6 route add 2001:db8::1/128 encap seg6local action End.DT4 vrfmode table 100 dev eth0
> ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 vrfmode table 100 dev eth0
> ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0
> 
> UAPI solution 2
> 
> we turn "table" into an optional parameter and we add the "vrftable" optional
> parameter. DT4 can only be used with the "vrftable" (hence it is a required
> parameter for DT4).
> DT6 can be used with "vrftable" (new vrf mode) or with "table" (legacy mode)
> (hence it is an optional parameter for DT6).
> 
> UAPI solution 2 examples:
> 
> ip -6 route add 2001:db8::1/128 encap seg6local action End.DT4 vrftable 100 dev eth0
> ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 vrftable 100 dev eth0
> ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0
> 
> IMO solution 2 is nicer from UAPI POV because we always have only one 
> parameter, maybe solution 1 is slightly easier to implement, all in all 
> we prefer solution 2 but we can go for 1 if you prefer.

Agreed, 2 looks better to me as well. But let's not conflate uABI with
iproute2's command line. I'm more concerned about the kernel ABI.

BTW you prefer to operate on tables (and therefore require
net.vrf.strict_mode=1) because that's closer to the spirit of the RFC,
correct? As I said from the implementation perspective passing any VRF
ifindex down from user space to the kernel should be fine?