Re: bgpd simplify update path
On Fri, Jan 08, 2021 at 09:42:57PM +0100, Sebastian Benoit wrote:
> Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.01.07 19:34:23 +0100:
> > When bgpd generates an UPDATE to update or withdraw prefixes it does this
> > from rde_generate_updates() and then decends into up_generate_update().
> > Now there is up_test_update() that checks if a new prefix is actually OK
> > to be distributed. It checks things for route reflectors and the common
> > communities (NO_EXPORT, ...). There are a few more checks that are pure
> > peer config checks and those should be moved up to rde_generate_updates().
> >
> > Last but not least there is this bit about ORIGINATOR_ID which seems
> > sensible but on second thought I think it is actually wrong and an
> > extension on top of the RFC. Since I think this code currently has not the
> > right withdraw behaviour I decided it is the best to just remove it.
>
> I think it should not matter because the receiving router will do the same
> check (against its own id) and ignore the update:
>
> A router [that recognizes the ORIGINATOR_ID attribute] SHOULD
> ignore a route received with its BGP Identifier as the ORIGINATOR_ID.
> (RFC 4456)
>
> However your change is correct because the RFC does say that the receiver
> should make this descision. We do seem to correctly check that when
> receiving updates in rde_reflector().
What I wonder about is that because of return -1 the prefix is actually
not withdrawn from the neighbor. So an old prefix could get stuck on the
peer. This is why I think it is best to remove this.
> > This code simplifies the return of up_test_update() to a pure true / false
> > case and make up_generate_update() simpler. Also I think doing the peer
> > checks early on will improve performance.
>
> ok benno@
> one whitespace error below
Will fix those and then commit. Thanks
> >
> > Please review :)
> > --
> > :wq Claudio
> >
> > Index: rde.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> > retrieving revision 1.510
> > diff -u -p -r1.510 rde.c
> > --- rde.c 30 Dec 2020 07:29:56 - 1.510
> > +++ rde.c 7 Jan 2021 17:04:53 -
> > @@ -2814,7 +2814,8 @@ rde_send_kroute(struct rib *rib, struct
> > void
> > rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix
> > *old)
> > {
> > - struct rde_peer *peer;
> > + struct rde_peer *peer;
> > + u_int8_t aid;
> >
> > /*
> > * If old is != NULL we know it was active and should be removed.
> > @@ -2824,6 +2825,11 @@ rde_generate_updates(struct rib *rib, st
> > if (old == NULL && new == NULL)
> > return;
> >
> > + if (new)
> > + aid = new->pt->aid;
> > + else
> > + aid = old->pt->aid;
> > +
> > LIST_FOREACH(peer, , peer_l) {
> > if (peer->conf.id == 0)
> > continue;
> > @@ -2831,6 +2837,14 @@ rde_generate_updates(struct rib *rib, st
> > continue;
> > if (peer->state != PEER_UP)
> > continue;
> > + /* check if peer actually supports the address family */
> > + if (peer->capa.mp[aid] == 0)
> > + continue;
> > + /* skip peers with special export types */
>
> spaces instead of tabs
>
>
> > + if (peer->conf.export_type == EXPORT_NONE ||
> > + peer->conf.export_type == EXPORT_DEFAULT_ROUTE)
> > + continue;
> > +
> > up_generate_updates(out_rules, peer, new, old);
> > }
> > }
> > Index: rde_update.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> > retrieving revision 1.123
> > diff -u -p -r1.123 rde_update.c
> > --- rde_update.c24 Jan 2020 05:44:05 - 1.123
> > +++ rde_update.c7 Jan 2021 18:13:45 -
> > @@ -47,11 +47,9 @@ static struct community comm_no_expsubco
> > static int
> > up_test_update(struct rde_peer *peer, struct prefix *p)
> > {
> > - struct bgpd_addr addr;
> > struct rde_aspath *asp;
> > struct rde_community*comm;
> > struct rde_peer *prefp;
> > - struct attr *attr;
> >
> > if (p == NULL)
> > /* no prefix available */
> > @@ -70,10 +68,6 @@ up_test_update(struct rde_peer *peer, st
> > if (asp->flags & F_ATTR_LOOP)
> > fatalx("try to send out a looped path");
> >
> > - pt_getaddr(p->pt, );
> > - if (peer->capa.mp[addr.aid] == 0)
> > - return (-1);
> > -
> > if (!prefp->conf.ebgp && !peer->conf.ebgp) {
> > /*
> > * route reflector redistribution rules:
> > @@ -90,16 +84,6 @@ up_test_update(struct rde_peer *peer, st
> > return (0);
> > }
> >
> > - /* export type handling */
> > - if (peer->conf.export_type == EXPORT_NONE ||
> > -
Re: bgpd simplify update path
Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.01.07 19:34:23 +0100:
> When bgpd generates an UPDATE to update or withdraw prefixes it does this
> from rde_generate_updates() and then decends into up_generate_update().
> Now there is up_test_update() that checks if a new prefix is actually OK
> to be distributed. It checks things for route reflectors and the common
> communities (NO_EXPORT, ...). There are a few more checks that are pure
> peer config checks and those should be moved up to rde_generate_updates().
>
> Last but not least there is this bit about ORIGINATOR_ID which seems
> sensible but on second thought I think it is actually wrong and an
> extension on top of the RFC. Since I think this code currently has not the
> right withdraw behaviour I decided it is the best to just remove it.
I think it should not matter because the receiving router will do the same
check (against its own id) and ignore the update:
A router [that recognizes the ORIGINATOR_ID attribute] SHOULD
ignore a route received with its BGP Identifier as the ORIGINATOR_ID.
(RFC 4456)
However your change is correct because the RFC does say that the receiver
should make this descision. We do seem to correctly check that when
receiving updates in rde_reflector().
> This code simplifies the return of up_test_update() to a pure true / false
> case and make up_generate_update() simpler. Also I think doing the peer
> checks early on will improve performance.
ok benno@
one whitespace error below
>
> Please review :)
> --
> :wq Claudio
>
> Index: rde.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.510
> diff -u -p -r1.510 rde.c
> --- rde.c 30 Dec 2020 07:29:56 - 1.510
> +++ rde.c 7 Jan 2021 17:04:53 -
> @@ -2814,7 +2814,8 @@ rde_send_kroute(struct rib *rib, struct
> void
> rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix *old)
> {
> - struct rde_peer *peer;
> + struct rde_peer *peer;
> + u_int8_t aid;
>
> /*
>* If old is != NULL we know it was active and should be removed.
> @@ -2824,6 +2825,11 @@ rde_generate_updates(struct rib *rib, st
> if (old == NULL && new == NULL)
> return;
>
> + if (new)
> + aid = new->pt->aid;
> + else
> + aid = old->pt->aid;
> +
> LIST_FOREACH(peer, , peer_l) {
> if (peer->conf.id == 0)
> continue;
> @@ -2831,6 +2837,14 @@ rde_generate_updates(struct rib *rib, st
> continue;
> if (peer->state != PEER_UP)
> continue;
> + /* check if peer actually supports the address family */
> + if (peer->capa.mp[aid] == 0)
> + continue;
> + /* skip peers with special export types */
spaces instead of tabs
> + if (peer->conf.export_type == EXPORT_NONE ||
> + peer->conf.export_type == EXPORT_DEFAULT_ROUTE)
> + continue;
> +
> up_generate_updates(out_rules, peer, new, old);
> }
> }
> Index: rde_update.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> retrieving revision 1.123
> diff -u -p -r1.123 rde_update.c
> --- rde_update.c 24 Jan 2020 05:44:05 - 1.123
> +++ rde_update.c 7 Jan 2021 18:13:45 -
> @@ -47,11 +47,9 @@ static struct communitycomm_no_expsubco
> static int
> up_test_update(struct rde_peer *peer, struct prefix *p)
> {
> - struct bgpd_addr addr;
> struct rde_aspath *asp;
> struct rde_community*comm;
> struct rde_peer *prefp;
> - struct attr *attr;
>
> if (p == NULL)
> /* no prefix available */
> @@ -70,10 +68,6 @@ up_test_update(struct rde_peer *peer, st
> if (asp->flags & F_ATTR_LOOP)
> fatalx("try to send out a looped path");
>
> - pt_getaddr(p->pt, );
> - if (peer->capa.mp[addr.aid] == 0)
> - return (-1);
> -
> if (!prefp->conf.ebgp && !peer->conf.ebgp) {
> /*
>* route reflector redistribution rules:
> @@ -90,16 +84,6 @@ up_test_update(struct rde_peer *peer, st
> return (0);
> }
>
> - /* export type handling */
> - if (peer->conf.export_type == EXPORT_NONE ||
> - peer->conf.export_type == EXPORT_DEFAULT_ROUTE) {
> - /*
> - * no need to withdraw old prefix as this will be
> - * filtered out as well.
> - */
> - return (-1);
> - }
> -
> /* well known communities */
> if (community_match(comm, _no_advertise, NULL))
> return (0);
> @@ -110,18 +94,6 @@ up_test_update(struct rde_peer *peer, st
> return
bgpd simplify update path
When bgpd generates an UPDATE to update or withdraw prefixes it does this
from rde_generate_updates() and then decends into up_generate_update().
Now there is up_test_update() that checks if a new prefix is actually OK
to be distributed. It checks things for route reflectors and the common
communities (NO_EXPORT, ...). There are a few more checks that are pure
peer config checks and those should be moved up to rde_generate_updates().
Last but not least there is this bit about ORIGINATOR_ID which seems
sensible but on second thought I think it is actually wrong and an
extension on top of the RFC. Since I think this code currently has not the
right withdraw behaviour I decided it is the best to just remove it.
This code simplifies the return of up_test_update() to a pure true / false
case and make up_generate_update() simpler. Also I think doing the peer
checks early on will improve performance.
Please review :)
--
:wq Claudio
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.510
diff -u -p -r1.510 rde.c
--- rde.c 30 Dec 2020 07:29:56 - 1.510
+++ rde.c 7 Jan 2021 17:04:53 -
@@ -2814,7 +2814,8 @@ rde_send_kroute(struct rib *rib, struct
void
rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix *old)
{
- struct rde_peer *peer;
+ struct rde_peer *peer;
+ u_int8_t aid;
/*
* If old is != NULL we know it was active and should be removed.
@@ -2824,6 +2825,11 @@ rde_generate_updates(struct rib *rib, st
if (old == NULL && new == NULL)
return;
+ if (new)
+ aid = new->pt->aid;
+ else
+ aid = old->pt->aid;
+
LIST_FOREACH(peer, , peer_l) {
if (peer->conf.id == 0)
continue;
@@ -2831,6 +2837,14 @@ rde_generate_updates(struct rib *rib, st
continue;
if (peer->state != PEER_UP)
continue;
+ /* check if peer actually supports the address family */
+ if (peer->capa.mp[aid] == 0)
+ continue;
+ /* skip peers with special export types */
+ if (peer->conf.export_type == EXPORT_NONE ||
+ peer->conf.export_type == EXPORT_DEFAULT_ROUTE)
+ continue;
+
up_generate_updates(out_rules, peer, new, old);
}
}
Index: rde_update.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
retrieving revision 1.123
diff -u -p -r1.123 rde_update.c
--- rde_update.c24 Jan 2020 05:44:05 - 1.123
+++ rde_update.c7 Jan 2021 18:13:45 -
@@ -47,11 +47,9 @@ static struct community comm_no_expsubco
static int
up_test_update(struct rde_peer *peer, struct prefix *p)
{
- struct bgpd_addr addr;
struct rde_aspath *asp;
struct rde_community*comm;
struct rde_peer *prefp;
- struct attr *attr;
if (p == NULL)
/* no prefix available */
@@ -70,10 +68,6 @@ up_test_update(struct rde_peer *peer, st
if (asp->flags & F_ATTR_LOOP)
fatalx("try to send out a looped path");
- pt_getaddr(p->pt, );
- if (peer->capa.mp[addr.aid] == 0)
- return (-1);
-
if (!prefp->conf.ebgp && !peer->conf.ebgp) {
/*
* route reflector redistribution rules:
@@ -90,16 +84,6 @@ up_test_update(struct rde_peer *peer, st
return (0);
}
- /* export type handling */
- if (peer->conf.export_type == EXPORT_NONE ||
- peer->conf.export_type == EXPORT_DEFAULT_ROUTE) {
- /*
-* no need to withdraw old prefix as this will be
-* filtered out as well.
-*/
- return (-1);
- }
-
/* well known communities */
if (community_match(comm, _no_advertise, NULL))
return (0);
@@ -110,18 +94,6 @@ up_test_update(struct rde_peer *peer, st
return (0);
}
- /*
-* Don't send messages back to originator
-* this is not specified in the RFC but seems logical.
-*/
- if ((attr = attr_optget(asp, ATTR_ORIGINATOR_ID)) != NULL) {
- if (memcmp(attr->data, >remote_bgpid,
- sizeof(peer->remote_bgpid)) == 0) {
- /* would cause loop don't send */
- return (-1);
- }
- }
-
return (1);
}
@@ -149,13 +121,8 @@ withdraw:
peer->up_wcnt++;
}
} else {
- switch (up_test_update(peer, new)) {
- case 1:
- break;
-
