[PATCH,RFC 1/3] BGP: Implement secondary remote AS number support.

[Lennert Buytenhek]

On Thu, Mar 09, 2017 at 05:53:51PM +0100, Ondrej Zajicek wrote:

> > This patch implements the latter option, and this functionality is
> > enabled by specifying two different remote AS numbers for a peer in
> > the neighbor statement.  Instead of specifying:
> > 
> > 	neighbor 1.2.3.4 as 12345;
> > 
> > Specify this:
> > 
> > 	neighbor 1.2.3.4 as 12345 as 23456;
> 
> I would prefer if the secondary as would either have a separate keyword,
> (e.g. neighbor 1.2.3.4 as 12345 secondary as 23456) or it was just plain
> positional (e.g. neighbor 1.2.3.4 as 12345 23456).

Right, okay.  There isn't really a difference in the code between the
"primary" and the "secondary" AS (currently they are remote_as and
remote_as2, and remote_as2 isn't always set, but the two variables are
entirely interchangeable if they are both set), so I would prefer the
latter option.  I'll change the patch accordingly.


> > (bird's config grammar already allows specifying multiple remote AS
> > numbers for a peer, and it will use the last AS number you specify
> > this way if you do that.  With this patch, it will use the last two
> > AS numbers you specify.)
> > ---
> >  proto/bgp/bgp.c     | 17 +++++++++--------
> >  proto/bgp/bgp.h     |  2 +-
> >  proto/bgp/config.Y  |  2 +-
> >  proto/bgp/packets.c |  8 ++++++--
> >  4 files changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c
> > index 0f1c944..bceae4a 100644
> > --- a/proto/bgp/bgp.c
> > +++ b/proto/bgp/bgp.c
> > @@ -1278,8 +1278,6 @@ bgp_init(struct proto_config *C)
> >  
> >    p->cf = c;
> >    p->local_as = c->local_as;
> > -  p->remote_as = c->remote_as;
> > -  p->is_internal = (c->local_as == c->remote_as);
> >    p->rs_client = c->rs_client;
> >    p->rr_client = c->rr_client;
> >    p->igp_table = get_igp_table(c);
> > @@ -1291,8 +1289,6 @@ bgp_init(struct proto_config *C)
> >  void
> >  bgp_check_config(struct bgp_config *c)
> >  {
> > -  int internal = (c->local_as == c->remote_as);
> > -
> 
> I don't like changes in bgp_check_config(). I think that if you set
> remote_as as EBGP, while remote_as2 as IBGP, you could end up with
> inconsistent settings. IMHO allowing remote_as2 only in EBGP makes
> sense, as in IBGP remote ASN must be the same as local ASN. So we
> could make internal / is_internal fixed, as it was.

My main use case for this patch is actually to migrate a data center
network from iBGP to eBGP, with a setup that is vaguely similar to
the one described in rfc7938.  I tried to be careful to keep the
behavior in bgp_check_config() consistent, and whenever iBGP versus
eBGP matters, I tried to err on the side of caution, e.g.:

* If multihop isn't set explicitly, it would default to 64 for iBGP
  or 0 for eBGP, and with this patch, it will default to 0 if either
  remote_as or remote_as2 indicate eBGP, because you don't want to
  default it to 64 if there is the potential that the session will
  establish as eBGP.

* If rr_client is configured, both remote_as and remote_as2 must
  indicate eBGP (or remote_as must indicate eBGP and remote_as2 must
  not be set).

* If rs_client is configured, both remote_as and remote_as2 must
  indicate iBGP (or, much more likely, remote_as must indicate iBGP
  and remote_as2 must not be set).

I accordingly moved the is_internal assignment into bgp_rx_open(),
because only when we receive the remote's OPEN can we know whether
this session is actually going to be iBGP or eBGP.

I'm sitting on a bunch more patches related to this, for example a
per-peer toggle to consider routes from an eBGP peer as iBGP routes
as far as the RFC 4271 9.1.2.2. d) check (Prefer external peers) is
concerned, and an extension to 'path metric' to skip leading private
ASes in the AS_PATH when comparing path lengths, et cetera, but I am
not actually a fan of using eBGP in the data center.  The main reason
why people do this, as far as I can see, is that using eBGP gives you
a superior route redistribution and loop detection model, and IMHO,
iBGP should have had some intra-AS variant of AS_PATH with router IDs
instead of AS numbers (called, say, INTRA_AS_PATH) -- and then we
wouldn't have needed ugly hacks like route reflectors either.

That said, these patches are invaluable to us, but they might not
all make a lot of sense to merge upstream.  We carry a lot more bird
patches internally, for example a patch to do ADD_PATHS only for a
subset of prefixes, more fine-grained control over how to merge
multipath BGP routes, and we have our own way to inject MPLS routes
(!), and my goal was to try a bit harder than we traditionally have
to get things upstream, but if you think that something doesn't make
sense to support in the upstream version of bird, then I totally
understand that, as we do have some pretty specific use cases.


Cheers,
Lennert