From d0b311ebccba0a7a38a4f0386dfa19c1a7395c21 Mon Sep 17 00:00:00 2001 From: claudio Date: Thu, 6 May 2021 09:18:54 +0000 Subject: [PATCH] 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@ --- usr.sbin/bgpd/rde.c | 63 ++++++++++++++++++++++++++------------ usr.sbin/bgpd/rde.h | 4 ++- usr.sbin/bgpd/rde_peer.c | 8 +++-- usr.sbin/bgpd/rde_update.c | 12 ++++---- usr.sbin/bgpd/session.c | 33 +++++++++++++++----- 5 files changed, 83 insertions(+), 37 deletions(-) diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c index bb3be1adb36..0c36005cff7 100644 --- 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 @@ -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 df20239c164..1fad1c41fdc 100644 --- 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 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 8f9dd053836..1bbebf93c48 100644 --- 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 @@ -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 0d7193af0d1..8910ca0db63 100644 --- 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 @@ -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 e2649432b9b..b8b6b6dc11b 100644 --- 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 @@ -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); -- 2.20.1