On Tue, Jan 17, 2023 at 03:11:26PM +0100, Claudio Jeker wrote:
> This diff adds all the plumbing to push the ASPA table from the RTR
> process into the RDE. It is still missing important bits but the table
> itself should load and `bgpctl show sets` will show what was loaded.
>
> After that the reload logic needs to be added and the filters need to
> be extended to check the ASPA validation state.
Please commit the diff at the end to fix regress when you land this
(needed because of the new getmonotime() call in aspa_table_prep()).
This generally reads fine. There is a crash in rde_aspa_reload(),
see comments inline. If you agree with my suggested fix, this is ok tb
> Index: bgpd/rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.586
> diff -u -p -r1.586 rde.c
> --- bgpd/rde.c 16 Jan 2023 10:37:08 -0000 1.586
> +++ bgpd/rde.c 17 Jan 2023 10:35:14 -0000
> @@ -83,6 +83,7 @@ static void rde_softreconfig_sync_reeva
> static void rde_softreconfig_sync_fib(struct rib_entry *, void *);
> static void rde_softreconfig_sync_done(void *, uint8_t);
> static void rde_roa_reload(void);
> +static void rde_aspa_reload(void);
> int rde_update_queue_pending(void);
> void rde_update_queue_runner(void);
> void rde_update6_queue_runner(uint8_t);
> @@ -109,7 +110,7 @@ static struct imsgbuf *ibuf_rtr;
> static struct imsgbuf *ibuf_main;
> static struct bgpd_config *conf, *nconf;
> static struct rde_prefixset rde_roa, roa_new;
> -static struct rde_aspa *rde_aspa /* , *aspa_new */;
> +static struct rde_aspa *rde_aspa, *aspa_new;
>
> volatile sig_atomic_t rde_quit = 0;
> struct filter_head *out_rules, *out_rules_tmp;
> @@ -641,6 +642,15 @@ badnetdel:
> imsg_compose(ibuf_se_ctl, IMSG_CTL_SHOW_SET, 0,
> imsg.hdr.pid, -1, &cset, sizeof(cset));
>
> + memset(&cset, 0, sizeof(cset));
> + cset.type = ASPA_SET;
> + strlcpy(cset.name, "RPKI ASPA", sizeof(cset.name));
> + aspa_table_stats(rde_aspa, &cset);
> +
> + imsg_compose(ibuf_se_ctl, IMSG_CTL_SHOW_SET, 0,
> + imsg.hdr.pid, -1, &cset, sizeof(cset));
> +
The line above is extra and adds trailing tabs
> +
> SIMPLEQ_FOREACH(aset, &conf->as_sets, entry) {
> memset(&cset, 0, sizeof(cset));
> cset.type = ASNUM_SET;
> @@ -1065,9 +1075,11 @@ rde_dispatch_imsg_parent(struct imsgbuf
> void
> rde_dispatch_imsg_rtr(struct imsgbuf *ibuf)
> {
> - struct imsg imsg;
> - struct roa roa;
> - int n;
> + static struct aspa_set *aspa;
> + struct imsg imsg;
> + struct roa roa;
> + struct aspa_prep ap;
> + int n;
>
> while (ibuf) {
> if ((n = imsg_get(ibuf, &imsg)) == -1)
> @@ -1094,9 +1106,66 @@ rde_dispatch_imsg_rtr(struct imsgbuf *ib
> log_addr(&p), roa.prefixlen);
> }
> break;
> + case IMSG_RECONF_ASPA_PREP:
> + if (imsg.hdr.len - IMSG_HEADER_SIZE !=
> + sizeof(ap))
Could unwrap this line
[...]
> @@ -3972,6 +4042,32 @@ rde_roa_reload(void)
> rib_byid(RIB_ADJ_IN), rde_roa_softreload,
> rde_roa_softreload_done, NULL) == -1)
> fatal("%s: rib_dump_new", __func__);
> +}
> +
> +static void
> +rde_aspa_reload(void)
> +{
> + struct rde_aspa *aspa_old;
> +
> + if (aspa_update_pending) {
> + log_info("ASPA softreload skipped, old still running");
> + return;
> + }
> +
> + aspa_old = rde_aspa;
> + rde_aspa = aspa_new;
> + aspa_new = NULL;
> +
> + /* check if aspa changed */
> + if (aspa_table_equal(rde_aspa, aspa_old)) {
The integration test crashes because rde_aspa == NULL && aspa_old == NULL.
A possible fix is to add
if (ra == NULL || old == NULL)
return;
to aspa_table_unchanged().
> + aspa_table_unchanged(rde_aspa, aspa_old);
> + aspa_table_free(aspa_old); /* old aspa no longer needed */
> + return;
> + }
> +
Trailing tab
> + aspa_table_free(aspa_old); /* old aspa no longer needed */
> + log_debug("ASPA change: reloading Adj-RIB-In");
> + /* XXX MISSING */
> }
>
> /*
[...]
> Index: bgpd/rde_aspa.c
> ===================================================================
[...]
> +/*
> + * Return true if the two rde_aspa tables are contain the same data.
> + */
> +int
> +aspa_table_equal(const struct rde_aspa *ra, const struct rde_aspa *rb)
> +{
> + uint32_t i;
> +
> + /* allow NULL pointers to be passed */
8 spaces -> tab
> + if (ra == NULL && rb == NULL)
> + return 1;
> + if (ra == NULL || rb == NULL)
> + return 0;
> +
> + if (ra->maxset != rb->maxset ||
> + ra->maxdata != rb->maxdata)
> + return 0;
> + for (i = 0; i < ra->maxset; i++)
> + if (ra->sets[i].as != rb->sets[i].as)
> + return 0;
> + if (memcmp(ra->data, rb->data, ra->maxdata * sizeof(ra->data[0])) != 0)
> + return 0;
> +
> + return 1;
> +}
> +
> +void
> +aspa_table_unchanged(struct rde_aspa *ra, const struct rde_aspa *old)
> +{
As mentioned above, you may want to add
if (ra == NULL || old == NULL)
return;
> + ra->lastchange = old->lastchange;
> }
Index: unittests/Makefile
===================================================================
RCS file: /cvs/src/regress/usr.sbin/bgpd/unittests/Makefile,v
retrieving revision 1.10
diff -u -p -r1.10 Makefile
--- unittests/Makefile 11 Jan 2023 13:55:08 -0000 1.10
+++ unittests/Makefile 17 Jan 2023 14:56:11 -0000
@@ -39,4 +39,6 @@ SRCS_rde_community_test= rde_community_t
SRCS_rde_decide_test= rde_decide_test.c rde_decide.c rde_attr.c util.c
+SRCS_rde_aspa_test+= rde_aspa_test.c timer.c
+
.include <bsd.regress.mk>
