[quagga-dev 4454] Re: [quagga-users 7613] Re: FW: Unicast routing with OSPF

Andrew J. Schorr aschorr at telemetry-investments.com
Fri Oct 27 17:23:20 BST 2006


On Fri, Oct 13, 2006 at 02:02:39PM +0100, Paul Jakma wrote:
> On Wed, 11 Oct 2006, Andrew J. Schorr wrote:
> >So why is CONNECTED_ID required?  The basic idea is that there may 
> >be some situations in which you want to try to identify a 
> >particular 'struct connected' by a single associated address.  In 
> >general, this is not possible, but there are nonetheless places in 
> >the code that call for this.
> 
> Ok. So we need a 'best effort'?

That's my thinking.  I could certainly be wrong.

> >But there's one corner case in which the user has assigned both a 
> >local and a peer address, and they both live in the same subnet. 
> >For example, suppose the local address is 192.168.1.1/30, and the 
> >peer address is 192.168.1.2/30. In principle, there was probably no 
> >need to specify the peer address in such a case, since the subnet 
> >assignment makes it clear enough what needs to be done.  But 
> >certainly linux, and I guess probably other platforms as well, do 
> >not stop you from assigning a peer address in the same subnet. 
> >And in such a case, I think you most likely want to use 192.168.1.1 
> >as the identifier, not 192.168.1.2.
> 
> Ok. Can that distinction not be handled in CONNECTED_PREFIX though?

In principle, we could just redefine CONNECTED_PREFIX using the
current definition for CONNECTED_ID.  And we could get rid of the CONNECTED_ID
macro and use the new CONNECTED_PREFIX everywhere, I think.  The problem
is that the CONNECTED_ID is much more inefficient than the current
CONNECTED_PREFIX macro, since CONNECTED_ID calls prefix_match.  So I wanted
to minimize usage of that macro.

> 
> >OK, so where are the 3 places in the quagga code that actually require
> >such an identifier?
> >
> >1. In ripd/rip_interface.c:rip_interface_multicast_set, we have this
> >code:
> >
> > addr = CONNECTED_ID(connected)->u.prefix4;
> >
> > if (setsockopt_multicast_ipv4 (sock, IP_MULTICAST_IF, addr, 0,
> >                                connected->ifp->ifindex) < 0)
> >
> >So which address should we pass to setsockopt_multicast_ipv4?  If 
> >we're lucky, we're on a platform that can use 
> >connected->ifp->ifindex to identify the interface to the kernel. 
> >But if we're not on such a platform, we need to try to select the 
> >address that the kernel would use to identify the interface.
> 
> >Would it be OK always to use the peer address if it exists (and 
> >ignore the strange case where peer and local are in the same 
> >subnet)?
> 
> Hmm, no idea. If a peer is specified in the address, then surely that 
> would always work? I.e. wouldn't relying on local IP to work be the 
> riskier in such a case?

I think this is really just a kernel implementation issue, and I don't
have the experience to answer this question.

> > Perhaps, I couldn't say without going through all the 
> >kernel code.  In linux, the relevant logic seems to be in 
> >net/ipv4/fib_frontend.c:ip_dev_find where it calls 
> >ip_fib_local_table->tb_lookup(address), but it's not obvious to me 
> >which address we need.  But if I conduct the following test:
> >
> >bash$ modprobe ip_gre
> >bash$ ip tunnel add ptp0 mode gre remote 192.168.50.31 local 192.168.20.93
> >bash$ ip link set ptp0 up multicast on
> >bash$ ip addr add 192.168.52.1/30 peer 192.168.52.2/30 brd + dev ptp0
> >bash$ ip route ls table all | fgrep .52.
> >192.168.52.0/30 dev ptp0  proto kernel  scope link  src 192.168.52.1
> >broadcast 192.168.52.0 dev ptp0  table local  proto kernel  scope link  
> >src 192.168.52.1
> >local 192.168.52.1 dev ptp0  table local  proto kernel  scope host  src 
> >192.168.52.1
> >broadcast 192.168.52.3 dev ptp0  table local  proto kernel  scope link  
> >src 192.168.52.1
> >
> >I get the impression that I need the 192.168.52.1 address in this case
> >(since there is no mention of 192.168.52.2 in the routing table), but
> >I certainly have doubts and am open to other opinions.
> 
> I don't know either. It seems, for the above, either local or peer 
> would work. We use the local address always though, when we use an 
> address.
> 
> What happens if you configure it such that the peer address does 
> /not/ fall within the local prefix?

I think that's the normal (usual) case where we would assume that the
local address is not unique.  In that case, CONNECTED_ID is the
same as CONNECTED_PREFIX.  So we will pass the peer address to the
kernel (same as current behavior).

Note that in general, my patch tries to mimic current behavior
except in cases where it's obviously wrong.  I think/hope there
are no regressions.

> >2. In ospfd/ospf_snmp.c:ospf_snmp_if_update, it needs an address
> >to use to identify the interface.  As discussed above, CONNECTED_ID
> >is my best guess as to what that address should be (and should give
> >consistent behavior with the current code).
> 
> For SNMP, the identifier most definitely is the peer. If "Peer is the 
> identifier" is more than a convention anywhere, SNMP is it :) 
> (MIB-II, I'll have to look it up, but OSPF RFC2385 refers to it for 
> PtP link construction in router-LSAs).

OK, but keep in mind we're basically discussing a case that is
not handled well by the current code.  There are basically 3 cases
when a peer address is set:

   1. We are using /32 addressing, and the identifying address is
   the peer address.  This is the most common case.

   2. We are not using /32 addressing, and the local and peer addresses
   are in the same prefix.  This case is handled by the current code,
   and the local address is used as the identifying address in such
   a case.  This behavior is preserved by using the CONNECTED_ID macro.

   3. We are not using /32 addressing, and the local and peer addresses
   are NOT in the same prefix.  This is a an unusual situation, and I
   think it is handled incorrectly by the current code.  At the moment,
   we would use the local address to identify that interface.  But I would
   argue that we should use the peer address instead, and that's what
   the CONNECTED_ID macro does.

> >3. In ospfd/ospfd.c:ospf_network_run.  This is the most complicated
> >case.  But first off, let me again note that using CONNECTED_ID will
> >be consistent with the current behavior, since the patch looks like this:
> 
> >-         if (CONNECTED_POINTOPOINT_HOST(co))
> >-           addr = co->destination;
> >-         else
> >-           addr = co->address;
> >+         addr = CONNECTED_ID(co);
> 
> >So the old code (part of my previous PtP patch) was ugly to begin with. :-)
> >So what is the 'addr' variable used for?  It's used in one spot, where
> >this call is made: ospf_if_is_configured(ospf, &(addr->u.prefix4)).
> >We then have to look at ospf_interface.c:ospf_if_is_configured to see
> >what it expects as an argument.  If oi->type is OSPF_IFTYPE_POINTOPOINT,
> >then the CONNECTED_ID address should match fine (since CONNECTED_PREFIX
> >should always match CONNECTED_ID).  And it seems very unlikely
> >that there would be a peer address if the interface type is
> >not set to OSPF_IFTYPE_POINTOPOINT, so I don't think we really need
> >to worry about the other case very much.  So I think we could possibly
> >use CONNECTED_PREFIX in this place instead of CONNECTED_ID,
> >if you prefer.  But it might break some strange case where the
> >admin assigned a peer address, but did not set the interface
> >type to OSPF_IFTYPE_POINTOPOINT...
> 
> We should probably automatically set OSPF_IFTYPE to PtP, unless 
> configured explicitely otherwise. Isn't this what your patch does 
> elsewhere in ospfd?

No, I don't do this at the moment.  It was not obvious to me how to
implement this robustly.  Currently, the logic in question is in
ospf_interface.c:ospf_default_iftype.  It's quite trivial:

   u_char
   ospf_default_iftype(struct interface *ifp)
   {
     if (if_is_pointopoint (ifp))
       return OSPF_IFTYPE_POINTOPOINT;
     else if (if_is_loopback (ifp))
       return OSPF_IFTYPE_LOOPBACK;
     else
       return OSPF_IFTYPE_BROADCAST;
   }

Ideally, we would inspect the interface's struct connected address info
to make the decision.  But this function is called from
osfp_zebra.c:ospf_interface_add, just after instantiating the
interface by calling zebra_interface_add_read.  So I don't think
the connected address information is available at this moment.

My thinking was that this is probably OK: if somebody is running
PtP over a broadcast medium, it seemed reasonable to me to require
them to configure the OSPF_IFTYPE to PtP explicitly.  Do you disagree?

> >By the way, this raises the whole question of what the 
> >ospf_interface 'address' field is supposed to represent.
> 
> The local address, always. But yeah, confused.

Agreed.  The local address is of limited value.  I suspect that
the ospf_interface structure should really not have an 'address' field
at all, and we should audit and replace all uses, but I don't have the
energy for this right now...

In summary, I would argue that my patch cleans up the code quite a bit,
improves behavior in some cases (should allow PtP configs to work
with broadcast mediums), and should not introduce regressions.  But
I agree it's not perfect, and some inelegant bits should ultimately
be ironed out.  But I claim these problems/issues were already in the
code, and are not being introduced by this patch.  I would say that
this patch is actually exposing them by imposing a more elegant structure
that makes the residual cruft more obvious.

So I would argue that we should commit the patch and move forward from
there.  I've had this in my private tree for months now, and I'd love
to get it integrated.  I think it's a step in the right direction.
My only concern is whether the various platform-specific stuff in
zebra to dig up the peer addresses has been implemented properly...

Regards,
Andy



More information about the Quagga-dev mailing list