Improve reload behaviour of RDE peer flags and export_type.

Add an extra reload barrier (IMSG_RECONF_DRAIN) to the sync of the peer
config from the session engine to the rde. Necessary to ensure that the
peer config is up to date in the RDE before hitting reconfiguration.

Store the export_type and the peer flags outside of peer->conf. Adjust all
users of these two fields so they only look at the copies in peer.
During reload check the values with the peer->conf to check for changes.

If the export_type or the rde evaluate or transparent-as flags changed
flush the Adj-RIB-Out for that peer and in a 2nd step rebuild the RIB from
scratch. This results in a lot of UPDATE churn but these configs are not
altered often.

Fix multiple issues in the rde_softreconfig_in_done handler that resulted
in multiple runs of the out stage of the softreconfig pipeline.

OK benno@

diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c
index bb3be1a..0c36005 100644 (file)
--- a/usr.sbin/bgpd/rde.c
+++ b/usr.sbin/bgpd/rde.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: rde.c,v 1.519 2021/04/27 09:07:10 claudio Exp $ */
+/*     $OpenBSD: rde.c,v 1.520 2021/05/06 09:18:54 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -666,6 +666,10 @@ badnetdel:
                                rde_dump_ctx_throttle(imsg.hdr.pid, 1);
                        }
                        break;
+               case IMSG_RECONF_DRAIN:
+                       imsg_compose(ibuf_se, IMSG_RECONF_DRAIN, 0, 0,
+                           -1, NULL, 0);
+                       break;
                default:
                        break;
                }
@@ -2136,7 +2140,7 @@ rde_update_log(const char *message, u_int16_t rid,
        char            *p = NULL;
 
        if (!((conf->log & BGPD_LOG_UPDATES) ||
-           (peer->conf.flags & PEERFLAG_LOG_UPDATES)))
+           (peer->flags & PEERFLAG_LOG_UPDATES)))
                return;
 
        if (next != NULL)
@@ -2919,7 +2923,7 @@ rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix *old,
                if (peer->state != PEER_UP)
                        continue;
                /* handle evaluate all, keep track if it is needed */
-               if (peer->conf.flags & PEERFLAG_EVALUATE_ALL)
+               if (peer->flags & PEERFLAG_EVALUATE_ALL)
                        rde_eval_all = 1;
                else if (eval_all)
                        /* skip default peers if the best path didn't change */
@@ -2931,8 +2935,8 @@ rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix *old,
                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)
+               if (peer->export_type == EXPORT_NONE ||
+                   peer->export_type == EXPORT_DEFAULT_ROUTE)
                        continue;
 
                up_generate_updates(out_rules, peer, new, old);
@@ -3289,13 +3293,34 @@ rde_reload_done(void)
                        continue;
                peer->reconf_out = 0;
                peer->reconf_rib = 0;
+               if (peer->export_type != peer->conf.export_type) {
+                       log_peer_info(&peer->conf, "export type change, "
+                           "reloading");
+                       peer->reconf_rib = 1;
+               }
+               if ((peer->flags & PEERFLAG_EVALUATE_ALL) !=
+                   (peer->conf.flags & PEERFLAG_EVALUATE_ALL)) {
+                       log_peer_info(&peer->conf, "rde evaluate change, "
+                           "reloading");
+                       peer->reconf_rib = 1;
+               }
+               if ((peer->flags & PEERFLAG_TRANS_AS) !=
+                   (peer->conf.flags & PEERFLAG_TRANS_AS)) {
+                       log_peer_info(&peer->conf, "transparent-as change, "
+                           "reloading");
+                       peer->reconf_rib = 1;
+               }
                if (peer->loc_rib_id != rib_find(peer->conf.rib)) {
                        log_peer_info(&peer->conf, "rib change, reloading");
                        peer->loc_rib_id = rib_find(peer->conf.rib);
                        if (peer->loc_rib_id == RIB_NOTFOUND)
                                fatalx("King Bula's peer met an unknown RIB");
                        peer->reconf_rib = 1;
-                       softreconfig++;
+               }
+               peer->export_type = peer->conf.export_type;
+               peer->flags = peer->conf.flags;
+
+               if (peer->reconf_rib) {
                        if (prefix_dump_new(peer, AID_UNSPEC,
                            RDE_RUNNER_ROUNDS, NULL, rde_up_flush_upcall,
                            rde_softreconfig_in_done, NULL) == -1)
@@ -3362,15 +3387,15 @@ rde_reload_done(void)
 
        log_info("RDE reconfigured");
 
+       softreconfig++;
        if (reload > 0) {
-               softreconfig++;
                if (rib_dump_new(RIB_ADJ_IN, AID_UNSPEC, RDE_RUNNER_ROUNDS,
-                   rib_byid(RIB_ADJ_IN), rde_softreconfig_in,
-                   rde_softreconfig_in_done, NULL) == -1)
+                   NULL, rde_softreconfig_in, rde_softreconfig_in_done,
+                   NULL) == -1)
                        fatal("%s: rib_dump_new", __func__);
                log_info("running softreconfig in");
        } else {
-               rde_softreconfig_in_done(NULL, AID_UNSPEC);
+               rde_softreconfig_in_done((void *)1, AID_UNSPEC);
        }
 }
 
@@ -3380,14 +3405,13 @@ rde_softreconfig_in_done(void *arg, u_int8_t dummy)
        struct rde_peer *peer;
        u_int16_t        i;
 
-       if (arg != NULL) {
-               softreconfig--;
-               /* one guy done but other dumps are still running */
-               if (softreconfig > 0)
-                       return;
+       softreconfig--;
+       /* one guy done but other dumps are still running */
+       if (softreconfig > 0)
+               return;
 
+       if (arg == NULL)
                log_info("softreconfig in done");
-       }
 
        /* now do the Adj-RIB-Out sync and a possible FIB sync */
        softreconfig = 0;
@@ -3419,11 +3443,10 @@ rde_softreconfig_in_done(void *arg, u_int8_t dummy)
                u_int8_t aid;
 
                if (peer->reconf_out) {
-                       if (peer->conf.export_type == EXPORT_NONE) {
+                       if (peer->export_type == EXPORT_NONE) {
                                /* nothing to do here */
                                peer->reconf_out = 0;
-                       } else if (peer->conf.export_type ==
-                           EXPORT_DEFAULT_ROUTE) {
+                       } else if (peer->export_type == EXPORT_DEFAULT_ROUTE) {
                                /* just resend the default route */
                                for (aid = 0; aid < AID_MAX; aid++) {
                                        if (peer->capa.mp[aid])
@@ -3739,7 +3762,7 @@ rde_as4byte(struct rde_peer *peer)
 static int
 rde_no_as_set(struct rde_peer *peer)
 {
-       return (peer->conf.flags & PEERFLAG_NO_AS_SET);
+       return (peer->flags & PEERFLAG_NO_AS_SET);
 }
 
 /* End-of-RIB marker, RFC 4724 */

diff --git a/usr.sbin/bgpd/rde.h b/usr.sbin/bgpd/rde.h
index df20239..1fad1c4 100644 (file)
--- a/usr.sbin/bgpd/rde.h
+++ b/usr.sbin/bgpd/rde.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: rde.h,v 1.237 2021/03/02 09:45:07 claudio Exp $ */
+/*     $OpenBSD: rde.h,v 1.238 2021/05/06 09:18:54 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Claudio Jeker <claudio@openbsd.org> and
@@ -103,12 +103,14 @@ struct rde_peer {
        u_int32_t                        up_nlricnt;
        u_int32_t                        up_wcnt;
        enum peer_state                  state;
+       enum export_type                 export_type;
        u_int16_t                        loc_rib_id;
        u_int16_t                        short_as;
        u_int16_t                        mrt_idx;
        u_int8_t                         reconf_out;    /* out filter changed */
        u_int8_t                         reconf_rib;    /* rib changed */
        u_int8_t                         throttled;
+       u_int8_t                         flags;
 };
 
 #define AS_SET                 1

diff --git a/usr.sbin/bgpd/rde_peer.c b/usr.sbin/bgpd/rde_peer.c
index 8f9dd05..1bbebf9 100644 (file)
--- a/usr.sbin/bgpd/rde_peer.c
+++ b/usr.sbin/bgpd/rde_peer.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: rde_peer.c,v 1.6 2020/12/04 11:57:13 claudio Exp $ */
+/*     $OpenBSD: rde_peer.c,v 1.7 2021/05/06 09:18:54 claudio Exp $ */
 
 /*
  * Copyright (c) 2019 Claudio Jeker <claudio@openbsd.org>
@@ -170,6 +170,8 @@ peer_add(u_int32_t id, struct peer_config *p_conf)
        if (peer->loc_rib_id == RIB_NOTFOUND)
                fatalx("King Bula's new peer met an unknown RIB");
        peer->state = PEER_NONE;
+       peer->export_type = peer->conf.export_type;
+       peer->flags = peer->conf.flags;
        SIMPLEQ_INIT(&peer->imsg_queue);
 
        head = PEER_HASH(id);
@@ -429,11 +431,11 @@ peer_stale(struct rde_peer *peer, u_int8_t aid)
 void
 peer_dump(struct rde_peer *peer, u_int8_t aid)
 {
-       if (peer->conf.export_type == EXPORT_NONE) {
+       if (peer->export_type == EXPORT_NONE) {
                /* nothing to send apart from the marker */
                if (peer->capa.grestart.restart)
                        prefix_add_eor(peer, aid);
-       } else if (peer->conf.export_type == EXPORT_DEFAULT_ROUTE) {
+       } else if (peer->export_type == EXPORT_DEFAULT_ROUTE) {
                up_generate_default(out_rules, peer, aid);
                rde_up_dump_done(peer, aid);
        } else {

diff --git a/usr.sbin/bgpd/rde_update.c b/usr.sbin/bgpd/rde_update.c
index 0d7193a..8910ca0 100644 (file)
--- a/usr.sbin/bgpd/rde_update.c
+++ b/usr.sbin/bgpd/rde_update.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: rde_update.c,v 1.126 2021/04/20 11:19:56 claudio Exp $ */
+/*     $OpenBSD: rde_update.c,v 1.127 2021/05/06 09:18:54 claudio Exp $ */
 
 /*
  * Copyright (c) 2004 Claudio Jeker <claudio@openbsd.org>
@@ -135,7 +135,7 @@ again:
                 * skip the filters.
                 */
                if (need_withdraw &&
-                   !(peer->conf.flags & PEERFLAG_EVALUATE_ALL)) {
+                   !(peer->flags & PEERFLAG_EVALUATE_ALL)) {
                        new = NULL;
                        goto again;
                }
@@ -148,7 +148,7 @@ again:
                    new->pt->prefixlen, prefix_vstate(new), &state) ==
                    ACTION_DENY) {
                        rde_filterstate_clean(&state);
-                       if (peer->conf.flags & PEERFLAG_EVALUATE_ALL)
+                       if (peer->flags & PEERFLAG_EVALUATE_ALL)
                                new = LIST_NEXT(new, entry.list.rib);
                        else
                                new = NULL;
@@ -360,7 +360,7 @@ up_generate_attr(u_char *buf, int len, struct rde_peer *peer,
                        break;
                case ATTR_ASPATH:
                        if (!peer->conf.ebgp ||
-                           peer->conf.flags & PEERFLAG_TRANS_AS)
+                           peer->flags & PEERFLAG_TRANS_AS)
                                pdata = aspath_prepend(asp->aspath,
                                    peer->conf.local_as, 0, &plen);
                        else
@@ -399,7 +399,7 @@ up_generate_attr(u_char *buf, int len, struct rde_peer *peer,
                         */
                        if (asp->flags & F_ATTR_MED && (!peer->conf.ebgp ||
                            asp->flags & F_ATTR_MED_ANNOUNCE ||
-                           peer->conf.flags & PEERFLAG_TRANS_AS)) {
+                           peer->flags & PEERFLAG_TRANS_AS)) {
                                tmp32 = htonl(asp->med);
                                if ((r = attr_write(buf + wlen, len,
                                    ATTR_OPTIONAL, ATTR_MED, &tmp32, 4)) == -1)
@@ -439,7 +439,7 @@ up_generate_attr(u_char *buf, int len, struct rde_peer *peer,
                case ATTR_AS4_PATH:
                        if (neednewpath) {
                                if (!peer->conf.ebgp ||
-                                   peer->conf.flags & PEERFLAG_TRANS_AS)
+                                   peer->flags & PEERFLAG_TRANS_AS)
                                        pdata = aspath_prepend(asp->aspath,
                                        peer->conf.local_as, 0, &plen);
                                else

diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c
index e264943..b8b6b6d 100644 (file)
--- a/usr.sbin/bgpd/session.c
+++ b/usr.sbin/bgpd/session.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: session.c,v 1.413 2021/05/03 14:08:09 claudio Exp $ */
+/*     $OpenBSD: session.c,v 1.414 2021/05/06 09:18:54 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004, 2005 Henning Brauer <henning@openbsd.org>
@@ -2734,10 +2734,24 @@ session_dispatch_imsg(struct imsgbuf *ibuf, int idx, u_int *listener_cnt)
                        }
                        break;
                case IMSG_RECONF_DRAIN:
-                       if (idx != PFD_PIPE_MAIN)
-                               fatalx("reconf request not from parent");
-                       imsg_compose(ibuf_main, IMSG_RECONF_DRAIN, 0, 0,
-                           -1, NULL, 0);
+                       switch (idx) {
+                       case PFD_PIPE_ROUTE:
+                               if (nconf != NULL)
+                                       fatalx("got unexpected %s from RDE",
+                                           "IMSG_RECONF_DONE");
+                               imsg_compose(ibuf_main, IMSG_RECONF_DONE, 0, 0,
+                                   -1, NULL, 0);
+                               break;
+                       case PFD_PIPE_MAIN:
+                               if (nconf == NULL)
+                                       fatalx("got unexpected %s from parent",
+                                           "IMSG_RECONF_DONE");
+                               imsg_compose(ibuf_main, IMSG_RECONF_DRAIN, 0, 0,
+                                   -1, NULL, 0);
+                               break;
+                       default:
+                               fatalx("reconf request not from parent or RDE");
+                       }
                        break;
                case IMSG_RECONF_DONE:
                        if (idx != PFD_PIPE_MAIN)
@@ -2771,8 +2785,10 @@ session_dispatch_imsg(struct imsgbuf *ibuf, int idx, u_int *listener_cnt)
                        nconf = NULL;
                        pending_reconf = 0;
                        log_info("SE reconfigured");
-                       imsg_compose(ibuf_main, IMSG_RECONF_DONE, 0, 0,
-                           -1, NULL, 0);
+                       /*
+                        * IMSG_RECONF_DONE is sent when the RDE drained
+                        * the peer config sent in merge_peers().
+                        */
                        break;
                case IMSG_IFINFO:
                        if (idx != PFD_PIPE_MAIN)
@@ -3343,6 +3359,9 @@ merge_peers(struct bgpd_config *c, struct bgpd_config *nc)
                }
        }
 
+       if (imsg_rde(IMSG_RECONF_DRAIN, 0, NULL, 0) == -1)
+               fatalx("imsg_compose error");
+
        /* pfkeys of new peers already loaded by the parent process */
        RB_FOREACH_SAFE(np, peer_head, &nc->peers, next) {
                RB_REMOVE(peer_head, &nc->peers, np);