On Fri, Jan 08, 2021 at 09:42:57PM +0100, Sebastian Benoit wrote:
> Claudio Jeker([email protected]) 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 -0000 1.510
> > +++ rde.c 7 Jan 2021 17:04:53 -0000
> > @@ -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, &peerlist, 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 -0000 1.123
> > +++ rde_update.c 7 Jan 2021 18:13:45 -0000
> > @@ -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, &addr);
> > - 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, &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, &peer->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;
> > - case 0:
> > + if (!up_test_update(peer, new)) {
> > goto withdraw;
> > - case -1:
> > - return;
> > }
> >
> > rde_filterstate_prep(&state, prefix_aspath(new),
> >
>
--
:wq Claudio
