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@

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 <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 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 <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 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 <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 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 <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 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 <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);