Re: [vpp-dev] Adding IP Addr to Multiple IFs

[Jon Loeliger]

On Fri, Sep 29, 2017 at 3:33 PM, Neale Ranns (nranns) 
wrote:

> Hi Jon,
>
>
>
> We don’t support overlapping subnets on interfaces. I was trying to fix
> the cases where it is errorneously allowed with this patch:
>
>   https://gerrit.fd.io/r/#/c/8057/
>
> but I’ve not yet found all the CSIT failures yet.
>
>
>
> /neale
>
>
>
Neale,

I have submitted a proposed patch for some of the issues here:

https://gerrit.fd.io/r/#/c/8619/

The important part of this patch is simply propagating the lower
level error message up to the API level where it can be used by
the API client.

Thanks,
jdl
___
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev

Re: [vpp-dev] Adding IP Addr to Multiple IFs

[Neale Ranns (nranns)]

Hi Jon,

We don’t support overlapping subnets on interfaces. I was trying to fix the 
cases where it is errorneously allowed with this patch:
  https://gerrit.fd.io/r/#/c/8057/
but I’ve not yet found all the CSIT failures yet.

/neale

From:  on behalf of Jon Loeliger 
Date: Thursday, 28 September 2017 at 10:20
To: vpp-dev 
Subject: [vpp-dev] Adding IP Addr to Multiple IFs

Packet Handlers,

I have a question regarding adding IP address on multiple interfaces.
I know.  Seems like it should be obvious and easy.  But I am not really
a Domain Expert here.  So questions about both expected behavior
and a possible bug.  We'll see.

Let's start with this sequence of vppctl commands:
vpp# show int
  Name   Idx   State  Counter  Count
TenGigabitEthernet6/0/0   1down
TenGigabitEthernet6/0/1   2down
TenGigabitEthernet6/0/2   3down
TenGigabitEthernet6/0/3   4down
local00down
vpp# set interface ip address TenGigabitEthernet6/0/0 
1.2.3.4/32
vpp# set interface ip address TenGigabitEthernet6/0/0 
1.2.3.4/32
set interface ip address: failed to add 1.2.3.4/32 which 
conflicts with 1.2.3.4/32 for interface 
TenGigabitEthernet6/0/0

So VPP has recognized an overlapping request and complains.  Good.
But not that it is an exact overlap and could have let it slide.  Ah well.
vpp# set interface ip address TenGigabitEthernet6/0/1 
1.2.3.4/32
vpp#

Hrm.  No complaint here.  Turns out the address is already associated
with FIB/table_id 0.  OK.

But also, no IP address added to the second IF either:
vpp# show int address
TenGigabitEthernet6/0/0 (dn):
  1.2.3.4/32
TenGigabitEthernet6/0/1 (dn):
TenGigabitEthernet6/0/2 (dn):
TenGigabitEthernet6/0/3 (dn):
local0 (dn):

Which means it was silently accepted, but didn't do what was asked of it.
To be very clear, that means that at the layer above the API call here,
there is no way to determine if that API call was successful or not.  In fact,
that API call always returns "0 == ALL OK" in these cases.  So any layer
above the API that relied on that return value to store configuration requests
in a side DB just got lied to and won't do the Right Thing here.

So, digging.

I think the first layer problem is in function
vl_api_sw_interface_add_del_address_t_handler():
static void vl_api_sw_interface_add_del_address_t_handler
  (vl_api_sw_interface_add_del_address_t * mp)
{
  vlib_main_t *vm = vlib_get_main ();
  vl_api_sw_interface_add_del_address_reply_t *rmp;
  int rv = 0;
  u32 is_del;
  VALIDATE_SW_IF_INDEX (mp);
  is_del = mp->is_add == 0;
  if (mp->del_all)
ip_del_all_interface_addresses (vm, ntohl (mp->sw_if_index));
  else if (mp->is_ipv6)
ip6_add_del_interface_address (vm, ntohl (mp->sw_if_index),
   (void *) mp->address,
   mp->address_length, is_del);
  else
ip4_add_del_interface_address (vm, ntohl (mp->sw_if_index),
   (void *) mp->address,
   mp->address_length, is_del);
  BAD_SW_IF_INDEX_LABEL;
  REPLY_MACRO (VL_API_SW_INTERFACE_ADD_DEL_ADDRESS_REPLY);

}
Notice that the REPLY_MACRO() always picks of a return value as rv = 0.
So we can add "error = ..." to the add and delete arms, and slam it to 0 in the
"delete all" arm, and check for a non-null error to return rv = -1 instead.
Adding all that doesn't fix it.  (Note that over in ip46_cli.c this is 
essentially
the approach taken there.)

Landing in ip4_add_del_interface_address_internal(), the FIB on the sw_if_index
is located and the additional IP addr is checked for conflicts with existing 
entries
in the foreach_ip_interface_address() macro loop.  That's all fine.

It then passes the buck to  ip_interface_address_add_del() along with the FIB.
The outline of that routine sort of goes like this:

addr = lookup IP addr in FIB table
check something

if (del)
delete ip addr
set invalid addr index return parameter
else if (addr failed to lookup above)
add ip addr
set new addr index return parameter
else
silently say we have ip addr already
set addr index from lookup in return parameter

return success

That last "return success" basically means everything will always
be happy all the way back to an initial API call essentially every time.

So.  In the case of adding the same IP address to a second IF, that
just happens to have the same (default) FIB on it, I think control flow
ends up in the "else" clause above.  And that always works.
Instead, is there a way to check if the addr is being added to an IF
whose sw_if_index is the same-as or different-from the sw_if_index
of the original addr in the FIB entry?  Could we return "all happy"
if they