From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: rfc8654 extended message support
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 9 Dec 2024 11:42:15 +0100

On Mon, Dec 09, 2024 at 10:34:04AM +0100, Theo Buehler wrote:
> On Tue, Dec 03, 2024 at 03:51:22PM +0100, Claudio Jeker wrote:
> > Since we can handle imsg > 16k now we can also handle 64k BGP messages.
> > So implement the bits for extended message support.
> > 
> > This should mostly follow rfc8654 with the following differences:
> > 
> > - NOTIFICATIONS are always truncated to fit in 4096 bytes. This is so that
> >   early errors do not ship too large packets. Also I see little point in
> >   having huge NOTIFICATIONS.
> > 
> > - I did not implement the SHOULD in:
> >     When propagating that UPDATE onward to a neighbor that has not advertised
> >     the BGP Extended Message Capability, the speaker SHOULD try to reduce the
> >     outgoing message size by removing attributes eligible under the "attribute
> >     discard" approach of [RFC7606].
> >   bgpd does the withdraw of previous NLRI since a while. There are only a
> >   few attributes that use "attribute discard" and in most cases their size
> >   is not the problem (or we already discarded them on input). So
> >   implementing that seems like a waste of time.
> > 
> > - I decided that bgpd only sends large messages if both sides announce
> >   extended messages. In other words if the local bgpd has not set it then
> >   no large message will be sent (even though the remote annouces it) so we
> >   take the MAY in:
> >     A BGP speaker MAY send BGP Extended Messages to a peer only if the BGP
> >     Extended Message Capability was received from that peer.
> > 
> > I tested this against a modifed version of the maxattr.sh regress test.
> > That part will be committed once this is in.
> 
> This seems to do what it says on the tin. I can't spot anything wrong with it.
> Let's get this in.
> 
> ok tb
> 
> Two nits below.
> 
> > Index: usr.sbin/bgpd/bgpd.conf.5
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
> > diff -u -p -r1.242 bgpd.conf.5
> > --- usr.sbin/bgpd/bgpd.conf.5	14 Aug 2024 19:09:51 -0000	1.242
> > +++ usr.sbin/bgpd/bgpd.conf.5	3 Dec 2024 14:39:29 -0000
> > @@ -1091,6 +1091,22 @@ The default is
> >  .Ic no .
> >  .Pp
> >  .It Xo
> > +.Ic announce extended
> > +.Pq Ic yes Ns | Ns Ic no Ns | Ns Ic enforce
> > +.Xc
> > +If set to
> > +.Ic yes ,
> > +the extended message capability is announced.
> > +If negotiated the default maximum message size is increaded from 4096 to 65535
> 
> Missing comma after negotiated.

Fixed.
 
> [...]
> 
> > Index: usr.sbin/bgpd/parse.y
> > ===================================================================
> 
> [...]
> 
> > @@ -3536,6 +3539,7 @@ lookup(char *s)
> >  		{ "export",		EXPORT},
> >  		{ "export-target",	EXPORTTRGT},
> >  		{ "ext-community",	EXTCOMMUNITY},
> > +		{ "extended",		EXTENDED },
> 
> other members have no space before },

Actually that table is a mess. There are both versions. I want to unify
this. I thought of adding a space before } for all of them. What do you
prefer?

> >  		{ "fib-priority",	FIBPRIORITY},
> >  		{ "fib-update",		FIBUPDATE},
> >  		{ "filtered",		FILTERED},
> 

-- 
:wq Claudio