On Sun, Sep 30, 2018 at 01:49:45AM +0000, Job Snijders wrote:
> Dear all,
>
> This small patch exposes the origin validation state in 'bgpctl show
> rib' and 'bgpctl show rib detail'. This will help debugging, and draw
> attention to routing problems.
>
> I know we're weary of spending horizontal space, but I think spending 3
> chars to show the OV state (and as such make it a first class citizen)
> will be worth it. When I tried putting the ovs state in the 'flags'
> column it just didn't look right.
While I agree that mixing the flags and ovs together is not ideal but I
wonder if we can only spend 1 char instead of the 3 (two spaces and the
ovs state). E.g. by reducing the header for 'flags'
Also the 2nd detail line is now getting rather long, wonder if we should
split it up.
The diff is OK claudio@ (the output can be fixed in tree if there is a
good proposal).
> Example output:
>
> $ bgpctl show rib
> flags: * = Valid, > = Selected, I = via IBGP, A = Announced,
> S = Stale, E = Error
> origin validation state: N = not-found, V = valid, ! = invalid
> origin: i = IGP, e = EGP, ? = Incomplete
>
> flags ovs destination gateway lpref med aspath origin
> *> V 192.0.2.0/24 100.64.2.3 100 0 2 i
> *> ! 192.0.2.0/26 100.64.2.3 100 0 2 i
> $ bgpctl show rib detail
>
> BGP routing table entry for 192.0.2.0/24
> 2
> Nexthop 100.64.2.3 (via 100.64.2.3) from 100.64.2.3 (192.147.168.1)
> Origin IGP, metric 0, localpref 100, weight 0, ovs valid, external,
> valid, best
> Last update: 00:02:19 ago
>
> BGP routing table entry for 192.0.2.0/26
> 2
> Nexthop 100.64.2.3 (via 100.64.2.3) from 100.64.2.3 (192.147.168.1)
> Origin IGP, metric 0, localpref 100, weight 0, ovs invalid, external,
> valid, best
> Last update: 00:02:19 ago
>
> OK?
>
> Kind regards,
>
> Job
>
> diff --git usr.sbin/bgpctl/bgpctl.c usr.sbin/bgpctl/bgpctl.c
> index 7275ea6a2c4..70648ed8f5b 100644
> --- usr.sbin/bgpctl/bgpctl.c
> +++ usr.sbin/bgpctl/bgpctl.c
> @@ -72,8 +72,9 @@ int show_nexthop_msg(struct imsg *);
> void show_interface_head(void);
> int show_interface_msg(struct imsg *);
> void show_rib_summary_head(void);
> -void print_prefix(struct bgpd_addr *, u_int8_t, u_int8_t);
> +void print_prefix(struct bgpd_addr *, u_int8_t, u_int8_t, u_int8_t);
> const char * print_origin(u_int8_t, int);
> +const char * print_ovs(u_int8_t, int);
> void print_flags(u_int8_t, int);
> int show_rib_summary_msg(struct imsg *);
> int show_rib_detail_msg(struct imsg *, int, int);
> @@ -183,6 +184,7 @@ main(int argc, char *argv[])
> ribreq.neighbor = neighbor;
> ribreq.aid = res->aid;
> ribreq.flags = res->flags;
> + ribreq.validation_state = res->validation_state;
> show_mrt.arg = &ribreq;
> if (!(res->flags & F_CTL_DETAIL))
> show_rib_summary_head();
> @@ -1183,17 +1185,20 @@ show_rib_summary_head(void)
> {
> printf("flags: * = Valid, > = Selected, I = via IBGP, A = Announced,\n"
> " S = Stale, E = Error\n");
> + printf("origin validation state: N = not-found, V = valid, ! =
> invalid\n");
> printf("origin: i = IGP, e = EGP, ? = Incomplete\n\n");
> - printf("%-5s %-20s %-15s %5s %5s %s\n", "flags", "destination",
> + printf("%-5s %3s %-20s %-15s %5s %5s %s\n", "flags", "ovs",
> "destination",
> "gateway", "lpref", "med", "aspath origin");
> }
>
> void
> -print_prefix(struct bgpd_addr *prefix, u_int8_t prefixlen, u_int8_t flags)
> +print_prefix(struct bgpd_addr *prefix, u_int8_t prefixlen, u_int8_t flags,
> + u_int8_t ovs)
> {
> char *p;
>
> print_flags(flags, 1);
> + printf("%3s ", print_ovs(ovs, 1));
> if (asprintf(&p, "%s/%u", log_addr(prefix), prefixlen) == -1)
> err(1, NULL);
> printf("%-20s", p);
> @@ -1252,6 +1257,19 @@ print_flags(u_int8_t flags, int sum)
> }
> }
>
> +const char *
> +print_ovs(u_int8_t validation_state, int sum)
> +{
> + switch (validation_state) {
> + case ROA_INVALID:
> + return (sum ? "!" : "invalid");
> + case ROA_VALID:
> + return (sum ? "V" : "valid");
> + default:
> + return (sum ? "N" : "not-found");
> + }
> +}
> +
> int
> show_rib_summary_msg(struct imsg *imsg)
> {
> @@ -1309,7 +1327,7 @@ show_rib_brief(struct ctl_show_rib *r, u_char *asdata)
> {
> char *aspath;
>
> - print_prefix(&r->prefix, r->prefixlen, r->flags);
> + print_prefix(&r->prefix, r->prefixlen, r->flags, r->validation_state);
> printf(" %-15s ", log_addr(&r->exit_nexthop));
> printf(" %5u %5u ", r->local_pref, r->med);
>
> @@ -1346,8 +1364,9 @@ show_rib_detail(struct ctl_show_rib *r, u_char *asdata,
> int nodescr, int flag0)
> id.s_addr = htonl(r->remote_id);
> printf("%s)%c", inet_ntoa(id), EOL0(flag0));
>
> - printf(" Origin %s, metric %u, localpref %u, weight %u, ",
> - print_origin(r->origin, 0), r->med, r->local_pref, r->weight);
> + printf(" Origin %s, metric %u, localpref %u, weight %u, ovs %s, ",
> + print_origin(r->origin, 0), r->med, r->local_pref, r->weight,
> + print_ovs(r->validation_state, 0));
> print_flags(r->flags, 0);
>
> now = time(NULL);
> diff --git usr.sbin/bgpctl/parser.h usr.sbin/bgpctl/parser.h
> index fd4847b199c..fd4e96f6efa 100644
> --- usr.sbin/bgpctl/parser.h
> +++ usr.sbin/bgpctl/parser.h
> @@ -70,6 +70,7 @@ struct parse_result {
> char shutcomm[SHUT_COMM_LEN];
> char *irr_outdir;
> int flags;
> + u_int8_t validation_state;
> u_int rtableid;
> enum actions action;
> u_int8_t prefixlen;
> diff --git usr.sbin/bgpd/bgpd.h usr.sbin/bgpd/bgpd.h
> index 8e6a4bcb2c7..2c4ba61d503 100644
> --- usr.sbin/bgpd/bgpd.h
> +++ usr.sbin/bgpd/bgpd.h
> @@ -89,6 +89,15 @@
> #define F_CTL_SSV 0x20000 /* only used by bgpctl */
> #define F_CTL_INVALID 0x40000 /* only used by bgpctl */
>
> +/*
> + * Note that these numeric assignments differ from the numbers commonly
> + * used in route origin validation context.
> + */
> +#define ROA_NOTFOUND 0x0 /* default */
> +#define ROA_INVALID 0x1
> +#define ROA_VALID 0x2
> +#define ROA_MASK 0x3
> +
> /*
> * Limit the number of messages queued in the session engine.
> * The SE will send an IMSG_XOFF messages to the RDE if the high water mark
> @@ -668,6 +677,7 @@ struct ctl_show_rib {
> u_int16_t aspath_len;
> u_int8_t prefixlen;
> u_int8_t origin;
> + u_int8_t validation_state;
> /* plus a aspath_len bytes long aspath */
> };
>
> @@ -768,6 +778,7 @@ struct ctl_show_rib_request {
> struct filter_largecommunity large_community;
> u_int32_t peerid;
> u_int32_t flags;
> + u_int8_t validation_state;
> pid_t pid;
> enum imsg_type type;
> u_int8_t prefixlen;
> diff --git usr.sbin/bgpd/rde.c usr.sbin/bgpd/rde.c
> index 8c4e2bdf017..871e419f53a 100644
> --- usr.sbin/bgpd/rde.c
> +++ usr.sbin/bgpd/rde.c
> @@ -2177,6 +2177,7 @@ rde_dump_rib_as(struct prefix *p, struct rde_aspath
> *asp, pid_t pid, int flags)
> pt_getaddr(p->re->prefix, &rib.prefix);
> rib.prefixlen = p->re->prefix->prefixlen;
> rib.origin = asp->origin;
> + rib.validation_state = p->validation_state;
> rib.flags = 0;
> if (p->re->active == p)
> rib.flags |= F_PREF_ACTIVE;
> diff --git usr.sbin/bgpd/rde.h usr.sbin/bgpd/rde.h
> index 69962d83585..76e78865e76 100644
> --- usr.sbin/bgpd/rde.h
> +++ usr.sbin/bgpd/rde.h
> @@ -36,11 +36,6 @@ enum peer_state {
> PEER_ERR /* error occurred going to PEER_DOWN state */
> };
>
> -#define ROA_NOTFOUND 0x0 /* default */
> -#define ROA_INVALID 0x1
> -#define ROA_VALID 0x2
> -#define ROA_MASK 0x3
> -
> /*
> * How do we identify peers between the session handler and the rde?
> * Currently I assume that we can do that with the neighbor_ip...
>
--
:wq Claudio
