Claudio Jeker([email protected]) on 2021.05.05 14:20:58 +0200:
> The peer flags (mainly rde evaluate all but also transparent-as) and the
> export options (none, default) are not properly handled on a config
> reload. In both cases a full session restart is needed after the config
> reload (with a bit of extra wait time to ensure that the peer config is
> actually up to date).
>
> The following diff should fix this.
>
> Add an extra reload barrier (IMSG_RECONF_DRAIN) to the sync of the peer
> config from the session engine to the rde. This way it can be ensured that
> the peer config is up to date in the RDE before hitting reconfiguration
> function.
>
> 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.
> This way the RDE is able to notice that a value has changed during reload.
> Flush the Adj-RIB-Out for a peer where either the export_type changed or
> where rde evaluate or transparent-as is changed. After the flush the RIB
> is rebuilt in a 2nd step.
>
> Fix multiple issues in the rde_softreconfig_in_done handler that resulted
> in multiple runs of the out stage of the softreconfig pipeline.
>
> OK?
yes, ok.
> --
> :wq Claudio
>
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.519
> diff -u -p -r1.519 rde.c
> --- rde.c 27 Apr 2021 09:07:10 -0000 1.519
> +++ rde.c 5 May 2021 12:04:55 -0000
> @@ -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_in
> 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, st
> 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, st
> 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_in
> 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_in
> 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 */
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.237
> diff -u -p -r1.237 rde.h
> --- rde.h 2 Mar 2021 09:45:07 -0000 1.237
> +++ rde.h 8 Mar 2021 11:41:28 -0000
> @@ -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
> Index: rde_peer.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 rde_peer.c
> --- rde_peer.c 4 Dec 2020 11:57:13 -0000 1.6
> +++ rde_peer.c 5 May 2021 09:26:26 -0000
> @@ -170,6 +170,8 @@ peer_add(u_int32_t id, struct peer_confi
> 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
> 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 {
> Index: rde_update.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> retrieving revision 1.126
> diff -u -p -r1.126 rde_update.c
> --- rde_update.c 20 Apr 2021 11:19:56 -0000 1.126
> +++ rde_update.c 5 May 2021 09:29:34 -0000
> @@ -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, s
> 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, s
> */
> 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, s
> 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
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.413
> diff -u -p -r1.413 session.c
> --- session.c 3 May 2021 14:08:09 -0000 1.413
> +++ session.c 5 May 2021 09:13:41 -0000
> @@ -2734,10 +2734,24 @@ session_dispatch_imsg(struct imsgbuf *ib
> }
> 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 *ib
> 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)
> @@ -3342,6 +3358,9 @@ merge_peers(struct bgpd_config *c, struc
> }
> }
> }
> +
> + 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) {
>
